Re: [PATCH manager] certificate: make sure that any new certificate and key match

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

Am 06.05.26 um 13:03 schrieb Shannon Sterz:
> previously it was possible to upload and set a key and certificate
> combination, that did not match each other. this lead to confusing
> errors as pveproxy would seemingly start, but not actually serve any
> http connections. in a cluster context this leads to "broken pipe"
> errors when connecting to such a misconfigured node. since this is
> rather confusing, verify that a key and certificate can actually be
> used before setting them as the current certificates by loading them
> into a TLS context.
> 
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
> 
> Notes:
>     this came up in the enterprise support and caused quite a bit of
>     confusion.

nice UX polishing.

> 
>  PVE/CertHelpers.pm | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/PVE/CertHelpers.pm b/PVE/CertHelpers.pm
> index ee945827f..2fc241c24 100644
> --- a/PVE/CertHelpers.pm
> +++ b/PVE/CertHelpers.pm
> @@ -7,6 +7,8 @@ use PVE::Certificate;
>  use PVE::JSONSchema;
>  use PVE::Tools;
>  
> +use Net::SSLeay;
> +
>  my $account_prefix = '/etc/pve/priv/acme';
>  
>  PVE::JSONSchema::register_standard_option(
> @@ -81,6 +83,31 @@ sub set_cert_files {
>          PVE::Tools::file_set_contents($cert_path, $cert);
>          PVE::Tools::file_set_contents($key_path, $key) if $key;



>          $info = PVE::Certificate::get_certificate_info($cert_path);
> +
> +        if (my $method = Net::SSLeay::TLS_method()) {
> +            my $ctx = Net::SSLeay::CTX_new_with_method($method);
> +
> +            eval {
> +                Net::SSLeay::CTX_use_certificate_chain_file($ctx, $cert_path)
> +                    or die "could not load certificate (chain)\n";
> +                Net::SSLeay::CTX_use_PrivateKey_file(
> +                    $ctx,
> +                    $key_path,
> +                    Net::SSLeay::FILETYPE_PEM(),
> +                ) or die "key does not match the certificate (chain)\n";

Should we include the error stack [0] here too in these calls? Might
make debugging even easier, and as the user provided these certs, (or ACME
generated them), we cannot really leak anything with doing that I think.

[0]: https://metacpan.org/pod/Net::SSLeay#Error-handling-functions

> +            };
> +
> +            my $err = $@;
> +
> +            if ($err) {
> +                # clean up invalid certificate and key
> +                unlink $cert_path;


error handling would be nice here:

unlink $cert_path or $!{ENOENT} or warn "failed to clean-up $cert_path - $!\n";

> +                unlink $key_path;

same here> +                die $err;
> +            }
> +        } else {
> +            warn "no TLS method to verify certificate and key match, continuing anyway\n";
> +        }
>      };
>      my $err = $@;
>