From d238b8eece7eeca182e7d9321fc2679b6b98d2ca Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Mon, 1 Jun 2026 17:38:18 +0200 Subject: [PATCH 1/2] System test covering SERVFAIL cache and CNAME Add a system test for the case where resolution SERVFAILs because the fetch context reaches the `max-query-count` threshold while following a CNAME. Resolving the CNAME target independently should still work, because the SERVFAIL cache stores the original query name rather than the target. --- bin/tests/system/sfcache_cname/ans1/ans.py | 53 +++++++++++++++++++ .../system/sfcache_cname/ns2/named.conf.j2 | 26 +++++++++ .../sfcache_cname/tests_sfcache_cname.py | 34 ++++++++++++ 3 files changed, 113 insertions(+) create mode 100644 bin/tests/system/sfcache_cname/ans1/ans.py create mode 100644 bin/tests/system/sfcache_cname/ns2/named.conf.j2 create mode 100644 bin/tests/system/sfcache_cname/tests_sfcache_cname.py diff --git a/bin/tests/system/sfcache_cname/ans1/ans.py b/bin/tests/system/sfcache_cname/ans1/ans.py new file mode 100644 index 0000000000..049a2d2009 --- /dev/null +++ b/bin/tests/system/sfcache_cname/ans1/ans.py @@ -0,0 +1,53 @@ +# Copyright (C) Internet Systems Consortium, Inc. ("ISC") +# +# SPDX-License-Identifier: MPL-2.0 +# +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, you can obtain one at https://mozilla.org/MPL/2.0/. +# +# See the COPYRIGHT file distributed with this work for additional +# information regarding copyright ownership. + + +import dns.name +import dns.rcode +import dns.rdataclass +import dns.rdatatype +import dns.rrset + +from isctest.asyncserver import AsyncDnsServer, QnameQtypeHandler, StaticResponseHandler + + +def rrset( + qname: dns.name.Name | str, + rtype: dns.rdatatype.RdataType, + rdata: str, + ttl: int = 300, +) -> dns.rrset.RRset: + return dns.rrset.from_text(qname, ttl, dns.rdataclass.IN, rtype, rdata) + + +class Tld1Handler(QnameQtypeHandler, StaticResponseHandler): + qnames = ["foo.tld1."] + qtypes = [dns.rdatatype.A] + answer = [rrset("foo.tld1.", dns.rdatatype.CNAME, "tld2.")] + + +class Tld2Handler(QnameQtypeHandler, StaticResponseHandler): + qnames = ["tld2."] + qtypes = [dns.rdatatype.A] + answer = [rrset("tld2.", dns.rdatatype.A, "1.2.3.4")] + + +def main() -> None: + server = AsyncDnsServer(default_aa=True, default_rcode=dns.rcode.NOERROR) + server.install_response_handlers( + Tld1Handler(), + Tld2Handler(), + ) + server.run() + + +if __name__ == "__main__": + main() diff --git a/bin/tests/system/sfcache_cname/ns2/named.conf.j2 b/bin/tests/system/sfcache_cname/ns2/named.conf.j2 new file mode 100644 index 0000000000..c501ad6416 --- /dev/null +++ b/bin/tests/system/sfcache_cname/ns2/named.conf.j2 @@ -0,0 +1,26 @@ +options { + query-source address @ns.ip@; + port @PORT@; + pid-file "named.pid"; + listen-on { @ns.ip@; }; + listen-on-v6 { none; }; + recursion yes; + dnssec-validation no; + qname-minimization off; + servfail-ttl 5; + max-query-count 2; +}; + +{% include "_common/controls.conf.j2" %} + +zone "tld1." { + type forward; + forward only; + forwarders { 10.53.0.1; }; +}; + +zone "tld2." { + type forward; + forward only; + forwarders { 10.53.0.1; }; +}; diff --git a/bin/tests/system/sfcache_cname/tests_sfcache_cname.py b/bin/tests/system/sfcache_cname/tests_sfcache_cname.py new file mode 100644 index 0000000000..df838ddebd --- /dev/null +++ b/bin/tests/system/sfcache_cname/tests_sfcache_cname.py @@ -0,0 +1,34 @@ +# Copyright (C) Internet Systems Consortium, Inc. ("ISC") +# +# SPDX-License-Identifier: MPL-2.0 +# +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, you can obtain one at https://mozilla.org/MPL/2.0/. +# +# See the COPYRIGHT file distributed with this work for additional +# information regarding copyright ownership. + + +import dns.message + +import isctest + + +# A SERVFAIL produced while following a CNAME must be cached against the +# original query name, not the CNAME target. +# +# ans1 serves "foo.tld1 CNAME tld2" and "tld2 A 1.2.3.4"; ns2 forwards +# both zones to it with "max-query-count 2". Resolving "foo.tld1/A" +# follows the CNAME to "tld2" and then exhausts the query budget, so the +# client gets SERVFAIL. That failure must be cached under the original +# name ("foo.tld1"), so a subsequent direct query for the CNAME target +# ("tld2") is not blocked by the SERVFAIL cache and resolves normally. +def test_sfcache_cname(ns2): + msg = dns.message.make_query("foo.tld1.", "A") + res = isctest.query.udp(msg, ns2.ip) + isctest.check.servfail(res) + + msg = dns.message.make_query("tld2.", "A") + res = isctest.query.udp(msg, ns2.ip) + isctest.check.noerror(res) From 66af5b464deb61da1cbb334d37bfe91918ae6944 Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Mon, 1 Jun 2026 16:58:45 +0200 Subject: [PATCH 2/2] Use original query name when caching SERVFAIL Instead of using `client->query.qname` when caching a SERVFAIL answer, use `client->query.origqname` when available. This avoids caching a SERVFAIL against a CNAME target when the failure occurs while the resolver is following the CNAME chain. This is problematic, for instance, when the SERVFAIL is triggered by the `max-query-count` threshold being reached, which would incorrectly prevent legitimate resolution of the CNAME target while in the SERVFAIL cache. Note that if the SERVFAIL genuinely originated from resolving the CNAME target, that specific failure will no longer be cached, and a direct query for the CNAME target will trigger a fresh (likely failing) resolution attempt. However, this is still preferable to the previous behaviour, which would wrongly prevent resolving the CNAME target if it was cached for other reasons (like the example above). --- bin/tests/system/sfcache_cname/ans1/ans.py | 53 ------------------- .../system/sfcache_cname/ns1/named.conf.j2 | 16 ++++++ bin/tests/system/sfcache_cname/ns1/root.db | 6 +++ .../system/sfcache_cname/ns2/named.conf.j2 | 12 ++--- .../sfcache_cname/tests_sfcache_cname.py | 2 +- lib/ns/client.c | 6 ++- 6 files changed, 31 insertions(+), 64 deletions(-) delete mode 100644 bin/tests/system/sfcache_cname/ans1/ans.py create mode 100644 bin/tests/system/sfcache_cname/ns1/named.conf.j2 create mode 100644 bin/tests/system/sfcache_cname/ns1/root.db diff --git a/bin/tests/system/sfcache_cname/ans1/ans.py b/bin/tests/system/sfcache_cname/ans1/ans.py deleted file mode 100644 index 049a2d2009..0000000000 --- a/bin/tests/system/sfcache_cname/ans1/ans.py +++ /dev/null @@ -1,53 +0,0 @@ -# Copyright (C) Internet Systems Consortium, Inc. ("ISC") -# -# SPDX-License-Identifier: MPL-2.0 -# -# This Source Code Form is subject to the terms of the Mozilla Public -# License, v. 2.0. If a copy of the MPL was not distributed with this -# file, you can obtain one at https://mozilla.org/MPL/2.0/. -# -# See the COPYRIGHT file distributed with this work for additional -# information regarding copyright ownership. - - -import dns.name -import dns.rcode -import dns.rdataclass -import dns.rdatatype -import dns.rrset - -from isctest.asyncserver import AsyncDnsServer, QnameQtypeHandler, StaticResponseHandler - - -def rrset( - qname: dns.name.Name | str, - rtype: dns.rdatatype.RdataType, - rdata: str, - ttl: int = 300, -) -> dns.rrset.RRset: - return dns.rrset.from_text(qname, ttl, dns.rdataclass.IN, rtype, rdata) - - -class Tld1Handler(QnameQtypeHandler, StaticResponseHandler): - qnames = ["foo.tld1."] - qtypes = [dns.rdatatype.A] - answer = [rrset("foo.tld1.", dns.rdatatype.CNAME, "tld2.")] - - -class Tld2Handler(QnameQtypeHandler, StaticResponseHandler): - qnames = ["tld2."] - qtypes = [dns.rdatatype.A] - answer = [rrset("tld2.", dns.rdatatype.A, "1.2.3.4")] - - -def main() -> None: - server = AsyncDnsServer(default_aa=True, default_rcode=dns.rcode.NOERROR) - server.install_response_handlers( - Tld1Handler(), - Tld2Handler(), - ) - server.run() - - -if __name__ == "__main__": - main() diff --git a/bin/tests/system/sfcache_cname/ns1/named.conf.j2 b/bin/tests/system/sfcache_cname/ns1/named.conf.j2 new file mode 100644 index 0000000000..9f2271e709 --- /dev/null +++ b/bin/tests/system/sfcache_cname/ns1/named.conf.j2 @@ -0,0 +1,16 @@ +options { + query-source address @ns.ip@; + notify-source @ns.ip@; + transfer-source @ns.ip@; + port @PORT@; + pid-file "named.pid"; + listen-on { @ns.ip@; }; + listen-on-v6 { none; }; + recursion no; + dnssec-validation no; +}; + +zone "." { + type primary; + file "root.db"; +}; diff --git a/bin/tests/system/sfcache_cname/ns1/root.db b/bin/tests/system/sfcache_cname/ns1/root.db new file mode 100644 index 0000000000..d2357839be --- /dev/null +++ b/bin/tests/system/sfcache_cname/ns1/root.db @@ -0,0 +1,6 @@ +$TTL 300 +. IN SOA ns. hostmaster. 1 600 600 1200 600 +. NS ns +ns A 10.53.0.1 +foo.tld1. CNAME tld2. +tld2. A 1.2.3.4 diff --git a/bin/tests/system/sfcache_cname/ns2/named.conf.j2 b/bin/tests/system/sfcache_cname/ns2/named.conf.j2 index c501ad6416..8bc955aceb 100644 --- a/bin/tests/system/sfcache_cname/ns2/named.conf.j2 +++ b/bin/tests/system/sfcache_cname/ns2/named.conf.j2 @@ -11,16 +11,12 @@ options { max-query-count 2; }; -{% include "_common/controls.conf.j2" %} - zone "tld1." { - type forward; - forward only; - forwarders { 10.53.0.1; }; + type static-stub; + server-addresses { 10.53.0.1; }; }; zone "tld2." { - type forward; - forward only; - forwarders { 10.53.0.1; }; + type static-stub; + server-addresses { 10.53.0.1; }; }; diff --git a/bin/tests/system/sfcache_cname/tests_sfcache_cname.py b/bin/tests/system/sfcache_cname/tests_sfcache_cname.py index df838ddebd..2c5fbe8363 100644 --- a/bin/tests/system/sfcache_cname/tests_sfcache_cname.py +++ b/bin/tests/system/sfcache_cname/tests_sfcache_cname.py @@ -18,7 +18,7 @@ import isctest # A SERVFAIL produced while following a CNAME must be cached against the # original query name, not the CNAME target. # -# ans1 serves "foo.tld1 CNAME tld2" and "tld2 A 1.2.3.4"; ns2 forwards +# ns1 serves "foo.tld1 CNAME tld2" and "tld2 A 1.2.3.4"; ns2 forwards # both zones to it with "max-query-count 2". Resolving "foo.tld1/A" # follows the CNAME to "tld2" and then exhausts the query budget, so the # client gets SERVFAIL. That failure must be cached under the original diff --git a/lib/ns/client.c b/lib/ns/client.c index 45fdab1c58..7c38b2a4b0 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -1030,6 +1030,9 @@ ns_client_error(ns_client_t *client, isc_result_t result) { isc_time_t expire; isc_interval_t i; uint32_t flags = 0; + dns_name_t *qname = client->query.origqname != NULL + ? client->query.origqname + : client->query.qname; if ((message->flags & DNS_MESSAGEFLAG_CD) != 0) { flags = NS_FAILCACHE_CD; @@ -1038,8 +1041,7 @@ ns_client_error(ns_client_t *client, isc_result_t result) { isc_interval_set(&i, client->inner.view->fail_ttl, 0); result = isc_time_nowplusinterval(&expire, &i); if (result == ISC_R_SUCCESS) { - dns_badcache_add(client->inner.view->failcache, - client->query.qname, + dns_badcache_add(client->inner.view->failcache, qname, client->query.qtype, flags, isc_time_seconds(&expire)); }