[pve-devel] [PATCH-SERIES v2 common/manager] Fix SSL Certificate and Key Upload Issues

[pve-devel] [PATCH-SERIES v2 common/manager] Fix SSL Certificate and Key Upload Issues

[Max Carrara]

This is v2 of https://lists.proxmox.com/pipermail/pve-devel/2023-February/055955.html

Changes from v1:
  - properly handle OpenSSL errors[0]
  - provide actually correct format changes[1]
  - include trailing comma[2]
  - make JavaScript code a little more concise[3]


[0] https://lists.proxmox.com/pipermail/pve-devel/2023-March/055962.html
[1] https://lists.proxmox.com/pipermail/pve-devel/2023-March/055963.html
[2] Not on mailing list unfortunately, boo!
[3] https://lists.proxmox.com/pipermail/pve-devel/2023-March/055961.html


pve-common:

Max Carrara (2):
  certificate: add subroutine that checks if cert and key match
  certificate: fix formatting and whitespace

 src/PVE/Certificate.pm | 74 +++++++++++++++++++++++++++++++++---------
 1 file changed, 59 insertions(+), 15 deletions(-)


pve-manager:

Max Carrara (2):
  fix #4552: certhelpers: check if custom cert and key match on change
  ui: cert upload: fix private key field sending empty string

 PVE/CertHelpers.pm                | 65 +++++++++++++++++++++++++++----
 www/manager6/node/Certificates.js |  3 ++
 2 files changed, 60 insertions(+), 8 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH v2 common 1/2] certificate: add subroutine that checks if cert and key match

[Max Carrara]

This is done here in order to allow other packages to make use of
this subroutine.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 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"
+	    );
+
+	Net::SSLeay::CTX_use_certificate_file($ctx, $cert_path, $filetype)
+	    or $ssl_die->(
+		"Failed to load certificate from '$cert_path' into SSL context"
+	    );
+
+	Net::SSLeay::CTX_check_private_key($ctx)
+	    or $ssl_die->(
+		"Failed to validate private key and certificate"
+	    );
+    };
+    my $err = $@;
+
+    Net::SSLeay::CTX_free($ctx);
+
+    die $err if $err;
+
+    return 1;
+}
+
 sub get_certificate_info {
     my ($cert_path) = @_;
 
-- 
2.30.2

[pve-devel] applied: [PATCH v2 common 1/2] certificate: add subroutine that checks if cert and key match

[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.

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 <m.carrara@proxmox.com>
> ---
>  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"
> +	    );
> +
> +	Net::SSLeay::CTX_use_certificate_file($ctx, $cert_path, $filetype)
> +	    or $ssl_die->(
> +		"Failed to load certificate from '$cert_path' into SSL context"
> +	    );
> +
> +	Net::SSLeay::CTX_check_private_key($ctx)
> +	    or $ssl_die->(
> +		"Failed to validate private key and certificate"
> +	    );
> +    };
> +    my $err = $@;
> +
> +    Net::SSLeay::CTX_free($ctx);
> +
> +    die $err if $err;
> +
> +    return 1;
> +}
> +
>  sub get_certificate_info {
>      my ($cert_path) = @_;
>  
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] applied: [PATCH v2 common 1/2] certificate: add subroutine that checks if cert and key match

[Thomas Lamprecht]

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 <m.carrara@proxmox.com>
>> ---
>>  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] [PATCH v2 common 2/2] certificate: fix formatting and whitespace

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Certificate.pm | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

NOTE: This patch can be dropped if not desired.

diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm
index 22de762..73091ff 100644
--- a/src/PVE/Certificate.pm
+++ b/src/PVE/Certificate.pm
@@ -400,19 +400,22 @@ sub generate_csr {
     $ssl_die->("Failed to allocate X509_NAME object\n") if !$name;
     my $add_name_entry = sub {
 	my ($k, $v) = @_;
-	if (!Net::SSLeay::X509_NAME_add_entry_by_txt($name,
-	                                             $k,
-	                                             &Net::SSLeay::MBSTRING_UTF8,
-	                                             encode('utf-8', $v))) {
-	    $cleanup->(1, "Failed to add '$k'='$v' to DN\n");
-	}
+
+	my $res = Net::SSLeay::X509_NAME_add_entry_by_txt(
+	    $name,
+	    $k,
+	    &Net::SSLeay::MBSTRING_UTF8,
+	    encode('utf-8', $v),
+	);
+
+	$cleanup->(1, "Failed to add '$k'='$v' to DN\n") if !$res;
     };
 
     $add_name_entry->('CN', $common_name);
     for (qw(C ST L O OU)) {
-        if (defined(my $v = $attr{$_})) {
+	if (defined(my $v = $attr{$_})) {
 	    $add_name_entry->($_, $v);
-        }
+	}
     }
 
     if (defined($pem_key)) {
@@ -445,13 +448,13 @@ sub generate_csr {
     $cleanup->(1, "Failed to set subject name\n")
 	if (!Net::SSLeay::X509_REQ_set_subject_name($req, $name));
 
-    $cleanup->(1, "Failed to add extensions to CSR\n")
-	if !Net::SSLeay::P_X509_REQ_add_extensions($req,
-	        &Net::SSLeay::NID_key_usage => 'digitalSignature,keyEncipherment',
-	        &Net::SSLeay::NID_basic_constraints => 'CA:FALSE',
-	        &Net::SSLeay::NID_ext_key_usage => 'serverAuth,clientAuth',
-	        &Net::SSLeay::NID_subject_alt_name => join(',', map { "DNS:$_" } @$san),
-	);
+    Net::SSLeay::P_X509_REQ_add_extensions(
+	$req,
+	&Net::SSLeay::NID_key_usage => 'digitalSignature,keyEncipherment',
+	&Net::SSLeay::NID_basic_constraints => 'CA:FALSE',
+	&Net::SSLeay::NID_ext_key_usage => 'serverAuth,clientAuth',
+	&Net::SSLeay::NID_subject_alt_name => join(',', map { "DNS:$_" } @$san),
+    ) or $cleanup->(1, "Failed to add extensions to CSR\n");
 
     $cleanup->(1, "Failed to set public key\n")
 	if !Net::SSLeay::X509_REQ_set_pubkey($req, $pk);
-- 
2.30.2

[pve-devel] applied: [PATCH v2 common 2/2] certificate: fix formatting and whitespace

[Fabian Grünbichler]

On March 3, 2023 6:57 pm, Max Carrara wrote:
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  src/PVE/Certificate.pm | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> NOTE: This patch can be dropped if not desired.
> 
> diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm
> index 22de762..73091ff 100644
> --- a/src/PVE/Certificate.pm
> +++ b/src/PVE/Certificate.pm
> @@ -400,19 +400,22 @@ sub generate_csr {
>      $ssl_die->("Failed to allocate X509_NAME object\n") if !$name;
>      my $add_name_entry = sub {
>  	my ($k, $v) = @_;
> -	if (!Net::SSLeay::X509_NAME_add_entry_by_txt($name,
> -	                                             $k,
> -	                                             &Net::SSLeay::MBSTRING_UTF8,
> -	                                             encode('utf-8', $v))) {
> -	    $cleanup->(1, "Failed to add '$k'='$v' to DN\n");
> -	}
> +
> +	my $res = Net::SSLeay::X509_NAME_add_entry_by_txt(
> +	    $name,
> +	    $k,
> +	    &Net::SSLeay::MBSTRING_UTF8,
> +	    encode('utf-8', $v),
> +	);
> +
> +	$cleanup->(1, "Failed to add '$k'='$v' to DN\n") if !$res;
>      };
>  
>      $add_name_entry->('CN', $common_name);
>      for (qw(C ST L O OU)) {
> -        if (defined(my $v = $attr{$_})) {
> +	if (defined(my $v = $attr{$_})) {
>  	    $add_name_entry->($_, $v);
> -        }
> +	}
>      }
>  
>      if (defined($pem_key)) {
> @@ -445,13 +448,13 @@ sub generate_csr {
>      $cleanup->(1, "Failed to set subject name\n")
>  	if (!Net::SSLeay::X509_REQ_set_subject_name($req, $name));
>  
> -    $cleanup->(1, "Failed to add extensions to CSR\n")
> -	if !Net::SSLeay::P_X509_REQ_add_extensions($req,
> -	        &Net::SSLeay::NID_key_usage => 'digitalSignature,keyEncipherment',
> -	        &Net::SSLeay::NID_basic_constraints => 'CA:FALSE',
> -	        &Net::SSLeay::NID_ext_key_usage => 'serverAuth,clientAuth',
> -	        &Net::SSLeay::NID_subject_alt_name => join(',', map { "DNS:$_" } @$san),
> -	);
> +    Net::SSLeay::P_X509_REQ_add_extensions(
> +	$req,
> +	&Net::SSLeay::NID_key_usage => 'digitalSignature,keyEncipherment',
> +	&Net::SSLeay::NID_basic_constraints => 'CA:FALSE',
> +	&Net::SSLeay::NID_ext_key_usage => 'serverAuth,clientAuth',
> +	&Net::SSLeay::NID_subject_alt_name => join(',', map { "DNS:$_" } @$san),
> +    ) or $cleanup->(1, "Failed to add extensions to CSR\n");
>  
>      $cleanup->(1, "Failed to set public key\n")
>  	if !Net::SSLeay::X509_REQ_set_pubkey($req, $pk);
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

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

[Max Carrara]

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@proxmox.com>
---
 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 = $@;
 
     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;
+	    };
+	    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 $@;
+	}
+	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 $@;
+	}
+	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;
+    }
 
     return $info;
 }
-- 
2.30.2

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

[Fabian Grünbichler]

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 <m.carrara@proxmox.com>
> ---

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!

>  
>      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.

>  
>      return $info;
>  }
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

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

[Max Carrara]

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 <m.carrara@proxmox.com>
>> ---
>
> 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
>
>

[pve-devel] [PATCH v2 manager 2/2] ui: cert upload: fix private key field sending empty string

[Max Carrara]

The private key's field is now excluded from the upload form's
JSON data if it's considered empty by Ext.js.

Prior to this change, the form still sent an empty string if no
private key was provided by the user, even though the private key's
field is marked as optional in `pve-manager/PVE/API2/Certificates.pm`,
causing the JSONSchema validation to fail.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 www/manager6/node/Certificates.js | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/www/manager6/node/Certificates.js b/www/manager6/node/Certificates.js
index 34013b44..84fc12ff 100644
--- a/www/manager6/node/Certificates.js
+++ b/www/manager6/node/Certificates.js
@@ -173,6 +173,9 @@ Ext.define('PVE.node.CertUpload', {
 	    emptyText: gettext('No change'),
 	    name: 'key',
 	    xtype: 'textarea',
+	    getSubmitValue: function() {
+		return this.getValue() || null;
+	    },
 	},
 	{
 	    xtype: 'filebutton',
-- 
2.30.2

Re: [pve-devel] [PATCH v2 manager 2/2] ui: cert upload: fix private key field sending empty string

[Fabian Grünbichler]

On March 3, 2023 6:57 pm, Max Carrara wrote:
> The private key's field is now excluded from the upload form's
> JSON data if it's considered empty by Ext.js.
> 
> Prior to this change, the form still sent an empty string if no
> private key was provided by the user, even though the private key's
> field is marked as optional in `pve-manager/PVE/API2/Certificates.pm`,
> causing the JSONSchema validation to fail.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

didn't apply for now, as without the additional check it actually makes the
pitfall a bit worse ;)

> ---
>  www/manager6/node/Certificates.js | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/www/manager6/node/Certificates.js b/www/manager6/node/Certificates.js
> index 34013b44..84fc12ff 100644
> --- a/www/manager6/node/Certificates.js
> +++ b/www/manager6/node/Certificates.js
> @@ -173,6 +173,9 @@ Ext.define('PVE.node.CertUpload', {
>  	    emptyText: gettext('No change'),
>  	    name: 'key',
>  	    xtype: 'textarea',
> +	    getSubmitValue: function() {
> +		return this.getValue() || null;
> +	    },
>  	},
>  	{
>  	    xtype: 'filebutton',
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v2 manager 2/2] ui: cert upload: fix private key field sending empty string

[Thomas Lamprecht]

Am 03/03/2023 um 18:57 schrieb Max Carrara:
> The private key's field is now excluded from the upload form's
> JSON data if it's considered empty by Ext.js.

nit: Ext JS

> 
> Prior to this change, the form still sent an empty string if no
> private key was provided by the user, even though the private key's
> field is marked as optional in `pve-manager/PVE/API2/Certificates.pm`,

nit: I'd avoid file system paths, they're an internal detail here, if use the
api path or just in "... optional in the upload cert API endpoint"

> causing the JSONSchema validation to fail.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  www/manager6/node/Certificates.js | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/www/manager6/node/Certificates.js b/www/manager6/node/Certificates.js
> index 34013b44..84fc12ff 100644
> --- a/www/manager6/node/Certificates.js
> +++ b/www/manager6/node/Certificates.js
> @@ -173,6 +173,9 @@ Ext.define('PVE.node.CertUpload', {
>  	    emptyText: gettext('No change'),
>  	    name: 'key',
>  	    xtype: 'textarea',
> +	    getSubmitValue: function() {
> +		return this.getValue() || null;
> +	    },

This works by luck, submitData is a boolean config for the textarea [0], and
getSubmitValue has nothing to do with the actual value, but just is the getter
for the submitValue bool, which the private actual getSubmitData fn checks [1]

[0]: https://docs.sencha.com/extjs/7.0.0/classic/Ext.form.field.TextArea.html#cfg-submitValue
[1]: https://docs.sencha.com/extjs/7.0.0/classic/src/Base.js-2.html#Ext.form.field.Base-method-getSubmitData

So if, then this would need to return boolean false not null to avoid being
confusing - but there's an alternative: wrap this in a inputpanel and use it's
onGetValues helper to filter this out, as that allows to also set the hard-coded
`force` and `reload` params directly, we could drop the hidden fields too, with
also dropping the now finally visible (but ugly) 'hr' autoEl we actually would
need fewer lines.

I.e., something like:

items: {
    xtype: 'inputpanel',
    onGetValues: function(values) {
        values.restart = 1;
        values.force = 1;
        if (!values.key) {
            delete values.key;
        }
        return values;
    },
    items: [
        ....

>  	},
>  	{
>  	    xtype: 'filebutton',

Re: [pve-devel] [PATCH v2 manager 2/2] ui: cert upload: fix private key field sending empty string

[Thomas Lamprecht]

Am 07/03/2023 um 19:33 schrieb Thomas Lamprecht:
>> diff --git a/www/manager6/node/Certificates.js b/www/manager6/node/Certificates.js
>> index 34013b44..84fc12ff 100644
>> --- a/www/manager6/node/Certificates.js
>> +++ b/www/manager6/node/Certificates.js
>> @@ -173,6 +173,9 @@ Ext.define('PVE.node.CertUpload', {
>>  	    emptyText: gettext('No change'),
>>  	    name: 'key',
>>  	    xtype: 'textarea',
>> +	    getSubmitValue: function() {
>> +		return this.getValue() || null;
>> +	    },
> 
> This works by luck, submitData is a boolean config for the textarea [0], and
> getSubmitValue has nothing to do with the actual value, but just is the getter
> for the submitValue bool, which the private actual getSubmitData fn checks [1]
> 

Actually I was mistaken here (thanks Dominik for the reminder), here it indeed is
the getter for the processed raw value even if there's a boolean config for
submitValue - meh. But I'd still go for the inputpanel way in two patches, as that
cleans the whole thing up nicer and avoid depending on a bit more internal details,
making it more explicit.


> [0]: https://docs.sencha.com/extjs/7.0.0/classic/Ext.form.field.TextArea.html#cfg-submitValue
> [1]: https://docs.sencha.com/extjs/7.0.0/classic/src/Base.js-2.html#Ext.form.field.Base-method-getSubmitData
>