Refactor ASNLookup code and documentation #1257

tgreenx · 2023-07-18T16:56:38Z

Purpose

This PR is four-fold:

It updates the ASN Lookup function of Zonemaster by using standard recursive DNS lookups instead of expecting dedicated zones.
It fixes the backup source functionality (see Corrects ASN lookup sources #1311 (comment))
It removes documentation and code for deprecated profile parameter asnroots
It updates documentation and unitary tests for profile parameters asn_db.style and asn_db.sources

Context

Fixes #1255, #1052, #1053, #835

Also follows up on #1311 (comment)

Requires #1288

Changes

Update code logic in Zonemaster::Engine::ASNLookup->_cymru_asn_lookup()
Add message tag ASN_LOOKUP_SOURCE (with level = DEBUG , module = System and testcase = Connectivity{03,04})
Fix backup source functionality in Zonemaster::Engine::ASNLookup
Remove asnroots profile property
Refactoring
Update documentation
Add and update unitary tests

How to test this PR

Tests should pass.

Manual testing:

Add ASNLookup servers fake.asnlookup.zonemaster.net and asnlookup.dufberg.se in the default profile (or just create a new one):

diff --git a/share/profile.json b/share/profile.json
index 2340c9fa..5ee8dc17 100644
  --- a/share/profile.json
  +++ b/share/profile.json
@@ -2,7 +2,7 @@
     "asn_db" : {
         "style" : "Cymru",
         "sources" : {
 -            "Cymru" : [ "asnlookup.zonemaster.net", "asn.cymru.com" ],
 +            "Cymru" : [ "fake.asnlookup.zonemaster.net", "asnlookup.dufberg.se" ],
             "RIPE" : [ "riswhois.ripe.net" ]
         }
     },

Start a test for Connectivity03 and Connectivity04: they should be able to reach the working ASN database server (asnlookup.dufberg.se), after skipping the non-working one (fake.asnlookup.zonemaster.net):

$ zonemaster-cli --show-module --show-testcase --test connectivity03 --test connectivity04 zonemaster.net --raw --level DEBUG | egrep -i "ASN_LOOKUP_SOURCE|_DIFFERENT_"
   1.11 DEBUG    System       Connectivity03 ASN_LOOKUP_SOURCE  name="fake.asnlookup.zonemaster.net"
  11.25 DEBUG    System       Connectivity03 ASN_LOOKUP_SOURCE  name="asnlookup.dufberg.se"
[ ... ]
  20.14 INFO     Connectivity Connectivity03 IPV4_DIFFERENT_ASN  asn_list=2486,8674,56908
  20.14 INFO     Connectivity Connectivity03 IPV6_DIFFERENT_ASN  asn_list=2486,8674,56908
   0.08 DEBUG    System       Connectivity04 ASN_LOOKUP_SOURCE  name="fake.asnlookup.zonemaster.net"
   0.08 DEBUG    System       Connectivity04 ASN_LOOKUP_SOURCE  name="asnlookup.dufberg.se"
[ ... ]
   0.09 INFO     Connectivity Connectivity04 CN04_IPV4_DIFFERENT_PREFIX  ns_list=ns2.nic.fr/192.93.0.4;nsa.dnsnode.net/194.58.192.46;nsp.dnsnode.net/194.58.198.32;nsu.dnsnode.net/185.42.137.98
   0.09 INFO     Connectivity Connectivity04 CN04_IPV6_DIFFERENT_PREFIX  ns_list=ns2.nic.fr/2001:660:3005:1::1:2;nsa.dnsnode.net/2a01:3f1:46::53;nsp.dnsnode.net/2a01:3f1:3032::53;nsu.dnsnode.net/2a01:3f0:400::32

matsduf · 2023-07-20T07:12:11Z

This is a breaking change, isn't it? Then it should be V-Major.

matsduf · 2023-07-20T07:25:01Z

lib/Zonemaster/Engine/Profile.pm

+For C<"Cymru">, the strings are domain names. For C<"RIPE">, they are whois servers. Normally only the first
+item in the list will be used, the rest are backups in case the previous ones didn't work.
+
+Default C<{Cymru: [ "asnlookup.zonemaster.net", "asn.cymru.com" ]}>.


In the current implementation, at least for Cymru style, only the first in the list is used. If a service has been completely removed then NXDOMAIN is returned, which is hard to distinguish from valid NXDOMAIN. Only for Cymru style the default value is included. For both I think it is better to refer the default value to to the default profile.json.

I am not sure I understand what you mean. Do you suggest having the full default value from the profile instead, i.e.:
Default C<{Cymru: [ "asnlookup.zonemaster.net", "asn.cymru.com" ], RIPE: [ "riswhois.ripe.net" ] }>

Just to clarify, at least in Cymru style the second in the list will never be used as far as I can see. I suggest that it documented, and that the second is removed (also in profile.json and profile.yaml.

My suggestion was that the doumentation in the Perl module does not list the defaults, but instead refer to the default profile, but that is not what we do in other cases.

But then it should be documented that riswhois.ripe.net is the default value if RIPE style is chosen.

My suggestion was that the doumentation in the Perl module does not list the defaults, but instead refer to the default profile, but that is not what we do in other cases.

But then it should be documented that riswhois.ripe.net is the default value if RIPE style is chosen.

Updated to: Default C<{Cymru: [ "asnlookup.zonemaster.net", "asn.cymru.com" ], RIPE: [ "riswhois.ripe.net" ] }>

In the current implementation, at least for Cymru style, only the first in the list is used. If a service has been completely removed then NXDOMAIN is returned, which is hard to distinguish from valid NXDOMAIN. Only for Cymru style the default value is included. For both I think it is better to refer the default value to to the default profile.json.

Just to clarify, at least in Cymru style the second in the list will never be used as far as I can see. I suggest that it documented, and that the second is removed (also in profile.json and profile.yaml.

I think we should probably update this part of the specification then, so that instead it reads something like:

5. If there is no response (timeout) or the DNS response does not have the RCODE "NOERROR", output *[ERROR_ASN_DATABASE]* and end these steps for Cymru look-up of the specific IP address. 6. If the DNS response contains an empty answer section, output *[EMPTY_ASN_SET]* and end these steps for Cymru look-up of the specific IP address.

What do you think?

matsduf · 2023-07-20T07:27:19Z

* It updates profile parameters "asn_db.style" and "asn_db.sources"

Is that an update of documentation only, or functional update?

tgreenx · 2023-07-20T08:33:50Z

* It updates profile parameters "asn_db.style" and "asn_db.sources"
Is that an update of documentation only, or functional update?

Documentation only. I have updated the description.

matsduf · 2023-07-20T08:47:45Z

Make ASNLookup.pm use standard recursive DNS lookup

I created asnlookup.dufberg.se, which has a DNAME to asnlookup.zonemaster.net:

% dig 178.111.124.3.origin.asnlookup.zonemaster.net TXT
(...)
;; ANSWER SECTION:
178.111.124.3.origin.asnlookup.zonemaster.net. 14311 IN	TXT "16509 | 3.124.0.0/14 | NA | NA | NA"

% dig 178.111.124.3.origin.asnlookup.dufberg.se TXT    

(...)
;; ANSWER SECTION:
asnlookup.dufberg.se.	809	IN	DNAME	asnlookup.zonemaster.net.
178.111.124.3.origin.asnlookup.dufberg.se. 809 IN CNAME	178.111.124.3.origin.asnlookup.zonemaster.net.
178.111.124.3.origin.asnlookup.zonemaster.net. 14256 IN	TXT "16509 | 3.124.0.0/14 | NA | NA | NA"

When I change to the asnlookup.dufberg.se Zonemaster, with this PR, will not do the lookup.

# zonemaster-cli --show-testcase --test connectivity/connectivity03 iis.se --level info
Seconds Level    Testcase       Message
======= ======== ============== =======
   0.00 INFO     UNSPECIFIED    Using version v4.7.0 of the Zonemaster engine.
   1.84 INFO     CONNECTIVITY03 At least two IPv4 addresses of the authoritative nameservers are announced by different AS sets. A merged list of all AS: (8674,56908).
   1.84 INFO     CONNECTIVITY03 At least two IPv6 addresses of the authoritative nameservers are announced by different AS sets. A merged list of all AS: (8674,56908).

# zonemaster-cli --show-testcase --test connectivity/connectivity03 iis.se --level info --profile /usr/local/etc/zonemaster/asn-profile.json
Loading profile from /usr/local/etc/zonemaster/asn-profile.json.

Seconds Level    Testcase       Message
======= ======== ============== =======
   0.00 INFO     UNSPECIFIED    Using version v4.7.0 of the Zonemaster engine.
   0.62 ERROR    CONNECTIVITY03 AS database returned no informations for IP address 185.42.137.98.
   0.69 ERROR    CONNECTIVITY03 AS database returned no informations for IP address 194.58.192.46.
   0.76 ERROR    CONNECTIVITY03 AS database returned no informations for IP address 194.58.198.32.
   0.83 ERROR    CONNECTIVITY03 AS database returned no informations for IP address 2a01:3f0:400::32.
   0.91 ERROR    CONNECTIVITY03 AS database returned no informations for IP address 2a01:3f1:3032::53.
   0.98 ERROR    CONNECTIVITY03 AS database returned no informations for IP address 2a01:3f1:46::53.

matsduf

No support for CNAME.

tgreenx · 2023-09-07T16:20:55Z

No support for CNAME.

So it seems that the recursive lookup function of Zonemaster does not follow CNAMEs. It stops at the first satisfying response.

I can update that but a lot of unit tests will break (due to missing data). But updating that data will also make a lot of them break, because some zones will have changed.

tgreenx · 2023-09-07T17:02:21Z

No support for CNAME.

So it seems that the recursive lookup function of Zonemaster does not follow CNAMEs. It stops at the first satisfying response.

I opened #1288 as to not overload this PR. I also rebased this PR on top of it.

tgreenx · 2023-10-24T16:41:02Z

I can update that but a lot of unit tests will break (due to missing data). But updating that data will also make a lot of them break, because some zones will have changed.

These unit tests are now fixed (in #1288). I rebased again on the latest commit of this PR.

tgreenx · 2024-05-28T07:58:19Z

Regarding the DEBUG message, it would currently be complex to do that but instead I can make one that it outputted whenever any ASN source is used (not just when switching from one to another). Would that be fine?

Yes, and that should be DEBUG as default. The message text should be very brief.

Done in commit d807920. Message tag is ASN_LOOKUP_SOURCE, untranslated and with one argument.

tgreenx · 2024-06-04T16:40:48Z

@matsduf @mattias-p please (re-)review

CNAME support has been added.

mattias-p

This looks good to me, thought I found a couple of nits looking through this again.

lib/Zonemaster/Engine/Profile.pm

lib/Zonemaster/Engine/ASNLookup.pm

t/profiles.t

- Update documentation - Update unit test

mattias-p

Great work!

matsduf

Looks fine.

marc-vanderwal · 2024-12-02T09:49:33Z

Release testing passed with no errors.

tgreenx added A-TestCase Area: Test case specification or implementation of test case V-Minor Versioning: The change gives an update of minor in version. labels Jul 18, 2023

tgreenx added this to the v2023.2 milestone Jul 18, 2023

tgreenx requested review from mattias-p, matsduf, vlevigneron, hannaeko, a user and marc-vanderwal July 18, 2023 16:56

tgreenx linked an issue Jul 18, 2023 that may be closed by this pull request

Use normal DNS lookup for ASN CONNECTIVITY03/04 #1255

Closed

tgreenx removed the A-TestCase Area: Test case specification or implementation of test case label Jul 18, 2023

tgreenx force-pushed the update-asnlookup branch from 253b4da to 0fc52ef Compare July 19, 2023 08:07

tgreenx changed the title ~~Refactor ASNLookup to use standard recursive DNS lookup~~ Refactor ASNLookup code and documentation Jul 19, 2023

This was linked to issues Jul 19, 2023

Remove deprecated key "asnroots" in profile #1052

Closed

Documentation of "asn_db" object in profile does not match implementation #1053

Closed

matsduf reviewed Jul 20, 2023

View reviewed changes

tgreenx added V-Major Versioning: The change gives an update of major in version. and removed V-Minor Versioning: The change gives an update of minor in version. labels Jul 20, 2023

matsduf previously requested changes Jul 20, 2023

View reviewed changes

tgreenx mentioned this pull request Sep 7, 2023

Add CNAME followage in Recursor #1288

Merged

tgreenx force-pushed the update-asnlookup branch from 9128434 to 0bc6bcd Compare September 7, 2023 17:01

tgreenx requested a review from matsduf September 7, 2023 17:03

tgreenx force-pushed the update-asnlookup branch from 0bc6bcd to 49e2420 Compare October 24, 2023 16:40

tgreenx mentioned this pull request Jun 4, 2024

Profile fixes #1356

Merged

tgreenx modified the milestones: v2024.1, v2024.2 Jun 12, 2024

tgreenx and others added 3 commits June 17, 2024 12:16

Merge branch 'develop' into update-asnlookup

9b3004e

Merge branch 'develop' into update-asnlookup

752e0fd

Restore deleted unit test for asn_db profile properties

49ed934

tgreenx requested a review from marc-vanderwal July 17, 2024 17:25

marc-vanderwal previously approved these changes Jul 18, 2024

View reviewed changes

mattias-p reviewed Jul 22, 2024

View reviewed changes

lib/Zonemaster/Engine/Profile.pm Outdated Show resolved Hide resolved

lib/Zonemaster/Engine/ASNLookup.pm Show resolved Hide resolved

t/profiles.t Show resolved Hide resolved

Update after review

582a500

- Update documentation - Update unit test

tgreenx dismissed marc-vanderwal’s stale review via 582a500 July 25, 2024 13:17

tgreenx requested review from marc-vanderwal and mattias-p July 25, 2024 13:21

tgreenx added 2 commits July 25, 2024 15:36

Refactoring

e57de32

Add new message tag ASN_LOOKUP_SOURCE to the profile

ec2ca1a

mattias-p approved these changes Jul 26, 2024

View reviewed changes

matsduf approved these changes Aug 21, 2024

View reviewed changes

marc-vanderwal approved these changes Sep 5, 2024

View reviewed changes

tgreenx merged commit 036baeb into zonemaster:develop Sep 17, 2024
3 checks passed

tgreenx deleted the update-asnlookup branch September 17, 2024 15:44

This was referenced Sep 17, 2024

Use normal DNS lookup for ASN CONNECTIVITY03/04 #1255

Closed

Remove deprecated key "asnroots" in profile #1052

Closed

Documentation of "asn_db" object in profile does not match implementation #1053

Closed

tgreenx linked an issue Sep 17, 2024 that may be closed by this pull request

Create unit test for riswhois in Connectivity03 #835

Closed

tgreenx mentioned this pull request Sep 17, 2024

Create unit test for riswhois in Connectivity03 #835

Closed

marc-vanderwal added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ASNLookup code and documentation #1257

Refactor ASNLookup code and documentation #1257

tgreenx commented Jul 18, 2023 •

edited

Loading

matsduf commented Jul 20, 2023

matsduf Jul 20, 2023

tgreenx Jul 20, 2023

matsduf Jul 20, 2023

tgreenx Nov 7, 2023

matsduf commented Jul 20, 2023

tgreenx commented Jul 20, 2023

matsduf commented Jul 20, 2023

matsduf left a comment

tgreenx commented Sep 7, 2023

tgreenx commented Sep 7, 2023 •

edited

Loading

tgreenx commented Oct 24, 2023 •

edited

Loading

tgreenx commented May 28, 2024

tgreenx commented Jun 4, 2024

mattias-p left a comment

mattias-p left a comment

matsduf left a comment

marc-vanderwal commented Dec 2, 2024

Refactor ASNLookup code and documentation #1257

Refactor ASNLookup code and documentation #1257

Conversation

tgreenx commented Jul 18, 2023 • edited Loading

Purpose

Context

Changes

How to test this PR

matsduf commented Jul 20, 2023

matsduf Jul 20, 2023

Choose a reason for hiding this comment

tgreenx Jul 20, 2023

Choose a reason for hiding this comment

matsduf Jul 20, 2023

Choose a reason for hiding this comment

tgreenx Nov 7, 2023

Choose a reason for hiding this comment

matsduf commented Jul 20, 2023

tgreenx commented Jul 20, 2023

matsduf commented Jul 20, 2023

matsduf left a comment

Choose a reason for hiding this comment

tgreenx commented Sep 7, 2023

tgreenx commented Sep 7, 2023 • edited Loading

tgreenx commented Oct 24, 2023 • edited Loading

tgreenx commented May 28, 2024

tgreenx commented Jun 4, 2024

mattias-p left a comment

Choose a reason for hiding this comment

mattias-p left a comment

Choose a reason for hiding this comment

matsduf left a comment

Choose a reason for hiding this comment

marc-vanderwal commented Dec 2, 2024

tgreenx commented Jul 18, 2023 •

edited

Loading

tgreenx commented Sep 7, 2023 •

edited

Loading

tgreenx commented Oct 24, 2023 •

edited

Loading