[pbs-devel] [PATCH proxmox v7 1/9] s3 client: add crate for AWS s3 compatible object store client

[Thomas Lamprecht]

Am 10.07.25 um 19:06 schrieb Christian Ebner:
> +    fn verify_certificate_fingerprint(
> +        openssl_valid: bool,
> +        context: &mut X509StoreContextRef,
> +        expected_fingerprint: Option<String>,
> +        trust_openssl: Arc<Mutex<bool>>,
> +    ) -> Result<Option<String>, 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::<Vec<&str>>()
> +            .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::<Vec<String>>()
    .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.

> +            if expected_fingerprint == certificate_fingerprint {
> +                return Ok(Some(certificate_fingerprint));
> +            }
> +        }
> +
> +        Err(format_err!(
> +            "unexpected certificate fingerprint {certificate_fingerprint}"
> +        ))
> +    }