Fix EDNS behavior for queries #1147

tgreenx · 2022-11-16T15:35:51Z

Purpose

This PR fixes bugs regarding EDNS behavior in Engine.

For future reference (i.e., what this PR does):

EDNS queries will now be correctly enabled using any edns_details parameters ("version", "z", "do", "size", etc), or when none are given (empty hash). The main issue before this PR was that using edns_details->version = 0 would not enable the query as an EDNS query. See

zonemaster-engine/lib/Zonemaster/Engine/Nameserver.pm

Lines 310 to 311 in 47475c6

    
           if ( defined $href->{edns_details} ) { 
        
               if ( defined $href->{edns_details}{version} and $href->{edns_details}{version} != 0 ) {

edns_details->size and edns_size have the same effect - edns_details->size takes precedence if both are provided. The first is a query parameter used exclusively in Test Cases, while the latter is a profile parameter (for queries too, but that can also be set directly in Test Cases).
Same goes for edns_details->do and dnssec; edns_details->do takes precedence.
Setting a custom value for either profile parameters "dnssec" or "edns_size" directly from the profile will now have no effect. Those settings are to be controlled by Test Cases exclusively.
Setting edns_size exclusively will not enable the query as an EDNS query. It will be used only when the query is enabled as an EDNS query (i.e. with dnssec or edns_details).

Context

While implementing #1034, I noticed odd behaviors when playing around EDNS queries and responses.

Also, note that Test Cases nameserver10 to 14 (14 being NYI, see #1033) don't have unitary tests which can partly explain why such an issue was not seen before. I created issue #1146 for that.

Also fixes #501

Changes

EDNS handling for queries has been updated - the main issue was that setting only the version of EDNS to 0 would not enable it as an EDNS query. Some refactoring as well in that regard.
Check for existence of hash keys instead of definedness for edns_details
Renamed edns_details->udp_size to edns_details->size and edns_details->extended_rcode to edns_details->rcode to better match with underlying Zonemaster::LDNS functions.
Default EDNS query packet size has been updated to 512, as per the specification (DNSQueryAndResponseDefaults.md), defined in a new constant UDP_EDNS_PAYLOAD_LIMIT.
~~Some test cases in Test/Nameserver.pm were not checking if the OPT record was present in the responses to EDNS queries (default behavior as per the same specification)~~.
Deprecated the ability to set custom values for profile parameters 'dnssec' and 'edns_size' in profile.json.
Explicit mention of edns_details->version for every EDNS query in all test cases
Unitary tests updated accordingly.

How to test this PR

Tests should pass.

tgreenx · 2022-11-16T16:43:20Z

Some test cases in Test/Nameserver.pm were not checking if the OPT record was present in the responses to EDNS queries (default behavior as per the same specification).

@matsduf I included this change initally but now I removed it because I am not entirely sure if it is needed. Looking at specifications, Nameserver02 is testing whether OPT records are present in responses of EDNS queries ("If an OPT record is present in a received request, compliant responders MUST include an OPT record in their respective responses"). This can be seen in the implementation also (using has_edns()).
Subsequent specifications, for example Nameserver10, say "Issues covered by [...] Nameserver02 (basic EDNS issues) will not result in messages from this test case." and so in implementations the OPT record is not checked in responses. But default behavior (as specified in DNSQueryAndResponseDefaults.md should apply to all specifications, shouldn't it? Should that check be included for all Test Cases checking for different EDNS parameters?

tgreenx · 2022-11-17T16:31:47Z

@vlevigneron Some unitary tests in t/Test-dnssec.t are now failing when doing live testing (ZONEMASTER_RECORD=1) so for now I commented out those checks (see https://github.com/zonemaster/zonemaster-engine/pull/1147/files#diff-9b39f87b4d45d87eef8e1aad7b0c399ab07e140747d776984307c4aa5c421aaf). The fails are not linked to this PR as the same behavior is observed in both develop and master branches. Could you investigate the test zones?

matsduf · 2022-11-17T16:58:54Z

This PR fixes bugs regarding EDNS behavior in Engine.
(...)
* EDNS handling for queries has been updated - the main issue was that setting only the version of EDNS would not enable it as an EDNS query. Some refactoring as well in that regard.

Do I understand it correctly that that some queries were meant to be EDNS queries but were sent as non-EDNS queries?

matsduf · 2022-11-17T17:06:54Z

@matsduf I included this change initally but now I removed it because I am not entirely sure if it is needed. Looking at specifications, Nameserver02 is testing whether OPT records are present in responses of EDNS queries ("If an OPT record is present in a received request, compliant responders MUST include an OPT record in their respective responses"). This can be seen in the implementation also (using has_edns()). Subsequent specifications, for example Nameserver10, say "Issues covered by [...] Nameserver02 (basic EDNS issues) will not result in messages from this test case." and so in implementations the OPT record is not checked in responses.

Yes, in Nameserver10 if a server does not respond properly to EDNS vers 0 queries they should just be ignored.

But default behavior (as specified in DNSQueryAndResponseDefaults.md should apply to all specifications, shouldn't it? Should that check be included for all Test Cases checking for different EDNS parameters?

The DNSQueryAndResponseDefaults.md applies to all specifications that have been updated to refer to it.

The idea is that we should not report the same problem i several test cases. If a server cannot handle EDNS at all, then only Nameserver02 should report that.

tgreenx · 2022-11-17T18:57:43Z

Do I understand it correctly that that some queries were meant to be EDNS queries but were sent as non-EDNS queries?

Yes that's right, in Nameserver10.

lib/Zonemaster/Engine/Nameserver.pm

t/Test-nameserver.t

matsduf

In general, I have no problem with this PR, but we do lack good test zones.

mattias-p

I haven't looked at all the calls to query() to understand the exact consequences of this change, but I do like the approach.

t/Test-dnssec.t

lib/Zonemaster/Engine/Nameserver.pm

tgreenx · 2022-11-24T15:53:29Z

@mattias-p @matsduf please re-review.

lib/Zonemaster/Engine/Nameserver.pm

t/Test-dnssec.t

tgreenx

Tests are failing, I am not sure why exactly. My latest change on the cache shouldn't impact all of them. Investigating...

lib/Zonemaster/Engine/Nameserver.pm

t/Test-dnssec.t

tgreenx · 2022-11-28T16:07:57Z

Issue has been identified (checking with defined() on a hash key of an undefined hash). An alternative was possible (using exists()) but this would overcomplicate the code for no reason. Instead, I went with a change in line with the previous changes of this PR. This modifies the md5 digest created for each query (cache) so this requires an update to all unitary tests data. @blacksponge will take care of that.

tgreenx · 2022-11-28T16:23:58Z

After reconsideration, this proves too much of a hassle to update all unitary tests data files. Another good reason to move to the upcoming zonemaster/zonemaster#1113. I went with the less elegant solution, that doesn't impact the query cache.

tgreenx · 2022-11-28T16:25:27Z

@matsduf @mattias-p please re-review

tgreenx · 2022-12-28T16:17:23Z

Unitary tests will need another pass as my machine didn't have an IPv6 connectivity

matsduf · 2022-12-30T16:13:16Z

* `edns_details->size` and `edns_size` have the same effect - `edns_details->size` takes precedence if both are provided. The first is a query parameter used in Test Cases, while the latter is a profile parameter (for queries, too).

There is really no reason to have edns_size as a profile parameter since the EDNS settings must be under the control of the the test case.

* Same goes for `edns_details->do` and `dnssec` - `edns_details->do` takes precedence.

The same thing here, the DO setting must be under the control of the test case.

I suggest that this PR deprecates those two profile settings. Keep them, but make it impossible to change the default values ("dnssec" : false, "edns_size" : 0) via profile setting. Then remove them completely in v2023.2.

lib/Zonemaster/Engine/Profile.pm

tgreenx · 2023-02-14T16:25:39Z

* `edns_details->size` and `edns_size` have the same effect - `edns_details->size` takes precedence if both are provided. The first is a query parameter used in Test Cases, while the latter is a profile parameter (for queries, too).
There is really no reason to have edns_size as a profile parameter since the EDNS settings must be under the control of the the test case.
* Same goes for `edns_details->do` and `dnssec` - `edns_details->do` takes precedence.
The same thing here, the DO setting must be under the control of the test case.

I suggest that this PR deprecates those two profile settings. Keep them, but make it impossible to change the default values ("dnssec" : false, "edns_size" : 0) via profile setting. Then remove them completely in v2023.2.

Done, both settings are now effectively useless when set in the profile. See commit b1fe3ee.

I also rebased on latest develop.

marc-vanderwal

Just one minor remark about documentation.

marc-vanderwal · 2023-03-28T13:34:55Z

lib/Zonemaster/Engine/Constants.pm

@@ -282,6 +284,12 @@ C<$UDP_PAYLOAD_LIMIT>

 =item *

+C<$UDP_EDNS_PAYLOAD_LIMIT>
+
+The EDNS0 UDP maximum size.


I don’t quite get the difference between UDP_PAYLOAD_LIMIT, UDP_EDNS_PAYLOAD_LIMIT and UDP_COMMON_EDNS_LIMIT from reading this file. Can you elaborate, and document those two other constants as well?

From the value, UDP_EDNS_PAYLOAD_LIMIT is the lowest value of UDP payload in EDNS. If lower, it must be treated as 512. UDP_EDNS_PAYLOAD_MIN is maybe a better name? There is also a maximum value. Should that be considered?

UDP_PAYLOAD_LIMIT is not used to check incoming DNS packets, but only used in DELEGATION03 to check if a constructed packet with the assumed referral for the domain name would exceed 512 Bytes or not.

UDP_COMMON_EDNS_LIMIT is used to generate a message out of test case if the packet data is larger than UDP_COMMON_EDNS_LIMIT (as defined above) which feels quite arbitrarily. There are two sizes that could be valid to react on:

Larger than the value set in the query. That is not permitted.

Larger than 1232 because the greatly increases the risk of fragmented UDP.

The new UDP_EDNS_PAYLOAD_LIMIT is used as the UDP size setting in the default query. I suggest the name UDP_EDNS_QUERY_DEFAULT or something like that.

The new UDP_EDNS_PAYLOAD_LIMIT is used as the UDP size setting in the default query. I suggest the name UDP_EDNS_QUERY_DEFAULT or something like that.

That works for me. Updated.

matsduf · 2023-03-29T15:22:30Z

lib/Zonemaster/Engine/Constants.pm

@@ -282,6 +284,12 @@ C<$UDP_PAYLOAD_LIMIT>

 =item *

+C<$UDP_EDNS_PAYLOAD_LIMIT>
+
+The EDNS0 UDP maximum size.


UDP_PAYLOAD_LIMIT is not used to check incoming DNS packets, but only used in DELEGATION03 to check if a constructed packet with the assumed referral for the domain name would exceed 512 Bytes or not.

UDP_COMMON_EDNS_LIMIT is used to generate a message out of test case if the packet data is larger than UDP_COMMON_EDNS_LIMIT (as defined above) which feels quite arbitrarily. There are two sizes that could be valid to react on:

Larger than the value set in the query. That is not permitted.

Larger than 1232 because the greatly increases the risk of fragmented UDP.

The new UDP_EDNS_PAYLOAD_LIMIT is used as the UDP size setting in the default query. I suggest the name UDP_EDNS_QUERY_DEFAULT or something like that.

matsduf · 2023-03-29T15:30:26Z

lib/Zonemaster/Engine/Nameserver.pm

-Set the DO flag in the query.
+Set the DO flag in the query. Defaults to false.
+
+Enables the query to be an EDNS query if set to true.


Unclear. Should it say that EDNS is automatically enabled if the DO flag is set?

Yes - is that not what this sentence says? Otherwise I can rephrase, e.g. "If set to true, enables the query to be an EDNS query", or something entirely different. I have used this wording for all the documentation here.

I think the sentence starting with "if set to true" is easier to read.

matsduf · 2023-04-14T16:36:02Z

@tgreenx, what is the status of this PR?

- EDNS handling has been updated - the main issue was that setting only the version of EDNS would not enable EDNS queries. Some refactoring as well in that regard. - Default EDNS packet size has been updated to 512 in profile.pm, as per the specification (DNSQueryAndResponseDefaults.md). - Some test cases in Test/Nameserver.pm were not checking if the OPT record was present in the responses to EDNS queries (default behavior as per the same specification). - Unitary tests updated accordingly. Revert Test Case checks for OPT record in EDNS responses Fixes EDNS behavior for non EDNS queries Add resolver.defaults.edns_size documentation in Profile.pm Refactoring. EDNS queries will now be correctly enabled using any "edns_details" parameters ("version", "z", "do", "udp_size", etc).. Setting "edns_size" will have no effect (i.e. not enable the query as an EDNS query) unless a parameter of "edns_details" is also specified. "edns_details->udp_size" and "edns_size" have the same effect, but "udp_size" takes precedence if both are provided. Update after review - more refactoring, more documentation Update after review - specify handling of dnssec/do, more documentation Fix existence check in hash Use less elegant fix Update unitary files

Check for existence of hash keys instead of definedness Rename supported keys 'edns_details->udp_size' to 'edns_details->size' and 'edns_details->extended_rcode' to 'edns_details->rcode' to better match with underlying Zonemaster::LDNS functions Update documentation of 'dnssec' and 'edns_size' Update all EDNS test cases with explicit mention of EDNS version when enabling it in query. Although this is not necessary (an empty hash would work the same since LDNS defaults the version number to 0 when not specified), more explicit the better (and future-proof). Update unitary tests 't/Test-dnssec.t' TODO section

tgreenx · 2023-04-18T14:16:56Z

@tgreenx, what is the status of this PR?

I have addressed latest comments and rebased on latest develop. Please re-review.

matsduf · 2023-04-18T15:16:19Z

lib/Zonemaster/Engine/Constants.pm

@@ -282,6 +284,12 @@ C<$UDP_PAYLOAD_LIMIT>

 =item *

+C<$UDP_EDNS_QUERY_DEFAULT>
+
+The EDNS0 UDP maximum size.


$UDP_EDNS_QUERY_DEFAULT is the default setting in EDNS in queries from Zonemaster. 512 is also the lower limit of what you may announce in EDNS.

I forgot to update that part - it now reads:

C<$UDP_EDNS_QUERY_DEFAULT> An integer, used to define the EDNS0 UDP packet size in EDNS queries.

Deprecate Profile settings "dnssec" and "edns_size" - changing their value will now have no effect. Planned for complete removal in 2023.2. Added new constant UDP_EDNS_QUERY_DEFAULT Restore 'edns_size' previous value in profile.json

marc-vanderwal

Looks good to me.

It’s a shame we cannot investigate the failing unit tests further. It does seem, however, that the test zones themselves are at fault and not the code, because I ran ZONEMASTER_RECORD=1 prove -l t/Test-dnssec.t with an unrelated branch and got even more errors.

tgreenx added T-Bug Type: Bug in software or error in test case description A-TestCase Area: Test case specification or implementation of test case labels Nov 16, 2022

tgreenx added this to the v2022.2 milestone Nov 16, 2022

tgreenx requested review from mattias-p, matsduf, hannaeko, a user and marc-vanderwal November 16, 2022 15:35

tgreenx mentioned this pull request Nov 17, 2022

New implementation of Nameserver11 - #1110 #1034

Merged

matsduf reviewed Nov 18, 2022

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

matsduf requested changes Nov 18, 2022

View reviewed changes

t/Test-nameserver.t Outdated Show resolved Hide resolved

matsduf reviewed Nov 18, 2022

View reviewed changes

tgreenx requested a review from matsduf November 22, 2022 09:37

matsduf previously approved these changes Nov 22, 2022

View reviewed changes

mattias-p reviewed Nov 23, 2022

View reviewed changes

t/Test-dnssec.t 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

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

tgreenx dismissed matsduf’s stale review via 3cc2594 November 24, 2022 13:43

tgreenx requested review from mattias-p and matsduf November 24, 2022 13:43

matsduf reviewed Nov 24, 2022

View reviewed changes

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

t/Test-dnssec.t Outdated Show resolved Hide resolved

tgreenx commented Nov 28, 2022

View reviewed changes

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

t/Test-dnssec.t Outdated Show resolved Hide resolved

tgreenx requested a review from matsduf November 28, 2022 16:25

matsduf reviewed Dec 30, 2022

View reviewed changes

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

tgreenx force-pushed the fix-edns branch from e34c8d0 to b1fe3ee Compare February 14, 2023 16:22

tgreenx requested a review from matsduf February 14, 2023 16:25

marc-vanderwal reviewed Mar 28, 2023

View reviewed changes

matsduf reviewed Mar 29, 2023

View reviewed changes

tgreenx added 3 commits April 18, 2023 15:50

Update unitary test data

ecd21c4

tgreenx force-pushed the fix-edns branch from 668bade to 08fd032 Compare April 18, 2023 14:15

matsduf reviewed Apr 18, 2023

View reviewed changes

Update after review - refactoring, documentation

041271b

Deprecate Profile settings "dnssec" and "edns_size" - changing their value will now have no effect. Planned for complete removal in 2023.2. Added new constant UDP_EDNS_QUERY_DEFAULT Restore 'edns_size' previous value in profile.json

tgreenx force-pushed the fix-edns branch from 08fd032 to 041271b Compare April 18, 2023 16:04

tgreenx requested review from marc-vanderwal and matsduf April 18, 2023 16:05

marc-vanderwal approved these changes Apr 27, 2023

View reviewed changes

matsduf approved these changes Apr 27, 2023

View reviewed changes

tgreenx merged commit 6282884 into zonemaster:develop May 2, 2023

tgreenx deleted the fix-edns branch May 2, 2023 08:28

tgreenx mentioned this pull request May 16, 2023

Remove items from profile #1148

Open

tgreenx linked an issue May 16, 2023 that may be closed by this pull request

Remove items from profile #1148

Open

matsduf added the V-Patch Versioning: The change gives an update of patch in version. label Jun 20, 2023

tgreenx linked an issue Jul 26, 2023 that may be closed by this pull request

Complete Zonemaster::Engine::Nameserver::query method $flagref documentation #501

Closed

tgreenx mentioned this pull request Jul 26, 2023

Complete Zonemaster::Engine::Nameserver::query method $flagref documentation #501

Closed

tgreenx mentioned this pull request Nov 29, 2023

Remove deprecated features planned for v2023.2 #1309

Merged

tgreenx mentioned this pull request Dec 9, 2024

Fix default settings of queries #1397

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix EDNS behavior for queries #1147

Fix EDNS behavior for queries #1147

tgreenx commented Nov 16, 2022 •

edited

Loading

tgreenx commented Nov 16, 2022 •

edited

Loading

tgreenx commented Nov 17, 2022

matsduf commented Nov 17, 2022 •

edited

Loading

matsduf commented Nov 17, 2022

tgreenx commented Nov 17, 2022 •

edited by matsduf

Loading

matsduf left a comment

mattias-p left a comment

tgreenx commented Nov 24, 2022

tgreenx left a comment •

edited

Loading

tgreenx commented Nov 28, 2022 •

edited

Loading

tgreenx commented Nov 28, 2022 •

edited

Loading

tgreenx commented Nov 28, 2022

tgreenx commented Dec 28, 2022

matsduf commented Dec 30, 2022

tgreenx commented Feb 14, 2023 •

edited

Loading

marc-vanderwal left a comment

marc-vanderwal Mar 28, 2023

matsduf Mar 28, 2023 •

edited

Loading

matsduf Mar 29, 2023

tgreenx Apr 18, 2023

matsduf Mar 29, 2023

matsduf Mar 29, 2023

tgreenx Apr 18, 2023

matsduf Apr 18, 2023

matsduf commented Apr 14, 2023

tgreenx commented Apr 18, 2023

matsduf Apr 18, 2023

tgreenx Apr 18, 2023

marc-vanderwal left a comment

	if ( defined $href->{edns_details} ) {
	if ( defined $href->{edns_details}{version} and $href->{edns_details}{version} != 0 ) {

Fix EDNS behavior for queries #1147

Fix EDNS behavior for queries #1147

Conversation

tgreenx commented Nov 16, 2022 • edited Loading

Purpose

Context

Changes

How to test this PR

tgreenx commented Nov 16, 2022 • edited Loading

tgreenx commented Nov 17, 2022

matsduf commented Nov 17, 2022 • edited Loading

matsduf commented Nov 17, 2022

tgreenx commented Nov 17, 2022 • edited by matsduf Loading

matsduf left a comment

Choose a reason for hiding this comment

mattias-p left a comment

Choose a reason for hiding this comment

tgreenx commented Nov 24, 2022

tgreenx left a comment • edited Loading

Choose a reason for hiding this comment

tgreenx commented Nov 28, 2022 • edited Loading

tgreenx commented Nov 28, 2022 • edited Loading

tgreenx commented Nov 28, 2022

tgreenx commented Dec 28, 2022

matsduf commented Dec 30, 2022

tgreenx commented Feb 14, 2023 • edited Loading

marc-vanderwal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf Mar 28, 2023 • edited Loading

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

Choose a reason for hiding this comment

matsduf commented Apr 14, 2023

tgreenx commented Apr 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-vanderwal left a comment

Choose a reason for hiding this comment

tgreenx commented Nov 16, 2022 •

edited

Loading

tgreenx commented Nov 16, 2022 •

edited

Loading

matsduf commented Nov 17, 2022 •

edited

Loading

tgreenx commented Nov 17, 2022 •

edited by matsduf

Loading

tgreenx left a comment •

edited

Loading

tgreenx commented Nov 28, 2022 •

edited

Loading

tgreenx commented Nov 28, 2022 •

edited

Loading

tgreenx commented Feb 14, 2023 •

edited

Loading

matsduf Mar 28, 2023 •

edited

Loading