plugin/rewrite: apply rcode rewrites to responses with no records (#8421)

* plugin/rewrite: apply rcode rewrites to record-less responses

An rcode rewrite rewrites the message-level RCODE, but the reverter only ran
response rules from inside the per-record loops in WriteMsg. When a response
carries no answer, authority or additional records - for example a bare
SERVFAIL that a downstream plugin returns to a non-EDNS client - none of the
loops iterate, so the rcode rewrite was silently skipped and the client
received the original RCODE.

Apply message-level response rules once when the response has no records, using
a small marker interface that mirrors the existing requestExtraRevertRule
pattern. This fixes the plugin's documented SERVFAIL-to-NOERROR use case for
responses without records.

Signed-off-by: Sueun Cho <sueun.dev@gmail.com>

* plugin/rewrite: apply fallback rcode rewrites for continue

Signed-off-by: Sueun Cho <sueun.dev@gmail.com>

---------

Signed-off-by: Sueun Cho <sueun.dev@gmail.com>

plugin/rewrite/edns0_test.go

@@ -26,9 +26,18 @@ func serveEdns0Rewrite(t *testing.T, rule Rule, next plugin.Handler, req *dns.Ms
 	rec := dnstest.NewRecorder(&test.ResponseWriter{})
 	// The server wraps the client writer in a ScrubWriter; reproduce that here.
 	sw := request.NewScrubWriter(req, rec)
-	if _, err := rw.ServeDNS(context.Background(), sw, req); err != nil {
+	rcode, err := rw.ServeDNS(context.Background(), sw, req)
+	if err != nil {
 		t.Fatal(err)
 	}
+	if !plugin.ClientWrite(rcode) {
+		state := request.Request{W: sw, Req: req}
+		resp := new(dns.Msg).SetRcode(req, rcode)
+		state.SizeAndDo(resp)
+		if err := sw.WriteMsg(resp); err != nil {
+			t.Fatal(err)
+		}
+	}
 	return rec.Msg
 }
 

plugin/rewrite/rcode.go

@@ -24,6 +24,10 @@ func (r *rcodeResponseRule) RewriteResponse(res *dns.Msg, _rr dns.RR) {
 	}
 }
 
+// rewriteMsg marks rcodeResponseRule as a message-level rule so it is applied
+// even when the response carries no resource records.
+func (r *rcodeResponseRule) rewriteMsg() {}
+
 type rcodeRuleBase struct {
 	nextAction string
 	response   rcodeResponseRule

plugin/rewrite/rcode_test.go

@@ -1,9 +1,11 @@
 package rewrite
 
 import (
+	"context"
 	"strings"
 	"testing"
 
+	"github.com/coredns/coredns/plugin"
 	"github.com/coredns/coredns/plugin/test"
 	"github.com/coredns/coredns/request"
 
@@ -72,6 +74,45 @@ func TestRCodeRewrite(t *testing.T) {
 	}
 }
 
+// TestRCodeRewriteEmptyResponse drives an rcode rewrite end-to-end through
+// ServeDNS for a response that carries no resource records, such as a bare
+// SERVFAIL from a downstream plugin. Rewriting SERVFAIL to NOERROR is the
+// plugin's documented use case (see README.md), and a non-EDNS client's failure
+// reply has no answer, authority or additional records, so the rewrite must be
+// applied at the message level rather than only per record.
+func TestRCodeRewriteEmptyResponse(t *testing.T) {
+	for _, mode := range []string{"stop", "continue"} {
+		for _, oldRcode := range []int{
+			dns.RcodeFormatError,
+			dns.RcodeServerFailure,
+			dns.RcodeRefused,
+			dns.RcodeNotImplemented,
+		} {
+			t.Run(mode+"/"+dns.RcodeToString[oldRcode], func(t *testing.T) {
+				rule, err := newRCodeRule(mode, "exact", "srv1.coredns.rocks", dns.RcodeToString[oldRcode], "NOERROR")
+				if err != nil {
+					t.Fatal(err)
+				}
+				// Downstream plugin fails without writing a response of its own.
+				next := plugin.HandlerFunc(func(_ context.Context, _ dns.ResponseWriter, _ *dns.Msg) (int, error) {
+					return oldRcode, nil
+				})
+				req := new(dns.Msg)
+				req.SetQuestion("srv1.coredns.rocks.", dns.TypeA) // no EDNS OPT record
+
+				resp := serveEdns0Rewrite(t, rule, next, req)
+				if resp == nil {
+					t.Fatal("no response was written to the client")
+				}
+				if resp.Rcode != dns.RcodeSuccess {
+					t.Fatalf("expected RCODE rewritten to NOERROR (%d), got %s (%d)",
+						dns.RcodeSuccess, dns.RcodeToString[resp.Rcode], resp.Rcode)
+				}
+			})
+		}
+	}
+}
+
 func TestNewRCodeRuleLargeRegex(t *testing.T) {
 	largeRegex := strings.Repeat("a", maxRegexpLen+1)
 	_, err := newRCodeRule("stop", "regex", largeRegex, "SERVFAIL", "NXDOMAIN")

plugin/rewrite/reverter.go

@@ -49,6 +49,14 @@ type requestExtraRevertRule interface {
 	revertRequestExtra()
 }
 
+// msgResponseRule is a ResponseRule that rewrites message-level fields, which
+// are independent of any resource record (for example the RCODE). Such a rule
+// must still be applied when the response carries no records.
+type msgResponseRule interface {
+	ResponseRule
+	rewriteMsg()
+}
+
 // ResponseRules describes an ordered list of response rules to apply
 // after a name rewrite
 type ResponseRules = []ResponseRule
@@ -96,6 +104,13 @@ func (r *ResponseReverter) WriteMsg(res1 *dns.Msg) error {
 		for _, rr := range res.Extra {
 			r.rewriteResourceRecord(res, rr)
 		}
+		// Message-level response rules (e.g. rcode) rewrite header fields that
+		// are independent of any resource record. The per-record loops above
+		// never run them when the response carries no records (e.g. a bare
+		// SERVFAIL from a downstream plugin), so apply them once here.
+		if len(res.Ns) == 0 && len(res.Answer) == 0 && len(res.Extra) == 0 {
+			r.rewriteMsg(res)
+		}
 	}
 	return r.writeWithRevertedRequestExtra(res)
 }
@@ -146,6 +161,19 @@ func (r *ResponseReverter) rewriteResourceRecord(res *dns.Msg, rr dns.RR) {
 	}
 }
 
+// rewriteMsg applies the message-level response rules once, in reversed order.
+// It is used for responses that carry no resource records, where the per-record
+// loops in WriteMsg would otherwise never apply them.
+func (r *ResponseReverter) rewriteMsg(res *dns.Msg) {
+	// The reverting rules need to be done in reversed order.
+	for i := len(r.ResponseRules) - 1; i >= 0; i-- {
+		if _, ok := r.ResponseRules[i].(msgResponseRule); !ok {
+			continue
+		}
+		r.ResponseRules[i].RewriteResponse(res, nil)
+	}
+}
+
 func (r *ResponseReverter) rewriteRequestExtra(req *dns.Msg, rr dns.RR) {
 	// The reverting rules need to be done in reversed order.
 	for i := len(r.ResponseRules) - 1; i >= 0; i-- {

plugin/rewrite/rewrite.go

@@ -57,24 +57,28 @@ func (rw Rewrite) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg
 				if !rw.DoRevert() {
 					return plugin.NextOrFailure(rw.Name(), rw.Next, ctx, w, r)
 				}
-				rcode, err := plugin.NextOrFailure(rw.Name(), rw.Next, ctx, wr, r)
+				return rw.serveWithResponseReverter(ctx, wr, r, state)
-				if plugin.ClientWrite(rcode) {
-					return rcode, err
-				}
-				// The next plugins didn't write a response, so write one now with the ResponseReverter.
-				// If server.ServeDNS does this then it will create an answer mismatch.
-				res := new(dns.Msg).SetRcode(r, rcode)
-				state.SizeAndDo(res)
-				wr.WriteMsg(res)
-				// return success, so server does not write a second error response to client
-				return dns.RcodeSuccess, err
 			}
 		}
 	}
 	if !rw.DoRevert() || len(wr.ResponseRules) == 0 {
 		return plugin.NextOrFailure(rw.Name(), rw.Next, ctx, w, r)
 	}
-	return plugin.NextOrFailure(rw.Name(), rw.Next, ctx, wr, r)
+	return rw.serveWithResponseReverter(ctx, wr, r, state)
+}
+
+func (rw Rewrite) serveWithResponseReverter(ctx context.Context, wr *ResponseReverter, r *dns.Msg, state request.Request) (int, error) {
+	rcode, err := plugin.NextOrFailure(rw.Name(), rw.Next, ctx, wr, r)
+	if plugin.ClientWrite(rcode) {
+		return rcode, err
+	}
+	// The next plugins didn't write a response, so write one now with the ResponseReverter.
+	// If server.ServeDNS does this then it will bypass response rewrite rules.
+	res := new(dns.Msg).SetRcode(r, rcode)
+	state.SizeAndDo(res)
+	wr.WriteMsg(res)
+	// Return success so server does not write a second error response to the client.
+	return dns.RcodeSuccess, err
 }
 
 // Name implements the Handler interface.