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

Suggestion: Upgrade to rustls 0.22 and don't use its default-features #2136

Open
EmilyMatt opened this issue Feb 18, 2024 · 11 comments
Open
Labels
rfc Request for comments. More discussion would help move this along.

Comments

@EmilyMatt
Copy link

One of the most groundbreaking changes of the 0.22 release of rustls is the separation of the 'ring' crate into a feature and allowing for custom cryptography providers.

This is one of the heaviest crates to compile, and while it is still widely used in most cases, in my opinion, the reqwest crate should allow an opt-out.

I suggest that while upgrading to 0.22, we add "default-features=false" to rustls and add a "ring" feature that will propagate to rustls.

This feature can be in reqwest's default features, which will still allow users to manually opt out of it.

Note that the reason rustls is not automatically upgraded and I open this issue is that reqwest demands the "dangerous_configuration" feature which no longer exists on 0.22, rendering cargo unable to resolve this dependency.

The functionalities enabled by this feature are now always available, under the "danger" module.

This is possibly breaking for users who use reqwest without default features enabled, and I feel should be marked for the 0.12 release.

Let me know your thoughts.

@seanmonstar
Copy link
Owner

Does it work with default features disabled? What happens if we do that?

@EmilyMatt
Copy link
Author

If the feature is disabled then there is no ClientConfig::builder(), as that uses ring by default, another function is available. ClientConfig::builder_with_provider(), allowing usage of any provider the user wishes, notable options are ring, aws_lc_rs, and a new emerging one rustls-rustcrypto, which is still in very early stages.

It seems rustls has also upgraded the rustls_pemfile crate to version 2.0, and has also both moved a lot of its structs to the pki_types module, and changed some of their APIs.

All of which induce a lot of breaking changes for us, but could possible be mitigated when passed on to user facing API.
At any rate, this seems more of a complex problem than I initially thought.
I can try and work on an initial POC to see how this would look for users in comparison with previous versions of reqwest, but all in all I believe this needs to be carefully considered on the roadmap, I believe this upgrade will have to be made eventually, but perhaps not directly for 0.12.

@MightyPork
Copy link

Even if you don't disable default features, upgrading to rustls 0.22 would be very useful.

I work on a large project where we finally removed dependencies on rustls 0.18 and 0.19 and tokio 0.2 but now we have rustls 0.21 and 0.22. Having just one version would be a great for compilation time and binary size. Reqwest is one of the last libraries we're waiting for.

@djc
Copy link
Contributor

djc commented Mar 6, 2024

I don't think it makes sense for reqwest not to pick one of rustls' crypto providers as the default. rustls 0.23 has made aws-lc-rs the default (which is a little controversial because aws-lc-rs depends on a bunch of C code, whereas ring has focused more on moving code into Rust) so it could make sense to match that default.

@EmilyMatt
Copy link
Author

@djc
I have not timed them both, but I feel that ring takes ages to compile, and if aws-lc-rs is faster, that might be a major factor in my decision for a crypto provider.
Though I won't pretend to know the various politics involved...

At any rate, I feel this issue(and the ones it entails) deserves a spot in the roadmap, at least.
I do think that using an additive feature list is the better approach here, so the top-level rustls feature will use aws-lc-rs, but it would be possible to use reqwest without being forced to choose either of the crypto providers.

@seanmonstar If you know when you'd wish this to take place in semver timelines, I can make a PR that will at the least upgrade the relevant crates.

@djc
Copy link
Contributor

djc commented Mar 6, 2024

IME aws-lc-rs takes more time to compile than ring does, and it also needs a bunch more tooling installed.

@asonix
Copy link
Sponsor

asonix commented Mar 6, 2024

I have historically opted to use rustls over openssl to reduce build complications and enable producing static binaries. if aws-lc-rs increases the required build tooling in a meaningful way I'd like to avoid it

@gaetanww
Copy link

gaetanww commented Mar 27, 2024

Why couldn't we allow the reqwest user to choose which rustls backend to use?
For my part, I'd like to bring in my own TLS provider with rustls but still use reqwest for HTTP. Would that be an option?

(Edited to make my comment clearer)

@djc
Copy link
Contributor

djc commented Mar 27, 2024

Why couldn't we allow the reqwest user to choose which rustls backend to use?
For my part, I'd like to bring in my own TLS provider with rustls but still use reqwest for HTTP. Would that be an option?

I think this already works:

https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html#method.use_preconfigured_tls

@mgorny
Copy link

mgorny commented Mar 28, 2024

Gentle ping. Is there any chance to move to rustls 0.23 and drop the dependency on ring? uv uses reqwest, and because of extremely bad platform support in ring, we can't use it on most platforms.

@CryZe
Copy link

CryZe commented Mar 28, 2024

@mgorny ring gained a lot of platform support recently, I don't believe there's many platforms left that aren't supported.

@seanmonstar seanmonstar added the rfc Request for comments. More discussion would help move this along. label Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Request for comments. More discussion would help move this along.
Projects
None yet
Development

No branches or pull requests

8 participants