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

Separate default rustls::ClientConfig for each protocol #2001

Closed
wants to merge 7 commits into from

Conversation

daxpedda
Copy link
Contributor

This PR separates the default configuration used by each protocol to allow correct default ALPN and SNI configuration.

The certificates provided by webpki-roots are parsed only once, the ClientConfig is cloned and then modified for each protocol. The clone doesn't actually clone the certificates, as they are stored in an Arc<WebPkiVerifier>, which avoids re-parsing the certificates over and over again but more importantly doesn't multiply the memory usage for each cloned ClientConfig.

See #1990 for more details on the issue addressed here.

Based on #1943.
Fixes #1990.

@bluejekyll
Copy link
Member

@daxpedda, does this change supercede #1943? i.e. that one is not relevant after this?

@daxpedda
Copy link
Contributor Author

@daxpedda, does this change supercede #1943? i.e. that one is not relevant after this?

It's based on #1943, so it includes it, yes.
Considering we now have #2005, this PR doesn't make a whole lot of sense right now until we figure that out.

@daxpedda
Copy link
Contributor Author

daxpedda commented Sep 19, 2023

@djc I was looking into continuing this, but immediately hit a barrier.

The last decision was to move the configs into ResolverOpts: #2005 (comment). Turns out that's not really possible without removing Copy and De/Serialize from ResolverOpts.

I'm not exactly seeing a clear way forward here:

  1. Remove Copy from ResolverOpts and look into implementing De/Serialize for rustls::ClientConfig.
  2. Remove Copy and De/Serialize from ResolverOpts.
  3. Forget the idea and go back to storing them globally in Lazy.

Personally I would like to avoid storing them globally, but otherwise I don't have a preference. Obviously 2. is the easiest for me to move forward on this.

@djc
Copy link
Collaborator

djc commented Sep 19, 2023

Is there any code actually depending on serde capabilities and/or Copy? Could you do a separate PR to see what it looks like to remove these? Maybe look at git blame to see when/why they were added?

@daxpedda
Copy link
Contributor Author

daxpedda commented Sep 19, 2023

This is the original issue adding Serde support to ResolverOpts: #490.
Removing Copy is a bit annoying because now we have to clone it everywhere, but it's not a big deal considering it's all Arcs and what-not.

trust-dns-server relies on it though, it uses Serde to be able to store and load the config. If you go all the way down ForwardConfig is the one using ResolverOpts, which bubbles up until it reaches trust_dns_server::Config.

Here is me playing around with the idea: 2720598...daxpedda:trust-dns:resolver-opts-copy-serde-test.

@djc
Copy link
Collaborator

djc commented Sep 19, 2023

@bluejekyll what do you think? I'm inclined to think that the complete reflection of ResolverOpts via the ForwarderConfig is probably not very load-bearing, and it seems to make sense to store TLS configs in the ResolverOpts instead of as process-wide statics.

@daxpedda
Copy link
Contributor Author

daxpedda commented Sep 19, 2023

I also made a branch only removing Copy. We could then use serde(skip) and document that ResolverOpts just doesn't de/serialize TLS configurations and inserts the default instead.

2720598...daxpedda:trust-dns:resolver-opts-remove-copy

@djc
Copy link
Collaborator

djc commented Sep 19, 2023

I also made a branch only removing Copy. We could then use serde(skip) and document that ResolverOpts just doesn't de/serialize TLS configurations and inserts the default instead.

2720598...daxpedda:trust-dns:resolver-opts-remove-copy

That sounds like a good path forward, let's do that in a separate PR as a first step?

@daxpedda
Copy link
Contributor Author

Closing in favor of #2031.

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.

DoQ default configuration is invalid
3 participants