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

revocation: more sophisticated revocation checking. #138

Merged
merged 4 commits into from
Aug 2, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Jul 26, 2023

This branch resolves the issues highlighted in #121, enforcing that certificate CRL coverage is handled properly, and unknown revocation status can be treated as an error condition when appropriate.

revocation: policy for unknown revocation status.

This commit reworks the CRL revocation checking interface to allow specifying more policy for how CRLs should be used with a builder-based API.

In particular:

  • The depth of revocation checking can be controlled, allowing users to only check the revocation status of end entity certificates.
  • Handling of unknown revocation status can be controlled, allowing users to decide if unknown status should be treated as an error or not.
  • We can ensure that when revocation options are provided, there is always at least one CRL to use.

revocation: use cert CRL DP and CRL IDP extensions.

This commit updates the CRL validation process so that when selecting a CRL we consider more than just whether the cert issuer matches the CRL issuer.

As of this commit we consider a CRL authoritative for a certificate when:

  • The certificate issuer matches the CRL issuer and,
  • The certificate has no CRL distribution points, and the CRL has no issuing distribution point extension.
  • Or, the certificate has no CRL distribution points, but the the CRL has an issuing distribution point extension with a scope that includes the certificate.
  • Or, the certificate has CRL distribution points, and the CRL has an issuing distribution point extension with a scope that includes the certificate, and at least one distribution point full name is a URI type general name that can also be found in the CRL issuing distribution point full name general name sequence.

In all other circumstances the CRL is not considered authoritative. If we can't find an authoritative CRL from the set of loaded CRLs the certificate will be considered as having unknown revocation status. Whether this is an error or not is controlled by the user provided revocation policy.

tests: integration tests for new revocation logic.

Extends existing integration tests for new revocation logic. Also de-duplicates the owned/borrowed test generation.

misc: clippy fixes

Addresses some clippy findings that bubbled up while working on this branch.

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #138 (3b4f7ad) into main (97f841d) will increase coverage by 0.13%.
The diff coverage is 99.04%.

@@            Coverage Diff             @@
##             main     #138      +/-   ##
==========================================
+ Coverage   96.08%   96.21%   +0.13%     
==========================================
  Files          15       15              
  Lines        4108     4304     +196     
==========================================
+ Hits         3947     4141     +194     
- Misses        161      163       +2     
Files Changed Coverage Δ
src/cert.rs 96.91% <ø> (ø)
src/x509.rs 97.91% <ø> (ø)
src/crl.rs 99.53% <98.07%> (-0.13%) ⬇️
src/verify_cert.rs 96.61% <99.35%> (+1.44%) ⬆️
src/end_entity.rs 100.00% <100.00%> (ø)
src/error.rs 50.00% <100.00%> (ø)

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

src/end_entity.rs Outdated Show resolved Hide resolved
src/verify_cert.rs Outdated Show resolved Hide resolved
src/verify_cert.rs Outdated Show resolved Hide resolved
src/verify_cert.rs Outdated Show resolved Hide resolved
src/verify_cert.rs Outdated Show resolved Hide resolved
src/crl.rs Outdated Show resolved Hide resolved
src/crl.rs Outdated Show resolved Hide resolved
src/crl.rs Outdated Show resolved Hide resolved
src/x509.rs Outdated Show resolved Hide resolved
src/crl.rs Outdated Show resolved Hide resolved
@cpu cpu force-pushed the cpu-221-enforce-coverage branch 3 times, most recently from 3b56bf9 to 78cdafa Compare July 27, 2023 21:31
@cpu
Copy link
Member Author

cpu commented Jul 27, 2023

I rebased this branch and addressed the first round of feedback but I've left is a draft because it's still missing test coverage. Will land that coverage ASAP and then flip this out of draft.

@cpu cpu force-pushed the cpu-221-enforce-coverage branch 2 times, most recently from cf06c99 to c1e0045 Compare July 28, 2023 21:06
@cpu
Copy link
Member Author

cpu commented Jul 28, 2023

Rebased and added some initial test coverage. I'm going to keep this as a draft for a little bit longer, the coverage isn't 100% there yet and the Python is getting pretty messy. Closer to its final form if someone is feeling eager to review :-)

@cpu cpu force-pushed the cpu-221-enforce-coverage branch 3 times, most recently from 1705ddd to 782aaa8 Compare July 31, 2023 18:34
@cpu cpu marked this pull request as ready for review July 31, 2023 18:36
@cpu
Copy link
Member Author

cpu commented Jul 31, 2023

I think this branch is now ready for review. It's freshly rebased & there's now sufficient test coverage.

src/verify_cert.rs Outdated Show resolved Hide resolved
src/verify_cert.rs Outdated Show resolved Hide resolved
src/verify_cert.rs Outdated Show resolved Hide resolved
src/verify_cert.rs Outdated Show resolved Hide resolved
src/crl.rs Outdated Show resolved Hide resolved
src/crl.rs Outdated Show resolved Hide resolved
src/crl.rs Outdated Show resolved Hide resolved
@cpu cpu force-pushed the cpu-221-enforce-coverage branch from 782aaa8 to 64386d0 Compare July 31, 2023 20:36
@cpu cpu marked this pull request as draft July 31, 2023 20:38
@cpu
Copy link
Member Author

cpu commented Jul 31, 2023

cpu marked this pull request as draft 2 minutes ago

Messed up my rebase and over-squashed. Fixing that up now.

@cpu cpu force-pushed the cpu-221-enforce-coverage branch 2 times, most recently from ed1684f to ce94f2a Compare July 31, 2023 20:52
@cpu cpu marked this pull request as ready for review July 31, 2023 20:54
@cpu
Copy link
Member Author

cpu commented Jul 31, 2023

Fixing that up now.

Done.

src/lib.rs Outdated Show resolved Hide resolved
src/verify_cert.rs Outdated Show resolved Hide resolved
@cpu cpu force-pushed the cpu-221-enforce-coverage branch from ce94f2a to 9ed69c4 Compare August 1, 2023 13:25
@cpu cpu requested a review from ctz August 1, 2023 13:27
@cpu
Copy link
Member Author

cpu commented Aug 1, 2023

ci / Build+test (--no-default-features, stable, ubuntu-20.04) (pull_request) Failing after 2m

Looks like a network flake:
Caused by:
  failed to download from `[https://index.crates.io/3/s/syn`](https://index.crates.io/3/s/syn%60)

Caused by:
  [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last [30](https://github.com/rustls/webpki/actions/runs/5727388816/job/15519692580?pr=138#step:4:31) seconds)

Going to kick that task.

cpu added 2 commits August 2, 2023 08:40
This commit reworks the CRL revocation checking interface to allow
specifying more policy for how CRLs should be used with a builder-based
API.

In particular:

* The depth of revocation checking can be controlled, allowing users to
  only check the revocation status of end entity certificates.
* Handling of unknown revocation status can be controlled, allowing
  users to decide if unknown status should be treated as an error or
  not.
* We can ensure that when revocation options are provided, there is
  always at least one CRL to use.
This commit updates the CRL validation process so that when selecting
a CRL we consider more than just whether the cert issuer matches the CRL
issuer.

As of this commit we consider a CRL authoritative for a certificate
when:

* The certificate issuer matches the CRL issuer and,
* The certificate has no CRL distribution points, and the CRL has no
  issuing distribution point extension.
* Or, the certificate has no CRL distribution points, but the the CRL
  has an issuing distribution point extension with a scope that includes
  the certificate.
* Or, the certificate has CRL distribution points, and the CRL has an
  issuing distribution point extension with a scope that includes the
  certificate, and at least one distribution point full name is a URI
  type general name that can also be found in the CRL issuing
  distribution point full name general name sequence.

In all other circumstances the CRL is not considered authoritative. If
we can't find an authoritative CRL from the set of loaded CRLs the
certificate will be considered as having unknown revocation status.
Whether this is an error or not is controlled by the user provided
revocation policy.
cpu added 2 commits August 2, 2023 08:41
Extends existing integration tests for new revocation logic. Also
de-duplicates the owned/borrowed test generation.
@cpu cpu force-pushed the cpu-221-enforce-coverage branch from 9ed69c4 to 3b4f7ad Compare August 2, 2023 12:44
@cpu cpu enabled auto-merge August 2, 2023 12:44
@cpu
Copy link
Member Author

cpu commented Aug 2, 2023

cpu force-pushed the cpu-221-enforce-coverage branch from 9ed69c4 to 3b4f7ad

Rebased and fixed conflicts. Queuing for merge. Thanks for the reviews!

@cpu cpu added this pull request to the merge queue Aug 2, 2023
Merged via the queue into rustls:main with commit e5c8c6d Aug 2, 2023
@cpu cpu deleted the cpu-221-enforce-coverage branch August 2, 2023 12:50
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