Complete reimplementation of Consistency05 - #1547
Conversation
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 |
|
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. |
When I test {a,f,j}.root-servers.net correctly sets the TC bit. |
9066757 to
35a8e5f
Compare
|
In your commit message you say it is an NS query to the parent name servers, but it is actually an SOA query. Also see the updated specification that calls for a fallback to UDP if the TCP query fails. |
35a8e5f to
a9d8088
Compare
There was a problem hiding this comment.
All scenarios (zonemaster/zonemaster#1514) pass now, but scenario ROOT-MISSING-GLUE-UNDEL-1 has been made N/A since it is not possible given the implementation.
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`.
552beb3 to
5ce2c45
Compare
|
I’ve addressed @tgreenx’s review comments, rebased on |
matsduf
left a comment
matsduf
left a comment
There was a problem hiding this comment.
I reran the scenarios with zonemaster-cli and they look fine.
4 participants
Purpose
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: