-
Notifications
You must be signed in to change notification settings - Fork 51
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
crl: retain issuing distribution point extension #128
Conversation
Codecov Report
@@ Coverage Diff @@
## main #128 +/- ##
==========================================
+ Coverage 95.04% 95.35% +0.31%
==========================================
Files 15 15
Lines 3735 4007 +272
==========================================
+ Hits 3550 3821 +271
- Misses 185 186 +1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
1fa8868
to
fe417d5
Compare
fe417d5
to
a3f6a57
Compare
I think this is generally ready for review now.
I see a couple low hanging test cases I missed, will push a revision. |
c31814c
to
4601fb6
Compare
Done.
So close 😩 In The other gap is in |
This commit adds an iterator over all possible `crl::RevocationReason` values. This avoids needing to hard-code the variant values into a range, or remember to skip the unused values.
Previously we implemented support for parsing the optional `DistributionPointName` from a certificate's CRL distribution points extension with code/types in `cert.rs`. While the top-level extension differs slightly for the CRL issuing distribution point extension, it shares the same encoding for the `distributionPoint` field holding a `DistributionPointName.` In anticipation of sharing this code between `cert.rs` and `crl.rs` this commit lifts the existing code from `cert.rs` into `x509.rs` where it can more easily be shared between the two, similar to other bits common to both certs and CRLs.
4601fb6
to
6e56f33
Compare
This commit updates both the `BorrowedCertRevocationList` and `OwnedCertRevocationList` to retain the raw DER encoding of the CRL issuing distribution point extension, if present. The `CertRevocationList` trait is then updated with a `issuing_distribution_point` fn that can return the raw DER for further processing when considering a CRL during CRL validation (not yet implemented). This commit additionally adds crate-local functions and types for working with a parsed representation of the CRL issuing distribution point extension. This mostly involves recognizing the top level extension properties. We're able to share some code with the certificate CRL distribution point extension (notably for handling the distribution point names). Since, when present, the IDP extension can assert that the CRL has features we don't support (e.g. that it's an indirect CRL, or that it has a distribution point name relative to an issuer name) we parse it up-front and validate that the CRL meets our requirements. Having both the certificate CRL distribution points, and the CRL issuing distribution point will allow subsequent work to tighten up the CRL validation process by matching information between the two.
This commit adds test coverage for the new parsing logic for CRL issuing distribution point extensions. For the "happy" paths we use a small Python script that uses pyca cryptography to generate test CRLs with the required extensions. For some invalid testcases we can't easily use pyca cryptography due to its (sensible) error checking. Instead, we use ascii2der, tweaking the ASCII representation of previously generated CRLs to produce the required invalid DER, converting back to DER with der2ascii. The associated .txt and .der files are checked in for convenience. The new test case generation is done separately from `tests/generate.py` because these test files are used in unit tests (since the code under test is internal to the crate) as opposed to integration tests (like `tests/generate.py` creates).
This commit extends the existing `testgen.yml` workflow to also ensure that running the CRL issuing distribution point testcase generation produces no diffs from what's checked-in.
6e56f33
to
d3b9e03
Compare
Thanks for the 🔍's |
Description
This branch implements the second requirement for resolving #121 - support for parsing and retaining the CRL issuing distribution point extension. This is similar, but not identical to the certificate CRL distribution points extension.
crl/cert: make reason codes more ergonomic.
This commit adds an iterator over all possible
crl::RevocationReason
values. This avoids needing to hard-code the variant values into a range, or remember to skip the unused values.cert: lift DistributionPointName bits to x509.
Previously we implemented support for parsing the optional
DistributionPointName
from a certificate's CRL distribution pointsextension with code/types in
cert.rs
.While the top-level extension differs slightly for the CRL issuing distribution point extension, it shares the same encoding for the
distributionPoint
field holding aDistributionPointName.
In anticipation of sharing this code between
cert.rs
andcrl.rs
this commit lifts the existing code fromcert.rs
intox509.rs
where it can more easily be shared between the two, similar to other bits common to both certs and CRLs.crl: retain CRL issuing distribution point ext.
This commit updates both the
BorrowedCertRevocationList
andOwnedCertRevocationList
to retain the raw DER encoding of the CRL issuing distribution point extension, if present. TheCertRevocationList
trait is then updated witha
issuing_distribution_point
fn that can return the raw DER for further processing when considering a CRL during CRL validation (not yet implemented).This commit additionally adds crate-local functions and types for working with a parsed representation of the CRL issuing distribution point extension. This mostly involves recognizing the top level extension properties. We're able to share some code with the certificate CRL distribution point extension (notably for handling the distribution point names).
Having both the certificate CRL distribution points, and the CRL issuing distribution point will allow subsequent work to tighten up the CRL validation process by matching information between the two.
crls: test issuing distribution point ext.
This commit adds test coverage for the new parsing logic for CRL issuing distribution point extensions.
For the "happy" paths we use a small Python script that uses pyca cryptography to generate test CRLs with the required extensions.
For some invalid testcases we can't easily use pyca cryptography due to its (sensible) error checking. Instead, we use ascii2der, tweaking the ASCII representation of previously generated CRLs to produce the required invalid DER, converting back to DER with der2ascii. The associated .txt and .der files are checked in for convenience.
The new test case generation is done separately from
tests/generate.py
because these test files are used in unit tests (since the code under test is internal to the crate) as opposed to integration tests (liketests/generate.py
creates).The net result is full coverage for the new parsing code.
ci: include CRL issuing distrib. point test gen in CI
This commit extends the existing
testgen.yml
workflow to also ensure that running the CRL issuing distribution point testcase generation produces no diffs from what's checked-in.