New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

- fixes dotse/zonemaster-engine#295 #355

Merged
merged 12 commits into from Dec 19, 2017

Conversation

Projects
None yet
3 participants
@vlevigneron
Contributor

vlevigneron commented Dec 7, 2017

No description provided.

@vlevigneron vlevigneron requested review from mattias-p and matsduf Dec 7, 2017

@vlevigneron

This comment has been minimized.

Show comment
Hide comment
@vlevigneron

vlevigneron Dec 7, 2017

Contributor

A new version of zonemaster-cli which support new features about fake delegations will follow...

Contributor

vlevigneron commented Dec 7, 2017

A new version of zonemaster-cli which support new features about fake delegations will follow...

vlevigneron added a commit to vlevigneron/zonemaster-cli that referenced this pull request Dec 7, 2017

@matsduf matsduf referenced this pull request Dec 7, 2017

Merged

Issue zonemaster engine 295 #63

@matsduf

matsduf requested changes Dec 7, 2017 edited

I think comments on PR zonemaster/zonemaster-cli#63 is relevant here too.

Show outdated Hide outdated lib/Zonemaster/Engine.pm Outdated
@mattias-p

Looks good to me. I have a few nits with regard to documentation.

@@ -99,6 +99,7 @@ sub recurse {
sub add_fake_delegation {

This comment has been minimized.

@mattias-p

mattias-p Dec 11, 2017

Contributor

The return value of this method has changed. Could you update the method documentation to state what values can be returned and what they mean? Perhaps also update the example to highlight that there is a meaningful return value.

Has been addressed.

@mattias-p

mattias-p Dec 11, 2017

Contributor

The return value of this method has changed. Could you update the method documentation to state what values can be returned and what they mean? Perhaps also update the example to highlight that there is a meaningful return value.

Has been addressed.

Show outdated Hide outdated lib/Zonemaster/Engine.pm Outdated
Show outdated Hide outdated lib/Zonemaster/Engine/Recursor.pm Outdated
Show outdated Hide outdated lib/Zonemaster/Engine/Recursor.pm Outdated
Show outdated Hide outdated lib/Zonemaster/Engine/Recursor.pm Outdated
@vlevigneron

This comment has been minimized.

Show comment
Hide comment
@vlevigneron

vlevigneron Dec 11, 2017

Contributor
Contributor

vlevigneron commented Dec 11, 2017

@mattias-p

Your added changes look good in general. I added a few comments on those.

I also added notes to old comments that have been addressed to make it possible to keep track of the review process. I left old comments that have not been addressed untouched.

Show outdated Hide outdated lib/Zonemaster/Engine.pm Outdated
Show outdated Hide outdated lib/Zonemaster/Engine.pm Outdated
Show outdated Hide outdated lib/Zonemaster/Engine/Recursor.pm Outdated
Will cache result of previous queries.
=item %fake_addresses_cache

This comment has been minimized.

@mattias-p

mattias-p Dec 15, 2017

Contributor

Please also mention that this is a class member.

Has been addressed.

@mattias-p

mattias-p Dec 15, 2017

Contributor

Please also mention that this is a class member.

Has been addressed.

Show outdated Hide outdated lib/Zonemaster/Engine/TestMethods.pm Outdated
Show outdated Hide outdated lib/Zonemaster/Engine/Recursor.pm Outdated
Show outdated Hide outdated lib/Zonemaster/Engine/TestMethods.pm Outdated
@mattias-p

Not sure why I couldn't add these comments to the last review. Working around by creating another one.

Show outdated Hide outdated lib/Zonemaster/Engine/TestMethods.pm Outdated
Show outdated Hide outdated lib/Zonemaster/Engine/TestMethods.pm Outdated
@mattias-p

This comment has been minimized.

Show comment
Hide comment
@mattias-p

mattias-p Dec 15, 2017

Contributor

I don't quite grasp Github's management of review comments. At first I added comments declaring what items had been addressed, but that didn't end up looking right. So instead I removed the "has been removed" comments and edited the original comments with a bold footer. Not sure how to make this review process better.

Contributor

mattias-p commented Dec 15, 2017

I don't quite grasp Github's management of review comments. At first I added comments declaring what items had been addressed, but that didn't end up looking right. So instead I removed the "has been removed" comments and edited the original comments with a bold footer. Not sure how to make this review process better.

@matsduf

This comment has been minimized.

Show comment
Hide comment
Contributor

matsduf commented Dec 15, 2017

@vlevigneron

This comment has been minimized.

Show comment
Hide comment
@vlevigneron

vlevigneron Dec 15, 2017

Contributor

@mattias-p Done unless for documentation of lib/Zonemaster/Engine/TestMethods.pm which is specified in https://github.com/dotse/zonemaster/blob/master/docs/specifications/tests/Methods.md . We should complete this link one day (we have plans to upgrade "Methods" algorithme, it could be done at this occasion).

Contributor

vlevigneron commented Dec 15, 2017

@mattias-p Done unless for documentation of lib/Zonemaster/Engine/TestMethods.pm which is specified in https://github.com/dotse/zonemaster/blob/master/docs/specifications/tests/Methods.md . We should complete this link one day (we have plans to upgrade "Methods" algorithme, it could be done at this occasion).

Show outdated Hide outdated lib/Zonemaster/Engine/Recursor.pm Outdated
@mattias-p

This comment has been minimized.

Show comment
Hide comment
@mattias-p

mattias-p Dec 15, 2017

Contributor

@vlevigneron Links from the test method API documentation of the test method specification documentation would be very helpful. With links to the specifications it's not necessary to describe the meaning of the return values yet again. But the data type of the return value is not obvious from the specifications.

Contributor

mattias-p commented Dec 15, 2017

@vlevigneron Links from the test method API documentation of the test method specification documentation would be very helpful. With links to the specifications it's not necessary to describe the meaning of the return values yet again. But the data type of the return value is not obvious from the specifications.

@vlevigneron

This comment has been minimized.

Show comment
Hide comment
@vlevigneron

vlevigneron Dec 17, 2017

Contributor

Sorry guys for this mess. It took me more time than expected to modify this sensitive part of the code I had not changed for years. I found a lighter way to fix the issue. If this version works (it should be) I guess it can be finally merged.

Contributor

vlevigneron commented Dec 17, 2017

Sorry guys for this mess. It took me more time than expected to modify this sensitive part of the code I had not changed for years. I found a lighter way to fix the issue. If this version works (it should be) I guess it can be finally merged.

@mattias-p

Good work coming up with the better solution!

Show outdated Hide outdated lib/Zonemaster/Engine/Recursor.pm Outdated
Show outdated Hide outdated lib/Zonemaster/Engine/Zone.pm Outdated
care to only look at as many entries as you really need.
be found will not be in this list. In this case, the list is lazy-loading, so take
care to only look at as many entries as you really need. In case of
undelegated tests and fake delegation the IP associated with name servers

This comment has been minimized.

@matsduf

matsduf Dec 18, 2017

Contributor

Are "undelegated tests" and "fake delegation" the same thing or different things?

@matsduf

matsduf Dec 18, 2017

Contributor

Are "undelegated tests" and "fake delegation" the same thing or different things?

be found will not be in this list. In this case, the list is lazy-loading, so take
care to only look at as many entries as you really need. In case of
undelegated tests and fake delegation the IP associated with name servers
for the tested zone will be the ones set by users (saved in

This comment has been minimized.

@matsduf

matsduf Dec 18, 2017

Contributor

When you write "name servers for the tested zone will be the ones set by users" I guess that you mean the in-bailiwick nameservers?

"In case of fake delegation (undelegated tests), the IP addresses associated with in-bailiwick nameservers will be the ones set by the user (saved in %Zonemaster::Engine::Recursor::fake_addresses_cache), instead of the ones found recursively. (For definition of 'in-bailiwick', see RFC 7719.)"

@matsduf

matsduf Dec 18, 2017

Contributor

When you write "name servers for the tested zone will be the ones set by users" I guess that you mean the in-bailiwick nameservers?

"In case of fake delegation (undelegated tests), the IP addresses associated with in-bailiwick nameservers will be the ones set by the user (saved in %Zonemaster::Engine::Recursor::fake_addresses_cache), instead of the ones found recursively. (For definition of 'in-bailiwick', see RFC 7719.)"

This comment has been minimized.

@vlevigneron

vlevigneron Dec 18, 2017

Contributor

The way namservers and there IP addresses are set for undelegated tests is explained in Zonemaster::Engine::add_fake_delegation and we will use these nameservers/IP addresses. I mean, that we will not do additional recurse queries to find addresses except the one done during add_fake_delegation method call. For isntance:

$ zonemaster-cli telia.com --ns dns2.telia.com --ns dns1.telia.com/145.239.76.199 --ns dns49.de.telia.net --show_module --locale en_US
Seconds Level     Module       Message
======= ========= ============ =======
   2.37 ERROR     SYSTEM       The fake delegation of domain telia.com includes an in-zone name server dns2.telia.com without mandatory glue (without IP address).
   0.54 ERROR     BASIC        Nameserver dns1.telia.com/145.239.76.199 did not return NS records. RCODE was REFUSED.
  12.13 WARNING   CONNECTIVITY All nameservers in the delegation have IPv6 addresses in the same AS (1299).
  12.34 NOTICE    CONSISTENCY  Parent has extra nameserver IP address(es) not listed at child (145.239.76.199).
  12.34 NOTICE    CONSISTENCY  Child has extra nameserver IP address(es) not listed at parent (204.70.57.242;81.228.10.67;81.228.11.67).
  12.41 NOTICE    DNSSEC       There are neither DS nor DNSKEY records for the zone.
  12.41 NOTICE    DNSSEC       The zone is not signed with DNSSEC.
  12.48 WARNING   DELEGATION   Nameserver dns1.telia.com response is not authoritative on UDP port 53.
  12.49 WARNING   DELEGATION   Nameserver dns1.telia.com response is not authoritative on TCP port 53.
  13.04 NOTICE    DELEGATION   Child has nameserver(s) not listed at parent (ns02.savvis.net).
  16.79 NOTICE    ZONE         SOA 'refresh' value (10800) is less than the recommended minimum (14400).
@vlevigneron

vlevigneron Dec 18, 2017

Contributor

The way namservers and there IP addresses are set for undelegated tests is explained in Zonemaster::Engine::add_fake_delegation and we will use these nameservers/IP addresses. I mean, that we will not do additional recurse queries to find addresses except the one done during add_fake_delegation method call. For isntance:

$ zonemaster-cli telia.com --ns dns2.telia.com --ns dns1.telia.com/145.239.76.199 --ns dns49.de.telia.net --show_module --locale en_US
Seconds Level     Module       Message
======= ========= ============ =======
   2.37 ERROR     SYSTEM       The fake delegation of domain telia.com includes an in-zone name server dns2.telia.com without mandatory glue (without IP address).
   0.54 ERROR     BASIC        Nameserver dns1.telia.com/145.239.76.199 did not return NS records. RCODE was REFUSED.
  12.13 WARNING   CONNECTIVITY All nameservers in the delegation have IPv6 addresses in the same AS (1299).
  12.34 NOTICE    CONSISTENCY  Parent has extra nameserver IP address(es) not listed at child (145.239.76.199).
  12.34 NOTICE    CONSISTENCY  Child has extra nameserver IP address(es) not listed at parent (204.70.57.242;81.228.10.67;81.228.11.67).
  12.41 NOTICE    DNSSEC       There are neither DS nor DNSKEY records for the zone.
  12.41 NOTICE    DNSSEC       The zone is not signed with DNSSEC.
  12.48 WARNING   DELEGATION   Nameserver dns1.telia.com response is not authoritative on UDP port 53.
  12.49 WARNING   DELEGATION   Nameserver dns1.telia.com response is not authoritative on TCP port 53.
  13.04 NOTICE    DELEGATION   Child has nameserver(s) not listed at parent (ns02.savvis.net).
  16.79 NOTICE    ZONE         SOA 'refresh' value (10800) is less than the recommended minimum (14400).

This comment has been minimized.

@vlevigneron

vlevigneron Dec 18, 2017

Contributor

Another example with out of bailiwick nameservers:

$ zonemaster-cli 200.193.193.in-addr.arpa --ns ns.lucky.net --ns ns.gu.kiev.ua/145.239.76.199 --locale en_US
Seconds Level     Message
======= ========= =======
   0.56 ERROR     Nameserver ns.gu.kiev.ua/145.239.76.199 did not return NS records. RCODE was REFUSED.
  27.06 NOTICE    Parent has extra nameserver IP address(es) not listed at child (145.239.76.199).
  27.14 NOTICE    There are neither DS nor DNSKEY records for the zone.
  27.14 NOTICE    The zone is not signed with DNSSEC.
  27.15 WARNING   Nameserver ns.gu.kiev.ua response is not authoritative on UDP port 53.
  27.15 WARNING   Nameserver ns.gu.kiev.ua response is not authoritative on TCP port 53.
  28.01 NOTICE    Nameserver ns.lucky.net/193.193.193.100 allow zone transfer using AXFR.
  28.80 NOTICE    SOA 'refresh' value (3600) is less than the recommended minimum (14400).
  28.80 NOTICE    SOA 'retry' value (900) is less than the recommended minimum (3600).
  28.94 NOTICE    No target (MX, A or AAAA record) to deliver e-mail for the domain name.
@vlevigneron

vlevigneron Dec 18, 2017

Contributor

Another example with out of bailiwick nameservers:

$ zonemaster-cli 200.193.193.in-addr.arpa --ns ns.lucky.net --ns ns.gu.kiev.ua/145.239.76.199 --locale en_US
Seconds Level     Message
======= ========= =======
   0.56 ERROR     Nameserver ns.gu.kiev.ua/145.239.76.199 did not return NS records. RCODE was REFUSED.
  27.06 NOTICE    Parent has extra nameserver IP address(es) not listed at child (145.239.76.199).
  27.14 NOTICE    There are neither DS nor DNSKEY records for the zone.
  27.14 NOTICE    The zone is not signed with DNSSEC.
  27.15 WARNING   Nameserver ns.gu.kiev.ua response is not authoritative on UDP port 53.
  27.15 WARNING   Nameserver ns.gu.kiev.ua response is not authoritative on TCP port 53.
  28.01 NOTICE    Nameserver ns.lucky.net/193.193.193.100 allow zone transfer using AXFR.
  28.80 NOTICE    SOA 'refresh' value (3600) is less than the recommended minimum (14400).
  28.80 NOTICE    SOA 'retry' value (900) is less than the recommended minimum (3600).
  28.94 NOTICE    No target (MX, A or AAAA record) to deliver e-mail for the domain name.

@vlevigneron vlevigneron merged commit 2ead987 into zonemaster:develop Dec 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment