Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cargo: update to rustls 0.22, associated updates #42

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

cpu
Copy link
Member

@cpu cpu commented Dec 5, 2023

Description

This branch updates to Rustls 0.22 and brings in the associated dependency updates this requires. For the time being, this branch continues to unconditionally use ring as the crypto provider. Follow-up work to expose this as a choice (e.g allowing aws-lc-rs as a provider) may be interesting.

Deps:

  • updated rustls 0.21 -> 0.22.1
  • added pki-types 1.0

Linux deps:

  • rustls-native-certs 0.6 -> 0.7
  • webpki 0.101 -> 0.102

Android deps:

  • webpki 0.101 -> 0.102

WASM32 deps:

  • webpki-roots 0.25 -> 0.26

Summary of breaking change updates:

  • We use rustls 0.22.1 in specific to benefit from the pki_types re-export, removing the need to add that as our own dep with matching version.
  • ServerName, Certificate, and OwnedTrustAnchor types are now sourced from pki_types, with an associated generic lifetime. The OwnedTrustAnchor type is now just TrustAnchor.
  • The 'dangerous' rustls crate feature was removed, and associated items moved into new locations with the import path emphasizing danger.
  • "Other error" types changed to use a specific rustls::OtherError inner variant.
  • SystemTime for verifiers replaced with pki_types::UnixTime.
  • Default fns on ServerCertVerifier trait were removed, must be reconstituted with rustls::verify_tls12_signature, rustls::verify_tls13_signature and WebPkiSupportedAlgorithms.supported_schemes using a CryptoProvider.
  • ServerName now supports a to_str operation, avoiding the need to match and handle unsupported name types.
  • WebPkiVerifier was renamed to WebPkiServerVerifier, handled as an Arc and constructed with a builder.

Reviewers will likely be most interested in some of the changes associated with the new pki_types::UnixTime type. Since we no longer receive a SystemTime from Rustls some of the code adjusting the verification timestamp into platform specific representations needed to change. I think I've handled this correctly but would appreciate more detailed review on this aspect of the branch.

@cpu cpu self-assigned this Dec 5, 2023
@cpu cpu marked this pull request as ready for review December 21, 2023 16:10
@cpu
Copy link
Member Author

cpu commented Dec 21, 2023

cpu marked this pull request as ready for review now

The main blocker that had this in draft status was the Reqwest compatibility unit test, but I think we should land #43 to address that. I've marked this branch ready for review and updated the PR description to remove the TODO notes.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Cargo.toml Outdated Show resolved Hide resolved
src/verification/android.rs Outdated Show resolved Hide resolved
@cpu cpu marked this pull request as draft January 2, 2024 22:21
@cpu
Copy link
Member Author

cpu commented Jan 2, 2024

cpu marked this pull request as draft now

Flipping this to draft - needs a rebase :-)

@cpu
Copy link
Member Author

cpu commented Jan 3, 2024

Flipping this to draft - needs a rebase :-)

Rebased and ready to go.

@cpu cpu marked this pull request as ready for review January 3, 2024 15:51
@cpu
Copy link
Member Author

cpu commented Jan 4, 2024

cpu force-pushed the cpu-rustls-0.22 branch from 239f0b3 to a3d1842

Rebased again to pick up the changes from #50

@cpu
Copy link
Member Author

cpu commented Jan 5, 2024

cpu force-pushed the cpu-rustls-0.22 branch from a3d1842 to 1ab5d96

rebased again for #55

@cpu
Copy link
Member Author

cpu commented Jan 5, 2024

CI / Test (FreeBSD) (pull_request) Failing after 2m

Ah, another Rustls 0.21 vs 0.22 breakage snuck in. I'll fix shortly.

@cpu
Copy link
Member Author

cpu commented Jan 5, 2024

I'll fix shortly.

Done.

Besides fixing the webpki-roots version I had to tweak the verification_without_mock_root test to use the same strategy we used in #55: initializing the Verifier under test with Verifier::new_with_extra_roots for FreeBSD.

As we know already, in FreeBSD CI the openssl-probe wasn't populating the Verifier with roots on its own. On Unix-likes w/ Rustls 0.21 the test's Verifier::new() would construct a webpki verifier under the hood that had no roots configured, and we get the expected CertificateError::UnknownIssuer error the test wants. With Rustls 0.22 this test fails because the webpki verifier construction was changed to error when initialized with an empty root store, resulting in a different err than the test expects. Switching the Verifier constructor out for Verifier::new_with_extra_roots results in the webpki verifier constructor succeeding, and then the test fails in the expected way because wepki-roots doesn't contain the mock root trust anchor.

Copy link
Collaborator

@complexspaces complexspaces left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for the most part, I just have some nits/questions regarding the new types and crypto backends.

Copy link
Collaborator

@complexspaces complexspaces left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great now, thanks!

There's just the one small thread left about maybe abstracting some OnceCell boilerplate away, but its not a blocker.

For the time being, this branch continues to unconditionally use *ring*
as the crypto provider. Follow-up work to expose this as a choice (e.g
allowing aws-lc-rs as a provider) may be interesting.

Deps:
* updated rustls 0.21 -> 0.22.1

Linux deps:
* rustls-native-certs 0.6 -> 0.7
* webpki 0.101 -> 0.102

Android deps:
* webpki 0.101 -> 0.102

WASM32 deps:
* webpki-roots 0.25 -> 0.26

Summary of breaking change updates:
* We use rustls 0.22.1 in specific to benefit from the `pki_types`
  re-export, removing the need to add that as our own dep with matching
  version.
* `ServerName`, `Certificate`, and `OwnedTrustAnchor` types are now
  sourced from `pki_types`, with an associated generic lifetime. The
  `OwnedTrustAnchor` type is now just `TrustAnchor`.
* The 'dangerous' rustls crate feature was removed, and associated items
  moved into new locations with the import path emphasizing danger.
* "Other error" types changed to use a specific `rustls::OtherError`
  inner variant.
* `SystemTime` for verifiers replaced with `pki_types::UnixTime`.
* Default fns on `ServerCertVerifier` trait were removed, must be
  reconstituted with `rustls::verify_tls12_signature`,
  `rustls::verify_tls13_signature` and
  `WebPkiSupportedAlgorithms.supported_schemes` using
  a `CryptoProvider`.
* `ServerName` now supports a `to_str` operation, avoiding the need to
  `match` and handle unsupported name types.
* `WebPkiVerifier` was renamed to `WebPkiServerVerifier`, handled as an
  `Arc` and constructed with a builder.
@complexspaces
Copy link
Collaborator

The latest cleanup changes still look good to me, so LGTM.

@cpu cpu merged commit c18a7e2 into rustls:main Jan 12, 2024
14 checks passed
@cpu cpu deleted the cpu-rustls-0.22 branch January 12, 2024 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants