[PATCH proxmox{,-backup,-websocket-tunnel} v3 0/6] unify openssl callback logic

[PATCH proxmox{,-backup,-websocket-tunnel} v3 0/6] unify openssl callback logic

[Dominik Csapak]

There are currently 3+ slightly different implementations of the openssl
verify callback in place. They differ in how an explicit fingerprint
would be checked:

* pbs-client: if verification was on, a valid certificate would trump a
  wrong epxlicit fingerprint
* proxmox-websocket-tunnel: if an explicit fingerprint was given, it was
  checked, regardless of the openssl result
* proxmox-client: the openssl validity had priority as in pbs-client,
  but the fingerprint was not checked against the leaf certificate, but
  agains all certificates in the chain (which would lead to false
  negatives). Note that this is currently only used in PDM
* PDM client has also a different implementation (not touched here)

This series aims to unify the general behavior, but design the interface
to be flexible enought to accomodate the different call sites needs.

I included the change of features for crates, but they have to be bumped
before hand of course and the version must be changed in Cargo.toml.

(if I should send that differently, please do tell how it should be done)

The last patch of the proxmox-http crate is to preserve backwards compatibility
with the current pbs client behavior, but can be switched to the new 'correct'
one via environment variable (which we might want to enable automatically for
the websocket-tunnel?)

Also, since it rather deep in the stack for PBS (remotes sync, etc.) and
PVE (remote migration) IMHO this is a series that should be tested very
well.

Further work could be to unify this behavior for our perl clients too,
but it seemed out of scope for this series. (notably the PVE::APIClient
and the client used in the SDN code)

I tried to implement some tests, but due to the openssl interface this
seems to be not really possible, except if we'd start a server + client
in the tests (which seems overkill). But if anyone has an idea how we
could test this code (and i mean not only it's interface, but the
openssl connection behavior), I'd be glad.

This series partially overlaps/interferes with shannons recent series:
https://lore.proxmox.com/pdm-devel/20260611120327.257523-1-s.sterz@proxmox.com/

Depending on whether this or shannons series is applied first, I'd either
send a follow-up for PDM or a new version rebased on shannons.

changes from v3:
* rebase on master
* add backwards compatibility switch via ENV variable
* add patch for pbs to check already verified fingerprints

changes from v1:
* rebase on master (drops one patch)
* drop hex dependency


proxmox:

Dominik Csapak (3):
  http: factor out openssl verification callback
  http: tls: use legacy behavior when PROXMOX_NEW_TLS_CHECK is not set
  client: use proxmox-http's openssl verification callback

 proxmox-client/Cargo.toml    |   2 +-
 proxmox-client/src/client.rs |  69 ++++++++--------------
 proxmox-http/Cargo.toml      |   7 +++
 proxmox-http/src/lib.rs      |   5 ++
 proxmox-http/src/tls.rs      | 109 +++++++++++++++++++++++++++++++++++
 5 files changed, 146 insertions(+), 46 deletions(-)
 create mode 100644 proxmox-http/src/tls.rs


proxmox-backup:

Dominik Csapak (2):
  pbs-client: use proxmox-https openssl callback
  pbs-client: honor already verified fingerprint

 Cargo.toml                    |   2 +-
 pbs-client/src/http_client.rs | 166 ++++++++++++++++------------------
 2 files changed, 78 insertions(+), 90 deletions(-)


proxmox-websocket-tunnel:

Dominik Csapak (1):
  use proxmox-http's openssl callback

 Cargo.toml  |  2 +-
 src/main.rs | 67 +++++++++++++++++++++--------------------------------
 2 files changed, 28 insertions(+), 41 deletions(-)


Summary over all repositories:
  9 files changed, 252 insertions(+), 177 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH proxmox v3 1/6] http: factor out openssl verification callback

[Dominik Csapak]

with the 'tls' feature offers a callback method that can be used within
openssl's `set_verify_callback` with a given expected fingerprint.

The logic is inspired by our perl and proxmox-websocket-tunnel
verification logic:

Use openssl's verification if no fingerprint is pinned. If a fingerprint
is given, ignore openssl's verification and check if the leafs
certificate is a match.

This introduces a custom error type for this, since we need to handle
errors differently for different users, e.g. pbs-client wants to be able
to use a fingerprint cache and let the user accept it in interactive cli
sessions. For this we want the 'thiserror' crate, so move it to the
workspace Cargo.toml and depend from there. (also change this for
proxmox-openid)

One thing to note here is that  the APPLICATION_VERIFICATION error of
openssl is used to mark the case where an untrusted root or intermediate
certificate is trusted from the callback. When that happens, openssl
might return true for the following certificates (if nothing else is
wrong aside from a missing trust anchor), so the error is checked for
this special value to determine if the openssl validation can be
trusted.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 proxmox-http/Cargo.toml |  5 +++
 proxmox-http/src/lib.rs |  5 +++
 proxmox-http/src/tls.rs | 90 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+)
 create mode 100644 proxmox-http/src/tls.rs

diff --git a/proxmox-http/Cargo.toml b/proxmox-http/Cargo.toml
index 66b11650..e4bff930 100644
--- a/proxmox-http/Cargo.toml
+++ b/proxmox-http/Cargo.toml
@@ -24,6 +24,7 @@ native-tls = { workspace = true, optional = true }
 openssl =  { version = "0.10", optional = true }
 serde_json = { workspace = true, optional = true }
 sync_wrapper = { workspace = true, optional = true }
+thiserror = { workspace = true, optional = true }
 tokio = { workspace = true, features = [], optional = true }
 tokio-openssl = { workspace = true, optional = true }
 tower-service = { workspace = true, optional = true }
@@ -105,3 +106,7 @@ websocket = [
     "tokio?/sync",
     "body",
 ]
+tls = [
+    "dep:openssl",
+    "dep:thiserror",
+]
diff --git a/proxmox-http/src/lib.rs b/proxmox-http/src/lib.rs
index ae3301e8..d27df7bc 100644
--- a/proxmox-http/src/lib.rs
+++ b/proxmox-http/src/lib.rs
@@ -39,3 +39,8 @@ pub use rate_limited_stream::{
 mod body;
 #[cfg(feature = "body")]
 pub use body::Body;
+
+#[cfg(feature = "tls")]
+mod tls;
+#[cfg(feature = "tls")]
+pub use tls::*;
diff --git a/proxmox-http/src/tls.rs b/proxmox-http/src/tls.rs
new file mode 100644
index 00000000..7365230e
--- /dev/null
+++ b/proxmox-http/src/tls.rs
@@ -0,0 +1,90 @@
+use openssl::x509::{X509StoreContextRef, X509VerifyResult};
+
+///
+/// Error type returned by failed [`openssl_verify_callback`].
+///
+#[derive(Debug, thiserror::Error)]
+pub enum SslVerifyError {
+    /// Occurs if no certificate is found in the current part of the chain. Should never happen!
+    #[error("SSL context lacks current certificate")]
+    NoCertificate,
+
+    /// Cannot calculate fingerprint from connection
+    #[error("failed to calculate fingerprint - {0}")]
+    InvalidFingerprint(openssl::error::ErrorStack),
+
+    /// Fingerprint match error
+    #[error("found fingerprint ({fingerprint}) does not match expected fingerprint ({expected})")]
+    FingerprintMismatch {
+        fingerprint: String,
+        expected: String,
+    },
+
+    /// Untrusted certificate with fingerprint information
+    #[error("certificate validation failed")]
+    UntrustedCertificate { fingerprint: String },
+}
+
+/// Intended as an openssl verification callback.
+///
+/// The following things are checked:
+///
+/// * If no fingerprint is given, return the openssl verification result
+/// * If a fingerprint is given, do:
+///     * Ignore all non-leaf certificates/
+pub fn openssl_verify_callback(
+    openssl_valid: bool,
+    ctx: &mut X509StoreContextRef,
+    expected_fp: Option<&str>,
+) -> Result<(), SslVerifyError> {
+    let trust_openssl = ctx.error() != X509VerifyResult::APPLICATION_VERIFICATION;
+    if expected_fp.is_none() && openssl_valid && trust_openssl {
+        return Ok(());
+    }
+
+    let cert = match ctx.current_cert() {
+        Some(cert) => cert,
+        None => {
+            return Err(SslVerifyError::NoCertificate);
+        }
+    };
+
+    if ctx.error_depth() > 0 {
+        // openssl was not valid, but we want to continue, so save that we don't trust openssl
+        ctx.set_error(X509VerifyResult::APPLICATION_VERIFICATION);
+        return Ok(());
+    }
+
+    let digest = cert
+        .digest(openssl::hash::MessageDigest::sha256())
+        .map_err(SslVerifyError::InvalidFingerprint)?;
+    let fingerprint = get_fingerprint_from_u8(&digest);
+
+    if let Some(expected_fp) = expected_fp {
+        if expected_fp.to_lowercase() == fingerprint.to_lowercase() {
+            ctx.set_error(X509VerifyResult::OK);
+            Ok(())
+        } else {
+            Err(SslVerifyError::FingerprintMismatch {
+                fingerprint,
+                expected: expected_fp.to_string(),
+            })
+        }
+    } else {
+        Err(SslVerifyError::UntrustedCertificate { fingerprint })
+    }
+}
+
+/// Returns the fingerprint from a byte slice ([`&[u8]`]) in the form `00:11:22:...`
+pub fn get_fingerprint_from_u8(fp: &[u8]) -> String {
+    use std::fmt::Write as _;
+
+    let mut target = String::with_capacity(fp.len() * 3); // 2 chars per byte + ~1 separator
+    for byte in fp {
+        if !target.is_empty() {
+            target.push(':');
+        }
+        let _ = write!(target, "{byte:02x}");
+    }
+    target
+}
-- 
2.47.3

[PATCH proxmox v3 2/6] http: tls: use legacy behavior when PROXMOX_NEW_TLS_CHECK is not set

[Dominik Csapak]

if that environment variable is not set to "1", give the openssl result
priority, and potentially ignore a given fingerprint that is not
matching. If that's the case, print a warning.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 proxmox-http/Cargo.toml |  2 ++
 proxmox-http/src/tls.rs | 31 +++++++++++++++++++++++++------
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/proxmox-http/Cargo.toml b/proxmox-http/Cargo.toml
index e4bff930..aadb6a42 100644
--- a/proxmox-http/Cargo.toml
+++ b/proxmox-http/Cargo.toml
@@ -20,6 +20,7 @@ http-body = { workspace = true, optional = true }
 http-body-util = { workspace = true, optional = true }
 hyper = { workspace = true, optional = true }
 hyper-util = { workspace = true, optional = true, features = ["http2"] }
+log = { workspace = true, optional = true }
 native-tls = { workspace = true, optional = true }
 openssl =  { version = "0.10", optional = true }
 serde_json = { workspace = true, optional = true }
@@ -107,6 +108,7 @@ websocket = [
     "body",
 ]
 tls = [
+    "dep:log",
     "dep:openssl",
     "dep:thiserror",
 ]
diff --git a/proxmox-http/src/tls.rs b/proxmox-http/src/tls.rs
index 7365230e..635b0e7f 100644
--- a/proxmox-http/src/tls.rs
+++ b/proxmox-http/src/tls.rs
@@ -27,19 +27,31 @@ pub enum SslVerifyError {
 
 /// Intended as an openssl verification callback.
 ///
-/// The following things are checked:
+/// If the 'PROXMOX_NEW_TLS_CHECK' environment variable is set to "1",
+/// the following things are checked:
 ///
 /// * If no fingerprint is given, return the openssl verification result
-/// * If a fingerprint is given, do:
-///     * Ignore all non-leaf certificates/
+/// * If a fingerprint is given, ignore all non-leaf certificates
+///
+/// Otherwise, we trust the openssl result if the whole chain was trusted
 pub fn openssl_verify_callback(
     openssl_valid: bool,
     ctx: &mut X509StoreContextRef,
     expected_fp: Option<&str>,
 ) -> Result<(), SslVerifyError> {
     let trust_openssl = ctx.error() != X509VerifyResult::APPLICATION_VERIFICATION;
-    if expected_fp.is_none() && openssl_valid && trust_openssl {
-        return Ok(());
+
+    let new_check = matches!(std::env::var("PROXMOX_NEW_TLS_CHECK").as_deref(), Ok("1"));
+
+    if openssl_valid && trust_openssl {
+        if new_check && expected_fp.is_none() {
+            return Ok(());
+        }
+
+        // legacy mode: skip all valid certs except the leaf, so we can warn if fingerprint does not match
+        if !new_check && ctx.error_depth() > 0 {
+            return Ok(());
+        }
     }
 
     let cert = match ctx.current_cert() {
@@ -50,7 +62,7 @@ pub fn openssl_verify_callback(
     };
 
     if ctx.error_depth() > 0 {
-        // openssl was not valid, but we want to continue, so save that we don't trust openssl
+        // if openssl is not valid, and we want to continue, save that we don't trust openssl
         ctx.set_error(X509VerifyResult::APPLICATION_VERIFICATION);
         return Ok(());
     }
@@ -65,6 +77,13 @@ pub fn openssl_verify_callback(
             ctx.set_error(X509VerifyResult::OK);
             Ok(())
         } else {
+            if !new_check && openssl_valid && trust_openssl {
+                log::warn!(
+                    "Certificate chain valid, but fingerprint does not match, ignoring fingerprint! To prioritize the fingerprint, set `PROXMOX_NEW_TLS_CHECK=1` in your environment."
+                );
+                return Ok(());
+            }
+
             Err(SslVerifyError::FingerprintMismatch {
                 fingerprint,
                 expected: expected_fp.to_string(),
-- 
2.47.3

[PATCH proxmox v3 3/6] client: use proxmox-http's openssl verification callback

[Dominik Csapak]

This changes the validation logic by always checking the fingerprint of
the leaf certificate, ignoring the openssl verification if a fingerprint
is configured. This now aligns with our perl implementation and the one
for proxmox-websocket-tunnel.

Before, a valid certificate chain would have precedence over an explicit
fingerprint.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 proxmox-client/Cargo.toml    |  2 +-
 proxmox-client/src/client.rs | 69 +++++++++++++-----------------------
 2 files changed, 25 insertions(+), 46 deletions(-)

diff --git a/proxmox-client/Cargo.toml b/proxmox-client/Cargo.toml
index 6ca33420..5cbcecd8 100644
--- a/proxmox-client/Cargo.toml
+++ b/proxmox-client/Cargo.toml
@@ -25,7 +25,7 @@ openssl = { workspace = true, optional = true }
 
 proxmox-login = { workspace = true, features = [ "http" ] }
 
-proxmox-http = { workspace = true, optional = true, features = [ "client" ] }
+proxmox-http = { workspace = true, optional = true, features = [ "client", "tls" ] }
 hyper = { workspace = true, optional = true }
 hyper-util = { workspace = true, optional = true, features = [ "client-legacy" ] }
 
diff --git a/proxmox-client/src/client.rs b/proxmox-client/src/client.rs
index 26913dbb..1a1d3fa8 100644
--- a/proxmox-client/src/client.rs
+++ b/proxmox-client/src/client.rs
@@ -12,6 +12,8 @@ use http_body_util::BodyExt;
 use openssl::hash::MessageDigest;
 use openssl::ssl::{SslConnector, SslMethod, SslVerifyMode};
 use openssl::x509::{self, X509};
+use proxmox_http::SslVerifyError;
+use proxmox_http::get_fingerprint_from_u8;
 use proxmox_login::Ticket;
 use serde::Serialize;
 
@@ -110,10 +112,29 @@ impl Client {
             TlsOptions::Insecure => connector.set_verify(SslVerifyMode::NONE),
             TlsOptions::Fingerprint(expected_fingerprint) => {
                 connector.set_verify_callback(SslVerifyMode::PEER, move |valid, chain| {
-                    if valid {
-                        return true;
+                    let fp = get_fingerprint_from_u8(&expected_fingerprint);
+                    match proxmox_http::openssl_verify_callback(valid, chain, Some(&fp)) {
+                        Ok(()) => true,
+                        Err(err) => {
+                            match err {
+                                SslVerifyError::FingerprintMismatch {
+                                    fingerprint,
+                                    expected,
+                                } => {
+                                    log::error!("bad fingerprint: {fingerprint}");
+                                    log::error!("expected fingerprint: {expected}");
+                                    log::error!(
+                                        r#"If this fingerprint has worked before, it is possible that it changed on the remote
+side. This can happen, for example, if the remote rotates it's certificate regularly.
+If you are sure no machine-in-the-middle attack (MitM) occurred, it is safe to set the
+above 'bad fingerprint' as the new fingerprint for the remote."#
+                                    );
+                                }
+                                _ => log::error!("{err}"),
+                            }
+                            false
+                        }
                     }
-                    verify_fingerprint(chain, &expected_fingerprint)
                 });
             }
             TlsOptions::Callback(cb) => {
@@ -539,48 +560,6 @@ impl HttpApiClient for Client {
     }
 }
 
-fn verify_fingerprint(chain: &x509::X509StoreContextRef, expected_fingerprint: &[u8]) -> bool {
-    let Some(cert) = chain.current_cert() else {
-        log::error!("no certificate in chain?");
-        return false;
-    };
-
-    let fp = match cert.digest(MessageDigest::sha256()) {
-        Err(err) => {
-            log::error!("error calculating certificate fingerprint: {err}");
-            return false;
-        }
-        Ok(fp) => fp,
-    };
-
-    if expected_fingerprint != fp.as_ref() {
-        log::error!("bad fingerprint: {}", fp_string(&fp));
-        log::error!("expected fingerprint: {}", fp_string(expected_fingerprint));
-        log::error!(
-            r#"If this fingerprint has worked before, it is possible that it changed on the remote
-side. This can happen, for example, if the remote rotates it's certificate regularly.
-If you are sure no machine-in-the-middle attack (MitM) occured, it is safe to set the
-above 'bad fingerpint' as the new fingerprint for the remote."#
-        );
-        return false;
-    }
-
-    true
-}
-
-fn fp_string(fp: &[u8]) -> String {
-    use std::fmt::Write as _;
-
-    let mut out = String::new();
-    for b in fp {
-        if !out.is_empty() {
-            out.push(':');
-        }
-        let _ = write!(out, "{b:02x}");
-    }
-    out
-}
-
 /// Classify an HTTP client error into either a connection-establishment failure
 /// ([`Error::Connect`]) or a post-connection error ([`Error::Client`]).
 ///
-- 
2.47.3

[PATCH proxmox-backup v3 4/6] pbs-client: use proxmox-https openssl callback

[Dominik Csapak]

instead of implementing it here. This changes the behavior when giving a
fingerprint explicitly when the certificate chain is trusted by openssl.
Previously this would be accepted due to openssls checks, regardless if
the given fingerprint would match or not.

With this patch, a given fingerprint has higher priority than openssls
validation.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 Cargo.toml                    |   2 +-
 pbs-client/src/http_client.rs | 149 ++++++++++++++--------------------
 2 files changed, 61 insertions(+), 90 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index dc8e2730c..7962398f1 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -64,7 +64,7 @@ proxmox-config-digest = "1"
 proxmox-daemon = "1"
 proxmox-fuse = "3"
 proxmox-docgen = "1"
-proxmox-http = { version = "1.0.2", features = [ "client", "http-helpers", "api-types", "websocket" ] } # see below
+proxmox-http = { version = "1.0.2", features = [ "client", "http-helpers", "api-types", "websocket", "tls" ] } # see below
 proxmox-human-byte = "1"
 proxmox-io = "1.0.1" # tools and client use "tokio" feature
 proxmox-lang = "1.1"
diff --git a/pbs-client/src/http_client.rs b/pbs-client/src/http_client.rs
index a6fcafcfd..ea6e5d7fa 100644
--- a/pbs-client/src/http_client.rs
+++ b/pbs-client/src/http_client.rs
@@ -15,10 +15,7 @@ use hyper::http::{Request, Response};
 use hyper_util::client::legacy::connect::dns::GaiResolver;
 use hyper_util::client::legacy::{Client, connect::HttpConnector};
 use hyper_util::rt::{TokioExecutor, TokioIo};
-use openssl::{
-    ssl::{SslConnector, SslMethod},
-    x509::X509StoreContextRef,
-};
+use openssl::ssl::{SslConnector, SslMethod};
 use percent_encoding::percent_encode;
 use serde_json::{Value, json};
 use xdg::BaseDirectories;
@@ -29,9 +26,9 @@ use proxmox_sys::linux::tty;
 
 use proxmox_async::broadcast_future::BroadcastFuture;
 use proxmox_http::Body;
-use proxmox_http::ProxyConfig;
 use proxmox_http::client::HttpsConnector;
 use proxmox_http::uri::{build_authority, json_object_to_query};
+use proxmox_http::{ProxyConfig, SslVerifyError, openssl_verify_callback};
 use proxmox_log::{error, info, warn};
 use proxmox_rate_limiter::RateLimiter;
 
@@ -425,30 +422,42 @@ impl HttpClient {
             let interactive = options.interactive;
             let fingerprint_cache = options.fingerprint_cache;
             let prefix = options.prefix.clone();
-            let trust_openssl_valid = Arc::new(Mutex::new(true));
             ssl_connector_builder.set_verify_callback(
                 openssl::ssl::SslVerifyMode::PEER,
-                move |valid, ctx| match Self::verify_callback(
+                move |valid, ctx| match openssl_verify_callback(
                     valid,
                     ctx,
-                    expected_fingerprint.as_ref(),
-                    interactive,
-                    Arc::clone(&trust_openssl_valid),
+                    expected_fingerprint.as_deref(),
                 ) {
-                    Ok(None) => true,
-                    Ok(Some(fingerprint)) => {
-                        if fingerprint_cache {
-                            if let Some(ref prefix) = prefix {
-                                if let Err(err) = store_fingerprint(prefix, &server, &fingerprint) {
-                                    error!("{}", err);
+                    Ok(()) => true,
+                    Err(err) => {
+                        match err {
+                            SslVerifyError::NoCertificate => error!(
+                                "certificate validation failed - context lacks current certificate"
+                            ),
+                            SslVerifyError::InvalidFingerprint(error_stack) => {
+                                error!("certificate validation failed - failed to calculate FP - {error_stack}")
+                            },
+                            SslVerifyError::UntrustedCertificate { fingerprint } => {
+                                if interactive && std::io::stdin().is_terminal() {
+                                    match Self::interactive_fp_check(prefix.as_deref(), &server, verified_fingerprint.clone(), fingerprint_cache, fingerprint) {
+                                        Ok(()) => return true,
+                                        Err(err) => error!("certificate validation failed - {err}"),
+                                    }
                                 }
                             }
+                            SslVerifyError::FingerprintMismatch { fingerprint, expected } => {
+                                warn!("WARNING: certificate fingerprint does not match expected fingerprint!");
+                                warn!("expected:    {expected}");
+
+                                if interactive && std::io::stdin().is_terminal() {
+                                    match Self::interactive_fp_check(prefix.as_deref(), &server, verified_fingerprint.clone(), fingerprint_cache, fingerprint) {
+                                        Ok(()) => return true,
+                                        Err(err) => error!("certificate validation failed - {err}"),
+                                    }
+                                }
+                            },
                         }
-                        *verified_fingerprint.lock().unwrap() = Some(fingerprint);
-                        true
-                    }
-                    Err(err) => {
-                        error!("certificate validation failed - {}", err);
                         false
                     }
                 },
@@ -661,79 +670,41 @@ impl HttpClient {
         bail!("no password input mechanism available");
     }
 
-    fn verify_callback(
-        openssl_valid: bool,
-        ctx: &mut X509StoreContextRef,
-        expected_fingerprint: Option<&String>,
-        interactive: bool,
-        trust_openssl: Arc<Mutex<bool>>,
-    ) -> Result<Option<String>, Error> {
-        let mut trust_openssl_valid = trust_openssl.lock().unwrap();
-
-        // we can only rely on openssl's prevalidation if we haven't forced it earlier
-        if openssl_valid && *trust_openssl_valid {
-            return Ok(None);
-        }
-
-        let cert = match ctx.current_cert() {
-            Some(cert) => cert,
-            None => bail!("context lacks current certificate."),
-        };
-
-        // force trust in case of a chain, but set flag to no longer trust prevalidation by openssl
-        if ctx.error_depth() > 0 {
-            *trust_openssl_valid = false;
-            return Ok(None);
-        }
-
-        // leaf certificate - if we end up here, we have to verify the fingerprint!
-        let fp = match cert.digest(openssl::hash::MessageDigest::sha256()) {
-            Ok(fp) => fp,
-            Err(err) => bail!("failed to calculate certificate FP - {}", err), // should not happen
-        };
-        let fp_string = hex::encode(fp);
-        let fp_string = fp_string
-            .as_bytes()
-            .chunks(2)
-            .map(|v| std::str::from_utf8(v).unwrap())
-            .collect::<Vec<&str>>()
-            .join(":");
-
-        if let Some(expected_fingerprint) = expected_fingerprint {
-            let expected_fingerprint = expected_fingerprint.to_lowercase();
-            if expected_fingerprint == fp_string {
-                return Ok(Some(fp_string));
-            } else {
-                warn!("WARNING: certificate fingerprint does not match expected fingerprint!");
-                warn!("expected:    {}", expected_fingerprint);
-            }
-        }
-
-        // If we're on a TTY, query the user
-        if interactive && std::io::stdin().is_terminal() {
-            info!("fingerprint: {}", fp_string);
-            loop {
-                eprint!("Are you sure you want to continue connecting? (y/n): ");
-                let _ = std::io::stdout().flush();
-                use std::io::{BufRead, BufReader};
-                let mut line = String::new();
-                match BufReader::new(std::io::stdin()).read_line(&mut line) {
-                    Ok(_) => {
-                        let trimmed = line.trim();
-                        if trimmed == "y" || trimmed == "Y" {
-                            return Ok(Some(fp_string));
-                        } else if trimmed == "n" || trimmed == "N" {
-                            bail!("Certificate fingerprint was not confirmed.");
-                        } else {
-                            continue;
+    fn interactive_fp_check(
+        prefix: Option<&str>,
+        server: &str,
+        verified_fingerprint: Arc<Mutex<Option<String>>>,
+        fingerprint_cache: bool,
+        fingerprint: String,
+    ) -> Result<(), Error> {
+        info!("fingerprint: {fingerprint}");
+        loop {
+            eprint!("Are you sure you want to continue connecting? (y/n): ");
+            let _ = std::io::stdout().flush();
+            use std::io::{BufRead, BufReader};
+            let mut line = String::new();
+            match BufReader::new(std::io::stdin()).read_line(&mut line) {
+                Ok(_) => {
+                    let trimmed = line.trim();
+                    if trimmed == "y" || trimmed == "Y" {
+                        if fingerprint_cache && prefix.is_some() {
+                            if let Err(err) =
+                                store_fingerprint(prefix.unwrap(), server, &fingerprint)
+                            {
+                                error!("{}", err);
+                            }
                         }
+                        *verified_fingerprint.lock().unwrap() = Some(fingerprint);
+                        return Ok(());
+                    } else if trimmed == "n" || trimmed == "N" {
+                        bail!("Certificate fingerprint was not confirmed.");
+                    } else {
+                        continue;
                     }
-                    Err(err) => bail!("Certificate fingerprint was not confirmed - {}.", err),
                 }
+                Err(err) => bail!("Certificate fingerprint was not confirmed - {}.", err),
             }
         }
-
-        bail!("Certificate fingerprint was not confirmed.");
     }
 
     pub async fn request(&self, mut req: Request<Body>) -> Result<Value, Error> {
-- 
2.47.3

[PATCH proxmox-backup v3 5/6] pbs-client: honor already verified fingerprint

[Dominik Csapak]

When there is an invalid fingerprint or a mismatch and the client is
executed in an interactive way, the user is asked if he wants to trust
the fingerprint.

OpenSSL might evaluate the verify callback multiple times per
certificate (for different errors for example), so we have to check
if the fingerprint matches the one we already verified.

Otherwise the user gets multiple prompts directly after another.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-client/src/http_client.rs | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/pbs-client/src/http_client.rs b/pbs-client/src/http_client.rs
index ea6e5d7fa..14aa4d317 100644
--- a/pbs-client/src/http_client.rs
+++ b/pbs-client/src/http_client.rs
@@ -422,6 +422,17 @@ impl HttpClient {
             let interactive = options.interactive;
             let fingerprint_cache = options.fingerprint_cache;
             let prefix = options.prefix.clone();
+            let check_fp_verified =
+                |verified_fingerprint: &Arc<Mutex<Option<String>>>, fingerprint: &str| -> bool {
+                    let verified = verified_fingerprint.lock().unwrap();
+                    if let Some(verified) = &*verified {
+                        if verified == fingerprint {
+                            // already verified
+                            return true;
+                        }
+                    }
+                    false
+                };
             ssl_connector_builder.set_verify_callback(
                 openssl::ssl::SslVerifyMode::PEER,
                 move |valid, ctx| match openssl_verify_callback(
@@ -439,6 +450,9 @@ impl HttpClient {
                                 error!("certificate validation failed - failed to calculate FP - {error_stack}")
                             },
                             SslVerifyError::UntrustedCertificate { fingerprint } => {
+                                if check_fp_verified(&verified_fingerprint, &fingerprint) {
+                                    return true;
+                                }
                                 if interactive && std::io::stdin().is_terminal() {
                                     match Self::interactive_fp_check(prefix.as_deref(), &server, verified_fingerprint.clone(), fingerprint_cache, fingerprint) {
                                         Ok(()) => return true,
@@ -447,6 +461,9 @@ impl HttpClient {
                                 }
                             }
                             SslVerifyError::FingerprintMismatch { fingerprint, expected } => {
+                                if check_fp_verified(&verified_fingerprint, &fingerprint) {
+                                    return true;
+                                }
                                 warn!("WARNING: certificate fingerprint does not match expected fingerprint!");
                                 warn!("expected:    {expected}");
 
-- 
2.47.3

[PATCH proxmox-websocket-tunnel v3 6/6] use proxmox-http's openssl callback

[Dominik Csapak]

no functional change intended, since the callback there should implement
the same behavior.

With this, we can drop the dependency on itertools.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 Cargo.toml  |  2 +-
 src/main.rs | 67 +++++++++++++++++++++--------------------------------
 2 files changed, 28 insertions(+), 41 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 02ac3d1..b491934 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -26,5 +26,5 @@ tokio = { version = "1", features = ["io-std", "io-util", "macros", "rt-multi-th
 tokio-stream = { version = "0.1", features = ["io-util"] }
 tokio-util = "0.7"
 
-proxmox-http = { version = "1", features = ["websocket", "client"] }
+proxmox-http = { version = "1", features = ["websocket", "client", "tls"] }
 proxmox-sys = "1"
diff --git a/src/main.rs b/src/main.rs
index 6d86575..4328d64 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -25,7 +25,7 @@ use tokio_stream::StreamExt;
 
 use proxmox_http::client::HttpsConnector;
 use proxmox_http::websocket::{OpCode, WebSocket, WebSocketReader, WebSocketWriter};
-use proxmox_http::Body;
+use proxmox_http::{Body, SslVerifyError};
 
 #[derive(Serialize, Deserialize, Debug)]
 #[serde(rename_all = "kebab-case")]
@@ -142,48 +142,35 @@ impl CtrlTunnel {
         }
 
         let mut ssl_connector_builder = SslConnector::builder(SslMethod::tls())?;
-        if let Some(expected) = fingerprint {
+        if fingerprint.is_some() {
             ssl_connector_builder.set_verify_callback(
                 openssl::ssl::SslVerifyMode::PEER,
-                move |_valid, ctx| {
-                    let cert = match ctx.current_cert() {
-                        Some(cert) => cert,
-                        None => {
-                            // should not happen
-                            eprintln!("SSL context lacks current certificate.");
-                            return false;
-                        }
-                    };
-
-                    // skip CA certificates, we only care about the peer cert
-                    let depth = ctx.error_depth();
-                    if depth != 0 {
-                        return true;
-                    }
-
-                    use itertools::Itertools;
-                    let fp = match cert.digest(openssl::hash::MessageDigest::sha256()) {
-                        Ok(fp) => fp,
-                        Err(err) => {
-                            // should not happen
-                            eprintln!("failed to calculate certificate FP - {}", err);
-                            return false;
+                move |valid, ctx| match proxmox_http::openssl_verify_callback(
+                    valid,
+                    ctx,
+                    fingerprint.as_deref(),
+                ) {
+                    Ok(()) => true,
+                    Err(err) => {
+                        match err {
+                            SslVerifyError::NoCertificate => {
+                                eprintln!("SSL context lacks current certificate");
+                            }
+                            SslVerifyError::InvalidFingerprint(err) => {
+                                eprintln!("failed to calculate certificate FP - {err}")
+                            }
+                            SslVerifyError::FingerprintMismatch {
+                                fingerprint,
+                                expected,
+                            } => {
+                                eprintln!(
+                                    "certificate fingerprint does not match expected fingerprint!"
+                                );
+                                eprintln!("expected:    {expected}");
+                                eprintln!("encountered: {fingerprint}");
+                            }
+                            SslVerifyError::UntrustedCertificate { .. } => {}
                         }
-                    };
-                    let fp_string = hex::encode(fp);
-                    let fp_string = fp_string
-                        .as_bytes()
-                        .chunks(2)
-                        .map(|v| unsafe { std::str::from_utf8_unchecked(v) })
-                        .join(":");
-
-                    let expected = expected.to_lowercase();
-                    if expected == fp_string {
-                        true
-                    } else {
-                        eprintln!("certificate fingerprint does not match expected fingerprint!");
-                        eprintln!("expected:    {}", expected);
-                        eprintln!("encountered: {}", fp_string);
                         false
                     }
                 },
-- 
2.47.3