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

Add lint enforcing the restrictions on subject DN fields for mailbox validated SMIME certificates #713

Merged

Conversation

robplee
Copy link
Contributor

@robplee robplee commented May 2, 2023

This PR is the first to attempt to address something from the big todo list in #712. It's a simple lint enforcing the field MAY/SHALL NOT table for subject DN fields in mailbox validated certificates. I think the changes are fairly straightforward so we should be able to discover if there are any extra things needed for us to start supporting SMIME BR linting.

My current wonder is about the TestCorpus for integration testing as I assume it won't have any applicable certs in it but I guess that's something we can try to work out now?

Copy link
Contributor

@aaomidi aaomidi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me! But my approval doesn't carry much weight either :)

@christopher-henderson christopher-henderson self-requested a review May 14, 2023 13:54
v3/util/ca.go Show resolved Hide resolved
@@ -221,6 +221,9 @@ func (l *CertificateLint) Execute(cert *x509.Certificate, config Configuration)
if l.Source == CABFBaselineRequirements && !util.IsServerAuthCert(cert) {
return &LintResult{Status: NA}
}
if l.Source == CABFSMIMEBaselineRequirements && !util.IsEmailProtectionCert(cert) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opening salvo!

robplee and others added 3 commits May 15, 2023 13:57
@christopher-henderson
Copy link
Member

My current wonder is about the TestCorpus for integration testing as I assume it won't have any applicable certs in it but I guess that's something we can try to work out now?

@robplee I'll take on this investigation as it's an interesting question. The test corpus from a query against the censys.io database, so I am as yet unsure of whether-or-not these certificates will be available to us. My guess would be no since it's my understanding that Censys get its certificate database by crawling the open internet.

@cardonator
Copy link
Contributor

I believe this PR sets some basic precedents and expectations required to start plowing through #712, so what else needs to be done here do you think? Does this need further review or just testing against a large corpus of SMIME certs? I could try to help run against an internal corpus here to at least get some initial testing results if that's needed before we feel comfortable merging this PR.

@christopher-henderson
Copy link
Member

@cardonator

Does this need further review or just testing against a large corpus of SMIME certs?

I have actually been pecking away at this over the past several weeks. We got a new batch of certs 100k+ large from Censys, but it's been a lot of work to format the corpus, deduplicate, sanitize, and smoke that the new failures make sense.

...before we feel comfortable merging this PR.

In the interest of time I think that we would be fine merging this now. If things go truly south in the immediate absence of more test certs then it will be easy to soft "back out" changes by simply not running SMIME checks,

@christopher-henderson
Copy link
Member

@cardonator

Does this need further review or just testing against a large corpus of SMIME certs?

I have actually been pecking away at this over the past several weeks. We got a new batch of certs 100k+ large from Censys, but it's been a lot of work to format the corpus, deduplicate, sanitize, and smoke that the new failures make sense.

...before we feel comfortable merging this PR.

In the interest of time I think that we would be fine merging this now. If things go truly south in the immediate absence of more test certs then it will be easy to soft "back out" changes by simply not running SMIME checks,

@mtgag
Copy link
Contributor

mtgag commented Aug 30, 2023

Would it be meaningful to change

https://github.com/zmap/zlint/blob/master/v3/lints/cabf_smime_br/mailbox_validated_enforce_subject_field_restrictions.go#L80

return util.IsMailboxValidatedCertificate(c)

to:

return util.IsMailboxValidatedCertificate(c) && util.IsSubscriberCert(c)

because this is under Section 7.1.4.2 which regards subscriber certificates?

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.

5 participants