Add `webpki-roots` and `native-certs` crate features #1943

daxpedda · 2023-05-22T15:36:50Z

I will need some guidance on how to proceed from here. Currently when both crate features are enabled, certificates from both webpki-roots and rustls-native-certs are loaded, which is probably not what we want.

Suggestions:

Either we prefer one or the other.
Allow only one crate feature to be enabled with a compile-time check.
Expose a configuration option that selects between one of them, with probably native certs being the default.

I'm leaning towards option 3., because a follow-up from me would be to expose some API to disable/enable them during runtime anyway. This is also how reqwest does it, but when both features are active they will just load both.

I didn't touch the integration tests crate and CI yet.

Follow-up to #1940.

djc · 2023-05-23T07:56:32Z

I would usually pick option 1. Do we have an API path that allows doing without either (that is, with the caller providing ClientConfig/ServerConfig)?

I'm leaning towards option 3., because a follow-up from me would be to expose some API to disable/enable them during runtime anyway. This is also how reqwest does it, but when both features are active they will just load both.

What do you mean by disabling them during runtime anyway? What are you looking to achieve for your use case?

daxpedda · 2023-05-23T08:49:48Z

Do we have an API path that allows doing without either (that is, with the caller providing ClientConfig/ServerConfig)?

Apparently yes: ResolverConfig::set_tls_client_config().

What do you mean by disabling them during runtime anyway? What are you looking to achieve for your use case?

My main use-case is when shipping an application, I want to add a settings toggle that allows users to use their native certs if the want to, but using webpki-roots by default. This will become extra useful with rustls-platform-verifier.

To achieve this, some runtime control would be required. E.g. adding fields or methods to ResolverOpts or ResolverConfig.

Alternatively, this could be deemed too niche and using ResolverConfig::set_tls_client_config() enables this already.

I would usually pick option 1.

I just recently found out that reqwest for example just loads both: https://github.com/seanmonstar/reqwest/blob/06c8e5b0b008afee8114fb979b85cd8b73415391/src/async_impl/client.rs#L381-L425.

Let me know how you want me to proceed. With ResolverConfig::set_tls_client_config() any solution we discussed so far works for me.

djc · 2023-05-23T08:53:54Z

I guess the simplest path forward is to enable rustls-native-certs by default everywhere and provide a webpki-roots feature that overrides the default.

daxpedda · 2023-05-23T09:07:42Z

I guess the simplest path forward is to enable rustls-native-certs by default everywhere [..]

This isn't desirable either, because that would force a dependency on rustls-native-certs even when it's not needed. That was the reason I originally made #1941.

djc · 2023-05-23T09:16:39Z

I meant, in terms of option 1.

crates/resolver/src/tls/dns_over_rustls.rs

daxpedda · 2023-05-23T14:54:09Z

Not sure why CI fails here: https://github.com/bluejekyll/trust-dns/actions/runs/5057735617/jobs/9077514569?pr=1943#step:5:298

2023-05-23T13:41:37.0805887Z running 0 tests
2023-05-23T13:41:37.0807928Z      Running tests/named_https_tests.rs (/home/runner/work/trust-dns/trust-dns/target/debug/deps/named_https_tests-20c4e45b4310e1ef)
2023-05-23T13:41:37.0808258Z 
2023-05-23T13:41:37.0853353Z test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
2023-05-23T13:41:37.0855807Z 
2023-05-23T13:41:37.0857952Z 
2023-05-23T13:41:37.0860197Z running 1 test
2023-05-23T13:42:07.0972205Z error: test failed, to rerun pass `--test named_https_tests`
2023-05-23T13:42:07.0972619Z 
2023-05-23T13:42:07.0972756Z Caused by:
2023-05-23T13:42:07.0973489Z   process didn't exit successfully: `/home/runner/work/trust-dns/trust-dns/target/debug/deps/named_https_tests-20c4e45b4310e1ef` (exit status: 255)

Locally this test works for me:

cargo test --features dns-over-https-rustls --test named_https_tests
cargo test --no-default-features --features dns-over-https-rustls --test named_https_tests

djc · 2023-05-23T15:23:40Z

Yeah, that test is flaky.

daxpedda · 2023-05-28T20:52:49Z

CI fails on nightly because of: sval-rs/value-bag#62. To fix this we need to update log to v0.4.18.

Do you want me to make a quick PR?

djc · 2023-05-29T08:55:14Z

Yes, please!

crates/proto/src/quic/quic_client_stream.rs

daxpedda · 2023-06-14T11:15:19Z

crates/proto/src/quic/quic_client_stream.rs

+        for cert in rustls_native_certs::load_native_certs()? {
+            if let Err(err) = root_store.add(&rustls::Certificate(cert.0)) {
+                tracing::warn!(
+                    "failed to parse certificate from native root store: {:?}",
+                    &err
+                );
+            }
+        }
+        if root_store.is_empty() {
+            return Err(ProtoErrorKind::NativeCerts.into());
+        }


The thing I did here in general was to log warnings when encountering invalid certificates and returning an error if either load_native_certs() failed or if no certs were found at all.

In comparison to how reqwest does it, they also error out if load_native_certs() fails but they don't error out if no certificates at all where found, only if at least one invalid and no valid certificate was found.

See https://github.com/seanmonstar/reqwest/blob/06c8e5b0b008afee8114fb979b85cd8b73415391/src/async_impl/client.rs#L381-L425.

daxpedda · 2023-06-14T11:22:41Z

crates/resolver/src/tls/dns_over_rustls.rs

@@ -28,10 +29,31 @@ use crate::config::TlsClientConfig;
 const ALPN_H2: &[u8] = b"h2";

 // using the mozilla default root store
-pub(crate) static CLIENT_CONFIG: Lazy<Arc<ClientConfig>> = Lazy::new(|| {
+pub(crate) static CLIENT_CONFIG: Lazy<Result<Arc<ClientConfig>, ProtoError>> = Lazy::new(|| {


So this doesn't try and reload native certs if it fails.
We don't want to turn this into a function either because otherwise we would just be allocating the same stuff on every single connection.

Instead of making CLIENT_CONFIG a global we would have to store it in the connection provider, that would allow for proper reloading of native certificates per connection provider instead of per connection.

Hmm, storing it in the connection provider doesn't sound so bad. Can you explain for me from first principles why this is necessary?

It comes down to how we want to reload the native certs.

With a Lazy, we can only load the certificates once, if it fails, we have to restart the application to try again.

With a OnceCell, we could potentially keep trying to load until it succeeds. To reload the certificates again after a success, we would have to restart the application.

With a function, we would have to re-parse all certificates for every new connection, because that's where we need the configuration. That's not really a viable option.

If we store it in the connection provider, you can reload or retry getting the certificates by creating a new connection provider instead of having to restart the application.

So depending on how we want to be able to reload native certificates, we have to choose one of those solutions:

To retry after a failure you have to restart the application -> 1.

To reload after a success you have to restart the application -> 2.

To reload or retry, you have to create a new connection provider -> 4.

Yes, sounds reasonable to me.

I mean this turned out a bit ugly, let me know what you think, I can figure out how to improve this if you agree with the approach.

@djc friendly ping, this is ready again.

daxpedda · 2023-06-14T12:56:57Z

I think this CI failure has nothing to do with this PR?
It was spurious network error before, now it's bad TLS data.

daxpedda · 2023-06-14T15:49:56Z

I can actually reproduce this failure locally, seems to be an issue with the PR actually.

daxpedda · 2023-06-14T16:00:08Z

So there are some tests that required any sort of root certificates, which was there by default before.
That should be fixed now.

daxpedda · 2023-07-06T12:23:08Z

Rebased on top of #1986 to let the CI run.

daxpedda · 2023-07-07T15:59:29Z

Rebased on top of main again.

bluejekyll

Overall this looks ok, I have a question about the Lazy usage if you wouldn't mind answering that?

bluejekyll · 2023-07-30T20:49:51Z

crates/client/Cargo.toml

@@ -48,6 +48,9 @@ dns-over-openssl = ["dns-over-tls", "dnssec-openssl"]
 dns-over-rustls = ["dns-over-tls", "dnssec-ring", "trust-dns-proto/dns-over-rustls"]
 dns-over-tls = []

+webpki-roots = ["trust-dns-proto/webpki-roots"]
+native-certs = ["trust-dns-proto/native-certs"]


I'm going to file an issue for this. I think we want to move away from controlling the TLS and Certificate system in use from the features based model (we'll keep the features) and toward a config system that will allow for the desired model to be chosen at that time. The current system can be confusing especially if dependencies might introduce one or the other accidentally.

I would like to work on that as well!

crates/proto/src/quic/quic_client_stream.rs

bluejekyll · 2023-07-30T21:08:36Z

crates/resolver/src/name_server/connection_provider.rs

    }
 }

 impl<P: RuntimeProvider + Default> Default for GenericConnector<P> {
    fn default() -> Self {
        Self {
            runtime_provider: P::default(),
+            #[cfg(feature = "dns-over-rustls")]
+            client_config: Arc::new(Lazy::new(crate::tls::client_config)),


I might have missed it, but I'm having trouble following the advantage of this lazy in this context, i.e. it seems like this will be a new reference, so even the Arc is unnecessary? Am I misunderstanding this?

The Arc here is required to clone the Lazy, which is needed if we want to keep GenericConnector cloneable.
Lazy is used here to lazily initialize the ClientConfig, because this config isn't used if the user supplies it's own.

djc · 2023-08-22T07:31:52Z

In order to make this a bit easier to digest, can I request that this is split in multiple PRs? Maybe even one per commit for some of these wide-ranging commits.

daxpedda · 2023-08-22T07:35:11Z

In order to make this a bit easier to digest, can I request that this is split in multiple PRs? Maybe even one per commit for some of these wide-ranging commits.

Some of these commits just overwrite each other or fix something from a previous one.
I will redo the whole PR and for sure can split it up into multiple ones.

daxpedda · 2023-09-07T14:21:02Z

#2005 included everything in here.

daxpedda force-pushed the native-certs branch from 0dc1fa1 to daa1545 Compare May 22, 2023 15:43

daxpedda force-pushed the native-certs branch 2 times, most recently from 9bd46f9 to a395fed Compare May 23, 2023 10:41

daxpedda commented May 23, 2023

View reviewed changes

crates/resolver/src/tls/dns_over_rustls.rs Outdated Show resolved Hide resolved

daxpedda force-pushed the native-certs branch 2 times, most recently from 572d374 to 87d15bf Compare May 23, 2023 13:12

daxpedda mentioned this pull request May 24, 2023

Replace lazy_static with once_cell #1944

Merged

daxpedda force-pushed the native-certs branch from 87d15bf to d081b3d Compare May 28, 2023 20:06

daxpedda force-pushed the native-certs branch from d081b3d to 1fa20df Compare June 14, 2023 10:44

daxpedda commented Jun 14, 2023

View reviewed changes

crates/proto/src/quic/quic_client_stream.rs Outdated Show resolved Hide resolved

daxpedda force-pushed the native-certs branch from 1fa20df to c36e813 Compare June 14, 2023 11:02

daxpedda commented Jun 14, 2023

View reviewed changes

daxpedda force-pushed the native-certs branch from c36e813 to d27a8ed Compare June 14, 2023 12:19

daxpedda force-pushed the native-certs branch from d27a8ed to b63c240 Compare June 14, 2023 13:38

daxpedda requested a review from djc June 14, 2023 13:39

daxpedda force-pushed the native-certs branch 2 times, most recently from 183f115 to 4c22ae1 Compare June 14, 2023 14:57

daxpedda force-pushed the native-certs branch 3 times, most recently from ed21c7e to 11e8fae Compare July 6, 2023 12:22

daxpedda force-pushed the native-certs branch from 11e8fae to 394fe4e Compare July 7, 2023 14:11

daxpedda marked this pull request as ready for review July 7, 2023 14:57

daxpedda force-pushed the native-certs branch from 394fe4e to 37b2884 Compare July 7, 2023 15:59

bluejekyll reviewed Jul 30, 2023

View reviewed changes

daxpedda force-pushed the native-certs branch 2 times, most recently from 875c390 to bdae6ba Compare August 20, 2023 14:41

daxpedda requested a review from bluejekyll August 20, 2023 15:05

daxpedda mentioned this pull request Aug 21, 2023

Separate default rustls::ClientConfig for each protocol #2001

Closed

daxpedda added 6 commits August 21, 2023 18:15

Add webpki-roots and native-certs features

14f6833

Fallible rustls-native-certs initialization

59aeb66

Store ClientConfig per connection provider

e9ca987

Use RootCertStore::add_parsable_certificates()

8a75049

Fix example requiring root certificates

de614db

Make QuicClientStreamBuilder construction non-fallible

140966b

daxpedda force-pushed the native-certs branch from bdae6ba to 140966b Compare August 21, 2023 16:15

daxpedda mentioned this pull request Aug 22, 2023

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

Merged

daxpedda closed this Sep 7, 2023

daxpedda mentioned this pull request Sep 22, 2023

ClientConfig and RootCertStore improvements #2038

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `webpki-roots` and `native-certs` crate features #1943

Add `webpki-roots` and `native-certs` crate features #1943

daxpedda commented May 22, 2023 •

edited

Loading

djc commented May 23, 2023

daxpedda commented May 23, 2023

djc commented May 23, 2023

daxpedda commented May 23, 2023

djc commented May 23, 2023

daxpedda commented May 23, 2023

djc commented May 23, 2023

daxpedda commented May 28, 2023

djc commented May 29, 2023

daxpedda Jun 14, 2023

daxpedda Jun 14, 2023 •

edited

Loading

djc Jun 14, 2023

daxpedda Jun 14, 2023 •

edited

Loading

djc Jun 14, 2023

daxpedda Jun 14, 2023

daxpedda Jul 6, 2023

daxpedda commented Jun 14, 2023

daxpedda commented Jun 14, 2023 •

edited

Loading

daxpedda commented Jun 14, 2023

daxpedda commented Jul 6, 2023

daxpedda commented Jul 7, 2023

bluejekyll left a comment

bluejekyll Jul 30, 2023

daxpedda Aug 11, 2023

bluejekyll Jul 30, 2023

daxpedda Aug 11, 2023

djc commented Aug 22, 2023

daxpedda commented Aug 22, 2023

daxpedda commented Sep 7, 2023

Add webpki-roots and native-certs crate features #1943

Add webpki-roots and native-certs crate features #1943

Conversation

daxpedda commented May 22, 2023 • edited Loading

djc commented May 23, 2023

daxpedda commented May 23, 2023

djc commented May 23, 2023

daxpedda commented May 23, 2023

djc commented May 23, 2023

daxpedda commented May 23, 2023

djc commented May 23, 2023

daxpedda commented May 28, 2023

djc commented May 29, 2023

Choose a reason for hiding this comment

daxpedda Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxpedda Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxpedda commented Jun 14, 2023

daxpedda commented Jun 14, 2023 • edited Loading

daxpedda commented Jun 14, 2023

daxpedda commented Jul 6, 2023

daxpedda commented Jul 7, 2023

bluejekyll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djc commented Aug 22, 2023

daxpedda commented Aug 22, 2023

daxpedda commented Sep 7, 2023

Add `webpki-roots` and `native-certs` crate features #1943

Add `webpki-roots` and `native-certs` crate features #1943

daxpedda commented May 22, 2023 •

edited

Loading

daxpedda Jun 14, 2023 •

edited

Loading

daxpedda Jun 14, 2023 •

edited

Loading

daxpedda commented Jun 14, 2023 •

edited

Loading