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

Implementation of DNSSEC10 with new specifications #995

Merged

Conversation

vlevigneron
Copy link
Contributor

@vlevigneron vlevigneron commented Oct 30, 2021

Purpose

This PR is the implementation of zonemaster/zonemaster#875

Context

fixes issue #992
fixes issue #722 (no longer relevant)

Changes

method Zonemaster::Engine::Test::DNSSEC::dnssec10 complete rewrite
15 new messages tag implemented (no translation yet)
15 old messages removed
Unit tests updated but many missing

How to test this PR

Use one of the following domains to do the tests:

dnssec10-non-existent-domain-name-exists-01.zft-root.rd.nic.fr
dnssec10-non-existent-domain-name-exists-02.zft-root.rd.nic.fr
dnssec10-non-existent-domain-name-exists-03.zft-root.rd.nic.fr
dnssec10-non-existent-domain-name-exists-04.zft-root.rd.nic.fr
dnssec10-non-existent-domain-name-exists-05.zft-root.rd.nic.fr
dnssec11-inconsistent-dnskey.zft-root.rd.nic.fr
dnssec10-inconsistent-nsec-nsec3.zft-root.rd.nic.fr
nic.se
zonemaster.fr

@vlevigneron vlevigneron added this to the v2021.2 milestone Oct 30, 2021
@vlevigneron
Copy link
Contributor Author

@matsduf message are not yet defined (any proposal are welcomed), but I face a problem with DS10_INCONSISTENT_NSEC_NSEC3. It is not possible to have 2 arguments with same label (ns_ip_list). Should we have 2 messages or 2 labels (to define in https://github.com/zonemaster/zonemaster-engine/blob/master/docs/logentry_args.md).

@matsduf
Copy link
Contributor

matsduf commented Nov 1, 2021

@matsduf message are not yet defined (any proposal are welcomed), but I face a problem with DS10_INCONSISTENT_NSEC_NSEC3. It is not possible to have 2 arguments with same label (ns_ip_list). Should we have 2 messages or 2 labels (to define in https://github.com/zonemaster/zonemaster-engine/blob/master/docs/logentry_args.md).

@vlevigneron, the messages should be one to make it clear. My suggestion is the the arguments are called "ns_ip_list_nsec" and "ns_ip_list_nsec3", respectively, to make the distinction. I have discussed this with @mattias-p too. If you agree that it is a good idea, I will create a proposed update to logentry_args that states that such suffix can be added if the same argument is used more than once.

Added 2021-11-05: #997 has been created.

lib/Zonemaster/Engine/Test/DNSSEC.pm Outdated Show resolved Hide resolved
lib/Zonemaster/Engine/Test/DNSSEC.pm Show resolved Hide resolved
lib/Zonemaster/Engine/Test/DNSSEC.pm Show resolved Hide resolved
lib/Zonemaster/Engine/Test/DNSSEC.pm Show resolved Hide resolved
lib/Zonemaster/Engine/Test/DNSSEC.pm Show resolved Hide resolved
lib/Zonemaster/Engine/Test/DNSSEC.pm Show resolved Hide resolved
lib/Zonemaster/Engine/Test/DNSSEC.pm Show resolved Hide resolved
lib/Zonemaster/Engine/Test/DNSSEC.pm Show resolved Hide resolved
lib/Zonemaster/Engine/Test/DNSSEC.pm Show resolved Hide resolved
lib/Zonemaster/Engine/Test/DNSSEC.pm Show resolved Hide resolved
@matsduf
Copy link
Contributor

matsduf commented Nov 1, 2021

The specification has also been updated by zonemaster/zonemaster#1008.

@vlevigneron
Copy link
Contributor Author

@matsduf Do you think we can merge it ?

@vlevigneron vlevigneron merged commit 69a40f4 into zonemaster:develop Nov 5, 2021
@matsduf matsduf mentioned this pull request Nov 5, 2021
@matsduf
Copy link
Contributor

matsduf commented Nov 29, 2021

Testing v2021.2

LGTM.

I tested available test zones and compared with the output when using dig.

  • The test case is not fully covered by test zones.
  • Besides the listing in t/Test-dnssec.t there is no documentation on the purpose of the test zones.

@matsduf matsduf added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Nov 29, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants