Add DNAME support (to Zone objects) #1213

tgreenx · 2023-04-05T14:42:52Z

Purpose

This PR proposes an implementation for the support of DNAME by adding a new attribute to Zonemaster::Engine::Zone objects, instead of modifying Zonemaster::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:

Discuss this implementation.
Add and update unitary tests, and re-record unit test data

Context

Relates to zonemaster/zonemaster#1075 and zonemaster/zonemaster#472

Depends on zonemaster/zonemaster-ldns#170

Changes

Add new attribute 'dname' to Zonemaster::Engine::Zone objects
Update implementation to account for that new attribute and make queries to the appropriate zone name, if any

How 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):

$ perl -MZonemaster::Engine -E 'my $zone = Zonemaster::Engine::Zone->new({ name => Zonemaster::Engine::DNSName->from_string("xn--mori-qsa.nz") }); say "zone: ", $zone->name; $zone->dname ? say "dname: ", $zone->dname->name : say "dname: undef"; say "ns:\n\t", join "\n\t", @{$zone->ns} if $zone->dname;'
zone: xn--mori-qsa.nz
dname: maori.nz
ns:
        ns1.dns.net.nz/2001:dce:2000:2::130
        ns1.dns.net.nz/202.46.190.130
        ns2.dns.net.nz/2001:dce:7000:2::130
        ns2.dns.net.nz/202.46.187.130
        ns3.dns.net.nz/2001:dce:d453::53
        ns3.dns.net.nz/202.46.188.130
        ns4.dns.net.nz/2001:dce:d454::53
        ns4.dns.net.nz/202.46.189.130
        ns5.dns.net.nz/185.159.197.130
        ns5.dns.net.nz/2620:10a:80aa::130
        ns6.dns.net.nz/185.159.198.130
        ns6.dns.net.nz/2620:10a:80ab::130
        ns7.dns.net.nz/194.146.106.54
        ns7.dns.net.nz/2001:67c:1010:13::53

lib/Zonemaster/Engine/Zone.pm

matsduf · 2023-04-05T16:48:44Z

lib/Zonemaster/Engine/Zone.pm

+    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 ) ) );
+        }
+    }


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.

But it is possible to have a chain of DNAME records, and possibly with some CNAME records in the chain too.

matsduf · 2023-04-06T15:38:15Z

I am not convinced that advanced support of DNAME is needed. It is at least not for BASIC01.

matsduf · 2023-04-09T13:47:16Z

As conluded in #1212 (comment) BASIC01 does not require advanced DNAME function.

If a DNAME record is present in example.com (with target.xa as target) then that will not be seen in queries for records in example.com (unless DNAME is the query type). Queries for sub.example.com (or next.sub.example.com) will result in the DNAME record plus a synthesized CNAME record being included. The CNAME record for our exampel will be sub.example.com. CNAME sub.target.xa. (next.sub.example.com. CNAME next.sub.target.xa.).

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 example.com. DNAME sub.example.com. then that seems to be handled differently on different name server software. If Zonemaster needs any special handling of DNAME then possibly that target of the DNAME record must not be a subdomain of owner name of the record. And such a test should then be done any time a DNAME record is included in the response. That would then be a SYSTEM message since that is not bound to a specific test case.

tgreenx · 2023-04-13T10:01:11Z

Considering the above comments, this work is put on hold until next release.

marc-vanderwal · 2024-07-18T09:53:29Z

lib/Zonemaster/Engine/Zone.pm

+    my @dnames;
+
+    foreach my $rr ( $p->get_records( 'DNAME', q{answer} ) ) {
+        if ( index( $self->name->fqdn, lc($rr->owner) ) != -1 ) {


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?

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.

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

marc-vanderwal · 2024-09-03T05:50:10Z

lib/Zonemaster/Engine/Zone.pm

+    # Break if there are too many records
+    if ( scalar @dname_rrs > $DNAME_MAX_RECORDS ) {
+        return undef;
+    }


We could do this check earlier, before we start removing duplicate DNAME resource records maybe.

My original intention was indeed to check for this limit after removing duplicate RRs. I could see both work. @matsduf any thoughts on this?

marc-vanderwal

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.

marc-vanderwal · 2024-09-03T05:53:46Z

lib/Zonemaster/Engine/Zone.pm

+        }
+        else {
+            $duplicate_dname_rrs{$rr_hash} = 0;
+            push @original_rrs, $rr;


How about calling it @unique_rrs instead?

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.

marc-vanderwal · 2024-09-03T05:55:02Z

lib/Zonemaster/Engine/Zone.pm

+
+    my ( %dnames, %seen_targets, %forbidden_targets );
+    for my $rr ( @dname_rrs ) {
+        my $rr_owner = Zonemaster::Engine::DNSName->new( lc( $rr->owner) );


Suggested change

my $rr_owner = Zonemaster::Engine::DNSName->new( lc( $rr->owner) );

my $rr_owner = Zonemaster::Engine::DNSName->new( lc( $rr->owner ) );

marc-vanderwal · 2024-09-03T05:57:23Z

lib/Zonemaster/Engine/Zone.pm

+        }
+
+        # 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 ) ) {


Suggested change

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} ) {

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)

marc-vanderwal · 2024-09-03T06:02:09Z

lib/Zonemaster/Engine/Zone.pm

+    # Make sure that the DNAME chain from the RRs is not broken
+    if ( $dname_counter != scalar @dname_rrs ) {
+        return undef;
+    }


Is this necessary? If the chain is broken, wouldn’t it make more sense to just follow it until we can’t?

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.

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.

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.

tgreenx added T-Feature Type: New feature in software or test case description V-Major Versioning: The change gives an update of major in version. labels Apr 5, 2023

tgreenx added this to the v2023.1 milestone Apr 5, 2023

tgreenx requested review from mattias-p, matsduf, hannaeko, a user and marc-vanderwal April 5, 2023 14:42

tgreenx mentioned this pull request Apr 5, 2023

Update Basic01 #1212

Merged

1 task

matsduf reviewed Apr 5, 2023

View reviewed changes

tgreenx modified the milestones: v2023.1, v2023.2 Apr 13, 2023

matsduf modified the milestones: v2023.2, v2024.1 Nov 22, 2023

tgreenx modified the milestones: v2024.1, v2024.2 Jun 12, 2024

tgreenx force-pushed the add-dname branch from 3bfc375 to 3ecc03d Compare July 17, 2024 17:42

marc-vanderwal reviewed Jul 18, 2024

View reviewed changes

tgreenx removed request for mattias-p and hannaeko July 18, 2024 11:26

Add DNAME support - initial commit

de5e35f

- 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

tgreenx force-pushed the add-dname branch from 3ecc03d to de5e35f Compare July 25, 2024 16:03

tgreenx changed the title ~~Add DNAME support~~ Add DNAME support (to Zone objects) Jul 25, 2024

tgreenx marked this pull request as ready for review July 25, 2024 16:45

tgreenx requested review from mattias-p, MichaelTimbert and matsduf July 25, 2024 16:45

tgreenx requested a review from marc-vanderwal July 25, 2024 16:45

marc-vanderwal reviewed Sep 3, 2024

View reviewed changes

Updates after review

e6e6d93

tgreenx requested a review from marc-vanderwal September 3, 2024 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DNAME support (to Zone objects) #1213

Add DNAME support (to Zone objects) #1213

tgreenx commented Apr 5, 2023 •

edited

Loading

matsduf Apr 5, 2023

matsduf Jul 18, 2024

matsduf commented Apr 6, 2023

matsduf commented Apr 9, 2023

tgreenx commented Apr 13, 2023

marc-vanderwal Jul 18, 2024

tgreenx Jul 18, 2024

tgreenx Jul 25, 2024 •

edited

Loading

marc-vanderwal Sep 3, 2024

tgreenx Sep 3, 2024

marc-vanderwal left a comment

marc-vanderwal Sep 3, 2024

tgreenx Sep 3, 2024

marc-vanderwal Sep 3, 2024

tgreenx Sep 3, 2024

marc-vanderwal Sep 3, 2024

tgreenx Sep 3, 2024

marc-vanderwal Sep 3, 2024

matsduf Sep 3, 2024

tgreenx Sep 3, 2024

matsduf Sep 4, 2024

	my $rr_owner = Zonemaster::Engine::DNSName->new( lc( $rr->owner) );
	my $rr_owner = Zonemaster::Engine::DNSName->new( lc( $rr->owner ) );

	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} ) {

Add DNAME support (to Zone objects) #1213

Are you sure you want to change the base?

Add DNAME support (to Zone objects) #1213

Conversation

tgreenx commented Apr 5, 2023 • edited Loading

Purpose

Context

Changes

How to test this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Apr 6, 2023

matsduf commented Apr 9, 2023

tgreenx commented Apr 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgreenx Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-vanderwal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgreenx commented Apr 5, 2023 •

edited

Loading

tgreenx Jul 25, 2024 •

edited

Loading