Skip to content
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

Disable rustls's aws-lc-rs feature #71

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flip1995
Copy link

@flip1995 flip1995 commented Oct 28, 2024

When both the ring and aws-lc-rs features in rustls are enabled, the rustls CryptoProvider installer from crate features no longer works, as the provider to be used is ambiguous 1.

This is the default configuration and feature set that other dependencies like reqwest are using.

@flip1995 flip1995 changed the title Disable rustls' aws-lc-rs feature Disable rustls's aws-lc-rs feature Oct 28, 2024
@Narsil
Copy link
Collaborator

Narsil commented Dec 25, 2024

Sorry for the delay, but I fail to see the issue.

Is there any way we can reproduce the issue ?

Whatever the bug, I feel like it's a non fixable issue since the issue would arise with the other feature enabled. I think the solution provided in code is the solution, if both are specified, user needs to install the crypto provider manually (or the default). No ?
This is just how features work, they need to be able to be built with both enabled, right?

When both the `ring` and `aws-lr-rs` features in `rustls` are enabled,
the rustls `CryptoProvider` installer from crate features no longer
works, as the provider to be used is ambiguous [1].

This is the default configuration and feature set that other
dependencies like `reqwest` are using.

[1]: https://docs.rs/rustls/latest/src/rustls/crypto/mod.rs.html#261-282
@flip1995
Copy link
Author

I think the solution provided in code is the solution, if both are specified, user needs to install the crypto provider manually

Yes, this is basically the issue. All other crates in the ecosystem have their features set up in a way, that either the aws OR the ring provider is used and never both, depending on which features you set for those crates.

Let me describe the issue in a bit more detail:

rustls picks the provider automatically here, if either ring OR aws-lc-rs is set as a feature flag. If both features are active, it can't auto-select a provider and the user of any crate that depends on rustls (transitively) needs to select the provider. With the rustls default features enabled, aws-lc-rs is enabled as a provider.

Why is this a problem for hf-hub? Well, it isn't really your problem. But changing the rustls dependency, like I did in this PR, would make it fit better in the ecosystem.

As hf-hub with the rustls-tls feature

rustls-tls = ["dep:rustls", "reqwest/rustls-tls"]

chooses to use reqwest with the rustls-tls feature, hf-hub indirectly also chooses ring as a provider through the reqwest dependency. reqwest, hyper, tokio, ureq, ... are all setup, so that you can choose ring OR aws-lc-rs as a crypto provider through feature flags, or use (in the case of ureq) ring as the default provider w/o the option of picking aws-lc-rs.

So when hf-hub now enables the default features of rustls, while at the same time using reqwest as a dependency with ring as the provider, hf-hub forces all of its users to depend on both ring AND aws-lc-rs. This installs unnecessary dependencies for downstream users that only want to use ring, and forces them to specify a provider.

As the rustls only gets installed with the rustls-tls feature enabled, I think the best fix is to go the ureq route and choose ring as the provider.


To reproduce:

Using main branch of this repo:

$ cargo new hf-hub-dep-test
$ cd hf-hub-dep-test
$ cargo add --git https://github.com/huggingface/hf-hub --branch main --no-default-features --features rustls-tls,tokio,ureq
$ rg "aws-lc-rs" Cargo.lock
42:name = "aws-lc-rs"
1239: "aws-lc-rs",
1273: "aws-lc-rs",

Using the branch of this PR:

$ cargo new hf-hub-dep-test
$ cd hf-hub-dep-test
$ cargo add --git https://github.com/flip1995/hf-hub --branch rustls-features --no-default-features --features rustls-tls,tokio,ureq
$ rg "aws-lc-rs" Cargo.lock
# No output

@@ -43,7 +45,7 @@ default = ["default-tls", "tokio", "ureq"]
# These features are only relevant when used with the `tokio` feature, but this might change in the future.
default-tls = []
native-tls = ["dep:reqwest", "reqwest/default", "dep:native-tls", "ureq/native-tls"]
rustls-tls = ["dep:rustls", "reqwest/rustls-tls"]
rustls-tls = ["dep:reqwest", "dep:rustls", "reqwest/rustls-tls"]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added back the dep:reqwest to the rustls-tls feature, as it didn't make sense to me not to depend on this crate in the rustls-tls case. LMK if I missed something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants