Fix default settings of queries #1397

tgreenx · 2024-11-19T18:30:06Z

Purpose

This PR proposes a fix to the default settings of queries which, due to an oversight regarding the edns_size flag from #1147, made all non-DNSSEC queries to be EDNS queries instead of regular 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 to align with current specification. The caching logic of queries was also impacted.

The basic usage for creating DNSSEC and EDNS queries remains unchanged.

Context

Fixes zonemaster/zonemaster#1308

Changes

Fix logic related to flags dnssecand 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
Add upper and lower bound checks for edns_size
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
Update unit tests data

How to test this PR

Units tests have been added or updated and should pass. Note that most of the newly added unit tests cannot be properly tested when replaying unit test data files (i.e without ZONEMASTER_RECORD=1), as the actual tested object is the dns attribute of a Zonemaster::Engine::Nameserver object and its properties are not changed when getting query responses directly from the cache.

Manual testing can be done using the commands below:

Default DNS query

$ perl -MZonemaster::Engine -E 'my $ns = Zonemaster::Engine->ns("d.nic.fr", "194.0.9.1"); my $p = $ns->query("fr", "A"); say "dnssec (DO) flag in query: ", $ns->dns->dnssec ? "true" : "false"; say "edns_size in query: ", $ns->dns->edns_size; say "DO flag in response: ", $p->do ? "true": "false"; say "has_edns in response: ", $p->has_edns ? "true": "false"';
dnssec (DO) flag in query: false
edns_size in query: 0
DO flag in response: false
has_edns in response: false

An optional hash can be passed as parameter to the query method, with keys such as dnssec, edns_size and edns_details. They should be changed and/or removed with various combinaisons to see the different behaviors.

Default DNSSEC query

$ perl -MZonemaster::Engine -E 'my $ns = Zonemaster::Engine->ns("d.nic.fr", "194.0.9.1"); my $p = $ns->query("fr", "A", { "dnssec" => 1 }); say "dnssec in query: ", $ns->dns->dnssec ? "true" : "false"; say "edns_size in query: ", $ns->dns->edns_size; say "DO flag in response: ", $p->do ? "true": "false"; say "has_edns in response: ", $p->has_edns ? "true": "false"';
dnssec in query: true
edns_size in query: 1232
DO flag in response: true
has_edns in response: true

Default EDNS query

$ perl -MZonemaster::Engine -E 'my $ns = Zonemaster::Engine->ns("d.nic.fr", "194.0.9.1"); my $p = $ns->query("fr", "A", { "edns_details" => {} }); say "dnssec in query: ", $ns->dns->dnssec ? "true" : "false"; say "edns_size in query: ", $ns->dns->edns_size; say "DO flag in response: ", $p->do ? "true": "false"; say "has_edns in response: ", $p->has_edns ? "true": "false"';
dnssec in query: false
edns_size in query: 512
DO flag in response: false
has_edns in response: true

matsduf · 2024-11-19T22:30:41Z

lib/Zonemaster/Engine/Nameserver.pm

+    if ( $flags{q{dnssec}} ) {
+        $flags{q{edns_size}} = $href->{q{edns_size}} // $UDP_EDNS_QUERY_DEFAULT;
+    }
+    else {
+        $flags{q{edns_size}} = $href->{q{edns_size}} // 0;
+    }


The value must no be larger than 64 kB. If the value is smaller than 512 it must be treated as 512. Maybe we will create a test case where it is set smaller.

Is it possible by this code to create a non-DNSSEC EDNS query without specifying the size and get the default?

I've entirely reworked this PR, it should now align with our expectations. Please see commit 2566bcc (description and unit tests, in particular)

I agree that it doesn’t make much sense to set a value larger than 65 535. I suggest to croak in this situation.

However, if the argument is 512 or lower, we have two options:

be a good citizen, not expose bugs in other people’s nameservers, log a warning on our side and force the value to be 512;

or, on the contrary, allow ourselves to send EDNS queries with silly buffer size values and see how other servers respond to that.

How should we proceed?

I've added a check for both lower (0) and upper (65535) bounds for this parameter.

tgreenx · 2024-11-20T18:56:55Z

Unit tests data need to be re-recorded. I'll switch this PR to a draft state in the meantime.

marc-vanderwal

Looking pretty good already. Just some minor issues with the updated POD.

lib/Zonemaster/Engine/Nameserver.pm

marc-vanderwal · 2024-11-21T07:02:26Z

lib/Zonemaster/Engine/Nameserver.pm

+    if ( $flags{q{dnssec}} ) {
+        $flags{q{edns_size}} = $href->{q{edns_size}} // $UDP_EDNS_QUERY_DEFAULT;
+    }
+    else {
+        $flags{q{edns_size}} = $href->{q{edns_size}} // 0;
+    }


I agree that it doesn’t make much sense to set a value larger than 65 535. I suggest to croak in this situation.

However, if the argument is 512 or lower, we have two options:

be a good citizen, not expose bugs in other people’s nameservers, log a warning on our side and force the value to be 512;

or, on the contrary, allow ourselves to send EDNS queries with silly buffer size values and see how other servers respond to that.

How should we proceed?

matsduf · 2024-11-21T11:22:34Z

The EDNS UDP buffer size is a 16-bit value. If the code tries to set something larger than that I think the code should "crash". This would only come from bad (Zonemaster) code.

If we ever want to set something lower than 512 we must permit that. We do not have such use case today, so I suggest that the code should "crash" so that we do not try to do the wrong thing.

marc-vanderwal

I’ve made bad suggestions and I apologize. It turns out that the > in those -> I made you add needed escaping. My new suggestions are correct: I’ve actually tested them with perldoc.

lib/Zonemaster/Engine/Nameserver.pm

tgreenx · 2024-11-21T18:10:44Z

@matsduf @marc-vanderwal @mattias-p This PR is now ready for review. Unit tests have been re-recorded where applicable, comments have been adressed, and it has been rebased on top of #1398 (commit e9060d1) so that the CI can pass.

matsduf

Besides some detailed comments it looks fine.

lib/Zonemaster/Engine/Constants.pm

matsduf · 2024-11-22T10:41:56Z

lib/Zonemaster/Engine/Nameserver.pm

    }

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


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

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.

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.

lib/Zonemaster/Engine/Nameserver.pm

matsduf · 2024-11-22T11:07:29Z

lib/Zonemaster/Engine/Nameserver.pm

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


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

The merge-base changed after approval.

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

…::Nameserver->query() The maximum value for this parameter, set either by "edns_size" or "edns_details->size", is a 16-bit value, thus it should not exceed 65535. Documentation and unit tests are updated too.

…::Nameserver->query() This parameter, set either by "edns_size" or "edns_details->size", is an unsigned 16-bit value, thus the minimum value should be 0. Documentation and unit tests are updated too.

Use a different query name for each query as to make sure that we do not get a cached response from previous, equivalent queries. This is needed because the tested object is actually the underlying object in the "dns" attribute of the Zonemaster::Engine::Nameserver class, which is only updated by the "_query()" method, and not the "query()" method.

Renamed some constants: - UDP_DNSSEC_QUERY_DEFAULT to EDNS_UDP_PAYLOAD_DNSSEC_DEFAULT - UDP_EDNS_QUERY_DEFAULT to EDNS_UDP_PAYLOAD_DEFAULT - UDP_EDNS_COMMON_LIMIT to EDNS_UDP_PAYLOAD_COMMON_LIMIT

tgreenx · 2024-11-27T13:12:28Z

@matsduf I fixed a conflict and rebased on latest develop, please re-review.

tgreenx added T-Bug Type: Bug in software or error in test case description V-Patch Versioning: The change gives an update of patch in version. labels Nov 19, 2024

tgreenx added this to the v2024.2 milestone Nov 19, 2024

tgreenx requested review from mattias-p, matsduf, marc-vanderwal and MichaelTimbert November 19, 2024 18:30

tgreenx linked an issue Nov 19, 2024 that may be closed by this pull request

Initial request for NS set when the auth. nameservers refuse EDNS zonemaster/zonemaster#1308

Closed

tgreenx changed the title ~~Fix default EDNS behavior~~ Fix queries default EDNS behavior Nov 19, 2024

matsduf reviewed Nov 19, 2024

View reviewed changes

tgreenx force-pushed the fix-edns branch from 24fa248 to 3347218 Compare November 20, 2024 18:50

tgreenx marked this pull request as draft November 20, 2024 18:58

tgreenx force-pushed the fix-edns branch from 3347218 to 2566bcc Compare November 20, 2024 19:06

marc-vanderwal reviewed Nov 21, 2024

View reviewed changes

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

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

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

tgreenx requested review from matsduf and marc-vanderwal November 21, 2024 15:06

tgreenx force-pushed the fix-edns branch 2 times, most recently from fbc11e9 to 52bc120 Compare November 21, 2024 18:04

tgreenx marked this pull request as ready for review November 21, 2024 18:11

tgreenx added V-Minor Versioning: The change gives an update of minor in version. and removed V-Patch Versioning: The change gives an update of patch in version. labels Nov 21, 2024

tgreenx changed the title ~~Fix queries default EDNS behavior~~ Fix default settings of queries Nov 21, 2024

matsduf reviewed Nov 22, 2024

View reviewed changes

tgreenx requested a review from matsduf November 25, 2024 16:44

matsduf previously approved these changes Nov 25, 2024

View reviewed changes

matsduf previously approved these changes Nov 26, 2024

View reviewed changes

tgreenx dismissed matsduf’s stale review via 68ce0ec November 26, 2024 15:10

tgreenx requested a review from matsduf November 26, 2024 15:10

matsduf previously approved these changes Nov 26, 2024

View reviewed changes

tgreenx added 8 commits November 27, 2024 13:08

Add maximum value check for edns_size parameter in Zonemaster::Engine…

d9cf0e7

…::Nameserver->query() The maximum value for this parameter, set either by "edns_size" or "edns_details->size", is a 16-bit value, thus it should not exceed 65535. Documentation and unit tests are updated too.

Add minimum value check for edns_size parameter in Zonemaster::Engine…

c9bcc37

…::Nameserver->query() This parameter, set either by "edns_size" or "edns_details->size", is an unsigned 16-bit value, thus the minimum value should be 0. Documentation and unit tests are updated too.

Update unit tests

80a4261

Update after review

f8f826d

Renamed some constants: - UDP_DNSSEC_QUERY_DEFAULT to EDNS_UDP_PAYLOAD_DNSSEC_DEFAULT - UDP_EDNS_QUERY_DEFAULT to EDNS_UDP_PAYLOAD_DEFAULT - UDP_EDNS_COMMON_LIMIT to EDNS_UDP_PAYLOAD_COMMON_LIMIT

Minor updates in unit test for clearer output

e116d4d

Fix conflict on unit test data

50287a1

tgreenx dismissed matsduf’s stale review via 50287a1 November 27, 2024 13:12

tgreenx force-pushed the fix-edns branch from 68ce0ec to 50287a1 Compare November 27, 2024 13:12

tgreenx requested a review from matsduf November 27, 2024 13:13

Update unit test data

4ef9276

matsduf previously approved these changes Nov 27, 2024

View reviewed changes

tgreenx dismissed matsduf’s stale review via 4ef9276 November 27, 2024 13:31

tgreenx merged commit 760f9ea into zonemaster:develop Nov 27, 2024
3 checks passed

tgreenx deleted the fix-edns branch November 27, 2024 13:50

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

This was referenced Dec 3, 2024

Fixes broken unit tests zonemaster/zonemaster-backend#1192

Merged

Initial request for NS set when the auth. nameservers refuse EDNS zonemaster/zonemaster#1308

Closed

Bug: Basic02 sends queries with EDNS when it should not #1314

Closed

This was linked to issues Dec 9, 2024

Bug: Basic02 sends queries with EDNS when it should not #1314

Closed

invalid edns queries #1313

Closed

tgreenx mentioned this pull request Dec 9, 2024

invalid edns queries #1313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix default settings of queries #1397

Fix default settings of queries #1397

tgreenx commented Nov 19, 2024 •

edited

Loading

matsduf Nov 19, 2024

tgreenx Nov 20, 2024 •

edited

Loading

marc-vanderwal Nov 21, 2024

tgreenx Nov 21, 2024

tgreenx commented Nov 20, 2024

marc-vanderwal left a comment

marc-vanderwal Nov 21, 2024

matsduf commented Nov 21, 2024

marc-vanderwal left a comment

tgreenx commented Nov 21, 2024

matsduf left a comment

matsduf Nov 22, 2024

matsduf Nov 22, 2024

tgreenx Nov 25, 2024

matsduf Nov 25, 2024

matsduf Nov 22, 2024

tgreenx commented Nov 27, 2024 •

edited

Loading

		}

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

		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.

Fix default settings of queries #1397

Fix default settings of queries #1397

Conversation

tgreenx commented Nov 19, 2024 • edited Loading

Purpose

Context

Changes

How to test this PR

Choose a reason for hiding this comment

tgreenx Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgreenx commented Nov 20, 2024

marc-vanderwal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Nov 21, 2024

marc-vanderwal left a comment

Choose a reason for hiding this comment

tgreenx commented Nov 21, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgreenx commented Nov 27, 2024 • edited Loading

tgreenx commented Nov 19, 2024 •

edited

Loading

tgreenx Nov 20, 2024 •

edited

Loading

tgreenx commented Nov 27, 2024 •

edited

Loading