plugin/tsig: don't echo client's TSIG.Error if verification is successful (#8215)

plugin/tsig/tsig.go

@@ -32,30 +32,31 @@ func (t TSIGServer) Name() string { return pluginName }
 
 // ServeDNS implements plugin.Handler
 func (t *TSIGServer) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
-	var err error
+	var (
-	state := request.Request{Req: r, W: w}
+		state  = request.Request{Req: r, W: w}
-	if z := plugin.Zones(t.Zones).Matches(state.Name()); z == "" {
+		tsigRR = r.IsTsig()
+	)
+	switch {
+	case tsigRR == nil && !t.tsigRequired(state.QType(), r.Opcode):
+		fallthrough
+	case plugin.Zones(t.Zones).Matches(state.Name()) == "":
 		return plugin.NextOrFailure(t.Name(), t.Next, ctx, w, r)
-	}
+	case tsigRR == nil:
-
-	var tsigRR = r.IsTsig()
-	rcode := dns.RcodeSuccess
-	if !t.tsigRequired(state.QType(), r.Opcode) && tsigRR == nil {
-		return plugin.NextOrFailure(t.Name(), t.Next, ctx, w, r)
-	}
-
-	if tsigRR == nil {
 		log.Debugf("rejecting '%s' request without TSIG\n", dns.TypeToString[state.QType()])
-		rcode = dns.RcodeRefused
+		resp := new(dns.Msg).SetRcode(r, dns.RcodeRefused)
+		w.WriteMsg(resp)
+		return dns.RcodeSuccess, nil
 	}
 
-	// wrap the response writer so the response will be TSIG signed.
+	// Strip the TSIG RR. Next, and subsequent plugins will not see the TSIG RRs.
+	// This violates forwarding cases (RFC 8945 5.5). See README.md Bugs
+	r.Extra = r.Extra[:len(r.Extra)-1]
+
+	// Wrap the response writer so the response will be TSIG signed.
 	w = &restoreTsigWriter{w, r, tsigRR}
 
-	tsigStatus := w.TsigStatus()
+	if tsigStatus := w.TsigStatus(); tsigStatus != nil {
-	if tsigStatus != nil {
 		log.Debugf("TSIG validation failed: %v %v", dns.TypeToString[state.QType()], tsigStatus)
-		rcode = dns.RcodeNotAuth
 		switch tsigStatus {
 		case dns.ErrSecret:
 			tsigRR.Error = dns.RcodeBadKey
@@ -64,26 +65,18 @@ func (t *TSIGServer) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.
 		default:
 			tsigRR.Error = dns.RcodeBadSig
 		}
-		resp := new(dns.Msg).SetRcode(r, rcode)
+		resp := new(dns.Msg).SetRcode(r, dns.RcodeNotAuth)
 		w.WriteMsg(resp)
 		return dns.RcodeSuccess, nil
 	}
 
-	// strip the TSIG RR. Next, and subsequent plugins will not see the TSIG RRs.
+	tsigRR.Error = dns.RcodeSuccess
-	// This violates forwarding cases (RFC 8945 5.5). See README.md Bugs
+	rcode, err := plugin.NextOrFailure(t.Name(), t.Next, ctx, w, r)
-	if len(r.Extra) > 1 {
+	if err != nil {
-		r.Extra = r.Extra[0 : len(r.Extra)-1]
+		log.Errorf("request handler returned an error: %v\n", err)
-	} else {
-		r.Extra = []dns.RR{}
 	}
-
+	// If the downstream plugin chain did not write, use custom ResponseWriter here
-	if rcode == dns.RcodeSuccess {
+	// because [dnsserver.errorFunc] ignores TSIG.
-		rcode, err = plugin.NextOrFailure(t.Name(), t.Next, ctx, w, r)
-		if err != nil {
-			log.Errorf("request handler returned an error: %v\n", err)
-		}
-	}
-	// If the plugin chain result was not an error, restore the TSIG and write the response.
 	if !plugin.ClientWrite(rcode) {
 		resp := new(dns.Msg).SetRcode(r, rcode)
 		w.WriteMsg(resp)
@@ -105,22 +98,21 @@ func (t *TSIGServer) tsigRequired(qtype uint16, opcode int) bool {
 	return typeMatches || opcodeMatches
 }
 
-// restoreTsigWriter Implement Response Writer, and adds a TSIG RR to a response
+// restoreTsigWriter implements [dns.ResponseWriter], and adds a [dns.TSIG] RR to a response.
 type restoreTsigWriter struct {
 	dns.ResponseWriter
-	req     *dns.Msg  // original request excluding TSIG if it has one
+	req     *dns.Msg  // original request excluding TSIG
 	reqTSIG *dns.TSIG // original TSIG
 }
 
 // WriteMsg adds a TSIG RR to the response
 func (r *restoreTsigWriter) WriteMsg(m *dns.Msg) error {
-	// Make sure the response has an EDNS OPT RR if the request had it.
+	if repTSIG := m.IsTsig(); repTSIG == nil { // respect TSIG set downstream
-	// Otherwise ScrubWriter would append it *after* TSIG, making it a non-compliant DNS message.
+		// Make sure the response has an EDNS OPT RR if the request had it.
-	state := request.Request{Req: r.req, W: r.ResponseWriter}
+		// Otherwise [request.ScrubWriter] would append it *after* TSIG, making it a non-compliant DNS message.
-	state.SizeAndDo(m)
+		state := request.Request{Req: r.req, W: r.ResponseWriter}
+		state.SizeAndDo(m)
 
-	repTSIG := m.IsTsig()
-	if r.reqTSIG != nil && repTSIG == nil {
 		repTSIG = new(dns.TSIG)
 		repTSIG.Hdr = dns.RR_Header{Name: r.reqTSIG.Hdr.Name, Rrtype: dns.TypeTSIG, Class: dns.ClassANY}
 		repTSIG.Algorithm = r.reqTSIG.Algorithm
@@ -140,7 +132,6 @@ func (r *restoreTsigWriter) WriteMsg(m *dns.Msg) error {
 		}
 		m.Extra = append(m.Extra, repTSIG)
 	}
-
 	return r.ResponseWriter.WriteMsg(m)
 }
 

plugin/tsig/tsig_test.go

@@ -3,6 +3,7 @@ package tsig
 import (
 	"context"
 	"fmt"
+	"slices"
 	"testing"
 	"time"
 
@@ -20,6 +21,7 @@ func TestServeDNS(t *testing.T) {
 		reqOpCodes  opCodes
 		qType       uint16
 		opcode      int
+		extra       []dns.RR
 		qTsig       bool
 		allTypes    bool
 		allOpcodes  bool
@@ -220,6 +222,7 @@ func TestServeDNSTsigErrors(t *testing.T) {
 	cases := []struct {
 		desc              string
 		tsigErr           error
+		reqError          int
 		expectRcode       int
 		expectError       int
 		expectOtherLength int
@@ -249,6 +252,15 @@ func TestServeDNSTsigErrors(t *testing.T) {
 			expectOtherLength: 6,
 			expectTimeSigned:  clientNow,
 		},
+		{
+			desc:              "Client Set Error",
+			tsigErr:           nil,
+			reqError:          dns.RcodeBadKey,
+			expectRcode:       dns.RcodeSuccess,
+			expectError:       dns.RcodeSuccess,
+			expectOtherLength: 0,
+			expectTimeSigned:  0,
+		},
 	}
 
 	tsig := TSIGServer{
@@ -267,8 +279,9 @@ func TestServeDNSTsigErrors(t *testing.T) {
 			r.SetQuestion("test.example.", dns.TypeA)
 			r.SetTsig("test.key.", dns.HmacSHA256, 300, clientNow)
 
-			// set a fake MAC and Size in request
 			rtsig := r.IsTsig()
+			rtsig.Error = uint16(tc.reqError)
+			// set a fake MAC and Size in request
 			rtsig.MAC = "0123456789012345678901234567890101234567890123456789012345678901"
 			rtsig.MACSize = 32
 
@@ -306,6 +319,101 @@ func TestServeDNSTsigErrors(t *testing.T) {
 	}
 }
 
+func TestServeDNSTsigNext(t *testing.T) {
+	cases := []struct {
+		desc         string
+		zones        []string
+		tsigRequired bool
+		tsigStatus   error
+		reqExtra     []dns.RR
+		reqSigned    bool
+		expectExtra  []uint16
+		expectNext   int
+	}{
+		{
+			desc:         "Optional TSIG",
+			zones:        []string{"."},
+			tsigRequired: false,
+			reqExtra:     []dns.RR{test.OPT(42, true)},
+			reqSigned:    false,
+			expectExtra:  []uint16{dns.TypeOPT},
+			expectNext:   1,
+		},
+		{
+			desc:         "Missing TSIG",
+			zones:        []string{"."},
+			tsigRequired: true,
+			tsigStatus:   nil,
+			reqExtra:     []dns.RR{test.OPT(42, true)},
+			reqSigned:    false,
+			expectNext:   0,
+		},
+		{
+			desc:        "Bad Zone",
+			zones:       []string{"another.domain."},
+			reqExtra:    []dns.RR{test.OPT(42, true)},
+			reqSigned:   true,
+			expectExtra: []uint16{dns.TypeOPT, dns.TypeTSIG},
+			expectNext:  1,
+		},
+		{
+			desc:         "Bad Status",
+			zones:        []string{"."},
+			tsigRequired: true,
+			tsigStatus:   dns.ErrSig,
+			reqExtra:     []dns.RR{test.OPT(42, true)},
+			reqSigned:    true,
+			expectNext:   0,
+		},
+		{
+			desc:         "Success",
+			zones:        []string{"."},
+			tsigRequired: true,
+			reqExtra:     []dns.RR{test.OPT(42, true)},
+			reqSigned:    true,
+			expectExtra:  []uint16{dns.TypeOPT},
+			expectNext:   1,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.desc, func(t *testing.T) {
+			h := testHandler()
+			var nextCalled int
+			tsig := TSIGServer{
+				Zones:    tc.zones,
+				allTypes: tc.tsigRequired,
+				Next: test.HandlerFunc(func(_ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+					nextCalled++
+					if !slices.EqualFunc(r.Extra, tc.expectExtra, func(rr dns.RR, t uint16) bool { return rr.Header().Rrtype == t }) {
+						t.Errorf("expected %v, got %v", tc.expectExtra, r.Extra)
+					}
+					return h(_ctx, w, r)
+				}),
+			}
+
+			ctx := context.TODO()
+
+			w := dnstest.NewRecorder(&ErrWriter{err: tc.tsigStatus})
+
+			r := new(dns.Msg)
+			r.SetQuestion("test.example.", dns.TypeA)
+			r.Extra = tc.reqExtra
+			if tc.reqSigned {
+				r.SetTsig("test.key.", dns.HmacSHA256, 300, time.Now().Unix())
+			}
+
+			_, err := tsig.ServeDNS(ctx, w, r)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if nextCalled != tc.expectNext {
+				t.Errorf("expected next plugin called")
+			}
+		})
+	}
+}
+
 func testHandler() test.HandlerFunc {
 	return func(_ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
 		state := request.Request{W: w, Req: r}