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 (take 2) #2031

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Sep 21, 2023

WIP!

  1. I have to store the configs as in Options to allow Default to work. Probably the simplest solution is to remove the Default implementation and add a ResolverOpts::new().
  2. This is just temporary but currently we don't reuse certificates, for that I would have to either add another field or apply the solution in 1.
  3. Current DoQ takes a ClientConfig instead of a Arc<ClientConfig>, as far as I can see the only reason for that is that it adds the correct ALPN, which is obviously unnecessary when using a default supplied one. I would propose to actually remove this ALPN correction completely.

Originally in #2001 I have added those configs to GenericConnector instead of ResolverOpts, maybe this is also an alternative to consider. I was trying to keep in mind that somewhere we will want to dynamically be able to select between native or WebPKI roots, which will be weird if done in ResolverOpts, because it's holding these configs but it doesn't have a builder or something to select it before constructing those configs.

I hope I'm making sense here, I'm happy to talk this out in realtime on Discord or wherever if it's easier.

This is take two of #2001.
Fixes #1990.

@djc
Copy link
Collaborator

djc commented Sep 21, 2023

  1. I have to store the configs as in Options to allow Default to work. Probably the simplest solution is to remove the Default implementation and add a ResolverOpts::new().

Yeah, let's get rid of the Default impl and do this right? Maybe make this another separate PR?

@daxpedda
Copy link
Contributor Author

Maybe make this another separate PR?

I would like to do it in this PR, I don't really consider this PR mergeable in this state.

@djc
Copy link
Collaborator

djc commented Sep 22, 2023

Yeah, I meant that that PR could be merged ahead of this one.

@daxpedda
Copy link
Contributor Author

Ah I see, sure.

@daxpedda
Copy link
Contributor Author

  1. I have to store the configs as in Options to allow Default to work. Probably the simplest solution is to remove the Default implementation and add a ResolverOpts::new().

Yeah, let's get rid of the Default impl and do this right? Maybe make this another separate PR?

So this doesn't work ... because ResolverOpts requires Deserialize and the initial solution was to use serde(skip), which would require Default. So removing Option doesn't work here.

To summarize solutions I can come up with how to proceed now:

  1. Remove Serialize from ResolverOpts. See Separate default rustls::ClientConfig for each protocol #2001 (comment) for a previous discussion why this can be problematic.
  2. Just continue with Option? I'm not sure what the downside is, but it's weird to me that TLS config initialization errors are not returned at initialization of the resolver.
  3. Move TLS configs to some other state then ResolverOpts, previously I've done it in GenericConnector.

@djc
Copy link
Collaborator

djc commented Sep 23, 2023

Can we use #[serde(default = "path")]?

@daxpedda
Copy link
Contributor Author

daxpedda commented Sep 23, 2023

Can we use #[serde(default = "path")]?

We can of course, but that would require us to route the native certificate error through Serde.

I made #2038 where I tried to explain all the issues in detail, but at this point I'm pretty convinced that storing ClientConfigs in ResolverOpts is making this unnecessarily complicated.

@daxpedda daxpedda marked this pull request as ready for review September 23, 2023 17:16
@daxpedda daxpedda marked this pull request as draft September 23, 2023 17:16
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
2 participants