Skip to content

feat: keep backwards compatibility with SSL_CERT_FILE without requiring --native-tls #2401

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.

Sign up for GitHub

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

Merged
charliermarsh merged 4 commits into from
Mar 13, 2024
Merged

feat: keep backwards compatibility with SSL_CERT_FILE without requiring --native-tls #2401

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f1d5f93
feat: keep backwards compatibility with `SSL_CERT_FILE` without requi…
samypr100 Mar 13, 2024
1566fcf
docs: update docs
samypr100 Mar 13, 2024
85d92e8
fixup: nicer error msg on invalid SSL_CERT_FILE values
samypr100 Mar 13, 2024
dd681b4
Nits
charliermarsh Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,8 +435,8 @@ system's certificate store. To instruct uv to use the system's trust store, run
`--native-tls` command-line flag.

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.
variable to the path of the certificate bundle, to instruct uv to use that file instead of the
system's trust store.

## Acknowledgements

Expand Down
2 changes: 1 addition & 1 deletion crates/uv-client/src/registry_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ impl RegistryClientBuilder {
// Initialize the base client.
let client = self.client.unwrap_or_else(|| {
// Load the TLS configuration.
let tls = tls::load(if self.native_tls {
let tls = tls::load(if self.native_tls || env::var("SSL_CERT_FILE").is_ok() {
Copy link
Copy Markdown
Member

@charliermarsh charliermarsh Mar 13, 2024

Choose a reason for hiding this comment

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

Should we instead change tls::load to add a certificate based on SSL_CERT_FILE? I think we would just call root_cert_store.add with the result of this: https://github.com/astral-sh/uv/pull/2351/files#diff-dcc1f69d132524500a0dcf217aa6db521517536daaf9364f3085961626fe722cR120

Copy link
Copy Markdown
Collaborator Author

@samypr100 samypr100 Mar 13, 2024

Choose a reason for hiding this comment

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

I was thinking more towards delegating the parsing of SSL_CERT_FILE to rustls-native-certs as we already do when we combine (--native-tls with SSL_CERT_FILE).

The SSL_CERT_FILE is actually a CA bundle, meaning it can contain more than one cert. I believe we'd have to load each entry, get a vector of Certificates, and them add them to the builder one by one and handle potential incompatible cert errors along the way (we get this for free with rustls-native-certs).
I did see an API reqwest to load a pem bundle, but didn't see one to add them all at once, so we'd need to call add_root_certificate for each one in a loop as we build client.

Sidenote, I've just tested this current PR with creating my own self-signed CA's and set SSL_CERT_FILE to this and can confirm this solution would effectively delegate the parsing of SSL_CERT_FILE to rustls-native-certs properly (as we already do with the combination of --native-tls with SSL_CERT_FILE)

Copy link
Copy Markdown
Collaborator Author

@samypr100 samypr100 Mar 13, 2024

Choose a reason for hiding this comment

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

If we're concerned with a user providing an invalid SSL_CERT_FILE path, how about a nice warning?

            let ssl_cert_file_exists = env::var_os("SSL_CERT_FILE")
                .map_or(false, |path| {
                    let path_exists = Path::new(&path).exists();
                    if !path_exists {
                        warn_user_once!("Ignoring invalid value from environment for SSL_CERT_FILE. File path {} does not exists.", path.to_string_lossy());
                    }
                    path_exists
                });

            let tls = tls::load(if self.native_tls || ssl_cert_file_exists {
                Roots::Native
            } else {
                Roots::Webpki
            })

Copy link
Copy Markdown
Member

@zanieb zanieb Mar 13, 2024

Choose a reason for hiding this comment

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

I think that seems reasonable.

Copy link
Copy Markdown
Collaborator Author

@samypr100 samypr100 Mar 13, 2024

Choose a reason for hiding this comment

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

Added in 85d92e8

Roots::Native
} else {
Roots::Webpki
Expand Down