Conversation
Merge master branch into develop (Engine)
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.
- `Zonemaster::Engine::Recursor::_resolve_cname()` now returns undef when the CNAME resolution fails, along with a new message tag: `CNAME_UNRESOLVABLE` - Update unit tests: move the content of scenario `TOO-LONG-CNAME-CHAIN` to a new scenario `TOO-MANY-CNAME`, and update scenario `TOO-LONG-CNAME-CHAIN` to check for tag `CNAME_CHAIN_TOO_LONG` instead
Co-authored-by: Marc van der Wal <103426270+marc-vanderwal@users.noreply.github.com>
…umentation updates; Re-record unit tests
Add missing DNSKEY/DS algorithm comparison in DNSSEC02
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. 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.
Co-authored-by: Mats Dufberg <mats.dufberg@iis.se>
Co-authored-by: Mats Dufberg <mats.dufberg@iis.se>
Fix edge cases in Get-Delegation and Get-Del-NS-Names-and-IPs when dealing with fake delegation
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. Also be sure to sort the ns_lists in messages (like CS05_DELEGATION) to ease visual comparisons. Be sure to get complete glue by doing SOA queries over TCP (falling back to UDP if no response). This is to work around name servers that do not set the TC bit if they drop glue records from in-domain name servers (which breaks RFC 9471): this happens with some root name servers when they get a SOA query for `se`.
Complete reimplementation of Consistency05
Rewrite the implementation of Zone09 completely, after the specification was updated. The previous implementation had two main issues: firstly, MX RRsets were not sorted before comparison; secondly, MX RRsets that only differ by TTL values were not deemed equal, although we wanted them to compare equal. Now, only the preference and exchange fields of the RDATA are used for sorting and comparison. Test scenarios were updated as well, so this commit also updates the corresponding unit tests. Said unit tests are ported to the DSL for good measure. When relevant, even the messages’ arguments are tested.
The rewriting of Zone09 also involved a migration from old Methods to MethodsV2, leading to a few more DNS queries made to authoritative servers in parent zones. The corresponding packets do not exist in the corresponding t/Test-zone.data file, so running this unit test fails. It cannot easily be rerecorded either and the .t file barely exercised the code anyway, so we can afford not to run Zone09 in that legacy test.
Use the newly-introduced _is_non_mail_domain() method and rewrite the logic of Zone11 in a way that is equivalent to the current logic, while involving fewer nested ifs.
…ings Rewrite Zone09 fully after specification update
Update CNAME resolution
Preparation for v2026.1.2 release (Engine)
matsduf
added
P-High
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This PR is step https://github.com/zonemaster/zonemaster/blob/develop/docs/internal/maintenance/ReleaseProcess-release.md#15-merge-develop-branch-into-master
How to test this PR
"No reviewer or approval is required for this update."