-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Upgrade to hyper-rustls 0.27/rustls 0.23 #2225
base: master
Are you sure you want to change the base?
Conversation
Thanks so much! I do have some questions, to help plan how we could allow using the different crypto providers.
|
Yes. To be safe at compile time, we could use the
Those features would be non-additive, because rustls will panic if both are enabled. Also note that people might want to use a third-party crypto provider -- this is of course already possible via the |
Oh, so reqwest would be picking the crypto provider for the application as a whole? If any other library depends on |
The https://github.com/rustls/rustls/blob/main/rustls/src/crypto/mod.rs#L254 This will:
As such, I think the idea is that reqwest should rely on this and if people run into a run-time panic they should call See more discussion in rustls/rustls#1877. |
Won't it be more reasonable, to prefer one crypto to another instead of panicking at runtime? Or maybe a |
Feel free to contribute to the discussion in the upstream issue. |
I would like to suggest a change to be able to setup a custom provider without having to compile both ring and the custom provider.
|
FWIW, this PR was just intended to do the rustls upgrade while making as few other changes as possible. Obviously there could be a number of new Cargo features that help control the crypto provider used by rustls. But I think reqwest will want to pick a default crypto provider to make adopting rustls as easy as possible. |
My suggested changes still pick up ring as a default provider without adding any new feature, and leaving the possibility to control the crypto provider used by rustls. That's why I think it's interesting to consider this now. This can also be, of course, in a follow-up PR as you suggest. |
This comment was marked as off-topic.
This comment was marked as off-topic.
I do want this to merge. As to the status of this upgrade: I'm hesitant to merge and publish since anyone upgrading that was already using rustls v0.23 may suddenly start seeing a panic, because reqwest enables the alternative backend. (And I don't really want to "fix" that by switching to aws-lc.) My feeling is that I'd want to wait for a rustls version that doesn't have that issue. Discussion is in rustls/rustls#1877. |
@seanmonstar I think we can work around that issue by using I feel like a solution to rustls/rustls#1877 is unlikely to happen soon. |
0b8a99a
to
b9240eb
Compare
I've revised this to explicitly select a ring (using the existing Cargo features) or aws-lc-rs (using a parallel set of Cargo features) provider and use the selected crypto provider explicitly when constructing a rustls This is motivated in part by rustup concerns for making rustls the default TLS implementation, since some scenarios need P521 cipher suites which only aws-lc-rs supports. |
So the MSRV build fails with this error:
Not entirely sure what's up with that? From looking at the state of (Seems to work okay with current stable.) |
fwiw just running |
__rustls_crypto_ring = ["rustls-base", "rustls/ring"] | ||
__rustls_crypto_aws_lc = ["rustls-base", "rustls/aws_lc_rs"] |
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.
__rustls_crypto_ring = ["rustls-base", "rustls/ring"] | |
__rustls_crypto_aws_lc = ["rustls-base", "rustls/aws_lc_rs"] | |
__rustls_crypto_ring = ["rustls-base", "rustls?/ring"] | |
__rustls_crypto_aws_lc = ["rustls-base", "rustls?/aws_lc_rs"] |
a few well-placed questionmarks seem to allow this to work on 1.63
hyper-rustls 0.27.1 also bumps msrv from 1.63 to 1.64 |
#[cfg(feature = "__rustls")] | ||
#[cfg(any( | ||
feature = "__rustls_crypto_ring", | ||
feature = "__rustls_crypto_aws_lc-rs" |
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.
feature = "__rustls_crypto_aws_lc-rs" | |
feature = "__rustls_crypto_aws_lc" |
Feature name is wrong
let provider = rustls::crypto::ring::default_provider(); | ||
|
||
#[cfg(all( | ||
feature = "__rustls_crypto_aws_lc-rs", |
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.
feature = "__rustls_crypto_aws_lc-rs", | |
feature = "__rustls_crypto_aws_lc", |
feature name is wrong here too
#[cfg_attr(docsrs, doc(cfg(feature = "rustls-tls")))] | ||
#[cfg(any( | ||
feature = "__rustls_crypto_ring", | ||
feature = "__rustls_crypto_aws_lc-rs" |
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.
feature = "__rustls_crypto_aws_lc-rs" | |
feature = "__rustls_crypto_aws_lc" |
and here
docsrs, | ||
doc(cfg(any( | ||
feature = "__rustls_crypto_ring", | ||
feature = "__rustls_crypto_aws_lc-rs" |
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.
feature = "__rustls_crypto_aws_lc-rs" | |
feature = "__rustls_crypto_aws_lc" |
here
#[cfg(feature = "__rustls")] | ||
#[cfg(any( | ||
feature = "__rustls_crypto_ring", | ||
feature = "__rustls_crypto_aws_lc-rs" |
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.
feature = "__rustls_crypto_aws_lc-rs" | |
feature = "__rustls_crypto_aws_lc" |
at this point it's probably easier to add -rs
to Cargo.toml
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.
actually I'd suggest changing this just to rustls-base
since it doesn't compile with only that feature otherwise
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.
Either that or gate the Debug
impl behind this same cfg(any())
From my look into this PR, I think TlsBackend can't have a In that scenario it is required that a user pass in a a rustls-compatible clientconfig so that the Would it make sense to set |
I think these compile issues could have been caught by CI if the new features were added to the github actions workflow, also |
Sticks with ring as the default crypto provider for rustls for now (see #2136).
This is also a prerequisite for re-enabling a Quinn-based H3 backend, since Quinn is skipping rustls 0.22.