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

chore: upgrade to rustls 0.20 #1367

Closed
wants to merge 9 commits into from

Conversation

satyarohith
Copy link

@satyarohith satyarohith commented Oct 26, 2021

All of the tests pass. Clean up left to do.

Closes #1363

@satyarohith satyarohith marked this pull request as ready for review November 12, 2021 06:57
@satyarohith satyarohith changed the title WIP: upgrade to rustls 0.20 chore: upgrade to rustls 0.20 Nov 12, 2021
@satyarohith
Copy link
Author

@seanmonstar, PR is ready for review. :)

@seanmonstar
Copy link
Owner

Thank you so much! I glanced through it, and it looks right, though granted I'm not familiar with the exact changes in rustls. Looks like there's some rustfmt changes needed.

@djc
Copy link
Contributor

djc commented Nov 12, 2021

I haven't yet looked at this in detail but it looks strange to me since currently released versions of hyper-rustls don't actually support rustls 0.20, so I expect there might be something fishy going on here?

@satyarohith
Copy link
Author

I haven't yet looked at this in detail but it looks strange to me since currently released versions of hyper-rustls don't actually support rustls 0.20, so I expect there might be something fishy going on here?

Hi @djc :)

It's currently using the code from the PR (rustls/hyper-rustls#156). I guess we would have to wait until it's merged and released.

@djc
Copy link
Contributor

djc commented Nov 13, 2021

Ah, sorry, I got confused (probably having to do with the git rev being at the end of the Cargo dep and me reviewing this on a mobile device initially). Hopefully we can get a new hyper-rustls out soon.

@g2p
Copy link

g2p commented Nov 14, 2021

As a heads-up, you might want to use the new builder interfaces added in the hyper-rustls PR (ClientConfigExt and ConnectorBuilder); they seem to handle all the configuration you are doing manually, and they also allow configuring the connector to be HTTPS-only.
If they don't handle it all, let us know!

Cargo.toml Outdated Show resolved Hide resolved
seanmonstar and others added 2 commits November 24, 2021 09:35
Co-authored-by: Paolo Barbolini <paolo@paolo565.org>
@seanmonstar
Copy link
Owner

Looks like something about pulling in the actual release made it so rustls_pemfile is no longer found?

@djc
Copy link
Contributor

djc commented Nov 24, 2021

Previously the PEM functionality was part of the rustls crate, so enabling the rustls feature would pull it in. Now it's a separate crate, so the Cargo rustls feature needs to make sure to enable the optional rustls-pemfile dependency.

@BiagioFesta
Copy link
Contributor

As @djc said, we need to enable rustls-pemfile dependency (to decode pem files) when rustls feature is enabled.

Something like:

diff --git a/Cargo.toml b/Cargo.toml
index 9c5a879..ea7c57b 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -70,7 +70,7 @@ __tls = []
 
 # Enables common rustls code.
 # Equivalent to rustls-tls-manual-roots but shorter :)
-__rustls = ["hyper-rustls", "tokio-rustls", "rustls", "__tls"]
+__rustls = ["hyper-rustls", "tokio-rustls", "rustls", "__tls", "rustls-pemfile"]
 
 # When enabled, disable using the cached SYS_PROXIES.
 __internal_proxy_sys_no_cache = []

should fix the current CI issue

@seanmonstar
Copy link
Owner

This was picked up and finished in #1388, thanks!

@satyarohith satyarohith deleted the upgrade_rustls branch November 30, 2021 01:19
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.

Port reqwest to rustls 0.20
6 participants