diff --git a/plugin/pkg/edns/edns.go b/plugin/pkg/edns/edns.go index cd8639915..46047679f 100644 --- a/plugin/pkg/edns/edns.go +++ b/plugin/pkg/edns/edns.go @@ -15,18 +15,15 @@ type supported struct { sync.RWMutex } -// SetSupportedOption adds a new supported option the set of EDNS0 options that we support. Plugins typically call -// this in their setup code to signal support for a new option. -// By default we support: -// dns.EDNS0NSID, dns.EDNS0EXPIRE, dns.EDNS0COOKIE, dns.EDNS0TCPKEEPALIVE, dns.EDNS0PADDING. These -// values are not in this map and checked directly in the server. +// SetSupportedOption adds an EDNS0 option to the set of options that CoreDNS may copy from a request to an +// OPT-less response. Plugins typically call this in their setup code to signal support for a custom option. func SetSupportedOption(option uint16) { sup.Lock() sup.m[option] = struct{}{} sup.Unlock() } -// SupportedOption returns true if the option code is supported as an extra EDNS0 option. +// SupportedOption returns true if the option code was explicitly registered. func SupportedOption(option uint16) bool { sup.RLock() _, ok := sup.m[option] diff --git a/plugin/rewrite/rewrite_test.go b/plugin/rewrite/rewrite_test.go index b24801132..528f18c3c 100644 --- a/plugin/rewrite/rewrite_test.go +++ b/plugin/rewrite/rewrite_test.go @@ -1193,16 +1193,12 @@ func TestRewriteEDNS0RevertDoesNotLeakThroughScrubWriter(t *testing.T) { if o == nil { t.Fatal("expected EDNS0 option record in response") } - var foundCookie bool for _, opt := range o.Option { if opt.Option() == 0xffee { t.Fatalf("expected rewritten EDNS0 option to be reverted, got %v", o.Option) } if opt.Option() == dns.EDNS0COOKIE { - foundCookie = true + t.Fatalf("expected request EDNS0 cookie option not to be copied to the response, got %v", o.Option) } } - if !foundCookie { - t.Fatalf("expected original EDNS0 cookie option to be preserved, got %v", o.Option) - } } diff --git a/request/edns0.go b/request/edns0.go index 89eb6b468..8fbf6b06c 100644 --- a/request/edns0.go +++ b/request/edns0.go @@ -7,24 +7,10 @@ import ( ) func supportedOptions(o []dns.EDNS0) []dns.EDNS0 { - var supported = make([]dns.EDNS0, 0, 3) - // For as long as possible try avoid looking up in the map, because that need an Rlock. + supported := make([]dns.EDNS0, 0, 3) 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: + if edns.SupportedOption(opt.Option()) { supported = append(supported, opt) - default: - if edns.SupportedOption(code) { - supported = append(supported, opt) - } } } return supported diff --git a/request/edns0_test.go b/request/edns0_test.go index 78addd026..031485cce 100644 --- a/request/edns0_test.go +++ b/request/edns0_test.go @@ -3,48 +3,31 @@ package request import ( "testing" + "github.com/coredns/coredns/plugin/pkg/edns" + "github.com/miekg/dns" ) func TestSupportedOptions(t *testing.T) { - tests := []struct { - name string - options []dns.EDNS0 - expected int - }{ - { - name: "empty options", - options: []dns.EDNS0{}, - expected: 0, - }, - { - name: "all supported options", - options: []dns.EDNS0{ - &dns.EDNS0_NSID{}, - &dns.EDNS0_EXPIRE{}, - &dns.EDNS0_COOKIE{}, - &dns.EDNS0_TCP_KEEPALIVE{}, - &dns.EDNS0_PADDING{}, - }, - expected: 5, - }, - { - name: "mixed supported and unsupported options", - options: []dns.EDNS0{ - &dns.EDNS0_NSID{}, - &dns.EDNS0_LOCAL{Code: 65001}, // unsupported code - &dns.EDNS0_PADDING{}, - }, - expected: 2, - }, + const supportedCode = 65001 + edns.SetSupportedOption(supportedCode) + + want := &dns.EDNS0_LOCAL{Code: supportedCode} + options := []dns.EDNS0{ + &dns.EDNS0_NSID{}, + &dns.EDNS0_EXPIRE{}, + &dns.EDNS0_COOKIE{}, + &dns.EDNS0_TCP_KEEPALIVE{}, + &dns.EDNS0_PADDING{}, + &dns.EDNS0_LOCAL{Code: supportedCode + 1}, + want, } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - result := supportedOptions(tc.options) - if len(result) != tc.expected { - t.Errorf("Expected %d supported options, got %d", tc.expected, len(result)) - } - }) + got := supportedOptions(options) + if len(got) != 1 { + t.Fatalf("Expected one explicitly supported option, got %d: %v", len(got), got) + } + if got[0] != want { + t.Errorf("Expected explicitly supported option %v, got %v", want, got[0]) } } diff --git a/request/request_test.go b/request/request_test.go index ae7410abf..7c1ffa097 100644 --- a/request/request_test.go +++ b/request/request_test.go @@ -107,6 +107,30 @@ func TestRequestSizeAndDo(t *testing.T) { } } +func TestRequestSizeAndDoDoesNotEchoEDNSOptions(t *testing.T) { + st := testRequest() + requestOPT := st.Req.IsEdns0() + requestOPT.Option = []dns.EDNS0{ + &dns.EDNS0_NSID{Code: dns.EDNS0NSID, Nsid: "request-nsid"}, + &dns.EDNS0_EXPIRE{Code: dns.EDNS0EXPIRE, Expire: 60}, + &dns.EDNS0_COOKIE{Code: dns.EDNS0COOKIE, Cookie: "abcdef0123456789"}, + &dns.EDNS0_TCP_KEEPALIVE{Code: dns.EDNS0TCPKEEPALIVE, Timeout: 10}, + &dns.EDNS0_PADDING{Padding: []byte{0, 0, 0, 0}}, + } + + response := new(dns.Msg) + if !st.SizeAndDo(response) { + t.Fatal("Expected SizeAndDo to add an OPT record") + } + responseOPT := response.IsEdns0() + if responseOPT == nil { + t.Fatal("Expected response to contain an OPT record") + } + if len(responseOPT.Option) != 0 { + t.Errorf("Expected request EDNS options to be ignored, got %v", responseOPT.Option) + } +} + // TestRequestNewWithQuestion tests the NewWithQuestion method func TestRequestNewWithQuestion(t *testing.T) { st := testRequest() diff --git a/test/cache_test.go b/test/cache_test.go index 958e8270d..8cf27bbd2 100644 --- a/test/cache_test.go +++ b/test/cache_test.go @@ -58,6 +58,10 @@ func TestLookupCache(t *testing.T) { t.Run("DNSSEC OPT", func(t *testing.T) { testCaseDNSSEC(t, "example.org.", udp, 0) }) + + t.Run("EDNS request options", func(t *testing.T) { + testCaseEDNSOptionsNotEchoed(t, "example.org.", udp) + }) } func testCase(t *testing.T, name, addr string, expectAnsLen int, expectTTL uint32) { @@ -118,6 +122,29 @@ func testCaseDNSSEC(t *testing.T, name, addr string, bufsize int) { } } +func testCaseEDNSOptionsNotEchoed(t *testing.T, name, addr string) { + t.Helper() + m := new(dns.Msg) + m.SetQuestion(name, dns.TypeA) + m.SetEdns0(4096, false) + m.IsEdns0().Option = []dns.EDNS0{ + &dns.EDNS0_NSID{Code: dns.EDNS0NSID}, + &dns.EDNS0_COOKIE{Code: dns.EDNS0COOKIE, Cookie: "abcdef0123456789"}, + } + + resp, err := dns.Exchange(m, addr) + if err != nil { + t.Fatalf("Expected to receive reply, but didn't: %s", err) + } + opt := resp.IsEdns0() + if opt == nil { + t.Fatal("Expected OPT RR in response") + } + if len(opt.Option) != 0 { + t.Fatalf("Expected request EDNS options not to be echoed, got %v", opt.Option) + } +} + func TestLookupCacheWithoutEdns(t *testing.T) { name, rm, err := test.TempFile(".", exampleOrg) if err != nil {