Update implementation of DNSSEC03 #1304

tgreenx · 2023-11-15T13:52:55Z

Purpose

This PR proposes an update to the implementation of DNSSEC03 following an update to the specification (zonemaster/zonemaster#1189).

Note that Prefix Suffix List check (step 13.2.2.1) is not yet implemented, as this service is not yet provided by Zonemaster.

Requires a fix to Zonemaster::LDNS::RR::NSEC3 to work: zonemaster/zonemaster-ldns#177

Context

Test Case specification: zonemaster/zonemaster#1189
Test Zones specification: zonemaster/zonemaster#1218

Requires zonemaster/zonemaster-ldns#177

Changes

lib/Zonemaster/Engine/Test/DNSSEC.pm

Update implementation
Update message tags and message ids

share/profile.json and share/profile.yaml

Update message tags

t/Test-dnssec03.t and t/Test-dnssec03.data

Update unitary tests implementation and data

How to test this PR

Unit tests are updated and should pass.

Manual testing:

$ git log -1 --oneline
25036e6 (HEAD -> update-dnssec03, origin/update-dnssec03) Update implementation of DNSSEC03

$ zonemaster-cli --test dnssec/dnssec03 --show-testcase --level INFO zonemaster.net
Seconds Level    Testcase       Message
======= ======== ============== =======
   0.01 INFO     UNSPECIFIED    Using version v4.7.3 of the Zonemaster engine.
   5.79 INFO     DNSSEC03       The zone does not use NSEC3. Testing for NSEC3 has been skipped. Fetched from name servers "ns2.nic.fr/192.93.0.4;ns2.nic.fr/2001:660:3005:1::1:2;nsa.dnsnode.net/194.58.192.46;nsa.dnsnode.net/2a01:3f1:46::53;nsp.dnsnode.net/194.58.198.32;nsp.dnsnode.net/2a01:3f1:3032::53;nsu.dnsnode.net/185.42.137.98;nsu.dnsnode.net/2a01:3f0:400::32".

$ zonemaster-cli --test dnssec/dnssec03 --show-testcase --level INFO afnic.fr
Seconds Level    Testcase       Message
======= ======== ============== =======
   0.00 INFO     UNSPECIFIED    Using version v4.7.3 of the Zonemaster engine.
   3.66 INFO     DNSSEC03       The following servers respond with a legal hash algorithm in NSEC3. Fetched from name servers "g.ext.nic.fr/194.0.36.1;g.ext.nic.fr/2001:678:4c::1;ns1.nic.fr/192.134.4.1;ns1.nic.fr/2001:67c:2218:2::4:1;ns2.nic.fr/192.93.0.4;ns2.nic.fr/2001:660:3005:1::1:2;ns3.nic.fr/192.134.0.49;ns3.nic.fr/2001:660:3006:1::1:1".
   3.66 NOTICE   DNSSEC03       The following servers respond with NSEC3 opt-out enabled. The recommended practice is to disable opt-out. Fetched from name servers "g.ext.nic.fr/194.0.36.1;g.ext.nic.fr/2001:678:4c::1;ns1.nic.fr/192.134.4.1;ns1.nic.fr/2001:67c:2218:2::4:1;ns2.nic.fr/192.93.0.4;ns2.nic.fr/2001:660:3005:1::1:2;ns3.nic.fr/192.134.0.49;ns3.nic.fr/2001:660:3006:1::1:1".
   3.66 INFO     DNSSEC03       The following servers respond with NSEC3 iteration value set to zero (as recommended). Fetched from name servers "g.ext.nic.fr/194.0.36.1;g.ext.nic.fr/2001:678:4c::1;ns1.nic.fr/192.134.4.1;ns1.nic.fr/2001:67c:2218:2::4:1;ns2.nic.fr/192.93.0.4;ns2.nic.fr/2001:660:3005:1::1:2;ns3.nic.fr/192.134.0.49;ns3.nic.fr/2001:660:3006:1::1:1".
   3.66 INFO     DNSSEC03       The following servers respond with a legal empty salt in NSEC3. Fetched from name servers "g.ext.nic.fr/194.0.36.1;g.ext.nic.fr/2001:678:4c::1;ns1.nic.fr/192.134.4.1;ns1.nic.fr/2001:67c:2218:2::4:1;ns2.nic.fr/192.93.0.4;ns2.nic.fr/2001:660:3005:1::1:2;ns3.nic.fr/192.134.0.49;ns3.nic.fr/2001:660:3006:1::1:1".

matsduf · 2023-11-16T12:38:42Z

"Not in MANIFEST: t/Test-dnssec03.t"

Follows specifiation update (zonemaster/zonemaster#1189). Note that Prefix Suffix List check (step 13.2.2.1) is not yet implemented, as this service is not yet provided by Zonemaster. Also note that this commit does not update unit tests yet.

Unit test data will be recorded and added in a later commit.

matsduf · 2023-11-17T09:43:19Z

"/usr/local/bin/perl" "-Iinc" "-MExtUtils::Manifest=fullcheck" -e fullcheck
No such file: t/Test-dnssec03.data

matsduf · 2023-11-17T11:01:14Z

$ zonemaster-cli --raw  --test DNSSEC/dnssec03 --hints COMMON/hintfile --level info bad-values.dnssec03.xa
   0.00 INFO     GLOBAL_VERSION   version=v4.7.3
free(): double free detected in tcache 2
Aborted (core dumped)

matsduf · 2023-11-17T11:42:24Z

$ zonemaster-cli --raw  --test DNSSEC/dnssec03 --hints COMMON/hintfile --level info inconsistent-values.dnssec03.xa
   0.00 INFO     GLOBAL_VERSION   version=v4.7.3
free(): double free detected in tcache 2
Aborted (core dumped)

matsduf · 2023-11-17T16:02:17Z

docs/public/specifications/test-zones/DNSSEC-TP/dnssec03.md in zonemaster/zonemaster#1218 has been updated with corrected mandatory/forbidden tags. The change will require update of the t file, but not the implementation.

matsduf · 2023-11-17T16:03:58Z

Scenario name	Mandatory message tags	Forbidden message tags
UNASSIGNED-FLAG-USED	DS03_UNASSIGNED_FLAG_USED, DS03_LEGAL_EMPTY_SALT, DS03_LEGAL_HASH_ALGO, DS03_LEGAL_ITERATION_VALUE, DS03_NSEC3_OPT_OUT_DISABLED	DS03_ERR_MULT_NSEC3, DS03_ILLEGAL_HASH_ALGO, DS03_ILLEGAL_ITERATION_VALUE, DS03_ILLEGAL_SALT_LENGTH, DS03_INCONSISTENT_HASH_ALGO, DS03_INCONSISTENT_ITERATION, DS03_INCONSISTENT_NSEC3_FLAGS, DS03_INCONSISTENT_SALT_LENGTH, DS03_NO_DNSSEC_SUPPORT, DS03_NO_NSEC3, DS03_NSEC3_OPT_OUT_ENABLED_NON_TLD, DS03_NSEC3_OPT_OUT_ENABLED_TLD, DS03_SERVER_NO_DNSSEC_SUPPORT, DS03_SERVER_NO_NSEC3

$ zonemaster-cli --raw  --test DNSSEC/dnssec03 --hints COMMON/hintfile --level info unassigned-flag-used.dnssec03.xa
   0.00 INFO     GLOBAL_VERSION   version=v4.7.3
   0.19 INFO     DS03_LEGAL_HASH_ALGO   ns_list=ns1.unassigned-flag-used.dnssec03.xa/127.15.3.31;ns1.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:31;ns2.unassigned-flag-used.dnssec03.xa/127.15.3.32;ns2.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:32
   0.19 ERROR    DS03_UNASSIGNED_FLAG_USED   int=3; ns_list=ns1.unassigned-flag-used.dnssec03.xa/127.15.3.31;ns1.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:31;ns2.unassigned-flag-used.dnssec03.xa/127.15.3.32;ns2.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:32
   0.19 INFO     DS03_LEGAL_ITERATION_VALUE   ns_list=ns1.unassigned-flag-used.dnssec03.xa/127.15.3.31;ns1.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:31;ns2.unassigned-flag-used.dnssec03.xa/127.15.3.32;ns2.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:32
   0.19 INFO     DS03_LEGAL_EMPTY_SALT   ns_list=ns1.unassigned-flag-used.dnssec03.xa/127.15.3.31;ns1.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:31;ns2.unassigned-flag-used.dnssec03.xa/127.15.3.32;ns2.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:32

--> Missing DS03_NSEC3_OPT_OUT_DISABLED

matsduf · 2023-11-17T16:07:25Z

In two cases zonemaster-cli crashes (see above). In the test zones manipulated NSEC3 records are used, and it might be that LDNS does not like that. Maybe I have to create "real" NSEC3 records for the tests, but in any case Zonemaster should not crash.

matsduf · 2023-11-17T16:13:39Z

Must be updated:

    DNSSEC03 => sub {
        __x    # DNSSEC:DNSSEC03
          "Check for too many NSEC3 iterations";
    },

(https://github.com/zonemaster/zonemaster-engine/blob/be1bde54d0dae23e4a534dcf5966528697b93d7e/lib/Zonemaster/Engine/Test/DNSSEC.pm#L565C1-L568C6)

tgreenx · 2023-11-20T11:11:15Z

In two cases zonemaster-cli crashes (see above). In the test zones manipulated NSEC3 records are used, and it might be that LDNS does not like that. Maybe I have to create "real" NSEC3 records for the tests, but in any case Zonemaster should not crash.

Indeed, there was a bug in the implementation of Zonemaster::LDNS::RR::NSEC3::salt(). It is being fixed by zonemaster/zonemaster-ldns#177. With this mentioned PR I have done the recording of test data. See commit c9c3f60.

$ zonemaster-cli --raw  --test DNSSEC/dnssec03 --hints COMMON/hintfile --level info unassigned-flag-used.dnssec03.xa
[...]
--> Missing DS03_NSEC3_OPT_OUT_DISABLED

See my comments first: zonemaster/zonemaster#1218 (comment) and zonemaster/zonemaster#1218 (comment).

Must be updated:
    DNSSEC03 => sub {
        __x    # DNSSEC:DNSSEC03
          "Check for too many NSEC3 iterations";
    },
(https://github.com/zonemaster/zonemaster-engine/blob/be1bde54d0dae23e4a534dcf5966528697b93d7e/lib/Zonemaster/Engine/Test/DNSSEC.pm#L565C1-L568C6)

Done in commit b006d92.

tgreenx · 2023-11-20T11:32:08Z

$ zonemaster-cli --raw  --test DNSSEC/dnssec03 --hints COMMON/hintfile --level info unassigned-flag-used.dnssec03.xa
[...]
--> Missing DS03_NSEC3_OPT_OUT_DISABLED
See my comments first: zonemaster/zonemaster#1218 (comment) and zonemaster/zonemaster#1218 (comment).

Commit c027a61 updates the implementation assuming the specification is correct. See zonemaster/zonemaster#1218 (comment).

matsduf · 2023-11-20T11:44:34Z

This PR will then require updatd Zonemaster-LDNS? Will that affect Engine elsewhere?

tgreenx · 2023-11-20T11:57:21Z

This PR will then require updatd Zonemaster-LDNS? Will that affect Engine elsewhere?

No, Zonemaster::LDNS::RR::NSEC3::salt() was not used anywhere. See:

~/zonemaster/zonemaster-engine$ git log -1 --oneline
0406b98e (HEAD -> develop, upstream/develop) Merge pull request #1307 from matsduf/update-manifest

~/zonemaster/zonemaster-engine$ ack salt lib/

matsduf · 2023-11-20T21:27:56Z

From scenario UNASSIGNED-FLAG-USED:

   0.18 ERROR    The following servers respond with an NSEC3 record where an unassigned flag is used (flag 2). Fetched from name servers "(...)".

The value of the flag integer is 2, but the flag, in this case, is 6, isn't it?

matsduf

Looks fine except maybe a detail from scenario UNASSIGNED-FLAG-USED (see my comment).

I have not checked why the two tests in t/Test-dnssec.t fails. Can they just be removed?

This commit corrects the implementation of step 13 (NSEC3 Flags) from the specification.

tgreenx · 2023-11-21T12:49:28Z

From scenario UNASSIGNED-FLAG-USED:
   0.18 ERROR    The following servers respond with an NSEC3 record where an unassigned flag is used (flag 2). Fetched from name servers "(...)".
The value of the flag integer is 2, but the flag, in this case, is 6, isn't it?

Ah yes, an oversight on my end. Thing is, Zonemaster::LDNS::RR::NSEC3::flags() returns "the native uint8_t representation from the rdf" (rdf: resource record data field). See ldns_nsec3_flags() definition and code. I have just changed the code in Engine to make a bit comparison from the decimal representation instead. See commit 38ecf00.
Now:

$ git log -1 --oneline
38ecf007 (HEAD -> update-dnssec03, origin/update-dnssec03) Correct DNSSEC03 implementation, and update unit test data

$ zonemaster-cli --test DNSSEC/dnssec03 --hints ~/zonemaster/zonemaster/test-zone-data/COMMON/hintfile --level info unassigned-flag-used.dnssec03.xa
Seconds Level    Message
======= ======== =======
   0.00 INFO     Using version v4.7.3 of the Zonemaster engine.
   0.08 INFO     The following servers respond with a legal hash algorithm in NSEC3. Fetched from name servers "ns1.unassigned-flag-used.dnssec03.xa/127.15.3.31;ns1.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:31;ns2.unassigned-flag-used.dnssec03.xa/127.15.3.32;ns2.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:32".
   0.08 ERROR    The following servers respond with an NSEC3 record where an unassigned flag is used (flag 6). Fetched from name servers "ns1.unassigned-flag-used.dnssec03.xa/127.15.3.31;ns1.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:31;ns2.unassigned-flag-used.dnssec03.xa/127.15.3.32;ns2.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:32".
   0.08 INFO     The following servers respond with NSEC3 opt-out disabled (as recommended). Fetched from name servers "ns1.unassigned-flag-used.dnssec03.xa/127.15.3.31;ns1.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:31;ns2.unassigned-flag-used.dnssec03.xa/127.15.3.32;ns2.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:32".
   0.08 INFO     The following servers respond with NSEC3 iteration value set to zero (as recommended). Fetched from name servers "ns1.unassigned-flag-used.dnssec03.xa/127.15.3.31;ns1.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:31;ns2.unassigned-flag-used.dnssec03.xa/127.15.3.32;ns2.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:32".
   0.08 INFO     The following servers respond with a legal empty salt in NSEC3. Fetched from name servers "ns1.unassigned-flag-used.dnssec03.xa/127.15.3.31;ns1.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:31;ns2.unassigned-flag-used.dnssec03.xa/127.15.3.32;ns2.unassigned-flag-used.dnssec03.xa/fda1:b2:c3:0:127:15:3:32".

I have not checked why the two tests in t/Test-dnssec.t fails. Can they just be removed?

Mainly it is because I forgot to re-record unit test data for that .t file. But when I do, one unit test ends up failing (DNSSEC17, unrelated to this PR). So yes I'll just remove DNSSEC03 from this file, this way I can avoid doing the re-recording of the data so that this PR can be merged. Note that the test from this file is only about checking that running the DNSSEC module with a specific profile comprising of only one Test Case does indeed only run that Test case. See commit 8e852a0.

Please re-review.

matsduf

Besides that the unit tests for DNSSEC03 seem to crash, I am fine with the PR.

tgreenx · 2023-11-21T13:27:29Z

Besides that the unit tests for DNSSEC03 seem to crash, I am fine with the PR.

It depends on the fix from zonemaster/zonemaster-ldns#177 which is not yet merged in develop. If you prefer we can wait for that first, and then merge this PR.

matsduf · 2023-11-21T13:38:29Z

@tgreenx, please approve zonemaster/zonemaster-ldns#177 and ask @marc-vanderwal to merge. I have approved this PR so for me it is fine, but I do not think it should be merged before we can see that CI passes.

marc-vanderwal · 2023-11-21T13:56:25Z

Alright, zonemaster/zonemaster-ldns#177 is merged, and I’m glad to see that all is well.

tgreenx · 2023-11-21T13:56:41Z

@tgreenx, please approve zonemaster/zonemaster-ldns#177 and ask @marc-vanderwal to merge. I have approved this PR so for me it is fine, but I do not think it should be merged before we can see that CI passes.

Done; it has been merged and now unit tests in this PR have passed too.

lib/Zonemaster/Engine/Test/DNSSEC.pm

matsduf · 2023-11-21T23:54:53Z

@tgreenx, zonemaster/zonemaster#1189 has been updated and all review comments should be taken care of.

The test scenarios and test zones have been updated in zonemaster/zonemaster#1218 to match the added test tags. Please note that all scenarios have been updated when it comes to "forbidden" tags, i.e. the new tags have been added to all old scenarios as "forbidden". Three new scenarios have been created and also test zones for them. As far as I can see the new test zones are correct.

tgreenx · 2023-11-22T14:28:21Z

@tgreenx, zonemaster/zonemaster#1189 has been updated and all review comments should be taken care of.

The test scenarios and test zones have been updated in zonemaster/zonemaster#1218 to match the added test tags. Please note that all scenarios have been updated when it comes to "forbidden" tags, i.e. the new tags have been added to all old scenarios as "forbidden". Three new scenarios have been created and also test zones for them. As far as I can see the new test zones are correct.

Implementation and unit tests should now be up to date. See commits 045acc6 and 4ca02b6, respectively.

tgreenx added A-TestCase Area: Test case specification or implementation of test case V-Minor Versioning: The change gives an update of minor in version. labels Nov 15, 2023

tgreenx added this to the v2023.2 milestone Nov 15, 2023

tgreenx requested review from mattias-p, matsduf, hannaeko, a user and marc-vanderwal November 15, 2023 13:52

tgreenx added 2 commits November 16, 2023 14:07

Update implementation of DNSSEC03

82e128e

Follows specifiation update (zonemaster/zonemaster#1189). Note that Prefix Suffix List check (step 13.2.2.1) is not yet implemented, as this service is not yet provided by Zonemaster. Also note that this commit does not update unit tests yet.

Update unitary tests of DNSSEC03, but without test data

3da5d46

Unit test data will be recorded and added in a later commit.

tgreenx force-pushed the update-dnssec03 branch from 42bf972 to 3da5d46 Compare November 16, 2023 13:44

Fix MANIFEST and update DNSSEC03 unit test

be1bde5

tgreenx added 2 commits November 20, 2023 12:04

Update unitary tests of DNSSEC03, and add unit test data

c9c3f60

Update DNSSEC03 Test Case name

b006d92

Correct implementation, unit tests and unit tests data

c027a61

marc-vanderwal mentioned this pull request Nov 20, 2023

Fix crashing induced by Zonemaster::LDNS::RR::NSEC3::salt() method zonemaster/zonemaster-ldns#177

Merged

matsduf previously approved these changes Nov 20, 2023

View reviewed changes

Correct DNSSEC03 implementation, and update unit test data

38ecf00

This commit corrects the implementation of step 13 (NSEC3 Flags) from the specification.

tgreenx dismissed matsduf’s stale review via 38ecf00 November 21, 2023 12:27

Remove DNSSEC03 from t/Test-dnssec.t

8e852a0

tgreenx requested review from marc-vanderwal and matsduf November 21, 2023 12:50

matsduf previously approved these changes Nov 21, 2023

View reviewed changes

marc-vanderwal reviewed Nov 21, 2023

View reviewed changes

lib/Zonemaster/Engine/Test/DNSSEC.pm Outdated Show resolved Hide resolved

tgreenx added 2 commits November 22, 2023 15:09

Update DNSSEC03 implementation

045acc6

Update DNSSEC03 unit tests and data

4ca02b6

tgreenx dismissed matsduf’s stale review via 4ca02b6 November 22, 2023 14:27

tgreenx requested a review from matsduf November 22, 2023 15:36

matsduf approved these changes Nov 22, 2023

View reviewed changes

marc-vanderwal approved these changes Nov 23, 2023

View reviewed changes

tgreenx merged commit 48efada into zonemaster:develop Nov 23, 2023
3 checks passed

tgreenx deleted the update-dnssec03 branch November 23, 2023 09:28

tgreenx mentioned this pull request Dec 5, 2023

support RFC 9276-guidance for NSEC3 in DNSSEC03 testcase zonemaster/zonemaster#1177

Closed

matsduf mentioned this pull request Dec 11, 2023

PRs for v2023.2 zonemaster/zonemaster#1232

Closed

hannaeko self-assigned this Jan 10, 2024

hannaeko added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jan 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update implementation of DNSSEC03 #1304

Update implementation of DNSSEC03 #1304

tgreenx commented Nov 15, 2023 •

edited

Loading

matsduf commented Nov 16, 2023

matsduf commented Nov 17, 2023

matsduf commented Nov 17, 2023

matsduf commented Nov 17, 2023

matsduf commented Nov 17, 2023

matsduf commented Nov 17, 2023

matsduf commented Nov 17, 2023

matsduf commented Nov 17, 2023

tgreenx commented Nov 20, 2023 •

edited

Loading

tgreenx commented Nov 20, 2023

matsduf commented Nov 20, 2023

tgreenx commented Nov 20, 2023

matsduf commented Nov 20, 2023

matsduf left a comment

tgreenx commented Nov 21, 2023 •

edited

Loading

matsduf left a comment

tgreenx commented Nov 21, 2023

matsduf commented Nov 21, 2023

marc-vanderwal commented Nov 21, 2023

tgreenx commented Nov 21, 2023

matsduf commented Nov 21, 2023

tgreenx commented Nov 22, 2023

Update implementation of DNSSEC03 #1304

Update implementation of DNSSEC03 #1304

Conversation

tgreenx commented Nov 15, 2023 • edited Loading

Purpose

Context

Changes

How to test this PR

matsduf commented Nov 16, 2023

matsduf commented Nov 17, 2023

matsduf commented Nov 17, 2023

matsduf commented Nov 17, 2023

matsduf commented Nov 17, 2023

matsduf commented Nov 17, 2023

matsduf commented Nov 17, 2023

matsduf commented Nov 17, 2023

tgreenx commented Nov 20, 2023 • edited Loading

tgreenx commented Nov 20, 2023

matsduf commented Nov 20, 2023

tgreenx commented Nov 20, 2023

matsduf commented Nov 20, 2023

matsduf left a comment

Choose a reason for hiding this comment

tgreenx commented Nov 21, 2023 • edited Loading

matsduf left a comment

Choose a reason for hiding this comment

tgreenx commented Nov 21, 2023

matsduf commented Nov 21, 2023

marc-vanderwal commented Nov 21, 2023

tgreenx commented Nov 21, 2023

matsduf commented Nov 21, 2023

tgreenx commented Nov 22, 2023

tgreenx commented Nov 15, 2023 •

edited

Loading

tgreenx commented Nov 20, 2023 •

edited

Loading

tgreenx commented Nov 21, 2023 •

edited

Loading