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

[Christian Ebner <c.ebner@proxmox.com>]

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

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<Self, Error> {
-        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