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

Rewrite of the specification of DNSSEC10 #1179

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

matsduf
Copy link
Contributor

@matsduf matsduf commented Jul 16, 2023

Purpose

There are several issues concerning problems with both the specification and implementation if DNSSEC10:

The old version of the test case tries to test NSEC/NSEC3 by creating a non-existing domain name, which is quite complex when the zone has a wildcard. NSEC3 opt out increases the complexity.

This rewrite changes the way NSEC/NSEC3 is tested. NSEC/NSEC3 records have two purposes in DNSSEC:

  • Providing proof that a certain name does not exist.
  • If the name exists, providing proof that a certain record type does not exist in node of that name.

The current version focuses on non-exiting domain names. The new version provided in this PR focuses instead on record type that do not exist, and it queries for record types that cannot exist in the zone. This will make the implementation less complex.

Context

The issues above.

Test zones will be added to this PR, but the specification is complete and ready for review.

Changes

The rewrite of the specification requires a rewrite of the implementation.

How to test this PR

This has to be reviewed.

@matsduf matsduf added the A-TestCase Area: Test case specification or implementation of test case label Jul 16, 2023
@matsduf matsduf added this to the v2023.2 milestone Jul 16, 2023
Copy link
Contributor

@tgreenx tgreenx left a comment

Choose a reason for hiding this comment

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

I don't have much to say in the test procedure of this specification. The logic looks good to me as is.

docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
DS10_NSEC_QUERY_GIVES_ERR_ANSWER | ERROR | ns_ip_list | Unexpected DNS record in the answer section on an NSEC query. Fetched from nameservers with IP addresses "{ns_ip_list}".
DS10_NSEC_QUERY_RESPONSE_ERROR | ERROR | ns_ip_list | No response or error in response on query for NSEC. Fetched from the nameservers with IP addresses "{ns_ip_list}".
DS10_NSEC_RRSIG_VERIFY_ERROR | ERROR | ns_ip_list | The signatures (RRSIG with tag {keytag}) for the NSEC record cannot be verified. Fetched from the nameservers with IP addresses "{ns_ip_list}".
DS10_SERVER_NO_DNSSEC_SUPPORT | ERROR | ns_ip_list | The name servers listed here do not support DNSSEC or has not properly configured. Testing for NSEC and NSEC3 has been skipped on these servers. Fetched from the nameservers with IP addresses "{ns_ip_list}".
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Suggestion: What about using ns_list instead here?

  • The name servers listed here do not support DNSSEC or has not properly configured. -> The following name servers do not support DNSSEC or it has not been properly configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MSIG updated.

I have changed to ns_list. Why here and not in every place?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. For some reason in the DNSSEC module ns_ip_list is used almost everywhere. I find ns_list often more appropriate, for example in such a generic message. I'm fine with having ns_list everywhere in this specification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, I think ns and ns_list to be more helpful in the output. They require a little more specification if we are strict. Usually we want to loop over the IP addresses, and then what should be reported if the there are two names to the same IP address? Just report one of them (kind of randomly)?

How much extra will the implementation require to output ns (ns_list) instead of ns_ip (ns_ip_list)?

One decision could be to say that in the specification the name server name could be implicit except for that in the msgid it must be spelled out if it should be ns (ns_list) or ns_ip (ns_ip_list).

Copy link
Contributor

@tgreenx tgreenx Jul 25, 2023

Choose a reason for hiding this comment

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

How much extra will the implementation require to output ns (ns_list) instead of ns_ip (ns_ip_list)?

In the implementation, name server objects (Engine::Nameserver) must always have a name and an IP address, or they cannot be created. So it adds no complexity to return either the name, the IP address, or both.

Usually we want to loop over the IP addresses, and then what should be reported if the there are two names to the same IP address? Just report one of them (kind of randomly)?

Method 4 and Method 5 both return name server objects. From what I see this is what is already being done in other DNSSEC test cases: a name server is skipped if its IP address has already been processed. I think its fine to do that. If we want to catch such occurrences I think a more generic test case, e.g. BASIC, would be more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tgreenx, would it then be OK to say that in the specification it normally only refer to the name server IP except in the msgid where it should be explicit if it should be {ns} or {ns_ip}? If we think that would be good, then that could be spelled out in the template.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think that is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let us try that. See updates.

docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
Copy link
Contributor

@tgreenx tgreenx left a comment

Choose a reason for hiding this comment

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

@matsduf
Copy link
Contributor Author

matsduf commented Jul 20, 2023

  • The section "Terminology" can be removed.

I think it is better to keep it, but with a comment.

* In section "Special procedural requirements", use [the proper sentence from the template](https://github.com/zonemaster/zonemaster/blob/c7c6793e8cb7dec1ca9ede5881f622f5cfae9148/docs/internal/templates/specifications/tests/Template01.md?plain=1#L195-L197).

Fixed.

docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
DS10_INCONSISTENT_NSEC3 | ERROR | ns_ip_list | Inconsistent responses from zone with NSEC3. Fetched from nameservers with IP addresses "{ns_ip_list}".
DS10_INCONSISTENT_NSEC_NSEC3 | ERROR |ns_ip_list_nsec, ns_ip_list_nsec3| The zone is inconsistent on NSEC and NSEC3. NSEC is fetched from nameservers with IP addresses "{ns_ip_list_nsec}". NSEC3 is fetched from nameservers with IP addresses "{ns_ip_list_nsec3}".
DS10_MISSING_NSEC_NSEC3 | ERROR | ns_ip_list | NSEC or NSEC3 is expected but both are missing. Fetched from nameservers with IP addresses "{ns_ip_list}".
DS10_MIXED_NSEC_NSEC3 | ERROR | ns_ip_list | Both NSEC and NSEC3 are responded from the zone, which is . Fetched from the nameservers with IP addresses "{ns_ip_list}".
Copy link
Contributor

Choose a reason for hiding this comment

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

At first glance, the way the msgid is worded makes it seem redundant with DS10_INCONSISTENT_NSEC_NSEC3 (besides, the first sentence seems incomplete). From my understanding, it means that the zone serves a mix of NSEC resource records on one hand, and either NSEC3 or NSEC3PARAM resource records on the other hand. Am I correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make the sentence complete ("Both NSEC and NSEC3 are responded from the zone, where only one of them is expected.").

Yes. If the server returns an NSEC record (either in the answer section when querying for NSEC or on the authority section when querying for NSEC3PARAM) we consider this server to NSEC type for the zone. If the server returns NSEC3PARAM record in the answer section when querying for it or NSEC3 record in the authority section when querying for NSEC we consider the server to NSEC3 type for the zone.

DS10_MIXED_NSEC_NSEC3 means that one or several servers have been identified as both NSEC type and NSEC3 type.

DS10_INCONSISTENT_NSEC_NSEC3 means that some servers are pure NSEC type and others are pure NSEC3 type for the same zone.

Do you want clarification in the specification? Should I copy the text above into the specification as a comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please; I think it can be a useful clarification. As for the msgid, I think it should work too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add that.

DS10_NSEC3PARAM_QUERY_RESPONSE_ERR | ERROR | ns_ip_list | No response or error in response on query for NSEC3PARAM. Fetched from the nameservers with IP addresses "{ns_ip_list}".
DS10_NSEC3PARAM_GIVES_ERR_ANSWER | ERROR | ns_ip_list | Unexpected DNS record in the answer section on an NSEC3PARAM query. Fetched from nameservers with IP addresses "{ns_ip_list}".
DS10_NSEC3_ERR_TYPE_LIST | ERROR | ns_ip_list | NSEC3 record for the zone apex with incorrect type list. Fetched from nameservers with IP addresses "{ns_ip_list}".
DS10_NSEC3_MISMATCH_APEX | ERROR | ns_ip_list | NSEC3 record with a non-apex owner name, which is unexpectation. Fetched from nameservers with IP addresses "{ns_ip_list}".
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
DS10_NSEC3_MISMATCH_APEX | ERROR | ns_ip_list | NSEC3 record with a non-apex owner name, which is unexpectation. Fetched from nameservers with IP addresses "{ns_ip_list}".
DS10_NSEC3_MISMATCH_APEX | ERROR | ns_ip_list | NSEC3 record with a non-apex owner name, which is unexpected. Fetched from nameservers with IP addresses "{ns_ip_list}".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An NSEC3 record can never have a true apex owner name. It needs rewording and I will do that.

docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
Comment on lines 256 to 260
2. Else if the type list in the NSEC record does not match the
following criteria then add name server IP to the
*NSEC Incorrect Type List* set:
1. Contains SOA, NS, DNSKEY, NSEC and RRSIG.
2. Does not contain NSEC3PARAM or NSEC3.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again: the list must contain SOA, NS, DNSKEY, NSEC and RRSIG but neither NSEC3PARAM nor NSEC3?
I’m thinking that it might be clearer to word this paragraph as:

Else if the type list in the NSEC record matches at least one of the following criteria then add name server IP to the NSEC Incorrect Type List set:

  1. SOA, NS, DNSKEY, NSEC or RRSIG is missing from the list;
  2. The list contains NSEC3PARAM or NSEC3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updates.

Comment on lines 439 to 441
### The Non-Existent Query Name

The term "The Non-Existent Query Name" is used for a name in the *Child Zone*,
Copy link
Contributor

Choose a reason for hiding this comment

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

This paragraph is no longer needed, it seems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Removed.

[RFC 4034#section-4]: https://datatracker.ietf.org/doc/html/rfc4034#section-4
[RFC 4035#section-3.1.3]: https://datatracker.ietf.org/doc/html/rfc4035#section-3.1.3
[RFC 5155#section-3]: https://datatracker.ietf.org/doc/html/rfc5155#section-3
[RFC 5155#section-4]: https://datatracker.ietf.org/doc/html/rfc5155#section-4
[RFC 5155#section-7.2]: https://datatracker.ietf.org/doc/html/rfc5155#section-7.2
[RFC 5890#section-2.3.1]: https://datatracker.ietf.org/doc/html/rfc5890#section-2.3.1
Copy link
Contributor

Choose a reason for hiding this comment

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

This link is no longer needed and can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Copy link
Contributor

@marc-vanderwal marc-vanderwal left a comment

Choose a reason for hiding this comment

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

Just one more typo :-)

docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
marc-vanderwal
marc-vanderwal previously approved these changes Jul 25, 2023
@matsduf
Copy link
Contributor Author

matsduf commented Jul 25, 2023

@marc-vanderwal, see my response in #1179 (comment). Any comment?

@marc-vanderwal, see my response in #1179 (comment). Any comment?

@tgreenx, see my response in #1179 (comment). Any comment?

DS10_NSEC3PARAM_QUERY_RESPONSE_ERR | ERROR | ns_ip_list | No response or error in response on query for NSEC3PARAM. Fetched from the nameservers with IP addresses "{ns_ip_list}".
DS10_NSEC3PARAM_GIVES_ERR_ANSWER | ERROR | ns_ip_list | Unexpected DNS record in the answer section on an NSEC3PARAM query. Fetched from nameservers with IP addresses "{ns_ip_list}".
DS10_NSEC3_ERR_TYPE_LIST | ERROR | ns_ip_list | NSEC3 record for the zone apex with incorrect type list. Fetched from nameservers with IP addresses "{ns_ip_list}".
DS10_NSEC3_MISMATCHES_APEX | ERROR | ns_ip_list | The NSEC3 record returned does not match the zone name, which it should. Fetched from nameservers with IP addresses "{ns_ip_list}".
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove which it should (See zonemaster/zonemaster-engine#1014)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is hard to refrain from including judgements in the msgid and at the same time giving a broadly understandable message. The issue is the view of the reporter, not a decided policy. I think it could be worth to have a work shop session discussing msgids that are good and bad and trying to define some policy for them.

So "expected" is fined, but not "which it should"?

I'll do an update.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that the line is fine - in that specific case, I suggested to remove that part of the sentence because it seemed unnecessarily redundant.

We could discuss of such topic in the upcoming F2F.

docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
@matsduf
Copy link
Contributor Author

matsduf commented Jul 25, 2023

The added reference to DNSSEC05 should be reviewed in the context of PR #1183.

@matsduf matsduf requested a review from tgreenx July 25, 2023 15:50
@matsduf
Copy link
Contributor Author

matsduf commented Jul 25, 2023

@tgreenx and @marc-vanderwal, please re-review.

DS10_NSEC3_MISSING_SIGNATURE | ERROR | ns_ip_list | Missing RRSIG (signature) for the NSEC3 record or records. Fetched from the nameservers with IP addresses "{ns_ip_list}".
DS10_NSEC3_NODATA_MISSING_SOA | ERROR | ns_ip_list | Missing SOA record in NODATA response with NSEC3. Fetched from nameservers with IP addresses "{ns_ip_list}".
DS10_NSEC3_NODATA_WRONG_SOA | ERROR | ns_ip_list | Wrong owner name on SOA record in NODATA response with NSEC3. Fetched from nameservers with IP addresses "{ns_ip_list}".
DS10_NSEC3_NODATA_WRONG_SOA | ERROR | ns_ip_list, domain | Wrong owner name ({"domain"}) on SOA record in NODATA response with NSEC3. Fetched from nameservers with IP addresses "{ns_ip_list}".
Copy link
Contributor

Choose a reason for hiding this comment

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

({"domain"}) -> ("{domain}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

docs/public/specifications/tests/DNSSEC-TP/dnssec10.md Outdated Show resolved Hide resolved
@matsduf matsduf requested a review from tgreenx July 25, 2023 19:43
matsduf and others added 8 commits August 11, 2023 18:00
* Replaces all ns_ip_list with ns_list.
* Makes support of name server name for msgid implicit.
* Adds comment on NSEC and NSEC3 mixing to make it more understandable.
* Additional editorial updates
* Removes definite article where it it probably best left out
* Replaces "nameserver" with "name server" to be consistent
marc-vanderwal
marc-vanderwal previously approved these changes Aug 16, 2023
marc-vanderwal
marc-vanderwal previously approved these changes Aug 17, 2023
@matsduf matsduf mentioned this pull request Aug 25, 2023
@matsduf
Copy link
Contributor Author

matsduf commented Sep 27, 2023

@tgreenx, are you fine with this? Could you implement it?

@matsduf
Copy link
Contributor Author

matsduf commented Nov 22, 2023

@tgreenx and @marc-vanderwal, I resolved a minor conflict. Please re-review.

@tgreenx
Copy link
Contributor

tgreenx commented Nov 23, 2023

@tgreenx, are you fine with this? Could you implement it?

We should wait for test zones specification (and data) first, so I propose to postpone to v2024.1.

@matsduf matsduf modified the milestones: v2023.2, v2024.1 Dec 4, 2023
@tgreenx tgreenx modified the milestones: v2024.1, v2024.2 Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-TestCase Area: Test case specification or implementation of test case
Projects
None yet
3 participants