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

cert: retain CRL distribution points extension. #127

Merged
merged 7 commits into from
Jul 26, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Jul 17, 2023

Description

This branch implements the first requirement for resolving #121 - support for parsing and retaining a certificate CRL distribution points extension.

crl: remove unused dead_code allow.

Small tidy commit - this allow(dead_code) annotation isn't necessary - we removed the unused fields that were being parsed but not consulted previously.

der: make bit_string_flag usable w/o outer tag/len.

Previously we introduced support in the der module for handling bit string flag sets. The way this was implemented was based on the use for certificate key usage fields where the BitString is wrapped in a type/length/value construction. The implementation of parsing the flag set expected to peel back this type/length and before processing the value.

In the certificate CRL distribution point extension there is also a bit string flag set for revocation reasons, but notably the encoding doesn't include a tag/length and instead the value appears directly in a primitive encoding.

To be able to support parsing both styles of flag sets we move the handling of the outer type/length/value to the verify_cert.rs call-site and update bit_string_flags to operate directly on the raw value.

This also requires removing one test case from the bit_string_flags unti test (for trailing data) that was being rejected by the tag/length/value parsing finding data in the value after the expected length bytes. This job isn't being performed by the function under test anymore and so the test case isn't relevant.

subject_name: crate export general name code.

This commit updates the subject_name module to export the verify::GeneralName type and the verify::general_name parser.

The certificate CRL distribution point extension embeds GeneralName's and this will allow the other parts of the crate to use the same logic for handling these as the subject_name crate.

cert: retain CRL distribution points extension.

This commit updates the Cert representation used for end entity and intermediate issuing certificates to recognize and retain the certificate revocation list (CRL) distribution points extension as described in RFC 5280 Section 4.2.3.13.

In the future we will use this extension along with the issuer distribution point extension in CRLs to ensure we properly match certificates to the CRL that should be used to check revocation status for the certificate.

At present these mechanisms are crate-internal visibility, and not used
in the CRL validation process, so we allow the dead_code warnings to prevent compilation errors until usage is implemented.

cert: add test coverage for CRL distrib point ext.

This commit adds test coverage for the new parsing logic for certificate CRL distribution point extensions.

For the "happy" paths we use a small Python script that uses pyca cryptography to generate test certificates 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 certificates 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).

The net result is 100% coverage for the new parsing code.

ci: include CRL distrib. point test in testgen step.

This commit extends the existing testgen.yml workflow to also ensure that running the CRL distribution point testcase generation produces no diffs from what's checked-in.

@cpu cpu self-assigned this Jul 17, 2023
@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #127 (76976be) into main (bef457f) will increase coverage by 0.26%.
The diff coverage is 98.54%.

@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
+ Coverage   94.75%   95.02%   +0.26%     
==========================================
  Files          15       15              
  Lines        3432     3738     +306     
==========================================
+ Hits         3252     3552     +300     
- Misses        180      186       +6     
Files Changed Coverage Δ
src/crl.rs 100.00% <ø> (ø)
src/cert.rs 96.49% <98.41%> (+4.30%) ⬆️
src/der.rs 98.09% <100.00%> (-0.30%) ⬇️
src/subject_name/verify.rs 96.56% <100.00%> (+0.01%) ⬆️
src/verify_cert.rs 95.23% <100.00%> (+0.05%) ⬆️

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

@cpu cpu changed the title cert: retain CRL distribution point extension. cert: retain CRL distribution points extension. Jul 18, 2023
Copy link
Member

@ctz ctz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. i did look closely to check that introducing new kinds of GeneralName won't have any surprising consequences for other APIs.

@cpu
Copy link
Member Author

cpu commented Jul 19, 2023

i did look closely to check that introducing new kinds of GeneralName won't have any surprising consequences for other APIs.

Thanks for double checking that 👍 I tried to be cautious there.

@cpu
Copy link
Member Author

cpu commented Jul 19, 2023

cpu force-pushed the cpu-221-cert-crl-dp branch from 36a3ca9 to 4b9e501

Tweaking tests/crl_distrib_point/make_testcerts.py - noticed I was accidentally hardcoding the with_reasons test name in all of the certificate subject/issuer fields instead of the correct test name.

@cpu
Copy link
Member Author

cpu commented Jul 21, 2023

(fwiw, I'm holding merging this until I've got everything working end to end in the event there are further changes required here on in #128)

src/subject_name/verify.rs Outdated Show resolved Hide resolved
src/cert.rs Outdated Show resolved Hide resolved
src/cert.rs Outdated Show resolved Hide resolved
cpu added 4 commits July 26, 2023 14:03
This `allow(dead_code)` annotation isn't necessary - we removed the
unused fields that were being parsed but not consulted previously.
Previously we introduced support in the `der` module for handling bit
string flag sets. The way this was implemented was based on the use
for certificate key usage fields where the `BitString` is wrapped in
a type/length/value construction. The implementation of parsing the flag
set expected to peel back this type/length and before processing the
value.

In the certificate CRL distribution point extension there is also
a bit string flag set for revocation reasons, but notably the encoding
doesn't include a tag/length and instead the value appears directly in
a primitive encoding.

To be able to support parsing both styles of flag sets we move the
handling of the outer type/length/value to the `verify_cert.rs`
call-site and update `bit_string_flags` to operate directly on the raw
value.

This also requires removing one test case (for trailing data) that was
being rejected by the tag/length/value parsing finding data in the value
after the expected length bytes.
This commit updates the `subject_name` module to export the
`verify::GeneralName` type.

The certificate CRL distribution point extension embeds `GeneralName`'s
and this will allow the other parts of the crate to use the same logic
for handling these as the `subject_name` crate.
The certificate CRL distribution point extension frequently contains
`GeneralName` instances that denote a Uniform Resource Identifier (URI)
that can be used to fetch a CRL over HTTP, or LDAP.

This commit updates `subject_name` with
a `GeneralName::UniformResourceIdentifier` variant, and support for
creating it in `general_name`.
@cpu
Copy link
Member Author

cpu commented Jul 26, 2023

I have confidence in the end-to-end shape of the overall work now and I think (with a +1 from @djc) this piece would be ready to merge.

src/cert.rs Outdated Show resolved Hide resolved
src/cert.rs Outdated Show resolved Hide resolved
cpu added 3 commits July 26, 2023 15:05
This commit updates the `Cert` representation used for end entity
and intermediate issuing certificates to recognize and retain the
certificate revocation list (CRL) distribution point extension as
described in RFC 5280 Section 4.2.3.13.

In the future we will use this extension along with the issuer
distribution point extension in CRLs to ensure we properly match
certificates to the CRL that should be used to check revocation status
for the certificate.

At present these mechanisms are crate-internal visibility, and not used
in the CRL validation process, so we allow the `dead_code` warnings to
prevent compilation errors until usage is implemented.
This commit adds test coverage for the new parsing logic for certificate
CRL distribution point extensions.

For the "happy" paths we use a small Python script that uses pyca
cryptography to generate test certificates 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 certificates to produce the
required invalid DER, converting back to DER with `der2ascii`. The
associated `.txt` and `.der` files are checked in for convenience.

The net result is 100% coverage for the new parsing code.
This commit extends the existing `testgen.yml` workflow to also ensure
that running the CRL distribution point testcase generation produces no
diffs from what's checked-in.
@cpu
Copy link
Member Author

cpu commented Jul 26, 2023

Thanks for the quick re-review 🙇

@cpu cpu enabled auto-merge July 26, 2023 19:06
@cpu cpu added this pull request to the merge queue Jul 26, 2023
Merged via the queue into rustls:main with commit cd964da Jul 26, 2023
22 checks passed
@cpu cpu deleted the cpu-221-cert-crl-dp branch July 26, 2023 19:11
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.

3 participants