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

Add feature to support rustls-native-certs root certificates #134

Merged
merged 3 commits into from
Nov 4, 2022

Conversation

charlespierce
Copy link
Contributor

Closes #133

Info

  • As mentioned in the issue, adding a feature flag to enable rustls-native-certs as root certificates allows consumers to easily opt-in to the native certificate stores.
  • Inspired by how the similar feature works in Reqwest, the feature flag for this is all-inclusive, automatically activating tls-rustls as well.

Changes

  • Added a feature flag tls-rustls-native-roots to enable loading the native certificates as root certificates for requests.
  • If the feature flag is enabled, load all of the native certs as root certificates.
    • We also ignore errors, as the native certs are sometimes invalid and we don't want to interrupt the entire request process if the user has an invalid cert somewhere in their native store.

Notes

  • I left the webpki certificate loading in regardless of the feature flag, as that's the current behavior and it seemed most reasonable to have the feature flag be additive, rather than a toggle.
  • I'm not sure if we want to have a "valid" counter the way that Reqwest does, so that we can error out if the native cert check failed to load any valid certs? If that's a useful feature I'm happy to add, but I left it out in the interest of simplicity.
  • I also updated a "." to '.' to resolve a clippy warning about using a single-character string as a pattern.

@@ -9,6 +9,8 @@ use rustls::{
client::{ServerCertVerified, ServerCertVerifier, WebPkiVerifier},
ClientConfig, ClientConnection, OwnedTrustAnchor, RootCertStore, ServerName, StreamOwned,
};
#[cfg(feature = "tls-rustls-native-roots")]
use rustls_native_certs::load_native_certs;
use webpki_roots::TLS_SERVER_ROOTS;
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't webpki_roots be disabled when native certs are used?

Copy link
Owner

Choose a reason for hiding this comment

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

I think that it's likely that users that enable the native certs feature do not want the webpki certs. They want to rely on the OS' cert store and not on Mozilla's.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question that I wasn't totally sure of. I noted in the description that I left it as-is initially so that the feature flag would be additive as opposed to a toggle. Reqwest has two separate feature flags for webpki and native, so you could enable both or only one. I'm not super knowledgeable about the expected behaviors, so I don't have a good sense of if having only the native certs would cause issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that loaded weird, didn't see your second comment right away! But yeah, that makes sense. I'll update to make the native certs exclusive of the webpki ones.

Copy link
Owner

Choose a reason for hiding this comment

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

Doesn't have to be you who does this, but I think the TLS features should be cleaned up. Maybe something like:

  • tls-native
  • tls-native-vendored
  • tls-rustls-webpki-roots
  • tls-rustls-native-roots

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that would make sense. Would probably need to keep the various aliases around, at least for backwards compatibility, but it would definitely make it clearer which features are intended for which use-cases.

I took a quick look, my guess is it should be a separate PR, because there's a lot of various places that reference the current feature names that would need to be changed. I was also running into some weird behavior from Cargo that I didn't fully understand, where it wasn't importing the optional dependency even though it was declared to include it 🙁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think I got it working in #135 (which is stacked on this one to also include the tls-rustls-native-roots feature flag).

src/tls/rustls_impl.rs Outdated Show resolved Hide resolved
@sbstp sbstp merged commit f29e0ce into sbstp:master Nov 4, 2022
@charlespierce charlespierce deleted the rustls_native_certs branch November 6, 2022 14:52
@charlespierce
Copy link
Contributor Author

@sbstp With this and #135 merged, would you be able to create / publish a new version on crates.io?

Not a super high priority, as we can use GH path imports if needed, but would be helpful when you have the time. Thanks in advance!

@sbstp
Copy link
Owner

sbstp commented Nov 6, 2022

Yeah I'm planning to today

@charlespierce
Copy link
Contributor Author

Perfect, thanks!

@sbstp
Copy link
Owner

sbstp commented Nov 6, 2022

Published 0.24.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for rustls-native-certs
2 participants