From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id C48491FF170 for ; Thu, 24 Jul 2025 09:30:30 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2E36D322F4; Thu, 24 Jul 2025 09:31:50 +0200 (CEST) Message-ID: <3d720c4a-5fdc-406d-9923-aa990a938c61@proxmox.com> Date: Thu, 24 Jul 2025 09:31:44 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Thomas Lamprecht , Proxmox Backup Server development discussion References: <20250521084524.829496-1-d.csapak@proxmox.com> <20250521084524.829496-2-d.csapak@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1753342303637 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.020 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox 1/2] http: factor out openssl verification callback X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 >> --- >> 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::>() >> + .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::>() > .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