cache: default to DNSSEC (#4085)

* cache: default to DNSSEC

This change does away with the DNS/DNSSEC distinction the cache
currently makes. Cache will always make coredns perform a DNSSEC query
and store that result. If a client just needs plain DNS, the DNSSEC
records are stripped from the response.

It should also be more memory efficient, because we store a reply once
and not one DNS and another for DNSSEC.

Fixes: #3836

Signed-off-by: Miek Gieben <miek@miek.nl>

* Change OPT RR when one is present in the msg.

Signed-off-by: Miek Gieben <miek@miek.nl>

* Fix comment for isDNSSEC

Signed-off-by: Miek Gieben <miek@miek.nl>

* Update plugin/cache/handler.go

Co-authored-by: Chris O'Haver <cohaver@infoblox.com>

* Update plugin/cache/item.go

Co-authored-by: Chris O'Haver <cohaver@infoblox.com>

* Code review; fix comment for isDNSSEC

Signed-off-by: Miek Gieben <miek@miek.nl>

* Update doc and set AD to false

Set Authenticated Data to false when DNSSEC was not wanted. Also update
the readme with the new behavior.

Signed-off-by: Miek Gieben <miek@miek.nl>

* Update plugin/cache/handler.go

Co-authored-by: Chris O'Haver <cohaver@infoblox.com>

Co-authored-by: Chris O'Haver <cohaver@infoblox.com>

6 files changed, 204 insertions, 44 deletions

diff --git a/plugin/cache/README.md b/plugin/cache/README.md
index 887acd956..28a427371 100644
--- a/plugin/cache/README.md
+++ b/plugin/cache/README.md
@@ -10,6 +10,9 @@ 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.
+
 This plugin can only be used once per Server Block.
 
 ## Syntax
diff --git a/plugin/cache/cache.go b/plugin/cache/cache.go
index 9988d9c70..32185de19 100644
--- a/plugin/cache/cache.go
+++ b/plugin/cache/cache.go
@@ -65,31 +65,21 @@ 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, do bool) (bool, uint64) {
+func key(qname string, m *dns.Msg, t response.Type) (bool, uint64) {
 	// We don't store truncated responses.
 	if m.Truncated {
 		return false, 0
 	}
-	// Nor errors or Meta or Update
+	// Nor errors or Meta or Update.
 	if t == response.OtherError || t == response.Meta || t == response.Update {
 		return false, 0
 	}
 
-	return true, hash(qname, m.Question[0].Qtype, do)
+	return true, hash(qname, m.Question[0].Qtype)
 }
 
-var one = []byte("1")
-var zero = []byte("0")
-
-func hash(qname string, qtype uint16, do bool) uint64 {
+func hash(qname string, qtype uint16) 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))
@@ -152,14 +142,10 @@ func (w *ResponseWriter) RemoteAddr() net.Addr {
 
 // WriteMsg implements the dns.ResponseWriter interface.
 func (w *ResponseWriter) WriteMsg(res *dns.Msg) error {
-	do := false
-	mt, opt := response.Typify(res, w.now().UTC())
-	if opt != nil {
-		do = opt.Do()
-	}
+	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, do)
+	hasKey, key := key(w.state.Name(), res, mt)
 
 	msgTTL := dnsutil.MinimalTTL(res, mt)
 	var duration time.Duration
@@ -187,18 +173,38 @@ func (w *ResponseWriter) WriteMsg(res *dns.Msg) error {
 		return nil
 	}
 
+	do := w.state.Do()
+
 	// 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())
-	for i := range res.Answer {
-		res.Answer[i].Header().Ttl = ttl
+	j := 0
+	for _, r := range res.Answer {
+		if !do && isDNSSEC(r) {
+			continue
+		}
+		res.Answer[j].Header().Ttl = ttl
+		j++
 	}
-	for i := range res.Ns {
-		res.Ns[i].Header().Ttl = ttl
+	res.Answer = res.Answer[:j]
+	j = 0
+	for _, r := range res.Ns {
+		if !do && isDNSSEC(r) {
+			continue
+		}
+		res.Ns[j].Header().Ttl = ttl
+		j++
 	}
-	for i := range res.Extra {
-		if res.Extra[i].Header().Rrtype != dns.TypeOPT {
-			res.Extra[i].Header().Ttl = ttl
+	res.Ns = res.Ns[:j]
+	j = 0
+	for _, r := range res.Extra {
+		if !do && isDNSSEC(r) {
+			continue
+		}
+		if res.Extra[j].Header().Rrtype != dns.TypeOPT {
+			res.Extra[j].Header().Ttl = ttl
 		}
+		j++
 	}
 	return w.ResponseWriter.WriteMsg(res)
 }
diff --git a/plugin/cache/cache_test.go b/plugin/cache/cache_test.go
index b3ed6d6cc..717276e66 100644
--- a/plugin/cache/cache_test.go
+++ b/plugin/cache/cache_test.go
@@ -46,17 +46,19 @@ var cacheTestCases = []cacheTestCase{
 	{
 		RecursionAvailable: true, AuthenticatedData: true,
 		Case: test.Case{
-			Qname: "mIEK.nL.", Qtype: dns.TypeMX,
+			Qname: "miek.nl.", Qtype: dns.TypeMX,
 			Answer: []dns.RR{
-				test.MX("mIEK.nL.	3600	IN	MX	1 aspmx.l.google.com."),
-				test.MX("mIEK.nL.	3600	IN	MX	10 aspmx2.googlemail.com."),
+				test.MX("miek.nl.	3600	IN	MX	1 aspmx.l.google.com."),
+				test.MX("miek.nl.	3600	IN	MX	10 aspmx2.googlemail.com."),
 			},
 		},
 		in: test.Case{
 			Qname: "mIEK.nL.", Qtype: dns.TypeMX,
 			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."),
+				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,
@@ -136,7 +138,7 @@ 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: false,
+		shouldCache: true,
 	},
 	{
 		RecursionAvailable: true,
@@ -196,7 +198,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, state.Do())
+		valid, k := key(state.Name(), m, mt)
 
 		if valid {
 			crr.set(m, k, mt, c.pttl)
@@ -211,14 +213,16 @@ func TestCache(t *testing.T) {
 		}
 
 		if ok {
-			resp := i.toMsg(m, time.Now().UTC())
+			resp := i.toMsg(m, time.Now().UTC(), state.Do())
 
 			if err := test.Header(tc.Case, resp); err != nil {
+				t.Logf("Bla %v", resp)
 				t.Error(err)
 				continue
 			}
 
 			if err := test.Section(tc.Case, test.Answer, resp.Answer); err != nil {
+				t.Logf("Bla %v -- %v", test.Answer, resp.Answer)
 				t.Error(err)
 			}
 			if err := test.Section(tc.Case, test.Ns, resp.Ns); err != nil {
diff --git a/plugin/cache/do_test.go b/plugin/cache/do_test.go
new file mode 100644
index 000000000..3cf87cabe
--- /dev/null
+++ b/plugin/cache/do_test.go
@@ -0,0 +1,75 @@
+package cache
+
+import (
+	"context"
+	"testing"
+
+	"github.com/coredns/coredns/plugin"
+	"github.com/coredns/coredns/plugin/pkg/dnstest"
+	"github.com/coredns/coredns/plugin/test"
+
+	"github.com/miekg/dns"
+)
+
+func TestDo(t *testing.T) {
+	// cache sets Do and requests that don't have them.
+	c := New()
+	c.Next = echoHandler()
+	req := new(dns.Msg)
+	req.SetQuestion("example.org.", dns.TypeA)
+	rec := dnstest.NewRecorder(&test.ResponseWriter{})
+
+	// No DO set.
+	c.ServeDNS(context.TODO(), rec, req)
+	reply := rec.Msg
+	opt := reply.Extra[len(reply.Extra)-1]
+	if x, ok := opt.(*dns.OPT); !ok {
+		t.Fatalf("Expected OPT RR, got %T", x)
+	}
+	if !opt.(*dns.OPT).Do() {
+		t.Errorf("Expected DO bit to be set, got false")
+	}
+	if x := opt.(*dns.OPT).UDPSize(); x != defaultUDPBufSize {
+		t.Errorf("Expected %d bufsize, got %d", defaultUDPBufSize, x)
+	}
+
+	// Do set - so left alone.
+	const mysize = defaultUDPBufSize * 2
+	setDo(req)
+	// set bufsize to something else than default to see cache doesn't touch it
+	req.Extra[len(req.Extra)-1].(*dns.OPT).SetUDPSize(mysize)
+	c.ServeDNS(context.TODO(), rec, req)
+	reply = rec.Msg
+	opt = reply.Extra[len(reply.Extra)-1]
+	if x, ok := opt.(*dns.OPT); !ok {
+		t.Fatalf("Expected OPT RR, got %T", x)
+	}
+	if !opt.(*dns.OPT).Do() {
+		t.Errorf("Expected DO bit to be set, got false")
+	}
+	if x := opt.(*dns.OPT).UDPSize(); x != mysize {
+		t.Errorf("Expected %d bufsize, got %d", mysize, x)
+	}
+
+	// edns0 set, but not DO, so _not_ left alone.
+	req.Extra[len(req.Extra)-1].(*dns.OPT).SetDo(false)
+	c.ServeDNS(context.TODO(), rec, req)
+	reply = rec.Msg
+	opt = reply.Extra[len(reply.Extra)-1]
+	if x, ok := opt.(*dns.OPT); !ok {
+		t.Fatalf("Expected OPT RR, got %T", x)
+	}
+	if !opt.(*dns.OPT).Do() {
+		t.Errorf("Expected DO bit to be set, got false")
+	}
+	if x := opt.(*dns.OPT).UDPSize(); x != defaultUDPBufSize {
+		t.Errorf("Expected %d bufsize, got %d", defaultUDPBufSize, x)
+	}
+}
+
+func echoHandler() plugin.Handler {
+	return plugin.HandlerFunc(func(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+		w.WriteMsg(r)
+		return dns.RcodeSuccess, nil
+	})
+}
diff --git a/plugin/cache/handler.go b/plugin/cache/handler.go
index 20a058ed2..f079e6c51 100644
--- a/plugin/cache/handler.go
+++ b/plugin/cache/handler.go
@@ -15,6 +15,7 @@ import (
 // ServeDNS implements the plugin.Handler interface.
 func (c *Cache) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
 	state := request.Request{W: w, Req: r}
+	do := state.Do()
 
 	zone := plugin.Zones(c.Zones).Matches(state.Name())
 	if zone == "" {
@@ -22,15 +23,24 @@ 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?
+
 	ttl := 0
 	i := c.getIgnoreTTL(now, state, server)
 	if i != nil {
 		ttl = i.ttl(now)
 	}
 	if i == nil {
+		if !do {
+			setDo(r)
+		}
 		crr := &ResponseWriter{ResponseWriter: w, Cache: c, state: state, server: server}
 		return plugin.NextOrFailure(c.Name(), c.Next, ctx, crr, r)
 	}
@@ -40,11 +50,14 @@ func (c *Cache) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg)
 		now = now.Add(time.Duration(ttl) * time.Second)
 		go func() {
 			r := r.Copy()
+			if !do {
+				setDo(r)
+			}
 			crr := &ResponseWriter{Cache: c, state: state, server: server, prefetch: true, remoteAddr: w.LocalAddr()}
 			plugin.NextOrFailure(c.Name(), c.Next, ctx, crr, r)
 		}()
 	}
-	resp := i.toMsg(r, now)
+	resp := i.toMsg(r, now, do)
 	w.WriteMsg(resp)
 
 	if c.shouldPrefetch(i, now) {
@@ -80,7 +93,7 @@ func (c *Cache) shouldPrefetch(i *item, now time.Time) bool {
 func (c *Cache) Name() string { return "cache" }
 
 func (c *Cache) get(now time.Time, state request.Request, server string) (*item, bool) {
-	k := hash(state.Name(), state.QType(), state.Do())
+	k := hash(state.Name(), state.QType())
 
 	if i, ok := c.ncache.Get(k); ok && i.(*item).ttl(now) > 0 {
 		cacheHits.WithLabelValues(server, Denial).Inc()
@@ -97,7 +110,7 @@ func (c *Cache) get(now time.Time, state request.Request, server string) (*item,
 
 // 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(), state.Do())
+	k := hash(state.Name(), state.QType())
 
 	if i, ok := c.ncache.Get(k); ok {
 		ttl := i.(*item).ttl(now)
@@ -118,7 +131,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(), state.Do())
+	k := hash(state.Name(), state.QType())
 	if i, ok := c.ncache.Get(k); ok {
 		return i.(*item)
 	}
@@ -127,3 +140,22 @@ 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 e3da5bb14..989d57bb0 100644
--- a/plugin/cache/item.go
+++ b/plugin/cache/item.go
@@ -55,7 +55,7 @@ func newItem(m *dns.Msg, now time.Time, d time.Duration) *item {
 // 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) *dns.Msg {
+func (i *item) toMsg(m *dns.Msg, now time.Time, do bool) *dns.Msg {
 	m1 := new(dns.Msg)
 	m1.SetReply(m)
 
@@ -64,6 +64,9 @@ func (i *item) toMsg(m *dns.Msg, now time.Time) *dns.Msg {
 	// just set it to true.
 	m1.Authoritative = true
 	m1.AuthenticatedData = i.AuthenticatedData
+	if !do {
+		m1.AuthenticatedData = false // when DNSSEC was not wanted, it can't be authenticated data.
+	}
 	m1.RecursionAvailable = i.RecursionAvailable
 	m1.Rcode = i.Rcode
 
@@ -72,19 +75,37 @@ func (i *item) toMsg(m *dns.Msg, now time.Time) *dns.Msg {
 	m1.Extra = make([]dns.RR, len(i.Extra))
 
 	ttl := uint32(i.ttl(now))
-	for j, r := range i.Answer {
+	j := 0
+	for _, r := range i.Answer {
+		if !do && isDNSSEC(r) {
+			continue
+		}
 		m1.Answer[j] = dns.Copy(r)
 		m1.Answer[j].Header().Ttl = ttl
+		j++
 	}
-	for j, r := range i.Ns {
+	m1.Answer = m1.Answer[:j]
+	j = 0
+	for _, r := range i.Ns {
+		if !do && isDNSSEC(r) {
+			continue
+		}
 		m1.Ns[j] = dns.Copy(r)
 		m1.Ns[j].Header().Ttl = ttl
+		j++
 	}
+	m1.Ns = m1.Ns[:j]
 	// newItem skips OPT records, so we can just use i.Extra as is.
-	for j, r := range i.Extra {
+	j = 0
+	for _, r := range i.Extra {
+		if !do && isDNSSEC(r) {
+			continue
+		}
 		m1.Extra[j] = dns.Copy(r)
 		m1.Extra[j].Header().Ttl = ttl
+		j++
 	}
+	m1.Extra = m1.Extra[:j]
 	return m1
 }
 
@@ -92,3 +113,22 @@ func (i *item) ttl(now time.Time) int {
 	ttl := int(i.origTTL) - int(now.UTC().Sub(i.stored).Seconds())
 	return ttl
 }
+
+// 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 explictly 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
+}