Update implementation Zone09 #1109

tgreenx · 2022-07-27T10:33:51Z

Purpose

This PR proposes a new implementation for test case Zone09, as the specification was recently updated.

Context

Fixes #1103

Changes

(per commit)

Add new test messages
Add new implementation
Modify unitary tests

How to test this PR

Checks should pass.

…default Profile values

lib/Zonemaster/Engine/Test/Zone.pm

matsduf · 2022-07-28T15:32:03Z

lib/Zonemaster/Engine/Test/Zone.pm

+        my $p2 = $ns->query( $zone->name, q{MX}, { fallback => 0, usevc => 0 } );
+
+        if ( $p2 and $p2->tc ){
+            $p2 = $ns->query( $zone->name, q{MX}, { fallback => 0, usevc => 1 } );
+        }


This should be the default querying when the specification sends a default DNS query. Would it make sense to implement the default query as a method? (I do not mean necessarily in this PR.)

I am not sure yet. I need more clarification about the behavior of LDNS, because now I see that there is a 3rd flag that can influence TCP behavior. It is unclear to me how it behaves when all of those flags are set/unset simultaneously.

https://github.com/zonemaster/zonemaster-engine/blob/master/lib/Zonemaster/Engine/Profile.pm#L610-L613

=head2 resolver.defaults.igntc A boolean. If false, UDP queries that get responses with the C<TC> flag set will be automatically resent over TCP. Default false.

@mattias-p maybe you have some info on this?

Yes, it really looks like we want "resolver.defaults.igntc" to be set false by default, and then we should not need to worry. A test what would happen if the server responds with TC set over UDP and then not at all over TCP would be interesting.

I agree with you @matsduf that we ought to have a method that encapsulates the default way we make DNS requests. And I'd like to expand on this idea too. For each behavior in the specifications that deviate from the default, the caller should be able to easily override that in the method parameters. (I guess possibly it could be a DNS request API that consists of more than one method.)

I agree with you @tgreenx too that this is a bit of a mess. I've tried to make sense of these flags before, but I'm afraid I've lost those notes. Though we should really deprecated and remove all properties from the Profile that correspond to LDNS flags that the DNS request method wants to set/clear.

I created #1122 for to continue this discussion.

matsduf · 2022-08-01T11:04:29Z

I have created a test zone xa with real MX on one name server and no MX on the other. I do not expect any Z09_TLD_EMAIL_DOMAIN, but I get four (it is a TLD):

$ zonemaster-cli --raw --show-testcase --test zone/zone09 --level INFO xa --ns zonemaster01-prd.iis.se --ns zonemaster05-prd.iis.se
   0.00 INFO      UNSPECIFIED    GLOBAL_VERSION   version=v4.5.1
   0.34 WARNING   ZONE09         Z09_INCONSISTENT_MX   
   0.34 INFO      ZONE09         Z09_NO_MX_FOUND   ns_ip_list=2001:67c:124c:7316::85;45.155.96.85
   0.34 INFO      ZONE09         Z09_MX_FOUND   ns_ip_list=2001:67c:124c:7316::81;45.155.96.81
   0.34 WARNING   ZONE09         Z09_TLD_EMAIL_DOMAIN   
   0.34 WARNING   ZONE09         Z09_TLD_EMAIL_DOMAIN   
   0.34 WARNING   ZONE09         Z09_TLD_EMAIL_DOMAIN   
   0.34 WARNING   ZONE09         Z09_TLD_EMAIL_DOMAIN

matsduf · 2022-08-01T11:22:12Z

A new test zone, xb. The two servers have different data in MX. I expecte two (not four) Z09_MX_DATA with "ns_ip_list" (not with "ns").

$ zonemaster-cli --raw --show-testcase --test zone/zone09 --level INFO --ns zonemaster01-prd.iis.se --ns zonemaster05-prd.iis.se xb
   0.00 INFO      UNSPECIFIED    GLOBAL_VERSION   version=v4.5.1
   0.34 WARNING   ZONE09         Z09_TLD_EMAIL_DOMAIN   
   0.34 WARNING   ZONE09         Z09_TLD_EMAIL_DOMAIN   
   0.34 WARNING   ZONE09         Z09_INCONSISTENT_MX_DATA   
   0.34 INFO      ZONE09         Z09_MX_DATA   domain_list=mahimahi.ripe.net.;molamola.ripe.net.; ns_ip_list=45.155.96.85
   0.34 INFO      ZONE09         Z09_MX_DATA   domain_list=mx1.iis.se.;mx2.iis.se.; ns_ip_list=45.155.96.81
   0.34 INFO      ZONE09         Z09_MX_DATA   domain_list=mx1.iis.se.;mx2.iis.se.; ns_ip_list=2001:67c:124c:7316::81
   0.34 INFO      ZONE09         Z09_MX_DATA   domain_list=mahimahi.ripe.net.;molamola.ripe.net.; ns_ip_list=2001:67c:124c:7316::85

matsduf · 2022-08-01T11:31:37Z

New test zone, xc. One server has two normal MX and the the other has one normal and one Null-MX. I think the specification says that only the inconsistencies between the servers should be reported. If we should follow the implementation then the specification has to be updated.

Z09_NULL_MX_WITH_OTHER_MX is outputted twice, I only expected once.

$ zonemaster-cli --raw --show-testcase --test zone/zone09 --level INFO --ns zonemaster01-prd.iis.se --ns zonemaster05-prd.iis.se xc
   0.00 INFO      UNSPECIFIED    GLOBAL_VERSION   version=v4.5.1
   0.34 WARNING   ZONE09         Z09_TLD_EMAIL_DOMAIN   
   0.34 WARNING   ZONE09         Z09_NULL_MX_WITH_OTHER_MX   
   0.34 WARNING   ZONE09         Z09_NULL_MX_WITH_OTHER_MX   
   0.34 WARNING   ZONE09         Z09_TLD_EMAIL_DOMAIN   
   0.34 WARNING   ZONE09         Z09_INCONSISTENT_MX_DATA   
   0.34 INFO      ZONE09         Z09_MX_DATA   domain_list=.;molamola.ripe.net.; ns_ip_list=45.155.96.85
   0.34 INFO      ZONE09         Z09_MX_DATA   domain_list=.;molamola.ripe.net.; ns_ip_list=2001:67c:124c:7316::85
   0.34 INFO      ZONE09         Z09_MX_DATA   domain_list=mx1.iis.se.;mx2.iis.se.; ns_ip_list=45.155.96.81
   0.34 INFO      ZONE09         Z09_MX_DATA   domain_list=mx1.iis.se.;mx2.iis.se.; ns_ip_list=2001:67c:124c:7316::81

matsduf · 2022-08-01T11:45:19Z

Test zone xd. Both servers have the same MX, one correct Null-MX and one with preference 5. Too many messages.

$zonemaster-cli --raw --show-testcase --test zone/zone09 --level INFO --ns zonemaster01-prd.iis.se --ns zonemaster05-prd.iis.se xd
   0.00 INFO      UNSPECIFIED    GLOBAL_VERSION   version=v4.5.1
   0.34 WARNING   ZONE09         Z09_NULL_MX_WITH_OTHER_MX   
   0.34 WARNING   ZONE09         Z09_NULL_MX_WITH_OTHER_MX   
   0.34 NOTICE    ZONE09         Z09_NULL_MX_NON_ZERO_PREF   
   0.34 WARNING   ZONE09         Z09_NULL_MX_WITH_OTHER_MX   
   0.34 WARNING   ZONE09         Z09_NULL_MX_WITH_OTHER_MX   
   0.34 NOTICE    ZONE09         Z09_NULL_MX_NON_ZERO_PREF   
   0.34 WARNING   ZONE09         Z09_NULL_MX_WITH_OTHER_MX   
   0.34 WARNING   ZONE09         Z09_NULL_MX_WITH_OTHER_MX   
   0.34 NOTICE    ZONE09         Z09_NULL_MX_NON_ZERO_PREF   
   0.34 WARNING   ZONE09         Z09_NULL_MX_WITH_OTHER_MX   
   0.34 WARNING   ZONE09         Z09_NULL_MX_WITH_OTHER_MX   
   0.34 NOTICE    ZONE09         Z09_NULL_MX_NON_ZERO_PREF

matsduf

I have tested as reported in the comments. The test zones are available.

tgreenx · 2022-08-02T08:35:48Z

I have created a test zone xa with real MX on one name server and no MX on the other. I do not expect any Z09_TLD_EMAIL_DOMAIN, but I get four (it is a TLD):

[ ... ]

The specification says:

If Child Zone is a TLD with a non-Null MX then output Z09_TLD_EMAIL_DOMAIN.

So, this zone is a TLD, and If I understand correctly, there are indeed 2 non-null MX records (mx1.iis.se. and mx2.iis.se.) from nameserver zonemaster01-prd.iis.se (which has an IPv4 and an IPv6 address, hence the 4 messages right now). What am I missing then? Shouldn't this message tag be outputted?

matsduf · 2022-08-02T12:52:13Z

The specification says:

If Child Zone is a TLD with a non-Null MX then output Z09_TLD_EMAIL_DOMAIN.

In the specification there is first

5.  For each name server IP in *Name Server IP* do:

which loops around the name servers and saves information into the sets. Then there is

10. If the *MX RRset* set is non-empty, then do:
     1. If the RRsets in *MX RRset* are not equal for all name servers then do:
     (...)
     2. Else do:
      (...)
        2. If *Child Zone* is a [TLD] with a [non-Null MX][Null MX] then
          output *[Z09_TLD_EMAIL_DOMAIN]*.

and that is not meant to say that the message should be outputted once per name server. Should I clarify the specification? There is no meaning of outputting that message more than once.

Secondly, as I read the specification, if the MX data are not equal in all servers then you come to 1. If the RRsets in *MX RRset* are not equal for all name servers then do: and never to 2. Else do:.

When it comes to the second bullet one could have different opinions if the Z09_TLD_EMAIL_DOMAIN message should be outputted or not. If it should, then the specification should be updated, it not it should maybe be clarified?

So, this zone is a TLD, and If I understand correctly, there are indeed 2 non-null MX records (mx1.iis.se. and mx2.iis.se.) from nameserver zonemaster01-prd.iis.se (which has an IPv4 and an IPv6 address, hence the 4 messages right now). What am I missing then? Shouldn't this message tag be outputted?

No, that was not my expectation, and definitely not more than once.

tgreenx · 2022-08-02T13:23:30Z

I agree on the fact that having it outputted more than once was a mistake. It has been corrected in the latest commit (same goes for other message tags for other zones you mentioned earlier).

However I think that the specification do require an update then, because in our case, regarding zone xa:

It has two nameservers, only one of which (zonemaster01-prd.iis.se) has MX records (mx1.iis.se. and mx2.iis.se.).
Meaning that: MX RRset will contain those records (from nameserver zonemaster01-prd.iis.se) , while No MX RRset will contain the other nameserver (zonemaster05-prd.iis.se).
Thus: MX RRset is non-empty and No MX RRset isn't either. This validates step 9, and outputs the corresponding message tags. However this also validates step 10, as from there we look at the content of MX RRset exclusively (I think the problem arises from that). Step 10.1 evaluates to false, 10.2.1 as well, thus we end up in step 10.2.2, where Z09_TLD_EMAIL_DOMAIN is outputted (because zone xa is indeed a TLD).

matsduf · 2022-08-03T10:10:21Z

However I think that the specification do require an update then, because in our case, regarding zone xa:

I agree with your reading. There will be some extra messages, but only in case there is an inconsistency, so I think it is fine.

matsduf · 2022-08-03T10:14:00Z

I have created a test zone xa with real MX on one name server and no MX on the other. I do not expect any Z09_TLD_EMAIL_DOMAIN, but I get four (it is a TLD):

Now it looks fine for zone xa:

   0.00 INFO      UNSPECIFIED    GLOBAL_VERSION   version=v4.5.1
   0.35 WARNING   ZONE09         Z09_INCONSISTENT_MX   
   0.35 INFO      ZONE09         Z09_NO_MX_FOUND   ns_ip_list=2001:67c:124c:7316::85;45.155.96.85
   0.35 INFO      ZONE09         Z09_MX_FOUND   ns_ip_list=2001:67c:124c:7316::81;45.155.96.81
   0.35 WARNING   ZONE09         Z09_TLD_EMAIL_DOMAIN

matsduf · 2022-08-03T10:16:20Z

A new test zone, xb. The two servers have different data in MX. I expecte two (not four) Z09_MX_DATA with "ns_ip_list" (not with "ns").

I expected two Z09_MX_DATA, one per set, but I only get one (zone xb):

   0.00 INFO      UNSPECIFIED    GLOBAL_VERSION   version=v4.5.1
   0.34 WARNING   ZONE09         Z09_INCONSISTENT_MX_DATA   
   0.34 INFO      ZONE09         Z09_MX_DATA   domain_list=mx1.iis.se.;mx2.iis.se.;mahimahi.ripe.net.;molamola.ripe.net.;mx1.iis.se.;mx2.iis.se.;mahimahi.ripe.net.;molamola.ripe.net.; ns_ip_list=2001:67c:124c:7316::81;2001:67c:124c:7316::85;45.155.96.81;45.155.96.85

matsduf · 2022-08-03T10:18:55Z

New test zone, xc. One server has two normal MX and the the other has one normal and one Null-MX. I think the specification says that only the inconsistencies between the servers should be reported. If we should follow the implementation then the specification has to be updated.

Also here I expect two Z09_MX_DATA, one per set, but I only get one (zone xc):

   0.00 INFO      UNSPECIFIED    GLOBAL_VERSION   version=v4.5.1
   0.36 WARNING   ZONE09         Z09_INCONSISTENT_MX_DATA   
   0.37 INFO      ZONE09         Z09_MX_DATA   domain_list=mx2.iis.se.;mx1.iis.se.;molamola.ripe.net.;.;mx2.iis.se.;mx1.iis.se.;.;molamola.ripe.net.; ns_ip_list=2001:67c:124c:7316::81;2001:67c:124c:7316::85;45.155.96.81;45.155.96.85

matsduf · 2022-08-03T10:27:30Z

Test zone xd. Both servers have the same MX, one correct Null-MX and one with preference 5. Too many messages.

Now it looks fine (xd):

   0.00 INFO      UNSPECIFIED    GLOBAL_VERSION   version=v4.5.1
   0.34 WARNING   ZONE09         Z09_NULL_MX_WITH_OTHER_MX   
   0.34 NOTICE    ZONE09         Z09_NULL_MX_NON_ZERO_PREF

matsduf · 2022-08-03T10:34:21Z

I have also created the following test zones on the same server and report as expected:

xe
2.0.192.in-addr.arpa
100.51.198.in-addr.arpa
113.0.203.in-addr.arpa
normal-mx.xa
no-mx.xa
null-mx.xa

matsduf

profile_example.json is not updated, see Keep profile.json and profile_example.json alined or make profile_example.json minimal #1118
You can use my test zones, and I'll document that they must be kept
Besides the lack of separation between MX RRsets in Z09_MX_DATA I see no other issues.

… name)

tgreenx · 2022-08-03T14:52:23Z

* profile_example.json is not updated, see #1118

* You can use my test zones, and I'll document that they must be kept

* Besides the lack of separation between MX RRsets in Z09_MX_DATA I see no other issues.

All these should be solved now. I chose to add unitary tests for zones xa to xe for now, if you think the other zones you created deserve an unitary test let me know and I'll add them.

matsduf · 2022-08-04T12:40:42Z

profile_example.json is not updated, see Keep profile.json and profile_example.json alined or make profile_example.json minimal #1118

I created a PR #1120 that removes all message tags etc from profile_example.json, and if the PR is excepted, then the file should not be included here.

matsduf

It looks fine. Maybe there should be more scenarios in the unit tests to cover all messages, e.g. having root zones with normal MX, with Null MX and with no MX, respectively.

mattias-p · 2022-08-15T14:24:57Z

MANIFEST

+t/Test-zone09-A.data
+t/Test-zone09-A.t
+t/Test-zone09-B.data
+t/Test-zone09-B.t
+t/Test-zone09-C.data
+t/Test-zone09-C.t
+t/Test-zone09-D.data
+t/Test-zone09-D.t
+t/Test-zone09-E.data
+t/Test-zone09-E.t


It's really nice that you continue the use of an established naming convention! However, the set of messages checked in those files is also the set of messages we need to keep stable because other people are relying on them. If we add new tests with this naming convention (and do nothing else), we might believe those messages are also covered by the stability guarantee, and that's an unnecessary maintenance burden.

I guess it would be a good idea to create a list in zonemaster/zonemaster that lists all the messages that we need to keep stable. But that's not enough. It should be visible somehow in the test files which messages are stable and which aren't. Otherwise it's really easy to forget that some messages are stable when making and reviewing updates. Maybe we could just rename the old Test-XXXX-X.t to Stable-XXXX-X.t? Maybe there are better ideas?

I created #1123 so we don't need to stall this PR because of this discussion.

mattias-p

This looks good to me! Though I've opened up a couple of discussions in the comments that I don't think we should forget about. I guess I'll just create separate issues for those.

matsduf · 2022-08-31T13:36:22Z

@tgreenx, I think this can be merged.

tgreenx added 3 commits July 21, 2022 14:26

zonemaster#1103 - Add new message tags, remove old ones

9f4f9cc

zonemaster#1103 - Add new test case implementation

776f8c2

zonemaster#1103 - Modify unitary tests

03a64a4

tgreenx added the A-TestCase Area: Test case specification or implementation of test case label Jul 27, 2022

tgreenx added this to the v2022.2 milestone Jul 27, 2022

tgreenx requested review from mattias-p, matsduf, a user and marc-vanderwal July 27, 2022 10:33

tgreenx linked an issue Jul 27, 2022 that may be closed by this pull request

Implement updated zone09 #1103

Closed

tgreenx added 2 commits July 27, 2022 16:44

zonemaster#1103 - Second update to unitary tests

36dbe19

zonemaster#1103 - Explicitly resend MX query over TCP, regardless of …

cc9b3e6

…default Profile values

matsduf reviewed Jul 28, 2022

View reviewed changes

lib/Zonemaster/Engine/Test/Zone.pm Outdated Show resolved Hide resolved

zonemaster#1103 - Explicitly set fallback flag for MX query

5f5e2b4

tgreenx requested a review from matsduf July 28, 2022 13:20

zonemaster#1103 - Correct handling of requerying over TCP

3d7b643

matsduf reviewed Jul 28, 2022

View reviewed changes

lib/Zonemaster/Engine/Test/Zone.pm Outdated Show resolved Hide resolved

zonemaster#1103 - Correct (again) handling of requerying over TCP

5e9a1fe

tgreenx requested a review from matsduf July 28, 2022 15:18

matsduf reviewed Jul 28, 2022

View reviewed changes

matsduf requested changes Aug 1, 2022

View reviewed changes

1103 - Update to code logic

5082b6a

tgreenx requested a review from matsduf August 2, 2022 12:08

1103 - Update for message ID consistency

5b04b43

matsduf requested changes Aug 3, 2022

View reviewed changes

tgreenx added 3 commits August 3, 2022 14:03

zonemaster#1103 - Update profile_example

578ff6b

zonemaster#1103 - Changed Z09_MX_DATA output to be per nameserver (by…

1c14785

… name)

zonemaster#1103 - Add new unitary tests for Zone09

b4c96ff

zonemaster#1103 - Update MANIFEST

46c3357

tgreenx requested a review from matsduf August 3, 2022 14:55

matsduf approved these changes Aug 4, 2022

View reviewed changes

mattias-p reviewed Aug 15, 2022

View reviewed changes

mattias-p approved these changes Aug 15, 2022

View reviewed changes

This was referenced Aug 15, 2022

Encapsulate DNS request behavior in a method #1122

Open

Document stable message tags #1123

Closed

matsduf mentioned this pull request Aug 31, 2022

Add test for Null MX records (RFC 7505) zonemaster/zonemaster#754

Closed

tgreenx merged commit ae92553 into zonemaster:develop Aug 31, 2022

tgreenx deleted the patch#1103 branch August 31, 2022 14:53

tgreenx mentioned this pull request Aug 31, 2022

Implement updated zone09 #1103

Closed

tgreenx mentioned this pull request Dec 7, 2022

Fix logic in zone09 #1163

Merged

hannaeko added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Dec 7, 2022

matsduf added the V-Minor Versioning: The change gives an update of minor in version. label Dec 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update implementation Zone09 #1109

Update implementation Zone09 #1109

tgreenx commented Jul 27, 2022 •

edited

Loading

matsduf Jul 28, 2022

tgreenx Jul 28, 2022

tgreenx Jul 28, 2022

matsduf Aug 1, 2022

mattias-p Aug 15, 2022

mattias-p Aug 15, 2022

matsduf commented Aug 1, 2022

matsduf commented Aug 1, 2022

matsduf commented Aug 1, 2022

matsduf commented Aug 1, 2022

matsduf left a comment

tgreenx commented Aug 2, 2022

matsduf commented Aug 2, 2022

tgreenx commented Aug 2, 2022

matsduf commented Aug 3, 2022

matsduf commented Aug 3, 2022 •

edited

Loading

matsduf commented Aug 3, 2022

matsduf commented Aug 3, 2022

matsduf commented Aug 3, 2022

matsduf commented Aug 3, 2022

matsduf left a comment

tgreenx commented Aug 3, 2022 •

edited

Loading

matsduf commented Aug 4, 2022

matsduf left a comment

mattias-p Aug 15, 2022

mattias-p Aug 15, 2022

mattias-p left a comment

matsduf commented Aug 31, 2022

Update implementation Zone09 #1109

Update implementation Zone09 #1109

Conversation

tgreenx commented Jul 27, 2022 • edited Loading

Purpose

Context

Changes

How to test this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Aug 1, 2022

matsduf commented Aug 1, 2022

matsduf commented Aug 1, 2022

matsduf commented Aug 1, 2022

matsduf left a comment

Choose a reason for hiding this comment

tgreenx commented Aug 2, 2022

matsduf commented Aug 2, 2022

tgreenx commented Aug 2, 2022

matsduf commented Aug 3, 2022

matsduf commented Aug 3, 2022 • edited Loading

matsduf commented Aug 3, 2022

matsduf commented Aug 3, 2022

matsduf commented Aug 3, 2022

matsduf commented Aug 3, 2022

matsduf left a comment

Choose a reason for hiding this comment

tgreenx commented Aug 3, 2022 • edited Loading

matsduf commented Aug 4, 2022

matsduf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattias-p left a comment

Choose a reason for hiding this comment

matsduf commented Aug 31, 2022

tgreenx commented Jul 27, 2022 •

edited

Loading

matsduf commented Aug 3, 2022 •

edited

Loading

tgreenx commented Aug 3, 2022 •

edited

Loading