-
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
Lints for CABF SMIME BRs 7.1.2.3.f - EKUs #747
Conversation
Merging is blocked until this conversation is resolved: #744 (comment) EDIT: no need to block merge. Conversation has been moved to #748 |
// - Mailbox Validated Legacy | ||
// - Mailbox Validated Multipurpose | ||
// - Mailbox Validated Strict | ||
func (l *mailboxValidatedEnforceSubjectFieldRestrictions) CheckApplies(c *x509.Certificate) bool { | ||
return util.IsMailboxValidatedCertificate(c) | ||
return util.IsMailboxValidatedCertificate(c) && util.IsSubscriberCert(c) |
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.
To address #713 (comment) raised by @mtgag
@christopher-henderson, would you be able to give this a look over? I'm starting to work on some KU SMIME lints and it'd be easier if some of the util changes from this PR were on master. |
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.
@robplee indeed! I was holding off on reviewing until I thought you were sure that the PR was ready for review.
If you were ready for a review much earlier, then I ask that in the future you assign me as a reviewer or reach out when you think it will be ready.
@@ -25,3 +27,33 @@ func IsMailboxValidatedCertificate(c *x509.Certificate) bool { | |||
|
|||
return false | |||
} | |||
|
|||
func IsLegacySMIMECertificate(c *x509.Certificate) bool { |
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.
Thank you so much for adding these 🙏 it was sorely needed for many of the other CheckApplies
.
lint.RegisterCertificateLint(&lint.CertificateLint{ | ||
LintMetadata: lint.LintMetadata{ | ||
Name: "e_smime_legacy_multipurpose_eku_check", | ||
Description: "Strict/Multipurpose and Legacy: id-kp-emailProtection SHALL be present. Other values MAY be present. The values id-kp-serverAuth, id-kp-codeSigning, id-kp-timeStamping, and anyExtendedKeyUsage values SHALL NOT be present.", |
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.
In general, it's preferred if there is only one SHALL
clause in a given lint. This is primarily due to the scenario of encountering both errors at the same time, which typically results in one error shadowing the other. Both errors will eventually be addressed, although the second one will only show up after the first one is resolved.
That's not a blocker for this particular lint, I think, but just a consideration for others.
Aiming for all three of the EKU entries from #712