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 #2038] `ClientConfig` and `RootCertStore` improvements #858

New issue

Closed

opened 2026-03-16 00:37:00 +03:00 by kerem · 5 comments

kerem commented

2026-03-16 00:37:00 +03:00

Owner

Originally created by @daxpedda on GitHub (Sep 22, 2023).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/2038

Goals

Ability to retry reloading native certificates after failure or when required.
Different default SNI settings between protocols.
Let users supply quinn::Endpoint (#2002).
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:

ResolverOpts requires Copy (#2029).
ResolverOpts requires Default (#2035).
ResolverOpts requires De/Serialize.

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

Remove De/Serialize from `ResolverOpts. Though it's required in other crates: https://github.com/bluejekyll/trust-dns/pull/2001#issuecomment-1725234084.
Custom Deserializer for RustlsConfig passing the native certificates error through ... somehow.
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.

Originally created by @daxpedda on GitHub (Sep 22, 2023). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/2038 ## 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` (#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: - `ResolverOpts` requires `Copy` (#2029). - `ResolverOpts` requires `Default` (#2035). - `ResolverOpts` requires `De/Serialize`. `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: https://github.com/bluejekyll/trust-dns/pull/2001#issuecomment-1725234084. 2. Custom `Deserialize`r for `RustlsConfig` passing the native certificates error through ... somehow. 3. Use `Option<ClientConfig>`s and `Option<Arc<RootCertStore>>` to create `ClientConfig`s 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 `ClientConfig`s 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 `ClientConfig`s 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 - #1939 - #1943 - #1944 - #1987 - #1990 - #2001 - #2003 - #2002 - #2005 - #2029 - #2031 - #2035 - #2036

kerem closed this issue

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

kerem commented

2026-03-16 00:37:11 +03:00

Author

Owner

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

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

kerem commented

2026-03-16 00:37:16 +03:00

Author

Owner

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

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.

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

kerem commented

2026-03-16 00:37:22 +03:00

Author

Owner

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

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

kerem commented

2026-03-16 00:37:27 +03:00

Author

Owner

@daxpedda commented on GitHub (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: https://github.com/bluejekyll/trust-dns/pull/1943#discussion_r1229437556.

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.

@daxpedda commented on GitHub (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: https://github.com/bluejekyll/trust-dns/pull/1943#discussion_r1229437556. > 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 `ClientConfig`s, 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.

kerem commented

2026-03-16 00:37:32 +03:00

Author

Owner

@djc commented on GitHub (Jan 24, 2025):

I believe #2735 addresses most of these issues (building on top of #2569), happy to get your feedback.

@djc commented on GitHub (Jan 24, 2025): I believe #2735 addresses most of these issues (building on top of #2569), happy to get your feedback.

kerem referenced this issue

2026-03-16 02:23:02 +03:00

[PR #858] [MERGED] fix all dyn clippy warnings #1744