Skip to content

(#983) Fix CRL extensions lint#984

Merged
christopher-henderson merged 1 commit intozmap:masterfrom
XolphinMartijn:#983-crl-extensions-should-not
Aug 17, 2025
Merged

(#983) Fix CRL extensions lint#984
christopher-henderson merged 1 commit intozmap:masterfrom
XolphinMartijn:#983-crl-extensions-should-not

Conversation

@XolphinMartijn
Copy link
Copy Markdown
Contributor

This update includes:

  • Removal of crl_with_wrong_critical_for_crl_distribution.pem and the accompanying test case
    • This test case appears to be incorrect. It tests the criticality of the CRL Distribution Point, however the BRs do not state any requirements on the criticality of this extension. I expect this was intended to test the Issuing Distribution Point criticality, but the test CRL does not include that extension
  • Mark any other not explicitly allowed extension as a NOT RECOMMENDED with accompanying WARNING response from zlint. The current code returns an Error response, however the BRs do not disallow other extensions

@certigna
Copy link
Copy Markdown

Thanks a lot

Copy link
Copy Markdown
Member

@christopher-henderson christopher-henderson left a comment

Choose a reason for hiding this comment

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

Thank you @XolphinMartijn

@kowshikRoy a heads-up that I agree with this change. At the heart of the issue is that the lint explicitly enumerated a list of discouraged extensions and (properly) issued a warning if they were present. However, the BRs states that this should be the behavior for Any other value, not just those specific values that were explicitly outlined.

In the test case changed in this PR, cRLReason changes from being an error to being a warning.

@christopher-henderson christopher-henderson merged commit 34901b1 into zmap:master Aug 17, 2025
4 checks passed
Copy link
Copy Markdown
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

This test case should not have been removed; it instead should have been replaced with a CRL that has the proper issuingDistributionPoint (OID 2.5.29.31) extension, instead of the improper crlDistributionPoints (OID 2.5.29.28) extension, which is used in Certificates.

This test fix was missed in #974, which only updated the happy-path test case.

@XolphinMartijn
Copy link
Copy Markdown
Contributor Author

This test case should not have been removed; it instead should have been replaced with a CRL that has the proper issuingDistributionPoint (OID 2.5.29.31) extension, instead of the improper crlDistributionPoints (OID 2.5.29.28) extension, which is used in Certificates.

I'll agree with Aaron here. This was a question of interpretation. My interpretation was they were testing the Fail on a not allowed extension, which was incorrect. If the test entity was incorrect, indeed Aaron's interpretation also makes sense, and we should add such a test

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.

4 participants