Fix default settings of queries

Due to an oversight in a previous refactoring, all non-DNSSEC DNS queries sent by Zonemaster became
EDNS queries. This commit makes it so that those queries are now non-EDNS queries. Default EDNS0 packet
size values will now be properly used when appropriate, and a new, missing one has been created for DNSSEC.
The caching logic of queries was also impacted.

Simply put, a DNSSEC query using the default EDNS0 packet size of 1232 is made by setting parameter "dnssec"
and/or "edns_details{do}" (the latter has precedence).
For a non-DNSSEC EDNS query, setting parameter "edns_size" and/or "edns_details{size}" (the latter has precedence)
will do the trick, but then it will use the provided value for the EDNS0 packet size. To use the default value of 512,
just set parameter "edns_details" with an empty hash (or non-empty with any of its subkey(s) other than edns_details{do,size})
instead.

- Fix logic related to flags dnssec and edns_size for when to use default values, and also when combined with edns_details
- Fix caching logic when using dnssec and edns_size parameters
- Make combined usage of dnssec with edns_details but without edns_details{do} to correctly set the dnssec (DO) flag
- Removed uneeded code related to resetting flags between queries (- it was already done earlier in the same method)
- Add constant UDP_DNSSEC_QUERY_DEFAULT (set to 1232)
- Rename constant UDP_COMMON_EDNS_LIMIT to UDP_EDNS_COMMON_LIMIT
- Add and update documentation
- Add unit tests

lib/Zonemaster/Engine/Constants.pm

@@ -88,8 +88,9 @@ our @EXPORT_OK = qw[
   $SERIAL_MAX_VARIATION
   $MINIMUM_NUMBER_OF_NAMESERVERS
   $UDP_PAYLOAD_LIMIT
+  $UDP_DNSSEC_QUERY_DEFAULT
   $UDP_EDNS_QUERY_DEFAULT
-  $UDP_COMMON_EDNS_LIMIT
+  $UDP_EDNS_COMMON_LIMIT
   @IPV4_SPECIAL_ADDRESSES
   @IPV6_SPECIAL_ADDRESSES
 ];
@@ -105,7 +106,7 @@ our %EXPORT_TAGS = (
     soa  => [
         qw($DURATION_5_MINUTES_IN_SECONDS $DURATION_1_HOUR_IN_SECONDS $DURATION_4_HOURS_IN_SECONDS $DURATION_12_HOURS_IN_SECONDS $DURATION_1_DAY_IN_SECONDS $DURATION_1_WEEK_IN_SECONDS $DURATION_180_DAYS_IN_SECONDS $SERIAL_BITS $SERIAL_MAX_VARIATION)
     ],
-    misc => [qw($UDP_PAYLOAD_LIMIT $UDP_EDNS_QUERY_DEFAULT $UDP_COMMON_EDNS_LIMIT $MINIMUM_NUMBER_OF_NAMESERVERS $BLACKLISTING_ENABLED)]
+    misc => [qw($UDP_PAYLOAD_LIMIT $UDP_DNSSEC_QUERY_DEFAULT $UDP_EDNS_QUERY_DEFAULT $UDP_EDNS_COMMON_LIMIT $MINIMUM_NUMBER_OF_NAMESERVERS $BLACKLISTING_ENABLED)]
     ,    # everything in %EXPORT_OK that isn't included in any of the other tags
     addresses => [qw(@IPV4_SPECIAL_ADDRESSES @IPV6_SPECIAL_ADDRESSES)],
 );
@@ -168,11 +169,15 @@ An integer, used to define the size of the serial number space, as defined in RF
 
 =item * C<$UDP_PAYLOAD_LIMIT>
 
-=item * C<$UDP_EDNS_QUERY_DEFAULT>
+=item * C<$UDP_DNSSEC_QUERY_DEFAULT>
+
+An integer, used to define the EDNS0 UDP packet size in DNSSEC queries.
 
-An integer, used to define the EDNS0 UDP packet size in EDNS queries.
+=item * C<$UDP_EDNS_COMMON_LIMIT>
+
+=item * C<$UDP_EDNS_QUERY_DEFAULT>
 
-=item * C<$UDP_COMMON_EDNS_LIMIT>
+An integer, used to define the EDNS0 UDP packet size in non-DNSSEC EDNS queries.
 
 =item * C<@IPV4_SPECIAL_ADDRESSES>
 
@@ -215,9 +220,10 @@ Readonly our $MINIMUM_NUMBER_OF_NAMESERVERS => 2;
 Readonly our $SERIAL_BITS => 32;
 Readonly our $SERIAL_MAX_VARIATION => 0;
 
-Readonly our $UDP_PAYLOAD_LIMIT      => 512;
-Readonly our $UDP_EDNS_QUERY_DEFAULT => 512;
-Readonly our $UDP_COMMON_EDNS_LIMIT  => 4_096;
+Readonly our $UDP_PAYLOAD_LIMIT        => 512;
+Readonly our $UDP_DNSSEC_QUERY_DEFAULT => 1232;
+Readonly our $UDP_EDNS_COMMON_LIMIT    => 4096;
+Readonly our $UDP_EDNS_QUERY_DEFAULT   => 512;
 
 Readonly::Array our @IPV4_SPECIAL_ADDRESSES => _extract_iana_ip_blocks($IP_VERSION_4);
 Readonly::Array our @IPV6_SPECIAL_ADDRESSES => _extract_iana_ip_blocks($IP_VERSION_6);

lib/Zonemaster/Engine/Nameserver.pm

@@ -166,7 +166,7 @@ sub _build_dns {
 
     $res->recurse( 0 );
     $res->dnssec( 0 );
-    $res->edns_size( $UDP_EDNS_QUERY_DEFAULT );
+    $res->edns_size( 0 );
 
     $res->retry( Zonemaster::Engine::Profile->effective->get( q{resolver.defaults.retry} ) );
     $res->retrans( Zonemaster::Engine::Profile->effective->get( q{resolver.defaults.retrans} ) );
@@ -225,7 +225,10 @@ sub query {
     my $dnssec    = $href->{dnssec}    // 0;
     my $usevc     = $href->{usevc}     // Zonemaster::Engine::Profile->effective->get( q{resolver.defaults.usevc} );
     my $recurse   = $href->{recurse}   // Zonemaster::Engine::Profile->effective->get( q{resolver.defaults.recurse} );
-    my $edns_size = $href->{edns_size} // $UDP_EDNS_QUERY_DEFAULT;
+
+    if ( exists $href->{edns_details} and exists $href->{edns_details}{do} ) {
+        $dnssec = $href->{edns_details}{do};
+    }
 
     # Fake a DS answer
     if ( $type eq 'DS' and $class eq 'IN' and $self->fake_ds->{ lc( $name ) } ) {
@@ -285,28 +288,22 @@ sub query {
     $md5->add( q{NAME}    , $name );
     $md5->add( q{TYPE}    , "\U$type" );
     $md5->add( q{CLASS}   , "\U$class" );
-
-    if ( exists $href->{edns_details} and exists $href->{edns_details}{do} ) {
-        $md5->add( q{DNSSEC} , $href->{edns_details}{do} );
-    }
-    else {
-        $md5->add( q{DNSSEC} , $dnssec );
-    }
-
+    $md5->add( q{DNSSEC}  , $dnssec );
     $md5->add( q{USEVC}   , $usevc );
     $md5->add( q{RECURSE} , $recurse );
 
+    my $edns_size = $href->{edns_size} // ( $dnssec ? $UDP_DNSSEC_QUERY_DEFAULT : 0 );
+
     if ( exists $href->{edns_details} ) {
         $md5->add( q{EDNS_VERSION}        , $href->{edns_details}{version} // 0 );
         $md5->add( q{EDNS_Z}              , $href->{edns_details}{z} // 0 );
         $md5->add( q{EDNS_EXTENDED_RCODE} , $href->{edns_details}{rcode} // 0 );
         $md5->add( q{EDNS_DATA}           , $href->{edns_details}{data} // q{} );
-        $md5->add( q{EDNS_UDP_SIZE}       , $href->{edns_details}{size} // $edns_size );
-    }
-    else {
-        $md5->add( q{EDNS_UDP_SIZE} , 0);
+        $edns_size = $href->{edns_details}{size} // ( $href->{edns_size} // ( $dnssec ? $UDP_DNSSEC_QUERY_DEFAULT : $UDP_EDNS_QUERY_DEFAULT ) );
     }
 
+    $md5->add( q{EDNS_UDP_SIZE} , $edns_size );
+
     my $idx = $md5->b64digest();
 
     my ( $in_cache, $p) = $self->cache->get_key( $idx );
@@ -406,11 +403,13 @@ sub _query {
     $flags{q{fallback}}  = $href->{q{fallback}}  // Zonemaster::Engine::Profile->effective->get( q{resolver.defaults.fallback} );
     $flags{q{recurse}}   = $href->{q{recurse}}   // Zonemaster::Engine::Profile->effective->get( q{resolver.defaults.recurse} );
     $flags{q{timeout}}   = $href->{q{timeout}}   // Zonemaster::Engine::Profile->effective->get( q{resolver.defaults.timeout} );
-    $flags{q{edns_size}} = $href->{q{edns_size}} // $UDP_EDNS_QUERY_DEFAULT;
 
     if ( exists $href->{edns_details} ) {
         $flags{q{dnssec}}    = $href->{edns_details}{do} // $flags{q{dnssec}};
-        $flags{q{edns_size}} = $href->{edns_details}{size} // $flags{q{edns_size}};
+        $flags{q{edns_size}} = $href->{edns_details}{size} // ( $href->{q{edns_size}} // ( $flags{q{dnssec}} ? $UDP_DNSSEC_QUERY_DEFAULT : $UDP_EDNS_QUERY_DEFAULT ) );
+    }
+    else {
+        $flags{q{edns_size}} = $href->{q{edns_size}} // ( $flags{q{dnssec}} ? $UDP_DNSSEC_QUERY_DEFAULT : 0 );
     }
 
     # Set flags for this query
@@ -447,6 +446,9 @@ sub _query {
     	    if ( exists $href->{edns_details}{do} ) {
                 $pkt->do($href->{edns_details}{do});
             }
+            elsif ( $flags{q{dnssec}} ) {
+                $pkt->do($flags{q{dnssec}});
+            }
     	    if ( exists $href->{edns_details}{size} ) {
                 $pkt->edns_size($href->{edns_details}{size});
             }
@@ -478,20 +480,13 @@ sub _query {
             }
         }
     }
-    push @{ $self->times }, ( time() - $before );
 
-    # Reset to defaults
-    foreach my $flag ( keys %flags ) {
-        # Except for any flag that is not configurable in the profile
-        unless ( grep( /^$flag$/, ( 'dnssec', 'edns_size' ) ) ) {
-            $self->dns->$flag( Zonemaster::Engine::Profile->effective->get( q{resolver.defaults.}.$flag ) );
-        }
-    }
+    push @{ $self->times }, ( time() - $before );
 
     if ( $res ) {
         my $p = Zonemaster::Engine::Packet->new( { packet => $res } );
         my $size = length( $p->data );
-        if ( $size > $UDP_COMMON_EDNS_LIMIT ) {
+        if ( $size > $UDP_EDNS_COMMON_LIMIT ) {
             my $command = sprintf q{dig @%s %s%s %s}, $self->address->short, $flags{dnssec} ? q{+dnssec } : q{},
               "$name", $type;
             Zonemaster::Engine->logger->add(
@@ -794,7 +789,7 @@ The retransmission interval.
 Set the DO flag in the query. Defaults to false.
 
 If set to true, it becomes an EDNS query.
-Value overridden by 'edns_details->do' (if also given). More details in 'edns_details' below.
+Value overridden by C<edns_details{do}> (if also given). More details in L<edns_details> below.
 
 =item debug
 
@@ -826,10 +821,11 @@ If set to true, prevents a server to be black-listed on a query in case there is
 
 =item edns_size
 
-Set the EDNS0 UDP maximum size. Defaults to 512.
+Set the EDNS0 UDP maximum size. Defaults to 0, or 512 if the query is an non-DNSSEC EDNS query,
+or 1232 if the query is a DNSSEC EDNS query.
 
-Used only when the query is an EDNS query. Does not enable on its own the query to be an EDNS query.
-Value overridden by 'edns_details->size' (if also given). More details in 'edns_details' below.
+Setting a value other than 0 will also implicitly enable the query to be an EDNS query.
+Value overridden by C<edns_details{size}> (if also given). More details in L<edns_details> below.
 
 =item edns_details
 
@@ -838,10 +834,10 @@ A hash. An empty hash or a hash with any keys below will enable the query to be
 The currently supported keys are 'version', 'z', 'do', 'rcode', 'size' and 'data'.
 See L<Zonemaster::LDNS::Packet> for more details (key names prefixed with 'edns_').
 
-Note that flag 'edns_size' also exists (see above) and has the same effect as 'edns_details->size', although the value of the 
+Note that flag L<edns_size> also exists (see above) and has the same effect as C<edns_details{size}>, although the value of the
 latter will take precedence if both are given.
 
-Similarly, note that flag 'dnssec' also exists (see above) and has the same effect as 'edns_details->do', although the value of the
+Similarly, note that flag L<dnssec> also exists (see above) and has the same effect as C<edns_details{do}>, although the value of the
 latter will take precedence if both are given.
 
 =back

t/nameserver.t

@@ -3,6 +3,7 @@ use Test::More;
 BEGIN { use_ok( 'Zonemaster::Engine::Nameserver' ); }
 use Zonemaster::Engine;
 use Zonemaster::Engine::Util;
+use Zonemaster::Engine::Constants qw( :misc );
 
 my $datafile = 't/nameserver.data';
 if ( not $ENV{ZONEMASTER_RECORD} ) {
@@ -136,6 +137,83 @@ is( $fail_p, undef, 'No return from broken server' );
 my ( $e ) = grep { $_->tag eq 'LOOKUP_ERROR' } @{ Zonemaster::Engine->logger->entries };
 isa_ok( $e, 'Zonemaster::Engine::Logger::Entry' );
 
+subtest 'dnssec, edns_size and edns_details{do, size} flags behavior for queries' => sub {
+    my $ns = new_ok( 'Zonemaster::Engine::Nameserver' => [ { name => 'd.nic.fr', address => '194.0.9.1' } ] );
+
+    my $p = $ns->query( 'fr', 'SOA' );
+    is( $ns->dns->dnssec, 0, 'dnssec flag is unset' );
+    is( $ns->dns->edns_size, 0, 'edns_size flag is unset' );
+    ok( !$p->has_edns and !$p->do, 'non-EDNS response received on query with all default parameters' );
+
+    $p = $ns->query( 'fr', 'SOA', { "dnssec" => 0 } );
+    is( $ns->dns->dnssec, 0, 'dnssec flag is unset' );
+    is( $ns->dns->edns_size, 0, 'edns_size flag is unset' );
+    ok( !$p->has_edns and !$p->do, 'non-EDNS response received on query with dnssec unset' );
+
+    # Note that the following tests also implicitly test that flags are correctly re-evaluated between
+    # each consecutive queries.
+
+    $p = $ns->query( 'fr', 'SOA', { "dnssec" => 1 } );
+    is( $ns->dns->dnssec, 1, 'dnssec flag is set' );
+    is( $ns->dns->edns_size, $UDP_DNSSEC_QUERY_DEFAULT, 'edns_size uses default DNSSEC query value' );
+    ok( $p->has_edns and $p->do, 'DNSSEC response received on query with dnssec set' );
+
+    $p = $ns->query( 'fr', 'SOA', { "edns_size" => 1000 );
+    is( $ns->dns->dnssec, 0, 'dnssec flag is unset' );
+    is( $ns->dns->edns_size, 1000, 'edns_size uses given value' );
+    ok( $p->has_edns and !$p->do, 'non-DNSSEC EDNS response received on query with edns_size set' );
+
+    $p = $ns->query( 'fr', 'SOA', { "dnssec" => 0, "edns_size" => 1001 } );
+    is( $ns->dns->dnssec, 0, 'dnssec flag is unset' );
+    is( $ns->dns->edns_size, 1001, 'edns_size uses given value instead of default for non-DNSSEC EDNS queries' );
+    ok( $p->has_edns and !$p->do, 'non-DNSSEC EDNS response received on query with dnssec unset and edns_size set' );
+
+    $p = $ns->query( 'fr', 'SOA', { "dnssec" => 1, "edns_size" => 1002 } );
+    is( $ns->dns->dnssec, 1, 'dnssec flag is set' );
+    is( $ns->dns->edns_size, 1002, 'edns_size uses given value instead of default for DNSSEC EDNS queries' );
+    ok( $p->has_edns and $p->do, 'DNSSEC response received on query with dnssec set and edns_size set' );
+
+    $p = $ns->query( 'fr', 'SOA', { "edns_details" => {} );
+    is( $ns->dns->dnssec, 0, 'dnssec flag is unset' );
+    is( $ns->dns->edns_size, $UDP_EDNS_QUERY_DEFAULT, 'edns_size uses default EDNS query value for non-DNSSEC EDNS queries' );
+    ok( $p->has_edns and !$p->do, 'non-DNSSEC EDNS response received on query with edns_details set' );
+
+    $p = $ns->query( 'fr', 'SOA', { "edns_details" => { "do" => 1 } );
+    is( $ns->dns->dnssec, 1, 'dnssec flag is also set via edns_details{do}' );
+    is( $ns->dns->edns_size, $UDP_DNSSEC_QUERY_DEFAULT, 'edns_size also uses default DNSSEC query value when set with edns_details{do}' );
+    ok( $p->has_edns and $p->do, 'DNSSEC response received on query with edns_details{do} set' );
+
+    $p = $ns->query( 'fr', 'SOA', { "edns_details" => { "size" => 900 } );
+    is( $ns->dns->dnssec, 0, 'dnssec flag is unset' );
+    is( $ns->dns->edns_size, 900, 'edns_size also uses given value when set with edns_details{size}' );
+    ok( $p->has_edns and !$p->do, 'non-DNSSEC EDNS response received on query with edns_details{size} set' );
+
+    $p = $ns->query( 'fr', 'SOA', { "edns_details" => { "size" => 0 } );
+    is( $ns->dns->dnssec, 0, 'dnssec flag is unset' );
+    is( $ns->dns->edns_size, 0, 'edns_size also uses given value when set with edns_details{size}' );
+    ok( $p->has_edns and !$p->do, 'non-DNSSEC EDNS response received on query even with edns_details{size} set to 0' );
+
+    $p = $ns->query( 'fr', 'SOA', { "dnssec" => 1, "edns_details" => { "do" => 0 } } );
+    is( $ns->dns->dnssec, 0, 'edns_details{do} takes precedence over dnssec for setting the dnssec flag' );
+    is( $ns->dns->edns_size, $UDP_EDNS_QUERY_DEFAULT, 'edns_size uses default EDNS query value when dnssec flag is unset by edns_details{do}' );
+    ok( $p->has_edns and !$p->do, 'non-DNSSEC EDNS response received on query with dnssec unset by edns_details{do}' );
+
+    $p = $ns->query( 'fr', 'SOA', { "edns_size" => 1003, "edns_details" => { "size" => 901 } } );
+    is( $ns->dns->dnssec, 0, 'dnssec flag is unset' );
+    is( $ns->dns->edns_size, 901, 'edns_details{size} takes precedence over edns_size for setting the edns_size flag' );
+    ok( $p->has_edns and !$p->do, 'non-DNSSEC EDNS response received on query with edns_size and edns_details{size} set' );
+
+    $p = $ns->query( 'fr', 'SOA', { "dnssec" => 1, "edns_size" => 1004, "edns_details" => { "size" => 0 } } );
+    is( $ns->dns->dnssec, 1, 'dnssec flag is set' );
+    is( $ns->dns->edns_size, 0, 'edns_size is unset' );
+    ok( $p->has_edns and $p->do, 'DNSSEC response received on query with dnssec set and even with edns_details{size} set to 0' );
+
+    $p = $ns->query( 'fr', 'SOA', { "dnssec" => 0, "edns_size" => 1005, "edns_details" => { "do" => 1, "size" => 0 } } );
+    is( $ns->dns->dnssec, 1, 'dnssec flag is set' );
+    is( $ns->dns->edns_size, 0, 'edns_size is unset' );
+    ok( $p->has_edns and $p->do, 'DNSSEC response received on query with dnssec set by edns_details{do} and even with edns_details{size} set to 0' );
+}
+
 if ( $ENV{ZONEMASTER_RECORD} ) {
     Zonemaster::Engine::Nameserver->save( $datafile );
 }