Re: [pve-devel] [PATCH common 1/2] certificate: add subroutine that checks if cert and key match

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

On 3/1/23 11:17, Fabian Grünbichler wrote:
> On February 28, 2023 5:36 pm, Max Carrara wrote:
>> This is done here in order to allow other packages to make use of
>> this subroutine.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>>  src/PVE/Certificate.pm | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm
>> index 31a7722..5ec1c6b 100644
>> --- a/src/PVE/Certificate.pm
>> +++ b/src/PVE/Certificate.pm
>> @@ -228,6 +228,32 @@ sub get_certificate_fingerprint {
>>      return $fp;
>>  }
>>  
>> +sub certificate_matches_key {
>> +    my ($cert_path, $key_path) = @_;
>> +
>> +    die "No certificate path given!\n" if !$cert_path;
>> +    die "No certificate key path given!\n" if !$key_path;
>> +
>> +    die "Certificate at '$cert_path' does not exist!\n" if ! -e $cert_path;
>> +    die "Certificate key '$key_path' does not exist!\n" if ! -e $key_path;
>> +
>> +    my $ctx = Net::SSLeay::CTX_new()
>> +	or $ssl_die->(
>> +	    "failed to create SSL context in order to verify private key\n"
>> +	);
> 
> probably everything after this point [continued at [0]]
> 
>> +
>> +    Net::SSLeay::set_cert_and_key($ctx, $cert_path, $key_path);
> 
> this can also fail, so should get "or $ssl_die->(..)"
> 
>> +
>> +    my $result = Net::SSLeay::CTX_check_private_key($ctx);
>> +
>> +    $ssl_warn->("Failed to validate private key and certificate\n")
>> +	if !$result;
> 
> this could simply be CTX_check_private_key($ctx) or $ssl_die->(..);
> 
>> +
> 
> [0]: until this should go into an eval block, so that we can capture $@
> 
>> +    Net::SSLeay::CTX_free($ctx);
> 
> then always run this, and then re-die if we had an error
> 
>> +
>> +    return $result;
> 
> this could then return 1;, since all errors above would already be handled.. or
> return nothing at all since it would just die in case of an error anyway..
> 
> so basically
> 
> ...
> ..setup ctx or ssl_die..
> eval {
>     all the stuff that could fail with or ssl_die
> }
> my $err = $@;
> ..destroy ctx..
> die "... $err" if $err;
> 
>> +}
>> +
>>  sub get_certificate_info {
>>      my ($cert_path) = @_;
>>  
>> -- 
>> 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
> 
> 

As we had discussed in person, I'll add the eval block and necessary
error handling in v2.

Also, I just had an idea: What do you think about collecting error
messages in an array (where applicable) before letting the sub die?
For example:

> my ($cert_path, $key_path) = @_;
> 
> my @errs;
> 
> push(@errs, 'No certificate path given!')
>     if !$cert_path;
> push(@errs, 'No certificate key path given!')
>     if !$key_path;
> 
> die join("\n", @errs) . "\n" if @errs;

Would maybe make it a little more "ergonomic" when debugging, but I'm
not entirely sure if that's actually necessary.