From d89f1e3d4bfcb2de7c3988c619d27bb6f5fac706 Mon Sep 17 00:00:00 2001
From: chantra <chantra@users.noreply.github.com>
Date: Mon, 17 Jun 2019 08:13:02 -0700
Subject: [PATCH] Reply with NOTIMPL when Opcode is not supported (#982)

One of the test from DNS Compliance testing validates that if the opcode
is not supported, a NOTIMPL rcode is returned.

https://gitlab.isc.org/isc-projects/DNS-Compliance-Testing/blob/e0884144dde6704128e22acc14e2b65ba41a92b2/genreport.c#L293

This diff makes the default acceptfunc support this case and reply with
NOTIMPL instead of FORMERR.
---
 acceptfunc.go  |  9 +++++----
 server.go      | 12 +++++++++---
 server_test.go | 29 +++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/acceptfunc.go b/acceptfunc.go
index 94e7c9d9..c6920f79 100644
--- a/acceptfunc.go
+++ b/acceptfunc.go
@@ -19,9 +19,10 @@ var DefaultMsgAcceptFunc MsgAcceptFunc = defaultMsgAcceptFunc
 type MsgAcceptAction int
 
 const (
-	MsgAccept MsgAcceptAction = iota // Accept the message
-	MsgReject                        // Reject the message with a RcodeFormatError
-	MsgIgnore                        // Ignore the error and send nothing back.
+	MsgAccept         MsgAcceptAction = iota // Accept the message
+	MsgReject                                // Reject the message with a RcodeFormatError
+	MsgIgnore                                // Ignore the error and send nothing back.
+	MsgRejectNotImplemented                        // Reject the message with a RcodeNotImplemented
 )
 
 func defaultMsgAcceptFunc(dh Header) MsgAcceptAction {
@@ -32,7 +33,7 @@ func defaultMsgAcceptFunc(dh Header) MsgAcceptAction {
 	// Don't allow dynamic updates, because then the sections can contain a whole bunch of RRs.
 	opcode := int(dh.Bits>>11) & 0xF
 	if opcode != OpcodeQuery && opcode != OpcodeNotify {
-		return MsgReject
+		return MsgRejectNotImplemented
 	}
 
 	if dh.Qdcount != 1 {
diff --git a/server.go b/server.go
index 354f7712..3cf1a024 100644
--- a/server.go
+++ b/server.go
@@ -560,18 +560,24 @@ func (srv *Server) serveDNS(m []byte, w *response) {
 	req := new(Msg)
 	req.setHdr(dh)
 
-	switch srv.MsgAcceptFunc(dh) {
+	switch action := srv.MsgAcceptFunc(dh); action {
 	case MsgAccept:
 		if req.unpack(dh, m, off) == nil {
 			break
 		}
 
 		fallthrough
-	case MsgReject:
+	case MsgReject, MsgRejectNotImplemented:
+		opcode := req.Opcode
 		req.SetRcodeFormatError(req)
+		req.Zero = false
+		if action == MsgRejectNotImplemented {
+			req.Opcode = opcode
+			req.Rcode = RcodeNotImplemented
+		}
+
 		// Are we allowed to delete any OPT records here?
 		req.Ns, req.Answer, req.Extra = nil, nil, nil
-		req.Zero = false
 
 		w.WriteMsg(req)
 		fallthrough
diff --git a/server_test.go b/server_test.go
index 26ffff61..0c08e9b4 100644
--- a/server_test.go
+++ b/server_test.go
@@ -215,6 +215,35 @@ func TestServeIgnoresZFlag(t *testing.T) {
 	}
 }
 
+// Verify that the server responds to a query with unsupported Opcode with a NotImplemented error and that Opcode is unchanged.
+func TestServeNotImplemented(t *testing.T) {
+	HandleFunc("example.com.", AnotherHelloServer)
+	opcode := 15
+
+	s, addrstr, err := RunLocalUDPServer(":0")
+	if err != nil {
+		t.Fatalf("unable to run test server: %v", err)
+	}
+	defer s.Shutdown()
+
+	c := new(Client)
+	m := new(Msg)
+
+	// Test that Opcode is like the unchanged from request Opcode and that Rcode is set to NotImplemnented
+	m.SetQuestion("example.com.", TypeTXT)
+	m.Opcode = opcode
+	r, _, err := c.Exchange(m, addrstr)
+	if err != nil {
+		t.Fatal("failed to exchange example.com with +zflag", err)
+	}
+	if r.Opcode != opcode {
+		t.Errorf("expected opcode %v, got %v", opcode, r.Opcode)
+	}
+	if r.Rcode != RcodeNotImplemented {
+		t.Errorf("expected rcode %v, got %v", RcodeNotImplemented, r.Rcode)
+	}
+}
+
 func TestServingTLS(t *testing.T) {
 	HandleFunc("miek.nl.", HelloServer)
 	HandleFunc("example.com.", AnotherHelloServer)