starred/hickory-dns

Fork 0

mirror of https://github.com/hickory-dns/hickory-dns.git synced 2026-04-25 11:15:54 +03:00

[GH-ISSUE #1990] DoQ default configuration is invalid #841

New issue

Closed

opened 2026-03-16 00:30:44 +03:00 by kerem · 7 comments

kerem commented

2026-03-16 00:30:44 +03:00

Owner

Originally created by @daxpedda on GitHub (Jul 21, 2023).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/1990

Currently DoT, DoH and DoQ share the same ClientConfig:
github.com/bluejekyll/trust-dns@bf04bc8d3e/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.

Originally created by @daxpedda on GitHub (Jul 21, 2023). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/1990 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`](https://docs.rs/rustls/0.21.5/rustls/client/struct.WebPkiVerifier.html), 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.

kerem

2026-03-16 00:30:44 +03:00

closed this issue
added the
crate:proto

feature:dns-over-quic
labels

kerem commented

2026-03-16 00:31:00 +03:00

Author

Owner

@djc commented on GitHub (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).

@djc commented on GitHub (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).

kerem commented

2026-03-16 00:31:05 +03:00

Author

Owner

@daxpedda commented on GitHub (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.

@daxpedda commented on GitHub (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.

kerem commented

2026-03-16 00:31:10 +03:00

Author

Owner

@djc commented on GitHub (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 https://github.com/rustls/rustls/issues/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 commented on GitHub (Aug 23, 2023): > This in turn could be solved by using [`WebPkiVerifier`](https://docs.rs/rustls/0.21.5/rustls/client/struct.WebPkiVerifier.html), 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 https://github.com/rustls/rustls/issues/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.

kerem commented

2026-03-16 00:31:15 +03:00

Author

Owner

@djc commented on GitHub (Aug 23, 2023):

See also https://github.com/rustls/rustls/pull/1413 (which will go into rustls 0.22).

@djc commented on GitHub (Aug 23, 2023): See also https://github.com/rustls/rustls/pull/1413 (which will go into rustls 0.22).

kerem commented

2026-03-16 00:31:20 +03:00

Author

Owner

@bluejekyll commented on GitHub (Sep 6, 2023):

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

@bluejekyll commented on GitHub (Sep 6, 2023): @daxpedda, with #2005 merged, what are the potential other changes needed here? Or is that enough?

kerem commented

2026-03-16 00:31:25 +03:00

Author

Owner

@daxpedda commented on GitHub (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!

@daxpedda commented on GitHub (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!

kerem commented

2026-03-16 00:31:30 +03:00

Author

Owner

@bluejekyll commented on GitHub (Sep 7, 2023):

Awesome! Thank you!

@bluejekyll commented on GitHub (Sep 7, 2023): Awesome! Thank you!

kerem referenced this issue

2026-03-16 02:22:13 +03:00

[PR #841] [CLOSED] Bump url from 1.7.2 to 2.0.0 #1727