-
Notifications
You must be signed in to change notification settings - Fork 619
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
expand webpki verifier CRL support #1547
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1547 +/- ##
==========================================
+ Coverage 96.49% 96.51% +0.02%
==========================================
Files 74 75 +1
Lines 15113 15355 +242
==========================================
+ Hits 14583 14820 +237
- Misses 530 535 +5
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I'll see if I can address some of these coverage gaps (likely ~Mon). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't looked at the testing commits much yet, here's a first round of feedback.
* Capitalize first word. * Break out headline sentence. * Wrap the rest.
In the future we'll have similar unit tests for the server certificate verifiers. This commit clarifies the tests that are specific to the client certificate verifier.
This commit exposes support for configuring client certificate revocation checking depth when building a webpki client certificate verifier. By default when CRLs are provided revocation status checks will be performed for all certificates in the verified chain to a trust anchor (minus the trust anchor itself). Optionally users of the verifier builder may opt to relax this behaviour to check only the end entity certificate's revocation status.
This commit exposes support for configuring client certificate revocation behaviour for the case where revocation status can not be determined. By default the webpki client certificate verifier built by the `ClientCertVerifierBuilder` will treat unknown revocation status as an error. Users of the builder may optionally relax this behaviour to allow unknown revocation status.
This commit renames the `client_verifier_builder.rs` module to be named `client_verifier.rs`. In a subsequent commit we'll move the verifier impl that is built by the verifier builder into the same module. This will make it easier to create a `server_verifier.rs` module for the server certificate verifier equivalents, and keeps the builder and the type it builds in the same module for clarity.
This commit moves the `WebPkiClientVerifier` type (and associated bits) from `verify.rs` into `client_verifier.rs` to live alongside its builder type.
This commit moves the unit tests for the `crl_error` and `pki_error` helpers to be defined alongside the helpers themselves, in `mod.rs`, as opposed to `verify.rs`.
This commit renames the existing `ClientCertVerifierBuilderError` to be called `VerifierBuilderError`. The top-level rustdoc comment is also updated to better reflect the usage of the type.
This commit moves the `VerifierBuilderError` from `client_verifier.rs` to `mod.rs` where it can be shared between both the client and server verifiers in the future.
This commit lifts out a private `borrow_crls` fn that can be used to convert a `&Vec<webpki::OwnedCertRevocationList>` to a `Vec<&dyn webpki::CertRevocationList>`. Presently there is only one call-site, but in future commits we will add a second and it's helpful to not have to duplicate the code + clippy allow.
This commit moves the existing `WebPkiServerVerifier` type out of `verify.rs` and into `server_verifier.rs`. This matches the new `client_verifier.rs` module introduced in a previous commit and paves the way for having a single module location for both the verifier type and a builder for making it.
This commit reworks the `WebPkiServerCertVerifier` type to use a builder model similar to the `WebPkiClientCertVerifier` type. The new `ServerCertVerifierBuilder` additionally exposes support for configuring the depth of revocation status checking, and how to handle unknown revocation status.
Previously the test-ca `build-a-pki.sh` script would revoke each key type's client certificate to produce a `client.revoked.crl.pem` CRL. In this commit we update the script to do the same for each key type's intermediate cert (`inter.cert`) to produce a `inter.revoked.crl.pem`, as well as the server ee cert (`end.cert`) to produce a `end.revoked.crl.pem` file. This will be useful for testing the chain depth revocation controls, and the server verifier CRL support.
8fd505a
to
a442247
Compare
This commit updates the existing client certificate revocation testing to also exercise the two new verifier options for controlling the depth of revocation checking, and deciding how to handle unknown revocation status.
This commit adds test coverage for a client connecting to a server with a webpki server certificate verifier configured to do CRL revocation checking.
a442247
to
076c1bf
Compare
This matches OpenSSL, BoringSSL and AWS-LC. Since the existing functionality was heavily referencing these projects it seems sensible to do the same here.
I took a look at where coverage was missing and it seemed to only be from the change to separate the public API helper for verifying a certificate chains to a trust anchor from the crate internal one. Our existing test coverage now only uses the crate internal |
73c846d
to
f839f18
Compare
Djc indicated he's going to hold doing a second pass on this until the ergonomic refactorings are rebased on top, so I'm going to go ahead and merge this one with the +1 from Ctz. Thanks for the reviews! |
The diffs here are either very small, or speedups, so I think we're OK here. |
This branch achieves three top-level goals: