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 to enforce SMIME BRs: 7.1.4.2.1 requirement for mailbox addr… #800

Merged
merged 13 commits into from
Mar 3, 2024

Conversation

toddgaunt-gs
Copy link
Contributor

Hello, I'm going to be attempting to continue the work Rob Lee had been doing and would like to contribute lints from GlobalSign to the zlint repository. We mostly have smime lints to contribute developed in the past year and my goal is to continue contributing them one at a time. This is my first contribution beyond a documentation update so let me know if there is anything missing.

Description:
All mailbox addresses appearing in subjectDN or dirName must be repeated in san:rfc822Name or san:otherName. This lint does its best to detect mailbox address values in the subjectDN or dirName and if any are detected ensures they are repeated.

…esses

All mailbox addresses appearing in subjectDN or dirName must be repeated
in san:rfc822Name or san:otherName. This lint does its best to detect
mailbox address values in the subjectDN or dirName and if any are
detected ensures they are repeated.
@toddgaunt-gs
Copy link
Contributor Author

toddgaunt-gs commented Feb 22, 2024

I missed the integration tests before opening the PR. I have run them for this lint and identified 6 expected errors:

> make integration INT_FLAGS="-lintSummary -fingerprintSummary -lintFilter='e_mailbox_address_shall_contain_an_rfc822_name'" PARALLELISM=24
...
summary of result type by certificate fingerprint:
3087f97b6cff020b5320e18d3e326074cbaa128142660f2debe4564ab1ab0179 	fatals: 0    errs: 1    warns: 0    infos: 0   
5f3fcccca91a7b39e8995f79c35cb5e604d4ee0487ea1a41993c84304c0a5c99 	fatals: 0    errs: 1    warns: 0    infos: 0   
63d23132c2511f33bb947f27c398bb824109ccf2d6a2037e3713fe9f7a43b15d 	fatals: 0    errs: 1    warns: 0    infos: 0   
b034fa1aa9e501dc14b43d43dfe2210de3e5551744494b55d5f0abd865c67efc 	fatals: 0    errs: 1    warns: 0    infos: 0   
c6ac841c78191101725ca7d5ed499be47c15ebeece7d74e6d095e2925e7bb404 	fatals: 0    errs: 1    warns: 0    infos: 0   
e4dbfc94e616ffb59904e394d9dcdd3ab55c26c5586440f37c058eecb907a344 	fatals: 0    errs: 1    warns: 0    infos: 0   


summary of result type by lint name:
e_mailbox_address_shall_contain_an_rfc822_name                   	fatals: 0    errs: 6    warns: 0    infos: 0   

    corpus_test.go:164: expected lint "e_mailbox_address_shall_contain_an_rfc822_name" to have result fatals: 0    errs: 0    warns: 0    infos: 0    got fatals: 0    errs: 6    warns: 0    infos: 0   
1 lint(s) failed--- FAIL: TestCorpus (71.55s)
FAIL
FAIL	github.com/zmap/zlint/v3/integration	71.555s
FAIL
make: *** [makefile:36: integration] Error 1

…l_contain_an_rfc822_name

The failures all have email addresses that don't have an **exact** match
in the SAN.

How the integration tests were run:
`make integration INT_FLAGS="-lintSummary -fingerprintSummary -lintFilter='e_mailbox_address_shall_contain_an_rfc822_name'"`

Fingerprints of the relevant certificates:
3087f97b6cff020b5320e18d3e326074cbaa128142660f2debe4564ab1ab0179
5f3fcccca91a7b39e8995f79c35cb5e604d4ee0487ea1a41993c84304c0a5c99
63d23132c2511f33bb947f27c398bb824109ccf2d6a2037e3713fe9f7a43b15d
b034fa1aa9e501dc14b43d43dfe2210de3e5551744494b55d5f0abd865c67efc
c6ac841c78191101725ca7d5ed499be47c15ebeece7d74e6d095e2925e7bb404
e4dbfc94e616ffb59904e394d9dcdd3ab55c26c5586440f37c058eecb907a344
…ess_shall_contain_an_rfc822_name"

This reverts commit 037b5ec.
…l_contain_an_rfc822_name

This commit is a proper version of the previously reverted one. It was
reverted because I accidently ran the script to update the config only
for the failing lint, rather than lints.

The failures all have email addresses that don't have an **exact** match
in the SAN.

How the integration tests were run:
`make integration INT_FLAGS="-lintSummary -fingerprintSummary -lintFilter='e_mailbox_address_shall_contain_an_rfc822_name'"`

Fingerprints of the relevant certificates:
3087f97b6cff020b5320e18d3e326074cbaa128142660f2debe4564ab1ab0179
5f3fcccca91a7b39e8995f79c35cb5e604d4ee0487ea1a41993c84304c0a5c99
63d23132c2511f33bb947f27c398bb824109ccf2d6a2037e3713fe9f7a43b15d
b034fa1aa9e501dc14b43d43dfe2210de3e5551744494b55d5f0abd865c67efc
c6ac841c78191101725ca7d5ed499be47c15ebeece7d74e6d095e2925e7bb404
e4dbfc94e616ffb59904e394d9dcdd3ab55c26c5586440f37c058eecb907a344
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.

Thank you for joining in on the contributions on behalf of GlobalSign! Help from the CAs is always appreciated.

I took a quick smoke check on the failed certificates, and their failures seem to make sense.

https://crt.sh/?id=36746425&opt=x509lint,cablint
https://crt.sh/?id=2369907900&opt=cablint,x509lint
https://crt.sh/?id=2380565472&opt=cablint,x509lint
https://crt.sh/?id=2369908278&opt=cablint,x509lint
https://crt.sh/?id=2370069568&opt=cablint,x509lint
https://crt.sh/?id=2370050635&opt=cablint,x509lint

If you happen to ever have any feedback on the workflow and tooling in the repository then I would love to hear them! Writing lints can be redundant, but also error prone. So I try to make it as low friction as reasonably possible for contributors to hop right in.

v3/util/san.go Outdated Show resolved Hide resolved
v3/lints/cabf_smime_br/mailbox_address_from_san.go Outdated Show resolved Hide resolved
v3/lints/cabf_smime_br/mailbox_address_from_san.go Outdated Show resolved Hide resolved
@toddgaunt-gs
Copy link
Contributor Author

Thank you for joining in on the contributions on behalf of GlobalSign! Help from the CAs is always appreciated.

I took a quick smoke check on the failed certificates, and their failures seem to make sense.

https://crt.sh/?id=36746425&opt=x509lint,cablint https://crt.sh/?id=2369907900&opt=cablint,x509lint https://crt.sh/?id=2380565472&opt=cablint,x509lint https://crt.sh/?id=2369908278&opt=cablint,x509lint https://crt.sh/?id=2370069568&opt=cablint,x509lint https://crt.sh/?id=2370050635&opt=cablint,x509lint

If you happen to ever have any feedback on the workflow and tooling in the repository then I would love to hear them! Writing lints can be redundant, but also error prone. So I try to make it as low friction as reasonably possible for contributors to hop right in.

The main thing I can think of is improving genTestCerts to add support for some unsupported fields at the moment, and potentially a way to save certificate generation configs but that may be wandering uncomfortably close to re-implementing openssl. Otherwise the tooling is very good once I figured it out, and I found it quite easy to fix my mistakes based on the github checks

@christopher-henderson
Copy link
Member

@bitlux

The main thing I can think of is improving genTestCerts to add support for some unsupported fields at the moment

This is indeed quite difficult. We rely on the ZMap's fork of x509 in order to do some custom, filthy, parsing that helps lints out so that may be an avenue. However, that fork has not been merged with its upstream in...like...7 years. I shutter to think what that could possibly look like.

and potentially a way to save certificate generation configs

I was actually thinking on this last week because I was having trouble generating a cert that even applied for an SMIME lint (I always forget which magical fields and which magical before/after make it all work). So perhaps at least a template or two would help.

@christopher-henderson christopher-henderson merged commit b9ff71f into zmap:master Mar 3, 2024
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.

None yet

3 participants