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

DoQ default configuration is invalid #1990

Closed
daxpedda opened this issue Jul 21, 2023 · 7 comments · Fixed by #2036 · May be fixed by #2031
Closed

DoQ default configuration is invalid #1990

daxpedda opened this issue Jul 21, 2023 · 7 comments · Fixed by #2036 · May be fixed by #2031

Comments

@daxpedda
Copy link
Contributor

daxpedda commented Jul 21, 2023

Currently DoT, DoH and DoQ share the same ClientConfig:
https://github.com/bluejekyll/trust-dns/blob/bf04bc8d3efec41d29cf558416b3dbed2d26ae02/crates/resolver/src/tls/dns_over_rustls.rs#L30-L56

All three implementations can't share the same configuration as they differ on some properties:

  • ALPN (mandatory)
    • DoT: None
    • DoH: "h2" (current)
    • DoQ: "doq"
  • SNI (optional)
    We probably want to continue disable SNI for DoT by default (though the only public server that I found supporting DoQ, AdGuard, requires SNI for both DoQ and DoT), but disabling SNI for DoH by default is strange.

The fast fix is probably easy, just use a different ClientConfig for each protocol. This does have a big downside though, when using multiple backends this will parse the root certificates multiple times.

This in turn could be solved by using WebPkiVerifier, but that would require dangerous_configuration and I'm not sure if we want to use that because it would activate it for upstream users as well.

So unless we are fine with activating dangerous_configuration, this would probably require some fix in Rustls. Considering @djc maintains both, I'm gonna hold back from creating an issue there until further feedback.

@daxpedda daxpedda mentioned this issue Jul 21, 2023
@djc
Copy link
Collaborator

djc commented Jul 21, 2023

I think the rustls QUIC code does the right thing for protocol versions, and will restrict to 1.3 when starting a QUIC connection. Do you have actual evidence to the contrary? Otherwise, I don't think that's an issue.

As long as we don't run the rustls-native-certs loading process multiple times, I think other parts of the config creation aren't likely to have much of a performance impact presuming that we create one TLS config per resolver config (or equivalent).

@daxpedda
Copy link
Contributor Author

daxpedda commented Jul 21, 2023

I think the rustls QUIC code does the right thing here, and will restrict to 1.3 when starting a QUIC connection. Do you have actual evidence to the contrary?

No! I only tested ALPN and SNI.
I didn't know that, but it makes sense.

EDIT: Removed it from OP.

@djc
Copy link
Collaborator

djc commented Aug 23, 2023

This in turn could be solved by using WebPkiVerifier, but that would require dangerous_configuration and I'm not sure if we want to use that because it would activate it for upstream users as well.

I think rustls would definitely accept a change to take an Arc<RootCertStore> instead of RootCertStore directly. See also rustls/rustls#1403. Perhaps there's a way of doing this that is semver-compatible? I was thinking with_root_certificates() could take some kind of impl AsRef<RootCertStore> + 'static but wrangling that in there semver-compatibly might not be feasible.

@djc
Copy link
Collaborator

djc commented Aug 23, 2023

See also rustls/rustls#1413 (which will go into rustls 0.22).

@bluejekyll
Copy link
Member

bluejekyll commented Sep 6, 2023

@daxpedda, with #2005 merged, what are the potential other changes needed here? Or is that enough?

@daxpedda
Copy link
Contributor Author

daxpedda commented Sep 6, 2023

#2005 did not fix this issue.

The current plan is to store the root certificates in an Arc<RootCertStore> and then store the default RustConfig in the runtime. There we need to split it up into a separate RustConfig for each backend: DoT, DoH, DoQ.

I'm planning to put up a PR in the next couple of days!

@bluejekyll
Copy link
Member

Awesome! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment