From 923471cf431344dc0b68f4e9f6e26d0f875379c1 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Sat, 1 Dec 2018 17:07:38 +0000 Subject: [PATCH] Fix EDNS0 compliance Do SizeAndDo in the server and remove all uses of this from the plugins and do it in the server in the ScrubWiriter. Also *always* do it. This is to get into compliance for https://dnsflagday.net/. The pkg/edns0 now exports the EDNS0 options we understand; this is exported to allow plugins add things here. Note I can't test it because of: https://github.com/miekg/dns/issues/857 Running a test instance and pointing the https://ednscomp.isc.org/ednscomp to it shows the tests are now fixed: ~~~ EDNS Compliance Tester Checking: 'miek.nl' as at 2018-12-01T17:53:15Z miek.nl. @147.75.204.203 (drone.coredns.io.): dns=ok edns=ok edns1=ok edns@512=ok ednsopt=ok edns1opt=ok do=ok ednsflags=ok docookie=ok edns512tcp=ok optlist=ok miek.nl. @2604:1380:2002:a000::1 (drone.coredns.io.): dns=ok edns=ok edns1=ok edns@512=ok ednsopt=ok edns1opt=ok do=ok ednsflags=ok docookie=ok edns512tcp=ok optlist=ok All Ok Codes ok - test passed. ~~~ Signed-off-by: Miek Gieben --- plugin/backend_lookup.go | 1 - plugin/chaos/chaos.go | 1 - plugin/dnssec/handler.go | 1 - plugin/dnssec/handler_test.go | 2 +- plugin/erratic/erratic.go | 1 - plugin/file/file.go | 1 - plugin/log/log.go | 1 - plugin/pkg/edns/edns.go | 27 +++++++++++++++++++++++++++ plugin/whoami/whoami.go | 1 - request/edns0.go | 32 ++++++++++++++++++++++++++++++++ request/request.go | 12 +++++++++--- request/request_test.go | 8 -------- request/writer.go | 2 ++ test/edns0_test.go | 33 +++++++++++++++++++++++++++++++++ 14 files changed, 104 insertions(+), 19 deletions(-) create mode 100644 request/edns0.go create mode 100644 test/edns0_test.go diff --git a/plugin/backend_lookup.go b/plugin/backend_lookup.go index 090f362156f5..95ec467fde86 100644 --- a/plugin/backend_lookup.go +++ b/plugin/backend_lookup.go @@ -425,7 +425,6 @@ func BackendError(b ServiceBackend, zone string, rcode int, state request.Reques m.Authoritative, m.RecursionAvailable = true, true m.Ns, _ = SOA(b, zone, state, opt) - state.SizeAndDo(m) state.W.WriteMsg(m) // Return success as the rcode to signal we have written to the client. return dns.RcodeSuccess, err diff --git a/plugin/chaos/chaos.go b/plugin/chaos/chaos.go index 60b002bdb25f..a66c5f003e38 100644 --- a/plugin/chaos/chaos.go +++ b/plugin/chaos/chaos.go @@ -46,7 +46,6 @@ func (c Chaos) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) ( } m.Answer = []dns.RR{&dns.TXT{Hdr: hdr, Txt: []string{trim(hostname)}}} } - state.SizeAndDo(m) w.WriteMsg(m) return 0, nil } diff --git a/plugin/dnssec/handler.go b/plugin/dnssec/handler.go index c8bddc01c471..6153bf331108 100644 --- a/plugin/dnssec/handler.go +++ b/plugin/dnssec/handler.go @@ -33,7 +33,6 @@ func (d Dnssec) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) if qname == z { resp := d.getDNSKEY(state, z, do, server) resp.Authoritative = true - state.SizeAndDo(resp) w.WriteMsg(resp) return dns.RcodeSuccess, nil } diff --git a/plugin/dnssec/handler_test.go b/plugin/dnssec/handler_test.go index 35444eecb512..8e997bca8121 100644 --- a/plugin/dnssec/handler_test.go +++ b/plugin/dnssec/handler_test.go @@ -26,7 +26,7 @@ var dnssecTestCases = []test.Case{ test.DNSKEY("miek.nl. 3600 IN DNSKEY 257 3 13 0J8u0XJ9GNGFEBXuAmLu04taHG4"), test.RRSIG("miek.nl. 3600 IN RRSIG DNSKEY 13 2 3600 20160503150844 20160425120844 18512 miek.nl. Iw/kNOyM"), }, - Extra: []dns.RR{test.OPT(4096, true)}, + /* Extra: []dns.RR{test.OPT(4096, true)}, this has moved to the server and can't be test here */ }, } diff --git a/plugin/erratic/erratic.go b/plugin/erratic/erratic.go index f60e605d15c1..3b22e225d38c 100644 --- a/plugin/erratic/erratic.go +++ b/plugin/erratic/erratic.go @@ -97,7 +97,6 @@ func (e *Erratic) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg time.Sleep(e.duration) } - state.SizeAndDo(m) w.WriteMsg(m) return 0, nil diff --git a/plugin/file/file.go b/plugin/file/file.go index ee4d64da0cbc..57ce3fd170d1 100644 --- a/plugin/file/file.go +++ b/plugin/file/file.go @@ -51,7 +51,6 @@ func (f File) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (i m := new(dns.Msg) m.SetReply(r) m.Authoritative, m.RecursionAvailable = true, true - state.SizeAndDo(m) w.WriteMsg(m) log.Infof("Notify from %s for %s: checking transfer", state.IP(), zone) diff --git a/plugin/log/log.go b/plugin/log/log.go index 98f00d762320..d963a134fd17 100644 --- a/plugin/log/log.go +++ b/plugin/log/log.go @@ -43,7 +43,6 @@ func (l Logger) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) } else { answer := new(dns.Msg) answer.SetRcode(r, rc) - state.SizeAndDo(answer) vars.Report(ctx, state, vars.Dropped, rcode.ToString(rc), answer.Len(), time.Now()) diff --git a/plugin/pkg/edns/edns.go b/plugin/pkg/edns/edns.go index 3f0ea5e16d0f..42cf74e9b6ab 100644 --- a/plugin/pkg/edns/edns.go +++ b/plugin/pkg/edns/edns.go @@ -3,10 +3,37 @@ package edns import ( "errors" + "sync" "github.com/miekg/dns" ) +// Supported is the list of extra supported EDNS0 options. By default we supoort: +// dns.EDNS0NSID, dns.EDNS0EXPIRE, dns.EDNS0COOKIE, dns.EDNS0TCPKEEPALIVE, dns.EDNS0PADDING. These +// values are not in this map and checked directly in the server. +var Supported = new(supported) + +type supported struct { + m map[uint16]struct{} + sync.RWMutex +} + +// SetOption adds a new supported option the set of EDNS0 options that we supported. Plugins typically call +// this in their setup code to signal support for a new option. +func (s *supported) SetOption(e dns.EDNS0) { + s.Lock() + s.m[e.Option()] = struct{}{} + s.Unlock() +} + +// Option returns true if e is supported as an extra EDNS0 option. +func (s *supported) Option(e dns.EDNS0) bool { + s.RLock() + _, ok := s.m[e.Option()] + s.RUnlock() + return ok +} + // Version checks the EDNS version in the request. If error // is nil everything is OK and we can invoke the plugin. If non-nil, the // returned Msg is valid to be returned to the client (and should). For some diff --git a/plugin/whoami/whoami.go b/plugin/whoami/whoami.go index b2ba25e5eec5..155880eae03d 100644 --- a/plugin/whoami/whoami.go +++ b/plugin/whoami/whoami.go @@ -49,7 +49,6 @@ func (wh Whoami) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) a.Extra = []dns.RR{rr, srv} - state.SizeAndDo(a) w.WriteMsg(a) return 0, nil diff --git a/request/edns0.go b/request/edns0.go new file mode 100644 index 000000000000..d6fc8c9cbafa --- /dev/null +++ b/request/edns0.go @@ -0,0 +1,32 @@ +package request + +import ( + "github.com/coredns/coredns/plugin/pkg/edns" + + "github.com/miekg/dns" +) + +func supportedOptions(o []dns.EDNS0) []dns.EDNS0 { + var supported = make([]dns.EDNS0, 0, 3) + // For as long as possible try looking up in the map, because that need an Rlock. + for _, opt := range o { + switch code := opt.Option(); code { + case dns.EDNS0NSID: + fallthrough + case dns.EDNS0EXPIRE: + fallthrough + case dns.EDNS0COOKIE: + fallthrough + case dns.EDNS0TCPKEEPALIVE: + fallthrough + case dns.EDNS0PADDING: + supported = append(supported, opt) + default: + if edns.Supported.Option(opt) { + supported = append(supported, opt) + } + } + + } + return supported +} diff --git a/request/request.go b/request/request.go index 260d73f5eba0..1163009e2113 100644 --- a/request/request.go +++ b/request/request.go @@ -194,7 +194,7 @@ func (r *Request) Size() int { // SizeAndDo adds an OPT record that the reflects the intent from request. // The returned bool indicated if an record was found and normalised. func (r *Request) SizeAndDo(m *dns.Msg) bool { - o := r.Req.IsEdns0() // TODO(miek): speed this up + o := r.Req.IsEdns0() if o == nil { return false } @@ -208,6 +208,10 @@ func (r *Request) SizeAndDo(m *dns.Msg) bool { mo.SetUDPSize(o.UDPSize()) mo.Hdr.Ttl &= 0xff00 // clear flags + if len(o.Option) > 0 { + o.Option = supportedOptions(o.Option) + } + if odo { mo.SetDo() } @@ -219,6 +223,10 @@ func (r *Request) SizeAndDo(m *dns.Msg) bool { o.SetVersion(0) o.Hdr.Ttl &= 0xff00 // clear flags + if len(o.Option) > 0 { + o.Option = supportedOptions(o.Option) + } + if odo { o.SetDo() } @@ -288,7 +296,6 @@ func (r *Request) Scrub(reply *dns.Msg) *dns.Msg { } if rl <= size { - r.SizeAndDo(reply) return reply } @@ -324,7 +331,6 @@ func (r *Request) Scrub(reply *dns.Msg) *dns.Msg { // this extra m-1 step does make it fit in the client's buffer however. } - r.SizeAndDo(reply) reply.Truncated = true return reply } diff --git a/request/request_test.go b/request/request_test.go index 5e814f76ee88..4411c6a8246e 100644 --- a/request/request_test.go +++ b/request/request_test.go @@ -123,10 +123,6 @@ func TestRequestScrubExtraEdns0(t *testing.T) { if reply.Truncated { t.Errorf("Want scrub to not set truncated bit") } - opt := reply.Extra[len(reply.Extra)-1] - if opt.Header().Rrtype != dns.TypeOPT { - t.Errorf("Last RR must be OPT record") - } } func TestRequestScrubExtraRegression(t *testing.T) { @@ -153,10 +149,6 @@ func TestRequestScrubExtraRegression(t *testing.T) { if reply.Truncated { t.Errorf("Want scrub to not set truncated bit") } - opt := reply.Extra[len(reply.Extra)-1] - if opt.Header().Rrtype != dns.TypeOPT { - t.Errorf("Last RR must be OPT record") - } } func TestTruncation(t *testing.T) { diff --git a/request/writer.go b/request/writer.go index ffbbe93e3cdf..67be53ebbe7e 100644 --- a/request/writer.go +++ b/request/writer.go @@ -15,6 +15,8 @@ func NewScrubWriter(req *dns.Msg, w dns.ResponseWriter) *ScrubWriter { return &S // scrub on the message m and will then write it to the client. func (s *ScrubWriter) WriteMsg(m *dns.Msg) error { state := Request{Req: s.req, W: s.ResponseWriter} + n := state.Scrub(m) + state.SizeAndDo(n) return s.ResponseWriter.WriteMsg(n) } diff --git a/test/edns0_test.go b/test/edns0_test.go new file mode 100644 index 000000000000..6cb1af958e73 --- /dev/null +++ b/test/edns0_test.go @@ -0,0 +1,33 @@ +package test + +import ( + "testing" + + "github.com/miekg/dns" +) + +func TestEDNS0(t *testing.T) { + corefile := `.:0 { + whoami +} +` + + i, udp, _, err := CoreDNSServerAndPorts(corefile) + if err != nil { + t.Fatalf("Could not get CoreDNS serving instance: %s", err) + } + defer i.Stop() + + m := new(dns.Msg) + m.SetQuestion("example.org.", dns.TypeSOA) + m.SetEdns0(4096, true) + + resp, err := dns.Exchange(m, udp) + if err != nil { + t.Fatalf("Expected to receive reply, but didn't: %v", err) + } + opt := resp.Extra[len(resp.Extra)-1] + if opt.Header().Rrtype != dns.TypeOPT { + t.Errorf("Last RR must be OPT record") + } +}