From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id B68291FF0E3 for ; Tue, 04 Aug 2026 12:55:36 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 7489B2162A; Tue, 04 Aug 2026 12:55:36 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 04 Aug 2026 12:55:30 +0200 Message-Id: Subject: Re: [PATCH common v3 1/2] fix #5978: pem parser: relax parsing of chain entries From: "Elias Huhsovitz" To: "Thomas Ellmenreich" , X-Mailer: aerc 0.20.0 References: <20260703105133.77817-1-t.ellmenreich@proxmox.com> <20260703105133.77817-2-t.ellmenreich@proxmox.com> In-Reply-To: <20260703105133.77817-2-t.ellmenreich@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1785840918649 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.112 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: UUYOVES7LT556PFTVRI3MZODP7YOE5UD X-Message-ID-Hash: UUYOVES7LT556PFTVRI3MZODP7YOE5UD X-MailFrom: e.huhsovitz@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 entr= y at > its end, grouping it with its leading text. > > Added testsuite to cover a number of parsing edge cases. > > Signed-off-by: Thomas Ellmenreich 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; > =20 > @@ -134,26 +133,48 @@ sub strip_leading_text { > return $content; > } > =20 > +# Splits the pem chain into entries with their leading text > sub split_pem { > my ($content, %opts) =3D @_; > - my $label =3D $opts{label} // 'CERTIFICATE'; > =20 > - my $header =3D $header_re->($label); > - return split(/(?=3D$header)/, $content); > + my $footer =3D $footer_re->($opts{label} // 'CERTIFICATE'); > + > + return $content =3D~ /(.*?$footer)/sg; > } Splitting at the END instead of the BEGIN makes sense. Very nice fix. > =20 > +# 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) =3D @_; > =20 > + my $label =3D $opts{label} // 'CERTIFICATE'; > $content =3D strip_leading_text($content); > =20 > - my $re =3D $pem_re->($opts{label} // 'CERTIFICATE'); > - $re =3D qr/($re\n+)*$re/ if $opts{multiple}; > + my $result_pem =3D ""; > + if (delete $opts{multiple}) { > + my @split =3D split_pem($content, label =3D> $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 =3D check_pem($entry, %opts); > + > + return undef if !$entry_pem; > + > + $result_pem .=3D $entry_pem; > + } > + } else { > + my $re =3D $pem_re->($label); > + $result_pem =3D $content if $content =3D~ /^$re$/; > + } > =20 > - return $content if $content =3D~ /^$re$/; # OK > + return $result_pem if $result_pem; > =20 > 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) =3D @_; my $clean =3D strip_leading_text($chunk); my $re =3D $pem_re->($label); # Guard clause: return immediately if valid return $clean if $clean =3D~ /^$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) =3D @_; my $label =3D $opts{label} // 'CERTIFICATE'; my $noerr =3D $opts{noerr}; # Treat a single PEM as an array of one chunk, or split if multiple my @chunks =3D $opts{multiple} ? split_pem($content, label =3D> $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 =3D ""; for my $chunk (@chunks) { my $valid =3D validate_single_pem($chunk, $label, $noerr); =20 return $valid if !defined($valid); $result_pem .=3D $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. > =20 > 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 =3D 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. > =20 > all: > =20 > 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 =3D [ > + { > + expected_success =3D> 1, > + name =3D> "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 =3D> <<'EOF', > +-----BEGIN CERTIFICATE----- > +MIIBsjCCAVugAwIBAgIJAO2g8Z0dXk9tMAoGCCqGSM49BAMCMEUxCzAJBgNVBAYT > +AlVTMQswCQYDVQQIDAJDQTEQMA4GA1UEBwwHQmVya2VsZXkxEDAOBgNVBAoMB1Rl > +c3QgQ0EwHhcNMjAwMTAxMDAwMDAwWhcNMzAwMTAxMDAwMDAwWjBFMQswCQYDVQQG > +EwJVUzELMAkGA1UECAwCQ0ExEDAOBgNVBAcMB0JlcmtlbGV5MRAwDgYDVQQKDAdU > +ZXN0IENBMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEv5Q8q1p7qZ2gqkQ0Qn5x > +0n9yqv8n8n7n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8aNT > +MFEwHQYDVR0OBBYEFOu2Y0bq8v3z7qkq1m1Qwqkq1m1QMB8GA1UdIwQYMBaAFOu2 > +Y0bq8v3z7qkq1m1Qwqkq1m1QMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwID > +SAAwRQIhANfakefakefakefakefakefakefakefakefake > +-----END CERTIFICATE----- > +EOF > + }, [snip] > + { > + expected_success =3D> 1, > + name =3D> "many newlines pem", > + pem =3D> <<'EOF', > +-----BEGIN CERTIFICATE----- > + > + > + > +-----END CERTIFICATE----- > +EOF > + }, > + { > + expected_success =3D> 0, > + name =3D> "no content pem", Same as above, i would prefer a more descriptive name. Something like "no content pem returns undef" [snip]