-
Notifications
You must be signed in to change notification settings - Fork 36
Update DNSSEC07 implementation #1476
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
Conversation
08f9bf8 to
d816b18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the description of zonemaster/zonemaster#1425:
After this update, DNSSEC07 should still be run first, and then DNSSEC11. If updated DNSSEC07 outputs DS07_NOT_SIGNED then no other test cases, besides DNSSEC11, in DNSSEC module should be run.
I.e. DNSSEC07 and DNSSEC11 should always be run.
Should that be included in the specification? I do not think so, but it should be stated somewhere.
matsduf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides that DNSSEC11 should always be run it looks fine. All scenarios pass.
For that purpose with have the "Special procedural requirements" section in the specification, which I followed. But it seems it was not entirely updated correctly then. |
d816b18 to
cea77de
Compare
Updated and rebased on top of #1475 |
|
Unit test data for t/Test-dnssec.t needs to be re-recorded in order for all unit tests to pass, but that can't be done right now (zut-root.rd.nic.fr is temporarily offline). It will be done at a later time. |
I suggest that the failing tests are marked as TODO. What test cases are affected? I could possibly create scenarios for them. |
b3f457e to
9db92f0
Compare
|
@matsduf please re-review, unit tests have been re-recorded (the test zones are back online). |
matsduf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not look like the tag NOT_SIGNED show up in the output, which seems strange.
It is not strange. It is outputted, but on DEBUG level. If level is set to DEBUG in |
matsduf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NOT_SIGNED tag should be cleaned away, but it does no harm. The cleaning could also be done later.
9db92f0 to
2f136ba
Compare
Done (removed). I've also rebased and fixed several conflicts. Please re-review |
2f136ba to
8c06297
Compare
8c06297 to
bb61d5b
Compare
Purpose
This PR proposes an update of test case DNSSEC07 implementation.
Context
Test case specification: zonemaster/zonemaster#1425
Test scenarios specification: zonemaster/zonemaster#1432
Changes
How to test this PR
Unit tests are updated and should pass.