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

[Max Carrara <m.carrara@proxmox.com>]

On 3/7/23 12:07, Fabian Grünbichler wrote:
> 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!
>

Very good point, I hadn't thought about that.

>>
>>      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.
>

That seems much more elegant to me; thanks for pointing that out. I'll
incorporate that in a v3 while also making the rest a little more
user-friendly.

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