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

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

On 7/23/25 21:26, Thomas Lamprecht wrote:
> 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
> 

will send a v2

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

i tried really hard to create in-rust tests by using the openssl
interfaces, but i wasn't able to massage them to actually test the
callback.

imho the only way i see how we could test is to write a short
server and start that and connect with socat/openssl/... directly
during the tests... but i'm not sure if that is really practical to do
in rust tests...

> 
>> +    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/

true

> 
>> +}
>> 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.

shall i send a seperate patch upfront that moves the thiserror crate
dependency to the workspace cargo.toml ?

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