Continued webpki CRL support. #66
Conversation
Codecov Report
@@ Coverage Diff @@
## main #66 +/- ##
=======================================
Coverage 95.59% 95.59%
=======================================
Files 15 15
Lines 2907 2953 +46
=======================================
+ Hits 2779 2823 +44
- Misses 128 130 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
cpu
changed the title
|
Rebased to resolve conflicts. Fixed clippy errs with some temp. rustdocs. Updated PR desc to reflect this only builds on 1 outstanding PR now (the CRL parsing pt1 work). |
Rebased, updated PR desc, and addressed the simple feedback. |
|
I pulled out a separate issue for the error specificity: #79 Of the discussion topics this one feels like the largest blocker to getting a CRL support feature completed. Key usage is handled for intermediates in this branch, implementing that for trust anchors won't be very difficult if we decide its worthwhile. Similar for the NextUpdate question, and extending support to the server certificate context. |
Just some small non-functional changes (PR desc updated as well):
I also noticed I accidentally dropped the KeyUsage processing in one of my force pushes. Since this branch has a +1 from ctz and that work introduces new code across a few files I'm going to bring that back as a separate follow up. It's a change set that stands on its own so I think it will be easier to review in isolation from this larger work. |
|
Crate-internal consumers of `build_chain` always pass `0` as the sub CA count, only the `verify_cert.rs` internal recursion changes this parameter. This commit separates the external interface from the internal recursion to remove one extra parameter from an already complicated interface.
Previously the `Cert` type was not exported through `lib.rs` for external usage. This commit relaxes this slightly in preparation for passing `Cert` objects to external code to use for sourcing relevant CRLs: * We export `Cert`, but keep the inner fields crate-visible only. This matches the existing usage and keeps `untrusted::Input` out of our public API. * We export `EndEntityOrCa`. * We add public methods that allow access to the cert's DER encoded serial, issuer, and subject as `&[u8]`. We also allow access to the `EndEntityOrCa` enum for determining if the cert is a leaf, or an issuer. * Lastly, some unreachable item clippy warnings in `der.rs` and `signed_data.rs` that emerge after making these changes are addressed by changing a few `pub` items to `pub(crate)`.
Based on discussions on other PRs I think we can say we favour up-front detection of invalid DER for CRLs as opposed to discovering these errors at the time we scan through the CRL looking for revoked certificates. That's already what is implemented so this commit removes a TODO that was there as a placeholder for discussion.
This commit adds a comment and a pointer to an issue in the issue tracker about replacing the linear scan of CRL contents with an optimized version that can be gated by the alloc feature.
Provides a mechanism for specifying whether revocation checking with CRLs is performed when building a chain, and how the CRLs will be provided. To start we only expose this as an optional argument in the public interface for checking the validity of a client certificate. It is not yet consulted during chain building.
In preparation for needing more than just the TrustAnchor SPKI this commit updates `check_signatures` to accept a reference to the full `TrustAnchor` instead of just the SPKI input.
This commit starts to connect up the pieces that will be required for revocation checking during path building. Presently the CRL checking is a `todo!` to be continued in subsequent commits.
* Find the relevant CRL by invoking the crl provider with the cert. * Verify the relevant CRL's signature using the cert's issuer's SPKI. * Use the verified CRL contents to determine if the cert is revoked. Left to do: enforce that the CRL issuer has the correct KU for CRL signing, or no KU at all.
This commit adds a generator for a number of client cert validation test cases. These test cases cover: * Revocation checking when no CRLs are provided. * Revocation checking when no relevant CRLs are provided. * Revocation checking when a relevant CRL is provided, but no certificates in the chain are listed as revoked. * Revocation checking when the CRL signature can't be validated. * Revocation checking when a certificate is revoked and the CRL issuer doesn't specify any key usage. All of these test scenarios are tested both for revocation checking at the depth of just the end entity certificate, and for the EE cert and any intermediates in use. Root certificates are not checked for revocation.
Created by running: ``` cd tests ./generate.py --no-tls-server-certs --no-signatures --no-clientauth ```
Description
This branch adds support for verifying certificates are not revoked by consulting CRLs provided through implementations of a simple provider trait. Trust anchor revocation status is not considered.
In general I've tried to adapt to the existing webpki code without making any significant invasive changes to the path building logic. There's not a lot of test coverage in this area of the codebase and it's a tricky problem domain with many pitfalls.
Points for discussion
There are some rough edges left to be figured out as follow-up work:
Error messages for revocation related events are incorrect. All error states are reported as "UnknownIssuer". See Options for improving build_chain error specificity #79
There is no enforcement of the current time being greater than the CRL's
NextUpdate. I found most implementations I surveyed chose not to treat this as a hard error so for now I've omitted any enforcement. Maybe this should be configurable?There are some optimizations left on the table. Providing a relevant CRL is handled by a trait that could do some kind of caching if desired, but the actual act of iterating the relevant CRL to find a particular serial is handled by webpki and done with a naive linear scan. Some implementations (like BoringSSL) support maintaining a sorted list to allow faster search. We can probably rework the trait to support this but it will be trickier so for now I've left the simple solution in place. Pulled out to a separate issue: CRL optimization: rework to avoid linear scan of CRL contents #80
Similarly we perform CRL signature validation per-usage instead of once up-front. Pulled out to a separate issue: CRL optimization: verify CRL signatures once, up-front. #81
None of the revocation interface/checking is exposed for server cert validation, it's strictly for client cert validation. I think that's the most important use case and we can adapt the existing code for server cert chains pretty easily once stabilized.
Related to rustls/rustls#1164, #57. Continues from #44.