search

zonemaster / zonemaster-engine

The Zonemaster Engine - part of the Zonemaster project
Other
34 stars 33 forks source link

Update MethodsV2 get_parent_ns_ips() to match new Basic01 algorithm #1373

Closed tgreenx closed 2 months ago

tgreenx commented 2 months ago

Purpose

This PR updates MethodsV2 get_parent_ns_ips() to match new Basic01 algorithm. Other related/convenient changes are made in other modules (e.g. Zonemaster::Engine::Recursor).

It also adds a loop protection mechanism (and message tag at the DEBUG2 level) for while() loops in case of any unforeseen issue with the methods using this new algorithm.

For easier reviewing you should discard commit https://github.com/zonemaster/zonemaster-engine/pull/1373/commits/b83a7a70c3ee661efa8f9e1ff69c77f3295ae330 (which only converts indentation from tabulation to space).

Context

Specification: https://github.com/zonemaster/zonemaster/pull/1287

This implementation contains slight deviations from the specification, see https://github.com/zonemaster/zonemaster/pull/1287#pullrequestreview-2169527420.

Changes

How to test this PR

Unit tests should pass.

Also, for manual testing you can run the method of your choice with the following, e.g. for get_parent_ns_ips():

$ perl -MZonemaster::Engine -MZonemaster::Engine::TestMethodsV2 -E 'Zonemaster::Engine::Recursor->remove_fake_addresses("."); Zonemaster::Engine::Recursor->add_fake_addresses("." => { ns1 => ["127.1.0.1", "fda1:b2:c3::127:1:0:1"], ns2 => ["127.1.0.2", "fda1:b2:c3::127:1:0:2"]  }); say "\n", join "\n", @{Zonemaster::Engine::TestMethodsV2->get_parent_ns_ips(Zonemaster::Engine::Zone->new({ name => Zonemaster::Engine::DNSName->from_string("child.parent.good-1.methodsv2.xa") }))};'

ns1.parent.good-1.methodsv2.xa/127.40.1.41
ns1.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:1:41
ns2.parent.good-1.methodsv2.xa/127.40.1.42
ns2.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:1:42
tgreenx commented 2 months ago

This implementation contains slight deviations from the specification, see https://github.com/zonemaster/zonemaster/pull/1287#pullrequestreview-2169527420.

matsduf commented 2 months ago

How to test this PR

Unit tests should pass.

Also, for manual testing you can run:

I think we should test the implementations that currently fail, see #1351.

matsduf commented 2 months ago

Updated my environment to run with this update. There are four failing scenarios. One is CHLD-FOUND-INCONSIST-6

The parent is expected to be 127.40.1.41 + IPv6, and that is found by the unit test. Querying the parent for the delegation gives:

$ dig @127.40.1.41 child.parent.chld-found-inconsist-6.methodsv2.xa ns
(...)
;; ANSWER SECTION:
child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN NS ns1.child.parent.chld-found-inconsist-6.methodsv2.xa.
child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN NS ns2.child.parent.chld-found-inconsist-6.methodsv2.xa.
child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN NS ns1.parent.chld-found-inconsist-6.methodsv2.xa.

;; ADDITIONAL SECTION:
ns1.child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN A 127.40.1.51
ns1.child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN AAAA fda1:b2:c3:0:127:40:1:51
ns2.child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN A 127.40.1.52
ns2.child.parent.chld-found-inconsist-6.methodsv2.xa. 3600 IN AAAA fda1:b2:c3:0:127:40:1:52

Six ns/ip are expected from the unit test, but it only provides two:

$ ZONEMASTER_METHODSV2_SCENARIO='CHLD-FOUND-INCONSIST-6' ZONEMASTER_RECORD=1 PERL_DL_NONLAZY=1 "/usr/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'inc', 'blib/lib', 'blib/arch')" t/methodsv2.t
t/methodsv2.t .. 1/? 
        #   Failed test 'Nameserver 'ns1.child.parent.chld-found-inconsist-6.methodsv2.xa/127.40.1.51' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 320.
        # Expected but missing: ns1.child.parent.chld-found-inconsist-6.methodsv2.xa/127.40.1.51

        #   Failed test 'Nameserver 'ns1.child.parent.chld-found-inconsist-6.methodsv2.xa/fda1:b2:c3:0:127:40:1:51' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 320.
        # Expected but missing: ns1.child.parent.chld-found-inconsist-6.methodsv2.xa/fda1:b2:c3:0:127:40:1:51

        #   Failed test 'Nameserver 'ns2.child.parent.chld-found-inconsist-6.methodsv2.xa/127.40.1.52' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 320.
        # Expected but missing: ns2.child.parent.chld-found-inconsist-6.methodsv2.xa/127.40.1.52

        #   Failed test 'Nameserver 'ns2.child.parent.chld-found-inconsist-6.methodsv2.xa/fda1:b2:c3:0:127:40:1:52' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 320.
        # Expected but missing: ns2.child.parent.chld-found-inconsist-6.methodsv2.xa/fda1:b2:c3:0:127:40:1:52

        #   Failed test at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 327.
        # Number of nameservers in both arrays does not match (found 2, expected 6)
        # Looks like you failed 5 tests of 10.

    #   Failed test 'get_del_ns_names_and_ips'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 332.
(...)
matsduf commented 2 months ago

Another example, IB-NOT-IN-ZONE-1.

The address records are not defined in the zone, but the NS records are there and the delegation works. Lookup with the resolver:

$ dig @127.3.0.53 child.parent.ib-not-in-zone-1.methodsv2.xa ns
(...)
;; ANSWER SECTION:
child.parent.ib-not-in-zone-1.methodsv2.xa. 3600 IN NS ns2.child.parent.ib-not-in-zone-1.methodsv2.xa.
child.parent.ib-not-in-zone-1.methodsv2.xa. 3600 IN NS ns1.child.parent.ib-not-in-zone-1.methodsv2.xa.

The unit test does not report any name servers for the zone:

$ ZONEMASTER_METHODSV2_SCENARIO='ib-not-in-zone-1' ZONEMASTER_RECORD=1 PERL_DL_NONLAZY=1 "/usr/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'inc', 'blib/lib', 'blib/arch')" t/methodsv2.t
t/methodsv2.t .. 1/? 
        #   Failed test 'Nameserver 'ns1.child.parent.ib-not-in-zone-1.methodsv2.xa' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 320.
        # Expected but missing: ns1.child.parent.ib-not-in-zone-1.methodsv2.xa

        #   Failed test 'Nameserver 'ns2.child.parent.ib-not-in-zone-1.methodsv2.xa' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 320.
        # Expected but missing: ns2.child.parent.ib-not-in-zone-1.methodsv2.xa

        #   Failed test at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 327.
        # Number of nameservers in both arrays does not match (found 0, expected 2)
        # Looks like you failed 3 tests of 4.

    #   Failed test 'get_zone_ns_names_and_ips'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 332.
    # Looks like you failed 1 test of 7.
matsduf commented 2 months ago

The t/TestUtil cannot handle when the method returns an undefined value. Is it ever meaningful that is does? Or should it return an empty array instead?

$ ZONEMASTER_METHODSV2_SCENARIO='NO-CHILD-1' ZONEMASTER_RECORD=1 PERL_DL_NONLAZY=1 "/usr/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'inc', 'blib/lib', 'blib/arch')" t/methodsv2.t
t/methodsv2.t .. 1/? 
        #   Failed test 'Result is defined'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 297.
        # Unexpected undefined result
        # Looks like you failed 1 test of 1.

    #   Failed test 'get_parent_ns_ips'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 307.
    # Looks like you failed 1 test of 1.

#   Failed test 'NO-CHILD-1'
#   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 391.
Can't use an undefined value as an ARRAY reference at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 302.

NO-CHILD-2 has the same problem as NO-CHILD-1.

tgreenx commented 2 months ago

The t/TestUtil cannot handle when the method returns an undefined value. Is it ever meaningful that is does? Or should it return an empty array instead?

It can, but you need to pass an undefined value and not an arrayref of an undefined value, i.e.: undef instead of [ undef ] in t/methodsv2.t:

    'NO-CHILD-2' => [
        1,
        q(child.parent.no-child-2.methodsv2.xa),
        undef,
        [ ], # Empty
        [ ], # Empty
        [ ],
    ],
tgreenx commented 2 months ago

@matsduf Could you push all your changes to the t/methodsv2.t file in https://github.com/zonemaster/zonemaster-engine/pull/1351 so that I can start debugging too?

tgreenx commented 2 months ago

@matsduf Could you push all your changes to the t/methodsv2.t file in #1351 so that I can start debugging too?

Ah you just did. From what I can see in your latest commit you mistakenly defined empty sets everywhere instead of undefined values, as the specification states. See my review in https://github.com/zonemaster/zonemaster/pull/1254.

These are the currently specified return values of MethodV2 "Get parent NS IP addresses":

* A set of name server IP address for the parent zone:
  * Non-empty set: The name servers have been identified.
  * Empty set: Root zone or undelegated test.
  * Undefined set: The name servers cannot be determined due to errors in the
    delegation.
matsduf commented 2 months ago

It does not seem to work to specify undef in t/methodsv2.t.

tgreenx commented 2 months ago

It does not seem to work to specify undef in t/methodsv2.t.

Indeed, I suggested a fix. See: https://github.com/zonemaster/zonemaster-engine/pull/1351#pullrequestreview-2172582144

With that fix it seems to work properly now, and (enabled) unit tests pass:

$ prove t/methodsv2.t
t/methodsv2.t .. 23/? Untested scenarios:
        Scenario CHLD-FOUND-INCONSIST-6 cannot be tested.
        Scenario CHLD-FOUND-PAR-UNDET-1 cannot be tested.
        Scenario GOOD-6 cannot be tested.
        Scenario IB-NOT-IN-ZONE-1 cannot be tested.
        Scenario NO-CHLD-PAR-UNDETER-1 cannot be tested.
t/methodsv2.t .. ok
All tests successful.
Files=1, Tests=25,  9 wallclock secs ( 0.08 usr  0.00 sys +  8.25 cusr  0.16 csys =  8.49 CPU)
Result: PASS
matsduf commented 2 months ago

Scenarios DELEG-OOB-W-ERROR-1 -- 4 fail and output

Can't locate object method "query" via package "Zonemaster::Engine::DNSName" at /usr/local/share/perl/5.34.0/Zonemaster/Engine/TestMethodsV2.pm line 581.

E.g.

$ ZONEMASTER_SINGLE_SCENARIO="DELEG-OOB-W-ERROR-4" ZONEMASTER_RECORD=1 PERL_DL_NONLAZY=1 "/usr/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'inc', 'blib/lib', 'blib/arch')" t/methodsv2.t
t/methodsv2.t .. 1/?         # No tests run!

    #   Failed test 'No tests run for subtest "get_zone_ns_names_and_ips"'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 336.
    # Looks like you failed 1 test of 3.
t/methodsv2.t .. 4/? 
#   Failed test 'DELEG-OOB-W-ERROR-4'
#   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 399.
Can't locate object method "query" via package "Zonemaster::Engine::DNSName" at /usr/local/share/perl/5.34.0/Zonemaster/Engine/TestMethodsV2.pm line 581.

Also see #1351 which is complete now.

tgreenx commented 2 months ago

@matsduf I added a few commits in https://github.com/zonemaster/zonemaster-engine/pull/1351. Now, together with the latest changes in this PR, all unit tests pass.

@matsduf @mattias-p @marc-vanderwal @MichaelTimbert please review.

tgreenx commented 2 months ago

Add loop protection mechanism in Basic01 and MethodsV2 (currently the loop will break at 1000 iterations)

(Initial comment: https://github.com/zonemaster/zonemaster/pull/1287#issuecomment-2223108843)

This relates to this part of the specification: https://github.com/zonemaster/zonemaster/blob/7b85ac86264a51bb8754ee86b7dcad33215cd670/docs/public/specifications/tests/MethodsV2.md?plain=1#L181-L237

Currently one of such loop protection mechanism already existed in Engine, located in the recursive function and with a max value set at 20:

https://github.com/zonemaster/zonemaster-engine/blob/b889bcbbb5b2ca997f95e5e08e3167b48b975495/lib/Zonemaster/Engine/Recursor.pm#L218

I re-ran the numbers using all test zones defined in t/methodsv2.t from #1351, as well as all test zones defined in t/Test-basic01.t. In both cases, the maximum $loop_counter value is 1 (i.e. 2 iterations).

So it seems reasonable to lower the number from 1000 to 20, to match the existing value found in Zonemaster::Engine::Recursor->_recurse(). What do you all think?

marc-vanderwal commented 2 months ago

So it seems reasonable to lower the number from 1000 to 20, to match the existing value found in Zonemaster::Engine::Recursor->_recurse(). What do you all think?

There is a risk of aborting too early in some corner cases that probably sound convoluted, but are legal.

IPv6 reverse zones have domain names with many labels: domain names in ip6.arpa mapping to IPv6 addresses have 34 labels. Any of those labels could be delegation points. Resolving such a name to PTR could thus potentially mean to iterate over 34 delegation points.

Unless there is a best practice somewhere stating that the number of delegation points be limited to some reasonable number? Or is that number similar to what other “big” resolvers (e.g. BIND, Unbound, etc.) do?

matsduf commented 2 months ago

There is a risk both in too low and too high value. I suggest that we set the value to less than 100, maybe as low as 20, and then adjust the other value to be the same. If I read the code a message will be outputted if the level is reached. It is not expected to happen, so I suggest the message to be on DEBUG level. It could even be motivated to have it on WARNING level since it might severely affect the test result.

matsduf commented 2 months ago

All tests in #1351 pass. I suggest that this PR is merged and then #1351 is rebased and merged. After that some adjustments on loop control can be done for MethodsV2 implementation (this PR).

We should figure out a settings when the loop control is fired and add that as a unit test.

tgreenx commented 2 months ago

Changed versioning to V-Major since this PR now introduces optional arguments to existing public methods.

matsduf commented 2 months ago

Changed versioning to V-Major since this PR now introduces optional arguments to existing public methods.

@tgreenx, adding optional arguments is not breaking, is it? It should be V-Minor then.