all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [PATCH common/proxmox-acme 0/2] fix #5978: pem parser: relax parsing of chain entries
@ 2026-06-09 13:15 Thomas Ellmenreich
  2026-06-09 13:15 ` [PATCH common 1/2] fix #5978: pem parser: relax parsing of chain entries: Thomas Ellmenreich
  2026-06-09 13:15 ` [PATCH proxmox-acme 2/2] " Thomas Ellmenreich
  0 siblings, 2 replies; 5+ messages in thread
From: Thomas Ellmenreich @ 2026-06-09 13:15 UTC (permalink / raw)
  To: pve-devel; +Cc: Thomas Ellmenreich

According to RFC 8555, expected certchains should come 
without whitespace or explanatory texts inbetween chain 
entries. These two patches relax our parser to also 
accept text or whitespaces inbetween chain entries.


pve-common:

Thomas Ellmenreich (1):
  fix #5978: pem parser: relax parsing of chain entries:

 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


proxmox-acme:

Thomas Ellmenreich (1):
  fix #5978: pem parser: relax parsing of chain entries:

 src/PVE/ACME.pm | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)


Summary over all repositories:
  4 files changed, 281 insertions(+), 15 deletions(-)

-- 
Generated by murpp 0.12.0




^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH common 1/2] fix #5978: pem parser: relax parsing of chain entries:
  2026-06-09 13:15 [PATCH common/proxmox-acme 0/2] fix #5978: pem parser: relax parsing of chain entries Thomas Ellmenreich
@ 2026-06-09 13:15 ` Thomas Ellmenreich
  2026-06-15 11:34   ` Fabian Grünbichler
  2026-06-09 13:15 ` [PATCH proxmox-acme 2/2] " Thomas Ellmenreich
  1 sibling, 1 reply; 5+ messages in thread
From: Thomas Ellmenreich @ 2026-06-09 13:15 UTC (permalink / raw)
  To: pve-devel; +Cc: Thomas Ellmenreich

Relaxes the parser to allow for text and whitespaces inbetween certchain
entries, thus also refactored the extraction to only extract chain entries
from the string.

Added testsuite to cover a bunch of parsing edge cases.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 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

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 {
 
 sub split_pem {
     my ($content, %opts) = @_;
-    my $label = $opts{label} // 'CERTIFICATE';
+    
+    my $re = $pem_re->($opts{label} // 'CERTIFICATE');
 
-    my $header = $header_re->($label);
-    return split(/(?=$header)/, $content);
+    if (!$content =~ /^$re$/)  {
+        return undef if $opts{noerr};
+        die "not a valid PEM-formatted string.\n";
+    }
+
+    my @pem_chain = $content =~ /($re)/sg;
+    return @pem_chain;
 }
 
 sub check_pem {
     my ($content, %opts) = @_;
 
-    $content = strip_leading_text($content);
-
-    my $re = $pem_re->($opts{label} // 'CERTIFICATE');
-    $re = qr/($re\n+)*$re/ if $opts{multiple};
-
-    return $content if $content =~ /^$re$/; # OK
-
-    return undef if $opts{noerr};
-    die "not a valid PEM-formatted string.\n";
+    my @result = split_pem($content, %opts);
+    return join("", @result);
 }
 
 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 = lock_file.test			\
 	is_deeply_test.test		\
 	section_config_property_isolation_test.pl \
 	file-test.pl \
+	check_pem_test.pl \
 
 all:
 
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 
+# PVE::Certificate::split_pem function for 
+# correctness and coverage of edgecases.
+
+use lib '../src';
+
+use Test::More;
+
+use PVE::Certificate;
+
+# Arrange
+my $setup = [
+    {
+        successful => 1,
+        name => "full pem",
+        pem => <<'EOF',
+-----BEGIN CERTIFICATE-----
+MIIBsjCCAVugAwIBAgIJAO2g8Z0dXk9tMAoGCCqGSM49BAMCMEUxCzAJBgNVBAYT
+AlVTMQswCQYDVQQIDAJDQTEQMA4GA1UEBwwHQmVya2VsZXkxEDAOBgNVBAoMB1Rl
+c3QgQ0EwHhcNMjAwMTAxMDAwMDAwWhcNMzAwMTAxMDAwMDAwWjBFMQswCQYDVQQG
+EwJVUzELMAkGA1UECAwCQ0ExEDAOBgNVBAcMB0JlcmtlbGV5MRAwDgYDVQQKDAdU
+ZXN0IENBMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEv5Q8q1p7qZ2gqkQ0Qn5x
+0n9yqv8n8n7n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8aNT
+MFEwHQYDVR0OBBYEFOu2Y0bq8v3z7qkq1m1Qwqkq1m1QMB8GA1UdIwQYMBaAFOu2
+Y0bq8v3z7qkq1m1Qwqkq1m1QMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwID
+SAAwRQIhANfakefakefakefakefakefakefakefakefake
+-----END CERTIFICATE-----
+EOF
+    },
+    {
+        successful => 1,
+        name => "other label pem",
+        label => "SOME LABEL",
+        pem => <<'EOF',
+-----BEGIN SOME LABEL-----
+MIIBsjCCAVugAwIBAgIJAO2g8Z0dXk9tMAoGCCqGSM49BAMCMEUxCzAJBgNVBAYT
+AlVTMQswCQYDVQQIDAJDQTEQMA4GA1UEBwwHQmVya2VsZXkxEDAOBgNVBAoMB1Rl
+c3QgQ0EwHhcNMjAwMTAxMDAwMDAwWhcNMzAwMTAxMDAwMDAwWjBFMQswCQYDVQQG
+EwJVUzELMAkGA1UECAwCQ0ExEDAOBgNVBAcMB0JlcmtlbGV5MRAwDgYDVQQKDAdU
+ZXN0IENBMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEv5Q8q1p7qZ2gqkQ0Qn5x
+0n9yqv8n8n7n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8aNT
+MFEwHQYDVR0OBBYEFOu2Y0bq8v3z7qkq1m1Qwqkq1m1QMB8GA1UdIwQYMBaAFOu2
+Y0bq8v3z7qkq1m1Qwqkq1m1QMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwID
+SAAwRQIhANfakefakefakefakefakefakefakefakefake
+-----END SOME LABEL-----
+EOF
+    },
+    {
+        successful => 1,
+        name => "simple pem",
+        pem => <<'EOF',
+-----BEGIN CERTIFICATE-----
+test
+-----END CERTIFICATE-----
+EOF
+    },
+    {
+        successful => 1,
+        name => "spaced out pem",
+        pem => <<'EOF',
+-----BEGIN CERTIFICATE-----
+
+test
+
+-----END CERTIFICATE-----
+EOF
+    },
+    {
+        successful => 1,
+        name => "only newline pem",
+        pem => <<'EOF',
+-----BEGIN CERTIFICATE-----
+-----END CERTIFICATE-----
+EOF
+    },
+    {
+        successful => 1,
+        name => "many newlines pem",
+        pem => <<'EOF',
+-----BEGIN CERTIFICATE-----
+
+
+
+-----END CERTIFICATE-----
+EOF
+    },
+    {
+        successful => 0,
+        name => "no content pem",
+        pem => <<'EOF',
+-----BEGIN CERTIFICATE----------END CERTIFICATE-----
+EOF
+    },
+    {
+        successful => 1,
+        name => "preceding content pem",
+        pem => <<'EOF',
+some extended content
+-----BEGIN CERTIFICATE-----
+test
+-----END CERTIFICATE-----
+EOF
+,
+        expected_result => << 'EOF',
+-----BEGIN CERTIFICATE-----
+test
+-----END CERTIFICATE-----
+EOF
+    },
+    {
+        successful => 1,
+        name => "following content pem",
+        pem => <<'EOF',
+-----BEGIN CERTIFICATE-----
+test
+-----END CERTIFICATE-----
+some extended content
+EOF
+,
+        expected_result => << 'EOF',
+-----BEGIN CERTIFICATE-----
+test
+-----END CERTIFICATE-----
+EOF
+    },
+    {
+        successful => 0,
+        name => "empty pem",
+        pem => <<'EOF',
+
+EOF
+    },
+    {
+        successful => 0,
+        name => "upsidedown pem",
+        pem => <<'EOF',
+-----END CERTIFICATE-----
+test
+-----BEGIN CERTIFICATE-----
+EOF
+    },
+    {
+        successful => 1,
+        name => "multi pem",
+        multiple => 1,
+        pem => <<'EOF',
+-----BEGIN CERTIFICATE-----
+12JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+22JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+32JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+42JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+EOF
+    },
+    {
+        successful => 1,
+        name => "big pem",
+        multiple => 1,
+        pem => <<'EOF',
+Subject: CN=some.test.content
+Issuer: CN=Org Issuing CA 3
+-----BEGIN CERTIFICATE-----
+1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
+dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
+dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
+c2JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+Subject: CN=Org Issuing CA 3
+Issuer: CN=Org Root CA
+-----BEGIN CERTIFICATE-----
+2WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
+dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
+dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
+c2JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+Subject: CN=Org Root CA
+Issuer: CN=Org Root CA
+-----BEGIN CERTIFICATE-----
+3WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
+dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
+dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
+c2JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+EOF
+,
+        expected_result => << 'EOF',
+-----BEGIN CERTIFICATE-----
+1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
+dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
+dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
+c2JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+2WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
+dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
+dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
+c2JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+3WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
+dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
+dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
+c2JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+EOF
+    },
+    {
+        successful => 1,
+        name => "other real pem",
+        pem => <<'EOF',
+Subject: CN=Some subject with 2 Points,O=A Organization,L=Some Lation,C=TT
+Issuer: CN=The Orgs Name RootCA 2015,O=The Orgs name Institutions Cert. Authority,L=Location,C=TT
+-----BEGIN CERTIFICATE-----
+1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
+dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
+dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
+c2JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+EOF
+,
+        expected_result => << 'EOF',
+-----BEGIN CERTIFICATE-----
+1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
+dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
+dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
+c2JlZ2tseTxzZWl2IGF3ZWlidm4=
+-----END CERTIFICATE-----
+EOF
+    }
+];
+
+for my $data ($setup->@*) {
+    # 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} . " accepted");       
+    } 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");
+    }
+}
+
+done_testing();
-- 
2.47.3





^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH proxmox-acme 2/2] fix #5978: pem parser: relax parsing of chain entries:
  2026-06-09 13:15 [PATCH common/proxmox-acme 0/2] fix #5978: pem parser: relax parsing of chain entries Thomas Ellmenreich
  2026-06-09 13:15 ` [PATCH common 1/2] fix #5978: pem parser: relax parsing of chain entries: Thomas Ellmenreich
@ 2026-06-09 13:15 ` Thomas Ellmenreich
  2026-06-15 11:34   ` Fabian Grünbichler
  1 sibling, 1 reply; 5+ messages in thread
From: Thomas Ellmenreich @ 2026-06-09 13:15 UTC (permalink / raw)
  To: pve-devel; +Cc: Thomas Ellmenreich

Instead of using a custom regex to parse pem chains, now uses
the pve-common Certificate::check_pem function to do so. This
now allows for additional text and whitespace inbetween the
chain entries.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/ACME.pm | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/PVE/ACME.pm b/src/PVE/ACME.pm
index e6fb9c2..ff10c22 100644
--- a/src/PVE/ACME.pm
+++ b/src/PVE/ACME.pm
@@ -530,9 +530,11 @@ sub get_certificate {
                 if !defined($res);
         }
 
-        if ($res =~ /^(-----BEGIN CERTIFICATE-----)(.+)(-----END CERTIFICATE-----)$/s) { # untaint
-            return $1 . $2 . $3;
-        }
+        my $checked = PVE::Certificate::check_pem->($res, no_err => 1);
+        if (defined $checked) {
+            return $checked;
+        } 
+
         die "Server reply does not look like a PEM encoded certificate\n";
     };
     $self->fatal("POST of '$order->{certificate}' failed - $@", $r) if $@;
-- 
2.47.3





^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH proxmox-acme 2/2] fix #5978: pem parser: relax parsing of chain entries:
  2026-06-09 13:15 ` [PATCH proxmox-acme 2/2] " Thomas Ellmenreich
@ 2026-06-15 11:34   ` Fabian Grünbichler
  0 siblings, 0 replies; 5+ messages in thread
From: Fabian Grünbichler @ 2026-06-15 11:34 UTC (permalink / raw)
  To: pve-devel, Thomas Ellmenreich

On June 9, 2026 3:15 pm, Thomas Ellmenreich wrote:
> Instead of using a custom regex to parse pem chains, now uses
> the pve-common Certificate::check_pem function to do so. This
> now allows for additional text and whitespace inbetween the
> chain entries.
> 
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/ACME.pm | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/ACME.pm b/src/PVE/ACME.pm
> index e6fb9c2..ff10c22 100644
> --- a/src/PVE/ACME.pm
> +++ b/src/PVE/ACME.pm
> @@ -530,9 +530,11 @@ sub get_certificate {
>                  if !defined($res);
>          }
>  
> -        if ($res =~ /^(-----BEGIN CERTIFICATE-----)(.+)(-----END CERTIFICATE-----)$/s) { # untaint
> -            return $1 . $2 . $3;
> -        }
> +        my $checked = PVE::Certificate::check_pem->($res, no_err => 1);

no_err is actually noerr in PVE::Certificate, so setting it this way has
no effect. but I don't think we need to set it here at all, we can
simply do

return PVE::Certificate::check_pem($res);

it will either die (with a hopefully meaningful error), or return the
cleaned up PEM data.

if you want to customize the error message, something like

my $res = eval { ... };
die "Server reply does not look like a PEM encoded certificate: $@\n"
  if $@;

return $res;

would be a common pattern we use in our code. eval {} is similar to
try/catch in other languages, if the code in the body dies with a fatal
error, it is stored in $@ which can be checked after.

setting noerr is usually reserved for parsing where an error is not
fatal. e.g. if we wanted to parse the data here with multiple possible
labels, we might set noerr for each attempt and only die with an error
if we find no label.

> +        if (defined $checked) {
> +            return $checked;
> +        } 

nit the last line here has trailing whitespace (you can configure your
editor or git to make this more obvious, e.g. mark it as red)

> +
>          die "Server reply does not look like a PEM encoded certificate\n";
>      };
>      $self->fatal("POST of '$order->{certificate}' failed - $@", $r) if $@;
> -- 
> 2.47.3
> 
> 
> 
> 
> 
> 




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH common 1/2] fix #5978: pem parser: relax parsing of chain entries:
  2026-06-09 13:15 ` [PATCH common 1/2] fix #5978: pem parser: relax parsing of chain entries: Thomas Ellmenreich
@ 2026-06-15 11:34   ` Fabian Grünbichler
  0 siblings, 0 replies; 5+ messages in thread
From: Fabian Grünbichler @ 2026-06-15 11:34 UTC (permalink / raw)
  To: pve-devel, Thomas Ellmenreich

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 entries
> from the string.
> 
> Added testsuite to cover a bunch of parsing edge cases.
> 
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  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
> 
> 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 {
>  
>  sub split_pem {
>      my ($content, %opts) = @_;
> -    my $label = $opts{label} // 'CERTIFICATE';
> +    

nit: trailing whitespace here

> +    my $re = $pem_re->($opts{label} // 'CERTIFICATE');
>  
> -    my $header = $header_re->($label);
> -    return split(/(?=$header)/, $content);
> +    if (!$content =~ /^$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. 

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, =~ 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 =~ /^$re$/) {
..
}

will evaluate to true for a match, and false if there is none. inverting
it by changing =~ to !~ works as expected.

but tacking on a ! at the front actually evaluates to

if ((!$content) =~ /^$re$/) {
..
}

because ! has higher precedence than =~ 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 = $content =~ /($re)/sg;

the list context part you can actually see in effect here.

> +    return @pem_chain;
>  }
>  
>  sub check_pem {
>      my ($content, %opts) = @_;
>  
> -    $content = strip_leading_text($content);
> -
> -    my $re = $pem_re->($opts{label} // 'CERTIFICATE');
> -    $re = qr/($re\n+)*$re/ if $opts{multiple};
> -
> -    return $content if $content =~ /^$re$/; # OK
> -
> -    return undef if $opts{noerr};
> -    die "not a valid PEM-formatted string.\n";
> +    my @result = split_pem($content, %opts);
> +    return join("", @result);

switching like this will now always allow multiple PEM-encoded things,
which previously required an opt-in..

>  }
>  
>  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 = lock_file.test			\
>  	is_deeply_test.test		\
>  	section_config_property_isolation_test.pl \
>  	file-test.pl \
> +	check_pem_test.pl \
>  
>  all:
>  
> 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 
> +# PVE::Certificate::split_pem function for 
> +# 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 = [
> +    {
> +        successful => 1,
> +        name => "full pem",
> +        pem => <<'EOF',
> +-----BEGIN CERTIFICATE-----
> +MIIBsjCCAVugAwIBAgIJAO2g8Z0dXk9tMAoGCCqGSM49BAMCMEUxCzAJBgNVBAYT
> +AlVTMQswCQYDVQQIDAJDQTEQMA4GA1UEBwwHQmVya2VsZXkxEDAOBgNVBAoMB1Rl
> +c3QgQ0EwHhcNMjAwMTAxMDAwMDAwWhcNMzAwMTAxMDAwMDAwWjBFMQswCQYDVQQG
> +EwJVUzELMAkGA1UECAwCQ0ExEDAOBgNVBAcMB0JlcmtlbGV5MRAwDgYDVQQKDAdU
> +ZXN0IENBMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEv5Q8q1p7qZ2gqkQ0Qn5x
> +0n9yqv8n8n7n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8aNT
> +MFEwHQYDVR0OBBYEFOu2Y0bq8v3z7qkq1m1Qwqkq1m1QMB8GA1UdIwQYMBaAFOu2
> +Y0bq8v3z7qkq1m1Qwqkq1m1QMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwID
> +SAAwRQIhANfakefakefakefakefakefakefakefakefake
> +-----END CERTIFICATE-----
> +EOF
> +    },
> +    {
> +        successful => 1,
> +        name => "other label pem",
> +        label => "SOME LABEL",
> +        pem => <<'EOF',
> +-----BEGIN SOME LABEL-----
> +MIIBsjCCAVugAwIBAgIJAO2g8Z0dXk9tMAoGCCqGSM49BAMCMEUxCzAJBgNVBAYT
> +AlVTMQswCQYDVQQIDAJDQTEQMA4GA1UEBwwHQmVya2VsZXkxEDAOBgNVBAoMB1Rl
> +c3QgQ0EwHhcNMjAwMTAxMDAwMDAwWhcNMzAwMTAxMDAwMDAwWjBFMQswCQYDVQQG
> +EwJVUzELMAkGA1UECAwCQ0ExEDAOBgNVBAcMB0JlcmtlbGV5MRAwDgYDVQQKDAdU
> +ZXN0IENBMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEv5Q8q1p7qZ2gqkQ0Qn5x
> +0n9yqv8n8n7n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8n8aNT
> +MFEwHQYDVR0OBBYEFOu2Y0bq8v3z7qkq1m1Qwqkq1m1QMB8GA1UdIwQYMBaAFOu2
> +Y0bq8v3z7qkq1m1Qwqkq1m1QMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwID
> +SAAwRQIhANfakefakefakefakefakefakefakefakefake
> +-----END SOME LABEL-----
> +EOF
> +    },
> +    {
> +        successful => 1,
> +        name => "simple pem",
> +        pem => <<'EOF',
> +-----BEGIN CERTIFICATE-----
> +test
> +-----END CERTIFICATE-----
> +EOF
> +    },
> +    {
> +        successful => 1,
> +        name => "spaced out pem",
> +        pem => <<'EOF',
> +-----BEGIN CERTIFICATE-----
> +
> +test
> +
> +-----END CERTIFICATE-----
> +EOF
> +    },
> +    {
> +        successful => 1,
> +        name => "only newline pem",
> +        pem => <<'EOF',
> +-----BEGIN CERTIFICATE-----
> +-----END CERTIFICATE-----
> +EOF
> +    },
> +    {
> +        successful => 1,
> +        name => "many newlines pem",
> +        pem => <<'EOF',
> +-----BEGIN CERTIFICATE-----
> +
> +
> +
> +-----END CERTIFICATE-----
> +EOF
> +    },
> +    {
> +        successful => 0,
> +        name => "no content pem",
> +        pem => <<'EOF',
> +-----BEGIN CERTIFICATE----------END CERTIFICATE-----
> +EOF
> +    },
> +    {
> +        successful => 1,
> +        name => "preceding content pem",
> +        pem => <<'EOF',
> +some extended content
> +-----BEGIN CERTIFICATE-----
> +test
> +-----END CERTIFICATE-----
> +EOF
> +,
> +        expected_result => << 'EOF',
> +-----BEGIN CERTIFICATE-----
> +test
> +-----END CERTIFICATE-----
> +EOF
> +    },
> +    {
> +        successful => 1,
> +        name => "following content pem",
> +        pem => <<'EOF',
> +-----BEGIN CERTIFICATE-----
> +test
> +-----END CERTIFICATE-----
> +some extended content
> +EOF
> +,
> +        expected_result => << 'EOF',
> +-----BEGIN CERTIFICATE-----
> +test
> +-----END CERTIFICATE-----
> +EOF
> +    },
> +    {
> +        successful => 0,
> +        name => "empty pem",
> +        pem => <<'EOF',
> +
> +EOF
> +    },
> +    {
> +        successful => 0,
> +        name => "upsidedown pem",
> +        pem => <<'EOF',
> +-----END CERTIFICATE-----
> +test
> +-----BEGIN CERTIFICATE-----
> +EOF
> +    },
> +    {
> +        successful => 1,
> +        name => "multi pem",
> +        multiple => 1,
> +        pem => <<'EOF',
> +-----BEGIN CERTIFICATE-----
> +12JlZ2tseTxzZWl2IGF3ZWlidm4=
> +-----END CERTIFICATE-----
> +-----BEGIN CERTIFICATE-----
> +22JlZ2tseTxzZWl2IGF3ZWlidm4=
> +-----END CERTIFICATE-----
> +-----BEGIN CERTIFICATE-----
> +32JlZ2tseTxzZWl2IGF3ZWlidm4=
> +-----END CERTIFICATE-----
> +-----BEGIN CERTIFICATE-----
> +42JlZ2tseTxzZWl2IGF3ZWlidm4=
> +-----END CERTIFICATE-----
> +EOF
> +    },
> +    {
> +        successful => 1,
> +        name => "big pem",
> +        multiple => 1,
> +        pem => <<'EOF',
> +Subject: CN=some.test.content
> +Issuer: CN=Org Issuing CA 3
> +-----BEGIN CERTIFICATE-----
> +1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
> +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
> +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
> +c2JlZ2tseTxzZWl2IGF3ZWlidm4=
> +-----END CERTIFICATE-----
> +Subject: CN=Org Issuing CA 3
> +Issuer: CN=Org Root CA
> +-----BEGIN CERTIFICATE-----
> +2WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
> +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
> +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
> +c2JlZ2tseTxzZWl2IGF3ZWlidm4=
> +-----END CERTIFICATE-----
> +Subject: CN=Org Root CA
> +Issuer: CN=Org Root CA
> +-----BEGIN CERTIFICATE-----
> +3WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
> +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
> +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
> +c2JlZ2tseTxzZWl2IGF3ZWlidm4=
> +-----END CERTIFICATE-----
> +EOF
> +,
> +        expected_result => << 'EOF',
> +-----BEGIN CERTIFICATE-----
> +1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
> +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
> +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
> +c2JlZ2tseTxzZWl2IGF3ZWlidm4=
> +-----END CERTIFICATE-----
> +-----BEGIN CERTIFICATE-----
> +2WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
> +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
> +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
> +c2JlZ2tseTxzZWl2IGF3ZWlidm4=
> +-----END CERTIFICATE-----
> +-----BEGIN CERTIFICATE-----
> +3WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
> +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
> +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
> +c2JlZ2tseTxzZWl2IGF3ZWlidm4=
> +-----END CERTIFICATE-----
> +EOF
> +    },
> +    {
> +        successful => 1,
> +        name => "other real pem",
> +        pem => <<'EOF',
> +Subject: CN=Some subject with 2 Points,O=A Organization,L=Some Lation,C=TT
> +Issuer: CN=The Orgs Name RootCA 2015,O=The Orgs name Institutions Cert. Authority,L=Location,C=TT
> +-----BEGIN CERTIFICATE-----
> +1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
> +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
> +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
> +c2JlZ2tseTxzZWl2IGF3ZWlidm4=
> +-----END CERTIFICATE-----
> +EOF
> +,
> +        expected_result => << 'EOF',
> +-----BEGIN CERTIFICATE-----
> +1WtzZGhmbGtqYXNoZ2RmaXVwYWJ2aXV1YXdlYmxmamlraGFzZGtmYWlzdXBkdW5i
> +dmFpdXdlYmdmw7ZrYWpkc2JoZmdpYXNkbmJpZnVhYmdzaWprZmdyYXNkaHJmaW9h
> +dXNkbmF1c2JmZ3ZramFzZGJma2phc2RibmZpanVhIHNkb3ZuYmF3b3VlYmZnb2F3
> +c2JlZ2tseTxzZWl2IGF3ZWlidm4=
> +-----END CERTIFICATE-----
> +EOF
> +    }
> +];
> +
> +for my $data ($setup->@*) {
> +    # Act
> +    my $check_result = PVE::Certificate::check_pem(
> +        $data->{pem}, 
> +        noerr => 1, 
> +        multiple => $data->{multiple},
> +        label => $data->{label},
> +    );
> +
> +    # Assert
> +    if($data->{successful}) {

nit: missing space between if and (

> +        ok($check_result, $data->{name} . " accepted");       
> +    } 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");
> +    }

these could all move into the first branch of the if above

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




^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-06-15 11:35 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-09 13:15 [PATCH common/proxmox-acme 0/2] fix #5978: pem parser: relax parsing of chain entries Thomas Ellmenreich
2026-06-09 13:15 ` [PATCH common 1/2] fix #5978: pem parser: relax parsing of chain entries: Thomas Ellmenreich
2026-06-15 11:34   ` Fabian Grünbichler
2026-06-09 13:15 ` [PATCH proxmox-acme 2/2] " Thomas Ellmenreich
2026-06-15 11:34   ` Fabian Grünbichler

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal