Fix edge cases in Get-Delegation and Get-Del-NS-Names-and-IPs when dealing with fake delegation - #1546
Fix edge cases in Get-Delegation and Get-Del-NS-Names-and-IPs when dealing with fake delegation#1546marc-vanderwal wants to merge 3 commits into
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.
c6b6de4 to
231b29f
Compare
This is a good change! |
|
Obviously, we could improve the scenarios set for MethodsV2. Do you have suggestions for some new scenarios to be added? |
The change isn’t effective everywhere: there are test cases that still list name servers while sorting them wrong. Likely because the code explicitly converts the Nameserver objects to strings before sorting. Fixing this is out of scope for this PR but might be part of some broader refactoring of test case code.
Due to time constraints, I haven’t taken a more detailed look at the scenarios yet (I know I should have), but we are definitely missing something. Looking at the reproducer script I mentioned in the PR’s description, it shouldn’t be too hard. |
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.
231b29f to
154f730
Compare
I understand that, but I think that it is great to have sorting well defined that can be used in all test cases when they are updated.
Improving the scenarios will be an eternal task! |
matsduf
added
V-Patch
| my ( $p, $ns_section ) = @_; | ||
|
|
||
| # Set of NS names from authority section | ||
| my %authority = map { lc name( $_->nsdname() ) => 1 } |
There was a problem hiding this comment.
This variable shouldn’t be called %authority; this name was fine before refactoring, but not anymore.
Be sure to also edit the comment above.
| [ qw( ns1-cname.child.parent.child-ns-cname-4.methodsv2.xa/127.40.1.51 ) ], | ||
| [ qw( ns1-cname.child.parent.child-ns-cname-4.methodsv2.xa/127.40.1.51 | ||
| ns2-cname.child.parent.child-ns-cname-4.methodsv2.xa ) ], |
There was a problem hiding this comment.
This will require an update of the corresponding test scenario specification.
Edit: done; see zonemaster/zonemaster#1528.
3022875 to
62a17cc
Compare
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. This commit also introduces a scenario that reproduces the bug and which failed before the patch. See also zonemaster/zonemaster#1526. 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. Turns out the scenario specification was wrong. See also zonemaster/zonemaster#1528.
62a17cc to
06b370f
Compare
2 participants
Purpose
Both the Get-Delegation and Get-Del-NS-Names-and-IPs methods in MethodsV2 had implementations that deviated slightly from the specifications, in such a way that they could return incorrect data in some edge cases involving fake delegation. This PR brings these methods back in line with the specification; they now return correct results in these situations.
In order to help fix this bug, I’ve introduced a new class for a new type of data structure called a NameserverSet. This class implements sets that specialize in holding a mix of Nameserver and DNSName objects. The inspiration comes from the Consistency05 specification update in zonemaster/zonemaster#1523, where the test procedure prescribes sets containing a mix of name/IP pairs and plain names, where adding a name/IP pair to a set already containing the same name also removes the plain name. NameserverSet objects have the same semantics.
Although I initially used NameserverSets to update the implementation of Consistency05, I then stumbled upon the problem described in #1545. Then, I discovered that NameserverSets were a very useful abstraction to help fix the issue. We use these sort of sets of either Nameservers, DNSNames or both in many places in the codebase and I hope that NameserverSets could help make the code more concise, both in test methods and test cases.
Note: this PR also introduces another user-visible change. One of the commits improves the
cmpoperator overloads in the Zonemaster::Engine::DNSName and Zonemaster::Engine::Nameserver classes. Name server lists in CLI and GUI output are now properly sorted on name server name first, IP version second, and numeric IP address number (as a 32-bit integer in IPv4 and 128-bit integer in IPv6) third. For example,ns1.example/2001:db8::1111sorts afterns1.example/2001:db8::2. Some message tags might still contain name server lists sorted differently, depending on how the test cases are implemented.Context
Fixes #1545.
Changes
cmpoperator overloads in Zonemaster::Engine::DNSName and Zonemaster::Engine::NameserverZonemaster::Engine::NameserverSetclassHow to test this PR