Update rustls to 0.23 #70

Ralith · 2024-03-08T23:03:12Z

Required for Quinn to update past rustls 0.21, so a release soon after would be appreciated. Also relaxes default feature requirements on rustls.

djc · 2024-03-08T23:26:22Z

I think I tried this and it won't work on macOS...

Ralith · 2024-03-08T23:53:10Z

This is difficult to test without CI working by default.

complexspaces · 2024-03-09T00:52:45Z

@Ralith I added you with the Triage role on the repository (it grants minimal issue and PR permissions). I can remove it once you've merged this and GitHub doesn't see you as an "untrusted contributor" here anymore but hopefully it doesn't bug you for now.

Ralith · 2024-03-11T05:02:36Z

Android CI failing:

> Task :rustls-platform-verifier:connectedDebugAndroidTest

[DDMLIB]: An unexpected packet was received before the handshake.
[DDMLIB]: An unexpected packet was received before the handshake.
[DDMLIB]: An unexpected packet was received before the handshake.
[DDMLIB]: An unexpected packet was received before the handshake.

> Task :rustls-platform-verifier:connectedDebugAndroidTest
Starting 3 tests on test(AVD) - 9

org.rustls.platformverifier.CertificateVerifierTests > runRealWorldTestSuite[test(AVD) - 9] FAILED 
	org.junit.ComparisonFailure: A test failed. Check the logs above for Rust panics. expected:<[success]> but was:<[test failed!]>
	at org.junit.Assert.assertEquals(Assert.java:115)
Tests on test(AVD) - 9 failed: There was 1 failure(s).

Not sure how to interpret that. There aren't any panics that I can see. "An unexpected packet was received" sounds like a rustls-side issue. There seems to be some sort of test result artifact that might have details, but it doesn't look human-readable.

zh-jq-b · 2024-03-11T11:03:09Z

It seems to be the same issue as reported here #68.

cpu · 2024-03-11T13:18:06Z

"An unexpected packet was received" sounds like a rustls-side issue.

I think these lines are unrelated noise. The same output has been happening on passing builds in the past. "ddmlib" looks like it's specific to the Android emulator<->Host debug bridge and so I think likely to be inconsequential to the Rustls tests.

It seems to be the same issue as reported here #68.

Yes, it is.

rustls-platform-verifier/src/verification/apple.rs

Ralith · 2024-03-11T20:41:10Z

If the only CI failure is #68, can we consider this unblocked?

cpu · 2024-03-11T20:55:45Z

If the only CI failure is #68, can we consider this unblocked?

I don't know how @complexspaces feels but I think we wouldn't want to cut a release until main is passing CI so even if we merged this it would be blocked on #68.

I've made some progress and think I have a fix. Will update #68 shortly

cpu · 2024-03-11T21:32:00Z

I've made some progress and think I have a fix. Will update #68 shortly

I think cherry-picking in 9872533 from #71 will sort the mac task out.

Ralith · 2024-03-12T00:50:36Z

macos is hitting:

rustls default CryptoProvider not set

which is from the expect I added on CryptoProvider::get_default() calls. This stopped happening on other platforms when the rustls ring feature was enabled. I'd expect to have to call install_default either everywhere or nowhere; having that requirement vary across platforms is baffling.

complexspaces · 2024-03-12T00:57:16Z

That is extraordinarily strange. I tried comparing the output of cargo tree --edges features between macOS and Linux and nothing jumps out as "broken" in rustls direct dependency tree. The only difference is that there's no rustls-webpki.

Ralith · 2024-03-12T01:02:53Z

Perhaps it's a test case ordering/global state issue. rustls tests suggest that None is the expected return value even when the feature is enabled:

https://github.com/rustls/rustls/blob/0398ac50fe18521da038aa39b55eec6c83450523/rustls/tests/process_provider.rs#L47-L58

I'll try setting defaults explicitly.

complexspaces · 2024-03-12T02:30:29Z

It looks like its back to the Android failures 🎉 If you rebase on main Ci should be green at least. I merged #71 a few minutes ago.

djc · 2024-03-12T09:23:02Z

rustls-platform-verifier/src/verification/windows.rs

@@ -438,7 +441,9 @@ impl Verifier {
    pub(crate) fn new_with_fake_root(root: &[u8]) -> Self {
        Self {
            test_only_root_ca_override: Some(root.into()),
-            default_provider: rustls::crypto::ring::default_provider(),
+            default_provider: rustls::crypto::CryptoProvider::get_default()


All these constructors should probably document that they're relying on the process-global crypto provider set via rustls?

I think proceeding with this design would be user-hostile, as a typical use is likely to panic at runtime for users who haven't explicitly installed a default, which isn't normally required by rustls. I think a better solution would be for rustls to expose CryptoProvider::get_default_or_install_from_crate_features, which would let us Just Work as well as rustls itself does (although I have mixed feelings about how that breaks if multiple providers are enabled).

We could also reimplement the implicit default logic with local features in this crate, but that would put users at higher risk of accidentally enabling multiple providers (and potentially breaking any part of their code that relies on implicit defaults) if they don't take care to match features on rustls and this crate.

I think this crate should probably just take an Arc as an argument.

That works, though it undermines the convenience of rustls's implicit defaults if an idiomatic user has to go and deal with explicit provider handling for the verifier anyway.

I think it might be cleaner if this operates at the level of a "parent" Arc<dyn ServerCertVerifier> rather than a CryptoProvider. But unfortunately right now WebPkiServerVerifier::builder() does require a non-empty set of roots, which makes it annoying to reuse for this purpose.

My other thought on this is: perhaps we should aim for windows.rs to use CryptVerifyMessageSignature (or similar) and for apple.rs to use SecKeyVerifySignature? That would minimise the chance that a certificate is accepted by the platform verifier, but the handshake can't complete because it the certificate is outside the subset that the rustls-webpki crate can parse.

My other thought on this is: perhaps we should aim for windows.rs to use CryptVerifyMessageSignature (or similar) and for apple.rs to use SecKeyVerifySignature?

I have thought about this before, but I'm still not sure I like the idea. It adds another "split" in the library's functionality set, especially on Windows where registry controls can influence what cryptographic operations the OS prefers/will use. This would make debugging harder, for example. So far I have only seen one cryptographic issue come in via bug report at work. Everything else has been missing trust roots, malformed X.509, etc.

I think this crate should probably just take an Arc as an argument.

On review, this feels really awkward on *nix which doesn't actually need it.

It adds another "split" in the library's functionality set

Isn't exposing platform-specific behavior the whole point of this lib?

I think it might be cleaner if this operates at the level of a "parent" Arc rather than a CryptoProvider.

This makes a lot of sense to me.

But unfortunately right now WebPkiServerVerifier::builder() does require a non-empty set of roots, which makes it annoying to reuse for this purpose.

Could we relax this, or introduce an alternative verifier in rustls that has an empty root set but implements the other methods? Practically speaking this amounts to creatively delegating to rustls's get_default_or_install_from_crate_features, which I think is the best user experience.

rustls-platform-verifier/src/tests/mod.rs

cpu

Reviewing the diff I don't have any feedback above what djc has already flagged. Thanks again for picking this up Ralith 🎉

Ralith · 2024-03-15T21:03:28Z

I've added a commit that makes calls to CryptoProvider::get_default() lazy, which should ensure a good user experience without hard-coding a specific provider or breaking the API, because rustls will not attempt to verify a certificate until after it's been fully configured. The rare case of users who were previously passing in a provider explicitly and never touching the default implicitly might still see a panic, but can easily resolve it.

Long-term, I think it would be better for rustls to expose CryptoProvider::get_default_or_install_from_crate_features directly, since its use in e.g. WebPkiServerVerifier seems to be a tacit admission that it's the most user-friendly way for library code to get a provider. However, this change will let rustls-platform-verifier update and unblock downstream users like Quinn immediately without regressing usability.

Allows rustls's implicit defaults to be applied in e.g. `ClientConfig::builder`, even if the verifier is constructed beforehand. This allows the library to Just Work as in earlier versions without breaking the API or baking in a particular provider.

rustls-platform-verifier/Cargo.toml

complexspaces

Sorry for the delay, LGTM.

Ralith force-pushed the update-rustls branch from e35e948 to 327876a Compare March 8, 2024 23:39

Ralith force-pushed the update-rustls branch from 327876a to 63a684c Compare March 11, 2024 20:37

Ralith commented Mar 11, 2024

View reviewed changes

rustls-platform-verifier/src/verification/apple.rs Outdated Show resolved Hide resolved

Ralith force-pushed the update-rustls branch 2 times, most recently from b2cb2ec to 8c11739 Compare March 11, 2024 20:45

Ralith force-pushed the update-rustls branch 2 times, most recently from ca3f7b2 to 9f58145 Compare March 11, 2024 21:31

Ralith force-pushed the update-rustls branch 4 times, most recently from 088f716 to 273b8a3 Compare March 12, 2024 01:22

Update rustls to 0.23

fe72708

Ralith force-pushed the update-rustls branch from 273b8a3 to 5e953b8 Compare March 12, 2024 05:12

djc reviewed Mar 12, 2024

View reviewed changes

cpu reviewed Mar 12, 2024

View reviewed changes

Relax hard *ring* dep

fb959a0

Ralith force-pushed the update-rustls branch from 5e953b8 to fb959a0 Compare March 13, 2024 18:25

Ralith force-pushed the update-rustls branch from 6aa2a7b to 0afc433 Compare March 15, 2024 21:04

cpu approved these changes Mar 18, 2024

View reviewed changes

rustls-platform-verifier/Cargo.toml Show resolved Hide resolved

flisky mentioned this pull request Mar 22, 2024

chore: update rusttls to 0.23.2 #76

Closed

This comment was marked as spam.

Sign in to view

djc mentioned this pull request Mar 22, 2024

Clippy 1.77 + dependency updates hickory-dns/hickory-dns#2174

Merged

cpu mentioned this pull request Mar 22, 2024

Update rustls to 0.23 rustls/hyper-rustls#263

Closed

complexspaces approved these changes Mar 25, 2024

View reviewed changes

complexspaces merged commit 0bd5440 into rustls:main Mar 25, 2024
15 checks passed

cpu mentioned this pull request Mar 25, 2024

Cargo: version 0.2.0 -> 0.3.0 #77

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update rustls to 0.23 #70

Update rustls to 0.23 #70

Ralith commented Mar 8, 2024 •

edited

Loading

djc commented Mar 8, 2024

Ralith commented Mar 8, 2024

complexspaces commented Mar 9, 2024

Ralith commented Mar 11, 2024

zh-jq-b commented Mar 11, 2024

cpu commented Mar 11, 2024 •

edited

Loading

Ralith commented Mar 11, 2024

cpu commented Mar 11, 2024

cpu commented Mar 11, 2024

Ralith commented Mar 12, 2024

complexspaces commented Mar 12, 2024

Ralith commented Mar 12, 2024

complexspaces commented Mar 12, 2024

djc Mar 12, 2024

Ralith Mar 13, 2024

djc Mar 13, 2024

Ralith Mar 13, 2024

ctz Mar 14, 2024

complexspaces Mar 14, 2024

Ralith Mar 15, 2024 •

edited

Loading

cpu left a comment

Ralith commented Mar 15, 2024 •

edited

Loading

This comment was marked as spam.

complexspaces left a comment

Update rustls to 0.23 #70

Update rustls to 0.23 #70

Conversation

Ralith commented Mar 8, 2024 • edited Loading

djc commented Mar 8, 2024

Ralith commented Mar 8, 2024

complexspaces commented Mar 9, 2024

Ralith commented Mar 11, 2024

zh-jq-b commented Mar 11, 2024

cpu commented Mar 11, 2024 • edited Loading

Ralith commented Mar 11, 2024

cpu commented Mar 11, 2024

cpu commented Mar 11, 2024

Ralith commented Mar 12, 2024

complexspaces commented Mar 12, 2024

Ralith commented Mar 12, 2024

complexspaces commented Mar 12, 2024

djc Mar 12, 2024

Choose a reason for hiding this comment

Ralith Mar 13, 2024

Choose a reason for hiding this comment

djc Mar 13, 2024

Choose a reason for hiding this comment

Ralith Mar 13, 2024

Choose a reason for hiding this comment

ctz Mar 14, 2024

Choose a reason for hiding this comment

complexspaces Mar 14, 2024

Choose a reason for hiding this comment

Ralith Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

cpu left a comment

Choose a reason for hiding this comment

Ralith commented Mar 15, 2024 • edited Loading

This comment was marked as spam.

complexspaces left a comment

Choose a reason for hiding this comment

Ralith commented Mar 8, 2024 •

edited

Loading

cpu commented Mar 11, 2024 •

edited

Loading

Ralith Mar 15, 2024 •

edited

Loading

Ralith commented Mar 15, 2024 •

edited

Loading