Re: [PATCH proxmox-acme v2 2/2] fix #5978: pem parser: relax parsing of chain entries:

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 17.06.26 um 14:42 schrieb 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, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/ACME.pm b/src/PVE/ACME.pm
> index e6fb9c2..4b06817 100644
> --- a/src/PVE/ACME.pm
> +++ b/src/PVE/ACME.pm
> @@ -530,10 +530,10 @@ sub get_certificate {
>                  if !defined($res);
>          }
>  
> -        if ($res =~ /^(-----BEGIN CERTIFICATE-----)(.+)(-----END CERTIFICATE-----)$/s) { # untaint
> -            return $1 . $2 . $3;
> -        }
> -        die "Server reply does not look like a PEM encoded certificate\n";
> +        my $parsed = eval { PVE::Certificate::check_pem->($res) };

Why the -> ? That's normally how one calls code references or blessed objects,
but not "normal" methods. As above will call check_perm without arguments and
then try to call the result as code reference with $res as parameter, so this
will always fail FWICT. Was this actually tested end to end?

Also, this fails with certs that are full chains, so this probably should be:

my $parsed = eval { PVE::Certificate::check_pem($res, multiple => 1) };

> +        die "Server reply does not look like a PEM encoded certificate: $@\n"

Nit: server is not really _that_ telling here, could be also interpreted as some
API server part of ours (and yes, that wording is somewhat pre-existing in another
error in this method, but still).

> +            if $@;
> +        return $parsed;
>      };
>      $self->fatal("POST of '$order->{certificate}' failed - $@", $r) if $@;
>      return $return;