-
Notifications
You must be signed in to change notification settings - Fork 111
Add support for aws-lc-rs #576
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty lukewarm on adding a load of extra feature flags, ultimately my opinion remains unchanged from #413 (comment) that we should make the crypto provider extensible.
#462 is still on my radar, perhaps I'll have some time to get back to it over the christmas break.
Edit: It is also generally good practice to disclose the use of AI tooling, it is disrespectful to reviewers to not do so.
| quick-xml = { version = "0.38.0", features = ["serialize", "overlapped-lists"], optional = true } | ||
| rand = { version = "0.9", default-features = false, features = ["std", "std_rng", "thread_rng"], optional = true } | ||
| reqwest = { version = "0.12", default-features = false, features = ["rustls-tls-native-roots", "http2"], optional = true } | ||
| reqwest = { version = "0.12", default-features = false, features = ["http2", "rustls-tls-webpki-roots-no-provider"], optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change that will likely break most clients
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't, it allows users to choose their own implementation of rustls instead of forcing them to use native tls which is harder to compile as it requires openssl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
native-roots doesn't depend on openssl, it concerns where it sources the trust store.
| cloud = ["serde", "serde_json", "quick-xml", "hyper", "reqwest", "reqwest/stream", "chrono/serde", "base64", "rand", "http-body-util", "form_urlencoded", "serde_urlencoded"] | ||
| azure = ["cloud", "httparse", "ring"] | ||
| gcp = ["cloud", "rustls-pki-types", "ring"] | ||
| aws = ["cloud", "md-5", "ring"] | ||
| http = ["cloud", "ring"] | ||
| azure-aws-lc = ["cloud", "httparse", "aws-lc"] | ||
| gcp-aws-lc = ["cloud", "rustls-pki-types", "aws-lc"] | ||
| aws-aws-lc = ["cloud", "md-5", "aws-lc"] | ||
| http-aws-lc = ["cloud", "aws-lc"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this explosion of feature flags, it will make testing really complicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The aws-lc flags are not necessary, it just avoids to redefine dependencies for users who want to use aws-lc. Another possibility is to use ring in the default feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my past experience using native tls required openssl, then I'm not an expert on this. But using rustls no provider allows more control on which rustls crypto provider is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[cfg(feature = "ring")] | ||
| #[allow(unused_imports)] | ||
| pub(crate) use ring as crypto; | ||
|
|
||
| #[cfg(feature = "aws-lc")] | ||
| #[allow(unused_imports)] | ||
| pub(crate) use aws_lc_rs as crypto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this formulation ends up making the feature flags mutually exclusive which goes against best practice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then what should be done ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
The problem is that this library is the maintained library for accessing parquet but uses ring instead of aws-lc which has become the standard. Something much easier would be to switch to aws-lc instead of using ring. It changes almost nothing and avoids issues with ring. |
|
I understand the challenge, it's working out how to achieve this in a way that doesn't break existing use-cases. IMO aws-lc represents a significant regression for the vast majority of users, e.g. longer build times, less portable, etc... , but so long as users don't have to use aws-lc, perhaps I can grin and bear it... That being said we need to do this carefully, ultimately reqwest IIRC forces ring to be included anyway, so providing an optional way to also bundle aws-lc if the user wants is probably the way to go... Although I'd still prefer this to be an out of tree concern. |
|
I disagree regarding reqwest, it doesn't require ring. If reqwest is configured as I did in my PR it allows to choose which implementation of rustls is used. You can see how it's done here I know because I've worked a lot at some point on avoiding native tls in the project I work on and configuring rustls to use aws-lc. The common choices are native tls, ring and aws-lc. Aws-lc is the best choice as it's the most validated library, something called FIPS. I don't think that aws-lc is problematic in terms of compile time. Basically aws-lc can be used in object store while still allowing users to use ring for reqwest if they prefer. It's just that using ring in object store adds the dependency back for people who don't want to use ring, which is an experimental library as someone mentioned in a previous comment. |
|
Right which breaks use-cases for people who rely on system certs not part of webpki 😅. It's possible using Anyway this needs some care to do correctly, I can try to find some time over the christmas break to chart a path forward |
|
I think the correct way as a library is not force a use case of rustls. I've seen libraries do this, but this is actually what breaks builds, not configuring reqwest in a minimal way which allows users to use what they want for rustls. Good if you can make some progress on this. If you want to avoid more feature flags while being more future proof, I think that aws-lc is a better choice than ring. Up to you to decide the best solution, I'm just a user of this project trying to not have to use ring. It's also possible to remove the extra aws-lc feature flags but it makes it harder to use them. |
Cryptographic backend abstraction
This PR introduces support for selecting between
ringandaws-lc-rsas cryptographic backends at compile time, providing users with flexibility in their cryptographic library choice while maintaining API compatibility.Changes
Feature flags and dependencies
Added
aws-lc-rsas an optional dependency with feature flagaws-lc.Updated
Cargo.tomlto support bothringandaws-lc-rsbackends:ringremains the default backend (included indefaultfeatures).azure-aws-lc,gcp-aws-lc,aws-aws-lc,http-aws-lcfor usingaws-lc-rswith specific cloud providers.ringandaws-lcare mutually exclusive.Updated
reqwestTLS configuration fromrustls-tls-native-rootstorustls-tls-webpki-roots-no-providerto support both backends.Cryptographic abstraction layer
Created new
crypto_backend.rsmodule that provides a unified interface overringandaws-lc-rs.The module re-exports the chosen backend as
crypto, allowing existing code to work with either backend transparently.Added
rsa_key_modulus_len()helper function to abstract API differences betweenringandaws-lc-rsfor RSA key operations.Code updates
Updated AWS client (
src/aws/client.rs) to use the abstracted crypto backend instead of directringimports.Updated GCP credential handling (
src/gcp/credential.rs) to use the abstracted crypto backend and the newrsa_key_modulus_len()helper.Updated utility functions (
src/util.rs) for HMAC-SHA256 and SHA256 digest operations to use the abstracted backend.Updated all feature gate conditions throughout the codebase to include the new
*-aws-lcfeature variants:builder.rs: Updated feature gates for HTTP request building methods.header.rs: Updated feature gates for header parsing functions.body.rs: Updated feature gates for JSON deserialization.mod.rs: Updated feature gates for cloud provider modules and client options.retry.rs: Updated feature gates for retry configuration.s3.rs: Updated feature gates for S3-specific types.token.rs: Updated feature gates for token caching.lib.rs: Updated feature gates for module exports.Design decisions
API compatibility: Since
aws-lc-rsis API-compatible withringfor most operations, a simple re-export pattern was used rather than a trait-based abstraction, minimizing code complexity.Compile-time selection: The backend choice is made at compile time via feature flags, avoiding runtime overhead and ensuring type safety.
Mutual exclusivity: The
ringandaws-lcfeatures are mutually exclusive to prevent conflicts and ensure clear backend selection.Backward compatibility: The default configuration continues to use
ring, ensuring existing users are not affected unless they explicitly opt intoaws-lc-rs.Testing considerations
All existing tests should continue to pass with the default
ringbackend.New feature flag combinations (
*-aws-lc) should be tested to ensure proper compilation and functionality with theaws-lc-rsbackend.