Create DNSSEC16 (Validate CDS) and DNSSEC17 (Validate CDNSKEY) #939

matsduf · 2021-04-21T16:42:15Z

Now updated to create two test cases, DNSSEC16 for validating CDS and DNSSEC17 for validating CDNSKEY. Previously there was one test case for both.

Also see #938 (PR to create DNSSEC15), #941 (PR to create DNSSEC18 and New-Test-Cases-for-CDS-CDNSKEY (a temporary document).

docs/specifications/tests/DNSSEC-TP/dnssec16.md

ulrichwisser · 2021-04-22T09:20:21Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+* [WARNING] message if a CDS record does not match any
+  DNSKEY in DNSKEY RRset (except for "delete" CDS).
+* [WARNING] message if a CDNSKEY record does not match any
+  DNSKEY in DNSKEY RRset (except for "delete" CDNSKEY).


Add test: [ERROR] if no CDS/CDNSKEY record matches any DNSKEY record.

There will be warning messages for every CDS and CDNSKEY record. That should be enough, shouldn't it?

If a single key doesn't match, that's a warning, but if no key matches the parent will not update. So that is an error.

docs/specifications/tests/DNSSEC-TP/dnssec16.md

ulrichwisser · 2021-04-22T09:26:42Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+* [WARNING] message if CDS RRset is signed by a key in the DNSKEY RRset
+  but not with one that has signed the DNSKEY RRset.
+* [WARNING] message if CDNSKEY RRset is signed by a key in the DNSKEY
+  RRset but not with one that has signed the DNSKEY RRset.


Actually RFC7344 says "MUST be signed with a key that is represented in both the current DNSKEY and DS RRsets,"

But that only applies if a DS set exists.

Will update.

…c keys.

ulrichwisser · 2021-04-23T08:43:53Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+* [WARNING] message if a CDS record does not match any
+  DNSKEY in DNSKEY RRset (except for "delete" CDS).
+* [WARNING] message if a CDNSKEY record does not match any
+  DNSKEY in DNSKEY RRset (except for "delete" CDNSKEY).


If a single key doesn't match, that's a warning, but if no key matches the parent will not update. So that is an error.

ulrichwisser · 2021-04-23T08:49:03Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

-* [WARNING] message if CDS RRset is signed by a key in the DNSKEY RRset
-  but not with one that has signed the DNSKEY RRset.
-* [WARNING] message if CDNSKEY RRset is signed by a key in the DNSKEY
-  RRset but not with one that has signed the DNSKEY RRset.


I don't think we should drop the requirement. The test needs to be more complex

if DS set exists ....

If DS does not exist -> in this case the above tests apply.

In this test case the DS records are not considered. That comes in DNSSEC17 (#941).

mattias-p

I found a bunch of spelling errors and minor things.

But I really like how you approach this, despite the sheer size of it.

docs/specifications/tests/DNSSEC-TP/dnssec16.md

* Adds explicit "Scope" headline * Updated text under new "Scope" * Editorial updates

* Merges table from "Outcome(s)" with summary into a new table. * Updates some message tags to make them more explicit. * Updates some set names to make them more explicit.

matsduf · 2021-04-27T16:29:01Z

I merged the table in "Outcome(s)" into the summary to make it more comprehensible on suggestion by @mattias-p. Some message tags and sets have been renamed. Also old methods are used, not MethodsNT.

@mattias-p, @ulrichwisser, @vlevigneron and @romu42, please rereview!

matsduf · 2021-04-27T16:30:21Z

@sandoche2k, I have updated the format and introduced a summary section. It replaces the table in "Outcome(s)". Can you review?

docs/specifications/tests/DNSSEC-TP/dnssec16.md

ulrichwisser · 2021-04-28T09:10:48Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+          *CDS Signed By Unknown DNSKEY* set.
+       2. Else, if the RRSIG cannot be validated by the DNSKEY it
+          refers to by key tag, then add the name server IP and RRSIG
+          key tag to the *CDS Invalid RRSIG* set.


6/7 must be done for CDS delete, too.

It is, but further down.

ulrichwisser · 2021-04-28T09:11:41Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+          *CDNSKEY Signed By Unknown DNSKEY* set.
+       2. Else, if the RRSIG cannot be validated by the DNSKEY it
+          refers to by key tag, then add the name server IP and RRSIG
+          key tag to the *CDNSKEY Invalid RRSIG* set.


Same as above. 6/7 needs to be done for CDS delete, too

It is, but further up.

ulrichwisser · 2021-04-28T09:12:43Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+
+10. If the *No DNSKEY RRset* set is non-empty, then output
+    *[DS16_CDS_CDNSKEY_WITHOUT_DNSKEY]* with all name server IP addresses
+    in the set.


Positive statement "if all DNSKEY RRsets are empty ....". Much easier to understand

The No DNSKEY RRset set is defined as a set of IP addresses.

…or CDNSKEY only.

matsduf · 2021-04-28T16:01:45Z

@romu42, @mattias-p, @ulrichwisser and @vlevigneron, DNSSEC16 was too large. It has now been split up into two test cases, one for CDS and one for CDNSKEY. Please rereview.

mattias-p

I have only looked at DNSSEC16.

docs/specifications/tests/DNSSEC-TP/dnssec16.md

mattias-p · 2021-05-07T16:34:36Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+
+4.  Create the following empty sets:
+    1.  Name server IP address and associated CDS RRset and its RRSIG
+        records ("CDS RRsets"). RRSIG records may be absent.


Optional sets is a new concept, IIRC. Would you consider representing this with an empty set instead? That should be operationally equivalent, but it would make the test cases more consistent in the way they're constructed.

There's another one like this below.

It is only the RRsig records that can be absent. They are, if present, connected to the CDS RRset. I think it is better to keep them in the same set as the CDS RRset.

Yes. My suggestion is to make it so that the RRSIG RRset is required, but that it may be empty. That way you only have one kind of absence to think about when you consider the state space that can be represented within the defined data structures. Having a smaller state space makes to consider makes the steps below easier to understand.

As authors of the test specifications we may know that the steps don't make use of the possibility for the RRSIG RRset to be present but empty. But for someone working through the test case who doesn't understand it yet, this distinction is another thing to keep in mind. And AFAICT for on good reason.

I honestly think that "absent" is better, but I have used empty in other specifications. It is clear, in a DNS context, what an absent RRSIG record is, but it is not defined what an empty set of RRSIG records is, and we have not defined it.

Another thing is that we cannot refer to RRSIG RRset.

I will do the change, with some hesitation.

Oh, that's interesting! You're thinking of an RRset as something different than a set of records of the same record type? Is there a meaningful distinction?

In any case the empty set is a very simple yet powerful idea that makes many things easier.

An RRset is a group of DNS records that share owner name, class and RR type. All members of a RRset share two features:

If one member of the RRset is present in the answer section, then all members must be present, or else the packet must set as truncated.

All members of the RRset must have the same TTL.

Neither of those two features apply to RRSIGs with the same owner name and class.

A RRSIG RR always has the same TTL as the RRset it signs, but not necessarily the same TTL as other RRSIGs. The RRSIGs of SOA, NS and DNSKEY RRsets can have different TTLs.

A RRSIG RR usually follows the RRset it signs. In a reply to a DNSSEC enabled SOA query, the answer section will containt the SOA record and the RRSIG for that, but not the RRSIG for the DNSKEY RRset.

RRSIGs are special. They are more of an extension of the RRset it signs than a RR of its own.

docs/specifications/tests/DNSSEC-TP/dnssec16.md

mattias-p · 2021-05-07T17:19:37Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+       2. Else, add the name server IP address to the *Delete CDS*
+          set.
+       3. Go to next name server IP.
+    3. Get the DNSKEY and its RRSIG records, if any, from the


What's the meaning of this "if any" clause again? Does it mean any DNSKEY and RRSIG records, or any RRSIG records for this DNSKEY since that set may be absent? Could it be present but empty? Anyway, what happens if the "if any" clause is false?

There are the following possibilities:

There is no key for the name server IP, and obviously no records.

There is a key for the name server IP and at least one DNSKEY record but no RRSIG records.

There is a key for the name server IP, at least one DNSKEY record and at least one RRSIG record.

If there are any RRSIG, then it is always over the DNSKEY RRset.

If there is no DNSKEY RRset, then we follow the next instruction "If there are no DNSKEY records, then do".

Okay, so "if any" isn't about control flow. It's only there to let the reader know that they shouldn't worry even if there is no RRSIG RRset associated with the name server IP. Right?

How do you feel about this wording?

Get the DNSKEY and its RRSIG records from the *DNSKEY RRsets* for the same name server IP. The set of RRSIG records may be empty.

To me that would be much better.

It is ok. I have done change.

mattias-p · 2021-05-07T17:39:56Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+11. If the *No Match CDS With DNSKEY* set is non-empty then for each
+    key tag in the set output *[DS16_CDS_MATCHES_NO_DNSKEY]* with the
+    CDS key tag and the name server IP addresses in the set per key
+    tag.


This could be simplified without changing the behavior, right? It'd be shorter, less complex and easier to parse.
If the *No Match CDS With DNSKEY* set is non-empty then for each key tag in the set output
->
For each key tag in the *No Match CDS With DNSKEY* output

Also, first it talks about "the key tag", then "the CDS key tag" and then "the key tag" again. Those three are all the same key tag, right?

What's going on with the tag arguments? It looks pretty complicated. Is the problem that you add them per name server IP but here you want to get them per key tag?

This goes for steps 12 and 15 as well.

It is complicated, or at least I have given room for complicated situations. If different server return different CDS records (different key tags) and none of them match DNSKEY record, the the message is to be outputted once for each CDS record (key tag) with a list of the servers that it was found on.

I will rewrite in smaller steps.

docs/specifications/tests/DNSSEC-TP/dnssec16.md

matsduf · 2021-05-07T21:25:09Z

@mattias-p, I applied your comments in the same way on both DNSSEC16 and DNSSEC17.

mattias-p · 2021-05-10T12:59:09Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+    1.  Name server IP address and associated CDS RRset and its RRSIG
+        records ("CDS RRsets"). RRSIG records may be absent.
+    2.  Name server IP address and associated DNSKEY RRset and its 
+        RRsig records ("DNSKEY RRsets"). RRSIG records may be absent.


RRsig -> RRSIG

mattias-p · 2021-05-10T13:00:39Z

docs/specifications/tests/DNSSEC-TP/dnssec17.md

@@ -0,0 +1,225 @@
+# DNSSEC16: Validate CDNSKEY


DNSSEC16 -> DNSSEC17

mattias-p · 2021-05-10T13:03:23Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+
+## Objective
+
+CDS and CDNSKEY record types are defined in [RFC 7344] and [RFC 8078].


Should we really be speaking of CDNSKEY in the objective section of DNSSEC16? It makes sense to mention it in the scope section. But having it in the Objective section makes it look like this test case also concerns itself with CDNSKEY.

And conversely in DNSSEC17.

I think it is. They are very connected. It says "The objective of this test case is to verify that the CDS RRset is valid" and "For tests of the CDNSKEY, see test case [DNSSEC17]".

mattias-p · 2021-05-10T13:07:16Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+
+4.  Create the following empty sets:
+    1.  Name server IP address and associated CDS RRset and its RRSIG
+        records ("CDS RRsets"). RRSIG records may be absent.


Yes. My suggestion is to make it so that the RRSIG RRset is required, but that it may be empty. That way you only have one kind of absence to think about when you consider the state space that can be represented within the defined data structures. Having a smaller state space makes to consider makes the steps below easier to understand.

As authors of the test specifications we may know that the steps don't make use of the possibility for the RRSIG RRset to be present but empty. But for someone working through the test case who doesn't understand it yet, this distinction is another thing to keep in mind. And AFAICT for on good reason.

mattias-p · 2021-05-10T13:16:08Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+       2. Else, add the name server IP address to the *Delete CDS*
+          set.
+       3. Go to next name server IP.
+    3. Get the DNSKEY and its RRSIG records, if any, from the


Okay, so "if any" isn't about control flow. It's only there to let the reader know that they shouldn't worry even if there is no RRSIG RRset associated with the name server IP. Right?

How do you feel about this wording?

Get the DNSKEY and its RRSIG records from the *DNSKEY RRsets* for the same name server IP. The set of RRSIG records may be empty.

To me that would be much better.

mattias-p

I have a couple of questions. But as a test specification I think this is really good! I haven't checked it thoroughly against the RFCs though.

mattias-p · 2021-05-10T17:15:31Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+          record in the answer section, then add the name server IP and
+          the DNSKEY RRset from the answer section to the 
+          *DNSKEY RRsets* set. Also include any associated RRSIG 
+          records in the answer section.


in the answer section -> from the answer section?

I guess both could do.

mattias-p · 2021-05-10T17:26:47Z

docs/specifications/tests/DNSSEC-TP/dnssec16.md

+       1. Compare the key tag from the CDS record with the calculated
+          key tags for the DNSKEY records.


Potentially stupid question: Are the calculated key tags read from the DNSKEY records, or do we perform the calculation ourselves?

If we're doing it ourselves, maybe it should be promoted to a step of its own?

No, the key tag is calculated from the DNSKEY RR. If you run this manually, dig can provide it to you. ldns provides a method to do it.

Let us save that possible change to the next iteration.

First version

f215abd

matsduf added A-TestCase Area: Test case specification or implementation of test case FA-Basic04 FA-MethodV2 Focus Area: Implementing and migrating to MethodNT for test cases. labels Apr 21, 2021

matsduf added this to the v2021.2 milestone Apr 21, 2021

matsduf requested review from romu42, mattias-p and vlevigneron April 21, 2021 16:42

matsduf force-pushed the create-dnssec16 branch from e98c878 to f215abd Compare April 21, 2021 16:43

ulrichwisser reviewed Apr 22, 2021

View reviewed changes

docs/specifications/tests/DNSSEC-TP/dnssec16.md Outdated Show resolved Hide resolved

matsduf mentioned this pull request Apr 22, 2021

Create DNSSEC15 (Existence of CDS and CDNSKEY) #938

Merged

ulrichwisser reviewed Apr 22, 2021

View reviewed changes

matsduf added 2 commits April 22, 2021 12:04

Editorial updates

c6ea3e7

Removes the requirement that DNSKEY RRset is signed with some specifi…

d07ed5b

…c keys.

matsduf requested a review from ulrichwisser April 23, 2021 06:59

matsduf mentioned this pull request Apr 23, 2021

Create DNSSEC18 (Validate trust from DS to CDS and CDNSKEY) #941

Merged

matsduf requested a review from nicpou April 23, 2021 10:48

ulrichwisser reviewed Apr 23, 2021

View reviewed changes

mattias-p reviewed Apr 23, 2021

View reviewed changes

Updates DNSSEC16

e80a5c3

* Adds explicit "Scope" headline * Updated text under new "Scope" * Editorial updates

matsduf requested review from mattias-p and ulrichwisser April 26, 2021 16:14

matsduf added 2 commits April 27, 2021 18:17

Updates specification

2d71acb

* Merges table from "Outcome(s)" with summary into a new table. * Updates some message tags to make them more explicit. * Updates some set names to make them more explicit.

Removes reference to MethodsNT and adds reference to old methods

0bad9d9

matsduf requested a review from sandoche2k April 27, 2021 16:29

mattias-p reviewed Apr 28, 2021

View reviewed changes

ulrichwisser reviewed Apr 28, 2021

View reviewed changes

matsduf added 2 commits April 28, 2021 17:55

Updates DNSSEC16 to include CDS related tests only.

33716b5

First version of DNSSEC17 that runs the same tests as DNSSEC16, but f…

cbba74c

…or CDNSKEY only.

matsduf changed the title ~~Create DNSSEC16 (Validate CDS and CDNSKEY)~~ Create DNSSEC16 (Validate CDS) and DNSSEC17 (Validate CDNSKEY) Apr 28, 2021

matsduf requested review from mattias-p and ulrichwisser April 28, 2021 16:00

matsduf removed the FA-MethodV2 Focus Area: Implementing and migrating to MethodNT for test cases. label Apr 28, 2021

Editorial update

2dfdc60

mattias-p reviewed May 7, 2021

View reviewed changes

matsduf added 2 commits May 7, 2021 23:03

Restructures and rewords, but changes no logic.

62cd37e

Restructures and rewords, but changes no logic (DNSSEC17).

edc1100

matsduf requested a review from mattias-p May 7, 2021 21:24

mattias-p reviewed May 10, 2021

View reviewed changes

Corrects typos and does editorial updates.

2c2f85c

matsduf requested a review from mattias-p May 10, 2021 15:06

mattias-p approved these changes May 10, 2021

View reviewed changes

matsduf merged commit 71a8d5e into zonemaster:develop May 10, 2021

matsduf deleted the create-dnssec16 branch May 10, 2021 19:07

This was referenced May 10, 2021

Create DNSSEC16 (Validate CDS) zonemaster/zonemaster-engine#897

Closed

Create DNSSEC17 (Validate CDNSKEY) zonemaster/zonemaster-engine#898

Closed


		## Objective

		CDS and CDNSKEY record types are defined in [RFC 7344] and [RFC 8078].

		1. Compare the key tag from the CDS record with the calculated
		key tags for the DNSKEY records.

Create DNSSEC16 (Validate CDS) and DNSSEC17 (Validate CDNSKEY) #939

Create DNSSEC16 (Validate CDS) and DNSSEC17 (Validate CDNSKEY) #939

Conversation

matsduf commented Apr 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf Apr 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattias-p left a comment

Choose a reason for hiding this comment

matsduf commented Apr 27, 2021

matsduf commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Apr 28, 2021

mattias-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented May 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf May 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattias-p left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Apr 21, 2021 •

edited

matsduf Apr 22, 2021 •

edited

matsduf May 10, 2021 •

edited

mattias-p left a comment •

edited