Re: [pve-devel] [PATCH v2 manager 1/2] fix #4552: certhelpers: check if custom cert and key match on change

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On March 3, 2023 6:57 pm, Max Carrara wrote:
> It is now checked whether the new custom SSL certificate actually
> matches the provided or existing custom key.
> 
> Also, the new custom certificate and key pair is now validated
> *before* it is used or replaced with the existing pair. Safety copies
> are still made; if a pair is currently in use, it is therefore left
> untouched until the new one is valid.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---

process related: when sending v3, please indicate here that this patch requires
pve-common >= XX , where XX is the actual version that has your pve-common
patch, or that it requires a bump+upload of pve-common if no such version exists yet.


>  PVE/CertHelpers.pm | 65 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 57 insertions(+), 8 deletions(-)
> 
> diff --git a/PVE/CertHelpers.pm b/PVE/CertHelpers.pm
> index 7e088cb9..826f39bc 100644
> --- a/PVE/CertHelpers.pm
> +++ b/PVE/CertHelpers.pm
> @@ -53,12 +53,15 @@ sub cert_lock {
>  sub set_cert_files {
>      my ($cert, $key, $path_prefix, $force) = @_;
>  
> -    my ($old_cert, $old_key, $info);
> +    my $info;
>  
>      my $cert_path = "${path_prefix}.pem";
>      my $cert_path_tmp = "${path_prefix}.pem.old";
> +    my $cert_path_new = "${path_prefix}.pem.new";
> +
>      my $key_path = "${path_prefix}.key";
>      my $key_path_tmp = "${path_prefix}.key.old";
> +    my $key_path_new = "${path_prefix}.key.new";
>  
>      die "Custom certificate file exists but force flag is not set.\n"
>  	if !$force && -e $cert_path;
> @@ -69,26 +72,72 @@ sub set_cert_files {
>      PVE::Tools::file_copy($key_path, $key_path_tmp) if -e $key_path;
>  
>      eval {
> -	PVE::Tools::file_set_contents($cert_path, $cert);
> -	PVE::Tools::file_set_contents($key_path, $key) if $key;
> +	PVE::Tools::file_set_contents($cert_path_new, $cert);
> +
> +	if ($key) {
> +	    PVE::Tools::file_set_contents($key_path_new, $key);
> +	} elsif (-e $key_path) {
> +	    PVE::Tools::file_copy($key_path, $key_path_new);
> +	} else {
> +	    die "Cannot set custom certificate without key.\n";
> +	}
> +
> +	die "Custom certificate does not match provided key.\n"
> +	    if !PVE::Certificate::certificate_matches_key($cert_path_new, $key_path_new);
> +
> +	PVE::Tools::file_copy($cert_path_new, $cert_path);
> +	PVE::Tools::file_copy($key_path_new, $key_path);
> +
>  	$info = PVE::Certificate::get_certificate_info($cert_path);
>      };
>      my $err = $@;

I think I'd actually warn about this error here, else the order is a bit weird:

 Setting custom certificate files
 Removing uploaded certificate...
 Removing uploaded key...
 Setting certificate files failed - 3385755: Failed to validate private key and certificate

and it looks like it removed something and then failed. in my experience, this
tends to confuse users, although it is unfortunately a pattern that is sometimes
hard to avoid without making the code harder to follow.

maybe the removing could even be quiet, unless something goes amiss? or the
messages could clearly indicate when error handling starts, e.g. like this:

 Setting custom certificate files
 Setting certificate files failed - 3385755: Failed to validate private key and certificate
 
 Starting cleanup:
 Removing uploaded certificate...
 Removing uploaded key...
 Cleanup done!

>  
>      if ($err) {
> -	if (-e $cert_path_tmp && -e $key_path_tmp) {
> +	if (-e $cert_path_new) {
> +	    eval {
> +		warn "Removing uploaded certificate...\n";
> +		unlink $cert_path_new;
> +	    };

unlink doesn't die on failure, it returns false and sets $!, so this error
handling needs to be adapted, but see last comment below.

> +	    warn "$@\n" if $@;
> +	}
> +	if (-e $cert_path_tmp) {
>  	    eval {
> -		warn "Attempting to restore old certificate files..\n";
> +		warn "Restoring previous certificate...\n";
>  		PVE::Tools::file_copy($cert_path_tmp, $cert_path);
> +		unlink $cert_path_tmp;
> +	    };
> +	    warn "$@\n" if $@;

same here

> +	}
> +	if (-e $key_path_new) {
> +	    eval {
> +		if ($key) {
> +		    warn "Removing uploaded key...\n";
> +		} else {
> +		    warn "Removing temporary copy of key...\n";
> +		}
> +		unlink $key_path_new;
> +	    };
> +	    warn "$@\n" if $@;

and here

> +	}
> +	if (-e $key_path_tmp) {
> +	    eval {
> +		warn "Restoring previous key...\n";
>  		PVE::Tools::file_copy($key_path_tmp, $key_path);
> +		unlink $key_path_tmp;
>  	    };
>  	    warn "$@\n" if $@;
>  	}
> -	die "Setting certificate files failed - $err\n"
> +	die "Setting certificate files failed - $err\n";
>      }
>  
> -    unlink $cert_path_tmp;
> -    unlink $key_path_tmp;
> +    for my $path (
> +	$cert_path_tmp,
> +	$cert_path_new,
> +	$key_path_tmp,
> +	$key_path_new,
> +    ) {
> +	unlink $path if -e $path;
> +    }

but actually, if you remove all those files here unconditionally, you don't need to remove them
higher up in the cleanup code path? like before, you could just restore the .tmp
files if they exist (and for the key, only if $key was set?)

down here, you could do

if (-e $path) {
    unlink $path or warn "Failed to clean up '$path' - $!";
}

or some variant thereof, so that if removing of these temporary files fails, the
user gets a warning, but otherwise it's silent.

>  
>      return $info;
>  }
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>