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.

Sign up for GitHub

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

Jump to bottom

Improving webpki verifier CRL support ergonomics #1552

Merged
merged 2 commits into from
Oct 30, 2023
Merged

Improving webpki verifier CRL support ergonomics #1552

merged 2 commits into from
Oct 30, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Oct 24, 2023 •
edited

Description

This branch updates Rustls to use the rustls-webpki v0.102.0-alpha.6 crate. This version of webpki improves CRL ergonomics (see rustls/webpki#203).

Since the changes in this alpha are breaking, I've taken the update & resolved the breaking changes in one commit to keep everything building and testing cleanly.

use with_status_policy builder fn

The upstream crate added a more ergonomic interface we can use in place of having to keep around a mutable builder and doing our own matching.

avoid CRL dyn trait hurdles

The upstream crate made working with CRLs easier by replacing the CertRevocationList trait with an enum representation.

Notably this makes working with the Vec<OwnedCertRevocationList> that the webpki verifier builders and verifiers hold much easier: we no long have to do as many contortions to convert to a &[&dyn CertRevocationList].

TODO

@cpu cpu self-assigned this Oct 24, 2023
@cpu cpu mentioned this pull request Oct 24, 2023
Merged
@codecov
Copy link

codecov bot commented Oct 24, 2023 •
edited

Codecov Report

Merging #1552 (3c58a24) into main (c3f00c7) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1552      +/-   ##
==========================================
- Coverage   96.42%   96.41%   -0.01%     
==========================================
  Files          75       75              
  Lines       15536    15524      -12     
==========================================
- Hits        14980    14968      -12     
  Misses        556      556
Files Coverage Δ
rustls/src/webpki/client_verifier.rs 97.77% <100.00%> (-0.05%) ⬇️
rustls/src/webpki/mod.rs 97.43% <100.00%> (ø)
rustls/src/webpki/server_verifier.rs 96.68% <100.00%> (-0.10%) ⬇️
rustls/src/webpki/verify.rs 99.26% <100.00%> (+<0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cpu cpu mentioned this pull request Oct 24, 2023
Merged
@djc
Copy link
Member

djc commented Oct 25, 2023

Would it be Hard to stack #1547 on top of this one instead of the other way around?

@cpu
Copy link
Member Author

cpu commented Oct 25, 2023

Would it be Hard to stack #1547 on top of this one instead of the other way around?

I don't think it would be too hard, but it does mean that it can't be merged until the webpki parts land and we cut a new alpha (neither very big blockers). Do you feel strongly? #1547 has a +1 from Ctz so it feels closer to being mergeable than this and the webpki PR it uses.

@djc
Copy link
Member

djc commented Oct 25, 2023 •
edited

It seems like the proper order to me just because it means there will be a bunch of churn from porting the code added in #1547 to the new webpki API. But if you prefer to merge #1547 first I can definitely live with that.

In that case I will probably skip doing another round on #1547 and check back in with the rebased version of this.

@cpu
Copy link
Member Author

cpu commented Oct 25, 2023

It seems like the proper order to me just because it means there will be a bunch of churn from porting the code added in #1547 to the new webpki API

I think 264bae2 is the sum of the churn and it seems pretty minimal to me.

In that case I will probably skip doing another round on #1547 and check back in with the rebased version of this.

Ok, that sounds good to me. I'm happy to revisit parts from #1547 if you find anything outside the scope of the rebased content.

@djc
Copy link
Member

djc commented Oct 25, 2023

I think 264bae2 is the sum of the churn and it seems pretty minimal to me.

Okay, that doesn't seem too bad!

djc
djc reviewed Oct 25, 2023
View reviewed changes
rustls/src/webpki/mod.rs Outdated Show resolved Hide resolved
@cpu cpu force-pushed the cpu-1541-crl-for-server-cert-ergonomics branch from 264bae2 to 1540e1a Compare October 25, 2023 16:38
@cpu
Copy link
Member Author

cpu commented Oct 25, 2023

Rebased on main, addressed feedback, and updated the PR desc. Once rustls/webpki#203 lands I can flip this out of draft status and should be able to resolve the few failed CI tasks.

@cpu cpu mentioned this pull request Oct 25, 2023
Merged
20 tasks
@cpu
Copy link
Member Author

cpu commented Oct 25, 2023

Leaving a reminder here: it'd be nice to get a new Rustls alpha after this lands so I can rework the FFI side in rustls/rustls-ffi#341.

@cpu cpu force-pushed the cpu-1541-crl-for-server-cert-ergonomics branch from 1540e1a to b31a413 Compare October 27, 2023 15:36
@cpu cpu marked this pull request as ready for review October 27, 2023 15:41
@cpu
Copy link
Member Author

cpu commented Oct 27, 2023

@djc This is ready for a once-over now if you're interested.

djc
djc approved these changes Oct 27, 2023
View reviewed changes
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.

Did you want to bump the rustls alpha version so you can continue moving downstream into -ffi?

rustls/src/webpki/client_verifier.rs Show resolved Hide resolved
rustls/src/webpki/server_verifier.rs Show resolved Hide resolved
@cpu cpu force-pushed the cpu-1541-crl-for-server-cert-ergonomics branch 2 times, most recently from f57ba4b to c12ce0f Compare October 27, 2023 18:46
@cpu
Copy link
Member Author

cpu commented Oct 30, 2023

@ctz This could use a 🔍 from you when you have a chance. Thanks!

@cpu
Cargo: use rustls-webpki v0.102.0-alpha.6
4143f66 
This version of webpki improves CRL ergonomics. Notable changes:

* use `with_status_policy builder` fn

The upstream crate added a more ergonomic interface we can use in
place of having to keep around a mutable builder and doing our own
matching.

* avoid CRL dyn trait hurdles

The upstream crate made working with CRLs easier by replacing the
`CertRevocationList` trait with an `enum` representation.

Notably this makes working with the `Vec<OwnedCertRevocationList>` that
the webpki verifier builders and verifiers hold much easier: we no long
have to do as many contortions to convert to a `&[&dyn
CertRevocationList]`.
@cpu
Cargo: 0.22.0-alpha.3 -> 0.22.0-alpha.4
3c58a24
@cpu cpu force-pushed the cpu-1541-crl-for-server-cert-ergonomics branch from c12ce0f to 3c58a24 Compare October 30, 2023 15:25
@cpu cpu enabled auto-merge October 30, 2023 15:25
@cpu
Copy link
Member Author

cpu commented Oct 30, 2023

cpu force-pushed the cpu-1541-crl-for-server-cert-ergonomics branch from c12ce0f to 3c58a24

Rebased to resolve conflicts after #1553 landed first.

@cpu cpu added this pull request to the merge queue Oct 30, 2023
Merged via the queue into rustls:main with commit b776a57 Oct 30, 2023
23 checks passed
@cpu cpu deleted the cpu-1541-crl-for-server-cert-ergonomics branch October 30, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc approved these changes

@ctz ctz ctz approved these changes

Assignees

@cpu cpu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cpu @djc @ctz