Skip to content

Fix CRL selection by cRLNumber and scope#491

Merged
djc merged 1 commit into
rustls:mainfrom
xyzzyz:fix-crl-selection
Jun 3, 2026
Merged

Fix CRL selection by cRLNumber and scope#491
djc merged 1 commit into
rustls:mainfrom
xyzzyz:fix-crl-selection

Conversation

@xyzzyz
Copy link
Copy Markdown
Contributor

@xyzzyz xyzzyz commented May 13, 2026

Fixes the issue in #488.

Previously, we used the first CRL with applicable scope on the passed list, without checking that it's the newest one we have.

In the new logic, once we find a CRL with applicable scope, we find the newest available CRL in the same scope, and use that for revocation check. We decide which is newest by CRL number, as RFC 5280 explicitly requires these to be present, and offers them for exactly this purpose:

5.2.3. CRL Number
The CRL number is a non-critical CRL extension that conveys a
monotonically increasing sequence number for a given CRL scope and
CRL issuer. This extension allows users to easily determine when a
particular CRL supersedes another CRL. CRL numbers also support the
identification of complementary complete CRLs and delta CRLs. CRL
issuers conforming to this profile MUST include this extension in all
CRLs and MUST mark this extension as non-critical.

There are some design considerations for handling scenarios when multiple scopes are applicable, and when some CRLs are invalid. These apply to pre-existing code too, so I'm not resolving these in this fix, and instead I'll open a separate issue to discuss them.

I used Codex GPT-5.5 on xhigh while working on this change, but I must say that it while it was very helpful with writing tests, it didn't do a good job on implementing the actual logic: its implementation was overly complicated, with extra unnecessary variables and unnecessarily nested match cases. I basically entirely rewrote it.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 13, 2026

Codecov Report

❌ Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.67%. Comparing base (ce0d2e1) to head (b5951c6).

Files with missing lines Patch % Lines
src/crl/mod.rs 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #491      +/-   ##
==========================================
- Coverage   97.68%   97.67%   -0.02%     
==========================================
  Files          20       20              
  Lines        3971     3995      +24     
==========================================
+ Hits         3879     3902      +23     
- Misses         92       93       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@xyzzyz
Copy link
Copy Markdown
Contributor Author

xyzzyz commented May 13, 2026

Regarding the code coverage results:

For this one:
image

it would be difficult to add coverage for this line in test: it would require deliberately creating an invalid CRL (one with missing CRL number). I made CRL number optional, because the library has not previously rejected CRLs without CRL number (even though RFC 5280 requires it). I could simplify code by requiring CRL number to be present. Should I do it? I think it's a good idea, because the change would only be observable on CRLs that we should be ignoring anyway.

For this one:

image

The library already didn't have coverage for hash, so I'm not sure if I should add this in this PR.

@ctz ctz self-requested a review May 14, 2026 16:33
@xyzzyz
Copy link
Copy Markdown
Contributor Author

xyzzyz commented May 17, 2026

Any chance this could get merged?

@djc
Copy link
Copy Markdown
Member

djc commented May 17, 2026

No need to ping again so soon.

@xyzzyz
Copy link
Copy Markdown
Contributor Author

xyzzyz commented May 17, 2026

Sorry, didn't know if anyone is looking at this.

Copy link
Copy Markdown
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.

Otherwise, this is looking alright to me.

Comment thread src/crl/mod.rs Outdated
@xyzzyz xyzzyz force-pushed the fix-crl-selection branch 4 times, most recently from 0f56a1f to d970949 Compare June 3, 2026 00:14
@xyzzyz
Copy link
Copy Markdown
Contributor Author

xyzzyz commented Jun 3, 2026

I rebased this change on top of main, should be ready for review/merge.

Comment thread src/crl/types.rs Outdated
Comment thread src/crl/mod.rs Outdated
@xyzzyz xyzzyz force-pushed the fix-crl-selection branch from d970949 to 0c4d31f Compare June 3, 2026 11:35
Choose the newest authoritative CRL within the same issuer/IDP scope before checking revocation status.
@xyzzyz xyzzyz force-pushed the fix-crl-selection branch from 0c4d31f to b5951c6 Compare June 3, 2026 12:05
@djc djc added this pull request to the merge queue Jun 3, 2026
Merged via the queue into rustls:main with commit 34e8f9e Jun 3, 2026
21 of 23 checks passed
@xyzzyz
Copy link
Copy Markdown
Contributor Author

xyzzyz commented Jun 3, 2026

Thanks for merging this!

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