From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id B97FE8F06 for ; Tue, 7 Mar 2023 18:47:04 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9669BFB6E for ; Tue, 7 Mar 2023 18:47:04 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 7 Mar 2023 18:47:03 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id C9D7445DF1 for ; Tue, 7 Mar 2023 18:47:02 +0100 (CET) Message-ID: <52ce7c93-3c97-62ba-2b4a-105f8c3bad8e@proxmox.com> Date: Tue, 7 Mar 2023 18:47:01 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:111.0) Gecko/20100101 Thunderbird/111.0 Content-Language: de-AT, en-GB To: Proxmox VE development discussion , =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= , Max Carrara References: <20230303175705.214121-1-m.carrara@proxmox.com> <20230303175705.214121-2-m.carrara@proxmox.com> <1678186229.93k8jsrj2h.astroid@yuna.none> From: Thomas Lamprecht In-Reply-To: <1678186229.93k8jsrj2h.astroid@yuna.none> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL -0.050 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [certificate.pm] Subject: Re: [pve-devel] applied: [PATCH v2 common 1/2] certificate: add subroutine that checks if cert and key match X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Mar 2023 17:47:04 -0000 Am 07/03/2023 um 11:52 schrieb Fabian Gr=C3=BCnbichler: > with an added 'check_' prefix for the sub name to indicate that this wi= ll 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 fun= ction 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 n= aming 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 imple= mented as "check" method in my understanding, and only dies if the cert could no= t 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 - i= t might be nicer to replace by a method that only extracts+returns the validity v= alue or range and then lets the call side do the smaller/greater than timestamp c= heck directly - but well, that's rather to much work for the very littel, if a= ny, code style improvement. >=20 > 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 >> --- >> 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; >> } >> =20 >> +sub certificate_matches_key { >> + my ($cert_path, $key_path) =3D @_; >> + >> + 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 =3D Net::SSLeay::CTX_new() >> + or $ssl_die->( >> + "Failed to create SSL context in order to verify private key" >> + ); >> + >> + eval { >> + my $filetype =3D &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 do= n't will clarify the style guide - but actually, these would be all short eno= ugh 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 lin= e 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 contex= t, then either $ssl_die->( "Failed to load private key from '$key_path' into SSL context\n"); or=20 $ssl_die->( "Failed to load private key from '$key_path' into SSL context\n", );=20 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 ma= de ;-) >> + }; >> + my $err =3D $@; >> + >> + Net::SSLeay::CTX_free($ctx); >> + >> + die $err if $err; >> + >> + return 1; >> +} >> + >> sub get_certificate_info { >> my ($cert_path) =3D @_; >> =20