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

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

Am 07/03/2023 um 11:52 schrieb Fabian Grünbichler:
> with an added 'check_' prefix for the sub name to indicate that this will die if
> the checked condition is not met.

please use assert_ for that in the future, check is hardly more telling
as it often indicates the booleaness of the return value not that the function
dies (i.e., asserts something).

E.g., SSLeay's CTX_check_private_key also has that semantic FWICT.

The other two existing methods in that module with the IMO not so ideal naming
scheme are check_pem and check_expiry, the former has not use outside of the
module, so it could be made private, the latter is actually already implemented
as "check" method in my understanding, and only dies if the cert could not be
parsed, but otherwise returns boolean - so it could stay.

But, as its only used once outside (no in-module use) in the ACME API - it might
be nicer to replace by a method that only extracts+returns the validity value or
range and then lets the call side do the smaller/greater than timestamp check
directly - but well, that's rather to much work for the very littel, if any,
code style improvement.

> 
> On March 3, 2023 6:57 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 | 41 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>>
>> diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm
>> index 31a7722..22de762 100644
>> --- a/src/PVE/Certificate.pm
>> +++ b/src/PVE/Certificate.pm
>> @@ -228,6 +228,47 @@ 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"
>> +	);
>> +
>> +    eval {
>> +	my $filetype = &Net::SSLeay::FILETYPE_PEM;
>> +
>> +	Net::SSLeay::CTX_use_PrivateKey_file($ctx, $key_path, $filetype)
>> +	    or $ssl_die->(
>> +		"Failed to load private key from '$key_path' into SSL context"

misses trailing new-line, like all existing call sites of this had ;-)
fixed in follow up

>> +	    );

meh this is IMO the same as the post-if, OK for one line but otherwise don't
will clarify the style guide - but actually, these would be all short enough
for 100cc - so why span a post-thingy multiline in the first place here?

Note also that in general, the closing parenthesis either goes on the line with
the param (for one or two parameters, if still << 100 CC) or an trailing comma
should be added.

I.e., if it wasn't in the stylistic already problematic post if/or context, then
either

$ssl_die->(
    "Failed to load private key from '$key_path' into SSL context\n");

or 

$ssl_die->(
    "Failed to load private key from '$key_path' into SSL context\n",
); 

The former saves a line for few but long arguments, the latter is easily
to extend without touching existing lines.


>> +
>> +	Net::SSLeay::CTX_use_certificate_file($ctx, $cert_path, $filetype)
>> +	    or $ssl_die->(
>> +		"Failed to load certificate from '$cert_path' into SSL context"
>> +	    );

same here

>> +
>> +	Net::SSLeay::CTX_check_private_key($ctx)
>> +	    or $ssl_die->(
>> +		"Failed to validate private key and certificate"
>> +	    );

same here

I pushed a few follow up cleaning that whole module up, while I tried to be careful,
a second eye couldn't hurt to detect any possible fat-fingered error I made ;-)

>> +    };
>> +    my $err = $@;
>> +
>> +    Net::SSLeay::CTX_free($ctx);
>> +
>> +    die $err if $err;
>> +
>> +    return 1;
>> +}
>> +
>>  sub get_certificate_info {
>>      my ($cert_path) = @_;
>>