-
Notifications
You must be signed in to change notification settings - Fork 590
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
Arc-wrap ClientConfig RootCertStore
and remove expensiveness warnings
#1413
Conversation
rustls/src/client/builder.rs
Outdated
@@ -16,7 +16,7 @@ impl<C: CryptoProvider> ConfigBuilder<ClientConfig<C>, WantsVerifier<C>> { | |||
/// Choose how to verify server certificates. | |||
pub fn with_root_certificates( | |||
self, | |||
root_store: webpki::RootCertStore, | |||
root_store: Arc<webpki::RootCertStore>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could take impl Into<Arc<webpki::RootCertStore>>
to make this a little more ergonomic at the cost of a downstream monomorphization (which, in this case, is probably not a large cost?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AIUI because there is a blanket implementation for that on all types this would make this much less invasive to users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I guess that would also make it semver-compatible?
Codecov Report
@@ Coverage Diff @@
## main #1413 +/- ##
=======================================
Coverage 96.33% 96.33%
=======================================
Files 66 66
Lines 14860 14862 +2
=======================================
+ Hits 14316 14318 +2
Misses 544 544
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
On that note, I'm aware that this is future talk here, but what is the plan for |
Conceptually I think there should be but it's not entirely obvious to me how that should work.
Like how? |
Probably expose |
It could make sense to drop The open question is in that today, any foreign verifier wouldn't be able to impl |
The nice thing about the builder API is that we can have multiple setters that affect the same underlying setting but use different types. For instance we could have: fn with_platform_verifier(v: rustls_platform_verifier::ClientConfig) This could be exposed in non- Similarly, for this change we could expose For the expensiveness warning: Rather than delete it we should replace it with text that says the opposite ("making one of these is cheap"). People have read the existing warning and if it goes away they are likely to assume they simply can't find it or misremembered where it is, rather than that they no longer need to consider these objects expensive. |
But currently |
Sure -- so far, though, we haven't "blessed" any verifier other than the webpki one by directly exposing it as part of the rustls API. This would change that, which feels like a larger conceptual change? Edit:
Ah yes, that's one reason this isn't quite trivial. I think the fix here would be that rustls-platform-verifier just offers an API (like rustls-webpki currently does) that mimics but doesn't implement the
I do like the idea of providing @ctz thoughts?
Should we explicitly acknowledge that we used to say it was expensive? |
Yes, part of a larger conceptual change, but I think it's the right direction. Maybe it should wait until rustls-platform-verifier has gotten more public usage, but I think long-term it should be blessed like webpki. Actually, long-term I would really love to be able to provide: ClientConfig::default() Which would choose default cipher suites and so on, plus rustls-platform-verifier as the verifier. I think its properties make it the right choice as a default verifier. It does what people expect, and is a good verifier on all platforms. And because it doesn't require loading all roots (on certain platforms), it's faster to start up. Still, that's somewhat aside from this specific PR, just something that's been on my mind to bring up.
Yes, I think that would be good. |
I'd agree with that approach (and seems like it would be marginally easier with the base types crate?). I've also found platform-verifier difficult to reuse/compose because of its dependency on rustls.
I don't think we're achieving anything by gating |
But how do you feel about a change from |
I think that would be OK, it's certainly not breaking the bank on top of the quite significant amount of ceremony needed to get to that point (making a Also, I have added a second constructor to |
I've changed the
I've left out further changes to start taking an explicit verifier for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than comment tweaks, this implementation plan and the backport look excellent to me.
rustls/src/server/server_conn.rs
Outdated
/// While creation of a `ServerConfig` was previously documented to be expensive, this has | ||
/// recently been imprecise. The largest potential cost in building a [`ServerConfig`] is in | ||
/// gathering trust roots from the operating system to add to the [`RootCertStore`] passed | ||
/// to to a `ClientCertVerifier` builder (the rustls-native-certs is often used for this). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// to to a `ClientCertVerifier` builder (the rustls-native-certs crate is often used for this).
not sure that is true, because rustls-native-certs filters root certs (on windows and macos at least) only for trusting servers. of course, most of those are also trusted for publicly-issued client certs...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually not so clear from the rustls-native-certs documentation. Maybe we should clear that up?
The actually expensive part is mostly the gathering of certificates from the platform trust root store, and it would be better to document that in the relevant API (that is, in rustls-native-certs). Apart from that, I believe that the use of `Arc`-wrapped types is also an effective signal that the wrapped types should be reused where possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Thanks for picking this up @djc.
From #1403:
I ended up just removing the remarks for now, after changing
with_root_certificates()
for theClientConfig
builder to take anArc<RootCertStore>
(theServerConfig
builder started taking anArc
-wrappedRootCertStore
in #1368). IMO it would be best to document the performance implications where they occur, which in this case meansrustls_native_certs::load_native_certs()
. Its documentation currently reads:I also believe the use of
Arc
wrappers itself will help signal that (re)creating something of that type is performance-sensitive, so I feel there's not much value in further discussion directly on the config types (which, at worst, cost a few allocations for clones, many of which are actually refcount bumps from otherArc
s).Fixes #1403. Motivated by discussion in hickory-dns/hickory-dns#1990 (trust-dns-resolver currently stores a
Lazy<Arc<ClientConfig>>
wrapping webpki-roots -- which doesn't seem great).