Skip to content

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

Merged
christopher-henderson merged 13 commits intozmap:masterfrom
robplee:mailbox_validated_fields_check
Aug 13, 2023
Merged

Add lint enforcing the restrictions on subject DN fields for mailbox validated SMIME certificates#713
christopher-henderson merged 13 commits intozmap:masterfrom
robplee:mailbox_validated_fields_check

Conversation

@robplee
Copy link
Copy Markdown
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
Copy Markdown
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
if l.Source == CABFBaselineRequirements && !util.IsServerAuthCert(cert) {
return &LintResult{Status: NA}
}
if l.Source == CABFSMIMEBaselineRequirements && !util.IsEmailProtectionCert(cert) {
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.

Opening salvo!

robplee and others added 3 commits May 15, 2023 13:57
@christopher-henderson
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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