-
Notifications
You must be signed in to change notification settings - Fork 110
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 lint to detect invalid cps uri #828
Add lint to detect invalid cps uri #828
Conversation
Added //nolint:all to comment block to avoid golangci-lint to complain about duplicate words in comment
Fixed import block
Fine to me. Co-authored-by: Christopher Henderson <chris@chenderson.org>
As per Chris Henderson's suggestion, to "improve readability".
As per Chris Henderson's suggestion.
Added CABFEV_Sec9_2_8_Date
It seems like there is some leakage between this PR and #830... was that intentional? |
Thank you @cardonator, I have fixed that. |
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.
Thanks! This looks good to me now.
} | ||
|
||
func (l *invalidCPSUri) Execute(c *x509.Certificate) *lint.LintResult { | ||
// There should normally be just one CPS URI, but one never knows... |
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.
It may make sense to raise a warning if there is more than one but not outright fail 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.
Generally, we try to have lints only raise a single severity level. If we want, we could certainly create a second lint though.
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.
Mainly trying to reconcile between this and #815
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.
On the first point: it seems to me that the CABF BRs do not forbid including more than one CPS URI in a certificate, and although it's uncommon and "ugly" (to me) I can imagine some more or less valid reasons for doing that, so I do not think there is a reason for raising a warning if there is more than one CPS URI.
On the second point: my lint checks than a CPS URI is valid according to the CABF BRs (which applies to any kind of certificate in the TLS context, both EV and non EV), while PR #815 seems to be specific for EV certs.
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.
Agreed, thank you!
Please add this lint to check that any CPS URIs (hopefully just one) found in the certificates is a valid HTTP or HTTPS URL as per CABF BR 7.1.2 (several subsections thereof).