Skip to content

Add specifications for testing SPF and DMARC policies#1100

Closed
marc-vanderwal wants to merge 3 commits into
zonemaster:developfrom
marc-vanderwal:feature/spf-dmarc
Closed

Add specifications for testing SPF and DMARC policies#1100
marc-vanderwal wants to merge 3 commits into
zonemaster:developfrom
marc-vanderwal:feature/spf-dmarc

Conversation

@marc-vanderwal

@marc-vanderwal marc-vanderwal commented Sep 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Purpose

This PR introduces three test cases for testing SPF and DMARC policies for the domain under test.

Context

Fixes #995.

Changes

Added three test cases:

  • checking SPF for compliance with respect to RFC 7208;
  • checking for non-use of the deprecated SPF resource record type;
  • checking DMARC for compliance with respect to RFC 7489.

How to test this PR

This is a documentation update. However #1099 should be merged first.

@marc-vanderwal marc-vanderwal self-assigned this Sep 28, 2022
@marc-vanderwal marc-vanderwal added T-Feature Type: New feature in software or test case description A-TestCase Area: Test case specification or implementation of test case A-Documentation Area: Documentation only. labels Sep 28, 2022
@marc-vanderwal marc-vanderwal linked an issue Sep 28, 2022 that may be closed by this pull request
Add SPF and possibly other mail related test cases [Feature Request] #995
Closed
matsduf
matsduf requested changes Sep 28, 2022
View reviewed changes

@matsduf matsduf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The proposals do not follow the specification template. Could you rewrite them? You could also check the active PRs for specification updates, e.g. #1098 and #1032.

@marc-vanderwal

marc-vanderwal commented Sep 28, 2022

Copy link
Copy Markdown
Contributor Author

Sure, I will.

I was also unsure about where to add these new test cases. I’ve added them in Zone because that’s where the tests involving MX records are, but they could be in a new category (maybe Email).

@matsduf

matsduf commented Sep 28, 2022

Copy link
Copy Markdown
Contributor

I was also unsure about where to add these new test cases. I’ve added them in Zone because that’s where the tests involving MX records are, but they could be in a new category (maybe Email).

I think Zone is fine. I am not sure that adding a new test level would make it better. Before adding a new level I think we should have a broader discussion about which levels to have and what they should contain.

@marc-vanderwal

marc-vanderwal commented Sep 28, 2022

Copy link
Copy Markdown
Contributor Author

Okay, sticking with Zone, then.

@marc-vanderwal

marc-vanderwal commented Sep 29, 2022

Copy link
Copy Markdown
Contributor Author

The proposals do not follow the specification template. Could you rewrite them? You could also check the active PRs for specification updates, e.g. #1098 and #1032.

Done.

matsduf
matsduf requested changes Sep 29, 2022
View reviewed changes
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
@marc-vanderwal

marc-vanderwal commented Oct 3, 2022

Copy link
Copy Markdown
Contributor Author

I’ve addressed some of your remarks in this iteration, but I haven’t updated the other test cases or acted on the remaining remarks yet. Also, step 14 is probably wrong because the real limits are described in RFC 7208 § 4.6.4 and it’s slightly more complicated than I thought. I’ll rephrase that step later.

matsduf
matsduf reviewed Oct 3, 2022
View reviewed changes
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
@marc-vanderwal marc-vanderwal force-pushed the feature/spf-dmarc branch 2 times, most recently from 591a2dd to 4f856a9 Compare October 4, 2022 14:13
@marc-vanderwal

marc-vanderwal commented Oct 4, 2022

Copy link
Copy Markdown
Contributor Author

Overhauled Zone11. Rewrites of Zone12 and Zone13 will come later.

matsduf
matsduf requested changes Oct 5, 2022
View reviewed changes

@matsduf matsduf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have only reviewed Zone11.

Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md
Comment thread docs/specifications/tests/Zone-TP/zone11.md
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
@marc-vanderwal marc-vanderwal mentioned this pull request Oct 6, 2022
Merged
matsduf
matsduf reviewed Oct 24, 2022
View reviewed changes
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
matsduf
matsduf reviewed Oct 26, 2022
View reviewed changes
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
@matsduf

matsduf commented Oct 26, 2022

Copy link
Copy Markdown
Contributor

Validation of an SPF policy according to the RFC is quite complex!

@marc-vanderwal

marc-vanderwal commented Oct 28, 2022

Copy link
Copy Markdown
Contributor Author

Yes, validating an SPF policy is indeed a chore (and a tool like Zonemaster cannot necessarily check everything). But I’m doing some research on the use of SPF in the .fr zone and as far as I can see, I’m pretty confident that this test case will catch most errors people make when publishing SPF policies.

@marc-vanderwal marc-vanderwal force-pushed the feature/spf-dmarc branch 2 times, most recently from 5ac1a60 to 6e5ca27 Compare October 28, 2022 15:59
@marc-vanderwal

marc-vanderwal commented Oct 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I think I can simplify this test case quite a bit.

People very rarely make mistakes in the SPF version number. Less than 0,1% of domains in the .fr zone, which our crawler has classified as “not parked”, have TXT records starting with v=spf followed by something else than 1. So such mistakes are not worth testing for.

@marc-vanderwal marc-vanderwal force-pushed the feature/spf-dmarc branch 2 times, most recently from 0e0c8fb to e0b1c00 Compare November 2, 2022 10:22
matsduf
matsduf reviewed Nov 7, 2022
View reviewed changes
Comment thread docs/specifications/tests/Zone-TP/zone11.md Outdated
@matsduf

matsduf commented Nov 7, 2022

Copy link
Copy Markdown
Contributor

People very rarely make mistakes in the SPF version number. Less than 0,1% of domains in the .fr zone, which our crawler has classified as “not parked”, have TXT records starting with v=spf followed by something else than 1. So such mistakes are not worth testing for.

If I understand it correctly, Zone11 will consider such TXT records as non-SPF records. It is hard to say how much errors to look for. There is always a limit.

@matsduf

matsduf commented Nov 7, 2022

Copy link
Copy Markdown
Contributor

This PR is a draft. Are Zone12 and Zone13 ready for review?

@marc-vanderwal marc-vanderwal marked this pull request as ready for review November 7, 2022 10:45
@marc-vanderwal

marc-vanderwal commented Nov 7, 2022

Copy link
Copy Markdown
Contributor Author

If I understand it correctly, Zone11 will consider such TXT records as non-SPF records. It is hard to say how much errors to look for. There is always a limit.

That’s right, Zone11 will output Z11_NO_SPF_FOUND.

However it is interesting, I think, to look for TXT records that do contain v=spf1 somewhere in their text. I’ve seen instances of mistakes such as:

  • including quotation marks around the SPF policy (which dig would display as … IN TXT "\"v=spf1 …\"");
  • copy-pasting the entire resource record in the data field (e.g example. 3600 IN TXT "example. 3600 IN TXT \"v=spf1 …\"").

I haven’t quantified this phenomenon yet but maybe it’s something to do in a separate test case.

This PR is a draft. Are Zone12 and Zone13 ready for review?

I’ve rewritten Zone12 and Zone13 as well based on your review of Zone11, so yes, they are. I even think that this specification is stable/mature enough for me to mark this PR as ready for review.

@marc-vanderwal
Add test case specification for SPF
ec79ac8 
Add a test case for SPF records in the domain under test.

The test case ascertains whether any SPF records exist for the zone, and
if so, whether they are valid.

In short, this test case will:

 * check for either zero or one TXT record containing an SPF
   version 1 policy;

 * check the SPF policy for valid syntax;

 * ensure that evaluating the policy will never require more than 10 DNS
   lookups.
@marc-vanderwal
Add test case for deprecated SPF RR types
4dee671 
Add a test case specification for a test checking that the deprecated
SPF resource record type isn’t used on the domain being tested.

The rationale for the test is detailed more clearly in the “Objective”
section.
@marc-vanderwal
Add test case specification for DMARC
30d85e3 
Add a specification for a test case validating a DMARC policy, if one
exists.

The idea is similar to that of the test case for SPF:

 * first, attempt to find a DMARC policy for either the domain under
   test or its organizational domain (which is more or less the
   second-level domain except in some cases such as `co.uk`).

 * then, if a DMARC policy is found, check its syntax.

The test case also checks if the e-mail addresses used for reporting
failures or aggregate data are in the same domain as the policy. If not,
Zonemaster shall use the procedure documented in RFC 7489 to verify that
the subdomain for the report e-mail addresses permits the domain under
test to send its reports there.
@matsduf

matsduf commented Nov 7, 2022

Copy link
Copy Markdown
Contributor

However it is interesting, I think, to look for TXT records that do contain v=spf1 somewhere in their text. I’ve seen instances of mistakes such as:

* including quotation marks around the SPF policy (which `dig` would display as `… IN TXT "\"v=spf1 …\""`);

* copy-pasting the entire resource record in the data field (e.g `example. 3600 IN TXT "example. 3600 IN TXT \"v=spf1 …\""`).

I haven’t quantified this phenomenon yet but maybe it’s something to do in a separate test case.

I think it fits into this test case instead of a separate one. The cases are maybe rare, but Zonemaster should also look for rare cases such as responding to queries for SOA but not MX. At least I think it should be simple to create test zones with the odd data in a TXT record. I think that looking for a TXT record that contains v=spf but not being valid SPF would be reasonable. It might not being ERROR, but NOTICE or WARNING.

I’ve rewritten Zone12 and Zone13 as well based on your review of Zone11, so yes, they are. I even think that this specification is stable/mature enough for me to mark this PR as ready for review.

OK, I will dig into Zone12 and Zone13 then.

@matsduf matsduf added this to the v2023.1 milestone Dec 17, 2022
@marc-vanderwal

marc-vanderwal commented Apr 6, 2023

Copy link
Copy Markdown
Contributor Author

After discussion, I’m going to close this PR in order to split it up into smaller ones.

I also finished conducting some preliminary research on the use of SPF, DKIM and DMARC on the .fr zone and published some results on (Afnic’s blog)[https://www.afnic.fr/en/observatory-and-resources/expert-papers/spf-dkim-and-dmarc-whats-the-situation-with-the-fr-tld/]. Erroneous SPF and DMARC policies are very rare, and I don’t think it’s worth giving detailed feedback on possible errors in SPF and DMARC records unless there is sufficient demand for it, which seems unlikely to me.

Based on that research, I’m therefore going to create new specifications that just focus on checking for SPF, DKIM and DMARC on the zone under test and maybe just conduct a basic syntax check on SPF and DMARC policies (I’ve actually used Mail::SPF and Mail::DMARC to do just that during my research, so that’s going to be an easy thing to implement), raising warnings if there’s an obvious error. All of these will go in separate PRs.

marc-vanderwal added a commit to marc-vanderwal/zonemaster that referenced this pull request Apr 6, 2023
@marc-vanderwal
Add test case specification for SPF
ae03c54 
Take the draft specification in the state it was in for pull request
\zonemaster#1100 before that PR was closed.
@marc-vanderwal marc-vanderwal mentioned this pull request Apr 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matsduf matsduf matsduf requested changes

Assignees

@marc-vanderwal marc-vanderwal

Labels

A-Documentation Area: Documentation only. A-TestCase Area: Test case specification or implementation of test case T-Feature Type: New feature in software or test case description

Projects

None yet

Milestone

v2023.1

Development

Successfully merging this pull request may close these issues.

Add SPF and possibly other mail related test cases [Feature Request]

2 participants

@marc-vanderwal @matsduf