Zone11: different handling of root zone, TLD zones and zones in .arpa#1415
Conversation
What is the motivation not to mention that is should be version 1? I think it should be kept even though it does not have to be mentioned more than once and not in the tags.
I think it is more reasonable to check for a policy, and expect that it says "no mail" if present. If absent, an info message is reasonable for those kind of zones.
"Replace legacy Methods with the equivalent methods in MethodsV2." It is not just a mentioning, but an update of the test case that requires an updated implementation.
It should be reviewed, isn't that what we mean by testing an update of a document? |
|
In Objective is says "Sender Policy Framework (SPF), described in [RFC 7208], is a mechanism". I think it should say "defined in". |
d3477ec to
e235d87
Compare
|
I have changed the approach and addressed your review comments. Please re-review. |
| Servers"). | ||
|
|
||
| 4. For each name server in *Name Server IP* do: | ||
| 4. For each name server in *Name Servers* do: |
There was a problem hiding this comment.
This PR has chosen a different approach than #1409 for DNSSEC05. In the implementation the name server IP what counts. In the messages name/IP should be included. I think we should find a wording that we consistently use in all updates where name/IP are expected in the messages.
There was a problem hiding this comment.
Okay, I’ll reword as “For each name server IP address in Name Server IP do:”
There was a problem hiding this comment.
Well, there are some aspects of the approach in #1409. First there is the text in
zonemaster/docs/public/specifications/tests/DNSSEC-TP/dnssec05.md
Lines 115 to 118 in e8b95a6
zonemaster/docs/public/specifications/tests/DNSSEC-TP/dnssec05.md
Lines 175 to 178 in e8b95a6
I am not sure if the approach in #1409 is the best one. In the implementation you probably want to go by IP address, and should that be reflected in the specification?
There was a problem hiding this comment.
Oh, I didn’t notice those bits in the DNSSEC05 proposal.
Yes, it is a recurring pattern that we iterate on name server IPs under the hood but that we report name server name and IP pairs in user-visible messages. And we need to be careful with the edge case where a zone has more than one name server name pointing to the same IP address.
So I think the approach makes sense. Maybe we should keep it somewhere in a template?
There was a problem hiding this comment.
It is not only the edge case when a zone has serveral names to the same IP address, it is also the case when the name in the delegation does not match the name in the zone, but the IP is the same. The latter is probably more common.
We have a template in https://github.com/zonemaster/zonemaster/blob/master/docs/internal/templates/specifications/tests/Template01.md which probably needs some updates.
d5b7f29 to
ac1dbaa
Compare
Looks fine now. Maybe there should be clarification on "ns_list" to catch the situation when more than one NS name has the same IP address.
ac1dbaa to
1290ecd
Compare
matsduf
left a comment
There was a problem hiding this comment.
Looks fine. Do you plan to create a scenario PR too?
The overhaul of the test specification for Zone11 (see zonemaster#1415) is a good opportunity to add some test scenarios for that same test case. Of note here are the scenarios that perform tests on three different root zones. They will require three changes to the root hints during unit testing.
Yes, I was working on that in parallel but I was waiting for the outline of Zone11’s new test case specification to be stabilized before creating the PR. I’ve just created #1417. Then I’ll follow up with the implementation when the outline of that specification is stabilized. |
The overhaul of the test specification for Zone11 (see zonemaster#1415) is a good opportunity to add some test scenarios for that same test case. Of note here are the scenarios that perform tests on three different root zones. They will require three changes to the root hints during unit testing.
This commit implements a few miscellaneous minor wording changes. Firstly, shorten the msgid of Z11_DIFFERENT_SPF_POLICIES_FOUND in order to reduce redundant information. This tag is always emitted along with Z11_INCONSISTENT_SPF_POLICIES. Secondly, remove explicit mentions to “version 1” of SPF except for the very first occurrence. There is currently no reason to make any distinctions between versions of SPF, because there is currently only one version. And despite the current RFC, RFC 7208, being an update to RFC 4408, SPF stayed at version 1 ever since. This commit alters both the specification text and the message tags.
The root zone, TLDs and zones in .arpa are not expected to be domains appropriate for receiving e-mail. They should not be appropriate as sender e-mail addresses either. In the current wording of the specification, a NOTICE (Z11_NO_SPF_FOUND) is output telling the user that no SPF record was found, when running Zonemaster on such zones. That message tends to stick out like a sore thumb, especially on well-configured TLD zones. That NOTICE should not be there. The specification is amended so that it outputs specific message tags when the test is being run on such a zone: if there is no SPF record or it is a null SPF policy, then we output a special message tag of level INFO, and if it is a non-null SPF policy, it’s another message of a higher level. In doing so, this has the effect of squelching the NOTICE (Z11_NO_SPF_FOUND) that users have been complaining about when testing root, TLDs and zones inside .arpa.
Since we’ve overhauling Zone11, we might as well do some more maintenance work on the specification.
Amend the message tags and the specification in order to be able to report the (name, IP) pairs instead of the IP addresses. This is a breaking change for the following message tags and will require a database migration script: * Z11_DIFFERENT_SPF_POLICIES_FOUND * Z11_SPF_MULTIPLE_RECORDS * Z11_SPF_SYNTAX_ERROR
Given our definition of what an ERROR is, it is inappropriate to report Z11_SPF_MULTIPLE_RECORDS, Z11_SYNTAX_ERROR and Z11_UNABLE_TO_CHECK_FOR_SPF as errors. These tags do not represent conditions that prevent correct operation for the zone, but at best, they are conditions that can cause problems in degraded scenarios. We have to degrade the messages with ERROR level to WARNING level. See also: zonemaster/zonemaster-engine#1471.
7014b2f to
a431554
Compare
matsduf
left a comment
There was a problem hiding this comment.
The test case has "ns_list" arguments for some tags, and then there is a chance that the same IP address is used by multiple name server names. It should be stated explicitly that "ns_list" includes all such name/IP pairs.
dc37361 to
7c620ec
Compare
|
Step 7 in the test procedure already states:
But I found a paragraph in the “Summary” section that was no longer true. I’ve updated it with a similar text. Does this make step 7 in the test procedure redundant? |
tgreenx
left a comment
There was a problem hiding this comment.
LGTM.
Implementation in zonemaster/zonemaster-engine#1477.
The overhaul of the test specification for Zone11 (see zonemaster#1415) is a good opportunity to add some test scenarios for that same test case. Of note here are the scenarios that perform tests on three different root zones. They will require three changes to the root hints during unit testing.
The overhaul of the test specification for Zone11 (see zonemaster#1415) is a good opportunity to add some test scenarios for that same test case. Of note here are the scenarios that perform tests on three different root zones. They will require three changes to the root hints during unit testing.
The overhaul of the test specification for Zone11 (see zonemaster#1415) is a good opportunity to add some test scenarios for that same test case. Of note here are the scenarios that perform tests on three different root zones. They will require three changes to the root hints during unit testing.
|
@marc-vanderwal, please merge. |
Purpose
This PR amends the specification for Zone11 so that different message tags of different severity levels are output if Zonemaster is run on the root zone, a TLD zone or a zone inside .arpa. It also cleans up some rough edges and migrates to MethodsV2.
Context
See #1256.
Changes
Breaking message tag changes
This PR changes the arguments to the following existing message tags:
This PR replaces the following message tags with new tags:
How to test this PR
Review.