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.

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

Updates NAMESERVER15 #1199

Merged
merged 9 commits into from
Nov 19, 2023
Merged

Conversation

matsduf
Copy link
Contributor

@matsduf matsduf commented Aug 25, 2023

Purpose

The objective of the test case states about name servers that "it may sometimes be desirable not to reveal that information" but then if it does, this test case only sends an INFO message. An INFO message does not signal that there is any reason to look at it. This PR raises the level to NOTICE.

$ zonemaster-cli --test nameserver/nameserver15 telia.com --raw
   0.00 INFO     UNSPECIFIED    GLOBAL_VERSION   version=v4.7.2
   1.57 INFO     NAMESERVER15   N15_SOFTWARE_VERSION   ns_ip_list=dns358.fi.telia.net/88.194.40.67;dns49.de.telia.net/2001:2030:c000:5::4;dns49.de.telia.net/213.248.77.82; query_name=version.bind; string=9.18.9
   1.58 INFO     NAMESERVER15   N15_SOFTWARE_VERSION   ns_ip_list=dns1.telia.com/2001:2040:c001:101::5;dns1.telia.com/81.228.11.67;dns2.telia.com/2001:2040:c001:401::6;dns2.telia.com/81.228.10.67; query_name=version.bind; string=9.18.11

The specification does not distinguish between a server that does not respond at all on a "version query" and a server that responds, but with no version information. In the following example some of the servers do not respond at all and some of them responds with no information. This PR splits the message and make the test case make a distinction.

$ zonemaster-cli --test nameserver/nameserver15 festo.press --raw
   0.00 INFO     UNSPECIFIED    GLOBAL_VERSION   version=v4.7.2
  41.67 INFO     NAMESERVER15   N15_NO_VERSION   ns_ip_list=ns2.atvirtual.com/185.136.96.210;ns2.atvirtual.com/2a06:fb00:1::1:210;ns6.atvirtual.com/185.136.97.210;ns6.atvirtual.com/2a06:fb00:1::2:210;ns7.atvirtual.com/185.136.98.210;ns7.atvirtual.com/2a06:fb00:1::3:210;ns8.atvirtual.com/185.136.99.210;ns8.atvirtual.com/2a06:fb00:1::4:210

This PR also make the test case skip servers that do not even answer SOA queries to make it lift up any servers that do not answer because it is a "version query".

This PR also changes the argument from ns_ip_list to ns_list to give more information. It uses the same technique introduced by #1179.

This PR also updates the test case to check the class in the response.

Context

Also see zonemaster/zonemaster-engine#1281

Also see #1208

Also see test zones that match this update in #1217.

Changes

The specification of NAMESERVER15.

How to test this PR

Review the proposed changes.

@matsduf matsduf added the A-TestCase Area: Test case specification or implementation of test case label Aug 25, 2023
@matsduf matsduf added this to the v2023.2 milestone Aug 25, 2023
@matsduf
Copy link
Contributor Author

matsduf commented Aug 25, 2023

@tgreenx, you wrote the specification. Please review this proposal of updates.

@matsduf matsduf requested a review from tgreenx August 30, 2023 10:14
Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>
marc-vanderwal
marc-vanderwal previously approved these changes Sep 14, 2023
tgreenx
tgreenx previously approved these changes Sep 14, 2023
* Adds test for class on the TXT records
* Remove check of owner name of TXT records
* Editorial update
@matsduf
Copy link
Contributor Author

matsduf commented Sep 15, 2023

@tgreenx and @marc-vanderwal, please re-review.

Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>
@matsduf matsduf requested a review from tgreenx October 16, 2023 15:57
tgreenx
tgreenx previously approved these changes Oct 18, 2023
@matsduf
Copy link
Contributor Author

matsduf commented Oct 18, 2023

@marc-vanderwal, are you also fine with this?

marc-vanderwal
marc-vanderwal previously approved these changes Oct 19, 2023
Copy link
Contributor

@marc-vanderwal marc-vanderwal left a comment

Choose a reason for hiding this comment

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

Yes, that looks good to me!

@matsduf
Copy link
Contributor Author

matsduf commented Oct 19, 2023

Test zone specification for this test case in #1217

@matsduf matsduf dismissed stale reviews from marc-vanderwal and tgreenx via d016fb2 October 27, 2023 13:17
@matsduf
Copy link
Contributor Author

matsduf commented Oct 27, 2023

@tgreenx and @marc-vanderwal, I made a version string just consisting of space to be equal to empty version string.

@matsduf
Copy link
Contributor Author

matsduf commented Oct 27, 2023

Test zone specification for this test case in #1217

Now the test zones have been implemented.

marc-vanderwal
marc-vanderwal previously approved these changes Oct 30, 2023
tgreenx
tgreenx previously approved these changes Oct 30, 2023
@matsduf
Copy link
Contributor Author

matsduf commented Oct 30, 2023

@tgreenx, you did the current implementation of Nameserver15. Could you do the update?

@tgreenx
Copy link
Contributor

tgreenx commented Oct 30, 2023

@tgreenx, you did the current implementation of Nameserver15. Could you do the update?

Yes

tgreenx added a commit to tgreenx/zonemaster-engine that referenced this pull request Nov 16, 2023
Follows test specification update (zonemaster/zonemaster#1199).

Unit tests and data are also updated, based on zonemaster/zonemaster#1217.
@matsduf matsduf dismissed stale reviews from tgreenx and marc-vanderwal via 8996345 November 19, 2023 22:44
@matsduf
Copy link
Contributor Author

matsduf commented Nov 19, 2023

A trivial merge conflict was resolved after two approvals. Merge without waiting for renewed approval.

@matsduf matsduf merged commit 323281e into zonemaster:develop Nov 19, 2023
@matsduf matsduf deleted the update-nameserver15 branch November 19, 2023 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-TestCase Area: Test case specification or implementation of test case
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nameserver15 may use TXT records of wrong class when processing response to CH TXT queries
3 participants