[RFC cluster/datacenter-manager/manager/proxmox 00/17] TLS Certificate Staging

[RFC cluster/datacenter-manager/manager/proxmox 00/17] TLS Certificate Staging

[Shannon Sterz]

the aim of this series is to allow clients to automatically adapt to regular
certificate rotation. the top-level overview of the mechanism proposed here is
as follows:

- hosts that rotate their certificate create a new certificate at the earliest
  four weeks before their current certificate expires. this certificate is
  considered as "staged" up until it becomes actively used.
- clients can query a host for a staged certificate at any moment, the host
  will provide information such as the fingerprint for the active and staged
  certificate(s).
- at the earliest two weeks before their current certificate expires, hosts may
  start using the "staged" certificate. the two week window is needed to give
  clients enough time to query a potential staged certificate.
- clients, that use fingerprints to validate a TLS certificate, should discard
  the previously used fingerprint and update to the new certificate's
  fingerprint (the previously staged certificate) as soon as they detect its
  usage. connections trying to authenticate themselves with the old Certificate
  should be rejected at this point.

this series implements the host part of this mechanism for pve 9 and pdm. the
first three patches in the series are intended for pve and implement the
staging mechanism. they also make it a little easier to query the certificate
of a node when we don't know the node name specifically.

the next few patches improve how fingerprints are handled for the
proxmox-client and pdm specifically. they also add the certificate info
endpoint to the pve client. specifically the following improvements are
provided:

* if a fingerprint is provided, never fall back to the system's trust store.
  providing proper pinning semantics (patches 4 and 6)
* if a fingerprint of a remote does not match, but pdm-client is in interactive
  mode, allow a user to accept the updated fingerprint then and there. this
  better matches the behaviour in interactive mode of connecting to a
  non-trusted node (patch 7).
* report mismatching fingerprints as untrusted when probing a remote and
  improve how the ui handles such situations by adding more context (patches
  9-11)

the remaining commits mostly prepare and the implement the rotation mechanism
within pdm. pdm will query pve remote nodes once every twelve hours to see if a
new staged certificate becomes available. if a new fingerprint is encountered,
it will be stored in the remotes.cfg. once a staged fingerprint is encountered,
it will replace the active fingerprint.

How to Test
-----------

the easiest way is probably to force pve to rotate and stage certificates by
setting a date with `date --set` that's far enough in the future to trigger the
action and then running `pveupdate`. to force pdm to query its remotes, it's
easiest to run `systemctl restart proxmox-datacenter-api.service`. the daemon
will execute the task query its remotes once on start.

How to Apply & Bump
-------------------

the first patch for pve-manager (02/17) depends on the changes for pve-cluster
(01/17). the second pve-manager patch can be applied independently.

the patches for proxmox-datacenter-manager can all be applied independently,
with the exception of the last one (17/17), which needs the patch for the
pve-api-types (05/17) to be applied and bumped.

Future Work
-----------

1. pbs remotes currently do not rotate their certificates. a series that is as
of yet not applied would add such a mechanism to pbs too. for now pbs remotes
are ignored by the staged certificates mechanism for the most part.

2. backporting of the pve patches to the bookworm branch probably makes sense
to improve compatibility. i'll send such patches once this series is.

3. somewhat orthogonal to this series: the mechanism outlined in the notes of
patch 16 would probably improve adding tasks to pdm.


pve-cluster:

Shannon Sterz (1):
  setup: allow caller to provide the certificate filename

 src/PVE/Cluster/Setup.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


pve-manager:

Shannon Sterz (2):
  bin/api: add a new staged certificate when renewing self-signed cert
  api: certificates: if node parameter is 'localhost' return local certs

 PVE/API2/Certificates.pm | 10 +++++++--
 PVE/CertHelpers.pm       |  6 ++++++
 bin/pveupdate            | 44 ++++++++++++++++++++++++++++++++--------
 3 files changed, 49 insertions(+), 11 deletions(-)


proxmox:

Shannon Sterz (2):
  client: ignore certificate trust store validation result on fp option
  pve-api-types: expose certificates info endpoint

 proxmox-client/src/client.rs        |  5 +----
 pve-api-types/Cargo.toml            |  1 +
 pve-api-types/generate.pl           |  3 +++
 pve-api-types/src/generated/code.rs | 15 ++++++++++++++-
 pve-api-types/src/types/mod.rs      |  1 +
 5 files changed, 20 insertions(+), 5 deletions(-)


proxmox-datacenter-manager:

Shannon Sterz (12):
  client: don't short-circuit on valid certificate when tls fp exists
  client: allow users to update a changed fingerprint interactively
  cli/api-types: move Fingerprint to common api type crate
  server: connection: report mismatching fingerprint as untrusted on
    probe
  ui: wizzard: add context if a provided fingerprint did not match
    remote
  ui: wizzard: nodes page: always update fingerprints on user
    confirmation
  pdm-api-types: implement ApiType for Fingerprint
  pdm-api-types: add staged_fingerprints field to NodeUrl
  server: remotes: lock remotes config when updating it
  server: connection: rotate in staged fingerprints when encountering
    them
  server: api: tasks: move `spawn_aborted_on_shutdown()` to super module
  server: bin: api: tasks: add task to discover new staged certificates

 cli/client/src/env/fingerprint_cache.rs       |  91 ++--------
 cli/client/src/env/mod.rs                     |  10 +-
 cli/client/src/main.rs                        |   6 +-
 lib/pdm-api-types/Cargo.toml                  |   1 +
 lib/pdm-api-types/src/fingerprint.rs          |  84 +++++++++
 lib/pdm-api-types/src/lib.rs                  |   3 +
 lib/pdm-api-types/src/remotes.rs              |  15 +-
 server/src/api/pbs/mod.rs                     |   2 +
 server/src/api/pve/mod.rs                     |   3 +
 server/src/api/remotes/mod.rs                 |  28 ++-
 server/src/bin/proxmox-datacenter-api/main.rs |   1 +
 .../tasks/ceph_detection.rs                   |  18 +-
 .../bin/proxmox-datacenter-api/tasks/mod.rs   |  17 ++
 .../tasks/remote_staged_fingerprints.rs       | 149 ++++++++++++++++
 server/src/connection.rs                      | 166 +++++++++++++++---
 ui/src/remotes/config.rs                      |   1 +
 ui/src/remotes/node_url_list.rs               |   1 +
 ui/src/remotes/wizard_page_connect.rs         |  26 ++-
 ui/src/remotes/wizard_page_info.rs            |   1 +
 ui/src/remotes/wizard_page_nodes.rs           |  40 ++++-
 20 files changed, 525 insertions(+), 138 deletions(-)
 create mode 100644 lib/pdm-api-types/src/fingerprint.rs
 create mode 100644 server/src/bin/proxmox-datacenter-api/tasks/remote_staged_fingerprints.rs


Summary over all repositories:
  29 files changed, 596 insertions(+), 156 deletions(-)

-- 
Generated by murpp 0.10.0

[PATCH cluster 01/17] setup: allow caller to provide the certificate filename

[Shannon Sterz]

so a caller can optionally generate a certificate in a different path
than the default certificate.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 src/PVE/Cluster/Setup.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Cluster/Setup.pm b/src/PVE/Cluster/Setup.pm
index 53935dc..b6a0d83 100644
--- a/src/PVE/Cluster/Setup.pm
+++ b/src/PVE/Cluster/Setup.pm
@@ -485,12 +485,12 @@ sub update_serial {
 }
 
 sub gen_pve_ssl_cert {
-    my ($force, $nodename, $ip) = @_;
+    my ($force, $nodename, $ip, $pvessl_cert_fn) = @_;
 
     die "no node name specified" if !$nodename;
     die "no IP specified" if !$ip;
 
-    my $pvessl_cert_fn = "$pmxcfs_base_dir/nodes/$nodename/pve-ssl.pem";
+    $pvessl_cert_fn //= "$pmxcfs_base_dir/nodes/$nodename/pve-ssl.pem";
 
     return if !$force && -f $pvessl_cert_fn;
 
-- 
2.47.3

[PATCH manager 02/17] bin/api: add a new staged certificate when renewing self-signed cert

[Shannon Sterz]

when a certificate is about to expire (within the next four weeks, up
to two weeks before it expires), create a new staged "next"
certificate. by including this certificate when querying the api,
clients can prepare for when the certificate is about to be rotated.
allowing them to update to the new fingerprint, without needing an
additional out-of-band communication channel.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 PVE/API2/Certificates.pm |  5 ++++-
 PVE/CertHelpers.pm       |  6 ++++++
 bin/pveupdate            | 44 ++++++++++++++++++++++++++++++++--------
 3 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/PVE/API2/Certificates.pm b/PVE/API2/Certificates.pm
index de8762c53..bcd650c3b 100644
--- a/PVE/API2/Certificates.pm
+++ b/PVE/API2/Certificates.pm
@@ -70,7 +70,10 @@ __PACKAGE__->register_method({
 
         my $res = [];
         my $cert_paths = [
-            '/etc/pve/pve-root-ca.pem', "$node_path/pve-ssl.pem", "$node_path/pveproxy-ssl.pem",
+            '/etc/pve/pve-root-ca.pem',
+            "$node_path/pve-ssl.pem",
+            "$node_path/pveproxy-ssl.pem",
+            "$node_path/pve-ssl-next.pem",
         ];
         for my $path (@$cert_paths) {
             eval {
diff --git a/PVE/CertHelpers.pm b/PVE/CertHelpers.pm
index 202dec0ee..4272eaf57 100644
--- a/PVE/CertHelpers.pm
+++ b/PVE/CertHelpers.pm
@@ -55,6 +55,12 @@ sub default_cert_path_prefix {
     return "/etc/pve/nodes/${node}/pve-ssl";
 }
 
+sub default_next_cert_path_prefix {
+    my ($node) = @_;
+
+    return "/etc/pve/nodes/${node}/pve-ssl-next";
+}
+
 sub cert_lock {
     my ($timeout, $code, @param) = @_;
 
diff --git a/bin/pveupdate b/bin/pveupdate
index b1960c353..0842ec893 100755
--- a/bin/pveupdate
+++ b/bin/pveupdate
@@ -103,10 +103,12 @@ syslog('err', "Renewing ACME certificate failed: $@") if $@;
 
 eval {
     my $certpath = PVE::CertHelpers::default_cert_path_prefix($nodename) . ".pem";
+    my $next_certpath = PVE::CertHelpers::default_next_cert_path_prefix($nodename) . ".pem";
     my $capath = "/etc/pve/pve-root-ca.pem";
+    my $now = time();
 
     my $renew = sub {
-        my ($msg) = @_;
+        my ($msg, $use_later) = @_;
 
         # get CA info
         my $cainfo = PVE::Certificate::get_certificate_info($capath);
@@ -127,19 +129,43 @@ eval {
         print "PVE certificate $msg\n";
         # create new certificate
         my $ip = PVE::Cluster::remote_node_ip($nodename);
-        PVE::Cluster::Setup::gen_pve_ssl_cert(1, $nodename, $ip);
 
-        print "Restarting pveproxy after renewing certificate\n";
-        PVE::Tools::run_command(['systemctl', 'reload-or-restart', 'pveproxy']);
+        if ($use_later) {
+            PVE::Cluster::Setup::gen_pve_ssl_cert(1, $nodename, $ip, $next_certpath);
+        } else {
+            PVE::Cluster::Setup::gen_pve_ssl_cert(1, $nodename, $ip);
+            print "Restarting pveproxy after renewing certificate\n";
+            PVE::Tools::run_command(['systemctl', 'reload-or-restart', 'pveproxy']);
+        }
     };
 
-    if (PVE::Certificate::check_expiry($certpath)) {
+    if (PVE::Certificate::check_expiry($certpath, $now)) {
         # already expired
         $renew->("expired, renewing...");
-    } elsif (PVE::Certificate::check_expiry($certpath, time() + 14 * 24 * 60 * 60)) {
-        # expires in next 2 weeks
-        $renew->("expires soon, renewing...");
-    } elsif (!PVE::Certificate::check_expiry($certpath, time() + 2 * 365 * 24 * 60 * 60)) {
+    } elsif (PVE::Certificate::check_expiry($certpath, $now + 14 * 24 * 60 * 60)) {
+        # expires in next 2 weeks, renew certificate if no "next" certificate already exists.
+        # rotate in the next certificate if it exists and does not expire.
+        if (
+            -f $next_certpath
+            && !PVE::Certificate::check_expiry($next_certpath, $now + 14 * 24 * 60 * 60)
+        ) {
+            rename($next_certpath, $certpath);
+            print "Restarting pveproxy after rotating certificate\n";
+            PVE::Tools::run_command(['systemctl', 'reload-or-restart', 'pveproxy']);
+        } else {
+            $renew->("expires soon, renewing...");
+
+            # clean up the now useless staged certificate, if there is one
+            if (-f $next_certpath) {
+                unlink $next_certpath
+                    or $!{ENOENT}
+                    or warn "failed to clean-up $next_certpath - $!\n";
+            }
+        }
+    } elsif (PVE::Certificate::check_expiry($certpath, $now + 28 * 24 * 60 * 60)) {
+        # expires in the next 4 weeks, create new cert, but don't use it yet
+        $renew->("staging new certificate...", 1);
+    } elsif (!PVE::Certificate::check_expiry($certpath, $now + 2 * 365 * 24 * 60 * 60)) {
         # expires in more than 2 years
         $renew->(
             "expires in more than 2 years, renewing to reduce certificate life-span for client compatibility..."
-- 
2.47.3

[PATCH manager 03/17] api: certificates: if node parameter is 'localhost' return local certs

[Shannon Sterz]

this aligns with many other api endpoints under `/nodes` where
`localhost` means "act as if i specified the node that is currently
handling this API request". previously this would only return the ca
certificate, as that's the only one that had the same path on all
nodes.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 PVE/API2/Certificates.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Certificates.pm b/PVE/API2/Certificates.pm
index bcd650c3b..732f98829 100644
--- a/PVE/API2/Certificates.pm
+++ b/PVE/API2/Certificates.pm
@@ -66,7 +66,10 @@ __PACKAGE__->register_method({
     code => sub {
         my ($param) = @_;
 
-        my $node_path = "/etc/pve/nodes/$param->{node}";
+        my $node = extract_param($param, 'node');
+        $node = PVE::INotify::nodename() if $node eq 'localhost';
+
+        my $node_path = "/etc/pve/nodes/$node";
 
         my $res = [];
         my $cert_paths = [
-- 
2.47.3

[PATCH proxmox 04/17] client: ignore certificate trust store validation result on fp option

[Shannon Sterz]

when a fingerprint is provided, the assumption is, that it acts like a
pinned certificate [1]. by short-circuiting if the certificate is
trusted by the system's trust store (meaning, returning `true` if the
`valid` parameter to the verify callback is `true`), this assumption
is broken. any certificate, whether the fingerprint matches or not,
that is trusted by the system's trust store is considered valid.

[1]: https://cheatsheetseries.owasp.org/cheatsheets/Pinning_Cheat_Sheet.html#what-is-pinning

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 proxmox-client/src/client.rs | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/proxmox-client/src/client.rs b/proxmox-client/src/client.rs
index 26913dbb..7617fee2 100644
--- a/proxmox-client/src/client.rs
+++ b/proxmox-client/src/client.rs
@@ -109,10 +109,7 @@ impl Client {
             TlsOptions::Verify => (),
             TlsOptions::Insecure => connector.set_verify(SslVerifyMode::NONE),
             TlsOptions::Fingerprint(expected_fingerprint) => {
-                connector.set_verify_callback(SslVerifyMode::PEER, move |valid, chain| {
-                    if valid {
-                        return true;
-                    }
+                connector.set_verify_callback(SslVerifyMode::PEER, move |_valid, chain| {
                     verify_fingerprint(chain, &expected_fingerprint)
                 });
             }
-- 
2.47.3

[PATCH proxmox 05/17] pve-api-types: expose certificates info endpoint

[Shannon Sterz]

so the certificate information of a cluster node can be queried.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 pve-api-types/Cargo.toml            |  1 +
 pve-api-types/generate.pl           |  3 +++
 pve-api-types/src/generated/code.rs | 15 ++++++++++++++-
 pve-api-types/src/types/mod.rs      |  1 +
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/pve-api-types/Cargo.toml b/pve-api-types/Cargo.toml
index a56cfef4..4b57f855 100644
--- a/pve-api-types/Cargo.toml
+++ b/pve-api-types/Cargo.toml
@@ -21,6 +21,7 @@ serde = { workspace = true, features = [ "derive" ] }
 serde_json.workspace = true
 serde_plain.workspace = true
 #
+proxmox-acme-api.workspace = true
 proxmox-api-macro.workspace = true
 proxmox-apt-api-types.workspace = true
 proxmox-fixed-string.workspace = true
diff --git a/pve-api-types/generate.pl b/pve-api-types/generate.pl
index 6e987753..465bba68 100755
--- a/pve-api-types/generate.pl
+++ b/pve-api-types/generate.pl
@@ -723,6 +723,9 @@ api(POST => '/nodes/{node}/ceph/start', 'start_ceph_services', 'param-name' => '
 api(POST => '/nodes/{node}/ceph/stop', 'stop_ceph_services', 'param-name' => 'StopCephServices', 'output-type' => 'PveUpid');
 api(POST => '/nodes/{node}/ceph/restart', 'restart_ceph_services', 'param-name' => 'RestartCephServices', 'output-type' => 'PveUpid');
 
+# Certificates: Allow query a node's current certificates
+api(GET => '/nodes/{node}/certificates/info', 'certificates_info', 'output-type' => 'Vec<CertificateInfo>');
+
 # NOW DUMP THE CODE:
 #
 # We generate one file for API types, and one for API method calls.
diff --git a/pve-api-types/src/generated/code.rs b/pve-api-types/src/generated/code.rs
index c66df0d9..1b3484a8 100644
--- a/pve-api-types/src/generated/code.rs
+++ b/pve-api-types/src/generated/code.rs
@@ -157,7 +157,6 @@
 /// - /nodes/{node}/certificates/acme
 /// - /nodes/{node}/certificates/acme/certificate
 /// - /nodes/{node}/certificates/custom
-/// - /nodes/{node}/certificates/info
 /// - /nodes/{node}/disks
 /// - /nodes/{node}/disks/directory
 /// - /nodes/{node}/disks/directory/{name}
@@ -350,6 +349,11 @@ pub trait PveClient {
         Err(Error::Other("ceph_osd_scrub not implemented"))
     }
 
+    /// Get information about node's certificates.
+    async fn certificates_info(&self, node: &str) -> Result<Vec<CertificateInfo>, Error> {
+        Err(Error::Other("certificates_info not implemented"))
+    }
+
     /// get the status of all ceph flags
     async fn cluster_ceph_flags(&self) -> Result<Vec<CephFlagInfo>, Error> {
         Err(Error::Other("cluster_ceph_flags not implemented"))
@@ -1870,6 +1874,15 @@ where
         self.0.post(url, &params).await?.nodata()
     }
 
+    /// Get information about node's certificates.
+    async fn certificates_info(&self, node: &str) -> Result<Vec<CertificateInfo>, Error> {
+        let url = &format!(
+            "/api2/extjs/nodes/{}/certificates/info",
+            percent_encode(node.as_bytes(), percent_encoding::NON_ALPHANUMERIC)
+        );
+        Ok(self.0.get(url).await?.expect_json()?.data)
+    }
+
     /// get the status of all ceph flags
     async fn cluster_ceph_flags(&self) -> Result<Vec<CephFlagInfo>, Error> {
         let url = "/api2/extjs/cluster/ceph/flags";
diff --git a/pve-api-types/src/types/mod.rs b/pve-api-types/src/types/mod.rs
index 22ded561..79016483 100644
--- a/pve-api-types/src/types/mod.rs
+++ b/pve-api-types/src/types/mod.rs
@@ -22,6 +22,7 @@ pub mod verifiers;
 
 use proxmox_fixed_string::FixedString;
 
+pub use proxmox_acme_api::CertificateInfo;
 pub use proxmox_apt_api_types::APTRepositoriesResult;
 
 include!("../generated/types.rs");
-- 
2.47.3

[PATCH datacenter-manager 06/17] client: don't short-circuit on valid certificate when tls fp exists

[Shannon Sterz]

if a user intentionally sets a fingerprint, the assumption is that
this acts similar to tls certificate pinning [1]. if we short-circuit
return on certificates trusted by the system's trust store (meaning,
if the `valid` parameter that is passed to the callback is true), this
assumption is broken. any certificate that is trusted by the
certificate store, whether there is one pinned for the corresponding
host or not, is trusted.

avoid this by not short-circuiting, but instead using that parameter
only if we don't have a fingerprint stored for a given host.

[1]: https://cheatsheetseries.owasp.org/cheatsheets/Pinning_Cheat_Sheet.html#what-is-pinning

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 cli/client/src/env/fingerprint_cache.rs | 5 +++--
 cli/client/src/env/mod.rs               | 8 ++++++--
 cli/client/src/main.rs                  | 6 +-----
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/cli/client/src/env/fingerprint_cache.rs b/cli/client/src/env/fingerprint_cache.rs
index b9a9f972..af48c353 100644
--- a/cli/client/src/env/fingerprint_cache.rs
+++ b/cli/client/src/env/fingerprint_cache.rs
@@ -125,6 +125,7 @@ impl FingerprintCache {
         &self,
         hostname: &str,
         chain: &mut X509StoreContextRef,
+        valid: bool,
     ) -> Result<VerifyResult, Error> {
         let cert = chain
             .current_cert()
@@ -142,8 +143,8 @@ impl FingerprintCache {
         let fp = Fingerprint::try_from(&*fp)
             .map_err(|_| format_err!("unexpected fingerprint length"))?;
 
-        if !self.interactive {
-            return Ok(VerifyResult::unmodified(false));
+        if !self.interactive || valid {
+            return Ok(VerifyResult::unmodified(valid));
         }
 
         println!("Certificate SHA256 fingerprint: {fp}");
diff --git a/cli/client/src/env/mod.rs b/cli/client/src/env/mod.rs
index 44a9388f..7c35e2a2 100644
--- a/cli/client/src/env/mod.rs
+++ b/cli/client/src/env/mod.rs
@@ -147,9 +147,13 @@ impl Env {
         Ok(())
     }
 
-    pub fn verify_cert(&self, chain: &mut x509::X509StoreContextRef) -> Result<bool, Error> {
+    pub fn verify_cert(
+        &self,
+        chain: &mut x509::X509StoreContextRef,
+        valid: bool,
+    ) -> Result<bool, Error> {
         let result = match self.connect_args.host.as_deref() {
-            Some(server) => self.fingerprint_cache.verify(server, chain)?,
+            Some(server) => self.fingerprint_cache.verify(server, chain, valid)?,
             None => return Ok(false),
         };
 
diff --git a/cli/client/src/main.rs b/cli/client/src/main.rs
index 4598a2bd..95b5b41f 100644
--- a/cli/client/src/main.rs
+++ b/cli/client/src/main.rs
@@ -47,11 +47,7 @@ pub fn client() -> Result<PdmClient<Client>, Error> {
     let address = env().url()?.parse()?;
 
     let options = TlsOptions::Callback(Box::new(|valid, store| {
-        if valid {
-            return true;
-        }
-
-        match env().verify_cert(store) {
+        match env().verify_cert(store, valid) {
             Ok(b) => b,
             Err(err) => {
                 eprintln!("failed to validate TLS certificate: {err}");
-- 
2.47.3

[PATCH datacenter-manager 07/17] client: allow users to update a changed fingerprint interactively

[Shannon Sterz]

if the client is operating in interactive mode and we notice that the
certificate of a remote has changed, the client now asks the user
whether the certificate should be updated.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 cli/client/src/env/fingerprint_cache.rs | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/cli/client/src/env/fingerprint_cache.rs b/cli/client/src/env/fingerprint_cache.rs
index af48c353..a4766ae9 100644
--- a/cli/client/src/env/fingerprint_cache.rs
+++ b/cli/client/src/env/fingerprint_cache.rs
@@ -137,16 +137,19 @@ impl FingerprintCache {
         };
 
         if let Some(stored_fp) = self.entries.read().unwrap().get(hostname) {
-            return Ok(VerifyResult::unmodified(**stored_fp == *fp));
+            if self.interactive && **stored_fp != *fp {
+                println!("Certificate fingerprint has changed!");
+                println!("Old certificate SHA256 fingerprint: {stored_fp}");
+            } else {
+                return Ok(VerifyResult::unmodified(**stored_fp == *fp));
+            }
+        } else if !self.interactive || valid {
+            return Ok(VerifyResult::unmodified(valid));
         }
 
         let fp = Fingerprint::try_from(&*fp)
             .map_err(|_| format_err!("unexpected fingerprint length"))?;
 
-        if !self.interactive || valid {
-            return Ok(VerifyResult::unmodified(valid));
-        }
-
         println!("Certificate SHA256 fingerprint: {fp}");
 
         let mut stdout = std::io::stdout();
-- 
2.47.3

[PATCH datacenter-manager 08/17] cli/api-types: move Fingerprint to common api type crate

[Shannon Sterz]

this type is more generally useful when working with certificates, so
move it to the common pdm api types.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 cli/client/src/env/fingerprint_cache.rs | 77 +-----------------------
 cli/client/src/env/mod.rs               |  2 +-
 lib/pdm-api-types/Cargo.toml            |  1 +
 lib/pdm-api-types/src/fingerprint.rs    | 78 +++++++++++++++++++++++++
 lib/pdm-api-types/src/lib.rs            |  3 +
 5 files changed, 84 insertions(+), 77 deletions(-)
 create mode 100644 lib/pdm-api-types/src/fingerprint.rs

diff --git a/cli/client/src/env/fingerprint_cache.rs b/cli/client/src/env/fingerprint_cache.rs
index a4766ae9..2040b2cf 100644
--- a/cli/client/src/env/fingerprint_cache.rs
+++ b/cli/client/src/env/fingerprint_cache.rs
@@ -6,82 +6,7 @@ use anyhow::{Error, bail, format_err};
 use openssl::hash::MessageDigest;
 use openssl::x509::X509StoreContextRef;
 
-/// A sha256 fingerprint.
-// NOTE: The difference to ConfigDigest is that this also allows colons between bytes when parsing.
-// Also the API type's description is different.
-#[derive(Clone, Debug, Eq, PartialEq)]
-pub struct Fingerprint([u8; 32]);
-serde_plain::derive_deserialize_from_fromstr!(Fingerprint, "valid sha256 fingerprint");
-serde_plain::derive_serialize_from_display!(Fingerprint);
-
-impl From<[u8; 32]> for Fingerprint {
-    #[inline]
-    fn from(fp: [u8; 32]) -> Self {
-        Self(fp)
-    }
-}
-
-impl From<Fingerprint> for [u8; 32] {
-    #[inline]
-    fn from(fp: Fingerprint) -> Self {
-        fp.0
-    }
-}
-
-impl TryFrom<&[u8]> for Fingerprint {
-    type Error = std::array::TryFromSliceError;
-
-    fn try_from(slice: &[u8]) -> Result<Self, Self::Error> {
-        Ok(Self(slice.try_into()?))
-    }
-}
-
-impl AsRef<[u8]> for Fingerprint {
-    fn as_ref(&self) -> &[u8] {
-        &self.0
-    }
-}
-
-impl AsRef<[u8; 32]> for Fingerprint {
-    fn as_ref(&self) -> &[u8; 32] {
-        &self.0
-    }
-}
-
-impl std::ops::Deref for Fingerprint {
-    type Target = [u8; 32];
-
-    fn deref(&self) -> &[u8; 32] {
-        &self.0
-    }
-}
-
-impl std::ops::DerefMut for Fingerprint {
-    fn deref_mut(&mut self) -> &mut [u8; 32] {
-        &mut self.0
-    }
-}
-
-impl std::fmt::Display for Fingerprint {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{:02x}", self[0])?;
-        for b in &self[1..] {
-            write!(f, ":{b:02x}")?;
-        }
-        Ok(())
-    }
-}
-
-impl std::str::FromStr for Fingerprint {
-    type Err = Error;
-
-    fn from_str(s: &str) -> Result<Self, Error> {
-        let s = s.replace(':', "");
-        let mut fp = [0u8; 32];
-        hex::decode_to_slice(s, &mut fp)?;
-        Ok(Fingerprint(fp))
-    }
-}
+use pdm_api_types::Fingerprint;
 
 pub struct FingerprintCache {
     pub interactive: bool,
diff --git a/cli/client/src/env/mod.rs b/cli/client/src/env/mod.rs
index 7c35e2a2..4c46e513 100644
--- a/cli/client/src/env/mod.rs
+++ b/cli/client/src/env/mod.rs
@@ -11,6 +11,7 @@ use http::Uri;
 use openssl::x509;
 use serde::{Deserialize, Serialize};
 
+pub use pdm_api_types::Fingerprint;
 use proxmox_auth_api::types::Userid;
 use proxmox_client::TfaChallenge;
 use proxmox_schema::api;
@@ -19,7 +20,6 @@ use crate::XDG;
 use crate::config::{FormatArgs, PdmConnectArgs};
 
 mod fingerprint_cache;
-pub use fingerprint_cache::Fingerprint;
 use fingerprint_cache::FingerprintCache;
 
 macro_rules! xdg_path {
diff --git a/lib/pdm-api-types/Cargo.toml b/lib/pdm-api-types/Cargo.toml
index f9e3d07e..e3a40934 100644
--- a/lib/pdm-api-types/Cargo.toml
+++ b/lib/pdm-api-types/Cargo.toml
@@ -8,6 +8,7 @@ description = "general API type helpers for PDM"
 [dependencies]
 anyhow.workspace = true
 const_format.workspace = true
+hex.workspace = true
 http.workspace = true
 regex.workspace = true
 serde.workspace = true
diff --git a/lib/pdm-api-types/src/fingerprint.rs b/lib/pdm-api-types/src/fingerprint.rs
new file mode 100644
index 00000000..ee7f9229
--- /dev/null
+++ b/lib/pdm-api-types/src/fingerprint.rs
@@ -0,0 +1,78 @@
+use anyhow::Error;
+
+/// A sha256 fingerprint.
+// NOTE: The difference to ConfigDigest is that this also allows colons between bytes when parsing.
+// Also the API type's description is different.
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct Fingerprint([u8; 32]);
+serde_plain::derive_deserialize_from_fromstr!(Fingerprint, "valid sha256 fingerprint");
+serde_plain::derive_serialize_from_display!(Fingerprint);
+
+impl From<[u8; 32]> for Fingerprint {
+    #[inline]
+    fn from(fp: [u8; 32]) -> Self {
+        Self(fp)
+    }
+}
+
+impl From<Fingerprint> for [u8; 32] {
+    #[inline]
+    fn from(fp: Fingerprint) -> Self {
+        fp.0
+    }
+}
+
+impl TryFrom<&[u8]> for Fingerprint {
+    type Error = std::array::TryFromSliceError;
+
+    fn try_from(slice: &[u8]) -> Result<Self, Self::Error> {
+        Ok(Self(slice.try_into()?))
+    }
+}
+
+impl AsRef<[u8]> for Fingerprint {
+    fn as_ref(&self) -> &[u8] {
+        &self.0
+    }
+}
+
+impl AsRef<[u8; 32]> for Fingerprint {
+    fn as_ref(&self) -> &[u8; 32] {
+        &self.0
+    }
+}
+
+impl std::ops::Deref for Fingerprint {
+    type Target = [u8; 32];
+
+    fn deref(&self) -> &[u8; 32] {
+        &self.0
+    }
+}
+
+impl std::ops::DerefMut for Fingerprint {
+    fn deref_mut(&mut self) -> &mut [u8; 32] {
+        &mut self.0
+    }
+}
+
+impl std::fmt::Display for Fingerprint {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{:02x}", self[0])?;
+        for b in &self[1..] {
+            write!(f, ":{b:02x}")?;
+        }
+        Ok(())
+    }
+}
+
+impl std::str::FromStr for Fingerprint {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self, Error> {
+        let s = s.replace(':', "");
+        let mut fp = [0u8; 32];
+        hex::decode_to_slice(s, &mut fp)?;
+        Ok(Fingerprint(fp))
+    }
+}
diff --git a/lib/pdm-api-types/src/lib.rs b/lib/pdm-api-types/src/lib.rs
index b9cc3234..5c86cb29 100644
--- a/lib/pdm-api-types/src/lib.rs
+++ b/lib/pdm-api-types/src/lib.rs
@@ -106,6 +106,9 @@ pub mod ceph;
 
 pub mod firewall;
 
+mod fingerprint;
+pub use fingerprint::Fingerprint;
+
 pub mod remotes;
 
 pub mod remote_updates;
-- 
2.47.3

[PATCH datacenter-manager 09/17] server: connection: report mismatching fingerprint as untrusted on probe

[Shannon Sterz]

instead of erroring out. previously this function returned a
connection error if the provided fingerprint did not match the
remote's fingerprint. instead, report the certificate as untrusted,
giving client's more appropriate information in such cases.

note that while the documentation for this function was technically
correct, the probe_tls endpoints for pve and pbs remotes stated:

> If the certificate is not trusted with the given parameters, returns
> the certificate information.

however, that was incorrect, since the endpoints returned an error if
the fingerprint did not match. since those two endpoints are currently
the only users that could actually provide a fingerprint (all other
callers explicitly provide `None`), this is more of a bug fix than a
public api break.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 server/src/connection.rs | 62 ++++++++++++++++++++++++++++------------
 1 file changed, 43 insertions(+), 19 deletions(-)

diff --git a/server/src/connection.rs b/server/src/connection.rs
index 7ad1a5b9..f8b277da 100644
--- a/server/src/connection.rs
+++ b/server/src/connection.rs
@@ -15,12 +15,14 @@ use std::time::{Duration, SystemTime};
 use anyhow::{Error, bail, format_err};
 use http::Method;
 use http::uri::Authority;
+use openssl::hash::MessageDigest;
 use openssl::x509::X509StoreContextRef;
 use serde::Serialize;
 
 use proxmox_acme_api::CertificateInfo;
 use proxmox_client::{Client, HttpApiClient, HttpApiResponse, HttpApiResponseStream, TlsOptions};
 
+use pdm_api_types::Fingerprint;
 use pdm_api_types::remotes::{NodeUrl, Remote, RemoteType, TlsProbeOutcome};
 use pve_api_types::client::PveClientImpl;
 
@@ -867,7 +869,7 @@ impl HttpApiClient for MultiClient {
 /// Checks TLS connection to the given remote
 ///
 /// Returns `Ok(TlsProbeOutcome::TrustedCertificate)` if connecting with the given parameters works
-/// Returns `Ok(TlsProbeOutcome::UntrustedCertificate)` if no fingerprint was given and some certificate could not be validated
+/// Returns `Ok(TlsProbeOutcome::UntrustedCertificate)` if the provided fingerprint does not match or a certificate could not be validated
 /// Returns `Err(err)` if some other error occurred
 ///
 /// # Example
@@ -902,25 +904,47 @@ pub async fn probe_tls_connection(
     // to save the invalid cert we find
     let invalid_cert = Arc::new(StdMutex::new(None));
 
-    let options = if let Some(fp) = &fingerprint {
-        TlsOptions::parse_fingerprint(fp)?
-    } else {
-        TlsOptions::Callback(Box::new({
-            let invalid_cert = invalid_cert.clone();
-            move |valid: bool, chain: &mut X509StoreContextRef| {
-                if let Some(cert) = chain.current_cert() {
-                    if !valid {
-                        let cert = cert
-                            .to_pem()
-                            .map_err(Error::from)
-                            .and_then(|pem| CertificateInfo::from_pem("", &pem));
-                        *invalid_cert.lock().unwrap() = Some(cert);
-                    }
-                }
-                true
+    let fingerprint = fingerprint
+        .map(|fp| fp.parse::<Fingerprint>())
+        .transpose()?;
+
+    let options = TlsOptions::Callback(Box::new({
+        let invalid_cert = invalid_cert.clone();
+        move |valid: bool, chain: &mut X509StoreContextRef| {
+            // If no fingerprint was provided and the trust store trusts the certificate, the
+            // connection is valid.
+            if fingerprint.is_none() && valid {
+                return true;
             }
-        }))
-    };
+
+            let Some(cert) = chain.current_cert() else {
+                return true;
+            };
+
+            // If a fingerprint was provided and the certificate matches it, the connection is
+            // valid.
+            if let Some(provided_fp) = &fingerprint {
+                if cert
+                    .digest(MessageDigest::sha256())
+                    .map(|fp| *fp == **provided_fp)
+                    .unwrap_or(false)
+                {
+                    return true;
+                }
+            }
+
+            // Otherwise, the certificate is not trusted.
+            let cert = cert
+                .to_pem()
+                .map_err(Error::from)
+                .and_then(|pem| CertificateInfo::from_pem("", &pem));
+
+            *invalid_cert.lock().unwrap() = Some(cert);
+
+            true
+        }
+    }));
+
     let client = proxmox_client::Client::with_options(uri, options, Default::default())?;
 
     // set fake auth info. we don't need any, but the proxmox client will return unauthenticated if
-- 
2.47.3

[PATCH datacenter-manager 10/17] ui: wizzard: add context if a provided fingerprint did not match remote

[Shannon Sterz]

if a remote's certificate does not match the fingerprint provided by a
user, add a warning to the probe result dialogs.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 ui/src/remotes/wizard_page_connect.rs | 26 ++++++++++++++++--
 ui/src/remotes/wizard_page_nodes.rs   | 38 +++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/ui/src/remotes/wizard_page_connect.rs b/ui/src/remotes/wizard_page_connect.rs
index abb2b1da..59031892 100644
--- a/ui/src/remotes/wizard_page_connect.rs
+++ b/ui/src/remotes/wizard_page_connect.rs
@@ -6,9 +6,9 @@ use serde_json::Value;
 use yew::html::IntoEventCallback;
 use yew::virtual_dom::{Key, VComp, VNode};
 
-use pwt::css::{FlexFit, JustifyContent};
+use pwt::css::{FlexFit, FontColor, JustifyContent};
 use pwt::widget::form::{Field, FormContext, FormContextObserver};
-use pwt::widget::{Button, Column, Container, Dialog, InputPanel, Mask, Row, error_message};
+use pwt::widget::{Button, Column, Container, Dialog, Fa, InputPanel, Mask, Row, error_message};
 use pwt::{AsyncAbortGuard, prelude::*};
 use pwt_macros::builder;
 
@@ -81,6 +81,14 @@ impl PdmWizardPageConnect {
             Some(Ok(TlsProbeOutcome::UntrustedCertificate(info))) => info.clone(),
             _ => return None,
         };
+
+        let provided_fp = ctx
+            .props()
+            .info
+            .form_ctx
+            .read()
+            .get_field_text("fingerprint");
+
         Some(
             Dialog::new(tr!("Connection Certificate"))
                 .on_close(link.callback(|_| Msg::ConfirmResult(false)))
@@ -92,6 +100,20 @@ impl PdmWizardPageConnect {
                         .with_child(Container::new().with_child(tr!(
                             "The certificate of the remote server is not trusted."
                         )))
+                        .with_optional_child((!provided_fp.is_empty()).then(||{
+                            Row::new()
+                                .gap(1)
+                                .margin_x(1)
+                                .with_child(
+                                    Fa::new("exclamation-triangle")
+                                        .class(FontColor::Warning)
+                                )
+                                .with_child(tr!(
+                                    "The provided SHA-256 fingerprint ({provided_fp}) did not match \
+                                    the remote certificate's fingerprint.",
+                                    provided_fp
+                                ))
+                        }))
                         .with_child(Container::new().with_child(tr!(
                             "Do you want to trust the certificate and save its fingerprint?"
                         )))
diff --git a/ui/src/remotes/wizard_page_nodes.rs b/ui/src/remotes/wizard_page_nodes.rs
index 15f60404..3428338d 100644
--- a/ui/src/remotes/wizard_page_nodes.rs
+++ b/ui/src/remotes/wizard_page_nodes.rs
@@ -6,8 +6,8 @@ use proxmox_schema::property_string::PropertyString;
 use serde_json::Value;
 use yew::virtual_dom::{VComp, VNode};
 
-use pwt::css::{FlexFit, FontStyle, JustifyContent, Overflow};
-use pwt::widget::{Button, Column, Container, Dialog, Mask, Row, error_message};
+use pwt::css::{FlexFit, FontColor, FontStyle, JustifyContent, Overflow};
+use pwt::widget::{Button, Column, Container, Dialog, Fa, Mask, Row, error_message};
 use pwt::{AsyncAbortGuard, prelude::*};
 use pwt_macros::builder;
 
@@ -74,12 +74,46 @@ impl PdmWizardPageNodes {
                             .padding(2)
                             .class(Overflow::Auto)
                             .children(certificates.into_iter().map(|(hostname, certificate)| {
+                                let nodes =
+                                    ctx.props().info.form_ctx.read().get_field_value("nodes");
+
+                                let provided_fp = if let Some(Value::Array(nodes_list)) = nodes {
+                                    nodes_list
+                                        .into_iter()
+                                        .find_map(|node| {
+                                            serde_json::from_value::<PropertyString<NodeUrl>>(node)
+                                                .ok()
+                                                .and_then(|n| {
+                                                    let node = n.into_inner();
+                                                    (node.hostname == *hostname)
+                                                        .then_some(node.fingerprint)
+                                                })
+                                        })
+                                        .flatten()
+                                } else {
+                                    None
+                                };
+
                                 Column::new()
                                     .with_child(
                                         Container::new().class(FontStyle::TitleSmall).with_child(
                                             format!("{}: {hostname}", tr!("Server Address")),
                                         ),
                                     )
+                                    .with_optional_child(provided_fp.map(|fp| {
+                                        Row::new()
+                                            .gap(1)
+                                            .margin(2)
+                                            .with_child(
+                                                Fa::new("exclamation-triangle")
+                                                    .class(FontColor::Warning),
+                                            )
+                                            .with_child(tr!(
+                                                "The provided SHA-256 fingerprint ({fp}) did not \
+                                                match the remote certificate's fingerprint.",
+                                                fp
+                                            ))
+                                    }))
                                     .with_child(
                                         KVGrid::new()
                                             .class(FlexFit)
-- 
2.47.3

[PATCH datacenter-manager 11/17] ui: wizzard: nodes page: always update fingerprints on user confirmation

[Shannon Sterz]

if a user needs to confirm the fingerprint at this step, than the
dialog had invalid fingerprints set. since we queried this information
via at least one valid fingerprint from the remote itself, the most
likely cause is an accident on the users part. previously confirming
the "Connection Certificate" dialog would only update the form, if no
value was set for the fingerprint. after this change the values are
always updated to the probed values.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 ui/src/remotes/wizard_page_nodes.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/src/remotes/wizard_page_nodes.rs b/ui/src/remotes/wizard_page_nodes.rs
index 3428338d..a027c280 100644
--- a/ui/src/remotes/wizard_page_nodes.rs
+++ b/ui/src/remotes/wizard_page_nodes.rs
@@ -233,7 +233,7 @@ impl Component for PdmWizardPageNodes {
                     {
                         Ok(mut nodes) => {
                             for node in nodes.iter_mut() {
-                                if node.fingerprint.is_none() && map.contains_key(&node.hostname) {
+                                if map.contains_key(&node.hostname) {
                                     node.fingerprint =
                                         Some(map.get(&node.hostname).unwrap().to_uppercase());
                                 }
-- 
2.47.3

[PATCH datacenter-manager 12/17] pdm-api-types: implement ApiType for Fingerprint

[Shannon Sterz]

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 lib/pdm-api-types/src/fingerprint.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/pdm-api-types/src/fingerprint.rs b/lib/pdm-api-types/src/fingerprint.rs
index ee7f9229..0240058c 100644
--- a/lib/pdm-api-types/src/fingerprint.rs
+++ b/lib/pdm-api-types/src/fingerprint.rs
@@ -1,4 +1,5 @@
 use anyhow::Error;
+use proxmox_schema::ApiType;
 
 /// A sha256 fingerprint.
 // NOTE: The difference to ConfigDigest is that this also allows colons between bytes when parsing.
@@ -76,3 +77,8 @@ impl std::str::FromStr for Fingerprint {
         Ok(Fingerprint(fp))
     }
 }
+
+impl ApiType for Fingerprint {
+    const API_SCHEMA: proxmox_schema::Schema =
+        proxmox_schema::api_types::CERT_FINGERPRINT_SHA256_SCHEMA;
+}
-- 
2.47.3

[PATCH datacenter-manager 13/17] pdm-api-types: add staged_fingerprints field to NodeUrl

[Shannon Sterz]

and fix up all use sides as well as the update endpoint.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 lib/pdm-api-types/src/remotes.rs   | 15 ++++++++++++++-
 server/src/api/pbs/mod.rs          |  2 ++
 server/src/api/pve/mod.rs          |  3 +++
 server/src/api/remotes/mod.rs      | 27 ++++++++++++++++++++++++++-
 ui/src/remotes/config.rs           |  1 +
 ui/src/remotes/node_url_list.rs    |  1 +
 ui/src/remotes/wizard_page_info.rs |  1 +
 7 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/lib/pdm-api-types/src/remotes.rs b/lib/pdm-api-types/src/remotes.rs
index 50c7892e..0a2e8651 100644
--- a/lib/pdm-api-types/src/remotes.rs
+++ b/lib/pdm-api-types/src/remotes.rs
@@ -8,7 +8,7 @@ use proxmox_schema::{ApiType, Schema, StringSchema, Updater, api};
 use proxmox_section_config::typed::ApiSectionDataEntry;
 use proxmox_section_config::{SectionConfig, SectionConfigPlugin};
 
-use crate::{Authid, HOST_OPTIONAL_PORT_FORMAT};
+use crate::{Authid, Fingerprint, HOST_OPTIONAL_PORT_FORMAT};
 
 pub const REMOTE_ID_SCHEMA: Schema = StringSchema::new("Remote ID.")
     .format(&crate::PROXMOX_SAFE_ID_FORMAT)
@@ -26,11 +26,19 @@ pub const REMOTE_ID_SCHEMA: Schema = StringSchema::new("Remote ID.")
             format: &crate::FINGERPRINT_SHA256_FORMAT,
             optional: true,
         },
+        "staged-fingerprints": {
+            type: Array,
+            optional: true,
+            items: {
+                type: Fingerprint,
+            }
+        }
     },
     default_key: "hostname",
 )]
 /// A node and its certificate information.
 #[derive(Clone, Debug, Deserialize, Serialize, PartialEq)]
+#[serde(rename_all = "kebab-case")]
 pub struct NodeUrl {
     /// The node address.
     pub hostname: String,
@@ -38,6 +46,11 @@ pub struct NodeUrl {
     /// Certificate fingerprint.
     #[serde(skip_serializing_if = "Option::is_none")]
     pub fingerprint: Option<String>,
+
+    /// A list of staged fingerprints. If one of these is encountered while connecting to a node,
+    /// they'll replace the main certificate fingerprint. The connection will be deemed valid.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub staged_fingerprints: Option<Vec<Fingerprint>>,
 }
 
 #[api]
diff --git a/server/src/api/pbs/mod.rs b/server/src/api/pbs/mod.rs
index 1fc75c34..98a88a23 100644
--- a/server/src/api/pbs/mod.rs
+++ b/server/src/api/pbs/mod.rs
@@ -273,6 +273,7 @@ pub async fn scan_remote_pbs(
         nodes: vec![PropertyString::new(NodeUrl {
             hostname,
             fingerprint,
+            staged_fingerprints: None,
         })],
         authid: authid.clone(),
         token,
@@ -325,6 +326,7 @@ pub async fn list_realm_remote_pbs(
         nodes: vec![PropertyString::new(NodeUrl {
             hostname,
             fingerprint,
+            staged_fingerprints: None,
         })],
         authid: "root@pam".parse()?,
         token: String::new(),
diff --git a/server/src/api/pve/mod.rs b/server/src/api/pve/mod.rs
index 0970f2ff..64413a3e 100644
--- a/server/src/api/pve/mod.rs
+++ b/server/src/api/pve/mod.rs
@@ -483,6 +483,7 @@ pub async fn scan_remote_pve(
         nodes: vec![PropertyString::new(NodeUrl {
             hostname,
             fingerprint,
+            staged_fingerprints: None,
         })],
         authid: authid.clone(),
         token,
@@ -508,6 +509,7 @@ pub async fn scan_remote_pve(
         nodes.push(PropertyString::new(NodeUrl {
             hostname: node.node,
             fingerprint,
+            staged_fingerprints: None,
         }));
     }
 
@@ -574,6 +576,7 @@ pub async fn list_realm_remote_pve(
         nodes: vec![PropertyString::new(NodeUrl {
             hostname,
             fingerprint,
+            staged_fingerprints: None,
         })],
         authid: "root@pam".parse()?,
         token: String::new(),
diff --git a/server/src/api/remotes/mod.rs b/server/src/api/remotes/mod.rs
index e416f619..090a3b32 100644
--- a/server/src/api/remotes/mod.rs
+++ b/server/src/api/remotes/mod.rs
@@ -412,7 +412,32 @@ pub fn update_remote(
         }
     }
 
-    if let Some(v) = updater.nodes {
+    if let Some(mut v) = updater.nodes {
+        for node in &mut v {
+            // If the updater included staged fingerprints for the remote, update them.
+            if node.staged_fingerprints.is_some() {
+                continue;
+            }
+
+            // If not, keep the previous fingerprints intact.
+            let staged_fp = entry
+                .nodes
+                .iter()
+                .find_map(|n| {
+                    if n.hostname == node.hostname
+                        && n.fingerprint.as_ref().map(|f| f.to_lowercase())
+                            == node.fingerprint.as_ref().map(|f| f.to_lowercase())
+                    {
+                        return Some(n.staged_fingerprints.clone());
+                    }
+
+                    None
+                })
+                .flatten();
+
+            node.staged_fingerprints = staged_fp;
+        }
+
         entry.nodes = v;
     }
     if let Some(v) = updater.authid {
diff --git a/ui/src/remotes/config.rs b/ui/src/remotes/config.rs
index ea3c5bcd..0e272c2c 100644
--- a/ui/src/remotes/config.rs
+++ b/ui/src/remotes/config.rs
@@ -59,6 +59,7 @@ pub async fn create_remote(mut data: Value, remote_type: RemoteType) -> Result<(
         let nodes = vec![PropertyString::new(NodeUrl {
             hostname: data["hostname"].as_str().unwrap_or_default().to_string(),
             fingerprint: data["fingerprint"].as_str().map(|fp| fp.to_string()),
+            staged_fingerprints: None,
         })];
         data["nodes"] = serde_json::to_value(nodes)?;
     }
diff --git a/ui/src/remotes/node_url_list.rs b/ui/src/remotes/node_url_list.rs
index d18d8a23..4afc6482 100644
--- a/ui/src/remotes/node_url_list.rs
+++ b/ui/src/remotes/node_url_list.rs
@@ -220,6 +220,7 @@ impl ManagedField for PdmNodeUrlField {
                                 data: NodeUrl {
                                     hostname: String::new(),
                                     fingerprint: None,
+                                    staged_fingerprints: None,
                                 },
                             })
                         }
diff --git a/ui/src/remotes/wizard_page_info.rs b/ui/src/remotes/wizard_page_info.rs
index 1e56c4e1..4f8e3302 100644
--- a/ui/src/remotes/wizard_page_info.rs
+++ b/ui/src/remotes/wizard_page_info.rs
@@ -131,6 +131,7 @@ async fn scan(
             PropertyString::new(NodeUrl {
                 hostname,
                 fingerprint: fingerprint.map(|fp| fp.to_uppercase()),
+                staged_fingerprints: None,
             }),
         );
     }
-- 
2.47.3

[PATCH datacenter-manager 14/17] server: remotes: lock remotes config when updating it

[Shannon Sterz]

since we read the config, then check the digest, and after modifying
write it back out again, another thread could modify the config
between reading and writing. this could lead to inconcsistencies, so
make sure to lock the config.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 server/src/api/remotes/mod.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/server/src/api/remotes/mod.rs b/server/src/api/remotes/mod.rs
index 090a3b32..d581681f 100644
--- a/server/src/api/remotes/mod.rs
+++ b/server/src/api/remotes/mod.rs
@@ -395,6 +395,7 @@ pub fn update_remote(
     delete: Option<Vec<DeletableProperty>>,
     digest: Option<ConfigDigest>,
 ) -> Result<(), Error> {
+    let _lock = pdm_config::remotes::lock_config()?;
     let (mut remotes, config_digest) = pdm_config::remotes::config()?;
     config_digest.detect_modification(digest.as_ref())?;
 
-- 
2.47.3

[PATCH datacenter-manager 15/17] server: connection: rotate in staged fingerprints when encountering them

[Shannon Sterz]

if a staged fingerprint is encountered when connecting to a node, move
the new fingerprint into the active position. the staged fingerprints
will also be updated to remove the new fingerprint from the staged
list.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 server/src/connection.rs | 104 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 98 insertions(+), 6 deletions(-)

diff --git a/server/src/connection.rs b/server/src/connection.rs
index f8b277da..500ad6c6 100644
--- a/server/src/connection.rs
+++ b/server/src/connection.rs
@@ -53,18 +53,108 @@ impl ConnectInfo {
         }
     }
 }
-///
+
+/// Updates the current fingerprint of a node of a given remote. Will also remove the fingerprint
+/// from the staged fingerprint list.
+fn update_current_fp(
+    remote_id: &str,
+    hostname: &str,
+    current_fingerprint: &Option<Fingerprint>,
+    new_fingerprint: &Fingerprint,
+) -> Result<(), Error> {
+    let _lock = pdm_config::remotes::lock_config()?;
+    let (mut config, _digest) = pdm_config::remotes::config()?;
+
+    let Some(remote) = config.get_mut(remote_id) else {
+        log::debug!("Remote '{remote_id}' vanished while updating current fingerprint.");
+        return Ok(());
+    };
+
+    for node in &mut remote.nodes {
+        if node.hostname == *hostname
+            && node.fingerprint.as_ref().and_then(|f| f.parse().ok()) == *current_fingerprint
+        {
+            node.fingerprint = Some(new_fingerprint.to_string());
+
+            if let Some(staged_fingerprints) = &mut node.staged_fingerprints {
+                staged_fingerprints.retain(|f| f != new_fingerprint);
+            }
+        }
+    }
+
+    pdm_config::remotes::save_config(config)
+}
+
 /// Returns a [`proxmox_client::Client`] set up to connect to a specific node.
 fn prepare_connect_client_to_node(
+    remote_id: &str,
     node: &NodeUrl,
     default_port: u16,
     pve_compat: bool,
 ) -> Result<Client, Error> {
-    let mut options = TlsOptions::default();
+    let fingerprint = node
+        .fingerprint
+        .as_ref()
+        .map(|fp| fp.parse::<Fingerprint>())
+        .transpose()?;
 
-    if let Some(fp) = &node.fingerprint {
-        options = TlsOptions::parse_fingerprint(fp)?;
-    }
+    let options = TlsOptions::Callback(Box::new({
+        let remote_id = remote_id.to_owned();
+        let staged_fingerprints = node.staged_fingerprints.clone();
+        let node = node.hostname.clone();
+        move |valid: bool, chain: &mut X509StoreContextRef| {
+            // If we have no fingerprint and no staged fingerprints, fall back to the system's
+            // trust store.
+            if fingerprint.is_none() && staged_fingerprints.is_none() {
+                return valid;
+            }
+
+            let Some(cert) = chain.current_cert() else {
+                log::error!("Could not get current certificate when connecting to node '{node}'.");
+                return false;
+            };
+
+            let cert_fp = match cert.digest(MessageDigest::sha256()) {
+                // A valid SHA-256 digest is by definition a valid Fingerprint. So the `expect`
+                // below must always succeed.
+                Ok(fp) => Fingerprint::try_from(&*fp)
+                    .expect("Could not get fingerprint from SHA256 digest."),
+                Err(e) => {
+                    log::error!("Could not compute remote certificate digest: {e:#}");
+                    return false;
+                }
+            };
+
+            // If the stored fingerprint matches, the connection is valid.
+            if let Some(fingerprint) = &fingerprint {
+                if *fingerprint == cert_fp {
+                    return true;
+                }
+            }
+
+            // If we have staged fingerprints and one of them matches the certificate, promote it
+            // to the current fingerprint and mark the connection as valid.
+            if let Some(staged_fingerprints) = &staged_fingerprints {
+                if staged_fingerprints.contains(&cert_fp) {
+                    // Update active fingerprint; handle this in a separate task, since this
+                    // requires locking, reading and writing the remotes configuration. There is no
+                    // need to handle this while establishing a connection.
+                    let (remote_id, node, fingerprint) =
+                        (remote_id.clone(), node.clone(), fingerprint.clone());
+                    tokio::spawn(async move {
+                        if let Err(e) = update_current_fp(&remote_id, &node, &fingerprint, &cert_fp)
+                        {
+                            log::error!("Could not update current fingerprint: {e:#}");
+                        }
+                    });
+                    return true;
+                }
+            }
+
+            // Otherwise, the connection is invalid.
+            false
+        }
+    }));
 
     let host_port: Authority = node.hostname.parse()?;
 
@@ -101,7 +191,8 @@ fn prepare_connect_client(
 
     let info = ConnectInfo::for_remote(remote);
 
-    let client = prepare_connect_client_to_node(node, info.default_port, info.pve_compat)?;
+    let client =
+        prepare_connect_client_to_node(&remote.id, node, info.default_port, info.pve_compat)?;
 
     Ok((client, info))
 }
@@ -137,6 +228,7 @@ fn prepare_connect_multi_client(remote: &Remote) -> Result<(MultiClient, Connect
     for node in &remote.nodes {
         clients.push(MultiClientEntry {
             client: Arc::new(prepare_connect_client_to_node(
+                &remote.id,
                 node,
                 info.default_port,
                 info.pve_compat,
-- 
2.47.3

[PATCH datacenter-manager 16/17] server: api: tasks: move `spawn_aborted_on_shutdown()` to super module

[Shannon Sterz]

so other new tasks can use it.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---

Notes:
    i wonder if we should create a trait here that handles this. we seem
    to repeat quite a bit of code here and imo the pattern of:
    
    `start_task` -> `run(_loop)` -> `run_single`
    
    can be moved into a trait implementation and most tasks will
    then only need to implement `run_single` and specify the interval for
    the loop. here a small example of how this could be done (only threw
    this together in a couple of minutes, so can be improved for sure):
    
    ```rs
    pub trait ServerTask {
        const INTERVAL: u64;
    
        fn start() -> JoinHandle<()> {
            tokio::spawn(async move {
                let future = pin!(Self::run());
                let abort_future = pin!(proxmox_daemon::shutdown_future());
                futures::future::select(future, abort_future).await;
            })
        }
    
        fn run() -> impl Future + Send + 'static {
            async {
                loop {
                    Self::run_once().await;
                    let delay_target = task_utils::next_aligned_instant(Self::INTERVAL);
                    tokio::time::sleep_until(tokio::time::Instant::from_std(delay_target)).await;
                }
            }
        }
    
        fn run_once() -> impl Future + Send + 'static;
    }
    
    struct CephTask;
    
    impl ServerTask for CephTask {
        const INTERVAL: u64 = 300;
    
        #[tracing::instrument(skip_all, name = "ceph_detection")]
        #[allow(refining_impl_trait)]
        async fn run_once() {
            if let Err(err) = server::ceph::sweep::sweep().await {
                log::warn!("ceph auto-detection sweep failed: {err:#}");
            }
        }
    }
    ```

 .../tasks/ceph_detection.rs                    | 18 +-----------------
 .../bin/proxmox-datacenter-api/tasks/mod.rs    | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/server/src/bin/proxmox-datacenter-api/tasks/ceph_detection.rs b/server/src/bin/proxmox-datacenter-api/tasks/ceph_detection.rs
index fde171ad..990e15aa 100644
--- a/server/src/bin/proxmox-datacenter-api/tasks/ceph_detection.rs
+++ b/server/src/bin/proxmox-datacenter-api/tasks/ceph_detection.rs
@@ -6,29 +6,13 @@
 //! daemon version), and refreshes the cached `ceph status` as a side effect so
 //! the cluster-list overview keeps showing health without a live fetch.
 
-use std::future::Future;
-use std::pin::pin;
-
-use tokio::task::JoinHandle;
-
 use server::task_utils;
 
 /// How often to sweep all PVE remotes for Ceph clusters, in seconds.
 const SWEEP_INTERVAL: u64 = 300;
 
-fn spawn_aborted_on_shutdown<F>(future: F) -> JoinHandle<()>
-where
-    F: Future + Send + 'static,
-{
-    tokio::spawn(async move {
-        let future = pin!(future);
-        let abort_future = pin!(proxmox_daemon::shutdown_future());
-        futures::future::select(future, abort_future).await;
-    })
-}
-
 pub fn start_task() {
-    spawn_aborted_on_shutdown(run());
+    super::spawn_aborted_on_shutdown(run());
 }
 
 async fn run() {
diff --git a/server/src/bin/proxmox-datacenter-api/tasks/mod.rs b/server/src/bin/proxmox-datacenter-api/tasks/mod.rs
index 34e4559d..2a05e9b3 100644
--- a/server/src/bin/proxmox-datacenter-api/tasks/mod.rs
+++ b/server/src/bin/proxmox-datacenter-api/tasks/mod.rs
@@ -4,3 +4,19 @@ pub mod ceph_detection;
 pub mod remote_node_mapping;
 pub mod remote_tasks;
 pub mod remote_updates;
+
+use std::future::Future;
+use std::pin::pin;
+
+use tokio::task::JoinHandle;
+
+fn spawn_aborted_on_shutdown<F>(future: F) -> JoinHandle<()>
+where
+    F: Future + Send + 'static,
+{
+    tokio::spawn(async move {
+        let future = pin!(future);
+        let abort_future = pin!(proxmox_daemon::shutdown_future());
+        futures::future::select(future, abort_future).await;
+    })
+}
-- 
2.47.3

[PATCH datacenter-manager 17/17] server: bin: api: tasks: add task to discover new staged certificates

[Shannon Sterz]

so pdm stays in the loop if a remote's certificate changes.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 server/src/bin/proxmox-datacenter-api/main.rs |   1 +
 .../bin/proxmox-datacenter-api/tasks/mod.rs   |   1 +
 .../tasks/remote_staged_fingerprints.rs       | 149 ++++++++++++++++++
 3 files changed, 151 insertions(+)
 create mode 100644 server/src/bin/proxmox-datacenter-api/tasks/remote_staged_fingerprints.rs

diff --git a/server/src/bin/proxmox-datacenter-api/main.rs b/server/src/bin/proxmox-datacenter-api/main.rs
index 6e915b67..43c40651 100644
--- a/server/src/bin/proxmox-datacenter-api/main.rs
+++ b/server/src/bin/proxmox-datacenter-api/main.rs
@@ -335,6 +335,7 @@ async fn run(debug: bool) -> Result<(), Error> {
     tasks::remote_tasks::start_task()?;
     tasks::remote_updates::start_task()?;
     tasks::ceph_detection::start_task();
+    tasks::remote_staged_fingerprints::start_task();
 
     server.await?;
     log::info!("server shutting down, waiting for active workers to complete");
diff --git a/server/src/bin/proxmox-datacenter-api/tasks/mod.rs b/server/src/bin/proxmox-datacenter-api/tasks/mod.rs
index 2a05e9b3..968a6dc2 100644
--- a/server/src/bin/proxmox-datacenter-api/tasks/mod.rs
+++ b/server/src/bin/proxmox-datacenter-api/tasks/mod.rs
@@ -2,6 +2,7 @@ pub mod logrotate;
 
 pub mod ceph_detection;
 pub mod remote_node_mapping;
+pub mod remote_staged_fingerprints;
 pub mod remote_tasks;
 pub mod remote_updates;
 
diff --git a/server/src/bin/proxmox-datacenter-api/tasks/remote_staged_fingerprints.rs b/server/src/bin/proxmox-datacenter-api/tasks/remote_staged_fingerprints.rs
new file mode 100644
index 00000000..9299b657
--- /dev/null
+++ b/server/src/bin/proxmox-datacenter-api/tasks/remote_staged_fingerprints.rs
@@ -0,0 +1,149 @@
+//! Periodically query remotes for staged fingerprints.
+//!
+//! Nodes of remotes that have a specified fingerprint are probed regularly to see if a new staged
+//! certificate was issued. This should allow us to keep TLS connections secure without manual user
+//! intervention even across certificate rotation.
+
+use std::collections::HashMap;
+use std::collections::hash_map::Entry;
+
+use anyhow::Error;
+
+use proxmox_schema::PropertyString;
+
+use pdm_api_types::Fingerprint;
+use pdm_api_types::remotes::{NodeUrl, Remote, RemoteType};
+
+use server::connection::make_pve_client_with_endpoint;
+use server::task_utils;
+
+// The daily update task renewing certificates on a remote runs once a day by default, so scan it
+// twice a day to pick up on a new certificate as early as possible.
+const TASK_INTERVAL: u64 = 12 * 60 * 60;
+
+pub fn start_task() {
+    super::spawn_aborted_on_shutdown(run());
+}
+
+async fn run() {
+    loop {
+        run_once().await;
+        let delay_target = task_utils::next_aligned_instant(TASK_INTERVAL);
+        tokio::time::sleep_until(tokio::time::Instant::from_std(delay_target)).await;
+    }
+}
+
+#[tracing::instrument(skip_all, name = "update_staged_fingerprints")]
+async fn run_once() {
+    if let Err(err) = query_staged_fps().await {
+        log::warn!("Could not query remotes for new staged fingerprints: {err:#}");
+    }
+}
+
+async fn query_staged_fps() -> Result<(), Error> {
+    // First check if and what we need to update.
+    let (config, _) = pdm_config::remotes::config()?;
+    let mut updates: HashMap<String, Vec<PropertyString<NodeUrl>>> = HashMap::new();
+
+    for (remote_name, remote) in config {
+        match remote.ty {
+            RemoteType::Pve => {
+                for node in &remote.nodes {
+                    let Some(fp) = node.fingerprint.as_ref() else {
+                        continue; // Node uses the system's trust store, skip querying staged certs.
+                    };
+
+                    let new_fps = match fetch_staged_fps_from_pve_remote(&remote, node, fp).await {
+                        Ok(res) => res,
+                        Err(e) => {
+                            log::warn!(
+                                "Could not get staged fingerprints of node '{}' of remote \
+                                    '{remote_name}': {e:#}",
+                                node.hostname
+                            );
+                            continue;
+                        }
+                    };
+
+                    if node.staged_fingerprints != new_fps {
+                        let new_node = NodeUrl {
+                            hostname: node.hostname.to_owned(),
+                            fingerprint: Some(fp.to_owned()),
+                            staged_fingerprints: new_fps,
+                        };
+
+                        match updates.entry(remote_name.clone()) {
+                            Entry::Occupied(mut e) => e.get_mut().push(new_node.into()),
+                            Entry::Vacant(e) => {
+                                e.insert(vec![new_node.into()]);
+                            }
+                        }
+                    }
+                }
+            }
+            RemoteType::Pbs => {
+                log::debug!("PBS remotes don't rotate certificates, skipping '{remote_name}'.");
+            }
+        }
+    }
+
+    // Only then lock the configuration, re-check if the remote node we want to update still
+    // matches the expected state and then update it.
+    let _lock = pdm_config::remotes::lock_config()?;
+    let (mut new_conf, _) = pdm_config::remotes::config()?;
+
+    for (remote, mut nodes) in updates {
+        if let Some(remote) = new_conf.get_mut(&remote) {
+            for node in &mut nodes {
+                remote.nodes.iter_mut().for_each(|n| {
+                    if node.hostname == n.hostname && node.fingerprint == n.fingerprint {
+                        // At this point we know that a) a remote by the same name with b) a node
+                        // with the same hostname and fingerprint still exists -> update its staged
+                        // fingerprints.
+                        log::info!(
+                            "Got new staged fingerprints for remote {} and node {}, updating...",
+                            remote.id,
+                            n.hostname
+                        );
+                        n.staged_fingerprints = node.staged_fingerprints.take();
+                    }
+                });
+            }
+        }
+    }
+
+    pdm_config::remotes::save_config(new_conf)
+}
+
+async fn fetch_staged_fps_from_pve_remote(
+    remote: &Remote,
+    node: &NodeUrl,
+    current_fp: &str,
+) -> Result<Option<Vec<Fingerprint>>, Error> {
+    // Query each node by connecting to it directly instead of specifying the node
+    // name in the request. The hostname in the remotes config may not match the
+    // node name, this avoids such a mismatch issue.
+    let client = make_pve_client_with_endpoint(remote, Some(&node.hostname))?;
+    let mut certificates = client.certificates_info("localhost").await?;
+    let current_fp = current_fp.to_lowercase();
+
+    let new_staged_fps = certificates
+        .iter_mut()
+        .filter_map(|c| {
+            if let Some(fp) = c.fingerprint.take() {
+                // Stage all but the current or ca certificates.
+                if current_fp != fp.to_lowercase() && c.filename != "pve-root-ca.pem" {
+                    return Some(fp.parse::<Fingerprint>());
+                }
+            }
+
+            None
+        })
+        .collect::<Result<Vec<Fingerprint>, Error>>()?;
+
+    if new_staged_fps.is_empty() {
+        Ok(None)
+    } else {
+        Ok(Some(new_staged_fps))
+    }
+}
-- 
2.47.3