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

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

[Max Carrara]

... by checking whether the uploaded or existing private key matches
the uploaded SSL certificate (see bug #4552 [0]).

The subroutine that performs the check is added to `pve-common` as it
fits to the other subroutines regarding `Net::SSLeay` there. Some
minor formatting and whitespace fixes are added along the way.

The check is then performed in `pve-manager` whenever a new pair of
custom cert and key is going to be used.

During testing, it turned out that users aren't able to upload a
certificate without a key, as the web UI sends an empty string as the
private key's value if the key's field is left blank. This is also
fixed in a separate patch.


Testing
-------

Two self-signed SSL certificates as well as their keys were generated
for testing purposes:
  * openssl req -x509 -newkey rsa:4096 -keyout foo.key \
    -out foo.pem -sha256 -days 365 -nodes
  * openssl req -x509 -newkey rsa:4096 -keyout bar.key \
    -out bar.pem -sha256 -days 365 -nodes

Then, the following tests were performed via the UI:
  1. Upload `foo.pem` without key
    => fails, as no custom key exists
  2. Upload `foo.key` and `foo.pem`
    => succeeds; the custom pair is now in use
    (browser warning, viewing cert in UI)
  3. Upload `bar.pem` without key
    => fails, as `foo.key` doesn't match `bar.pem`
  4. Upload `bar.key` and `bar.pem`
    => succeeds; the existing pair is replaced with the uploaded pair
    (browser warning, viewing cert in UI)
  5. Upload `foo.pem` without key
    => fails, as `bar.key` doesn't match `foo.pem`
  6. Delete custom certificate, reload page, upload `foo.pem` again
    => fails, as `foo.key` does not exist anymore

However, what was not tested was setting up ACME via a custom domain. 
Since ACME certs are also updated via `PVE::CertHelpers::set_cert_files`,
the check performed is the same. So, if an HTTP or DNS challenge
returns an invalid key/cert pair (for whatever reason), attempting to
use that pair should therefore also fail. 
Is there anyway this can be tested locally (if necessary)?


[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4552


pve-common:

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

 src/PVE/Certificate.pm | 49 ++++++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 11 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 |  7 ++++
 2 files changed, 64 insertions(+), 8 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH 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 | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm
index 31a7722..5ec1c6b 100644
--- a/src/PVE/Certificate.pm
+++ b/src/PVE/Certificate.pm
@@ -228,6 +228,32 @@ 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\n"
+	);
+
+    Net::SSLeay::set_cert_and_key($ctx, $cert_path, $key_path);
+
+    my $result = Net::SSLeay::CTX_check_private_key($ctx);
+
+    $ssl_warn->("Failed to validate private key and certificate\n")
+	if !$result;
+
+    Net::SSLeay::CTX_free($ctx);
+
+    return $result;
+}
+
 sub get_certificate_info {
     my ($cert_path) = @_;
 
-- 
2.30.2

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

[Max Carrara]

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

diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm
index 5ec1c6b..31def4c 100644
--- a/src/PVE/Certificate.pm
+++ b/src/PVE/Certificate.pm
@@ -385,19 +385,19 @@ 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))) {
+	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");
 	}
     };
 
     $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)) {
@@ -431,11 +431,12 @@ sub generate_csr {
 	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),
+	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),
 	);
 
     $cleanup->(1, "Failed to set public key\n")
-- 
2.30.2

[pve-devel] [PATCH 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 an existing pair. Safety copies
are still made before existing files are replaced.

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

[pve-devel] [PATCH 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 | 7 +++++++
 1 file changed, 7 insertions(+)

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

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

[Matthias Heiserer]

On 28.02.2023 17:36, 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>
> ---
>   www/manager6/node/Certificates.js | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/www/manager6/node/Certificates.js b/www/manager6/node/Certificates.js
> index 34013b44..4c0b18d1 100644
> --- a/www/manager6/node/Certificates.js
> +++ b/www/manager6/node/Certificates.js
> @@ -173,6 +173,13 @@ Ext.define('PVE.node.CertUpload', {
>   	    emptyText: gettext('No change'),
>   	    name: 'key',
>   	    xtype: 'textarea',
> +	    getSubmitValue: function() {
> +		let value = this.getValue();
> +		if (Ext.isEmpty(value)) {
> +		    return null;
> +		}
> +		return value;
I guess you could save space and write "return this.getValue() || null;"
The SubmitValue is a string anyways, so behaviour for Ext.isEmpty and || 
should be the same 
(https://docs.sencha.com/extjs/7.0.0/classic/Ext.html#method-isEmpty and 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Logical_OR)
> +	    },
>   	},
>   	{
>   	    xtype: 'filebutton',

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

[Fabian Grünbichler]

On February 28, 2023 5:36 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 | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm
> index 31a7722..5ec1c6b 100644
> --- a/src/PVE/Certificate.pm
> +++ b/src/PVE/Certificate.pm
> @@ -228,6 +228,32 @@ 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\n"
> +	);

probably everything after this point [continued at [0]]

> +
> +    Net::SSLeay::set_cert_and_key($ctx, $cert_path, $key_path);

this can also fail, so should get "or $ssl_die->(..)"

> +
> +    my $result = Net::SSLeay::CTX_check_private_key($ctx);
> +
> +    $ssl_warn->("Failed to validate private key and certificate\n")
> +	if !$result;

this could simply be CTX_check_private_key($ctx) or $ssl_die->(..);

> +

[0]: until this should go into an eval block, so that we can capture $@

> +    Net::SSLeay::CTX_free($ctx);

then always run this, and then re-die if we had an error

> +
> +    return $result;

this could then return 1;, since all errors above would already be handled.. or
return nothing at all since it would just die in case of an error anyway..

so basically

...
..setup ctx or ssl_die..
eval {
    all the stuff that could fail with or ssl_die
}
my $err = $@;
..destroy ctx..
die "... $err" if $err;

> +}
> +
>  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] [PATCH common 2/2] certificate: fix formatting and whitespace

[Fabian Grünbichler]

On February 28, 2023 5:36 pm, Max Carrara wrote:
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  src/PVE/Certificate.pm | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm
> index 5ec1c6b..31def4c 100644
> --- a/src/PVE/Certificate.pm
> +++ b/src/PVE/Certificate.pm
> @@ -385,19 +385,19 @@ 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))) {
> +	if (!Net::SSLeay::X509_NAME_add_entry_by_txt(
> +		$name, $k, &Net::SSLeay::MBSTRING_UTF8, encode('utf-8', $v)

not sure if I wouldn't prefer

    if (!Net::SSLeay::X509_NAME_add_entry_by_txt(
        $name,
        $k,
        &Net::SSLeay::MBSTRING_UTF8,
        encode('utf-8', $v),
    )) {

here, it's kinda ugly in both variants to be honest ;)

maybe 

my $res = Net::SSLeay::X509_NAME_add_entry_by_txt(
    $name,
    $k,
    &Net::SSLeay::MBSTRING_UTF8,
    encode('utf-8', $v),
);

$cleanup->(..) if !$res;

would be nicer?

> +	    )
> +	) {
>  	    $cleanup->(1, "Failed to add '$k'='$v' to DN\n");
>  	}
>      };
>  
>      $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)) {
> @@ -431,11 +431,12 @@ sub generate_csr {
>  	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),
> +	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),
>  	);

this one is actually against our style guide in both old and new, it should use
a regular if with proper wrapping, not a post-if, like the first hunk of this
patch :)

https://pve.proxmox.com/wiki/Perl_Style_Guide#Breaking_long_lines_and_strings

could also use the 'store result in variable' style if it helps with readability.

>  
>      $cleanup->(1, "Failed to set public key\n")
> -- 
> 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 manager 2/2] ui: cert upload: fix private key field sending empty string

[Max Carrara]

On 3/1/23 10:35, Matthias Heiserer wrote:
> On 28.02.2023 17:36, 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>
>> ---
>>   www/manager6/node/Certificates.js | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/www/manager6/node/Certificates.js
>> b/www/manager6/node/Certificates.js
>> index 34013b44..4c0b18d1 100644
>> --- a/www/manager6/node/Certificates.js
>> +++ b/www/manager6/node/Certificates.js
>> @@ -173,6 +173,13 @@ Ext.define('PVE.node.CertUpload', {
>>           emptyText: gettext('No change'),
>>           name: 'key',
>>           xtype: 'textarea',
>> +        getSubmitValue: function() {
>> +        let value = this.getValue();
>> +        if (Ext.isEmpty(value)) {
>> +            return null;
>> +        }
>> +        return value;
> I guess you could save space and write "return this.getValue() || null;"
> The SubmitValue is a string anyways, so behaviour for Ext.isEmpty and
> || should be the same
> (https://docs.sencha.com/extjs/7.0.0/classic/Ext.html#method-isEmpty
> and
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Logical_OR)

Good point! Will include that in v2.

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

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

[Max Carrara]

On 3/1/23 11:17, Fabian Grünbichler wrote:
> On February 28, 2023 5:36 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 | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm
>> index 31a7722..5ec1c6b 100644
>> --- a/src/PVE/Certificate.pm
>> +++ b/src/PVE/Certificate.pm
>> @@ -228,6 +228,32 @@ 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\n"
>> +	);
> 
> probably everything after this point [continued at [0]]
> 
>> +
>> +    Net::SSLeay::set_cert_and_key($ctx, $cert_path, $key_path);
> 
> this can also fail, so should get "or $ssl_die->(..)"
> 
>> +
>> +    my $result = Net::SSLeay::CTX_check_private_key($ctx);
>> +
>> +    $ssl_warn->("Failed to validate private key and certificate\n")
>> +	if !$result;
> 
> this could simply be CTX_check_private_key($ctx) or $ssl_die->(..);
> 
>> +
> 
> [0]: until this should go into an eval block, so that we can capture $@
> 
>> +    Net::SSLeay::CTX_free($ctx);
> 
> then always run this, and then re-die if we had an error
> 
>> +
>> +    return $result;
> 
> this could then return 1;, since all errors above would already be handled.. or
> return nothing at all since it would just die in case of an error anyway..
> 
> so basically
> 
> ...
> ..setup ctx or ssl_die..
> eval {
>     all the stuff that could fail with or ssl_die
> }
> my $err = $@;
> ..destroy ctx..
> die "... $err" if $err;
> 
>> +}
>> +
>>  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
>>
>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

As we had discussed in person, I'll add the eval block and necessary
error handling in v2.

Also, I just had an idea: What do you think about collecting error
messages in an array (where applicable) before letting the sub die?
For example:

> my ($cert_path, $key_path) = @_;
> 
> my @errs;
> 
> push(@errs, 'No certificate path given!')
>     if !$cert_path;
> push(@errs, 'No certificate key path given!')
>     if !$key_path;
> 
> die join("\n", @errs) . "\n" if @errs;

Would maybe make it a little more "ergonomic" when debugging, but I'm
not entirely sure if that's actually necessary.

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

[Max Carrara]

On 3/1/23 11:27, Fabian Grünbichler wrote:
> On February 28, 2023 5:36 pm, Max Carrara wrote:
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>>  src/PVE/Certificate.pm | 23 ++++++++++++-----------
>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/PVE/Certificate.pm b/src/PVE/Certificate.pm
>> index 5ec1c6b..31def4c 100644
>> --- a/src/PVE/Certificate.pm
>> +++ b/src/PVE/Certificate.pm
>> @@ -385,19 +385,19 @@ 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))) {
>> +	if (!Net::SSLeay::X509_NAME_add_entry_by_txt(
>> +		$name, $k, &Net::SSLeay::MBSTRING_UTF8, encode('utf-8', $v)
> 
> not sure if I wouldn't prefer
> 
>     if (!Net::SSLeay::X509_NAME_add_entry_by_txt(
>         $name,
>         $k,
>         &Net::SSLeay::MBSTRING_UTF8,
>         encode('utf-8', $v),
>     )) {
> 
> here, it's kinda ugly in both variants to be honest ;)
> 
> maybe 
> 
> my $res = Net::SSLeay::X509_NAME_add_entry_by_txt(
>     $name,
>     $k,
>     &Net::SSLeay::MBSTRING_UTF8,
>     encode('utf-8', $v),
> );
> 
> $cleanup->(..) if !$res;
> 
> would be nicer?
>

That does look much nicer -- including that in v2. Thanks!

>> +	    )
>> +	) {
>>  	    $cleanup->(1, "Failed to add '$k'='$v' to DN\n");
>>  	}
>>      };
>>  
>>      $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)) {
>> @@ -431,11 +431,12 @@ sub generate_csr {
>>  	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),
>> +	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),
>>  	);
> 
> this one is actually against our style guide in both old and new, it should use
> a regular if with proper wrapping, not a post-if, like the first hunk of this
> patch :)
>
> https://pve.proxmox.com/wiki/Perl_Style_Guide#Breaking_long_lines_and_strings

Mea culpa; missed that.

>
> could also use the 'store result in variable' style if it helps with readability.
> 

Either way feels a bit "clumsy" to be honest.

Maybe let it short-circuit on success? Something like this:

    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")
>> -- 
>> 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
> 
>