Complete reimplementation of Consistency05 - #1547
Conversation
Nameservers are now properly sorted on name first, IP address version second, and IP address converted to 32-bit or 128-bit integer third. That way, ns1.example/2001:db8::1111 sorts before ns1.example/2001:db8::2, for example. The cmp operator overloads now also fully honor the calling convention that Perl uses (see perldoc overload). Operator overloads in Perl have a third argument in their calling convention, $reverse, which is called when the operands are reversed compared to the order they are written. This can happen when doing “Yoda comparisons” with strings and Nameserver objects. But the overloads method did not take that argument into account and could therefore return wrong results. This is especially problematic when sorting collections of DNSName or Nameserver objects, or even collections mixing both.
It’s a set type specialized for DNSName and Nameserver objects, with some special semantics. The main feature of this set is that pushing a name server to a set that already contains a plain name which is identical to the name server’s, that plain name is removed. I hope this structure can be useful in many situations, test cases and test methods alike.
Both Get-Delegation and Get-Del-NS-Names-and-IPs had implementations that did not fully respect the specifications, and therefore caused problems that were not properly caught by the test suite. Firstly, if Get-Delegation was called with Undelegated Data being non-empty, it should return name/IP pairs for in-domain name servers that have addresses, and plain names for either out-of-domain name servers, or in-domain name servers that lack glue records. Instead, in-domain name servers without glue were missing from its return value. Secondly, Get-Del-NS-Names-and-IPs uses Get-Delegation, then is supposed to extract the out-of-domain name server names from that set to resolve those into addresses. Instead of taking only the out-of-domain name server names, it merely took the items from the set that are plain names without filtering out the in-domain names. The implementation for Get-Delegation is significantly rewritten in order to reduce repetition, reduce the number of branches, avoid too many nested indentation levels, and fix some miscellaneous programming oversights. Both Get-Delegation and Get-Del-NS-Names-and-IPs leverage the new Zonemaster::Engine::NameserverSet class to factor out common operations on sets mixing name server/IP pairs and plain names. It also turned out that one scenario had incorrect test data. CHILD-NS-CNAME-4 has one delegation with missing glue, that Get-Del-NS-Names-and-IPs should have returned as a plain name, but ignored instead.
Methods Get-Del-NS-IPs and Get-Zone-NS-IPs are modified to return
Zonemaster::Engine::Nameserver objects instead of plain addresses.
This change is necessary because the new specification of Consistency05
involves sending queries to name servers obtained by the combination of
these two methods. But they return plain addresses, and for queries, we
really do need Nameserver objects instead.
No other test case has used these two methods so far, so this change
does not break any existing test case. And it also means that the
behavior of Get-{Del,Zone}-NS-IPs is brought in line with
Get-Parent-NS-IPs. Which is technically breaking the specification…
It’s not great, but it’s a necessary evil so that Consistency05 can
work.
In the testing DSL, if a fake_ns keyword is used in a scenario
declaration that tests a root zone to add a fake name server without
addresses that has the same name as a name server in the root hints, the
addresses in the root hints would continue to be used despite the
fake_ns keyword declaring that they be cleared.
As an example, suppose we load the following root hints:
. IN NS ns1.root-servers.test.
. IN NS ns2.root-servers.test.
ns1.root-servers.test. IN AAAA 2001:db8:111::53
ns2.root-servers.test. IN AAAA 2001:db8:222::53
and a .t file declares the following scenario
scenario 'BUGGY-SCENARIO' => sub {
zone '.';
fake_ns 'ns1.root-servers.test' => [ qw(3ffe::111:53) ];
fake_ns 'ns2.root-servers.test';
};
then the scenario would be run against
ns1.root-servers.test/3ffe::111:53, which is correct, but also
ns2.root-servers.test/2001:db8:222::53, which is incorrect.
We address this by ensuring that if we are operating on the root zone,
the previously-loaded root hints are entirely cleared before adding the
fake delegation.
Rewrite the implementation of Consistency05 from top to bottom after the test case’s specification was revised. Add scenario-based unit tests too, using the scenario-based testing DSL, making the legacy Consistency05 tests redundant. Do not run Consistency05 in the legacy t/Test-consistency.t file anymore. This test case’s conversion to MethodsV2 means that it generates queries that weren’t sent out before. The old data file cannot be recorded again, and the old tests were redundant with the new tests anyway.
marc-vanderwal
added
A-TestCase
|
@marc-vanderwal, I think "draft" means not ready for review. You say this is ready for review, but only to be merged after #1546 is merged. That is a different thing. |
matsduf
left a comment
matsduf
left a comment
There was a problem hiding this comment.
All scenarios have been tested using zonemaster-cli including zonemaster/zonemaster-cli#466 but a few scenarios do not match.
I see. GitHub really lacks a way to say “this is ready for review but shouldn’t be merged yet”. I’ll just keep using text surrounded with emoji as a workaround.
Weird, because the unit tests pass. Did I make a mistake in the |
| my $set = Zonemaster::Engine::NameserverSet->new(); | ||
| $set->push( keys %authority, @additional ); |
There was a problem hiding this comment.
Note to self: should have written this as:
| my $set = Zonemaster::Engine::NameserverSet->new(); | |
| $set->push( keys %authority, @additional ); | |
| my $set = Zonemaster::Engine::NameserverSet->new( keys %authority, @additional ); |
There was a problem hiding this comment.
@marc-vanderwal, should this update be added before I test again?
|
Running this proposed change for a tld with many namservers and glue records would yield a lot of different responses from the root servers with regards to the glue records because of trimming the additional section, thus emitting CS05_INCONSISTENT_DELEGATION. This could be resolved by using EDNS0 to increase buffer size for this queries, or dropping the glue. Have you looked into this? The .se domain would exhibit CS05_INCONSISTENT_DELEGATION, and .net would show CS05_MISSING_GLUE_FOR_NS. |
|
Hi @pawal! Thanks for testing this along with us. That’s an interesting observation you make. Both Yet as per RFC 9471, if any in-domain glue records are dropped from the response to make it fit in 512 bytes over UDP, the TC bit must be set. None of the root servers do this, so they are breaking RFC 9471 in that regard. If they did, these errors likely wouldn’t happen. |
Also be sure to sort the ns_lists in messages (like CS05_DELEGATION) to ease visual comparisons.
When I test {a,f,j}.root-servers.net correctly sets the TC bit. |
3 participants
Purpose
When #1546 is merged into develop, this branch should be rebased on develop.⚠️
This PR updates the implementation of Consistency05 after its specification was rewritten and its test scenarios were updated.
Context
Changes
How to test this PR
Unit tests were updated and should pass.
Run a test against
disjoint.superdns.nl. The output should be similar to the following:For reference, the former (incorrect) behavior was as follows:
Run a test against
mnc001.mcc240.pub.3gppnetwork.org. The output should be similar to the following:For reference, the former (incorrect) behavior was as follows: