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 <[email protected]> >> --- >> 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) = @_; >> _______________________________________________ pve-devel mailing list [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
