request: stop echoing unhandled EDNS options (#8514)

Signed-off-by: houyuwushang <liuluoqianqiu@outlook.com>

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
+// SetSupportedOption adds an EDNS0 option to the set of options that CoreDNS may copy from a request to an
-// this in their setup code to signal support for a new option.
+// OPT-less response. Plugins typically call this in their setup code to signal support for a custom 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.
 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]

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)
-	}
 }

request/edns0.go

@@ -7,24 +7,10 @@ import (
 )
 
 func supportedOptions(o []dns.EDNS0) []dns.EDNS0 {
-	var supported = make([]dns.EDNS0, 0, 3)
+	supported := make([]dns.EDNS0, 0, 3)
-	// For as long as possible try avoid looking up in the map, because that need an Rlock.
 	for _, opt := range o {
-		switch code := opt.Option(); code {
+		if edns.SupportedOption(opt.Option()) {
-		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.SupportedOption(code) {
-				supported = append(supported, opt)
-			}
 		}
 	}
 	return supported

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 {
+	const supportedCode = 65001
-		name     string
+	edns.SetSupportedOption(supportedCode)
-		options  []dns.EDNS0
+
-		expected int
+	want := &dns.EDNS0_LOCAL{Code: supportedCode}
-	}{
+	options := []dns.EDNS0{
-		{
+		&dns.EDNS0_NSID{},
-			name:     "empty options",
+		&dns.EDNS0_EXPIRE{},
-			options:  []dns.EDNS0{},
+		&dns.EDNS0_COOKIE{},
-			expected: 0,
+		&dns.EDNS0_TCP_KEEPALIVE{},
-		},
+		&dns.EDNS0_PADDING{},
-		{
+		&dns.EDNS0_LOCAL{Code: supportedCode + 1},
-			name: "all supported options",
+		want,
-			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,
-		},
 	}
 
-	for _, tc := range tests {
+	got := supportedOptions(options)
-		t.Run(tc.name, func(t *testing.T) {
+	if len(got) != 1 {
-			result := supportedOptions(tc.options)
+		t.Fatalf("Expected one explicitly supported option, got %d: %v", len(got), got)
-			if len(result) != tc.expected {
+	}
-				t.Errorf("Expected %d supported options, got %d", tc.expected, len(result))
+	if got[0] != want {
-			}
+		t.Errorf("Expected explicitly supported option %v, got %v", want, got[0])
-		})
 	}
 }

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()

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 {