Skip to content

Add testcases exercising general CRL structure - #577

Merged
woodruffw merged 2 commits into
C2SP:mainfrom
trail-of-forks:ap/crl-structure
Sep 24, 2026
Merged

woodruffw merged 2 commits into
C2SP:mainfrom
trail-of-forks:ap/crl-structure

Conversation

@tnytown

@tnytown tnytown commented Mar 26, 2026 •

Copy link
Copy Markdown
Contributor

Here's a tranche of CRL testcases as outlined in pyca/cryptography#10393 (comment):

  • CRL with invalid signature
  • CRL signed by incorrect private key
  • Empty CRL (no revoked certificates)
  • Very large CRL (performance test)
  • CRL with unknown critical extension
  • CRL with unknown non-critical extension
  • CRL with duplicate revoked entry serial numbers
  • CRL entry with unknown critical extension
  • CRL entry with unknown non-critical extension

@tnytown

tnytown commented Mar 26, 2026 •

Copy link
Copy Markdown
Contributor Author

For most of these tests, I had to be careful not to conflate a failure to interpret the CRL and a failure to build the chain. Consider a CRL containing an uninterpretable critical extension and an entry revoking the leaf certificate. For this case, a conformant verifier would fail because the CRL is invalid, while a non-conformant verifier would fail to build the chain as it interprets the CRL and revokes the certificate. I worked around this for these tests, but there may be something we can do in limbo to better support verifying CRL validation behavior.

@tnytown

tnytown commented Jun 15, 2026 •

Copy link
Copy Markdown
Contributor Author

@woodruffw can I get a review on this when you're available? i think the regression check is failing because it's a PR from a fork.

@tnytown
tnytown force-pushed the ap/crl-structure branch from 997c361 to 6b1bdb1 Compare June 17, 2026 00:37
@tnytown
tnytown marked this pull request as draft June 17, 2026 14:48
@tnytown
tnytown marked this pull request as ready for review June 17, 2026 17:22
@nbastin

nbastin commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

These look generally OK to me, but I just wanted to jump in here to say that "CRL with duplicate revoked entry serial numbers" should be slotted into RFC5280 tests, as this is completely valid (and intentionally used) in non-RFC5280 X.509 PKI profiles.

@nbastin

nbastin commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Also, and maybe I'm just not interpreting the test cases properly, but the descriptions for extensions don't make clear whether the test is about crlExtensions at the top level, or about crlEntryExtensions inside revokedCertificates.

@tnytown

tnytown commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Also, and maybe I'm just not interpreting the test cases properly, but the descriptions for extensions don't make clear whether the test is about crlExtensions at the top level, or about crlEntryExtensions inside revokedCertificates.

Only the last two tests are applicable to crlEntryExtensions (hence the "CRL entry" verbiage) whereas the other extension tests are for crlExtensions; I can reword the descriptions for the latter to reflect that they apply to the top-level extensions.

@tnytown
tnytown force-pushed the ap/crl-structure branch 2 times, most recently from 08851a6 to 59ab528 Compare September 21, 2026 20:48
Comment thread limbo/testcases/_core.py
revoked: list[x509.RevokedCertificate] = [],
aki: _Extension[x509.AuthorityKeyIdentifier] | Literal[True] | None = True,
crl_number: _Extension[x509.CRLNumber] | Literal[True] | None = True,
extra_extension: _Extension[x509.ExtensionType] | None = None,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This seems fine, but maybe it can be extra_extensions for generality?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

agree! we'll have to change the identical surface in the cert builders as well; probably deserves a separate PR

@tnytown
tnytown force-pushed the ap/crl-structure branch 4 times, most recently from a75269e to 0055618 Compare September 23, 2026 20:33

@woodruffw woodruffw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you @tnytown!

@woodruffw
woodruffw merged commit 554528a into C2SP:main Sep 24, 2026
11 checks passed
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