Enable feature rustls-tls-native-roots for reqwest#7697
Enable feature rustls-tls-native-roots for reqwest#7697joncinque merged 1 commit intoanza-xyz:masterfrom
rustls-tls-native-roots for reqwest#7697Conversation
8089648 to
e3a9a36
Compare
|
@yihau would you know the right person to review this? |
e3a9a36 to
0fc66de
Compare
joncinque
left a comment
There was a problem hiding this comment.
Thanks for your contribution! It makes sense to me, but let's localize the change to avoid pulling in more dependencies for crates unnecessarily.
| reed-solomon-erasure = "6.0.0" | ||
| regex = "1.11.2" | ||
| reqwest = { version = "0.12.23", default-features = false } | ||
| reqwest = { version = "0.12.23", default-features = false, features = ["rustls-tls-native-roots"] } |
There was a problem hiding this comment.
We shouldn't enable all of these features for every crate using reqwest in the workspace.
Instead, can you only make the change in cli/Cargo.toml?
There was a problem hiding this comment.
Fair point. I've enabled it for solana-cli and solana-cargo-build-sbf. We also have agave-install but since it's not only just a bin but also a lib published on crates.io I haven't enabled the feature for it.
0fc66de to
543d3cb
Compare
543d3cb to
1c1c1ab
Compare
joncinque
left a comment
There was a problem hiding this comment.
Looks great, thanks!
And if you want, you can also add the feature to agave-install -- even though it's technically published as a lib, I think that's more to facilitate having multiple bins.
Also, it doesn't have any dependents: https://crates.io/crates/agave-install/reverse_dependencies, so it should be safe to change.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7697 +/- ##
=========================================
- Coverage 83.1% 83.0% -0.1%
=========================================
Files 812 812
Lines 356963 356963
=========================================
- Hits 296642 296559 -83
- Misses 60321 60404 +83 🚀 New features to boost your workflow:
|
|
Merging this PR, feel free to create a new one for agave-install -- thanks again for your contribution! |
Problem
Since #5181, some cli apps like
solana-cliaren't working well on environments that CAs are needed to connect to the RPC endpoints.That's because its dependency
reqwestisn't enabling any feature for loading CAs. I believe that it's reasonable to enablerustls-tls-native-roots, which loads system CAs viarustls-native-certs, becausesolana-cliis not a library but cli app, so the users can't really load CAs manually.But why it had been working well before #5181? That was purely by accident by resolving with another dependency here:
agave/Cargo.toml
Line 271 in 47c0383
goauthenablesreqwest'sdefault-tlsfeature by default, and that feature automatically loads system CAs via OpenSSL.
Before #5181, there had been only single version of
reqwestacross the workspace, so it had been resolved withdefault-tlsfeature turned on implicitly. (And every tls things had been done via OpenSSL, notrustls)But that PR has introduced the
reqwest = "0.12", so there are two versions ofreqwestnow,0.11and0.12. The former is still enablingdefault-tlsfeature but the later now lacks of it.Summary of Changes
This makes cli apps like
solana-clito load system CAs