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

ClientConfig and RootCertStore improvements #2038

Open
daxpedda opened this issue Sep 22, 2023 · 4 comments
Open

ClientConfig and RootCertStore improvements #2038

daxpedda opened this issue Sep 22, 2023 · 4 comments

Comments

@daxpedda
Copy link
Contributor

daxpedda commented Sep 22, 2023

Goals

  1. Ability to retry reloading native certificates after failure or when required.
  2. Different default SNI settings between protocols.
  3. Let users supply quinn::Endpoint (Pass quinn::Endpoint for DoQ and DoH3 #2002).
  4. Decide at runtime which certificate store to use instead of selecting it with crate features.

Problems

Storage

ResolverOpts

Storing rustls::ClientConfig in ResolverOpts is currently not possible for several reasons:

Copy and Default can be removed, but De/Serialize would require one of the following solutions:

  1. Remove De/Serialize from `ResolverOpts. Though it's required in other crates: Separate default rustls::ClientConfig for each protocol #2001 (comment).
  2. Custom Deserializer for RustlsConfig passing the native certificates error through ... somehow.
  3. Use Option<ClientConfig>s and Option<Arc<RootCertStore>> to create ClientConfigs when needed and store RootCertStore for re-usage. Will require a custom PartialEq implementation with Arc::ptr_eq() for RootCertStore.

GenericConnector

I have originally done this in #2001. Not sure what exactly the downside would be here, so this is my preferred solution right now.

If we decide on this course we could also potentially revert #2029.

A new type

This could be stored in a field inside AsyncResolver, which would be quite similar to GenericConnector.

Runtime root certificate store selection

I believe this would require to store ClientConfig somewhere else then ResolverOpts. If we add fields to ResolverOpts that specify which store to use, already initialized ClientConfigs inside ResolverOpts would require to be re-initialized when changing those fields.

We could mitigate this issue by making sure that ResolverOpts can never be accessed through the API again after ClientConfigs are never initialized, which I believe is currently the case anyway. In this case I'm not sure why we would decide to store them in ResolverOpts anyway.

Related issues and PRs

@djc
Copy link
Collaborator

djc commented Sep 23, 2023

Note that rustls-platform-verifier devolves to rustls-native-certs (optionally with webpki-roots as fallback) on Linux, so it probably doesn't solve many problems if you consider Linux an important deployment target.

I think cloning ClientConfig once per protocol is probably okay, but I'm wondering how that makes your "retry reloading certificates" scenario look?

@daxpedda
Copy link
Contributor Author

Note that rustls-platform-verifier devolves to rustls-native-certs (optionally with webpki-roots as fallback) on Linux, so it probably doesn't solve many problems if you consider Linux an important deployment target.

Ah, will remove it from OP then!

I think cloning ClientConfig once per protocol is probably okay, but I'm wondering how that makes your "retry reloading certificates" scenario look?

In that regard it would solve nothing, unfortunately, I would say both these problems are unrelated.
But if you are saying it's not a problem, then I can as well remove it from the OP.

@djc
Copy link
Collaborator

djc commented Sep 25, 2023

So it seems your currently favored solution is to store client configs in the GenericConnector? How does that work out? How does it enable you to replace/update them at a later point in time? The solution in #2001 still seems to rely on a bunch of Lazy values which I don't think enables updates?

@daxpedda
Copy link
Contributor Author

daxpedda commented Sep 25, 2023

How does it enable you to replace/update them at a later point in time?

In the case of GenericConnector it means to reload the native certs, users have to create a new Resolver. Depending on how exactly we implement it, it should also be easy to detect load failure when initializing the Resolver.

We have discussed this previously here: #1943 (comment).

The solution in #2001 still seems to rely on a bunch of Lazy values which I don't think enables updates?

These Lazy values were just option.unwrap_or_else(), I was doing that to lazily intialize ClientConfigs, and therefor load certificates, on demand and not on initialization of the Resolver. But now that we have Arc<RootCertStore>, we might want to load those on initialization of Resolver to get a nice error message if loading native certificates fails.

But initializing a new Resolver would still reload native certificates in #2001.

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

No branches or pull requests

2 participants