From acf9a0fa19928e605ac8ac3314890c9fef73e16b Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Thu, 17 Sep 2020 16:28:43 +0200 Subject: [PATCH] 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 * Change OPT RR when one is present in the msg. Signed-off-by: Miek Gieben * Fix comment for isDNSSEC Signed-off-by: Miek Gieben * Update plugin/cache/handler.go Co-authored-by: Chris O'Haver * Update plugin/cache/item.go Co-authored-by: Chris O'Haver * Code review; fix comment for isDNSSEC Signed-off-by: Miek Gieben * 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 * Update plugin/cache/handler.go Co-authored-by: Chris O'Haver Co-authored-by: Chris O'Haver --- plugin/cache/README.md | 3 ++ plugin/cache/cache.go | 64 +++++++++++++++++--------------- plugin/cache/cache_test.go | 20 ++++++---- plugin/cache/do_test.go | 75 ++++++++++++++++++++++++++++++++++++++ plugin/cache/handler.go | 42 ++++++++++++++++++--- plugin/cache/item.go | 48 ++++++++++++++++++++++-- request/request.go | 2 +- 7 files changed, 207 insertions(+), 47 deletions(-) create mode 100644 plugin/cache/do_test.go 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 - } - for i := range res.Ns { - res.Ns[i].Header().Ttl = ttl - } - for i := range res.Extra { - if res.Extra[i].Header().Rrtype != dns.TypeOPT { - res.Extra[i].Header().Ttl = ttl + j := 0 + for _, r := range res.Answer { + if !do && isDNSSEC(r) { + continue } + res.Answer[j].Header().Ttl = ttl + j++ + } + res.Answer = res.Answer[:j] + j = 0 + for _, r := range res.Ns { + if !do && isDNSSEC(r) { + continue + } + res.Ns[j].Header().Ttl = ttl + j++ + } + 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 +} diff --git a/request/request.go b/request/request.go index 7374b0bd6..649e573c1 100644 --- a/request/request.go +++ b/request/request.go @@ -144,7 +144,7 @@ func (r *Request) Family() int { return 2 } -// Do returns if the request has the DO (DNSSEC OK) bit set. +// Do returns true if the request has the DO (DNSSEC OK) bit set. func (r *Request) Do() bool { if r.size != 0 { return r.do