From e073d1c05b2a8c71349a783905be0596648e4dec Mon Sep 17 00:00:00 2001 From: Nitin Nizhawan Date: Thu, 30 Jul 2026 09:09:49 +0530 Subject: [PATCH] plugin/cache: do not cache SOA-less NODATA responses (#8232) * plugin/cache: do not cache SOA-less NODATA responses An upstream may return NOERROR with a non-empty answer that still does not resolve the question and without an SOA record to bound a negative TTL: a CNAME chain that does not terminate in a record of the queried type at the chain's terminal name (an incomplete recursion result from a forwarder). Because the answer section is non-empty, response.Typify classifies it as NoError (positive), so the cache plugin stores it keyed on and replays the non-answer to clients until the TTL expires. Per RFC 2308 section 5, negative responses without an SOA record SHOULD NOT be cached. Following RFC 1034 section 3.6.2 and RFC 2308 sections 1 and 2.2, the effective owner name is the target at the end of the CNAME chain, and the response is NODATA unless it carries the queried type at that terminal name; this holds for every query type, not just A/AAAA. Skip caching such a response (mirroring the existing NameError && !hasSOA guard) and let the next query be resolved upstream again. An empty answer section is deliberately left cacheable: it is indistinguishable from a legitimate NOERROR positive response that carries its data outside the answer section (for example the whoami plugin). Refs coredns#6958, coredns#5077, coredns#4987. Signed-off-by: Nitin Nizhawan * plugin/cache: fail closed on malformed CNAME chains in isNODATA canonicalName now returns a validity flag and rejects chains that are not a single unambiguous path to a terminal name: an owner with more than one distinct CNAME target (RFC 2181 section 10.1) and a revisited owner / CNAME loop (RFC 1034 section 3.6.2). isNODATA treats an invalid chain as a non-answer, so a SOA-less response with such a chain is not cached. This makes the classification order-independent (previously the first CNAME per owner won, so a two-target owner was cacheable or not depending on wire order) and closes the loop-with-co-located-record case. Duplicate CNAME records naming the same target are still tolerated. Adds regression tests for the two-distinct-targets case in both orders, the CNAME loop with a co-located A, and the tolerated duplicate-identical-CNAME case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d59b4564-a9df-425f-858e-aadee0f35581 Signed-off-by: Nitin Nizhawan * plugin/cache: split canonicalName into self-documenting helpers Extract the per-owner CNAME lookup into uniqueCNAMETarget and loop detection into a small case-insensitive nameSet type, leaving canonicalName as a short driver. Signature and algorithm are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d59b4564-a9df-425f-858e-aadee0f35581 Signed-off-by: Nitin Nizhawan --------- Signed-off-by: Nitin Nizhawan Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d59b4564-a9df-425f-858e-aadee0f35581 --- plugin/cache/cache.go | 130 ++++++++++++++++++++ plugin/cache/cache_test.go | 241 ++++++++++++++++++++++++++++++++++++- 2 files changed, 369 insertions(+), 2 deletions(-) diff --git a/plugin/cache/cache.go b/plugin/cache/cache.go index 973d78e47..76a73ff40 100644 --- a/plugin/cache/cache.go +++ b/plugin/cache/cache.go @@ -5,6 +5,7 @@ import ( "encoding/binary" "hash/fnv" "net" + "strings" "time" "github.com/coredns/coredns/plugin" @@ -94,6 +95,20 @@ func key(qname string, m *dns.Msg, t response.Type, do, cd bool) (bool, uint64) if t == response.NameError && !hasSOA(m) { return false, 0 } + // An upstream may return NOERROR with a non-empty answer that still does not + // resolve the question and without an SOA to bound a negative TTL: a CNAME + // chain that does not terminate in the queried type (an incomplete recursion + // result from a forwarder). This is effectively an SOA-less NODATA response, + // which per RFC 2308 section 5 SHOULD NOT be cached. response.Typify classifies + // it as NoError because the answer section is non-empty, so caching it in the + // positive cache would replay the non-answer to clients until it expires. Skip + // caching so the next query is resolved upstream again. An empty answer section + // is deliberately left cacheable: it is indistinguishable from a legitimate + // NOERROR positive response that carries its data outside the answer section + // (for example the whoami plugin, which answers in the additional section). + if t == response.NoError && !hasSOA(m) && isNODATA(m) { + return false, 0 + } return true, hash(qname, m.Question[0].Qtype, m.Question[0].Qclass, do, cd) } @@ -107,6 +122,121 @@ func hasSOA(m *dns.Msg) bool { return false } +// isNODATA reports whether a NOERROR response with a non-empty answer section +// does not answer the question. Following RFC 1034 section 3.6.2 and RFC 2308 +// sections 1 and 2.2, a query of any type other than CNAME (and ANY) is +// restarted along the CNAME chain, so the effective owner name is the target at +// the end of the CNAME chain that starts at the question name. The response +// answers the question only if it carries a record of the queried type at that +// terminal name (records at any other owner name are irrelevant, and per RFC +// 1034 a CNAME's owner never co-locates other data). This rule is independent of +// the queried type: an MX, TXT, SRV, etc. chain that does not reach the queried +// type is NODATA just like an A or AAAA one. When the chain is malformed (an +// owner with more than one distinct CNAME target, or a loop) it has no +// well-defined terminal name, so the response is treated as NODATA, which errs +// toward re-querying upstream rather than caching a non-answer. An empty answer +// section returns false so that legitimate positive responses carrying data +// outside the answer section (for example the whoami plugin) remain cacheable. +// ANY queries are excluded because any record answers them. Note: a bare DNAME +// (RFC 6672) without its synthesized CNAME is treated as NODATA; standard +// responses include the synthesized CNAME, which the chain walk follows. +func isNODATA(m *dns.Msg) bool { + if len(m.Answer) == 0 { + return false + } + qtype := m.Question[0].Qtype + if qtype == dns.TypeANY { + return false + } + // A CNAME query is answered by the CNAME itself, so the chain is not + // followed; otherwise resolve it to the terminal owner name. + name := m.Question[0].Name + if qtype != dns.TypeCNAME { + terminal, ok := canonicalName(m.Answer, name) + if !ok { + // The CNAME chain is malformed (an owner with more than one + // distinct target, or a loop) and therefore has no well-defined + // QNAME per RFC 2181 section 10.1 and RFC 1034 section 3.6.2. Such + // a response cannot be shown to answer the question, so treat it as + // NODATA and (being SOA-less) leave it uncacheable. + return true + } + name = terminal + } + for _, r := range m.Answer { + h := r.Header() + if h.Rrtype == qtype && strings.EqualFold(h.Name, name) { + return false + } + } + return true +} + +// canonicalName follows the owner-linked CNAME chain in answer starting at name +// and returns the terminal target name together with a validity flag. Records +// whose owner is not on the chain are ignored. The chain is invalid (ok=false) +// when it is not a single unambiguous path to a terminal name: an owner that has +// more than one distinct CNAME target violates RFC 2181 section 10.1 (an alias +// has exactly one canonical name), and a revisited owner is a CNAME loop, which +// RFC 1034 section 3.6.2 says must be signalled as an error. Reporting validity +// rather than silently stopping keeps the classification order-independent and +// fail-closed: callers treat a malformed chain as a non-answer. Duplicate CNAME +// records that name the same target are tolerated, since they still describe a +// single canonical name. +func canonicalName(answer []dns.RR, name string) (string, bool) { + visited := nameSet{} + for { + if visited.contains(name) { + // Revisited owner: the chain contains a CNAME loop. + return name, false + } + visited.add(name) + + target, ok := uniqueCNAMETarget(answer, name) + if !ok { + // Owner has more than one distinct canonical name. + return name, false + } + if target == "" { + // Terminal owner reached: no CNAME continues the chain. + return name, true + } + name = target + } +} + +// uniqueCNAMETarget returns the canonical name that owner is aliased to by a +// CNAME record in answer. ok is false when owner carries more than one distinct +// CNAME target, which violates RFC 2181 section 10.1. When owner has no CNAME the +// returned target is empty and ok is true, marking a terminal owner. Duplicate +// CNAME records naming the same target are tolerated. +func uniqueCNAMETarget(answer []dns.RR, owner string) (target string, ok bool) { + for _, r := range answer { + c, isCNAME := r.(*dns.CNAME) + if !isCNAME || !strings.EqualFold(c.Header().Name, owner) { + continue + } + if target != "" && !strings.EqualFold(target, c.Target) { + return "", false + } + target = c.Target + } + return target, true +} + +// nameSet is a set of domain names compared case-insensitively, used to detect +// revisited owners (loops) while walking a CNAME chain. +type nameSet map[string]struct{} + +func (s nameSet) contains(name string) bool { + _, ok := s[strings.ToLower(name)] + return ok +} + +func (s nameSet) add(name string) { + s[strings.ToLower(name)] = struct{}{} +} + var one = []byte("1") var zero = []byte("0") diff --git a/plugin/cache/cache_test.go b/plugin/cache/cache_test.go index 1f60a605c..848eaf7f8 100644 --- a/plugin/cache/cache_test.go +++ b/plugin/cache/cache_test.go @@ -282,6 +282,243 @@ func TestCacheInsertion(t *testing.T) { }, shouldCache: true, }, + { + name: "test NOERROR dangling CNAME chain without SOA does not cache", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeA, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target1.example.net."), + test.CNAME("target1.example.net. 3600 IN CNAME target2.example.net."), + }, + RecursionAvailable: true, + }, + shouldCache: false, + }, + { + name: "test NOERROR CNAME chain ending in a different type without SOA does not cache", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeA, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + test.AAAA("target.example.net. 3600 IN AAAA ::1"), + }, + RecursionAvailable: true, + }, + shouldCache: false, + }, + { + name: "test NOERROR MX query CNAME chain without terminal MX without SOA does not cache", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeMX, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + }, + RecursionAvailable: true, + }, + shouldCache: false, + }, + { + name: "test NOERROR MX query CNAME chain terminating in MX caches", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeMX, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + test.MX("target.example.net. 3600 IN MX 10 mail.example.net."), + }, + RecursionAvailable: true, + }, + out: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeMX, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + test.MX("target.example.net. 3600 IN MX 10 mail.example.net."), + }, + RecursionAvailable: true, + }, + shouldCache: true, + }, + { + name: "test NOERROR DNSSEC dangling CNAME chain without SOA does not cache", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Do: true, + Qname: "alias.example.org.", Qtype: dns.TypeA, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + test.RRSIG("alias.example.org. 3600 IN RRSIG CNAME 8 2 3600 20170521031301 20170421031301 12051 example.org. lAaEzB5teQLLKyDenatmyhca7blLRg9DoGNrhe3NReBZN5C5/pMQk8Jc u25hv2fW23/SLm5IC2zaDpp2Fzgm6Jf7e90/yLcwQPuE7JjS55WMF+HE LEh7Z6AEb+Iq4BWmNhUz6gPxD4d9eRMs7EAzk13o1NYi5/JhfL6IlaYy qkc="), + }, + RecursionAvailable: true, + }, + shouldCache: false, + }, + { + name: "test NOERROR CNAME chain with unrelated A off the chain without SOA does not cache", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeA, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + test.A("unrelated.example.net. 3600 IN A 192.0.2.1"), + }, + RecursionAvailable: true, + }, + shouldCache: false, + }, + { + name: "test NOERROR lone CNAME answer to an ANY query caches", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeANY, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + }, + RecursionAvailable: true, + }, + out: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeANY, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + }, + RecursionAvailable: true, + }, + shouldCache: true, + }, + { + name: "test NOERROR CNAME chain terminating in A record caches", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeA, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + test.A("target.example.net. 3600 IN A 127.0.0.1"), + }, + RecursionAvailable: true, + }, + out: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeA, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + test.A("target.example.net. 3600 IN A 127.0.0.1"), + }, + RecursionAvailable: true, + }, + shouldCache: true, + }, + { + name: "test NOERROR CNAME chain terminating in A record out of order caches", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeA, + Answer: []dns.RR{ + test.A("target.example.net. 3600 IN A 127.0.0.1"), + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + }, + RecursionAvailable: true, + }, + out: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeA, + Answer: []dns.RR{ + test.A("target.example.net. 3600 IN A 127.0.0.1"), + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + }, + RecursionAvailable: true, + }, + shouldCache: true, + }, + { + name: "test NOERROR CNAME answer to a CNAME query caches", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeCNAME, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + }, + RecursionAvailable: true, + }, + out: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeCNAME, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + }, + RecursionAvailable: true, + }, + shouldCache: true, + }, + { + name: "test NOERROR CNAME chain with two distinct targets at one owner without SOA does not cache", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeA, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target1.example.net."), + test.CNAME("alias.example.org. 3600 IN CNAME target2.example.net."), + test.A("target1.example.net. 3600 IN A 192.0.2.1"), + }, + RecursionAvailable: true, + }, + shouldCache: false, + }, + { + name: "test NOERROR CNAME chain with two distinct targets at one owner reversed order without SOA does not cache", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeA, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target2.example.net."), + test.CNAME("alias.example.org. 3600 IN CNAME target1.example.net."), + test.A("target1.example.net. 3600 IN A 192.0.2.1"), + }, + RecursionAvailable: true, + }, + shouldCache: false, + }, + { + name: "test NOERROR CNAME loop with co-located A without SOA does not cache", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeA, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + test.CNAME("target.example.net. 3600 IN CNAME alias.example.org."), + test.A("alias.example.org. 3600 IN A 192.0.2.1"), + }, + RecursionAvailable: true, + }, + shouldCache: false, + }, + { + name: "test NOERROR CNAME chain with duplicate identical CNAME terminating in A caches", + in: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeA, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + test.A("target.example.net. 3600 IN A 127.0.0.1"), + }, + RecursionAvailable: true, + }, + out: test.Case{ + Rcode: dns.RcodeSuccess, + Qname: "alias.example.org.", Qtype: dns.TypeA, + Answer: []dns.RR{ + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + test.CNAME("alias.example.org. 3600 IN CNAME target.example.net."), + test.A("target.example.net. 3600 IN A 127.0.0.1"), + }, + RecursionAvailable: true, + }, + shouldCache: true, + }, } now, _ := time.Parse(time.UnixDate, "Fri Apr 21 10:51:21 BST 2017") utc := now.UTC() @@ -780,7 +1017,7 @@ func ttlBackend(ttl int) plugin.Handler { m.SetReply(r) m.Response, m.RecursionAvailable = true, true - m.Answer = []dns.RR{test.A(fmt.Sprintf("example.org. %d IN A 127.0.0.53", ttl))} + m.Answer = []dns.RR{test.A(fmt.Sprintf("%s %d IN A 127.0.0.53", r.Question[0].Name, ttl))} w.WriteMsg(m) return dns.RcodeSuccess, nil }) @@ -813,7 +1050,7 @@ func slowTTLBackend(ttl int, delay time.Duration, done chan<- struct{}) plugin.H m := new(dns.Msg) m.SetReply(r) m.Response, m.RecursionAvailable = true, true - m.Answer = []dns.RR{test.A(fmt.Sprintf("example.org. %d IN A 127.0.0.53", ttl))} + m.Answer = []dns.RR{test.A(fmt.Sprintf("%s %d IN A 127.0.0.53", r.Question[0].Name, ttl))} w.WriteMsg(m) if done != nil { close(done)