plugin/cache: cache now uses source query DNSSEC option for upstream refresh (#5671)

Signed-off-by: Grant Spence <gspence@redhat.com>

Signed-off-by: Grant Spence <gspence@redhat.com>

7 files changed, 69 insertions, 85 deletions

diff --git a/plugin/cache/README.md b/plugin/cache/README.md
index 562f5bd9a..6fc20ae2c 100644
--- a/plugin/cache/README.md
+++ b/plugin/cache/README.md
@@ -10,8 +10,7 @@ With *cache* enabled, all records except zone transfers and metadata records wil
 3600s. Caching is mostly useful in a scenario when fetching data from the backend (upstream,
 database, etc.) is expensive.
 
-*Cache* will change the query to enable DNSSEC (DNSSEC OK; DO) if it passes through the plugin. If
-the client didn't request any DNSSEC (records), these are filtered out when replying.
+*Cache* will pass DNSSEC (DNSSEC OK; DO) options through the plugin for upstream queries.
 
 This plugin can only be used once per Server Block.
 
diff --git a/plugin/cache/cache.go b/plugin/cache/cache.go
index b4767937d..54f2587fa 100644
--- a/plugin/cache/cache.go
+++ b/plugin/cache/cache.go
@@ -76,7 +76,7 @@ func New() *Cache {
 // key returns key under which we store the item, -1 will be returned if we don't store the message.
 // Currently we do not cache Truncated, errors zone transfers or dynamic update messages.
 // qname holds the already lowercased qname.
-func key(qname string, m *dns.Msg, t response.Type) (bool, uint64) {
+func key(qname string, m *dns.Msg, t response.Type, do bool) (bool, uint64) {
 	// We don't store truncated responses.
 	if m.Truncated {
 		return false, 0
@@ -86,11 +86,21 @@ func key(qname string, m *dns.Msg, t response.Type) (bool, uint64) {
 		return false, 0
 	}
 
-	return true, hash(qname, m.Question[0].Qtype)
+	return true, hash(qname, m.Question[0].Qtype, do)
 }
 
-func hash(qname string, qtype uint16) uint64 {
+var one = []byte("1")
+var zero = []byte("0")
+
+func hash(qname string, qtype uint16, do bool) uint64 {
 	h := fnv.New64()
+
+	if do {
+		h.Write(one)
+	} else {
+		h.Write(zero)
+	}
+
 	h.Write([]byte{byte(qtype >> 8)})
 	h.Write([]byte{byte(qtype)})
 	h.Write([]byte(qname))
@@ -145,6 +155,7 @@ func newPrefetchResponseWriter(server string, state request.Request, c *Cache) *
 		Cache:          c,
 		state:          state,
 		server:         server,
+		do:             state.Do(),
 		prefetch:       true,
 		remoteAddr:     addr,
 	}
@@ -163,7 +174,7 @@ func (w *ResponseWriter) WriteMsg(res *dns.Msg) error {
 	mt, _ := response.Typify(res, w.now().UTC())
 
 	// key returns empty string for anything we don't want to cache.
-	hasKey, key := key(w.state.Name(), res, mt)
+	hasKey, key := key(w.state.Name(), res, mt, w.do)
 
 	msgTTL := dnsutil.MinimalTTL(res, mt)
 	var duration time.Duration
@@ -191,11 +202,10 @@ func (w *ResponseWriter) WriteMsg(res *dns.Msg) error {
 	}
 
 	// Apply capped TTL to this reply to avoid jarring TTL experience 1799 -> 8 (e.g.)
-	// We also may need to filter out DNSSEC records, see toMsg() for similar code.
 	ttl := uint32(duration.Seconds())
-	res.Answer = filterRRSlice(res.Answer, ttl, w.do, false)
-	res.Ns = filterRRSlice(res.Ns, ttl, w.do, false)
-	res.Extra = filterRRSlice(res.Extra, ttl, w.do, false)
+	res.Answer = filterRRSlice(res.Answer, ttl, false)
+	res.Ns = filterRRSlice(res.Ns, ttl, false)
+	res.Extra = filterRRSlice(res.Extra, ttl, false)
 
 	if !w.do && !w.ad {
 		// unset AD bit if requester is not OK with DNSSEC
diff --git a/plugin/cache/cache_test.go b/plugin/cache/cache_test.go
index 0328c8411..861e9b8fd 100644
--- a/plugin/cache/cache_test.go
+++ b/plugin/cache/cache_test.go
@@ -27,6 +27,7 @@ type cacheTestCase struct {
 
 var cacheTestCases = []cacheTestCase{
 	{
+		// Test with Authenticated Data bit
 		RecursionAvailable: true, AuthenticatedData: true,
 		Case: test.Case{
 			Qname: "miek.nl.", Qtype: dns.TypeMX,
@@ -45,6 +46,7 @@ var cacheTestCases = []cacheTestCase{
 		shouldCache: true,
 	},
 	{
+		// Test case sensitivity
 		RecursionAvailable: true, AuthenticatedData: true,
 		Case: test.Case{
 			Qname: "miek.nl.", Qtype: dns.TypeMX,
@@ -58,13 +60,12 @@ var cacheTestCases = []cacheTestCase{
 			Answer: []dns.RR{
 				test.MX("miek.nl.	3601	IN	MX	1 aspmx.l.google.com."),
 				test.MX("miek.nl.	3601	IN	MX	10 aspmx2.googlemail.com."),
-				// RRSIG must be here, because we are always doing DNSSEC lookups, and miek.nl MX is tested later in this list as well.
-				test.RRSIG("miek.nl.	3600	IN	RRSIG	MX 8 2 1800 20160521031301 20160421031301 12051 miek.nl. lAaEzB5teQLLKyDenatmyhca7blLRg9DoGNrhe3NReBZN5C5/pMQk8Jc u25hv2fW23/SLm5IC2zaDpp2Fzgm6Jf7e90/yLcwQPuE7JjS55WMF+HE LEh7Z6AEb+Iq4BWmNhUz6gPxD4d9eRMs7EAzk13o1NYi5/JhfL6IlaYy qkc="),
 			},
 		},
 		shouldCache: true,
 	},
 	{
+		// Test truncated responses don't get cached
 		Truncated: true,
 		Case: test.Case{
 			Qname: "miek.nl.", Qtype: dns.TypeMX,
@@ -74,6 +75,7 @@ var cacheTestCases = []cacheTestCase{
 		shouldCache: false,
 	},
 	{
+		// Test dns.RcodeNameError cache
 		RecursionAvailable: true,
 		Case: test.Case{
 			Rcode: dns.RcodeNameError,
@@ -92,6 +94,7 @@ var cacheTestCases = []cacheTestCase{
 		shouldCache: true,
 	},
 	{
+		// Test dns.RcodeServerFailure cache
 		RecursionAvailable: true,
 		Case: test.Case{
 			Rcode: dns.RcodeServerFailure,
@@ -106,6 +109,7 @@ var cacheTestCases = []cacheTestCase{
 		shouldCache: true,
 	},
 	{
+		// Test dns.RcodeNotImplemented cache
 		RecursionAvailable: true,
 		Case: test.Case{
 			Rcode: dns.RcodeNotImplemented,
@@ -120,6 +124,8 @@ var cacheTestCases = []cacheTestCase{
 		shouldCache: true,
 	},
 	{
+		// Test cache has separate items for query DO Bit value
+		// This doesn't cache because this RRSIG is expired and previous tests used DO Bit false
 		RecursionAvailable: true,
 		Case: test.Case{
 			Qname: "miek.nl.", Qtype: dns.TypeMX,
@@ -139,9 +145,10 @@ var cacheTestCases = []cacheTestCase{
 				test.RRSIG("miek.nl.	1800	IN	RRSIG	MX 8 2 1800 20160521031301 20160421031301 12051 miek.nl. lAaEzB5teQLLKyDenatmyhca7blLRg9DoGNrhe3NReBZN5C5/pMQk8Jc u25hv2fW23/SLm5IC2zaDpp2Fzgm6Jf7e90/yLcwQPuE7JjS55WMF+HE LEh7Z6AEb+Iq4BWmNhUz6gPxD4d9eRMs7EAzk13o1NYi5/JhfL6IlaYy qkc="),
 			},
 		},
-		shouldCache: true,
+		shouldCache: false,
 	},
 	{
+		// Test DO Bit with a RRSIG that is not expired
 		RecursionAvailable: true,
 		Case: test.Case{
 			Qname: "example.org.", Qtype: dns.TypeMX,
@@ -164,6 +171,7 @@ var cacheTestCases = []cacheTestCase{
 		shouldCache: true,
 	},
 	{
+		// Test negative zone exception
 		in: test.Case{
 			Rcode: dns.RcodeNameError,
 			Qname: "neg-disabled.example.org.", Qtype: dns.TypeA,
@@ -175,6 +183,7 @@ var cacheTestCases = []cacheTestCase{
 		shouldCache: false,
 	},
 	{
+		// Test positive zone exception
 		in: test.Case{
 			Rcode: dns.RcodeSuccess,
 			Qname: "pos-disabled.example.org.", Qtype: dns.TypeA,
@@ -186,6 +195,7 @@ var cacheTestCases = []cacheTestCase{
 		shouldCache: false,
 	},
 	{
+		// Test positive zone exception with negative answer
 		in: test.Case{
 			Rcode: dns.RcodeNameError,
 			Qname: "pos-disabled.example.org.", Qtype: dns.TypeA,
@@ -203,6 +213,7 @@ var cacheTestCases = []cacheTestCase{
 		shouldCache: true,
 	},
 	{
+		// Test negative zone exception with positive answer
 		in: test.Case{
 			Rcode: dns.RcodeSuccess,
 			Qname: "neg-disabled.example.org.", Qtype: dns.TypeA,
@@ -258,7 +269,7 @@ func TestCache(t *testing.T) {
 		state := request.Request{W: &test.ResponseWriter{}, Req: m}
 
 		mt, _ := response.Typify(m, utc)
-		valid, k := key(state.Name(), m, mt)
+		valid, k := key(state.Name(), m, mt, state.Do())
 
 		if valid {
 			crr.set(m, k, mt, c.pttl)
@@ -647,6 +658,7 @@ func TestCacheWildcardMetadata(t *testing.T) {
 
 	req := new(dns.Msg)
 	req.SetQuestion(qname, dns.TypeA)
+	state := request.Request{W: &test.ResponseWriter{}, Req: req}
 
 	// 1. Test writing wildcard metadata retrieved from backend to the cache
 
@@ -656,7 +668,7 @@ func TestCacheWildcardMetadata(t *testing.T) {
 	if c.pcache.Len() != 1 {
 		t.Errorf("Msg should have been cached")
 	}
-	_, k := key(qname, w.Msg, response.NoError)
+	_, k := key(qname, w.Msg, response.NoError, state.Do())
 	i, _ := c.pcache.Get(k)
 	if i.(*item).wildcard != wildcard {
 		t.Errorf("expected wildcard reponse to enter cache with cache item's wildcard = %q, got %q", wildcard, i.(*item).wildcard)
diff --git a/plugin/cache/dnssec.go b/plugin/cache/dnssec.go
index cf908037e..ec5ff41cb 100644
--- a/plugin/cache/dnssec.go
+++ b/plugin/cache/dnssec.go
@@ -2,35 +2,13 @@ package cache
 
 import "github.com/miekg/dns"
 
-// isDNSSEC returns true if r is a DNSSEC record. NSEC,NSEC3,DS and RRSIG/SIG
-// are DNSSEC records. DNSKEYs is not in this list on the assumption that the
-// client explicitly asked for it.
-func isDNSSEC(r dns.RR) bool {
-	switch r.Header().Rrtype {
-	case dns.TypeNSEC:
-		return true
-	case dns.TypeNSEC3:
-		return true
-	case dns.TypeDS:
-		return true
-	case dns.TypeRRSIG:
-		return true
-	case dns.TypeSIG:
-		return true
-	}
-	return false
-}
-
-// filterRRSlice filters rrs and removes DNSSEC RRs when do is false. In the returned slice
-// the TTLs are set to ttl. If dup is true the RRs in rrs are _copied_ into the slice that is
+// filterRRSlice filters out OPT RRs, and sets all RR TTLs to ttl.
+// If dup is true the RRs in rrs are _copied_ into the slice that is
 // returned.
-func filterRRSlice(rrs []dns.RR, ttl uint32, do, dup bool) []dns.RR {
+func filterRRSlice(rrs []dns.RR, ttl uint32, dup bool) []dns.RR {
 	j := 0
 	rs := make([]dns.RR, len(rrs))
 	for _, r := range rrs {
-		if !do && isDNSSEC(r) {
-			continue
-		}
 		if r.Header().Rrtype == dns.TypeOPT {
 			continue
 		}
diff --git a/plugin/cache/dnssec_test.go b/plugin/cache/dnssec_test.go
index a746387bf..b73d52cf7 100644
--- a/plugin/cache/dnssec_test.go
+++ b/plugin/cache/dnssec_test.go
@@ -7,6 +7,7 @@ import (
 	"github.com/coredns/coredns/plugin"
 	"github.com/coredns/coredns/plugin/pkg/dnstest"
 	"github.com/coredns/coredns/plugin/test"
+	"github.com/coredns/coredns/request"
 
 	"github.com/miekg/dns"
 )
@@ -67,19 +68,27 @@ func dnssecHandler() plugin.Handler {
 	return plugin.HandlerFunc(func(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
 		m := new(dns.Msg)
 		m.SetQuestion("example.org.", dns.TypeA)
+		state := request.Request{W: &test.ResponseWriter{}, Req: r}
 
 		m.AuthenticatedData = true
-		m.Answer = make([]dns.RR, 4)
-		m.Answer[0] = test.CNAME("invent.example.org.		1781	IN	CNAME	leptone.example.org.")
-		m.Answer[1] = test.RRSIG("invent.example.org.		1781	IN	RRSIG	CNAME 8 3 1800 20201012085750 20200912082613 57411 example.org. ijSv5FmsNjFviBcOFwQgqjt073lttxTTNqkno6oMa3DD3kC+")
-		m.Answer[2] = test.A("leptone.example.org.	1781	IN	A	195.201.182.103")
-		m.Answer[3] = test.RRSIG("leptone.example.org.	1781	IN	RRSIG	A 8 3 1800 20201012093630 20200912083827 57411 example.org. eLuSOkLAzm/WIOpaZD3/4TfvKP1HAFzjkis9LIJSRVpQt307dm9WY9")
+		// If query has the DO bit, then send DNSSEC responses (RRSIGs)
+		if state.Do() {
+			m.Answer = make([]dns.RR, 4)
+			m.Answer[0] = test.CNAME("invent.example.org.		1781	IN	CNAME	leptone.example.org.")
+			m.Answer[1] = test.RRSIG("invent.example.org.		1781	IN	RRSIG	CNAME 8 3 1800 20201012085750 20200912082613 57411 example.org. ijSv5FmsNjFviBcOFwQgqjt073lttxTTNqkno6oMa3DD3kC+")
+			m.Answer[2] = test.A("leptone.example.org.	1781	IN	A	195.201.182.103")
+			m.Answer[3] = test.RRSIG("leptone.example.org.	1781	IN	RRSIG	A 8 3 1800 20201012093630 20200912083827 57411 example.org. eLuSOkLAzm/WIOpaZD3/4TfvKP1HAFzjkis9LIJSRVpQt307dm9WY9")
+		} else {
+			m.Answer = make([]dns.RR, 2)
+			m.Answer[0] = test.CNAME("invent.example.org.		1781	IN	CNAME	leptone.example.org.")
+			m.Answer[1] = test.A("leptone.example.org.	1781	IN	A	195.201.182.103")
+		}
 		w.WriteMsg(m)
 		return dns.RcodeSuccess, nil
 	})
 }
 
-func TestFliterRRSlice(t *testing.T) {
+func TestFilterRRSlice(t *testing.T) {
 	rrs := []dns.RR{
 		test.CNAME("invent.example.org.		1781	IN	CNAME	leptone.example.org."),
 		test.RRSIG("invent.example.org.		1781	IN	RRSIG	CNAME 8 3 1800 20201012085750 20200912082613 57411 example.org. ijSv5FmsNjFviBcOFwQgqjt073lttxTTNqkno6oMa3DD3kC+"),
@@ -87,7 +96,7 @@ func TestFliterRRSlice(t *testing.T) {
 		test.RRSIG("leptone.example.org.	1781	IN	RRSIG	A 8 3 1800 20201012093630 20200912083827 57411 example.org. eLuSOkLAzm/WIOpaZD3/4TfvKP1HAFzjkis9LIJSRVpQt307dm9WY9"),
 	}
 
-	filter1 := filterRRSlice(rrs, 0, true, false)
+	filter1 := filterRRSlice(rrs, 0, false)
 	if len(filter1) != 4 {
 		t.Errorf("Expected 4 RRs after filtering, got %d", len(filter1))
 	}
@@ -101,9 +110,9 @@ func TestFliterRRSlice(t *testing.T) {
 		t.Errorf("Expected 2 RRSIGs after filtering, got %d", rrsig)
 	}
 
-	filter2 := filterRRSlice(rrs, 0, false, false)
-	if len(filter2) != 2 {
-		t.Errorf("Expected 2 RRs after filtering, got %d", len(filter2))
+	filter2 := filterRRSlice(rrs, 0, false)
+	if len(filter2) != 4 {
+		t.Errorf("Expected 4 RRs after filtering, got %d", len(filter2))
 	}
 	rrsig = 0
 	for _, f := range filter2 {
@@ -111,7 +120,7 @@ func TestFliterRRSlice(t *testing.T) {
 			rrsig++
 		}
 	}
-	if rrsig != 0 {
-		t.Errorf("Expected 0 RRSIGs after filtering, got %d", rrsig)
+	if rrsig != 2 {
+		t.Errorf("Expected 2 RRSIGs after filtering, got %d", rrsig)
 	}
 }
diff --git a/plugin/cache/handler.go b/plugin/cache/handler.go
index ec2135e8c..e60637af8 100644
--- a/plugin/cache/handler.go
+++ b/plugin/cache/handler.go
@@ -28,12 +28,10 @@ func (c *Cache) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg)
 	now := c.now().UTC()
 	server := metrics.WithServer(ctx)
 
-	// On cache miss, if the request has the OPT record and the DO bit set we leave the message as-is. If there isn't a DO bit
-	// set we will modify the request to _add_ one. This means we will always do DNSSEC lookups on cache misses.
-	// When writing to cache, any DNSSEC RRs in the response are written to cache with the response.
-	// When sending a response to a non-DNSSEC client, we remove DNSSEC RRs from the response. We use a 2048 buffer size, which is
-	// less than 4096 (and older default) and more than 1024 which may be too small. We might need to tweaks this
-	// value to be smaller still to prevent UDP fragmentation?
+	// On cache refresh, we will just use the DO bit from the incoming query for the refresh since we key our cache
+	// with the query DO bit. That means two separate cache items for the query DO bit true or false. In the situation
+	// in which upstream doesn't support DNSSEC, the two cache items will effectively be the same. Regardless, any
+	// DNSSEC RRs in the response are written to cache with the response.
 
 	ttl := 0
 	i := c.getIgnoreTTL(now, state, server)
@@ -101,9 +99,6 @@ func (c *Cache) doPrefetch(ctx context.Context, state request.Request, cw *Respo
 }
 
 func (c *Cache) doRefresh(ctx context.Context, state request.Request, cw dns.ResponseWriter) (int, error) {
-	if !state.Do() {
-		setDo(state.Req)
-	}
 	return plugin.NextOrFailure(c.Name(), c.Next, ctx, cw, state.Req)
 }
 
@@ -121,7 +116,7 @@ func (c *Cache) Name() string { return "cache" }
 
 // getIgnoreTTL unconditionally returns an item if it exists in the cache.
 func (c *Cache) getIgnoreTTL(now time.Time, state request.Request, server string) *item {
-	k := hash(state.Name(), state.QType())
+	k := hash(state.Name(), state.QType(), state.Do())
 	cacheRequests.WithLabelValues(server, c.zonesMetricLabel, c.viewMetricLabel).Inc()
 
 	if i, ok := c.ncache.Get(k); ok {
@@ -145,7 +140,7 @@ func (c *Cache) getIgnoreTTL(now time.Time, state request.Request, server string
 }
 
 func (c *Cache) exists(state request.Request) *item {
-	k := hash(state.Name(), state.QType())
+	k := hash(state.Name(), state.QType(), state.Do())
 	if i, ok := c.ncache.Get(k); ok {
 		return i.(*item)
 	}
@@ -154,22 +149,3 @@ func (c *Cache) exists(state request.Request) *item {
 	}
 	return nil
 }
-
-// setDo sets the DO bit and UDP buffer size in the message m.
-func setDo(m *dns.Msg) {
-	o := m.IsEdns0()
-	if o != nil {
-		o.SetDo()
-		o.SetUDPSize(defaultUDPBufSize)
-		return
-	}
-
-	o = &dns.OPT{Hdr: dns.RR_Header{Name: ".", Rrtype: dns.TypeOPT}}
-	o.SetDo()
-	o.SetUDPSize(defaultUDPBufSize)
-	m.Extra = append(m.Extra, o)
-}
-
-// defaultUDPBufsize is the bufsize the cache plugin uses on outgoing requests that don't
-// have an OPT RR.
-const defaultUDPBufSize = 2048
diff --git a/plugin/cache/item.go b/plugin/cache/item.go
index 6b51a5bad..c5aeccdcc 100644
--- a/plugin/cache/item.go
+++ b/plugin/cache/item.go
@@ -87,9 +87,9 @@ func (i *item) toMsg(m *dns.Msg, now time.Time, do bool, ad bool) *dns.Msg {
 	m1.Extra = make([]dns.RR, len(i.Extra))
 
 	ttl := uint32(i.ttl(now))
-	m1.Answer = filterRRSlice(i.Answer, ttl, do, true)
-	m1.Ns = filterRRSlice(i.Ns, ttl, do, true)
-	m1.Extra = filterRRSlice(i.Extra, ttl, do, true)
+	m1.Answer = filterRRSlice(i.Answer, ttl, true)
+	m1.Ns = filterRRSlice(i.Ns, ttl, true)
+	m1.Extra = filterRRSlice(i.Extra, ttl, true)
 
 	return m1
 }