[pve-devel] [PATCH v3 manager 1/3] fix #4552: certhelpers: check if custom cert and key match on change

Max Carrara m.carrara at proxmox.com
Tue Mar 14 16:08:37 CET 2023


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 <m.carrara at proxmox.com>
---
 NOTE: This patch requies a version bump+upload of pve-common.

 PVE/CertHelpers.pm | 70 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 57 insertions(+), 13 deletions(-)

diff --git a/PVE/CertHelpers.pm b/PVE/CertHelpers.pm
index 7e088cb9..e97331b8 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,67 @@ 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 = $@;
 
     if ($err) {
-	if (-e $cert_path_tmp && -e $key_path_tmp) {
-	    eval {
-		warn "Attempting to restore old certificate files..\n";
-		PVE::Tools::file_copy($cert_path_tmp, $cert_path);
-		PVE::Tools::file_copy($key_path_tmp, $key_path);
-	    };
-	    warn "$@\n" if $@;
+	warn $err . "\n";
+
+	# remove PID from front
+	$err =~ s|^(\d+:\s)||;
+
+	if (-e $cert_path_tmp || -e $key_path_tmp) {
+	    warn "Beginning to restore previous files.\n";
+
+	    if (-e $cert_path_tmp) {
+		eval {
+		    warn "Restoring previous certificate...\n";
+		    PVE::Tools::file_copy($cert_path_tmp, $cert_path);
+		};
+		warn "$@\n" if $@;
+	    }
+
+	    if (-e $key_path_tmp) {
+		eval {
+		    warn "Restoring previous key...\n";
+		    PVE::Tools::file_copy($key_path_tmp, $key_path);
+		};
+		warn "$@\n" if $@;
+	    }
+
+	    warn "Finished restoring previous files!\n";
+	}
+    }
+
+    for my $path (
+	$cert_path_tmp,
+	$cert_path_new,
+	$key_path_tmp,
+	$key_path_new,
+    ) {
+	if (-e $path) {
+	    unlink $path or warn "Failed to remove temporary file: '$path' - $!\n";
 	}
-	die "Setting certificate files failed - $err\n"
     }
 
-    unlink $cert_path_tmp;
-    unlink $key_path_tmp;
+    die "Setting certificate files failed due to the following error: $err" if $err;
 
     return $info;
 }
-- 
2.39.2





More information about the pve-devel mailing list