diff --git a/plugin/cache/cache_test.go b/plugin/cache/cache_test.go index f4bfa8160..50f80bf5c 100644 --- a/plugin/cache/cache_test.go +++ b/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 != true { - t.Error("Expected Authoritative Answer bit to be true, but was false") + if resp.Authoritative != tc.out.Authoritative { + t.Errorf("Expected Authoritative Answer bit to be %t, but got %t", tc.out.Authoritative, resp.Authoritative) } 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 { - t.Error("expected cached fresh response to preserve authoritative cache reply shaping") + if rec.Msg.Authoritative { + 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) diff --git a/plugin/cache/item.go b/plugin/cache/item.go index 18ec9fdc2..9634395d7 100644 --- a/plugin/cache/item.go +++ b/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. - // This is probably not according to spec, but the bit itself is not super useful as this point, so - // just set it to true. - m1.Authoritative = true + // The AA bit comes from the cached answer instead of being synthesized: a + // reply served from cache is no more authoritative than the reply that + // populated it (RFC 1035 section 4.1.1). It was hardcoded to 1 for legacy + // 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. diff --git a/plugin/cache/item_test.go b/plugin/cache/item_test.go index f925c8d01..62a5b3fa0 100644 --- a/plugin/cache/item_test.go +++ b/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) + } +}