Pluggable Crypto / Update reqwest 0.13

[tustvold]

Which issue does this PR close?

Closes #462 #576 #413.

Rationale for this change

Adds the ability to compile object_store without a dependency on ring, and instead use a user-provided crypto implementation.

What changes are included in this PR?

Are there any user-facing changes?

[tustvold]

One aspect that is potentially worth highlighting is that it if skip_signature is false, it should be possible to instantiate without a crypto provider.

The downside is that the absence is not necessarily detected until request time, I debated including a check in build, but in many cases it isn't always immediately obvious what crypto provision is necessary.

[tustvold]

I'm not a massive fan of the feature explosion, but this was the only way to avoid needing to cut a breaking release.

[tustvold]

I opted to allow for incremental computation to somewhat future-proof this API

[tustvold]

I debated making this method fallible, but decided any error can be returned by finish

[tustvold]

This formulation is a bit funky, but was the best I could come up with that would:

• Avoid needing breaking changes to AwsAuthorizer and AzureAuthorizer
• Allow not specifying a crypto provider if skip_signature is enabled

[tustvold]

This is technically a breaking change, however, the previous error type was not public and therefore there wasn't any way to actually name it. I think we can therefore get away with this change

[tustvold]

TBC this will need a LOT more testing before I'd feel comfortable merging it, it might be safest to leave for a breaking release just because of the heightened risk of breakage...

[tustvold]

I included the fallibility because of openssl - but I'm not sure if any of those errors are actually worth propagating, or if they would imply incorrect usage of openssl...

[tustvold]

@JakeDern you don't suppose you could test this against Azure blob storage, I no longer have access to any azure accounts with which to test this

[JakeDern]

@tustvold Tried it out this morning, it worked great for me - Thank you for taking this up!

I tried it with both the built-in ring implementation and with a quick openssl implementation. Here it is in case you would like to include something similar as an example:

use object_store::azure::MicrosoftAzureBuilder;
use object_store::client::{
    CryptoProvider, DigestAlgorithm, DigestContext, HmacContext, Signer, SigningAlgorithm,
};
use object_store::path::Path;
use object_store::{ObjectStore, ObjectStoreExt, Result};
use openssl::hash::MessageDigest;
use openssl::pkey::{PKey, Private};
use openssl::sign::Signer as OpenSslSigner;
use std::sync::Arc;

#[derive(Debug, thiserror::Error)]
pub enum OpenSslCryptoError {
    #[error("OpenSSL error: {0}")]
    OpenSsl(#[from] openssl::error::ErrorStack),

    #[error("No RSA key found in PEM file")]
    MissingKey,
}

impl From<OpenSslCryptoError> for object_store::Error {
    fn from(value: OpenSslCryptoError) -> Self {
        Self::Generic {
            store: "OpenSslCryptoProvider",
            source: Box::new(value),
        }
    }
}

#[derive(Debug, Default)]
pub struct OpenSslCryptoProvider;

impl CryptoProvider for OpenSslCryptoProvider {
    fn digest(&self, algorithm: DigestAlgorithm) -> Result<Box<dyn DigestContext>> {
        let md = match algorithm {
            DigestAlgorithm::Sha256 => MessageDigest::sha256(),
            _ => unimplemented!("unsupported digest algorithm: {:?}", algorithm),
        };
        let hasher = openssl::hash::Hasher::new(md).map_err(OpenSslCryptoError::from)?;
        Ok(Box::new(OpenSslDigestContext {
            hasher,
            output: Vec::new(),
        }))
    }

    fn hmac(&self, algorithm: DigestAlgorithm, secret: &[u8]) -> Result<Box<dyn HmacContext>> {
        let md = match algorithm {
            DigestAlgorithm::Sha256 => MessageDigest::sha256(),
            _ => unimplemented!("unsupported digest algorithm: {:?}", algorithm),
        };
        let key = PKey::hmac(secret).map_err(OpenSslCryptoError::from)?;
        let signer = OpenSslSigner::new(md, &key).map_err(OpenSslCryptoError::from)?;
        Ok(Box::new(OpenSslHmacContext {
            signer,
            output: Vec::new(),
        }))
    }

    fn sign(&self, algorithm: SigningAlgorithm, pem: &[u8]) -> Result<Box<dyn Signer>> {
        match algorithm {
            SigningAlgorithm::RS256 => {
                let key = PKey::private_key_from_pem(pem).map_err(OpenSslCryptoError::from)?;
                Ok(Box::new(OpenSslRsaSigner { key }))
            }
            _ => unimplemented!("unsupported signing algorithm: {:?}", algorithm),
        }
    }
}

struct OpenSslDigestContext {
    hasher: openssl::hash::Hasher,
    output: Vec<u8>,
}

impl DigestContext for OpenSslDigestContext {
    fn update(&mut self, data: &[u8]) {
        let _ = self.hasher.update(data);
    }

    fn finish(&mut self) -> Result<&[u8]> {
        let digest = self.hasher.finish().map_err(OpenSslCryptoError::from)?;
        self.output = digest.to_vec();
        Ok(&self.output)
    }
}

struct OpenSslHmacContext {
    signer: OpenSslSigner<'static>,
    output: Vec<u8>,
}

impl HmacContext for OpenSslHmacContext {
    fn update(&mut self, data: &[u8]) {
        let _ = self.signer.update(data);
    }

    fn finish(&mut self) -> Result<&[u8]> {
        self.output = self
            .signer
            .sign_to_vec()
            .map_err(OpenSslCryptoError::from)?;
        Ok(&self.output)
    }
}

#[derive(Debug)]
struct OpenSslRsaSigner {
    key: PKey<Private>,
}

impl Signer for OpenSslRsaSigner {
    fn sign(&self, data: &[u8]) -> Result<Vec<u8>> {
        let mut signer = OpenSslSigner::new(MessageDigest::sha256(), &self.key)
            .map_err(OpenSslCryptoError::from)?;
        signer.update(data).map_err(OpenSslCryptoError::from)?;
        let signature = signer.sign_to_vec().map_err(OpenSslCryptoError::from)?;
        Ok(signature)
    }
}

#[tokio::main(flavor = "current_thread")]
async fn main() -> std::result::Result<(), Box<dyn std::error::Error>> {
    let crypto_provider = Arc::new(OpenSslCryptoProvider);

    let azure = MicrosoftAzureBuilder::new()
        .with_account("mystorageaccount")
        .with_access_key("myaccesskey")
        .with_container_name("mycontainer")
        .with_crypto_provider(crypto_provider)
        .build()?;

    // Write a file
    let path = Path::from("example_openssl.txt");
    let data = bytes::Bytes::from("Hello from OpenSSL crypto provider!");
    azure.put(&path, data.into()).await?;

    // Read it back
    let result = azure.get(&path).await?;
    let bytes = result.bytes().await?;
    println!("Content: {}", String::from_utf8_lossy(&bytes));

    Ok(())
}

[tustvold]

I've rolled the reqwest 0.13 upgrade into this. Currently this effectively forces us to switch to aws-lc-rs, but I will raise this upstream as I suspect this is unintentional

Edit: seanmonstar/reqwest#2924 (comment)

[tustvold]

Suggested change

	//! Note: for TLS to work you will additionally need to register ring as the default rustls cryptography
	//! Note: for TLS to work you will may need to register ring as the default rustls cryptography

As whether you need this depends on what HTTPConnector you are using.

[tustvold]

Given this change is now spicy enough that I don't think it should go in a non-breaking release, I opted to take the opportunity to remove this feature flag. People can register the certificates manually if they wish to.

[crepererum]

That's elegant 👍

Could you apply your own GitHub change recommendations (I think there's one doc type/change)?

Maybe we can aim for another breaking release in Q2.

[crepererum]

It seems that cargo-deny should be able to do that, but I DON'T think it can ban dependencies based on features:

https://embarkstudios.github.io/cargo-deny/checks/bans/cfg.html

[abonander]

Hey folks, is this just blocked on review, or fixing CI, or a maintainer decision, or something else?

It turns out we could really use this in ClickHouse, as we're trying to eliminate ring from our Rust deps for FIPS compliance reasons, and because it's causing CI issues: ClickHouse/ClickHouse#96856 (comment)

We already have a fork of OpenSSL that's linked in as part of the build process, so it'd be preferable to just use that if we can. I was going to submit a PR just adding openssl as an optional feature when I saw this, so I'm happy to help move this across the finish line in anyway I can.

[crepererum]

Hey folks, is this just blocked on review, or fixing CI, or a maintainer decision, or something else?

It's queued for the next major release, which is now tracked in #657.

[crepererum]

@tustvold do you have time to rebase this PR, since we're now in the 0.14 merge phase and could merge it? (otherwise someone else can pick this work up)