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 EA18D8E10 for ; Tue, 7 Mar 2023 12:08:05 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C7207B277 for ; Tue, 7 Mar 2023 12:07:35 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 7 Mar 2023 12:07:34 +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 54880436A3 for ; Tue, 7 Mar 2023 12:07:34 +0100 (CET) Date: Tue, 07 Mar 2023 12:07:26 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20230303175705.214121-1-m.carrara@proxmox.com> <20230303175705.214121-4-m.carrara@proxmox.com> In-Reply-To: <20230303175705.214121-4-m.carrara@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1678186435.yucj1q2em3.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.123 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 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. [proxmox.com, certhelpers.pm] Subject: Re: [pve-devel] [PATCH v2 manager 1/2] fix #4552: certhelpers: check if custom cert and key match on change 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 11:08:06 -0000 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. >=20 > 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. >=20 > Signed-off-by: Max Carrara > --- process related: when sending v3, please indicate here that this patch requ= ires pve-common >=3D XX , where XX is the actual version that has your pve-commo= n patch, or that it requires a bump+upload of pve-common if no such version e= xists yet. > PVE/CertHelpers.pm | 65 ++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 57 insertions(+), 8 deletions(-) >=20 > 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) =3D @_; > =20 > - my ($old_cert, $old_key, $info); > + my $info; > =20 > my $cert_path =3D "${path_prefix}.pem"; > my $cert_path_tmp =3D "${path_prefix}.pem.old"; > + my $cert_path_new =3D "${path_prefix}.pem.new"; > + > my $key_path =3D "${path_prefix}.key"; > my $key_path_tmp =3D "${path_prefix}.key.old"; > + my $key_path_new =3D "${path_prefix}.key.new"; > =20 > 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; > =20 > 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 =3D PVE::Certificate::get_certificate_info($cert_path); > }; > my $err =3D $@; I think I'd actually warn about this error here, else the order is a bit we= ird: 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, t= his tends to confuse users, although it is unfortunately a pattern that is some= times 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 =20 Starting cleanup: Removing uploaded certificate... Removing uploaded key... Cleanup done! > =20 > 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"; > } > =20 > - 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. > =20 > return $info; > } > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20