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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

a few more comments below, but this is already shaping up nicely!

On June 17, 2026 2:42 pm, 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 bunch of parsing edge cases.
> 
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/Certificate.pm |  25 +++-
>  test/Makefile          |   2 +
>  test/check_pem_test.pl | 332 +++++++++++++++++++++++++++++++++++++++++
>  test/split_pem_test.pl | 272 +++++++++++++++++++++++++++++++++
>  4 files changed, 625 insertions(+), 6 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..387405b 100644
> --- a/src/PVE/Certificate.pm
> +++ b/src/PVE/Certificate.pm
> @@ -134,23 +134,36 @@ 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');
> +    

nit: trailing whitespace here (the blank line is not actually empty)

there's a few occurences of this below as well, I can really recommend
making your editor and/or git flag those ;) sometimes trailing
whitespace (e.g. in test data) can be okay, but usually it's a sign
something is off formatting-wise.

> +    return $content =~ /(.*?$footer)/sg;
>  }
>  
> +# 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 = undef;

just

my $result_pem;

is enough, variables start out undefined.

> +    if($opts{multiple}) {

nit: missing space after "if"

> +        my @split = split_pem($content, label=>$label);

you probably want to check for an empty list here and die with and
error/return undef depending on whether noerr is set

> +
> +        $result_pem = eval {
> +            join("", map { check_pem($_, label=>$label ) } @split)
> +        };

you can move the $result_pem inside the eval, makes for easier reading
IMHO, but you don't even need that.

if you pass all the options (except multiple) to the recursive call,
then check_pem will
- die if noerr is not set, which is okay (no need for eval to catch it)
- return undef if noerr is set, which you can then handle here

this probably means it's more readable to not use map, but use a proper
loop instead.

> +    } else {
> +        my $re = $pem_re->($label);
> +        $result_pem = $content if $content =~ /^$re$/;
> +    }
>  
> -    return $content if $content =~ /^$re$/; # OK
> +    return $result_pem if defined($result_pem);
>  
>      return undef if $opts{noerr};
>      die "not a valid PEM-formatted string.\n";
> 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 \
>  
>  all:
>  
> diff --git a/test/check_pem_test.pl b/test/check_pem_test.pl
> new file mode 100755
> index 0000000..10ef295
> --- /dev/null
> +++ b/test/check_pem_test.pl
> @@ -0,0 +1,332 @@
> +#!/usr/bin/perl
> +# Tests the PVE::Certificate::check_pem function for 
> +# correctness and coverage of edgecases.
> +
> +use lib '../src';
> +
> +use Test::More;
> +
> +use PVE::Certificate;
> +
> +# Arrange
> +my $setup = [

> [..]

> +    {
> +        successful => 1,

I'd call this `expected_success` or `expect_success`, to make it easy to
understand even at a glance. successful could also be referring to the
actual result, not the expectation.

> +        name => "single as multiple pem",
> +        multiple => 1,
> +        pem => <<'EOF',
> +-----BEGIN CERTIFICATE-----
> +1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
> +dmFpdXdlYmdtheseZ2RmaXVwYWJ2aXV1YXdlYmxmamlraWprZmdyYXNkaHJmaW9h
> +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
> +c2JlZ2tseTxzZWl2IGF3ZWlidm4=
> +-----END CERTIFICATE-----

we could probably also add a case here for single PEM with comment and
multiple=>1, for completeness' sake

thanks for adding test cases!

> +EOF
> +    }
> +];
> +
> +for my $data ($setup->@*) {

you could treat each iteration here as a subtest

> +    # Act
> +    my $check_result = PVE::Certificate::check_pem(
> +        $data->{pem}, 
> +        noerr => 1, 
> +        multiple => $data->{multiple},
> +        label => $data->{label},
> +    );
> +
> +    # Assert
> +    if ($data->{successful}) {
> +        ok($check_result, $data->{name} . " is successful");
> +    } else {
> +        ok(!$check_result, $data->{name} . " correctly rejected");
> +    }
> +
> +    my $expected_result = $data->{expected_result}
> +        ? $data->{expected_result}
> +        : $data->{pem};
> +
> +    if ($data->{successful}) {
> +        ok($check_result eq $expected_result,  $data->{name} . " has correct check output");

if you use `is` here, you get nice output in case of a mismatch

> +    } else {
> +        ok(!defined($check_result),  $data->{name} . " has correct check output");

this one here and the else branch above test the same thing, except that
the first variant above is slightly too coarse. I'd move the defined up,
and drop the else branch here. and then the whole test could be
restructured:

if ($successful) {
  1. check return value is defined
  2. construct expected_result
  3. check result is expected_result
} else {
  1. check return value is undef
}

> +    }
> +}
> +
> +done_testing();
> diff --git a/test/split_pem_test.pl b/test/split_pem_test.pl
> new file mode 100755
> index 0000000..f7f7bc0
> --- /dev/null
> +++ b/test/split_pem_test.pl
> @@ -0,0 +1,272 @@
> +#!/usr/bin/perl
> +# Tests the PVE::Certificate::split_pem function for 
> +# correctness and coverage of edgecases.
> +
> +use lib '../src';
> +
> +use Test::More;
> +
> +use PVE::Certificate;
> +
> +# Arrange
> +my $setup = [

> [..]

> +];
> +
> +for my $data ($setup->@*) {

same here as above

> +    # Act
> +    my @split_result = PVE::Certificate::split_pem(
> +        $data->{pem},
> +        label => $data->{label}
> +    );
> +
> +    # Assert
> +    if ($data->{successful}) {
> +        ok(@split_result, $data->{name} . " is successful");
> +    } else {
> +        ok(!@split_result, $data->{name} . " correctly rejected");       

nit: trailing whitespace here as well

I think here it would make sense to explicitly check for empty lists
being returned, that would also allow us to drop the else branch below
same as for the check_pem test.

> +    }
> +
> +    my $expected_result = $data->{expected_result} 
> +        ? $data->{expected_result} 
> +        : [ $data->{pem} ];
> +
> +    if ($data->{successful}) {
> +        ok(@split_result eq @$expected_result,  $data->{name} . " has correct check output");
> +    } else {
> +        ok(!@split_result eq @$expected_result,  $data->{name} . " has correct check output");
> +    }

the checks here are wrong:

1. the inversion of `eq` is `ne` ;)

perl is tricky/special here as well - you have `==` and `!=` which
compare "numerically", and `eq` and `ne` which compare "stringwise".
because of coercion happening behind your back there can be all sorts of
funny side-effects, sometimes practical, sometimes confusing.

e.g., comparing

$a = 5;

with `eq "5"` is true, as is `== 5`.

but comparing

$a = "5foo"

with `eq "5"` is obviously false, while `== 5` will print a warning *and
evaluate to true*

similarly, empty lists , arrays and hashes vs. definedness checks vs.
explicit scalar conversions can be tricky. when in doubt, be explicit,
and add a call to `scalar(..)` or `defined(..)` instead of relying on
easy-to-get-wrong semantics/side-effects.

2. you cannot use `eq` to compare arrays like you want here

but - Test::More comes with `is_deeply`, which has the added benefit of
giving nice output in case of broken tests, e.g.:

not ok 6 - leading and interleaved text pem has correct check output
#   Failed test 'leading and interleaved text pem has correct check output'
#   at ./split_pem_test.pl line 267.
#     Structures begin differing at:
#          $got->[0] = 'Subject: CN=some.test.content
#     Issuer: CN=Org Issuing CA 3
#     -----BEGIN CERTIFICATE-----
#     1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
#     dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
#     dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
#     c2JlZ2tseTxzZWl2IGF3ZWlidm4=
#     -----END CERTIFICATE-----
#     '
#     $expected->[0] = '-----BEGIN CERTIFICATE-----
#     1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
#     dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
#     dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
#     c2JlZ2tseTxzZWl2IGF3ZWlidm4=
#     -----END CERTIFICATE-----
#     '



> +}
> +
> +done_testing();
> -- 
> 2.47.3
> 
> 
> 
> 
> 
>