-
Notifications
You must be signed in to change notification settings - Fork 33
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
Fix CDS/CDNSKEY RRsets comparison in DNSSEC15 #1383
base: develop
Are you sure you want to change the base?
Conversation
I've proposed zonemaster/zonemaster-ldns#203, which would allow for making a more straightforward solution for this PR. So I'm switching it to a draft state for now. |
@mattias-p @matsduf @MichaelTimbert @marc-vanderwal This PR has been updated and is now ready for review. |
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.
I find the code quite hard to follow. I would like to see comments that says what sections are meant to do. This is not due to the current PR. That is how the code is already. But that fact makes it harder to review the code.
I have tested the update and I have found no issues.
@mattias-p , @marc-vanderwal, @MichaelTimbert - please review. |
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.
I suggest that we wait until zonemaster/zonemaster-ldns#209 is merged.
|
||
my $rrlist = scalar @{ $cds_rrsets{ $ns_ip } } ? Zonemaster::LDNS::RRList->new( $cds_rrsets{ $ns_ip } ) : undef; | ||
|
||
if ( ( $rrlist and not $first_rrlist ) or ( not $rrlist and $first_rrlist ) or ( $rrlist and $first_rrlist and $rrlist ne $first_rrlist ) ) { |
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.
As soon as zonemaster/zonemaster-ldns#209 is merged, this can be rewritten as:
if ( ( $rrlist and not $first_rrlist ) or ( not $rrlist and $first_rrlist ) or ( $rrlist and $first_rrlist and $rrlist ne $first_rrlist ) ) { | |
if ( $rrlist ne $first_rrlist ) { |
|
||
my $rrlist = scalar @{ $cdnskey_rrsets{ $ns_ip } } ? Zonemaster::LDNS::RRList->new( $cdnskey_rrsets{ $ns_ip } ) : undef; | ||
|
||
if ( ( $rrlist and not $first_rrlist ) or ( not $rrlist and $first_rrlist ) or ( $rrlist and $first_rrlist and $rrlist ne $first_rrlist ) ) { |
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.
See comments and suggestions above.
elsif ( $rrset_string ne $first_rrset_string ) { | ||
push @results, _emit_log( DS15_INCONSISTENT_CDS => {} ); | ||
|
||
my $rrlist = scalar @{ $cds_rrsets{ $ns_ip } } ? Zonemaster::LDNS::RRList->new( $cds_rrsets{ $ns_ip } ) : undef; |
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.
As soon as zonemaster/zonemaster-ldns#209 is merged, this can be simplified to:
my $rrlist = scalar @{ $cds_rrsets{ $ns_ip } } ? Zonemaster::LDNS::RRList->new( $cds_rrsets{ $ns_ip } ) : undef; | |
my $rrlist = Zonemaster::LDNS::RRList->new( $cds_rrsets{ $ns_ip } ); |
if ( not defined $first_rrset_string ) { | ||
$first_rrset_string = $rrset_string; | ||
if ( $first ) { | ||
$first_rrlist = scalar @{ $cds_rrsets{ $ns_ip } } ? Zonemaster::LDNS::RRList->new( $cds_rrsets{ $ns_ip } ) : undef; |
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.
As soon as zonemaster/zonemaster-ldns#209 is merged, this can be simplified to:
$first_rrlist = scalar @{ $cds_rrsets{ $ns_ip } } ? Zonemaster::LDNS::RRList->new( $cds_rrsets{ $ns_ip } ) : undef; | |
$first_rrlist = Zonemaster::LDNS::RRList->new( $cds_rrsets{ $ns_ip } ); |
Purpose
This PR proposes a fix to the handling of CDS and CDNSKEY RRsets comparison in DNSSEC15.
PS: The proper fix for the code below https://github.com/zonemaster/zonemaster-engine/pull/1383/files#diff-3c6ad87ddb974fb813839bacdaafd4e212c238d0ff6865a9bf6cf677403343deR3953 requires changes in LDNS. PRs have been made to make it happen, and it should be available in the next release of LDNS (1.8.5). See NLnetLabs/ldns@b398138.
Context
Fixes #1381
Requires zonemaster/zonemaster-ldns#199
Changes
How to test this PR
This requires zonemaster/zonemaster-ldns#199.
Unit tests should pass.
Manual testing (from #1381):