Add webpki-roots and native-certs crate features, take 2

[daxpedda]

This splits the changes from #1943 into two parts. This is the first part, only introducing loading the native certificates. The changes are basically only touching the code that was loading certificates from webpki-roots and introducing the new crate features.

I could split this up further into trust-dns-proto and trust-dns-resolver?

This implementation has the big downside that trust-dns-resolver will attempt to load the native certificates only once and save the Result in a Lazy. So there currently no way for the user to re-attempt or reload the native certificates. This will be addressed in the second part.

Replaces #1943.

[djc]

Sorry, being nitpicky about the commit history, because I know there's a bunch more large changes here and I'd like to make reviewing them easy.

I'm inclined to say the Lazy<Arc<ClientConfig>> design doesn't make all that much sense and we should instead use a design where a "global" default Arc<ClientConfig> (or maybe even only the RootCertStore?) lives in something like the ResolverConfig, NameServerConfigGroup or ResolverOpts but maybe this should be left for a future PR? (Maybe this is already what you were planning on doing in a future PR.)

[daxpedda]

Sorry, being nitpicky about the commit history, because I know there's a bunch more large changes here and I'd like to make reviewing them easy.

This is totally fine! I'm happy to do the legwork here and make it as easy as possible to review!

I'm inclined to say the Lazy<Arc<ClientConfig>> design doesn't make all that much sense and we should instead use a design where a "global" default Arc<ClientConfig> (or maybe even only the RootCertStore?) lives in something like the ResolverConfig, NameServerConfigGroup or ResolverOpts but maybe this should be left for a future PR? (Maybe this is already what you were planning on doing in a future PR.)

I think the big upside of Lazy<Arc<ClientConfig>> is for the bundled certificates to not consume memory more then once, but that doesn't play well with the native certs as we've seen in #1943.

Unfortunately storing a RootCertStore doesn't really work because we would have to clone it for every ClientConfig created from it. We could store a sharable WebPkiVerifier but that's not possible without enabling rustls/dangerous_configuration, I've described the issue in more detail here: #1990 (comment). I've worked around that in #2001 by creating a base ClientConfig that can be cloned, which just clones a bunch of Arcs, to reuse the root certificates without consuming additional memory.

As for where to store it I would argue that a "global" default would make sense to live in ResolverOpts.

Personally I'm also in favor of moving away from the Lazy and my preference would be to store it instead in ResolverOpts.

Let me know what you think!

[daxpedda]

Let me know if you want me to squash anything or split a commit into a separate PR. E.g. I think 431db81 should be squashed.

[djc]

I've worked around that in #2001 by creating a base ClientConfig that can be cloned, which just clones a bunch of Arcs, to reuse the root certificates without consuming additional memory.

As for where to store it I would argue that a "global" default would make sense to live in ResolverOpts.

Personally I'm also in favor of moving away from the Lazy and my preference would be to store it instead in ResolverOpts.

This sounds like a good direction!

[daxpedda]

CI currently fails on a test called test_sec_lookup_fails, doesn't work for me locally on main either so I believe this is unrelated to this PR. Can be reproduced with:
cargo test -p trust-dns-resolver --all-features -- test_sec_lookup_fails

[daxpedda]

CI currently fails on a test called test_sec_lookup_fails, doesn't work for me locally on main either so I believe this is unrelated to this PR. Can be reproduced with: cargo test -p trust-dns-resolver --all-features -- test_sec_lookup_fails

I guess https://trust-dns.org is down?

[daxpedda]

CI currently fails on a test called test_sec_lookup_fails, doesn't work for me locally on main either so I believe this is unrelated to this PR. Can be reproduced with: cargo test -p trust-dns-resolver --all-features -- test_sec_lookup_fails

I guess https://trust-dns.org is down?

I temporarily disabled the test to make the CI run. Let me know if you want me to remove it before merging.

[djc]

I'm going to leave it to @bluejekyll to decide if he wants to merge this with the ignored test (or fix up his DNS/web server setup?).

[daxpedda]

If we decide that we don't want to merge this now because it's a breaking change, let me know. There are other PRs I want to work on that (hopefully) don't require a breaking change, but I am waiting on this because I don't want to spend time constantly rebasing everything.

No stress though, I'm not in a hurry!

[djc]

I think we pretty much default to only doing breaking releases except if there's a security response needed.

[daxpedda]

I removed the commit disabling the test.

The last commit "Fix CI" was probably necessary because of the 1.72 release and has nothing to do with this PR. I'm happy to split it into a separate PR if needed.

[djc]

Cool, look forward to merging this.

The last commit "Fix CI" was probably necessary because of the 1.72 release and has nothing to do with this PR. I'm happy to split it into a separate PR if needed.

Separate PR is unnecessary from my side, but would like one commit for cargo fmt and one commit for clippy fixes.

[bluejekyll]

Sorry for the delay on this. I'm reviewing this now.

[bluejekyll]

Thank you for all the cleanup on the examples and configuration as well. This looks great. You clearly spent a lot of time and energy on this change, I really appreciate all that effort.

See the one comment about the panic, once that is fixed, I will happily merge this in.

[bluejekyll]

ok, this is an awesome change. Thank you for all the work!

[daxpedda]

Separate PR is unnecessary from my side, but would like one commit for cargo fmt and one commit for clippy fixes.

Apologies, never got to separate those commits!

Thank you all for the guidance, was a great experience!