Skip to content

Lints for CABF SMIME BRs 7.1.2.3.f - EKUs#747

Merged
christopher-henderson merged 7 commits intozmap:masterfrom
robplee:eku_smime_lints
Oct 22, 2023
Merged

Lints for CABF SMIME BRs 7.1.2.3.f - EKUs#747
christopher-henderson merged 7 commits intozmap:masterfrom
robplee:eku_smime_lints

Conversation

@robplee
Copy link
Copy Markdown
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
Copy Markdown
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 Strict
func (l *mailboxValidatedEnforceSubjectFieldRestrictions) CheckApplies(c *x509.Certificate) bool {
return util.IsMailboxValidatedCertificate(c)
return util.IsMailboxValidatedCertificate(c) && util.IsSubscriberCert(c)
Copy link
Copy Markdown
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
Copy Markdown
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
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.

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

Comment thread v3/util/smime_policies.go
return false
}

func IsLegacySMIMECertificate(c *x509.Certificate) bool {
Copy link
Copy Markdown
Member

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.",
Copy link
Copy Markdown
Member

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.

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