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).

diff --git a/bin/tests/system/sfcache_cname/ans1/ans.py b/bin/tests/system/sfcache_cname/ans1/ans.py
deleted file mode 100644 (file)
index 049a2d2..0000000
--- 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 (file)
index 0000000..9f2271e
--- /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 (file)
index 0000000..d235783
--- /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 c501ad6416fa042a5d237ed1fde111a93ff523e0..8bc955aceb5931c7ca59019797bb841800456a20 100644 (file)
--- 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 df838ddebd52a874684179ebe90dfe1fe9722f55..2c5fbe8363ff0e49f7aa802820bb9bde5958606a 100644 (file)
--- 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 45fdab1c5891066c5f32fcd66a0036a2ec557841..7c38b2a4b0e494c2c6c287fd6143695223c18d34 100644 (file)
--- 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));
                }