Skip to content
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.

Sign up for GitHub

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

Update implementation Zone09 #1109

Merged
merged 14 commits into from
Aug 31, 2022
Merged

Update implementation Zone09 #1109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9f4f9cc
#1103 - Add new message tags, remove old ones
tgreenx Jul 21, 2022
776f8c2
#1103 - Add new test case implementation
tgreenx Jul 27, 2022
03a64a4
#1103 - Modify unitary tests
tgreenx Jul 27, 2022
36dbe19
#1103 - Second update to unitary tests
tgreenx Jul 27, 2022
cc9b3e6
#1103 - Explicitly resend MX query over TCP, regardless of default Pr…
tgreenx Jul 28, 2022
5f5e2b4
#1103 - Explicitly set fallback flag for MX query
tgreenx Jul 28, 2022
3d7b643
#1103 - Correct handling of requerying over TCP
tgreenx Jul 28, 2022
5e9a1fe
#1103 - Correct (again) handling of requerying over TCP
tgreenx Jul 28, 2022
5082b6a
1103 - Update to code logic
tgreenx Aug 2, 2022
5b04b43
1103 - Update for message ID consistency
tgreenx Aug 2, 2022
578ff6b
#1103 - Update profile_example
tgreenx Aug 3, 2022
1c14785
#1103 - Changed Z09_MX_DATA output to be per nameserver (by name)
tgreenx Aug 3, 2022
b4c96ff
#1103 - Add new unitary tests for Zone09
tgreenx Aug 3, 2022
46c3357
#1103 - Update MANIFEST
tgreenx Aug 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions MANIFEST
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,16 @@ t/Test-syntax06-L.data
t/Test-syntax06-L.t
t/Test-zone.data
t/Test-zone.t
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
Comment on lines +277 to +286
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

t/translator.t
t/undelegated.data
t/undelegated.t
Expand Down
231 changes: 191 additions & 40 deletions lib/Zonemaster/Engine/Test/Zone.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ use Carp;
use List::MoreUtils qw[none];
use Locale::TextDomain qw[Zonemaster-Engine];
use Readonly;
use JSON::PP;

use Zonemaster::Engine::Profile;
use Zonemaster::Engine::Constants qw[:soa :ip];
use Zonemaster::Engine::Recursor;
Expand Down Expand Up @@ -140,9 +142,19 @@ sub metadata {
],
zone09 => [
qw(
NO_MX_RECORD
MX_RECORD_EXISTS
NO_RESPONSE_MX_QUERY
Z09_INCONSISTENT_MX
Z09_INCONSISTENT_MX_DATA
Z09_MISSING_MAIL_TARGET
Z09_MX_DATA
Z09_MX_FOUND
Z09_NON_AUTH_MX_RESPONSE
Z09_NO_MX_FOUND
Z09_NO_RESPONSE_MX_QUERY
Z09_NULL_MX_NON_ZERO_PREF
Z09_NULL_MX_WITH_OTHER_MX
Z09_ROOT_EMAIL_DOMAIN
Z09_TLD_EMAIL_DOMAIN
Z09_UNEXPECTED_RCODE_MX
TEST_CASE_END
TEST_CASE_START
)
Expand Down Expand Up @@ -182,14 +194,6 @@ Readonly my %TAG_DESCRIPTIONS => (
__x # ZONE:MNAME_IS_NOT_CNAME
'SOA \'mname\' value ({mname}) refers to a NS which is not an alias (CNAME).', @_;
},
NO_MX_RECORD => sub {
__x # ZONE:NO_MX_RECORD
'No target (MX, A or AAAA record) to deliver e-mail for the domain name.', @_;
},
MX_RECORD_EXISTS => sub {
__x # ZONE:MX_RECORD_EXISTS
'MX with mail target ({mailtarget_list}) exists for the domain name.', @_;
},
REFRESH_MINIMUM_VALUE_LOWER => sub {
__x # ZONE:REFRESH_MINIMUM_VALUE_LOWER
'SOA \'refresh\' value ({refresh}) is less than the recommended one ({required_refresh}).', @_;
Expand Down Expand Up @@ -296,6 +300,58 @@ Readonly my %TAG_DESCRIPTIONS => (
__x # ZONE:WRONG_SOA
'Nameserver {ns} responds with a wrong owner name ({owner} instead of {name}) on SOA queries.', @_;
},
Z09_INCONSISTENT_MX => sub {
__x # ZONE:Z09_INCONSISTENT_MX
'Some name servers return an MX RRset while others return none.', @_;
},
Z09_INCONSISTENT_MX_DATA => sub {
__x # ZONE:Z09_INCONSISTENT_MX_DATA
'The MX RRset data is inconsistent between the name servers.', @_;
},
Z09_MISSING_MAIL_TARGET => sub {
__x # ZONE:Z09_MISSING_MAIL_TARGET
'The child zone has no mail target (no MX).', @_;
},
Z09_MX_DATA => sub {
__x # ZONE:Z09_MX_DATA
'Mail targets in the MX RRset "{domain_list}" returned from name servers "{ns_ip_list}".', @_;
},
Z09_MX_FOUND => sub {
__x # ZONE:Z09_MX_FOUND
'MX RRset was returned by name servers "{ns_ip_list}".', @_;
},
Z09_NON_AUTH_MX_RESPONSE => sub {
__x # ZONE:Z09_NON_AUTH_MX_RESPONSE
'Non-authoritative response on MX query from name servers "{ns_ip_list}".', @_;
},
Z09_NO_MX_FOUND => sub {
__x # ZONE:Z09_NO_MX_FOUND
'No MX RRset was returned by name servers "{ns_ip_list}".', @_;
},
Z09_NO_RESPONSE_MX_QUERY => sub {
__x # ZONE:Z09_NO_RESPONSE_MX_QUERY
'No response on MX query from name servers "{ns_ip_list}".', @_;
},
Z09_NULL_MX_NON_ZERO_PREF => sub {
__x # ZONE:Z09_NULL_MX_NON_ZERO_PREF
'The zone has a Null MX with non-zero preference.', @_;
},
Z09_NULL_MX_WITH_OTHER_MX => sub {
__x # ZONE:Z09_NULL_MX_WITH_OTHER_MX
'The zone has a Null MX mixed with other MX records.', @_;
},
Z09_ROOT_EMAIL_DOMAIN => sub {
__x # ZONE:Z09_ROOT_EMAIL_DOMAIN
'Root zone with an unexpected MX RRset (non-Null MX).', @_;
},
Z09_TLD_EMAIL_DOMAIN => sub {
__x # ZONE:Z09_TLD_EMAIL_DOMAIN
'The zone is a TLD and has an unexpected MX RRset (non-Null MX).', @_;
},
Z09_UNEXPECTED_RCODE_MX => sub {
__x # ZONE:Z09_UNEXPECTED_RCODE_MX
'Unexpected RCODE value on the MX query from name servers "{ns_ip_list}".', @_;
},
);

sub tag_descriptions {
Expand Down Expand Up @@ -650,44 +706,139 @@ sub zone08 {
sub zone09 {
my ( $class, $zone ) = @_;
push my @results, info( TEST_CASE_START => { testcase => (split /::/, (caller(0))[3])[-1] } );
my $info;

my $p = $zone->query_auth( $zone->name, q{MX} );
my %ip_already_processed;

my @no_response_mx;
my %unexpected_rcode_mx;
my @non_authoritative_mx;
my @no_mx_set;
my %mx_set;

if ( $p ) {
if ( not $p->has_rrs_of_type_for_name( q{MX}, $zone->name ) ) {
my $p_a = _retrieve_record_from_zone( $zone, $zone->name, q{A} );
my $p_aaaa = _retrieve_record_from_zone( $zone, $zone->name, q{AAAA} );
if (
( not defined $p_a and not defined $p_aaaa )
or ( ( not defined $p_a or not $p_a->has_rrs_of_type_for_name( q{A}, $zone->name ) )
and ( not defined $p_aaaa or not $p_aaaa->has_rrs_of_type_for_name( q{AAAA}, $zone->name ) ) )
)
{
push @results, info( NO_MX_RECORD => {} );
my %all_ns;

foreach my $ns ( @{ Zonemaster::Engine::TestMethods->method4and5( $zone ) } ){

next if exists $ip_already_processed{$ns->address->short};
$ip_already_processed{$ns->address->short} = 1;

if ( _is_ip_version_disabled( $ns ) ) {
next;
}

my $p1 = $ns->query( $zone->name, q{SOA} );

if ( not $p1 or $p1->rcode ne q{NOERROR} or not $p1->aa or not $p1->has_rrs_of_type_for_name(q{SOA}, $zone->name) ){
next;
}

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 } );
}
Comment on lines +735 to +739
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #1122 for to continue this discussion.


if ( not $p2 ){
push @no_response_mx, $ns->address->short;
}
elsif ( $p2->rcode ne q{NOERROR} ){
push @{ $unexpected_rcode_mx{$p2->rcode} }, $ns->address->short;
}
elsif ( not $p2->aa ){
push @non_authoritative_mx, $ns->address->short;
}
elsif ( not scalar grep { $_->owner eq $zone->name } $p2->get_records_for_name(q{MX}, $zone->name, q{answer}) ){
push @no_mx_set, $ns->address->short;
}
else{
push @{ $mx_set{$ns->address->short} }, $p2->get_records_for_name(q{MX}, $zone->name, q{answer});
}

push @{ $all_ns{$ns->name->string} }, $ns->address->short;
}

if ( scalar @no_response_mx ){
push @results, info( Z09_NO_RESPONSE_MX_QUERY => { ns_ip_list => join( q{;}, sort @no_response_mx) } );
}

if ( scalar %unexpected_rcode_mx ){
foreach my $rcode ( keys %unexpected_rcode_mx ){
push @results, info( Z09_UNEXPECTED_RCODE_MX => {
rcode => $rcode,
ns_ip_list => join( q{;}, sort $unexpected_rcode_mx{$rcode} )
}
);
}
}

if ( scalar @non_authoritative_mx ){
push @results, info( Z09_NON_AUTH_MX_RESPONSE => { ns_ip_list => join( q{;}, sort @no_response_mx) } );
}

if ( scalar @no_mx_set and scalar %mx_set ){
push @results, info( Z09_INCONSISTENT_MX => {} );
push @results, info( Z09_NO_MX_FOUND => { ns_ip_list => join( q{;}, sort @no_mx_set) } );
push @results, info( Z09_MX_FOUND => { ns_ip_list => join( q{;}, sort keys %mx_set) } );
}

if ( scalar %mx_set ){
my $data_json;
my $json = JSON::PP->new->canonical->pretty;
my $first = 1;

foreach my $ns ( keys %mx_set ){
if ( $first ){
my @data = map { lc $_->string } sort @{ $mx_set{$ns} };
$data_json = $json->encode( \@data );
$first = 0;
}
else {
my @as = defined $p_a ? $p_a->get_records_for_name( q{A}, $zone->name ) : ();
my @aaas = defined $p_aaaa ? $p_aaaa->get_records_for_name( q{AAAA}, $zone->name ) : ();
$info = join q{/}, map { $_ =~ /:/smx ? q{AAAA=} . $_->address : q{A=} . $_->address } ( @as, @aaas );
else{
my @next_data = map { lc $_->string } sort @{ $mx_set{$ns} };
if ( $json->encode( \@next_data ) ne $data_json ){
push @results, info( Z09_INCONSISTENT_MX_DATA => {} );

foreach my $ns_name ( keys %all_ns ){
push @results, info( Z09_MX_DATA => {
domain_list => join( q{;}, map { $_->exchange } @{ $mx_set{@{$all_ns{$ns_name}}[0]} } ),
ns_ip_list => join( q{;}, @{ $all_ns{$ns_name} } )
}
)
}

last;
}
}
}
else {
my @mx = $p->get_records_for_name( q{MX}, $zone->name );
for my $mx ( @mx ) {
my $tmp = $mx->exchange;
$tmp =~ s/[.]\z//smx;
$info .= $tmp . q{;};

unless ( grep{$_->tag eq 'Z09_INCONSISTENT_MX_DATA'} @results ){
foreach my $ns ( keys %mx_set ){
foreach my $rr ( @{$mx_set{$ns}} ){
if ( $rr->exchange eq '.' ){
if ( scalar @{$mx_set{$ns}} > 1 ){
push @results, info( Z09_NULL_MX_WITH_OTHER_MX => {} ) unless grep{$_->tag eq 'Z09_NULL_MX_WITH_OTHER_MX'} @results;
}

if ( $rr->preference > 0 ){
push @results, info( Z09_NULL_MX_NON_ZERO_PREF => {} ) unless grep{$_->tag eq 'Z09_NULL_MX_NON_ZERO_PREF'} @results;
}
}

elsif ( $zone->name->string eq '.' ){
push @results, info( Z09_ROOT_EMAIL_DOMAIN => {} ) unless grep{$_->tag eq 'Z09_ROOT_EMAIL_DOMAIN'} @results;
}

elsif ( $zone->name->next_higher() eq '.' ){
push @results, info( Z09_TLD_EMAIL_DOMAIN => {} ) unless grep{$_->tag eq 'Z09_TLD_EMAIL_DOMAIN'} @results;
}
}
}
chop $info;
}
}

if ( not grep { $_->tag ne q{TEST_CASE_START} } @results ) {
push @results, info( MX_RECORD_EXISTS => { mailtarget_list => $info } );
if ( scalar @no_mx_set ){
unless ( $zone->name eq '.' or $zone->name->next_higher() eq '.' or $zone->name =~ /\.arpa/ ){
push @results, info( Z09_MISSING_MAIL_TARGET => {} );
}
} ## end if ( $p )
else {
push @results, info( NO_RESPONSE_MX_QUERY => {} );
}

return ( @results, info( TEST_CASE_END => { testcase => (split /::/, (caller(0))[3])[-1] } ) )
Expand Down
17 changes: 14 additions & 3 deletions share/profile.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,10 +417,8 @@
"MNAME_NO_RESPONSE" : "NOTICE",
"MNAME_RECORD_DOES_NOT_EXIST" : "WARNING",
"MULTIPLE_SOA" : "ERROR",
"MX_RECORD_EXISTS" : "INFO",
"MX_RECORD_IS_CNAME" : "INFO",
"MX_RECORD_IS_NOT_CNAME" : "INFO",
"NO_MX_RECORD" : "NOTICE",
"NO_RESPONSE" : "DEBUG",
"NO_RESPONSE_MX_QUERY" : "DEBUG",
"NO_RESPONSE_SOA_QUERY" : "DEBUG",
Expand All @@ -437,7 +435,20 @@
"SOA_DEFAULT_TTL_MAXIMUM_VALUE_OK" : "INFO",
"TEST_CASE_END" : "DEBUG",
"TEST_CASE_START" : "DEBUG",
"WRONG_SOA" : "DEBUG"
"WRONG_SOA" : "DEBUG",
"Z09_INCONSISTENT_MX" : "WARNING",
"Z09_INCONSISTENT_MX_DATA" : "WARNING",
"Z09_MISSING_MAIL_TARGET" : "NOTICE",
"Z09_MX_DATA" : "INFO",
"Z09_MX_FOUND" : "INFO",
"Z09_NON_AUTH_MX_RESPONSE" : "WARNING",
"Z09_NO_MX_FOUND" : "INFO",
"Z09_NO_RESPONSE_MX_QUERY" : "WARNING",
"Z09_NULL_MX_NON_ZERO_PREF" : "NOTICE",
"Z09_NULL_MX_WITH_OTHER_MX" : "WARNING",
"Z09_ROOT_EMAIL_DOMAIN" : "NOTICE",
"Z09_TLD_EMAIL_DOMAIN" : "WARNING",
"Z09_UNEXPECTED_RCODE_MX" : "WARNING"
}
},
"test_cases": [
Expand Down
17 changes: 14 additions & 3 deletions share/profile_example.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,10 +453,8 @@
"MNAME_NO_RESPONSE" : "NOTICE",
"MNAME_RECORD_DOES_NOT_EXIST" : "WARNING",
"MULTIPLE_SOA" : "ERROR",
"MX_RECORD_EXISTS" : "INFO",
"MX_RECORD_IS_CNAME" : "INFO",
"MX_RECORD_IS_NOT_CNAME" : "INFO",
"NO_MX_RECORD" : "NOTICE",
"NO_RESPONSE" : "DEBUG",
"NO_RESPONSE_MX_QUERY" : "DEBUG",
"NO_RESPONSE_SOA_QUERY" : "DEBUG",
Expand All @@ -473,7 +471,20 @@
"SOA_DEFAULT_TTL_MAXIMUM_VALUE_OK" : "INFO",
"TEST_CASE_END" : "DEBUG",
"TEST_CASE_START" : "DEBUG",
"WRONG_SOA" : "DEBUG"
"WRONG_SOA" : "DEBUG",
"Z09_INCONSISTENT_MX" : "WARNING",
"Z09_INCONSISTENT_MX_DATA" : "WARNING",
"Z09_MISSING_MAIL_TARGET" : "NOTICE",
"Z09_MX_DATA" : "INFO",
"Z09_MX_FOUND" : "INFO",
"Z09_NON_AUTH_MX_RESPONSE" : "WARNING",
"Z09_NO_MX_FOUND" : "INFO",
"Z09_NO_RESPONSE_MX_QUERY" : "WARNING",
"Z09_NULL_MX_NON_ZERO_PREF" : "NOTICE",
"Z09_NULL_MX_WITH_OTHER_MX" : "WARNING",
"Z09_ROOT_EMAIL_DOMAIN" : "NOTICE",
"Z09_TLD_EMAIL_DOMAIN" : "WARNING",
"Z09_UNEXPECTED_RCODE_MX" : "WARNING"
}
},
"test_cases": [
Expand Down
Loading