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 53B768DF8 for ; Tue, 7 Mar 2023 16:45:59 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3222BDEC2 for ; Tue, 7 Mar 2023 16:45:59 +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 16:45:56 +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 93381453B5 for ; Tue, 7 Mar 2023 16:45:56 +0100 (CET) Message-ID: <24273efd-09a2-5fca-b047-f2d77096e2d0@proxmox.com> Date: Tue, 7 Mar 2023 16:45:54 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Content-Language: en-US To: pve-devel@lists.proxmox.com References: <20230303175705.214121-1-m.carrara@proxmox.com> <20230303175705.214121-4-m.carrara@proxmox.com> <1678186435.yucj1q2em3.astroid@yuna.none> From: Max Carrara In-Reply-To: <1678186435.yucj1q2em3.astroid@yuna.none> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.370 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_ASCII_DIVIDERS 0.8 Email that uses ascii formatting dividers and possible spam tricks 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. [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 15:45:59 -0000 On 3/7/23 12:07, Fabian Grünbichler wrote: > 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. >> >> 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. >> >> Signed-off-by: Max Carrara >> --- > > process related: when sending v3, please indicate here that this patch requires > pve-common >= XX , where XX is the actual version that has your pve-common > patch, or that it requires a bump+upload of pve-common if no such version exists yet. > > >> PVE/CertHelpers.pm | 65 ++++++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 57 insertions(+), 8 deletions(-) >> >> 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) = @_; >> >> - my ($old_cert, $old_key, $info); >> + my $info; >> >> my $cert_path = "${path_prefix}.pem"; >> my $cert_path_tmp = "${path_prefix}.pem.old"; >> + my $cert_path_new = "${path_prefix}.pem.new"; >> + >> my $key_path = "${path_prefix}.key"; >> my $key_path_tmp = "${path_prefix}.key.old"; >> + my $key_path_new = "${path_prefix}.key.new"; >> >> 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; >> >> 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 = PVE::Certificate::get_certificate_info($cert_path); >> }; >> my $err = $@; > > I think I'd actually warn about this error here, else the order is a bit weird: > > 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, this > tends to confuse users, although it is unfortunately a pattern that is sometimes > 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 > > Starting cleanup: > Removing uploaded certificate... > Removing uploaded key... > Cleanup done! > Very good point, I hadn't thought about that. >> >> 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"; >> } >> >> - 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. > That seems much more elegant to me; thanks for pointing that out. I'll incorporate that in a v3 while also making the rest a little more user-friendly. >> >> return $info; >> } >> -- >> 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 > >