Skip to content

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

Merged
tgreenx merged 7 commits intozonemaster:developfrom
tgreenx:use-normalization
Nov 27, 2023
Merged

Use Zonemaster::Engine::Normalization module for input name normalization#357
tgreenx merged 7 commits intozonemaster:developfrom
tgreenx:use-normalization

Conversation

@tgreenx
Copy link
Copy Markdown
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 'ns1@.zonemaster@.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 'ns3@.zonemaster.!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
Copy Markdown
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
Copy Markdown
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.

Comment thread lib/Zonemaster/CLI.pm Outdated
Comment thread lib/Zonemaster/CLI.pm
Comment thread lib/Zonemaster/CLI.pm Outdated
@matsduf
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
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.

@mattias-p
Copy link
Copy Markdown
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