Add CNAME followage in Recursor #1288

tgreenx · 2023-09-07T16:58:47Z

Purpose

This PR makes the recursive lookup functionality of Zonemaster able to follow CNAME redirections.

Attempts to follow CNAMEs will be made when all of the following are true:

the response has RCODE "NoError"
the answer section of the response does not contain records of the queried type, but does contain at least one CNAME record for the query name
the answer section of the response does not contain multiple CNAME records with the same owner name
the final target of the CNAME record(s) chain has not been followed before
there are no records of the queried type with owner name as the final target of the CNAME record(s)

Context

Necessary for #1257 (#1257 (comment))

Test Zones specification: zonemaster/zonemaster#1220

Changes

Update recursive lookup code (lib/Zonemaster/Engine/Recursor.pm)
Add system debug message tags CNAME_START, CNAME_RECORDS_DUPLICATES, CNAME_CHAIN_TOO_LONG, CNAME_LOOP_INNER, CNAME_LOOP_OUTER, CNAME_NO_MATCH, CNAME_RECORDS_CHAIN_BROKEN, CNAME_MULTIPLE_FOR_NAME and CNAME_RECORDS_TOO_MANY
Add constants CNAME_MAX_CHAIN_LENGTH and CNAME_MAX_RECORDS
Update test cases related to CNAME (where applicable): Syntax06 and Zone07
Add and update unitary tests
Add and update documentation

How to test this PR

Unit tests are updated and should pass (based on zonemaster/zonemaster#1220).

Manual testing:
You can can test any zone by providing it to Zonemaster::Engine->recurse(), e.g. with any of the zones from zonemaster/zonemaster#1220. See below:

$ perl -MZonemaster::Engine -E 'Zonemaster::Engine->add_fake_delegation( ( "." => { ns1 => [ "127.1.0.1", "fda1:b2:c3::127:1:0:1" ], ns2 => [ "127.1.0.2", "fda1:b2:c3::127:1:0:2" ] } ) ); my $p = Zonemaster::Engine->recurse("good-cname-1.cname.recursor.engine.xa"); for my $entry ( @{ Zonemaster::Engi
ne->logger->entries } ) { say "\n", $entry if index($entry->tag, "CNAME") != -1 }; say "\n", $p->string if defined $p'

;; ->>HEADER<<- opcode: QUERY, rcode: NOERROR, id: 24524
;; flags: qr aa ; QUERY: 1, ANSWER: 2, AUTHORITY: 1, ADDITIONAL: 0
;; QUESTION SECTION:
;; good-cname-1.cname.recursor.engine.xa.       IN      A

;; ANSWER SECTION:
good-cname-1.cname.recursor.engine.xa.  3600    IN      CNAME   good-cname-1-target.cname.recursor.engine.xa.
good-cname-1-target.cname.recursor.engine.xa.   3600    IN      A       127.0.0.1

;; AUTHORITY SECTION:
cname.recursor.engine.xa.       3600    IN      NS      ns1.cname.recursor.engine.xa.

;; ADDITIONAL SECTION:

;; Query time: 0 msec
;; EDNS: version 0; flags: ; udp: 512
;; SERVER: fda1:b2:c3:0:127:30:1:31
;; WHEN: Tue Nov 28 12:30:33 2023
;; MSG SIZE  rcvd: 287

$ perl -MZonemaster::Engine -E 'Zonemaster::Engine->add_fake_delegation( ( "." => { ns1 => [ "127.1.0.1", "fda1:b2:c3::127:1:0:1" ], ns2 => [ "127.1.0.2", "fda1:b2:c3::127:1:0:2" ] } ) ); my $p = Zonemaster::Engine->recurse("mult-cname.cname.recursor.engine.xa"); for my $entry ( @{ Zonemaster::Engine
->logger->entries } ) { say "\n", $entry if index($entry->tag, "CNAME") != -1 }; say "\n", $p->string if defined $p'

SYSTEM:UNSPECIFIED:CNAME_MULTIPLE_FOR_NAME name=mult-cname.cname.recursor.engine.xa

$ perl -MZonemaster::Engine -E 'Zonemaster::Engine->add_fake_delegation( ( "." => { ns1 => [ "127.1.0.1", "fda1:b2:c3::127:1:0:1" ], ns2 => [ "127.1.0.2", "fda1:b2:c3::127:1:0:2" ] } ) ); my $p = Zonemaster::Engine->recurse("looped-cname-out-of-zone.sub2.cname.recursor.engine.xa"); for my $entry ( @{ Zonemaster::Engine->logger->entries } ) { say "\n", $entry if index($entry->tag, "CNAME") != -1 }; say "\n", $p->string if defined $p'

SYSTEM:UNSPECIFIED:CNAME_LOOP_OUTER cnames=looped-cname-out-of-zone.sub2.cname.recursor.engine.xa;looped-cname-out-of-zone.sub3.cname.recursor.engine.xa; name=looped-cname-out-of-zone.sub2.cname.recursor.engine.xa; target="looped-cname-out-of-zone.sub3.cname.recursor.engine.xa"

matsduf · 2023-10-31T12:38:58Z

the response (answer) does not contain records related to the query (question), but does contain at least one CNAME record for the name

I will be clearer to say that the answer section of the response does not contain any record with the same type as the query type. And that the answer section contains one CNAME with the same owner name as the query name. It may contain additional CNAME records of all CNAME records form a single CNAME chain.

the response (answer) does not contain multiple CNAME records for the same name

Not multipel CNAME records with the same owner name.

the target of the CNAME record has not been followed before (case-insensitive)

What does that mean?

tgreenx · 2023-10-31T16:00:39Z

the response (answer) does not contain records related to the query (question), but does contain at least one CNAME record for the name

I will be clearer to say that the answer section of the response does not contain any record with the same type as the query type. And that the answer section contains one CNAME with the same owner name as the query name. It may contain additional CNAME records of all CNAME records form a single CNAME chain.

the response (answer) does not contain multiple CNAME records for the same name

Not multipel CNAME records with the same owner name.
* the target of the CNAME record has not been followed before (case-insensitive)
What does that mean?

Sure, I will make an update to provide better wording according to your suggestions.
In the meantime, I have just pushed a commit that adds new unitary tests (and test zones). This should answer your concerns. The "How to test this PR" section has also been updated.

matsduf · 2023-10-31T16:02:32Z

share/profile.json

+            "CNAME_LOOP_INNER" : "DEBUG",
+            "CNAME_LOOP_OUTER" : "DEBUG",
+            "CNAME_MULTIPLE_FOR_NAME" : "DEBUG",


Can these three be defined or described in Recursor.pm?

What do you mean by defined/described? Do you mean adding descriptive message ids such as in Zonemaster::Engine::Translator, or expanding the POD documentation of the associated function that output them (i.e. Zonemaster::Engine::Recursor::recurse())?

I do not think it is necessary to have a message ID on those, which would put a burden on the translators. Definition or description in POD would be fine.

tgreenx · 2023-10-31T16:33:31Z

Sure, I will make an update to provide better wording according to your suggestions.

Done. Description of this PR and commit's log message are now updated.

tgreenx · 2023-11-02T14:17:15Z

Zone files:

$TTL 3600
$ORIGIN recursor.xa.

@                       IN  SOA  (
                                   ns1.xa.
                                   marc\.vanderwal.afnic.fr.
                                   2023103100
                                   86400
                                   14400
                                   3600000
                                   3600
                                 )

                            NS   ns1.xa.
                            NS   ns2.xa.

$ORIGIN good-cname.recursor.xa.

target                  IN  A      192.0.2.1
                        IN  AAAA   2001:db8:8::1
                        IN  TXT    "example text contents"

alias                   IN  CNAME  target

chain                   IN  CNAME  chain0
chain0                  IN  CNAME  chain1
chain1                  IN  CNAME  target

$ORIGIN bad-cname.recursor.xa.

target                  IN  A      192.0.2.1
                        IN  AAAA   2001:db8:8::1
                        IN  TXT    "example text contents"

target0                 IN  TXT    "target0"
target1                 IN  TXT    "target1"

loop0                   IN  CNAME  LOOP0

loop                    IN  CNAME  a.LoOp
a.lOoP                  IN  CNAME  B.LOOP
b.loop                  IN  CNAME  a.loop

loop2                   IN  NS     ns2.xa.
loop3                   IN  NS     ns3.xa.

illegal0                IN  CNAME  target
iLLeGAL0                IN  A      192.0.2.2

illegal1                IN  CNAME  target0
ILLEGAL1                IN  CNAME  target1

$TTL 3600
$ORIGIN loop2.bad-cname.recursor.xa.

@                       IN  SOA  (
                                   ns2.xa.
                                   marc\.vanderwal.afnic.fr.
                                   2023103100
                                   86400
                                   14400
                                   3600000
                                   3600
                                 )

                            NS   ns2.xa.

x                           CNAME y.loop3.bad-cname.recursor.xa.

$TTL 3600
$ORIGIN loop3.bad-cname.recursor.xa.

@                       IN  SOA  (
                                   ns3.xa.
                                   marc\.vanderwal.afnic.fr.
                                   2023103100
                                   86400
                                   14400
                                   3600000
                                   3600
                                 )

                            NS   ns3.xa.

y                           CNAME x.loop2.bad-cname.recursor.xa.

matsduf · 2023-11-02T16:57:05Z

What is the purpose of mixing case in

illegal1                IN  CNAME  target0
ILLEGAL1                IN  CNAME  target1

and others?

tgreenx · 2023-11-02T17:30:30Z

What is the purpose of mixing case in
illegal1                IN  CNAME  target0
ILLEGAL1                IN  CNAME  target1
and others?

Generally it is to test case-insensitivity of the implementation. But here it is also about testing that several CNAME records with the same owner name (with different cases) lead to an error, i.e. that the implementation outputs CNAME_MULTIPLE_FOR_NAME (this part of the code).

matsduf · 2023-11-02T21:27:15Z

Generally it is to test case-insensitivity of the implementation. But here it is also about testing that several CNAME records with the same owner name (with different cases) lead to an error, i.e. that the implementation outputs CNAME_MULTIPLE_FOR_NAME (this part of the code).

Then, shouldn't there be a test where the two CNAME records have identical owner names in a case sensitive comparison. too?

matsduf · 2023-11-03T15:40:40Z

t/recursor-A.t

+# Good CNAMEs
+my $p = Zonemaster::Engine->recurse( 'alias.good-cname.recursor.xa' );
+isa_ok( $p, 'Zonemaster::Engine::Packet' );
+is( scalar( $p->answer ), 1, 'one answer record' );


I suggest "one DNS record in answer section" if that is what it means.

matsduf · 2023-11-03T15:41:58Z

t/recursor-A.t

+$p = Zonemaster::Engine->recurse( 'chain.good-cname.recursor.xa' );
+isa_ok( $p, 'Zonemaster::Engine::Packet' );
+is( scalar( $p->answer ), 1, 'one answer record' );
+is( name( ($p->answer)[0]->owner ), 'target.good-cname.recursor.xa', 'RR name ok' );


The answer section will contain four DNS records, as far as I can see:

;; ANSWER SECTION: chain.good-cname.recursor.core.xa. 3600 IN CNAME chain0.good-cname.recursor.core.xa. chain0.good-cname.recursor.core.xa. 3600 IN CNAME chain1.good-cname.recursor.core.xa. chain1.good-cname.recursor.core.xa. 3600 IN CNAME target.good-cname.recursor.core.xa. target.good-cname.recursor.core.xa. 3600 IN A 127.0.0.1

This is indeed the answer section of the first response, on A query for name chain.good-cname.recursor.xa.. But the $p object only contains the final response, which is, after CNAME followage, from the A query of the CNAME target (target.good-cname.recursor.xa.):

$ perl -MZonemaster::Engine -E 'Zonemaster::Engine::Recursor->remove_fake_addresses("."); Zonemaster::Engine->add_fake_delegation( ( "." => { ibns01.labs.prive.nic.fr => [ "10.1.72.23" ] } ) ); my $p = Zonemaster::Engine->recurse("chain.good-cname.recursor.xa", "A"); for my $entry ( @{ Zonemaster::Engine->logger->entries } ) { say "\n", $entry if index($entry->tag, "CNAME") != -1 }; say $p->string;' ;; ->>HEADER<<- opcode: QUERY, rcode: NOERROR, id: 57427 ;; flags: qr aa ; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 0 ;; QUESTION SECTION: ;; target.good-cname.recursor.xa. IN A ;; ANSWER SECTION: target.good-cname.recursor.xa. 3600 IN A 192.0.2.1 ;; AUTHORITY SECTION: ;; ADDITIONAL SECTION: ;; Query time: 17 msec ;; SERVER: 10.1.72.23 ;; WHEN: Mon Nov 6 11:57:35 2023 ;; MSG SIZE rcvd: 63

So if there are three A records in target.good-cname.recursor.xa then that would match is( scalar( $p->answer ), 3, 'three answer records' );

Yes. But note that more generally scalar( $p->answer ) will count the number of (any) ressource records in the answer section of a packet. So if there are RRs with owner name different than QNAME they would be counted too. If you only want to count RRs with specific owner names or type, then you can use scalar( $p->get_records_for_name('A', 'target.good-cname.recursor.xa', 'answer')).

But for test zones, since you write the zone's content, I guess scalar( $p->answer ) is fine.

I get two records in the response and the unit test fails:

$ perl -MZonemaster::Engine -E 'Zonemaster::Engine::Recursor->remove_fake_addresses("."); Zonemaster::Engine->add_fake_delegation( ( "." => { ns1 => [ "127.1.0.1" ] } ) ); my $p = Zonemaster::Engine->recurse("good-cname-1.cname.recursor.engine.xa", "A"); for my $entry ( @{ Zonemaster::Engine->logger->entries } ) { say "\n", $entry if index($entry->tag, "CNAME") != -1 }; say $p->string;' ;; ->>HEADER<<- opcode: QUERY, rcode: NOERROR, id: 45265 ;; flags: qr aa ; QUERY: 1, ANSWER: 2, AUTHORITY: 1, ADDITIONAL: 0 ;; QUESTION SECTION: ;; good-cname-1.cname.recursor.engine.xa. IN A ;; ANSWER SECTION: good-cname-1.cname.recursor.engine.xa. 3600 IN CNAME good-cname-1-target.cname.recursor.engine.xa. good-cname-1-target.cname.recursor.engine.xa. 3600 IN A 127.0.0.1 ;; AUTHORITY SECTION: cname.recursor.engine.xa. 3600 IN NS ns1.cname.recursor.engine.xa. ;; ADDITIONAL SECTION: ;; Query time: 0 msec ;; EDNS: version 0; flags: ; udp: 512 ;; SERVER: fda1:b2:c3:0:127:30:1:31 ;; WHEN: Wed Nov 15 09:40:41 2023 ;; MSG SIZE rcvd: 287

matsduf · 2023-11-05T23:09:41Z

@tgreenx, How should the unit tests react on NODATA and NXDOMAIN responses, respectively?

NODATA:

; <<>> DiG 9.18.18-0ubuntu0.22.04.1-Ubuntu <<>> @127.31.1.31 alias-nodata.recursor.core.xa A
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 38985
;; flags: qr aa rd; QUERY: 1, ANSWER: 1, AUTHORITY: 1, ADDITIONAL: 1
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1232
; COOKIE: a15d870f95dcc703 (echoed)
;; QUESTION SECTION:
;alias-nodata.recursor.core.xa.	IN	A

;; ANSWER SECTION:
alias-nodata.recursor.core.xa. 3600 IN	CNAME	target-nodata.recursor.core.xa.

;; AUTHORITY SECTION:
recursor.core.xa.	3600	IN	NS	ns1.recursor.core.xa.

;; Query time: 0 msec
;; SERVER: 127.31.1.31#53(127.31.1.31) (UDP)
;; WHEN: Sun Nov 05 23:02:53 UTC 2023
;; MSG SIZE  rcvd: 193

NXDOMAIN:

; <<>> DiG 9.18.18-0ubuntu0.22.04.1-Ubuntu <<>> @127.31.1.31 alias-nxdomain.recursor.core.xa A
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NXDOMAIN, id: 11309
;; flags: qr aa rd; QUERY: 1, ANSWER: 1, AUTHORITY: 1, ADDITIONAL: 1
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1232
; COOKIE: c82ff6fab047c6dc (echoed)
;; QUESTION SECTION:
;alias-nxdomain.recursor.core.xa. IN	A

;; ANSWER SECTION:
alias-nxdomain.recursor.core.xa. 3600 IN CNAME	target-nxdomain.recursor.core.xa.

;; AUTHORITY SECTION:
recursor.core.xa.	3600	IN	NS	ns1.recursor.core.xa.

;; Query time: 0 msec
;; SERVER: 127.31.1.31#53(127.31.1.31) (UDP)
;; WHEN: Sun Nov 05 23:02:45 UTC 2023
;; MSG SIZE  rcvd: 199

And how will the code handle broken chain (and here with a duplicate CNAME record)?

broken-chain.recursor.core.xa. --> broken-chain2.recursor.core.xa. --> broken-chain3.recursor.core.xa. --> NULL

NULL --> broken-chain-target.recursor.core.xa. 100 IN A 127.0.0.1

; <<>> DiG 9.18.18-0ubuntu0.22.04.1-Ubuntu <<>> @127.31.1.31 broken-chain.recursor.core.xa A
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 54152
;; flags: qr aa rd; QUERY: 1, ANSWER: 4, AUTHORITY: 0, ADDITIONAL: 1
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1232
; COOKIE: 83ca9efec06ee54d (echoed)
;; QUESTION SECTION:
;broken-chain.recursor.core.xa.	IN	A

;; ANSWER SECTION:
broken-chain.recursor.core.xa. 100 IN	CNAME	broken-chain2.recursor.core.xa.
broken-chain2.recursor.core.xa.	3600 IN	CNAME	broken-chain3.recursor.core.xa.
broken-chain2.recursor.core.xa.	100 IN	CNAME	broken-chain3.recursor.core.xa.
broken-chain-target.recursor.core.xa. 100 IN A	127.0.0.1

;; Query time: 0 msec
;; SERVER: 127.31.1.31#53(127.31.1.31) (UDP)
;; WHEN: Sun Nov 05 23:02:21 UTC 2023
;; MSG SIZE  rcvd: 343

tgreenx · 2023-12-07T18:55:25Z

@matsduf All comments have been addressed in 5756f49. Also note that with the removal of duplicate records all unit tests now work as expected too.

matsduf · 2023-12-11T13:42:25Z

lib/Zonemaster/Engine/Recursor.pm

+If CNAMEs are successfully resolved, a L<packet|Zonemaster::Engine::Packet> (which could be C<undef>) is returned along with
+one of the following message tags: CNAME_FOLLOWED_IB, CNAME_FOLLOWED_OOB.
+Note that CNAME records are also validated and, in case of an error, an empty (C<undef>) L<packet|Zonemaster::Engine::Packet>
+is returned and one of the following message tags will be logged: CNAME_CHAIN_TOO_LONG, CNAME_LOOP_INNER, CNAME_LOOP_OUTER,
+CNAME_NO_MATCH, CNAME_RECORDS_CHAIN_BROKEN, CNAME_RECORDS_MULTIPLE_FOR_NAME, CNAME_RECORDS_TOO_MANY.


I suggest CNAME_FOLLOWED_IZ ("in zone") and CNAME_FOLLOWED_OOZ ("out-of-zone"), respectively, instead of CNAME_FOLLOWED_IB and CNAME_FOLLOWED_OOB. That would capture the meaning, wouldn't it?

Could you make a list of the tags with one tag per "=item" and include a short description of what the tag stands for?

Updated. I didn't include a short description though considering that the message tags are sufficiently self-explanatory.

matsduf · 2024-04-10T07:53:17Z

@tgreenx, could we complete this together with Test zones for the CNAME function in Recursor.pm in Engine?

tgreenx · 2024-04-10T12:46:19Z

@tgreenx, could we complete this together with Test zones for the CNAME function in Recursor.pm in Engine?

Yes this should be finished in time.

This commit makes Engine able to follow CNAMEs when doing recursive lookups. Currently CNAMEs will be followed when all of the following are true: - the response has RCODE "NoError" - the answer section of the response does not contain records of the queried type, but does contain at least one CNAME record for the query name - the answer section of the response does not contain multiple CNAME records with the same owner name - the final target of the CNAME record(s) chain has not been followed before - there are no records of the queried type with owner name as the final target of the CNAME record(s) Three system, debug level messages are created: 'CNAME_LOOP_INNER', 'CNAME_LOOP_OUTER' and 'CNAME_MULTIPLE_FOR_NAME'. Some test cases have been modified to account for this new behavior. Unitary tests have also been updated.

- Move CNAME resolution to a dedicated internal method 'Zonemaster::Engine::Recursor::_resolve_cname()' - Various refactoring (renaming of variables, removal of unneeded code, etc) - Update Test Cases code that relates to CNAME - Add documentation for 'Zonemaster::Engine::Recursor::_resolve_cname()' and 'Zonemaster::Engine::Recursor::_recurse()' - Update unit tests and unit tests data

- Add constants CNAME_MAX_RECORDS and CNAME_MAX_CHAIN_LENGTH - Add message tags CNAME_START, CNAME_RECORDS_TOO_MANY, CNAME_RECORDS_CHAIN_BROKEN, CNAME_CHAIN_TOO_LONG, CNAME_FOLLOWED_IB, CNAME_FOLLOWED_OOB, CNAME_NO_MATCH - Rename message tag CNAME_MULTIPLE_FOR_NAME to CNAME_RECORDS_MULTIPLE_FOR_NAME - Add stopping conditions based on CNAME_MAX_RECORDS and CNAME_MAX_CHAIN_LENGTH - Check that CNAME target is out of zone before making a new recursive lookup for that name - Document further Zonemaster::Engine::Recursor::_recurse() - Update unit tests

- Lower value of constant CNAME_MAX_RECORDS from 10 to 9 - Remove duplicates CNAME RRs - Add message tag CNAME_RECORDS_DUPLICATES - Adjust logging level of some message tags - Refactoring - Update documentation - Update unit tests

- Rename CNAME_FOLLOWED_IB to CNAME_FOLLOWED_IN_ZONE and CNAME_FOLLOWED_OOB to CNAME_FOLLOWED_OUT_OF_ZONE - Update documentation

tgreenx · 2024-05-14T16:00:53Z

@matsduf please re-review. I addressed your latest comment, and rebased on latest develop.

lib/Zonemaster/Engine/Recursor.pm

marc-vanderwal · 2024-05-21T12:48:43Z

lib/Zonemaster/Engine/Recursor.pm

+    # Remove duplicate CNAME RRs
+    my ( %duplicate_cname_rrs, @original_rrs );
+    for my $rr ( @cname_rrs ) {
+        my $rr_hash = $rr->class . '/' . $rr->ttl . '/CNAME/' . $rr->owner . '/' . $rr->cname;


Suppose a response contains two identical CNAME records that only differ by TTL. Should we consider them as duplicates? I’d say yes.

And how about two CNAME records that only differ by case in the owner name, CNAME name, or both? I think they should be considered duplicate too.

marc-vanderwal · 2024-05-21T12:54:45Z

lib/Zonemaster/Engine/Recursor.pm

+        }
+
+        # CNAME owner name is target, or target has already been seen in this response, or owner name cannot be a target
+        if ( lc( $rr_owner ) eq lc( $rr_target ) or exists $seen_targets{lc( $rr_target )} or grep( /^$rr_target$/, keys %forbidden_targets ) ) {


The grep can have odd side-effects if $rr_target contains names with special characters in them. It was also missing a conversion to lowercase. It’s better to do:

grep { lc( $_ ) eq $rr_target } ( keys %forbidden_targets )

I think you meant grep { $_ eq lc( $rr_target ) } ( keys %forbidden_targets ) instead.
Fixed.

marc-vanderwal · 2024-05-21T12:56:50Z

lib/Zonemaster/Engine/Recursor.pm

+
+        $seen_targets{lc( $rr_target )} = 1;
+        $forbidden_targets{lc( $rr_owner )} = 1;
+        $cnames{$rr_owner} = $rr_target;


We should do case-insensitive comparisons when checking whether we’ve already queried the name before (and thus, detect loops). But we should also be case-preserving: if foo.example is an alias to BAR.example, we should query BAR.example, not bar.example, even though both of these QNAMEs should in theory get us the same RRset.

matsduf · 2024-05-22T11:46:56Z

Is case preservation really needed here? I cannot see that is wrong to query for bar.xa even it RDATA of the CNAME record was BAR.XA.

marc-vanderwal · 2024-05-24T09:11:25Z

Is case preservation really needed here? I cannot see that is wrong to query for bar.xa even it RDATA of the CNAME record was BAR.XA.

RFC 1034 does, in fact, state that:

When you receive a domain name or label, you should preserve its case.

My understanding is that when you “receive” a CNAME, if the resolver decides to follow it, the follow-up query’s QNAME should retain the case exactly as it was received in the previous response.

So in a nutshell: it’s better to preserve case here. If not, I feel like we are adding an assumption to Recursor that the name server will give identical RRsets when queried for BAR.XA and bar.xa. For the initial use case of this code, it’s probably overkill. But if we decide to use the same Recursor to query untrusted nameservers, than anything goes.

- Omit TTL and names case in resource record duplicate comparison - Fix condition

tgreenx

@marc-vanderwal @matsduf @mattias-p Comments have been addressed, please re-review.

tgreenx · 2024-06-10T15:14:03Z

lib/Zonemaster/Engine/Recursor.pm

+    # Remove duplicate CNAME RRs
+    my ( %duplicate_cname_rrs, @original_rrs );
+    for my $rr ( @cname_rrs ) {
+        my $rr_hash = $rr->class . '/' . $rr->ttl . '/CNAME/' . $rr->owner . '/' . $rr->cname;


tgreenx · 2024-06-10T15:14:12Z

lib/Zonemaster/Engine/Recursor.pm

+        }
+
+        # CNAME owner name is target, or target has already been seen in this response, or owner name cannot be a target
+        if ( lc( $rr_owner ) eq lc( $rr_target ) or exists $seen_targets{lc( $rr_target )} or grep( /^$rr_target$/, keys %forbidden_targets ) ) {


I think you meant grep { $_ eq lc( $rr_target ) } ( keys %forbidden_targets ) instead.
Fixed.

matsduf · 2024-07-04T15:04:10Z

I think this can be merged now.

tgreenx added T-Feature Type: New feature in software or test case description V-Minor Versioning: The change gives an update of minor in version. labels Sep 7, 2023

tgreenx added this to the v2023.2 milestone Sep 7, 2023

tgreenx requested review from mattias-p, matsduf, hannaeko, a user and marc-vanderwal September 7, 2023 16:58

tgreenx mentioned this pull request Sep 7, 2023

Refactor ASNLookup code and documentation #1257

Merged

tgreenx force-pushed the cname-in-recursor branch 3 times, most recently from 2450985 to 5018a87 Compare October 24, 2023 16:35

tgreenx marked this pull request as ready for review October 24, 2023 16:41

tgreenx force-pushed the cname-in-recursor branch 4 times, most recently from 68c5e67 to 99e8759 Compare October 31, 2023 10:28

tgreenx force-pushed the cname-in-recursor branch from 99e8759 to 6a32b4c Compare October 31, 2023 15:41

matsduf reviewed Oct 31, 2023

View reviewed changes

tgreenx force-pushed the cname-in-recursor branch from 6a32b4c to 2cab9b6 Compare October 31, 2023 16:28

tgreenx requested a review from matsduf November 2, 2023 16:07

matsduf reviewed Nov 3, 2023

View reviewed changes

tgreenx requested a review from matsduf December 7, 2023 18:54

matsduf requested changes Dec 11, 2023

View reviewed changes

tgreenx added 5 commits May 14, 2024 17:31

Update after review - updates related to CNAME resolution

2cff836

- Rename CNAME_FOLLOWED_IB to CNAME_FOLLOWED_IN_ZONE and CNAME_FOLLOWED_OOB to CNAME_FOLLOWED_OUT_OF_ZONE - Update documentation

tgreenx force-pushed the cname-in-recursor branch from 5756f49 to 2cff836 Compare May 14, 2024 15:59

tgreenx requested a review from matsduf May 14, 2024 16:00

tgreenx added 3 commits May 14, 2024 18:09

Add missing method import

5147f54

Update names of message tags in the profile

e67a074

Update names of message tags in unit tests

a587333

marc-vanderwal reviewed May 21, 2024

View reviewed changes

Updates after review comments

38efad5

- Omit TTL and names case in resource record duplicate comparison - Fix condition

tgreenx commented Jun 10, 2024

View reviewed changes

tgreenx requested a review from marc-vanderwal June 10, 2024 15:20

marc-vanderwal approved these changes Jun 12, 2024

View reviewed changes

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

matsduf approved these changes Jul 4, 2024

View reviewed changes

tgreenx merged commit 154433f into zonemaster:develop Jul 16, 2024
3 checks passed

tgreenx mentioned this pull request Jul 25, 2024

Add DNAME support (to Zone objects) #1213

Open

2 tasks

tgreenx mentioned this pull request Sep 9, 2024

Clarify MethodsV2? zonemaster/zonemaster#1290

Merged

Add CNAME followage in Recursor #1288

Add CNAME followage in Recursor #1288

Conversation

tgreenx commented Sep 7, 2023 • edited Loading

Purpose

Context

Changes

How to test this PR

matsduf commented Oct 31, 2023

tgreenx commented Oct 31, 2023

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 Oct 31, 2023

tgreenx commented Nov 2, 2023

matsduf commented Nov 2, 2023

tgreenx commented Nov 2, 2023

matsduf commented Nov 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgreenx Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgreenx Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Nov 5, 2023

tgreenx commented Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Apr 10, 2024

tgreenx commented Apr 10, 2024

tgreenx commented May 14, 2024

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

matsduf commented May 22, 2024

marc-vanderwal commented May 24, 2024

tgreenx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Jul 4, 2024

tgreenx commented Sep 7, 2023 •

edited

Loading

tgreenx Nov 6, 2023 •

edited

Loading

tgreenx Nov 6, 2023 •

edited

Loading

tgreenx commented Dec 7, 2023 •

edited

Loading