Re: [pbs-devel] [PATCH proxmox 1/2] http: factor out openssl verification callback

From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>,
	Dominik Csapak <d.csapak@proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox 1/2] http: factor out openssl verification callback
Date: Wed, 23 Jul 2025 21:26:09 +0200	[thread overview]
Message-ID: <b71ff85f-1300-459e-85e5-6034fbf42088@proxmox.com> (raw)
In-Reply-To: <20250521084524.829496-2-d.csapak@proxmox.com>

Am 21.05.25 um 10:45 schrieb 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.

needs rebasing (sorry for the delay), and some comments inline

> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  Cargo.toml                |  1 +
>  proxmox-http/Cargo.toml   |  7 +++
>  proxmox-http/src/lib.rs   |  5 +++
>  proxmox-http/src/tls.rs   | 89 +++++++++++++++++++++++++++++++++++++++
>  proxmox-openid/Cargo.toml |  2 +-
>  5 files changed, 103 insertions(+), 1 deletion(-)
>  create mode 100644 proxmox-http/src/tls.rs
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index 95ade7d7..8aecc6ba 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -107,6 +107,7 @@ serde_json = "1.0"
>  serde_plain = "1.0"
>  syn = { version = "2", features = [ "full", "visit-mut" ] }
>  tar = "0.4"
> +thiserror = "1"
>  tokio = "1.6"
>  tokio-openssl = "0.6.1"
>  tokio-stream = "0.1.0"
> diff --git a/proxmox-http/Cargo.toml b/proxmox-http/Cargo.toml
> index afa5981d..fb2eb26d 100644
> --- a/proxmox-http/Cargo.toml
> +++ b/proxmox-http/Cargo.toml
> @@ -15,11 +15,13 @@ rust-version.workspace = true
>  anyhow.workspace = true
>  base64 = { workspace = true, optional = true }
>  futures = { workspace = true, optional = true }
> +hex = { workspace = true, optional = true }

is not be required, see below.

>  http = { workspace = true, optional = true }
>  hyper = { workspace = true, optional = true }
>  native-tls = { workspace = true, optional = true }
>  openssl =  { version = "0.10", optional = true }
>  serde_json = { 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
> @@ -79,3 +81,8 @@ websocket = [
>      "tokio?/io-util",
>      "tokio?/sync",
>  ]
> +tls = [
> +    "dep:hex",
> +    "dep:openssl",
> +    "dep:thiserror",
> +]
> diff --git a/proxmox-http/src/lib.rs b/proxmox-http/src/lib.rs
> index 4770aaf4..a676acf9 100644
> --- a/proxmox-http/src/lib.rs
> +++ b/proxmox-http/src/lib.rs
> @@ -35,3 +35,8 @@ pub use rate_limiter::{RateLimit, RateLimiter, RateLimiterVec, ShareableRateLimi
>  mod rate_limited_stream;
>  #[cfg(feature = "rate-limited-stream")]
>  pub use rate_limited_stream::RateLimitedStream;
> +
> +#[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..6da03cd9
> --- /dev/null
> +++ b/proxmox-http/src/tls.rs
> @@ -0,0 +1,89 @@
> +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,

hmm, not sure how simple it is to construct/derive above type manually,
but would be great to get this method some actual tests, potentially
with some temporary certs created on demand (or the snake oil one).
But that doesn't need to block this series, it's already an improvement
to have just one of these around in any case.

> +    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 {
> +    let fp_string = hex::encode(fp);
> +    let fp_string = fp_string
> +        .as_bytes()
> +        .chunks(2)
> +        .map(|v| unsafe { std::str::from_utf8_unchecked(v) })
> +        .collect::<Vec<_>>()
> +        .join(":");
> +
> +    fp_string

As mentioned recently in the review for the s3-client [0], this can be done
simpler:

fp
    .iter()
    .map(|byte| format!("{byte:02x}"))
    .collect::<Vec<String>>()
    .join(":")

[0]: https://lore.proxmox.com/all/38e3b3b5-a1a6-43d6-b925-1d04d8b1e22d@proxmox.com/

> +}
> diff --git a/proxmox-openid/Cargo.toml b/proxmox-openid/Cargo.toml
> index a5249214..4223d10c 100644
> --- a/proxmox-openid/Cargo.toml
> +++ b/proxmox-openid/Cargo.toml
> @@ -18,7 +18,7 @@ http.workspace = true
>  nix.workspace = true
>  serde = { workspace = true, features = ["derive"] }
>  serde_json.workspace = true
> -thiserror = "1"
> +thiserror.workspace = true

a bit unrelated to the rest, but can be OK to do in a patch like this I guess.

>  native-tls.workspace = true
>  
>  openidconnect = { version = "2.4", default-features = false, features = ["accept-rfc3339-timestamps"] }

_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel