Change crypto provider from ring to aws-rust-lc

[faysou]

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

ring is currently the library used in this project as crypto functions provider.

I suggest that aws-lc-rs would be a better choice as it's FIPS compliant which could be a better selling point in some industries.

The migration from ring to aws-ls-rs can easily be done.

aws-lc-rs is also the default library used by rustls.

I could do a PR for this, but given it's dealing with security I suppose someone more used to contribute to this project will want to do it.

Describe the solution you'd like

Replace ring as explicit dependency of the crate with aws-lc-rs

Describe alternatives you've considered

Additional context

If aws-lc-rs is already used in some project, having to compile ring is actually redundant. Both libraries are similar in purpose but aws-lc-rs has a higher standard in terms of validation.

[faysou]

I can do a PR if someone from this project allows me to.

[kylebarron]

Is there a way to support both? To allow users to opt-in to which is used?

[faysou]

Yes I had a similar issue a few weeks ago, and got this library to be modified.
You can have a default feature in your library that uses rustls/aws-lc-rs while still allowing rustls to be configured to use ring, similarly to what is done here with reqwest.

https://github.com/databento/databento-rs/blob/main/Cargo.toml#L21

Actually you are explicitely using ring in your code for crypto functions, I would suggest that aws-lc-rs is a better choice as it's more officially validated by amazon, you can have a look at it. In the library I work on (nautilus-trader), we have moved to aws-lc-rs as well and are trying to remove ring as dependency to reduce compile times as it doesn't make sense to compile aws-lc-rs and ring as they do the same thing.

[kylebarron]

I was thinking the opposite; have the default feature remain using ring for backwards compatibility but allow for some way to opt-in to aws-lc-rs.

[faysou]

It's up to you. You use ring explicitely in your code so the ring dependency cannot be avoided at the moment. Note that the default provider of rustls is aws-lc-rs.

[tustvold]

I think we should keep ring as the default at least initially, it is the older and arguably more battle-tested crate, and the corresponding code in object_store is well tested. I know some providers, e.g. rustls, have recently switched defaults over, however, I think at least initially aws-lc-rs should be an opt-in feature to avoid potential breakage. Ultimately the actual cryptographic algorithms in aws-lc-rs and ring are both just lifted from OpenSSL / BoringSSL (itself an OpenSSL fork), and so from a security standpoint should be largely similar, although I will concede compliance is a whole different story that sadly often has very little to do with actual security.

Edit: aws-lc-rs is also not a native rust crate, instead relying on bindings to aws-lc - this may cause issues when targetting some environments, e.g. WASM.

[faysou]

ring is not pure rust as well, it wraps some C and assembly from BoringSSL as well. The problem is that there are two similar libraries aws-lc-rs and ring that do the same thing but one is more heavily validated so based on that aws-lc-rs seems better.

And given that aws-lc-rs is now the default of rustls, it means that people who use the more validated library have to compile ring as well when using object_store, which is redundant.

Anyway it's just my point of view, it's up to this project to decide.

Edit: if you look at where ring is explicitely used in the library (search for ring::) it's in three files, and it would take one minute to switch to aws-lc-rs using an agent. I doubt this would impact downstream users, as it's in internal code.

[JakeDern]

I'll just add +1 to the fact that I'm currently blocked from using this crate for compliance reasons surrounding the ring crate.

Writing a simpler storage abstraction for my own use-case isn't too large of a deal, but the integration between this library and others in the arrow ecosystem (Parquet writer especially) is super convenient and I would love to be able to use it.

[tustvold]

We'd be open to contributions to allow switching between aws-lc-rs and ring, provided ring is kept as the default at least initially

[faysou]

There would need to be two versions of the file that explicitly uses ring in your library, ie. another one with aws-lc-rs. And a way to use one of them depending on if ring or aws-lc-rs is chosen. An AI agent would probably find a way in no time.

[faysou]

I've just made a PR related to my previous message.

[JakeDern]

I had also started poking around with an implementation. Mine is a little different from yours @faysou in that I was trying to avoid mutually exclusive features because my understanding is that those can cause problems if two different dependencies use them differently. The idea being the crate uses ring by default, but if some feature like open-ssl is enabled then it uses the openssl crate instead.

I also pulled everything out to a crypto module and tried to encapsulate it behind a trait. Here's as far as I got:

use std::ops::Deref;

/// Error type for crypto operations
/// TODO: Figure out a better pattern here
#[derive(Debug)]
pub(crate) enum CryptoError {
    #[cfg(feature = "open-ssl")]
    OpenSsl(openssl::error::ErrorStack),
}

impl std::fmt::Display for CryptoError {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            #[cfg(feature = "open-ssl")]
            CryptoError::OpenSsl(e) => write!(f, "OpenSSL error: {}", e),
        }
    }
}

impl std::error::Error for CryptoError {}

#[cfg(feature = "open-ssl")]
impl From<openssl::error::ErrorStack> for CryptoError {
    fn from(e: openssl::error::ErrorStack) -> Self {
        CryptoError::OpenSsl(e)
    }
}

/// Wrapper type for digests that abstracts over different crypto providers' types, some of which don't allocate
pub(crate) struct Digest<T> {
    inner: T,
}

impl<T> Digest<T> {
    fn new(inner: T) -> Self {
        Self { inner }
    }
}

impl<T: AsRef<[u8]>> Deref for Digest<T> {
    type Target = [u8];

    fn deref(&self) -> &Self::Target {
        self.inner.as_ref()
    }
}

/// Wrapper type for HMAC tags that abstracts over different crypto providers' types, some of which don't allocate
pub(crate) struct Tag<T> {
    inner: T,
}

impl<T> Tag<T> {
    fn new(inner: T) -> Self {
        Self { inner }
    }
}

impl<T: AsRef<[u8]>> Deref for Tag<T> {
    type Target = [u8];

    fn deref(&self) -> &Self::Target {
        self.inner.as_ref()
    }
}

pub trait Crypto {
    type DigestType: AsRef<[u8]>;
    type TagType: AsRef<[u8]>;

    fn digest_sha256(bytes: &[u8]) -> Result<Digest<Self::DigestType>, CryptoError>;
    fn hmac_sha256(secret: &[u8], bytes: &[u8]) -> Result<Tag<Self::TagType>, CryptoError>;
}

// Type alias to the current crypto provider based on feature flags
#[cfg(feature = "open-ssl")]
type CryptoProvider = openssl_crypto::OpenSslCrypto;

#[cfg(not(feature = "open-ssl"))]
type CryptoProvider = ring_crypto::RingCrypto;

// Type aliases for the associated types of the current provider
// Note: This seems to be necessary or we get ambiguous type errors on the public functions
type CurrentDigest = <CryptoProvider as Crypto>::DigestType;
type CurrentTag = <CryptoProvider as Crypto>::TagType;

pub(crate) fn digest_sha256(bytes: &[u8]) -> Result<Digest<CurrentDigest>, CryptoError> {
    CryptoProvider::digest_sha256(bytes)
}

pub(crate) fn hmac_sha256(secret: &[u8], bytes: &[u8]) -> Result<Tag<CurrentTag>, CryptoError> {
    CryptoProvider::hmac_sha256(secret, bytes)
}

pub(crate) fn hex_digest(bytes: &[u8]) -> Result<String, CryptoError> {
    let sha_256_hash = digest_sha256(bytes)?;
    Ok(hex_encode(&sha_256_hash))
}

pub(crate) fn hex_encode(bytes: &[u8]) -> String {
    use std::fmt::Write;
    let mut out = String::with_capacity(bytes.len() * 2);
    for byte in bytes {
        // String writing is infallible
        let _ = write!(out, "{byte:02x}");
    }
    out
}

#[cfg(feature = "open-ssl")]
mod openssl_crypto {
    use openssl::hash::{DigestBytes, MessageDigest};
    use openssl::pkey::PKey;
    use openssl::sign::Signer;

    use super::{Crypto, CryptoError, Digest, Tag};

    pub struct OpenSslCrypto;

    impl Crypto for OpenSslCrypto {
        type DigestType = DigestBytes;
        type TagType = Vec<u8>;

        fn digest_sha256(bytes: &[u8]) -> Result<Digest<Self::DigestType>, CryptoError> {
            let digest = openssl::hash::hash(MessageDigest::sha256(), bytes)?;
            Ok(Digest::new(digest))
        }

        fn hmac_sha256(secret: &[u8], bytes: &[u8]) -> Result<Tag<Self::TagType>, CryptoError> {
            let key = PKey::hmac(secret)?;
            let mut signer = Signer::new(MessageDigest::sha256(), &key)?;
            signer.update(bytes)?;
            let hmac = signer.sign_to_vec()?;
            Ok(Tag::new(hmac))
        }
    }
}

#[cfg(not(feature = "open-ssl"))]
mod ring_crypto {
    use super::{Crypto, CryptoError, Digest, Tag};

    pub struct RingCrypto;

    impl Crypto for RingCrypto {
        type DigestType = ring::digest::Digest;
        type TagType = ring::hmac::Tag;

        fn digest_sha256(bytes: &[u8]) -> Result<Digest<Self::DigestType>, CryptoError> {
            let digest = ring::digest::digest(&ring::digest::SHA256, bytes);
            Ok(Digest::new(digest))
        }

        fn hmac_sha256(secret: &[u8], bytes: &[u8]) -> Result<Tag<Self::TagType>, CryptoError> {
            let key = ring::hmac::Key::new(ring::hmac::HMAC_SHA256, secret);
            let tag = ring::hmac::sign(&key, bytes);
            Ok(Tag::new(tag))
        }
    }
}

I'm happy to clean this up as well, I just need to write some tests and also move the code in the crate to use this instead.

[JakeDern]

Also I could swap out the open-ssl implementation in the above with aws-lc-rs, since it sounds like that's preferred. I was really just trying to get a feel for the implementation mechanisms since I haven't worked with features much