-
Notifications
You must be signed in to change notification settings - Fork 271
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
chore(deps): Upgrade tokio-rustls to 0.26 #3419
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.
Consider pulling the rustls dependencies up to the workload (/Cargo.toml) so we can pin the featureset in one place
rustls::ServerConfig::builder() | ||
.with_cipher_suites(TLS_SUPPORTED_CIPHERSUITES) | ||
.with_safe_default_kx_groups() | ||
let mut provider = rustls::crypto::ring::default_provider(); |
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 the most invasive part of this upgrade. In order to set the supported cipher suites to just the ones we want, we have create a crypto provider separately from the provider.
Also, the AllowAnyAnonymousOrAuthenticatedClient
has been removed and replaced with client/server verifier builder methods.
Self(roots.into()) | ||
Self { | ||
roots: roots.into(), | ||
supported: rustls::crypto::ring::default_provider().signature_verification_algorithms, |
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 change I'm not entirely confident in. The cert verification methods below now take a list of supported algorithms instead of implicitly using the default, and this was the only reasonable way I was able to get the default list that the WebPkiServerVerifier
implementation we copied was using.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cipher_suites: vec![CipherSuite::TLS_NULL_WITH_NULL_NULL], | ||
compression_methods: vec![Compression::Null], | ||
extensions: vec![ClientExtension::make_sni(sni.borrow())], |
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 ClientExtension::make_sni
was made private (these aren't part of the rustls
public API), which is the motivator behind the changes in this file.
The purpose of this function is to generate a ClientHello
message for tests, and in my playing around I wasn't able to find a more ergonomic way of generating one outside of this. I'm definitely open to ideas so we don't have to rely on internal rustls
types for this.
cc5ff36
to
587e28f
Compare
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 looks good to me! great work.
as a small aside, i hadn't ever thought to try dependency.workspace = true
syntax before. that's nice. 🙂
This bumps rustls itself from 0.21 to 0.23, which comes with a few breaking API changes. Most of these are limited to types being moved or renamed, or how the certificate verifiers are constructed. Signed-off-by: Scott Fleener <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3419 +/- ##
==========================================
- Coverage 67.68% 66.80% -0.88%
==========================================
Files 332 388 +56
Lines 15158 18139 +2981
==========================================
+ Hits 10259 12118 +1859
- Misses 4899 6021 +1122
... and 162 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Self(roots.into()) | ||
Self { | ||
roots: roots.into(), | ||
supported: rustls::crypto::ring::default_provider().signature_verification_algorithms, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This bumps rustls itself from 0.21 to 0.23, which comes with a few breaking API changes. Most of these are limited to types being moved or renamed, or how the certificate verifiers are constructed. Signed-off-by: Scott Fleener <[email protected]>
Signed-off-by: Scott Fleener <[email protected]>
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.
Thanks for making the requested changes. Something is still amiss with the Cargo.lock, though. To fix this, I would probably run something akin to git co origin/main -- Cargo.lock && cargo check
. There's probably something funky in your dev setup that is causing gratuitous lock modification?
This seems to have been causing build/fetch errors in CI. Signed-off-by: Scott Fleener <[email protected]>
I've fixed the Cargo.lock, best as I can figure RA in vscode was clobbering lockfile changes for some reason. |
Good work, @sfleen! |
nice job, @sfleen! |
This bumps rustls itself from 0.21 to 0.23, which comes with a few breaking API changes. Most of these are limited to types being moved or renamed, or how the certificate verifiers are constructed.