Conversation
|
Will work on failing tests (apologies) |
|
We can now also update |
thanks @musicinmybrain, was waiting for the official release! Only thing to watch for is axoupdater and the update to |
9bdfe48 to
ec325cf
Compare
|
Hey @zanieb, wanted to check-in and see if y'all have a plan for this or if there's anything I can do to help to get this implemented? |
|
|
||
| /// Whether to load TLS certificates from the platform's native store [env: UV_NATIVE_TLS=] | ||
| /// Whether to use the native-tls TLS backend instead of rustls [env: UV_NATIVE_TLS=] | ||
| /// | ||
| /// By default, uv loads certificates from the bundled `webpki-roots` crate. The | ||
| /// `webpki-roots` are a reliable set of trust roots from Mozilla, and including them in uv | ||
| /// improves portability and performance (especially on macOS). | ||
| /// By default, uv uses the rustls TLS backend, which loads certificates from the platform's | ||
| /// native certificate store via the `rustls-platform-verifier` crate. This provides a good | ||
| /// balance of portability and performance across platforms. | ||
| /// | ||
| /// However, in some cases, you may want to use the platform's native certificate store, | ||
| /// especially if you're relying on a corporate trust root (e.g., for a mandatory proxy) that's | ||
| /// included in your system's certificate store. | ||
| /// However, in some cases, you may want to use the native-tls backend instead, which uses | ||
| /// the platform's native TLS implementation (e.g., SChannel on Windows, Secure Transport on | ||
| /// macOS, OpenSSL on Linux). | ||
| #[arg(global = true, long, value_parser = clap::builder::BoolishValueParser::new(), overrides_with("no_native_tls"))] | ||
| pub native_tls: bool, |
There was a problem hiding this comment.
Hm this is a bit different than what I'd expect. This means that
- We now use system certificates by default
- There's no way to for users to recover using webpki-riots
Right?
For users, --native-tls was a way to opt-in to reading certificates from the system. I think we'll probably need to make more changes here.
I presume there's a way to recover the webpki-roots behavior?
There was a problem hiding this comment.
I think the ideal rollout plan for us is probably something like
Here:
- Upgrade to the latest reqwests version
- Continue using
webpki-rootsby default - Add
--tls-backend <native-tls|rusttls>to allow choosing the tls backend
Next:
- Add
rusttlspreview feature which usesrusttlsinstead ofnative-tls
Next (breaking):
- Stabilize using
rusttlsby default
Next:
- Add
system-certs-defaultpreview feature which uses system certificates instead of webpki roots - Deprecate
--native-tlsin favor of--system-certsand--no-system-certs
Next (breaking):
- Stabilize the preview behavior
There was a problem hiding this comment.
I can look into this more. Looks like webpki-roots is an optional dependency in 0.13.1 but they were removed here in a subsequent commit that hasn't been released yet.
There was a problem hiding this comment.
I'll look into it a bit too. We can just depend on webpki-roots directly, I presume?
It's unfortunate the rollout needs to be so many steps but alas that's the way things need to be since we're so widely used.
There was a problem hiding this comment.
Yep, we can totally depend on it directly. And totally understand attempting to keep as much existing functionality as possible while allowing for incremental improvements.
So here's what I have as immediate requirements:
- depend on
webpki-rootsdirectly and support it as default - keep existing
--native-tlsflag? - add new
--tls-backend(hidden?) flag (add support forconflicts_with)- that flag can flex between native-tls and rustls w/ rustls-platform-verifier
Good news, implementation should be manageable (more so than I originally thought). After reviewing the release notes for reqwest, we have the following:
rustls roots features removed, rustls-platform-verifier is used by default.
To use different roots, call tls_certs_only(your_roots)
I'll work to get this implemented.
There was a problem hiding this comment.
Scratch that. tls_certs_only() would only work with webpki-root-certs (full X.509 certs) vs webpki-roots (TrustAnchor objects).
So we'll go the route of implementing a rustls config:
use rustls::RootCertStore;
// Create a root certificate store, webpki-roots into the store
let mut root_store = RootCertStore::empty();
root_store.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned());
// Build a rustls ClientConfig with the root store
let mut tls_config = rustls::ClientConfig::builder()
.with_root_certificates(root_store)
// Use the preconfigured TLS config with reqwest
let client = reqwest::Client::builder()
.tls_backend_preconfigured(tls_config)
.build()?;Here's the sample webpki-root-certs implementation:
let certs: Result<Vec<reqwest::Certificate>, _> = webpki_root_certs::TLS_SERVER_ROOT_CERTS
.iter()
.map(|cert_der| reqwest::Certificate::from_der(cert_der))
.collect();
let certs = certs?;
let client = reqwest::Client::builder()
.tls_backend_rustls()
.tls_certs_only(certs)
.build()?;There was a problem hiding this comment.
@zanieb ended up going the tls_certs_only() route. I've made comments throughout, let me know what you think.
|
All dependency updates are merged and published. |
311d8ac to
9694a89
Compare
|
rebased onto uv will work to implement these suggestions: #17543 (comment) |
9694a89 to
2b18f63
Compare
| TlsBackend::Rustls => { | ||
| let client_builder = client_builder.tls_backend_rustls(); | ||
| // Merge custom certificates for rustls with platform-verifier | ||
| if !custom_certs.is_empty() { | ||
| client_builder.tls_certs_merge(custom_certs.to_vec()) | ||
| } else { | ||
| client_builder | ||
| } | ||
| } |
There was a problem hiding this comment.
Decided to keep this in via TlsBackend since it will be under the hidden flag.
| TlsBackend::RustlsWebpki => { | ||
| // Convert webpki-root-certs to reqwest::Certificate objects | ||
| let webpki_certs: Vec<Certificate> = webpki_root_certs::TLS_SERVER_ROOT_CERTS | ||
| .iter() | ||
| .filter_map(|cert_der| Certificate::from_der(cert_der).ok()) | ||
| .collect(); | ||
|
|
||
| let client_builder = client_builder | ||
| .tls_backend_rustls() | ||
| .tls_certs_only(webpki_certs); | ||
|
|
||
| // Merge custom certificates on top of webpki-root-certs | ||
| if !custom_certs.is_empty() { | ||
| client_builder.tls_certs_merge(custom_certs.to_vec()) | ||
| } else { | ||
| client_builder | ||
| } | ||
| } |
There was a problem hiding this comment.
Ended up using webpki-root-certs here, instead of webpki-roots.
Three reasons:
- While slightly larger in size,
webpki-root-certsallows to use the new rustls backend, but only with the certs we pass thanks totls_certs_only. I think this is a better and less complex implementation that still meets our default needs from the previous version. - With
webpki-roots, we'd have to pull inrustls, as well, to configure our ownRootCertStoreto store certs (in addition to auth, cert trust, SNI, ALPN), so that we can use with reqwest'stls_backend_preconfiguredmethod. - Due to
tls_backend_preconfiguredrequiring a TlsConfig, we'd having to parse through the custom cert env vars again to add them to our cert store and handling mTLS inside the building of the root store. While I could have spent some time to find a way a solution, I think this approach is clean.
| // Specify identity encoding to prevent double compression from async_http_range_reader and reqwest | ||
| headers.insert( | ||
| reqwest::header::ACCEPT_ENCODING, | ||
| reqwest::header::HeaderValue::from_static("identity"), | ||
| ); |
There was a problem hiding this comment.
More info here: astral-sh/async_http_range_reader#3 (comment)
crates/uv/tests/it/run.rs
Outdated
| let script = context.temp_dir.child("term_signal.py"); | ||
| script.write_str(indoc! {r" | ||
| import os | ||
| os.kill(os.getpid(), 11) | ||
|
|
||
| os.kill(os.getpid(), 15) | ||
| "})?; | ||
| let status = context.run().arg(script.path()).status()?; | ||
| assert_eq!(status.code().expect("a status code"), 139); | ||
| assert_eq!(status.code().expect("a status code"), 143); |
There was a problem hiding this comment.
Had some issues with this test hanging. Ran this code in my REPL, where it also hung. Open to suggestions.
There was a problem hiding this comment.
What is your system? I've never heard of it hanging
There was a problem hiding this comment.
yea, it seemed fishy from the jump, but ended up reverting it here after a successful re-run: 27d53ce
| use tracing::debug; | ||
|
|
||
| use uv_client::{BaseClientBuilder, WrappedReqwestError}; | ||
| use uv_client::BaseClientBuilder; |
There was a problem hiding this comment.
This removal is related to axoupdater (and depedent axoasset) on an older version of reqwest.
Related:
| connectivity: Online, | ||
| offline: Disabled, | ||
| native_tls: false, | ||
| tls_backend: RustlsWebpki, |
There was a problem hiding this comment.
Might be overkill, but wanted to show new tls_backend setting working.
|
CI is currently failing because we introduced an openssl dependency. |
27d53ce to
1aae7bf
Compare
I've pushed this commit: salmonsd@93de71f PR isn't updating for some reason. |
…registry_client.rs for cargo build
This reverts commit 2b18f63.
93de71f to
9026d31
Compare
Summary
This PR improves the TLS experience by upgrading reqwest to
0.13.1via #17427It adds support for three TLS backends via a new hidden
--tls-backendflag:rustls-webpki— bundled Mozilla roots fromwebpki-root-certs(default)rustls— platform/system verifier viarustls-platform-verifiernative-tls— native system TLS stackCustom certificates from
SSL_CERT_FILE/SSL_CERT_DIRare merged unconditionally into the root store across all backends usingreqwest::tls_merge_certs(), ensuring consistent support in corporate or CI setups without backend-specific gating.The
--native-tlsflag andUV_NATIVE_TLSenv var are retained for compatibility, mapping to thenative-tlsbackend.Motivation
reqwest0.13.1 defaults torustlsas its TLS backend w/ platform verification and removes built-inwebpki-roots, and moves its default crypto provider to aws-lc instead of ring (increasing the number of cert signature algos supported) to improve TLS experience.Changes
Dependency updates
reqwest→0.13.1(pinned)reqsign→0.19.0webpki-root-certs0.13.1reqwest-middleware#110.13.1ambient-id#210.13.1async_http_range_reader#3TLS backend selection
--tls-backendflag:rustls-webpki|rustls|native-tls--native-tlspreserved (with explicit conflict handling)UV_NATIVE_TLSenv var maps tonative-tlsbackendrustls-webpkiCertificate handling
webpki-root-certsreqwest::tls_certs_onlyto initialize the root store with bundled certsSSL_CERT_FILE/SSL_CERT_DIRusingtls_merge_certsreqwest's certificate merging machinery → avoids custom root store or TLS config managementRefactoring & cleanup
uv-client/base_client.rsanduv-client/ssl_certs.rsaccept-encoding: identityinregistry_client.rswhere requiredDocumentation
certificates.md:rustls-webpkifor consistency,native-tlsfor proxies)SSL_CERT_*behavior and migration notesTesting
uv-client/tests/ssl_certs.rs(loading, precedence, all backends)nextest.tomlwith SSL test profile overrideTrade-offs & Future Work
webpki-root-certs+tls_certs_only+tls_merge_certskeeps maintenance low and avoids re-implementing root store logicreqwestinternals--native-tlsretained for smooth transition; long-term plan is deprecation--tls-backendto visible/stable--system-certs/--no-system-certsaliases (preview)rustls(platform verifier) in a future breaking release--native-tlsandUV_NATIVE_TLS