From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id DAEC01FF138 for ; Mon, 15 Jun 2026 13:34:47 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id ED96C120FB; Mon, 15 Jun 2026 13:34:45 +0200 (CEST) Date: Mon, 15 Jun 2026 13:34:37 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= Subject: Re: [PATCH common 1/2] fix #5978: pem parser: relax parsing of chain entries: To: pve-devel@lists.proxmox.com, Thomas Ellmenreich References: <20260609131549.104216-1-t.ellmenreich@proxmox.com> <20260609131549.104216-2-t.ellmenreich@proxmox.com> In-Reply-To: <20260609131549.104216-2-t.ellmenreich@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1781519756.4d3e17amu4.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1781523226754 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.054 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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: OPFXWA7T6ZTPAO2KJP3NUFNPLLCWRM63 X-Message-ID-Hash: OPFXWA7T6ZTPAO2KJP3NUFNPLLCWRM63 X-MailFrom: f.gruenbichler@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 June 9, 2026 3:15 pm, Thomas Ellmenreich wrote: > Relaxes the parser to allow for text and whitespaces inbetween certchain > entries, thus also refactored the extraction to only extract chain entrie= s > from the string. >=20 > Added testsuite to cover a bunch of parsing edge cases. >=20 > Signed-off-by: Thomas Ellmenreich > --- > src/PVE/Certificate.pm | 23 ++-- > test/Makefile | 1 + > test/check_pem_test.pl | 264 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 276 insertions(+), 12 deletions(-) > create mode 100755 test/check_pem_test.pl >=20 > diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm > index b8415e2..fc45b63 100644 > --- a/src/PVE/Certificate.pm > +++ b/src/PVE/Certificate.pm > @@ -136,24 +136,23 @@ sub strip_leading_text { > =20 > sub split_pem { > my ($content, %opts) =3D @_; > - my $label =3D $opts{label} // 'CERTIFICATE'; > + =20 nit: trailing whitespace here > + my $re =3D $pem_re->($opts{label} // 'CERTIFICATE'); > =20 > - my $header =3D $header_re->($label); > - return split(/(?=3D$header)/, $content); > + if (!$content =3D~ /^$re$/) { > + return undef if $opts{noerr}; > + die "not a valid PEM-formatted string.\n"; > + } this changes the behaviour quite substantially though - previously the input string was just split at the header lines. now it requires the whole file to be valid PEM. but it seems this is only called by the ACME code anyway. I was quite confused though how this works with the test cases below, after all $pem_re does not accept leading or trailing text after the first header or last footer.=20 then I spotted it - what you actually wanted there is: if ($content !~ /^$re$/) { } which actually causes 10 of the 25 test cases to fail, because there is no handling of leading or trailing or interspersed non-PEM text here. perl can be quite tricky because it has call-site context dependenct behaviour changing the return type and value of a called function, and operator binding can be a bit tricky sometimes. in list context, =3D~ will return the matches of the RE (similar to how $N are set afterwards if there were matches). in scalar context, it will return whether there was a match or not. so if ($content =3D~ /^$re$/) { .. } will evaluate to true for a match, and false if there is none. inverting it by changing =3D~ to !~ works as expected. but tacking on a ! at the front actually evaluates to if ((!$content) =3D~ /^$re$/) { .. } because ! has higher precedence than =3D~ and !~ negating a string is just a false value (unless the string is completely empty ;)), and that will never match our regular expression here. > + > + my @pem_chain =3D $content =3D~ /($re)/sg; the list context part you can actually see in effect here. > + return @pem_chain; > } > =20 > sub check_pem { > my ($content, %opts) =3D @_; > =20 > - $content =3D strip_leading_text($content); > - > - my $re =3D $pem_re->($opts{label} // 'CERTIFICATE'); > - $re =3D qr/($re\n+)*$re/ if $opts{multiple}; > - > - return $content if $content =3D~ /^$re$/; # OK > - > - return undef if $opts{noerr}; > - die "not a valid PEM-formatted string.\n"; > + my @result =3D split_pem($content, %opts); > + return join("", @result); switching like this will now always allow multiple PEM-encoded things, which previously required an opt-in.. > } > =20 > sub pem_to_der { > diff --git a/test/Makefile b/test/Makefile > index 9b9f81b..de6ae60 100644 > --- a/test/Makefile > +++ b/test/Makefile > @@ -14,6 +14,7 @@ TESTS =3D lock_file.test \ > is_deeply_test.test \ > section_config_property_isolation_test.pl \ > file-test.pl \ > + check_pem_test.pl \ > =20 > all: > =20 > diff --git a/test/check_pem_test.pl b/test/check_pem_test.pl > new file mode 100755 > index 0000000..dc5fd7e > --- /dev/null > +++ b/test/check_pem_test.pl > @@ -0,0 +1,264 @@ > +#!/usr/bin/perl > +# Tests the PVE::Certificate::check_pem and the=20 > +# PVE::Certificate::split_pem function for=20 > +# correctness and coverage of edgecases. nit: this only indirectly tests split_pem, so I'd drop that part or add actual test cases for verifying the splitting as well. > + > +use lib '../src'; > + > +use Test::More; > + > +use PVE::Certificate; > + > +# Arrange > +my $setup =3D [ > + { > + successful =3D> 1, > + name =3D> "full pem", > + pem =3D> <<'EOF', > +-----BEGIN CERTIFICATE----- > +MIIBsjCCAVugAwIBAgIJAO2g8Z0dXk9tMAoGCCqGSM49BAMCMEUxCzAJBgNVBAYT > +AlVTMQswCQYDVQQIDAJDQTEQMA4GA1UEBwwHQmVya2VsZXkxEDAOBgNVBAoMB1Rl > +c3QgQ0EwHhcNMjAwMTAxMDAwMDAwWhcNMzAwMTAxMDAwMDAwWjBFMQswCQYDVQQG > +EwJVUzELMAkGA1UECAwCQ0ExEDAOBgNVBAcMB0JlcmtlbGV5MRAwDgYDVQQKDAdU > +ZXN0IENBMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEv5Q8q1p7qZ2gqkQ0Qn5x > +0n9yqv8n8n7n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8aNT > +MFEwHQYDVR0OBBYEFOu2Y0bq8v3z7qkq1m1Qwqkq1m1QMB8GA1UdIwQYMBaAFOu2 > +Y0bq8v3z7qkq1m1Qwqkq1m1QMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwID > +SAAwRQIhANfakefakefakefakefakefakefakefakefake > +-----END CERTIFICATE----- > +EOF > + }, > + { > + successful =3D> 1, > + name =3D> "other label pem", > + label =3D> "SOME LABEL", > + pem =3D> <<'EOF', > +-----BEGIN SOME LABEL----- > +MIIBsjCCAVugAwIBAgIJAO2g8Z0dXk9tMAoGCCqGSM49BAMCMEUxCzAJBgNVBAYT > +AlVTMQswCQYDVQQIDAJDQTEQMA4GA1UEBwwHQmVya2VsZXkxEDAOBgNVBAoMB1Rl > +c3QgQ0EwHhcNMjAwMTAxMDAwMDAwWhcNMzAwMTAxMDAwMDAwWjBFMQswCQYDVQQG > +EwJVUzELMAkGA1UECAwCQ0ExEDAOBgNVBAcMB0JlcmtlbGV5MRAwDgYDVQQKDAdU > +ZXN0IENBMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEv5Q8q1p7qZ2gqkQ0Qn5x > +0n9yqv8n8n7n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8aNT > +MFEwHQYDVR0OBBYEFOu2Y0bq8v3z7qkq1m1Qwqkq1m1QMB8GA1UdIwQYMBaAFOu2 > +Y0bq8v3z7qkq1m1Qwqkq1m1QMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwID > +SAAwRQIhANfakefakefakefakefakefakefakefakefake > +-----END SOME LABEL----- > +EOF > + }, > + { > + successful =3D> 1, > + name =3D> "simple pem", > + pem =3D> <<'EOF', > +-----BEGIN CERTIFICATE----- > +test > +-----END CERTIFICATE----- > +EOF > + }, > + { > + successful =3D> 1, > + name =3D> "spaced out pem", > + pem =3D> <<'EOF', > +-----BEGIN CERTIFICATE----- > + > +test > + > +-----END CERTIFICATE----- > +EOF > + }, > + { > + successful =3D> 1, > + name =3D> "only newline pem", > + pem =3D> <<'EOF', > +-----BEGIN CERTIFICATE----- > +-----END CERTIFICATE----- > +EOF > + }, > + { > + successful =3D> 1, > + name =3D> "many newlines pem", > + pem =3D> <<'EOF', > +-----BEGIN CERTIFICATE----- > + > + > + > +-----END CERTIFICATE----- > +EOF > + }, > + { > + successful =3D> 0, > + name =3D> "no content pem", > + pem =3D> <<'EOF', > +-----BEGIN CERTIFICATE----------END CERTIFICATE----- > +EOF > + }, > + { > + successful =3D> 1, > + name =3D> "preceding content pem", > + pem =3D> <<'EOF', > +some extended content > +-----BEGIN CERTIFICATE----- > +test > +-----END CERTIFICATE----- > +EOF > +, > + expected_result =3D> << 'EOF', > +-----BEGIN CERTIFICATE----- > +test > +-----END CERTIFICATE----- > +EOF > + }, > + { > + successful =3D> 1, > + name =3D> "following content pem", > + pem =3D> <<'EOF', > +-----BEGIN CERTIFICATE----- > +test > +-----END CERTIFICATE----- > +some extended content > +EOF > +, > + expected_result =3D> << 'EOF', > +-----BEGIN CERTIFICATE----- > +test > +-----END CERTIFICATE----- > +EOF > + }, > + { > + successful =3D> 0, > + name =3D> "empty pem", > + pem =3D> <<'EOF', > + > +EOF > + }, > + { > + successful =3D> 0, > + name =3D> "upsidedown pem", > + pem =3D> <<'EOF', > +-----END CERTIFICATE----- > +test > +-----BEGIN CERTIFICATE----- > +EOF > + }, > + { > + successful =3D> 1, > + name =3D> "multi pem", > + multiple =3D> 1, > + pem =3D> <<'EOF', > +-----BEGIN CERTIFICATE----- > +12JlZ2tseTxzZWl2IGF3ZWlidm4=3D > +-----END CERTIFICATE----- > +-----BEGIN CERTIFICATE----- > +22JlZ2tseTxzZWl2IGF3ZWlidm4=3D > +-----END CERTIFICATE----- > +-----BEGIN CERTIFICATE----- > +32JlZ2tseTxzZWl2IGF3ZWlidm4=3D > +-----END CERTIFICATE----- > +-----BEGIN CERTIFICATE----- > +42JlZ2tseTxzZWl2IGF3ZWlidm4=3D > +-----END CERTIFICATE----- > +EOF > + }, > + { > + successful =3D> 1, > + name =3D> "big pem", > + multiple =3D> 1, > + pem =3D> <<'EOF', > +Subject: CN=3Dsome.test.content > +Issuer: CN=3DOrg Issuing CA 3 > +-----BEGIN CERTIFICATE----- > +1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i > +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h > +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3 > +c2JlZ2tseTxzZWl2IGF3ZWlidm4=3D > +-----END CERTIFICATE----- > +Subject: CN=3DOrg Issuing CA 3 > +Issuer: CN=3DOrg Root CA > +-----BEGIN CERTIFICATE----- > +2WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i > +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h > +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3 > +c2JlZ2tseTxzZWl2IGF3ZWlidm4=3D > +-----END CERTIFICATE----- > +Subject: CN=3DOrg Root CA > +Issuer: CN=3DOrg Root CA > +-----BEGIN CERTIFICATE----- > +3WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i > +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h > +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3 > +c2JlZ2tseTxzZWl2IGF3ZWlidm4=3D > +-----END CERTIFICATE----- > +EOF > +, > + expected_result =3D> << 'EOF', > +-----BEGIN CERTIFICATE----- > +1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i > +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h > +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3 > +c2JlZ2tseTxzZWl2IGF3ZWlidm4=3D > +-----END CERTIFICATE----- > +-----BEGIN CERTIFICATE----- > +2WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i > +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h > +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3 > +c2JlZ2tseTxzZWl2IGF3ZWlidm4=3D > +-----END CERTIFICATE----- > +-----BEGIN CERTIFICATE----- > +3WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i > +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h > +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3 > +c2JlZ2tseTxzZWl2IGF3ZWlidm4=3D > +-----END CERTIFICATE----- > +EOF > + }, > + { > + successful =3D> 1, > + name =3D> "other real pem", > + pem =3D> <<'EOF', > +Subject: CN=3DSome subject with 2 Points,O=3DA Organization,L=3DSome Lat= ion,C=3DTT > +Issuer: CN=3DThe Orgs Name RootCA 2015,O=3DThe Orgs name Institutions Ce= rt. Authority,L=3DLocation,C=3DTT > +-----BEGIN CERTIFICATE----- > +1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i > +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h > +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3 > +c2JlZ2tseTxzZWl2IGF3ZWlidm4=3D > +-----END CERTIFICATE----- > +EOF > +, > + expected_result =3D> << 'EOF', > +-----BEGIN CERTIFICATE----- > +1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i > +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h > +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3 > +c2JlZ2tseTxzZWl2IGF3ZWlidm4=3D > +-----END CERTIFICATE----- > +EOF > + } > +]; > + > +for my $data ($setup->@*) { > + # Act > + my $check_result =3D PVE::Certificate::check_pem( > + $data->{pem},=20 > + noerr =3D> 1,=20 > + multiple =3D> $data->{multiple}, > + label =3D> $data->{label}, > + ); > + > + # Assert > + if($data->{successful}) { nit: missing space between if and ( > + ok($check_result, $data->{name} . " accepted"); =20 > + } else { > + ok(!$check_result, $data->{name} . " correctly rejected"); = =20 > + } > + > + my $expected_result =3D $data->{expected_result}=20 > + ? $data->{expected_result}=20 > + : $data->{pem}; > + > + if ($data->{successful}) { > + ok($check_result eq $expected_result, $data->{name} . " has cor= rect check output"); > + } these could all move into the first branch of the if above > +} > + > +done_testing(); > --=20 > 2.47.3 >=20 >=20 >=20 >=20 >=20 >=20