-
Notifications
You must be signed in to change notification settings - Fork 109
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
Add CRL Lints for the ReasonCode extension from the baseline requirements and RFC 5280 #715
Conversation
54fa290
to
d871b51
Compare
…ents and RFC 5280.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing major but I think there are a few wording changes that would definitely improve the PR
v3/lints/cabf_br/lint_cabf_crl_reason_code_not_critical_test.go
Outdated
Show resolved
Hide resolved
v3/testdata/crlReasonCodeCrit.pem
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because I was thinking the same thing when creating my current PR (#713 ), do you think it would help keep the test data a bit easier to navigate if you added all these CRLs under a 'crl' subdirectory in testdata?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably? Let's consider moving them in a separate PR though since there already is another CRL Lint in here already.
} | ||
|
||
func (l *crlReasonCodeNotCritical) CheckApplies(c *x509.RevocationList) bool { | ||
for _, c := range c.RevokedCertificates { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it'd be easier to just return true in place of this loop, or if there really must be something in here you could return something like c.RevokedCertificates > 0
. If it returns true then part of the CRL has been iterated over twice, if it returns false then all of the CRL has been iterated over once which might as well happen while executing the lint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. I'd say premature optimization, but for the size of CRLs I think it's worth it.
Co-authored-by: Rob <3725956+robplee@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I'm afraid my approval doesn't help you to merge but here it is anyway :)
Any other thoughts on this PR? |
BRs: https://github.com/cabforum/servercert/blob/main/docs/BR.md#722-crl-and-crl-entry-extensions
RFC 5280: https://datatracker.ietf.org/doc/html/rfc5280#section-5.3.1
Before I mark this as ready for review, I'll need to re-read what I've written to reduce the toil on maintainers.