-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add DNAME support (to Zone objects) #1213
base: develop
Are you sure you want to change the base?
Conversation
lib/Zonemaster/Engine/Zone.pm
Outdated
foreach my $rr ( $p->get_records( 'DNAME', q{answer} ) ) { | ||
if ( index( $self->name->fqdn, lc($rr->owner) ) != -1 ) { | ||
$dname = Zonemaster::Engine::DNSName->new( lc($rr->dname) ); | ||
$owner = Zonemaster::Engine::DNSName->new( lc($rr->owner) ); | ||
|
||
splice @name_labels, scalar @name_labels - scalar @{$owner->labels}, scalar @name_labels, @{$dname->labels}; | ||
|
||
push @dnames, Zonemaster::Engine::DNSName->new( lc( join( q{.}, @name_labels ) ) ); | ||
} | ||
} |
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 is not permitted to have more than one DNAME record with the same owner name. One approach is to do as here and let the calling function do what it was.
- The code gets the owner name of the RR in the answer section, doesn't it? The owner name should (must) be the same as in the query.
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.
But it is possible to have a chain of DNAME records, and possibly with some CNAME records in the chain too.
I am not convinced that advanced support of DNAME is needed. It is at least not for BASIC01. |
As conluded in #1212 (comment) BASIC01 does not require advanced DNAME function. If a DNAME record is present in Besides the delegation issue handled by BASIC01, Zonemaster can meet synthesized CNAME records for host names (right side) of MX or NS or the SOA MNAME. In those cases Zonemaster can ignore the DNAME and just follow the CNAME until we have a test case that should validate those. Just as with normal CNAME, DNAME can create a loop. DNAME loop can be handles as a CNAME loop since we get the synthesized CNAME. The is one special "loop" of DNAME. If we have |
Considering the above comments, this work is put on hold until next release. |
lib/Zonemaster/Engine/Zone.pm
Outdated
my @dnames; | ||
|
||
foreach my $rr ( $p->get_records( 'DNAME', q{answer} ) ) { | ||
if ( index( $self->name->fqdn, lc($rr->owner) ) != -1 ) { |
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.
Careful with that: you don’t want ab.example.com
to match b.example.com
. $self->name->fqdn
should be either equal to or a subdomain of $rr->owner
. Don’t we have a function that tests whether a domain is equal to or a subdomain of another?
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.
Yes this implementation is more than a year old and things have changed since. This PR is a draft, so don't bother reviewing it; it will have changed a lot once its ready. I just rebased on latest develop yesterday to be able to start working on it again.
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.
This PR should now be in a better state for review.
- Add new attribute 'dname' to Engine::Zone objects - Update implementation to account for that new attribute and make queries to the appropriate delegation name, if one exists
# Break if there are too many records | ||
if ( scalar @dname_rrs > $DNAME_MAX_RECORDS ) { | ||
return 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.
We could do this check earlier, before we start removing duplicate DNAME resource records maybe.
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.
My original intention was indeed to check for this limit after removing duplicate RRs. I could see both work. @matsduf any thoughts on this?
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 new code lacks unit tests. It could be made more testable if _build_dname()
were broken up in two pieces: one function doing the DNAME query and another processing the response packet. The second function could then be tested using synthetic response packets.
lib/Zonemaster/Engine/Zone.pm
Outdated
} | ||
else { | ||
$duplicate_dname_rrs{$rr_hash} = 0; | ||
push @original_rrs, $rr; |
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.
How about calling it @unique_rrs
instead?
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.
Sure, I think it conveys better meaning too. I'll also change it in the CNAME counterpart of the similar algorithm in Zonemaster::Engine::Recursor->_resolve_cname()
.
Updated.
lib/Zonemaster/Engine/Zone.pm
Outdated
|
||
my ( %dnames, %seen_targets, %forbidden_targets ); | ||
for my $rr ( @dname_rrs ) { | ||
my $rr_owner = Zonemaster::Engine::DNSName->new( lc( $rr->owner) ); |
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.
my $rr_owner = Zonemaster::Engine::DNSName->new( lc( $rr->owner) ); | |
my $rr_owner = Zonemaster::Engine::DNSName->new( lc( $rr->owner ) ); |
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.
Updated
lib/Zonemaster/Engine/Zone.pm
Outdated
} | ||
|
||
# DNAME owner name is target, or target has already been seen in this response, or owner name cannot be a target | ||
if ( $rr_owner eq $rr_target or exists $seen_targets{$rr_target} or grep { $_ eq $rr_target } ( keys %forbidden_targets ) ) { |
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.
if ( $rr_owner eq $rr_target or exists $seen_targets{$rr_target} or grep { $_ eq $rr_target } ( keys %forbidden_targets ) ) { | |
if ( $rr_owner eq $rr_target or exists $seen_targets{$rr_target} or exists $forbidden_targets{$rr_target} ) { |
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.
Updated. I also removed the %forbidden_targets
hash entirely, as case preservation is not needed for this method (no direct re-query, as opposed to Zonemaster::Engine::Recursor->_resolve_cname()
), see https://github.com/zonemaster/zonemaster-engine/pull/1288/files#r1410588480)
lib/Zonemaster/Engine/Zone.pm
Outdated
# Make sure that the DNAME chain from the RRs is not broken | ||
if ( $dname_counter != scalar @dname_rrs ) { | ||
return 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.
Is this necessary? If the chain is broken, wouldn’t it make more sense to just follow it until we can’t?
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.
What does broken DNAME chain mean? Does it just mean that the domain name in RDATA does not exist? Another way a DNAME chain (or DNAME record) can be broken is that it points to itself or below itself.
foo.xa. DNAME foo.xa.
foo.xa. DNAME www.foo.xa.
And of course, we can create a loop, just like with CNAMEs.
foo.xa. DNAME bar.xa.
bar.xa. DNAME foo.xa.
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.
Is this necessary? If the chain is broken, wouldn’t it make more sense to just follow it until we can’t?
This check is to make sure that the previous while()
looped the correct amount of times when going over the DNAME RRset to reach the correct DNAME target. For example, if there are three (validated) DNAME records in the RRset, it should loop three times.
Note that this RRset has already been validated in the previous steps, i.e. in the previous two for()
loops (removal of duplicates and then aborting on illegal $rr_{owner, target}
combinations).
It should not be necessary (due to the previous verification) but in general I prefer to have such safe-guards.
Another way a DNAME chain (or DNAME record) can be broken is that it points to itself or below itself.
foo.xa. DNAME foo.xa. foo.xa. DNAME www.foo.xa.
Ah, regarding the second case, I didn't have a subdomain check implemented. Added.
And of course, we can create a loop, just like with CNAMEs.
This is already handled in the implementation.
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.
For example, if there are three (validated) DNAME records in the RRset, it should loop three times.
What do you mean by that? There cannot be more than one DNAME in a DNAME RRset. A name can only point at one other name.
Purpose
This PR proposes an implementation for the support of DNAME by adding a new attribute to
Zonemaster::Engine::Zone
objects, instead of modifyingZonemaster::Engine::Recursor
.This means that this PR does not make recursive lookups of Zonemaster follow DNAME records (similar to #1288 for CNAME), but that querying zone data (e.g glue, name servers, etc) for any name (zone) that is an alias (DNAME) will return the proper data for that zone.
TBD:
Context
Relates to zonemaster/zonemaster#1075 and zonemaster/zonemaster#472
Depends on zonemaster/zonemaster-ldns#170
Changes
Zonemaster::Engine::Zone
objectsHow to test this PR
Unit tests are to be created. So far many existing unit tests will fail because of incomplete unit test data (I haven't re-recorded them yet).
Manual testing (zone:
xn--mori-qsa.nz
):