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

Enable TLS native root toggling at runtime #2362

Merged
merged 3 commits into from Mar 12, 2024
Merged

Enable TLS native root toggling at runtime #2362

merged 3 commits into from Mar 12, 2024

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Mar 11, 2024

Summary

It turns out that on macOS, reading the native certificates can add hundreds of milliseconds to client initialization. This PR makes --native-tls a command-line flag, to toggle (at runtime) the choice of the webpki roots or the native system roots.

You can't accomplish this kind of configuration with the reqwest builder API, so instead, I pulled out the heart of that logic from the crate (https://github.com/seanmonstar/reqwest/blob/e3192638518d577759dd89da489175b8f992b12f/src/async_impl/client.rs#L498), and modified it to allow toggling a choice of root.

Note that there's an open PR for this in reqwest (seanmonstar/reqwest#1848), along with an issue (seanmonstar/reqwest#1843), which I may ping, but it's been around for a while and I believe reqwest is focused on its next major release.

Closes #2346.

crates/uv/src/main.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@zanieb
Copy link
Member

zanieb commented Mar 11, 2024

Technically this is a breaking change, should we create a label for that and highlight accordingly?

@charliermarsh charliermarsh added the breaking A breaking change label Mar 11, 2024
@charliermarsh charliermarsh added the performance Potential performance improvement label Mar 11, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) March 11, 2024 23:56
@charliermarsh charliermarsh merged commit e9c16e9 into main Mar 12, 2024
7 checks passed
@charliermarsh charliermarsh deleted the charlie/tls branch March 12, 2024 04:05
zanieb added a commit that referenced this pull request Mar 12, 2024
... yet.

I think we're not quite ready for a versioning policy over here. Now
that we have a "labeled" breaking change in #2362 we need to decide if
it should be a minor or patch version.
@samypr100
Copy link
Contributor

samypr100 commented Mar 13, 2024

@charliermarsh Is it worth checking if SSL_CERT_FILE is set and make it behave as-if --native-tls was used?
I think that would keep this change backwards compatible with users leveraging SSL_CERT_FILE

e.g. something like

-let tls = tls::load(if self.native_tls {
+let tls = tls::load(if self.native_tls || env::var("SSL_CERT_FILE").is_ok() {
    Roots::Native
} else {
    Roots::Webpki
})

@charliermarsh
Copy link
Member Author

We can support SSL_CERT_FILE even without --native-tls, I think. I'm happy to add that. It would effectively be this PR: #2351.

@samypr100
Copy link
Contributor

Per this updated statement in the docs

If a direct path to the certificate is required (e.g., in CI), set the SSL_CERT_FILE environment
variable to the path of the certificate bundle (alongside the --native-tls flag), to instruct uv
to use that file instead of the system's trust store.

It sounds like reqwest will honor still SSL_CERT_FILE (if it's set), even if the loading is happening via on uv's end via tls::load/use_preconfigured_tls.
Assuming this, the suggested code snippet above would work to keep this change backwards compatible.

charliermarsh pushed a commit that referenced this pull request Mar 13, 2024
…ring `--native-tls` (#2401)

## Summary

Small follow up to #2362 to check if
`SSL_CERT_FILE` is set to enable `--native-tls` functionality. This
maintains backwards compatibility with `0.1.17` and below users
leveraging only `SSL_CERT_FILE`.

Closes #2400

## Test Plan

<!-- How was it tested? -->
Assuming `SSL_CERT_FILE` is already working via `--native-tls`, this is
simply a shortcut to enable `--native-tls` functionality implicitly
while still being able to let `rustls-native-certs` handle the loading
of `SSL_CERT_FILE` instead of ourselves.

Edit: Manually tested by setting up own self-signed CA certificate
bundle and set `SSL_CERT_FILE` to this and confirmed the loading happens
without having to specify `--native-tls`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking change performance Potential performance improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate overhead in initializing HTTP client
3 participants