Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Include support for exists element in SPF. #356

Merged
merged 2 commits into from
May 3, 2018
Merged

Include support for exists element in SPF. #356

merged 2 commits into from
May 3, 2018

Conversation

kjacobsen
Copy link
Contributor

Hi,

Related to issues #355, I suspect it would only take a simple change of the handling for the exists: element to work, however I could be very wrong (and totally happy for you to tell me so).

@kjacobsen
Copy link
Contributor Author

What's the best way to test these changes?

@captncraig
Copy link
Contributor

Awesome to hear that somebody is using this kinda fringe feature!

I had to dig into the spec on this one. exists is a part of spf I was hoping wasn't widely used. Apparently I was wrong.

Apparently exists:%{i}._spf.sparkpostmail.com means "look for an A record at 1.2.3.4._spf.sparkpostmail.com" where the IP is the sender IP. Not sure exactly what cases this would be preferable to just listing ip4 records (maybe to not publish IPs so easily?), but seems potentially useful.

So my question is: are there any situations where a macro will expand to cause multiple lookups, or will it always be one? Reading the spec I suspect not, but my eyes kinda glazed over after a while.

If that is the case, I have no reason not to include this. Thank you!

@captncraig
Copy link
Contributor

With regards to testing, the tests in that package are not very robust. You could probably add an exists clause to the test here I'd love to hear ideas if you have a good way to test all of this better. But I wouldn't block this for that.

@kjacobsen
Copy link
Contributor Author

Thanks for the fast response.

My hope is that the SPF support simplifies the management of our SPF records in the long run.

From my reading of the RFC, I can't see a situation where the expansion would cause multiple lookups. Section 5.7 says

The domain-spec is expanded as per Section 8. The resulting domain name is used for a DNS A RR lookup. If any A record is returned, this mechanism matches. The lookup type is A even when the connection type is IPv6.

In terms of testing, we could include an entry using exists: to test if the code correctly produces the SPF we are expecting.

@tlimoncelli
Copy link
Contributor

The change looks good. Please add an "exists:" example to dnscontrol/pkg/spflib/parse_test.go then we'll merge the PR.

@kjacobsen
Copy link
Contributor Author

Forgot to let you know that I have added the example into the tests.

@tlimoncelli tlimoncelli merged commit f77f202 into StackExchange:master May 3, 2018
rblenkinsopp pushed a commit to rblenkinsopp/dnscontrol that referenced this pull request Aug 21, 2020
* Include support for "exists" element in SPF.
* Add exists: SPF entry to test support
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants