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

update rustls v0.20 -> v0.21 #9

Merged
merged 1 commit into from
May 17, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Mar 27, 2023

Description

This branch updates rustls-platform-verifier to use the recently released rustls 0.21.0, both as a direct dependency and through upgrades to relevant transitive deps reqwest, hyper-rustls, and tokio-rustls).

Details

Most notable for this codebase, the rustls::Error::InvalidCertificateData has been removed and replaced
by InvalidCertificate and a number of CertificateError sub-variants.

Now that the upstream InvalidCertificate error offers a way to specify what went wrong with more granularity we're able to trim all of the error_messages consts, choosing instead to return the more specific upstream error types. For the case where invalid extensions are detected we define our own error type to use consistently across verifiers.

In all other circumstances where we want to return an InvalidCertificate error with some platform specific error message we
use the InvalidCertificate(CertificateError::Other) variant with the error message, re-purposing the invalid_certificate helper for constructing an Arc over a Box with the error message.

We also have to take some special care when asserting the equality of errors, handling the case where looking at a CertificateError::Other specially, since it can box a dyn error that can't be compared directly without downcasting the error to a concrete type.

Remaining work

@cpu cpu requested a review from complexspaces March 27, 2023 16:13
@cpu cpu force-pushed the cpu-rustls-0.21.0-prep branch 2 times, most recently from 36e489c to 6d613c9 Compare March 27, 2023 19:38
@cpu
Copy link
Member Author

cpu commented Mar 27, 2023

@complexspaces I think this is ready for an initial review now. I'm happy to make adjustments if any parts are still off the mark. My familiarity with this crate is still very low :-)

@cpu cpu force-pushed the cpu-rustls-0.21.0-prep branch from 6d613c9 to 0cdc9d2 Compare March 28, 2023 13:41
@cpu
Copy link
Member Author

cpu commented Mar 28, 2023

cpu force-pushed the cpu-rustls-0.21.0-prep branch from 6d613c9 to 0cdc9d2

Since Rustls restored PartialEq on the error type I was able to drop the debug assertion match helper. We're back to using assert_eq! with a small helper for coarsely matching the CertificateError::Other case. PR description updated accordingly.

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.

Overall this looks pretty good, my only real concern is how the EKU errors aren't being exactly tested for anymore.

src/tests/verification_mock/mod.rs Outdated Show resolved Hide resolved
src/verification/mod.rs Show resolved Hide resolved
src/verification/windows.rs Show resolved Hide resolved
src/tests/mod.rs Outdated Show resolved Hide resolved
@cpu cpu force-pushed the cpu-rustls-0.21.0-prep branch from 0cdc9d2 to a029eed Compare March 29, 2023 21:05
@cpu
Copy link
Member Author

cpu commented Mar 29, 2023

@complexspaces I think this is ready for another review pass when you have a chance. There's no big rush because we still have a few other dependencies that need to be updated so we can remove the Cargo patches here.

@cpu cpu requested a review from complexspaces March 29, 2023 21:11
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.

I have one last thing, but otherwise this looks good. Now its just a matter of the rest of the ecosystem catching up :)

src/tests/verification_mock/mod.rs Outdated Show resolved Hide resolved
@cpu cpu force-pushed the cpu-rustls-0.21.0-prep branch from a029eed to cf59e4b Compare March 30, 2023 18:30
src/tests/mod.rs Show resolved Hide resolved
@cpu cpu force-pushed the cpu-rustls-0.21.0-prep branch from cf59e4b to dda43b4 Compare March 30, 2023 20:46
@cpu
Copy link
Member Author

cpu commented Mar 30, 2023

cpu force-pushed the cpu-rustls-0.21.0-prep branch from cf59e4b to dda43b4

Rebased on main and fixed the Cargo.toml conflict.

Now its just a matter of the rest of the ecosystem catching up :)

Slowly but surely :-) I updated the README with a rough checklist of things we're waiting on.

@complexspaces
Copy link
Collaborator

Thanks for the list, that's pretty helpful 👍

@cpu cpu force-pushed the cpu-rustls-0.21.0-prep branch 3 times, most recently from 2389b76 to 00a7677 Compare April 3, 2023 13:14
@cpu
Copy link
Member Author

cpu commented May 11, 2023

Haven't forgotten about this PR :-) Things are moving again and I'm hopeful we'll be able to tie a bow on this in the next week or two.

@complexspaces
Copy link
Collaborator

No worries at all, I've been following along with the other releases that are leading up to this.

@cpu cpu force-pushed the cpu-rustls-0.21.0-prep branch from 00a7677 to 11e2987 Compare May 16, 2023 21:46
@cpu cpu changed the title WIP: update rustls v0.20 -> v0.21.0 update rustls v0.20 -> v0.21.0 May 16, 2023
@cpu cpu changed the title update rustls v0.20 -> v0.21.0 update rustls v0.20 -> v0.21 May 16, 2023
@cpu cpu marked this pull request as ready for review May 16, 2023 21:51
@cpu
Copy link
Member Author

cpu commented May 16, 2023

@complexspaces All set! 🚀

Cargo.toml Outdated Show resolved Hide resolved
@cpu cpu force-pushed the cpu-rustls-0.21.0-prep branch from 11e2987 to 7917d03 Compare May 17, 2023 12:18
Update rustls dependency from 0.20.0 to 0.21.0.

Also patches transitive deps on rustls-native-certs, webpki-roots,
reqwest, hyper-rustls and tokio-rustls to use releases that also depend
on rustls 0.21.0.

Most notable for this codebase, the
`rustls::Error::InvalidCertificateData` has been removed and replaced
by `InvalidCertificate` and a number of `CertificateError` sub-variants.

Now that the upstream `InvalidCertificate` error offers a way to specify
what went wrong with more granularity we're able to trim most of the
`error_messages` consts, choosing instead to return the more specific
upstream error types.

For the case where invalid extensions are detected we define our own
type to use for this error case.

In all other circumstances where we want to return an
`InvalidCertificate` error with some platform specific error message we
use the `InvalidCertificate(CertificateError::Other)` variant with the
error message, repurposing the `invalid_certificate` helper for
constructing an `Arc` over a `Box` with the error message.

We also have to take some special care when asserting the equality of
errors, handling the case where looking at a `CertificateError::Other`
specially, since it can box a dyn error that can't be compared directly
without downcasting the error to a concrete type.
@cpu cpu force-pushed the cpu-rustls-0.21.0-prep branch from 7917d03 to 4c8048f Compare May 17, 2023 15:27
@complexspaces complexspaces merged commit c18e462 into rustls:main May 17, 2023
@complexspaces
Copy link
Collaborator

Thanks again for handling this!

@cpu cpu deleted the cpu-rustls-0.21.0-prep branch May 17, 2023 20:34
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.

2 participants