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

Fix default settings of queries #1397

Merged
merged 9 commits into from
Nov 27, 2024
Merged

Fix default settings of queries #1397

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
92080b1
Fix default settings of queries
tgreenx Nov 19, 2024
d9cf0e7
Add maximum value check for edns_size parameter in Zonemaster::Engine…
tgreenx Nov 21, 2024
c9bcc37
Add minimum value check for edns_size parameter in Zonemaster::Engine…
tgreenx Nov 21, 2024
80a4261
Update unit tests
tgreenx Nov 21, 2024
a2d6e64
Update unit tests and unit tests data
tgreenx Nov 21, 2024
f8f826d
Update after review
tgreenx Nov 25, 2024
e116d4d
Minor updates in unit test for clearer output
tgreenx Nov 26, 2024
50287a1
Fix conflict on unit test data
tgreenx Nov 27, 2024
4ef9276
Update unit test data
tgreenx Nov 27, 2024
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
24 changes: 15 additions & 9 deletions lib/Zonemaster/Engine/Constants.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,9 @@ our @EXPORT_OK = qw[
$SERIAL_MAX_VARIATION
$MINIMUM_NUMBER_OF_NAMESERVERS
$UDP_PAYLOAD_LIMIT
$UDP_EDNS_QUERY_DEFAULT
$UDP_COMMON_EDNS_LIMIT
$EDNS_UDP_PAYLOAD_DNSSEC_DEFAULT
$EDNS_UDP_PAYLOAD_DEFAULT
$EDNS_UDP_PAYLOAD_COMMON_LIMIT
@IPV4_SPECIAL_ADDRESSES
@IPV6_SPECIAL_ADDRESSES
];
Expand All @@ -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 $EDNS_UDP_PAYLOAD_DNSSEC_DEFAULT $EDNS_UDP_PAYLOAD_DEFAULT $EDNS_UDP_PAYLOAD_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)],
);
Expand Down Expand Up @@ -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<$EDNS_UDP_PAYLOAD_DEFAULT>

An integer, used to define the EDNS0 UDP packet size in EDNS queries.
An integer, used to define the EDNS0 UDP packet size in non-DNSSEC EDNS queries.

=item * C<$UDP_COMMON_EDNS_LIMIT>
=item * C<$EDNS_UDP_PAYLOAD_COMMON_LIMIT>

=item * C<$EDNS_UDP_PAYLOAD_DNSSEC_DEFAULT>

An integer, used to define the EDNS0 UDP packet size in DNSSEC queries.

=item * C<@IPV4_SPECIAL_ADDRESSES>

Expand Down Expand Up @@ -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 $EDNS_UDP_PAYLOAD_DEFAULT => 512;
Readonly our $EDNS_UDP_PAYLOAD_COMMON_LIMIT => 4096;
Readonly our $EDNS_UDP_PAYLOAD_DNSSEC_DEFAULT => 1232;

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);
Expand Down
64 changes: 31 additions & 33 deletions lib/Zonemaster/Engine/Nameserver.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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} ) );
Expand Down Expand Up @@ -225,7 +225,12 @@ 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};
}

my $edns_size = $href->{edns_size} // ( $dnssec ? $EDNS_UDP_PAYLOAD_DNSSEC_DEFAULT : 0 );

# Fake a DS answer
if ( $type eq 'DS' and $class eq 'IN' and $self->fake_ds->{ lc( $name ) } ) {
Expand Down Expand Up @@ -285,14 +290,7 @@ 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 );

Expand All @@ -301,12 +299,13 @@ sub query {
$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 ? $EDNS_UDP_PAYLOAD_DNSSEC_DEFAULT : $EDNS_UDP_PAYLOAD_DEFAULT ) );
}

croak "edns_size (or edns_details->size) parameter must be a value between 0 and 65535" if $edns_size > 65535 or $edns_size < 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
croak "edns_size (or edns_details->size) parameter must be a value between 0 and 65535" if $edns_size > 65535 or $edns_size < 0;
croak "edns_size (or edns_details->size) parameter must be 0 (no EDNS) or a value between 512 and 65535" if $edns_size != 0 and ($edns_size > 65535 or $edns_size < 512);

Copy link
Contributor

Choose a reason for hiding this comment

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

Today we have no use cases for values 1-511. Any such value would today be a mistake and it would be better to catch those. For EDNS 0-511 is defined to mean the same thing as 512. If we ever want to check for servers handling small values we could always change the logic.

One could also argue that high values would never be used. We have today no use cases for high values. From that perspective it could be argued that the code should set a limit to catch mistakes.

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'm not sure that it is really needed to have such "subjective" checks. These are still "valid" values in the sense that the underlying library (LDNS) will accept them. I don't think we should impose those to us, or to any other user of Zonemaster-Engine.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.


$md5->add( q{EDNS_UDP_SIZE} , $edns_size );

my $idx = $md5->b64digest();

my ( $in_cache, $p) = $self->cache->get_key( $idx );
Expand Down Expand Up @@ -406,11 +405,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}} ? $EDNS_UDP_PAYLOAD_DNSSEC_DEFAULT : $EDNS_UDP_PAYLOAD_DEFAULT ) );
}
else {
$flags{q{edns_size}} = $href->{q{edns_size}} // ( $flags{q{dnssec}} ? $EDNS_UDP_PAYLOAD_DNSSEC_DEFAULT : 0 );
}

# Set flags for this query
Expand Down Expand Up @@ -447,6 +448,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});
}
Expand Down Expand Up @@ -478,20 +482,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 > $EDNS_UDP_PAYLOAD_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(
Expand Down Expand Up @@ -773,7 +770,7 @@ Remove all cached nameserver objects and queries.

Send a DNS query to the nameserver the object represents. C<$name> and C<$type> are the name and type that will be queried for (C<$type> defaults
to 'A' if it's left undefined). C<$flagref> is a reference to a hash, the keys of which are flags and the values are their corresponding values.
The available flags are as follows. All but 'class' and 'edns_details' directly correspond to methods in the L<Zonemaster::LDNS::Resolver> object.
The available flags are as follows. All but 'class' and 'edns_details' directly correspond to methods in the L<Zonemaster::LDNS> object.

=over

Expand All @@ -794,7 +791,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

Expand Down Expand Up @@ -826,22 +823,23 @@ 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. The value must be comprised between 0 and 65535.
Defaults to 0, or 512 if the query is a non-DNSSEC EDNS query, or 1232 if the query is a DNSSEC query.
Comment on lines +826 to +827
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Set the EDNS0 UDP maximum size. The value must be comprised between 0 and 65535.
Defaults to 0, or 512 if the query is a non-DNSSEC EDNS query, or 1232 if the query is a DNSSEC query.
Set the EDNS0 UDP requestor's maximum size. The value must be 0 or comprised between 512 and 65535 (inclusive).
Defaults to 0 (non-EDNS query), or 512 (non-DNSSEC EDNS query), or 1232 (DNSSEC 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 EDNS for the query.
Value overridden by C<edns_details-E<gt>{size}> (if also given). More details in L<edns_details> below.

=item edns_details

A hash. An empty hash or a hash with any keys below will enable the query to be an EDNS query.
A hash. An empty hash or a hash with any keys below will enable EDNS for the query.

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-E<gt>{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-E<gt>{do}>, although the value of the
latter will take precedence if both are given.

=back
Expand Down
Loading
Loading