From 9a623cdeed037a0261f2d3c198c1633338905088 Mon Sep 17 00:00:00 2001 From: Sueun Cho Date: Tue, 18 Aug 2026 12:12:32 +0900 Subject: [PATCH] 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 * plugin/rewrite: apply fallback rcode rewrites for continue Signed-off-by: Sueun Cho --------- Signed-off-by: Sueun Cho --- plugin/rewrite/edns0_test.go | 11 +++++++++- plugin/rewrite/rcode.go | 4 ++++ plugin/rewrite/rcode_test.go | 41 ++++++++++++++++++++++++++++++++++++ plugin/rewrite/reverter.go | 28 ++++++++++++++++++++++++ plugin/rewrite/rewrite.go | 28 +++++++++++++----------- 5 files changed, 99 insertions(+), 13 deletions(-) diff --git a/plugin/rewrite/edns0_test.go b/plugin/rewrite/edns0_test.go index eb6681740..50419d2a9 100644 --- a/plugin/rewrite/edns0_test.go +++ b/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 } diff --git a/plugin/rewrite/rcode.go b/plugin/rewrite/rcode.go index d14e7789c..c3581064c 100644 --- a/plugin/rewrite/rcode.go +++ b/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 diff --git a/plugin/rewrite/rcode_test.go b/plugin/rewrite/rcode_test.go index 97d6459be..7abcc91e3 100644 --- a/plugin/rewrite/rcode_test.go +++ b/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") diff --git a/plugin/rewrite/reverter.go b/plugin/rewrite/reverter.go index 16811076c..a99a6b076 100644 --- a/plugin/rewrite/reverter.go +++ b/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-- { diff --git a/plugin/rewrite/rewrite.go b/plugin/rewrite/rewrite.go index 5320c5b5d..32e436651 100644 --- a/plugin/rewrite/rewrite.go +++ b/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) - 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 + return rw.serveWithResponseReverter(ctx, wr, r, state) } } } 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.