plugin/cache: stop setting AA on answers served from cache (#8419)

* plugin/cache: stop setting AA on answers served from cache

toMsg hardcoded m1.Authoritative = true, so a reply rebuilt from a cache entry claimed authority the answer that populated it never had.

The hardcoding was a workaround for legacy stub resolvers that dropped non-authoritative answers, but it only ever ran on the cache hit path: the same query still returned AA=0 on every miss and after every TTL expiry, so those clients were never actually protected.

Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>

* plugin/cache: pin the AA=1 side of the cache round trip

Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>

* plugin/cache: assert AA=0 on verified stale refresh and prove the cache hit

Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>

* plugin/cache: count backend calls in TestCachePreservesAA

Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>

---------

Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>

plugin/cache/cache_test.go

@@ -558,9 +558,8 @@ func TestCacheInsertion(t *testing.T) {
 
 				// TODO: If we incorporate these individual checks into the
 				//       test.Header function, we can eliminate them from here.
-				// Cache entries are always Authoritative.
+				if resp.Authoritative != tc.out.Authoritative {
-				if resp.Authoritative != true {
+					t.Errorf("Expected Authoritative Answer bit to be %t, but got %t", tc.out.Authoritative, resp.Authoritative)
-					t.Error("Expected Authoritative Answer bit to be true, but was false")
 				}
 				if resp.AuthenticatedData != tc.out.AuthenticatedData {
 					t.Errorf("Expected Authenticated Data bit to be %t, but got %t", tc.out.AuthenticatedData, resp.AuthenticatedData)
@@ -980,8 +979,8 @@ func TestServeFromStaleCacheFetchVerifyTimeoutFastUpstream(t *testing.T) {
 	if got := rec.Msg.Answer[0].Header().Ttl; got != 200 {
 		t.Errorf("expected fresh TTL=200, got %d", got)
 	}
-	if !rec.Msg.Authoritative {
+	if rec.Msg.Authoritative {
-		t.Error("expected cached fresh response to preserve authoritative cache reply shaping")
+		t.Error("expected AA=0: the freshly verified answer came from a non-authoritative backend")
 	}
 }
 
@@ -1116,6 +1115,22 @@ func BackendHandler() plugin.Handler {
 	})
 }
 
+func authoritativeBackend(calls *int) plugin.Handler {
+	return plugin.HandlerFunc(func(_ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+		*calls++
+		m := new(dns.Msg)
+		m.SetReply(r)
+		m.Response = true
+		m.Authoritative = true
+
+		owner := m.Question[0].Name
+		m.Answer = []dns.RR{test.A(owner + " 303 IN A 127.0.0.53")}
+
+		w.WriteMsg(m)
+		return dns.RcodeSuccess, nil
+	})
+}
+
 func nxDomainBackend(ttl int) plugin.Handler {
 	return plugin.HandlerFunc(func(_ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
 		m := new(dns.Msg)

plugin/cache/item.go

@@ -16,6 +16,7 @@ type item struct {
 	QType              uint16
 	QClass             uint16
 	Rcode              int
+	Authoritative      bool
 	AuthenticatedData  bool
 	RecursionAvailable bool
 	Answer             []dns.RR
@@ -46,6 +47,7 @@ func newItem(m *dns.Msg, now time.Time, d time.Duration) *item {
 		i.QClass = m.Question[0].Qclass
 	}
 	i.Rcode = m.Rcode
+	i.Authoritative = m.Authoritative
 	i.AuthenticatedData = m.AuthenticatedData
 	i.RecursionAvailable = m.RecursionAvailable
 	i.Answer = m.Answer
@@ -74,12 +76,6 @@ func newItem(m *dns.Msg, now time.Time, d time.Duration) *item {
 }
 
 // toMsg turns i into a message, it tailors the reply to m.
-// The Authoritative bit should be set to 0, but some client stub resolver implementations, most notably,
-// on some legacy systems(e.g. ubuntu 14.04 with glib version 2.20), low-level glibc function `getaddrinfo`
-// useb by Python/Ruby/etc.. will discard answers that do not have this bit set.
-// So we're forced to always set this to 1; regardless if the answer came from the cache or not.
-// On newer systems(e.g. ubuntu 16.04 with glib version 2.23), this issue is resolved.
-// So we may set this bit back to 0 in the future ?
 func (i *item) toMsg(m *dns.Msg, now time.Time, do bool, ad bool) *dns.Msg {
 	ttl := uint32(i.ttl(now)) // #nosec G115 -- ttl is bounded by DNS TTL limits
 	return i.toMsgWithTTL(m, ttl, do, ad)
@@ -90,10 +86,13 @@ func (i *item) toMsgWithTTL(m *dns.Msg, ttl uint32, do bool, ad bool) *dns.Msg {
 	m1 := new(dns.Msg)
 	m1.SetReply(m)
 
-	// Set this to true as some DNS clients discard the *entire* packet when it's non-authoritative.
+	// The AA bit comes from the cached answer instead of being synthesized: a
-	// This is probably not according to spec, but the bit itself is not super useful as this point, so
+	// reply served from cache is no more authoritative than the reply that
-	// just set it to true.
+	// populated it (RFC 1035 section 4.1.1). It was hardcoded to 1 for legacy
-	m1.Authoritative = true
+	// stub resolvers that dropped non-authoritative answers, but that only ever
+	// applied on the cache hit path, so those clients still saw AA=0 on every
+	// miss. See #6185.
+	m1.Authoritative = i.Authoritative
 	m1.AuthenticatedData = i.AuthenticatedData
 	if !do && !ad {
 		// When DNSSEC was not wanted, it can't be authenticated data.

plugin/cache/item_test.go

@@ -1,10 +1,14 @@
 package cache
 
 import (
+	"context"
 	"reflect"
 	"testing"
 	"time"
 
+	"github.com/coredns/coredns/plugin/pkg/dnstest"
+	"github.com/coredns/coredns/plugin/test"
+
 	"github.com/miekg/dns"
 )
 
@@ -21,3 +25,65 @@ func TestNewItemPreservesMonotonicClock(t *testing.T) {
 		t.Fatalf("stored time = %v; want original time %v", i.stored, now)
 	}
 }
+
+// TestCacheDoesNotSynthesizeAA guards issue #6185.
+//
+// A cached answer that came from a non-authoritative upstream must not gain the
+// AA bit when it is served from the cache. Historically toMsg set AA
+// unconditionally, so the same query answered AA=0 on a miss and AA=1 on a hit.
+// Both directions are pinned here.
+func TestCacheDoesNotSynthesizeAA(t *testing.T) {
+	c := New()
+	c.Next = BackendHandler() // replies with Authoritative unset
+
+	req := new(dns.Msg)
+	req.SetQuestion("example.org.", dns.TypeA)
+
+	// Miss: the answer is passed through, AA must stay 0.
+	rec := dnstest.NewRecorder(&test.ResponseWriter{})
+	c.ServeDNS(context.TODO(), rec, req)
+	if rec.Msg.Authoritative {
+		t.Fatalf("cache miss: expected AA=0 from a non-authoritative backend, got AA=1")
+	}
+
+	// Hit: the same answer is rebuilt from the cached item, AA must still be 0.
+	rec = dnstest.NewRecorder(&test.ResponseWriter{})
+	c.ServeDNS(context.TODO(), rec, req)
+	if rec.Msg.Authoritative {
+		t.Errorf("cache hit: expected AA=0, the cached answer was not authoritative, got AA=1")
+	}
+}
+
+// TestCachePreservesAA is the AA=1 counterpart of
+// TestCacheDoesNotSynthesizeAA: an answer that *was* authoritative must still
+// be authoritative when it is replayed from cache. Without it the AA=0 test
+// alone would also pass if toMsg hardcoded AA to 0, silently breaking answers
+// from file, hosts and secondary.
+func TestCachePreservesAA(t *testing.T) {
+	c := New()
+	calls := 0
+	c.Next = authoritativeBackend(&calls)
+
+	req := new(dns.Msg)
+	req.SetQuestion("example.org.", dns.TypeA)
+
+	// Miss: the authoritative answer is passed through.
+	rec := dnstest.NewRecorder(&test.ResponseWriter{})
+	c.ServeDNS(context.TODO(), rec, req)
+	if !rec.Msg.Authoritative {
+		t.Fatalf("cache miss: expected AA=1 from an authoritative backend, got AA=0")
+	}
+
+	// Hit: rebuilt from the cached item, AA must survive the round trip.
+	rec = dnstest.NewRecorder(&test.ResponseWriter{})
+	c.ServeDNS(context.TODO(), rec, req)
+	if !rec.Msg.Authoritative {
+		t.Errorf("cache hit: expected AA=1, the cached answer was authoritative, got AA=0")
+	}
+
+	// A second backend call would mean the "hit" was really a second miss,
+	// which satisfies the AA=1 assertion without ever touching the cache.
+	if calls != 1 {
+		t.Errorf("expected exactly one backend call after miss and hit, got %d", calls)
+	}
+}