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 4341C1FF15C for ; Fri, 11 Jul 2025 12:52:03 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5C2FBE4EA; Fri, 11 Jul 2025 12:52:49 +0200 (CEST) Message-ID: <5f1a3122-5478-4f4c-8b74-4487bab5dd9e@proxmox.com> Date: Fri, 11 Jul 2025 12:52:14 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Thomas Lamprecht , Proxmox Backup Server development discussion References: <20250710170728.102829-1-c.ebner@proxmox.com> <20250710170728.102829-2-c.ebner@proxmox.com> <38e3b3b5-a1a6-43d6-b925-1d04d8b1e22d@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <38e3b3b5-a1a6-43d6-b925-1d04d8b1e22d@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.042 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [rust-lang.org] Subject: Re: [pbs-devel] [PATCH proxmox v7 1/9] s3 client: add crate for AWS s3 compatible object store client 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/11/25 09:42, Thomas Lamprecht wrote: > Am 10.07.25 um 19:06 schrieb Christian Ebner: >> + fn verify_certificate_fingerprint( >> + openssl_valid: bool, >> + context: &mut X509StoreContextRef, >> + expected_fingerprint: Option, >> + trust_openssl: Arc>, >> + ) -> Result, Error> { > > This method seems a bit like it might fit better into a (micro) crate specific for > "cert stuff". FWIW, there is a verify_fingerprint function in the proxmox-client > crate already, this one here seems to be a bit more generic, or well also include > things like the fp_string function for doing &[u8] -> String the client has separately. > > > As both use openssl, i.e. X509StoreContextRef as base, it quite probably can share > most of the implementation. > > FWIW, I'd be even open for a quite specific proxmox-tls-cert-fingerprint micro > crate, as IMO those micro crates to not produce much maintenance cost, especially > if one assembles it after having the use case already in a few places, thus being > pretty likely that the API will work OK that way for new future use cases too. > Note, not promoting creation of trivial things, e.g. the famous leftpad crates, > but TLS (fingerprint) cert verification is not really trivial and can have > critical implications, which then can be IMO enough to justify a micro crate. > > Anyhow, this can be refactored out transparently at any time, so really not > a blocker for getting this client in. > >> + let mut trust_openssl_valid = trust_openssl.lock().unwrap(); >> + >> + // only rely on openssl prevalidation if was not forced earlier >> + if openssl_valid && *trust_openssl_valid { >> + return Ok(None); >> + } >> + >> + let certificate = match context.current_cert() { >> + Some(certificate) => certificate, >> + None => bail!("context lacks current certificate."), >> + }; >> + > > > This below would do well with a (short) explanatory comment IMO. > >> + if context.error_depth() > 0 { >> + *trust_openssl_valid = false; >> + return Ok(None); >> + } >> + >> + let certificate_digest = certificate >> + .digest(MessageDigest::sha256()) >> + .context("failed to calculate certificate digest")?; >> + let certificate_fingerprint = hex::encode(certificate_digest); >> + let certificate_fingerprint = certificate_fingerprint >> + .as_bytes() >> + .chunks(2) >> + .map(|v| std::str::from_utf8(v).unwrap()) >> + .collect::>() >> + .join(":"); > > I really have nothing against iterator chains, but here unrolling that seems a > bit more readable, e.g. the aforementioned fp_string fn from proxmox-client does: > > let mut cert_fingerprint_str = String::new(); > for b in fp { > if !cert_fingerprint_str.is_empty() { > out.push(':'); > } > let _ = write!(cert_fingerprint_str, "{b:02x}"); > } > > Or at least avoid the upfront hex::encode and to that on the fly, e.g. like: > > let certificate_fingerprint = certificate_digest > .iter() > .map(|byte| format!("{byte:02x}")) > .collect::>() > .join(":"); > > > That variant would also be fine, IMO even nicer as the yours and the one from > proxmox-client (and shorter as both!), but as disclaimer I only quicktested it in > the rust playground: > > https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=f575fdf9da37aa3ebefc28a66a93be2b > >> + >> + if let Some(expected_fingerprint) = expected_fingerprint { >> + let expected_fingerprint = expected_fingerprint.to_lowercase(); > > The to_lowercase would not be required if we control the format and use a lower > case x in the {byte:02x} fmt. So actually this needs some more checking. After all the expected_fingerprint is provided to the client via the S3ClientOptions, which have this as optional String parameter. There is no guarantee that this will stem from a config, where it is checked a-priori. So instead of doing a to_lowercase() here, during client instantiation there should happen some check along the lines of (untested): ```rust @@ -105,7 +107,17 @@ impl S3Client { /// Creates a new S3 client instance, connecting to the provided endpoint using https given the /// provided options. pub fn new(options: S3ClientOptions) -> Result { - let expected_fingerprint = options.fingerprint.clone(); + let expected_fingerprint = if let Some(ref fingerprint) = options.fingerprint { + match CERT_FINGERPRINT_SHA256_SCHEMA { + Schema::String(ref schema) => schema + .check_constraints(fingerprint) + .context("invalid fingerprint provided")?, + _ => unreachable!(), + } + Some(fingerprint.to_lowercase()) + } else { + None + }; let verified_fingerprint = Arc::new(Mutex::new(None)); let trust_openssl_valid = Arc::new(Mutex::new(true)); let mut ssl_connector_builder = SslConnector::builder(SslMethod::tls())?; ``` That should make this more robust. > >> + if expected_fingerprint == certificate_fingerprint { >> + return Ok(Some(certificate_fingerprint)); >> + } >> + } >> + >> + Err(format_err!( >> + "unexpected certificate fingerprint {certificate_fingerprint}" >> + )) >> + } > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel