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

Limit scope for cn checking in SAN #825

Merged
merged 31 commits into from
Apr 7, 2024

Conversation

mtgag
Copy link
Contributor

@mtgag mtgag commented Apr 5, 2024

This PR addresses issues of discussion in PR #809

for _, commonName := range name.CommonNames {
if util.IsMailboxAddress(commonName) {
mailboxAddresses = append(mailboxAddresses, commonName)
if includeCN {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if includeCN {
if len(name.EmailAddress) == 0 {

This would cover more scenario's and doesn't require additional function parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would not that, probably very unlikely, produce some false positives? E.g. for a sponsor-validated certificate with a subjectDN featuring a CN which has a pseudonym with a @ character and no emailAddress RDN in subject.

Copy link
Contributor

Choose a reason for hiding this comment

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

That might be possible indeed.

@christopher-henderson
Copy link
Member

Howdy @mtgag and @vanbroup, I'm just catching up on this. We cut a release candidate last week, but this seems like it would be a good call for an RC2, no?

@vanbroup
Copy link
Contributor

vanbroup commented Apr 6, 2024

Agreed

@mtgag
Copy link
Contributor Author

mtgag commented Apr 7, 2024

Howdy @mtgag and @vanbroup, I'm just catching up on this. We cut a release candidate last week, but this seems like it would be a good call for an RC2, no?

Yes, it looks much better now after the comments. Thanks for this.

@christopher-henderson christopher-henderson merged commit 308a138 into zmap:master Apr 7, 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.

4 participants