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

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

Merged

Conversation

samypr100
Copy link
Collaborator

@samypr100 samypr100 commented Mar 13, 2024

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

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.

@samypr100 samypr100 changed the title feat: keep backwards compatibility with SSL_CERT_FILE without requiring --native-tls feat: keep backwards compatibility with SSL_CERT_FILE without requiring --native-tls Mar 13, 2024
@samypr100 samypr100 marked this pull request as ready for review March 13, 2024 02:08
@@ -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
Member

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
Collaborator Author

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
Collaborator Author

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
Member

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
Collaborator Author

Choose a reason for hiding this comment

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

Added in 85d92e8

@zanieb
Copy link
Member

zanieb commented Mar 13, 2024

Thanks! I was going to flag this too when looking at the changelog — I think this is a very reasonable behavior.

@zanieb zanieb added the enhancement New feature or improvement to existing functionality label Mar 13, 2024
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Thanks!

@charliermarsh charliermarsh force-pushed the backwards-compat-ssl-cert-file branch from 633e932 to dd681b4 Compare March 13, 2024 04:23
@charliermarsh charliermarsh enabled auto-merge (squash) March 13, 2024 04:23
@charliermarsh charliermarsh merged commit e0ac5b4 into astral-sh:main Mar 13, 2024
26 checks passed
@samypr100 samypr100 deleted the backwards-compat-ssl-cert-file branch March 13, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Respect SSL_CERT_FILE regardless of --native-tls flag
3 participants