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

Update single email subject if present #802

Merged

Conversation

mtgag
Copy link
Contributor

@mtgag mtgag commented Feb 25, 2024

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
Copy link
Member

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
Copy link
Contributor

cardonator commented Feb 25, 2024

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

@mtgag
Copy link
Contributor Author

mtgag commented Feb 29, 2024

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
Copy link
Contributor

My suggestion would be the latter.

@mtgag
Copy link
Contributor Author

mtgag commented Mar 1, 2024

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 #795. We can still decide whether to go for one or two lints. I see benefits in both approaches.

UPDATE: PR #808

mtgag added a commit to mtgag/zlint that referenced this pull request Mar 1, 2024
@christopher-henderson christopher-henderson merged commit e33bae9 into zmap:master Mar 17, 2024
4 checks passed
christopher-henderson added a commit that referenced this pull request Mar 17, 2024
* lint about the encoding of qcstatements for PSD2

* Revert "lint about the encoding of qcstatements for PSD2"

This reverts commit 6c23670.

* util: gtld_map autopull updates for 2021-10-21T07:25:20 UTC

* always check and perform the operation in the execution

* synchronised with project

* synchronised with project

* synchronised with project

* synchronised with project

* added same lint for subject values instead of SAN values

* resolved conflict issue

* addressed review comments and hint to citation from #795

* addressing issue #795 and review comments of PR #802

---------

Co-authored-by: mtg <git@mtg.de>
Co-authored-by: GitHub <noreply@github.com>
Co-authored-by: Christopher Henderson <chris@chenderson.org>
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

6 participants