Skip to content
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

Merged
merged 7 commits into from
Oct 22, 2023

Conversation

robplee
Copy link
Contributor

@robplee robplee commented Oct 4, 2023

Aiming for all three of the EKU entries from #712

@robplee robplee changed the title Eku smime lints Lints for CABF SMIME BRs 7.1.2.3.f - EKUs Oct 4, 2023
@robplee
Copy link
Contributor Author

robplee commented Oct 5, 2023

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)
Copy link
Contributor Author

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

@robplee
Copy link
Contributor Author

robplee commented Oct 19, 2023

@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.

Copy link
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.

@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 {

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.",

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.

@christopher-henderson christopher-henderson merged commit 1c307f4 into zmap:master Oct 22, 2023
4 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.

2 participants