zonemaster-engine
zonemaster-engine copied to clipboard
zonemaster
Add CNAME followage in Recursor
Purpose
This PR makes the recursive lookup functionality of Zonemaster to be able to follow CNAME redirections.
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)
Context
Necessary for https://github.com/zonemaster/zonemaster-engine/pull/1257 (https://github.com/zonemaster/zonemaster-engine/pull/1257#issuecomment-1710444437)
Test Zones specification: https://github.com/zonemaster/zonemaster/pull/1220
Changes
- Update recursive lookup code (
lib/Zonemaster/Engine/Recursor.pm) - Add system debug message tags
CNAME_LOOP_INNER,CNAME_LOOP_OUTERandCNAME_MULTIPLE_FOR_NAME - Update test cases related to CNAME (where applicable): Syntax06 and Zone07
- Create and update unitary tests
How to test this PR
Unit tests are updated and should pass (based on https://github.com/zonemaster/zonemaster/pull/1220).
Manual testing:
You can can test any zone by providing it to Zonemaster::Engine->recurse(), e.g. with any of the zones from https://github.com/zonemaster/zonemaster/pull/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"
- 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?
- 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.
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.
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.
What is the purpose of mixing case in
illegal1 IN CNAME target0
ILLEGAL1 IN CNAME target1
and others?
What is the purpose of mixing case in
illegal1 IN CNAME target0 ILLEGAL1 IN CNAME target1and 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).
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?
@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, How should the unit tests react on NODATA and NXDOMAIN responses, respectively?
NODATA: [...] NXDOMAIN: [...]
CNAME evaluation will not be done in NODATA and NXDOMAIN responses. So no new system message tag from this PR should be seen in those cases.
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
If there's a duplicate CNAME record, then CNAME_MULTIPLE_FOR_NAME will be outputted, CNAME followage stops and no packet is returned.
But in case of a broken chain without duplicate CNAME record, if the target is followed but does not exist then this will lead to a NXDOMAIN response, and CNAME followage will stop there. Right now the code does not evaluate in-response A/AAAA records for a CNAME target. So in that sense maybe "broken chain", as you present it, is not supported.
@matsduf This PR has been updated to include the change discussed in the work group, and rebased on latest develop. But currently two unit tests are failing due to what appears to be an issue with CoreDNS. See https://github.com/zonemaster/zonemaster/pull/1220#discussion_r1407581156.
(I removed an erroneous comment)
With updated test data only one unit test fails (my update of the t file to print out the scenario name):
ok 56 - undefined as expected (LOOPED-CNAME-IN-ZONE-1)
not ok 57 - should emit CNAME_LOOP_INNER (LOOPED-CNAME-IN-ZONE-1)
# Failed test 'should emit CNAME_LOOP_INNER (LOOPED-CNAME-IN-ZONE-1)'
# at t/recursor-A.t line 121.
ok 58 - undefined as expected (LOOPED-CNAME-IN-ZONE-2)
ok 59 - should emit CNAME_LOOP_INNER (LOOPED-CNAME-IN-ZONE-2)
@matsduf @mattias-p I have addressed or responded to your comments. See commit https://github.com/zonemaster/zonemaster-engine/pull/1288/commits/132402960fcf0dc11e385d4f3ff4723e55349c47 - please re-review.
How will the code react if the answer section has a CNAME pointing at a target name, but the A record has another owner name?
;; ANSWER SECTION:
cname-one.cname.recursor.engine.xa. 3600 IN CNAME cname-two.cname.recursor.engine.xa.
cname-three.cname.recursor.engine.xa. 3600 IN A 127.0.0.1
How will the code react if the answer section has a broken CNAME chain, i.e. a CNAME is missing in the chain?
;; ANSWER SECTION:
cname-one.cname.recursor.engine.xa. 3600 IN CNAME cname-two.cname.recursor.engine.xa.
cname-three.cname.recursor.engine.xa. 3600 IN CNAME cname-four.cname.recursor.engine.xa.
cname-four.cname.recursor.engine.xa. 3600 IN A 127.0.0.1
I could easily try to create these and other scenarios.
How will the code react if the answer section has a CNAME pointing at a target name, but the
Arecord has another owner name?;; ANSWER SECTION: cname-one.cname.recursor.engine.xa. 3600 IN CNAME cname-two.cname.recursor.engine.xa. cname-three.cname.recursor.engine.xa. 3600 IN A 127.0.0.1
Assuming it was a QTYPE A query with QNAME cname-one.cname.recursor.engine.xa, currently it will make a new recursive lookup A query to cname-two.cname.recursor.engine.xa.
How will the code react if the answer section has a broken CNAME chain, i.e. a CNAME is missing in the chain?
;; ANSWER SECTION: cname-one.cname.recursor.engine.xa. 3600 IN CNAME cname-two.cname.recursor.engine.xa. cname-three.cname.recursor.engine.xa. 3600 IN CNAME cname-four.cname.recursor.engine.xa. cname-four.cname.recursor.engine.xa. 3600 IN A 127.0.0.1
Assuming it was a QTYPE A query with QNAME cname-one.cname.recursor.engine.xa, it will consider the final target to be cname-two.cname.recursor.engine.xa, and, considering there are no RR of QTYPE A for name cname-two.cname.recursor.engine.xa in that response, it will make a new recursive lookup A query to cname-two.cname.recursor.engine.xa.
I could easily try to create these and other scenarios.
Yes please do.
How will the code react if the answer section has a CNAME pointing at a target name, but the
Arecord has another owner name?;; ANSWER SECTION: cname-one.cname.recursor.engine.xa. 3600 IN CNAME cname-two.cname.recursor.engine.xa. cname-three.cname.recursor.engine.xa. 3600 IN A 127.0.0.1Assuming it was a QTYPE
Aquery with QNAMEcname-one.cname.recursor.engine.xa, currently it will make a new recursive lookupAquery tocname-two.cname.recursor.engine.xa.
I would say that it is a broken response, and Zonemaster should maybe break.
How will the code react if the answer section has a broken CNAME chain, i.e. a CNAME is missing in the chain?
;; ANSWER SECTION: cname-one.cname.recursor.engine.xa. 3600 IN CNAME cname-two.cname.recursor.engine.xa. cname-three.cname.recursor.engine.xa. 3600 IN CNAME cname-four.cname.recursor.engine.xa. cname-four.cname.recursor.engine.xa. 3600 IN A 127.0.0.1Assuming it was a QTYPE
Aquery with QNAMEcname-one.cname.recursor.engine.xa, it will consider the final target to becname-two.cname.recursor.engine.xa, and, considering there are no RR of QTYPEAfor namecname-two.cname.recursor.engine.xain that response, it will make a new recursive lookupAquery tocname-two.cname.recursor.engine.xa.
I would say that it is a broken response, and Zonemaster should maybe break.
@mattias-p wrote in https://github.com/zonemaster/zonemaster-engine/pull/1288#discussion_r1409339655:
I believe we need to protect ourselves from an infinite loop here. E.g. when getting a response like this:
;; QUESTION SECTION: ;a.example. IN A ;; ANSWER SECTION: a.example. IN CNAME b.example. b.example. IN CNAME a.example.
Due to a bug (I think) in CoreDNS this cannot be produced by CoreDNS unless the following is acceptable, but I do not think the code likes it.
;; QUESTION SECTION:
;a.example. IN A
;; ANSWER SECTION:
a.example. IN CNAME b.example.
b.example. IN CNAME a.example.
a.example. IN CNAME b.example.
b.example. IN CNAME a.example.
a.example. IN CNAME b.example.
b.example. IN CNAME a.example.
a.example. IN CNAME b.example.
b.example. IN CNAME a.example.
a.example. IN CNAME b.example.
b.example. IN CNAME a.example.
I could easily try to create these and other scenarios.
Yes please do.
I have added three more scenarios. I guess you will updated the unit tests.
Three new scenarios, LOOPED-CNAME-IN-ZONE-3, WRONG-CNAME-OWNER-NAME and EXTRA-CNAME-IN-ANSWER, have been added to the test zone (https://github.com/zonemaster/zonemaster/pull/1220).
Those are are in addition to TOO-LONG-CNAME-CHAIN, TARGET-NO-MATCH-CNAME and BROKEN-CNAME-CHAIN added yesterday.
@matsduf @mattias-p Comments are addressed, please re-review. See commit https://github.com/zonemaster/zonemaster-engine/pull/1288/commits/32244e921c92c095e1d814311feb011562dfdb2e.
@matsduf All comments have been addressed in https://github.com/zonemaster/zonemaster-engine/pull/1288/commits/5756f4951d9e452de8fc1bfe1e45fb8541750d50. Also note that with the removal of duplicate records all unit tests now work as expected too.
@tgreenx, could we complete this together with Test zones for the CNAME function in Recursor.pm in Engine?
@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.
@matsduf please re-review. I addressed your latest comment, and rebased on latest develop.
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.
Is case preservation really needed here? I cannot see that is wrong to query for
bar.xaeven it RDATA of the CNAME record wasBAR.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.
