search

zmap / zlint

X.509 Certificate Linter focused on Web PKI standards and requirements.
https://zmap.io
Apache License 2.0
347 stars 107 forks source link

Update single email subject if present #802

Closed mtgag closed 3 months ago

mtgag commented 4 months ago

This PR addresses an issue with lint_single_email_if_present.go. The lint takes a look into the SAN entries to check if there is an entry with two email addresses. The description and citation shows to the entries in the subjectDN. On the one hand taking a look into the SAN entries for two email addresses is meaningful, on the other hand the citation and implementation do not align. Therefore this PR adds a new lint to check the subjectDN entries, re-using the implementation. I believe following actions can be further taken. One is to keep the original lint and change the citation/description. A second is to incorporate the two implementations into one lint that checks both the subjectDN and the SAN entries.

Best, Vangelis

christopher-henderson commented 4 months ago

Ah, I see the issue, thank you for catching that @mtgag.

I think that my take is that the later is preferred...

A second is to incorporate the two implementations into one lint that checks both the subjectDN and the SAN entries.

We've done this before in the repo wherein a lint checks additional, related, structures as a part of one unified lint. I believe that that this helps keep citation implementations near each other as otherwise it would be easy to forget to update one if the implementation were split.

cardonator commented 4 months ago

I agree that an additional lint is warranted here. I believe this will close #795 as well. Thanks for tackling that, @mtgag

mtgag commented 4 months ago

I agree that an additional lint is warranted here. I believe this will close #795 as well. Thanks for tackling that, @mtgag

Should I incorporate lint_single_email_if_present (and then delete it) into lint_single_subject_email_if_present, or keep both and slightly refactor it to reflect the discussion here and comments from #795?

cardonator commented 4 months ago

My suggestion would be the latter.

mtgag commented 4 months ago

My suggestion would be the latter.

I will open a new PR for this. Then we have a stable pre-review and pre-merge state in lint_single_email_if_present addressing https://github.com/zmap/zlint/issues/795. We can still decide whether to go for one or two lints. I see benefits in both approaches.

UPDATE: PR https://github.com/zmap/zlint/pull/808