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

dmarc/dkim entries for the professionalmail product #568

Merged
merged 12 commits into from
Nov 9, 2024

Conversation

thoag-godaddy
Copy link
Contributor

No description provided.

Copy link

github-actions bot commented Nov 5, 2024

Linter OK:

Linter result for godaddy.com.professional_mail.json
Linter result for secureserver.net.professional_mail.json

@thoag-godaddy thoag-godaddy marked this pull request as ready for review November 7, 2024 15:13
Copy link
Member

@pawel-kow pawel-kow left a comment

Choose a reason for hiding this comment

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

These templates generate invalid records (DKIM).
Please test the templates with Online Editor and submit corrections.

@thoag-godaddy
Copy link
Contributor Author

I also see the error in the editor but I don't agree with its output. the error is:
Error! A problem has been occurred while submitting your data: Invalid data for CNAME host: secureserver_s1._domainkey (from secureserver_s1._domainkey)

Some testing seems to imply that it is rejecting the presence of an underscore within the label - if I change secureserver_s1 to secureservers1 then it accepts it. Also it accepts _secureservers1. It should be valid to have an underscore within the label. observe
dig cname secureserver_s2._domainkey.tyrawr-externaldkim.co

@pawel-kow
Copy link
Member

Some testing seems to imply that it is rejecting the presence of an underscore within the label - if I change secureserver_s1 to secureservers1 then it accepts it. Also it accepts _secureservers1. It should be valid to have an underscore within the label. observe dig cname secureserver_s2._domainkey.tyrawr-externaldkim.co

Well, as per rfc1035:

<label> ::= <letter> [ [ <ldh-str> ] <let-dig> ]
<ldh-str> ::= <let-dig-hyp> | <let-dig-hyp> <ldh-str>
<let-dig-hyp> ::= <let-dig> | "-"
<let-dig> ::= <letter> | <digit>

In theory rfc2181 stipulates about any character being allowed in DNS as protocol, however it's questionable whether underscore in the label can be safely assumed to be supported between DNS operators.

Further RFCs, as per rfc8552 only seem to use underscore as a prefix label.

Finally rfc6376 (DKIM) actually specifies

selector =   sub-domain *( "." sub-domain )

sub-domain is imported from rfc5321:

sub-domain     = Let-dig [Ldh-str]
Let-dig        = ALPHA / DIGIT
Ldh-str        = *( ALPHA / DIGIT / "-" ) Let-dig

So I would conclude that using underscore in DKIM selector is not allowed and therefore the tool makes a good job complaining about it in this case.

I would also claim that using underscore generally in labels other than prefix would not be supported by many DNS providers making such template less interoperable, therefore I would keep applying this rule also outside of DKIM context.

@thoag-godaddy
Copy link
Contributor Author

Thanks for providing a good write up on the issue. After discussing with my peers we have decided it will be better for the selector to be a variable anyway. I was able to use your tool to test the template.

@kerolasa
Copy link
Collaborator

kerolasa commented Nov 8, 2024

Side note, there is a pull request to validator that aims to catch problematic underscores.

Domain-Connect/dc-template-linter#14

@pawel-kow
Copy link
Member

Hi, OK @thoag-godaddy.
Now after I actually fixed the checking script, it reported that the version bump would be needed.

@thoag-godaddy
Copy link
Contributor Author

version updated. thanks for all your help on this.

@pawel-kow pawel-kow added this pull request to the merge queue Nov 9, 2024
Merged via the queue into Domain-Connect:master with commit 2ad32ac Nov 9, 2024
2 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.

3 participants