[pve-devel] [PATCH v3 manager 0/3] Fix SSL Certificate and Key Upload Issues

[pve-devel] [PATCH v3 manager 0/3] Fix SSL Certificate and Key Upload Issues

[Max Carrara]

The respective previous versions can be found here:
  - v1: https://lists.proxmox.com/pipermail/pve-devel/2023-February/055955.html
  - v2: https://lists.proxmox.com/pipermail/pve-devel/2023-March/056011.html

Note: The changes to pve-common are not included in this series
anymore, as they've already been applied.


Testing
-------

The same tests as in v1 were performed.


Changes from v2:
  - properly handle cleanup and warnings as well as temp file removal
    if setting a new key/cert pair fails[0]
  - if given, remove PID from beginning of error message after warning,
    making the error message in the UI a little prettier
  - use inputpanel widget for certificate upload form before
    providing fix for private key upload[1]


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


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


pve-manager:

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

 PVE/CertHelpers.pm                | 70 ++++++++++++++++++----
 www/manager6/node/Certificates.js | 99 +++++++++++++++----------------
 2 files changed, 105 insertions(+), 64 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH v3 manager 1/3] 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>
---
 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

[pve-devel] [PATCH v3 manager 2/3] ui: cert upload: use inputpanel for certificate upload

[Max Carrara]

This change replaces the the certificate upload form's items with
a single inputpanel widget. The components for the key and cert fields
are preserved as-is. Hardcoded values are now explicitly set in
`onGetValues` instead of using hidden widgets.

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

diff --git a/www/manager6/node/Certificates.js b/www/manager6/node/Certificates.js
index 34013b44..29c17359 100644
--- a/www/manager6/node/Certificates.js
+++ b/www/manager6/node/Certificates.js
@@ -166,62 +166,56 @@ Ext.define('PVE.node.CertUpload', {
 	Ext.defer(() => window.location.reload(true), 10000); // reload after 10 seconds automatically
     },
 
-    items: [
-	{
-	    fieldLabel: gettext('Private Key (Optional)'),
-	    labelAlign: 'top',
-	    emptyText: gettext('No change'),
-	    name: 'key',
-	    xtype: 'textarea',
+    items: {
+	xtype: 'inputpanel',
+	onGetValues: function(values) {
+	    values.restart = 1;
+	    values.force = 1;
+	    return values;
 	},
-	{
-	    xtype: 'filebutton',
-	    text: gettext('From File'),
-	    listeners: {
-		change: function(btn, e, value) {
-		    let form = this.up('form');
-		    for (const file of e.event.target.files) {
-			PVE.Utils.loadFile(file, res => form.down('field[name=key]').setValue(res));
-		    }
-		    btn.reset();
+	items: [
+	    {
+		fieldLabel: gettext('Private Key (Optional)'),
+		labelAlign: 'top',
+		emptyText: gettext('No change'),
+		name: 'key',
+		xtype: 'textarea',
+	    },
+	    {
+		xtype: 'filebutton',
+		text: gettext('From File'),
+		listeners: {
+		    change: function(btn, e, value) {
+			let form = this.up('form');
+			for (const file of e.event.target.files) {
+			    PVE.Utils.loadFile(file, res => form.down('field[name=key]').setValue(res));
+			}
+			btn.reset();
+		    },
 		},
 	    },
-	},
-	{
-	    xtype: 'box',
-	    autoEl: 'hr',
-	},
-	{
-	    fieldLabel: gettext('Certificate Chain'),
-	    labelAlign: 'top',
-	    allowBlank: false,
-	    name: 'certificates',
-	    xtype: 'textarea',
-	},
-	{
-	    xtype: 'filebutton',
-	    text: gettext('From File'),
-	    listeners: {
-		change: function(btn, e, value) {
-		    let form = this.up('form');
-		    for (const file of e.event.target.files) {
-			PVE.Utils.loadFile(file, res => form.down('field[name=certificates]').setValue(res));
-		    }
-		    btn.reset();
+	    {
+		fieldLabel: gettext('Certificate Chain'),
+		labelAlign: 'top',
+		allowBlank: false,
+		name: 'certificates',
+		xtype: 'textarea',
+	    },
+	    {
+		xtype: 'filebutton',
+		text: gettext('From File'),
+		listeners: {
+		    change: function(btn, e, value) {
+			let form = this.up('form');
+			for (const file of e.event.target.files) {
+			    PVE.Utils.loadFile(file, res => form.down('field[name=certificates]').setValue(res));
+			}
+			btn.reset();
+		    },
 		},
 	    },
-	},
-	{
-	    xtype: 'hidden',
-	    name: 'restart',
-	    value: '1',
-	},
-	{
-	    xtype: 'hidden',
-	    name: 'force',
-	    value: '1',
-	},
-    ],
+	],
+    },
 
     initComponent: function() {
 	let me = this;
-- 
2.39.2

[pve-devel] [PATCH v3 manager 3/3] 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 left empty.

Prior to this change, the form still used an empty string for the
private key's field, even if no key was provided by the user.
Because the key's field is marked as optional in the upload cert
API endpoint, JSONSchema validation would therefore 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 29c17359..a086581c 100644
--- a/www/manager6/node/Certificates.js
+++ b/www/manager6/node/Certificates.js
@@ -171,6 +171,9 @@ Ext.define('PVE.node.CertUpload', {
 	onGetValues: function(values) {
 	    values.restart = 1;
 	    values.force = 1;
+	    if (!values.key) {
+		delete values.key;
+	    }
 	    return values;
 	},
 	items: [
-- 
2.39.2

[pve-devel] applied: [PATCH v3 manager 2/3] ui: cert upload: use inputpanel for certificate upload

[Thomas Lamprecht]

Am 14/03/2023 um 16:08 schrieb Max Carrara:
> This change replaces the the certificate upload form's items with
> a single inputpanel widget. The components for the key and cert fields
> are preserved as-is. Hardcoded values are now explicitly set in
> `onGetValues` instead of using hidden widgets.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  www/manager6/node/Certificates.js | 96 +++++++++++++++----------------
>  1 file changed, 45 insertions(+), 51 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v3 manager 3/3] ui: cert upload: fix private key field sending empty string

[Thomas Lamprecht]

Am 14/03/2023 um 16:08 schrieb Max Carrara:
> The private key's field is now excluded from the upload form's
> JSON data if it's left empty.
> 
> Prior to this change, the form still used an empty string for the
> private key's field, even if no key was provided by the user.
> Because the key's field is marked as optional in the upload cert
> API endpoint, JSONSchema validation would therefore fail.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  www/manager6/node/Certificates.js | 3 +++
>  1 file changed, 3 insertions(+)
> 
>

applied, thanks!

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

[Max Carrara]

On 3/14/23 16:08, 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>
> ---
>  NOTE: This patch requies a version bump+upload of pve-common.
> 

Ping - I've been testing the most recent ISO of PVE 7.4 and I think
this patch should be included in the release, as it's now technically
even easier to encounter bug #4552[0]. Before the fix for the optional
cert upload in the UI was applied (thanks, btw!) the user was forced
to provide both key *and* cert, which is not necessary anymore now if
a key/cert pair was already uploaded some time before.

So, it's now easier to lock oneself out of their own PVE instance, imo.

Additionally, if the host with the invalid key/cert pair is in a
cluster, it cannot be accessed via another host in the same cluster
either - it's displayed as online, but *no* actions in the UI can be
performed anymore.

I'm not sure what other implications a key/cert mismatch has, but
since it requires the user to log in via SSH, manually delete the
mismatching key/cert pair, and then running `pvecm updatecerts -f`.

Therefore I feel like this is rather important to include in PVE 7.4,
so if there are any open questions/issues with this patch, I'd gladly
answer/fix/update/etc. anything if necessary.


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

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

[Thomas Lamprecht]

Am 22/03/2023 um 16:41 schrieb Max Carrara:
> On 3/14/23 16:08, 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>
>> ---
>>  NOTE: This patch requies a version bump+upload of pve-common.
>>
> 

shortly sumarizing what we talked off-list yesterday.
 

> So, it's now easier to lock oneself out of their own PVE instance, imo.

1) not really relevant, as we always recommend an additional out-of-band
channel, relevant for *every* update, which could, even if quite unlikely,
introduce a setup specific regression that renders the API inaccessible
too, maybe even without any user interaction required. There's a reason
that servers got OOB management stacks like IPMI/iKVM/...
2) getting a cert+key now and manually uploading it is quite uncommon
nowadays, especially for inexperienced users thanks to ACME and projects
like Let's Encrypt. Note also that we documented how to manually switch
the cert since years (maybe even over a decade) and I don't have any
reports of this error happening to users, definitively not frequently
and that even before automatic ACME integration existed.

> 
> Additionally, if the host with the invalid key/cert pair is in a
> cluster, it cannot be accessed via another host in the same cluster
> either - it's displayed as online, but *no* actions in the UI can be
> performed anymore.
> 
> I'm not sure what other implications a key/cert mismatch has, but
> since it requires the user to log in via SSH, manually delete the
> mismatching key/cert pair, and then running `pvecm updatecerts -f`.

yes setting up wrong certificates, independent if done via UI or via
API/CLI, which was always possible, or even the filesystem directly
(nothing you can check there), needs to be cleaned up - nothing new
there...

> 
> Therefore I feel like this is rather important to include in PVE 7.4,
> so if there are any open questions/issues with this patch, I'd gladly
> answer/fix/update/etc. anything if necessary.
> 

Manual certificate uploads are a bit of a niche use case especially since
we have full ACME implementation and switching certs out of any HTTP
related stack, be it PVE, a nginx/apache/... or whatever else there is,
has always needed some care - and an out-of-band channel besides the
thing one is currently changing (API access here). 

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