Re: [PATCH common v3 1/2] fix #5978: pem parser: relax parsing of chain entries

["Elias Huhsovitz" <e.huhsovitz@proxmox.com>]

On Fri Jul 3, 2026 at 12:51 PM CEST, Thomas Ellmenreich wrote:
> Relaxes the parser to allow for text and whitespaces inbetween certchain
> entries. The splitting of PEM chains was also reworked to split each entry at
> its end, grouping it with its leading text.
>
> Added testsuite to cover a number of parsing edge cases.
>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>

Summary
-------
* Patch needs a rebase onto master, in order to be applied using b4 pain
free.

* Consider splitting tests into separate patch. This allows reviewing
  the bug-fix and tests separately

* die-path of check_pem is not tested

* use iteration instead of recursion in check_pem

* improve test names

See more comments inline

> ---
>  src/PVE/Certificate.pm |  37 ++++-
>  test/Makefile          |   2 +
>  test/check_pem_test.pl | 357 +++++++++++++++++++++++++++++++++++++++++
>  test/split_pem_test.pl | 279 ++++++++++++++++++++++++++++++++
>  4 files changed, 667 insertions(+), 8 deletions(-)
>  create mode 100755 test/check_pem_test.pl
>  create mode 100755 test/split_pem_test.pl
>
> diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm
> index b8415e2..e74887c 100644
> --- a/src/PVE/Certificate.pm
> +++ b/src/PVE/Certificate.pm
> @@ -1,5 +1,4 @@
>  package PVE::Certificate;
> -

nit: Why remove this newline?

>  use strict;
>  use warnings;
>  
> @@ -134,26 +133,48 @@ sub strip_leading_text {
>      return $content;
>  }
>  
> +# Splits the pem chain into entries with their leading text
>  sub split_pem {
>      my ($content, %opts) = @_;
> -    my $label = $opts{label} // 'CERTIFICATE';
>  
> -    my $header = $header_re->($label);
> -    return split(/(?=$header)/, $content);
> +    my $footer = $footer_re->($opts{label} // 'CERTIFICATE');
> +
> +    return $content =~ /(.*?$footer)/sg;
>  }

Splitting at the END instead of the BEGIN makes sense. Very nice fix.

>  
> +# Parses the pem or pem chain for complete validity and returns
> +# only the pem/pem chain removing any extra text
>  sub check_pem {
>      my ($content, %opts) = @_;
>  
> +    my $label = $opts{label} // 'CERTIFICATE';
>      $content = strip_leading_text($content);
>  
> -    my $re = $pem_re->($opts{label} // 'CERTIFICATE');
> -    $re = qr/($re\n+)*$re/ if $opts{multiple};
> +    my $result_pem = "";
> +    if (delete $opts{multiple}) {
> +        my @split = split_pem($content, label => $label);
> +
> +        if (!@split) {
> +            return undef if $opts{noerr};
> +            die "pem chain could not be split into separate entries\n";
> +        }

nit: Capitalize Acronyms. e.g.,
die "PEM chain could not be split into separate entries\n";

> +
> +        for my $entry (@split) {
> +            my $entry_pem = check_pem($entry, %opts);
> +
> +            return undef if !$entry_pem;
> +
> +            $result_pem .= $entry_pem;
> +        }
> +    } else {
> +        my $re = $pem_re->($label);
> +        $result_pem = $content if $content =~ /^$re$/;
> +    }
>  
> -    return $content if $content =~ /^$re$/; # OK
> +    return $result_pem if $result_pem;
>  
>      return undef if $opts{noerr};
> -    die "not a valid PEM-formatted string.\n";
> +    die "not a valid PEM-formatted string\n";

This die-path is not included in the test suite.

>  }

Stylistic nit: I like the ingenuity of the recurisve approach, but IMO
this complicates the code unnecessarily. I would go for an iterative
approach, since it is usually less error prone and easier to maintain.
For example it could look like this (not-tested, just for
visualization):


my sub validate_single_pem {
    my ($chunk, $label, $noerr) = @_;

    my $clean = strip_leading_text($chunk);
    my $re = $pem_re->($label);

    # Guard clause: return immediately if valid
    return $clean if $clean =~ /^$re$/;

    # Guard clause: handle error state without deep nesting
    return undef if $noerr;
    die "Not a valid PEM-formatted string\n";
}

sub check_pem {
    my ($content, %opts) = @_;

    my $label = $opts{label} // 'CERTIFICATE';
    my $noerr = $opts{noerr};

    # Treat a single PEM as an array of one chunk, or split if multiple
    my @chunks = $opts{multiple} ? split_pem($content, label => $label) : ($content);

    if ($opts{multiple} && !@chunks) {
        return undef if $noerr;
        die "The PEM chain could not be split into separate entries\n";
    }

    my $result_pem = "";
    for my $chunk (@chunks) {
        my $valid = validate_single_pem($chunk, $label, $noerr);
        
        return $valid if !defined($valid);

        $result_pem .= $valid;
    }

    return $result_pem if $result_pem;

    return undef if $noerr;
    die "Not a valid PEM-formatted string\n";
}

If you think a recursive approach is better suited here, then i would
recommend reducing the level of indenation to indentation. Long complex
recursive functions are usually hard to maintain.

>  
>  sub pem_to_der {
> diff --git a/test/Makefile b/test/Makefile
> index 9b9f81b..8b725c5 100644
> --- a/test/Makefile
> +++ b/test/Makefile
> @@ -14,6 +14,8 @@ TESTS = lock_file.test			\
>  	is_deeply_test.test		\
>  	section_config_property_isolation_test.pl \
>  	file-test.pl \
> +	check_pem_test.pl \
> +	split_pem_test.pl \

This currently causes issues when apllying the patch using b4. Please
rebase onto master.

>  
>  all:
>  
> diff --git a/test/check_pem_test.pl b/test/check_pem_test.pl
> new file mode 100755
> index 0000000..f26a38b
> --- /dev/null
> +++ b/test/check_pem_test.pl
> @@ -0,0 +1,357 @@
> +#!/usr/bin/perl
> +# Tests the PVE::Certificate::check_pem function for
> +# correctness and coverage of edgecases.
> +use strict;
> +use warnings;
> +
> +use lib '../src';
> +
> +use Test::More;
> +
> +use PVE::Certificate;
> +
> +# Arrange
> +my $setup = [
> +    {
> +        expected_success => 1,
> +        name => "full pem",

nit: include success/failure in the name. e.g.,
"full pem succeeds" or "invalid full pem fails with error XYZ"

I know you already set expected_success, but this is easy to miss when
reading the test output.

> +        pem => <<'EOF',
> +-----BEGIN CERTIFICATE-----
> +MIIBsjCCAVugAwIBAgIJAO2g8Z0dXk9tMAoGCCqGSM49BAMCMEUxCzAJBgNVBAYT
> +AlVTMQswCQYDVQQIDAJDQTEQMA4GA1UEBwwHQmVya2VsZXkxEDAOBgNVBAoMB1Rl
> +c3QgQ0EwHhcNMjAwMTAxMDAwMDAwWhcNMzAwMTAxMDAwMDAwWjBFMQswCQYDVQQG
> +EwJVUzELMAkGA1UECAwCQ0ExEDAOBgNVBAcMB0JlcmtlbGV5MRAwDgYDVQQKDAdU
> +ZXN0IENBMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEv5Q8q1p7qZ2gqkQ0Qn5x
> +0n9yqv8n8n7n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8aNT
> +MFEwHQYDVR0OBBYEFOu2Y0bq8v3z7qkq1m1Qwqkq1m1QMB8GA1UdIwQYMBaAFOu2
> +Y0bq8v3z7qkq1m1Qwqkq1m1QMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwID
> +SAAwRQIhANfakefakefakefakefakefakefakefakefake
> +-----END CERTIFICATE-----
> +EOF
> +    },

[snip]

> +    {
> +        expected_success => 1,
> +        name => "many newlines pem",
> +        pem => <<'EOF',
> +-----BEGIN CERTIFICATE-----
> +
> +
> +
> +-----END CERTIFICATE-----
> +EOF
> +    },
> +    {
> +        expected_success => 0,
> +        name => "no content pem",

Same as above, i would prefer a more descriptive name. Something like
"no content pem returns undef"

[snip]