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

Use Zonemaster::Engine::Normalization module for input name normalization #357

Merged
merged 7 commits into from
Nov 27, 2023

Conversation

tgreenx
Copy link
Contributor

@tgreenx tgreenx commented Oct 18, 2023

Purpose

This PR introduces the usage of the Zonemaster::Engine::Normalization module for input DNS name normalization.
It also adds input validation to the --ns argument..

Context

The current specification can found here, and will replace the Basic00 Test Case.

Changes

  • Make use of Zonemaster::Engine::Normalization::normalize_name()
  • Significantly improve --ns argument validation:
    • Add check for several slash (/) characters
    • Add IP address validation which leverages both regex checks and Net::IP::XS::Error() for detailed error messages
  • Remove Zonemaster::CLI::to_idn()
  • List Readonly and Net::IP::XS as direct dependencies

How to test this PR

Testing should proceed as before, although more errors are now supported and several can be reported simultaneously:

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 '@.İabcİ.İcc'
Ambiguous downcaseing of character "LATIN CAPITAL LETTER I WITH DOT ABOVE" in the domain name. Use all lower case instead.

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01
Must give the name of a domain to test.

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 ''
Must give the name of a domain to test.

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 ' '
Domain name is empty.

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 ' @!.'
Domain name has an ASCII label ("@!") with a character not permitted.

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 zonemaster.net --ns 'ns1/.zonemaster.net/10.0.0.1'
--ns must be a name or a name/ip pair.

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 zonemaster.net --ns 'ns1.zonemaster.net/1'
Invalid IP address in --ns argument.

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 zonemaster.net --ns 'ns1.zonemaster.net/10.0.0.Z'
Invalid IP address in --ns argument:
        Invalid chars in IP 10.0.0.Z
        
$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 zonemaster.net --ns 'ns1.zonemaster.net/fe10:::'
Invalid IP address in --ns argument:
        Invalid address fe10::: (More than one :: pattern)

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 zonemaster.net --ns '[email protected]@.net!.'
Invalid name in --ns argument:
        Domain name has an ASCII label ("ns1@") with a character not permitted.
        Domain name has an ASCII label ("zonemaster@") with a character not permitted.
        Domain name has an ASCII label ("net!") with a character not permitted.

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 zonemaster.net --ns 'ns1.zonemaster.net' --ns 'ns2.zonemaster.net/10.0.0.2' --ns '[email protected].!net.'
Invalid name in --ns argument:
        Domain name has an ASCII label ("ns3@") with a character not permitted.
        Domain name has an ASCII label ("!net") with a character not permitted.

Examples of (in)valid domain names can be found in the t/normalization.t unit test file in Engine, e.g:

#Valid
example。com.
café.example.com

#Invalid
example。.com.
bad:%;!$.example.com.
❤️.example.com

@tgreenx tgreenx added the V-Minor Versioning: The change gives an update of minor in version. label Oct 18, 2023
@tgreenx tgreenx added this to the v2023.2 milestone Oct 18, 2023
…name normalization

- Use Zonemaster::Engine::Normalization::normalize_name() in function add_fake_delegation()
- Add conditional check to argument '--ns' in case of several slash characters
@tgreenx
Copy link
Contributor Author

tgreenx commented Oct 18, 2023

Seems like at first I forgot about the normalization of name server names. It is now done (commit 4ab0aaf). I also pushed another commit (0c8093e) that removes the now unused Zonemaster::CLI::to_idn() method.

By the way, in both the specification and the implementation (message ids) of Zonemaster::Engine::Normalization we might want to rename 'domain name' to 'DNS name' to avoid possible confusion, since name server names are also normalized by this module. Note that we do in fact already have a module named Zonemaster::Engine::DNSName.

Also, in this PR , for normalization of name server names, I added the message Invalid name in --ns argument: followed by indented error message(s) for clarity.

@tgreenx tgreenx changed the title Uses Zonemaster::Engine::Normalization module for input domain name normalization Use Zonemaster::Engine::Normalization module for input name normalization Oct 19, 2023
This is no longer needed following the switch to Zonemaster::Engine::Normalization.
mattias-p
mattias-p previously approved these changes Nov 17, 2023
Copy link
Member

@mattias-p mattias-p left a comment

Choose a reason for hiding this comment

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

I like this change but I have a couple of nits.

lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
@matsduf
Copy link
Contributor

matsduf commented Nov 17, 2023

$ zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 '@.İabcİ.İcc'
Domain name has an ASCII label ("@") with a character not permitted.
Domain name has a non-ASCII label ("İabcİ") which is not a valid U-label.
Domain name has a non-ASCII label ("İcc") which is not a valid U-label.

The problem with "İabcİ" is that it cannot be downcased in an unambiguous way. The specification says "AMBIGUOUS_DOWNCASING" (https://github.com/zonemaster/zonemaster/blob/master/docs/public/specifications/tests/RequirementsAndNormalizationOfDomainNames.md).

@hannaeko
Copy link
Member

I tested the PR with café.test and also got an INVALID_ASCII error. There seems to be missing decoding from utf8 (or whatever encoding the terminal is using), I added Encode::decode("utf8", $domain ) before passing the domain to nomalize_name and it worked.

Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

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

Does it work correctly? On FreeBSD I get

# zonemaster-cli råttgift.se
Domain name has a non-ASCII label ("råttgift") which is not a valid U-label.

Then I tested on Ubuntu 22.04 and get the same error.

@tgreenx
Copy link
Contributor Author

tgreenx commented Nov 23, 2023

Does it work correctly? On FreeBSD I get

# zonemaster-cli råttgift.se
Domain name has a non-ASCII label ("råttgift") which is not a valid U-label.

Then I tested on Ubuntu 22.04 and get the same error.

As suggested by @blacksponge, with proper decoding this should now work as expected. See commit ce9b025.

@tgreenx tgreenx requested a review from matsduf November 23, 2023 13:33
mattias-p
mattias-p previously approved these changes Nov 23, 2023
Update IP address validation methodology
	- It is now primarly based on regex checks, but also leverages Net::IP::XS::Error (when applicable) for more detailed error messages
Add Readonly and Net::IP::XS as direct dependencies
Copy link
Member

@hannaeko hannaeko left a comment

Choose a reason for hiding this comment

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

Looks fine to me! I tested some of the test commands and it works as expected.

Copy link
Member

@mattias-p mattias-p left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

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

Nice to have it in place! This gives consistent and documented behavior.

@tgreenx tgreenx merged commit 6c85987 into zonemaster:develop Nov 27, 2023
1 check passed
@tgreenx tgreenx deleted the use-normalization branch November 27, 2023 10:14
@mattias-p
Copy link
Member

mattias-p commented Jan 8, 2024

v2023.2 release testing

I've tested this for v2023.2 and I could not find any deviations between the described behavior and the actual behavior.

However, upon reflection I think the behavior could be improved in a couple of ways. I created #364 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ReleaseTested Status: The PR has been successfully tested in release testing V-Minor Versioning: The change gives an update of minor in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Normalize zone name before test start
4 participants