From 6268e6eafdf1d07e7d551b5bc47ccdd4b44d764a Mon Sep 17 00:00:00 2001 From: Ryan Malloy Date: Fri, 22 May 2026 09:24:12 -0600 Subject: [PATCH] =?UTF-8?q?Sign=20responses=20to=20TSIG-signed=20UPDATEs?= =?UTF-8?q?=20(RFC=208945=20=C2=A75.4.2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a request arrives with TSIG, attach a TSIG record to the response so dns.ResponseWriter computes the MAC at write time using the secret in TsigSecret. Without this, BIND nsupdate complains "expected a TSIG or SIG(0)" on every UPDATE, even when the update applies successfully. Two response paths fixed: - handleUpdate success/per-rcode replies (update.go) - ServeDNS rejection when TSIG verification fails (plugin.go) The new helper in tsig.go is a no-op for unsigned requests. Unknown keys still silently skip signing — we can't authenticate to a peer we don't share a key with. Tests verify both branches: signed request → response carries matching TSIG (key name + algorithm); unsigned request → response stays plain. --- plugin.go | 6 ++++++ tsig.go | 27 ++++++++++++++++++++++++ update.go | 1 + update_test.go | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 91 insertions(+) diff --git a/plugin.go b/plugin.go index 8891dfb..0794bfc 100644 --- a/plugin.go +++ b/plugin.go @@ -80,6 +80,12 @@ func (p *RFC2136) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg log.Warningf("UPDATE rejected: %v", err) resp := new(dns.Msg) resp.SetRcode(r, dns.RcodeRefused) + // Best-effort: if the request had TSIG and we recognize + // the key, the framework will sign the rejection so the + // client can authenticate "yes, server rejected this." + // Unknown keys silently skip signing — correct, since we + // can't prove identity to a peer we don't share a key with. + signResponseIfSigned(resp, r) _ = w.WriteMsg(resp) return dns.RcodeRefused, nil } diff --git a/tsig.go b/tsig.go index 5ca9882..e028e44 100644 --- a/tsig.go +++ b/tsig.go @@ -3,10 +3,37 @@ package rfc2136 import ( "fmt" "strings" + "time" "github.com/miekg/dns" ) +// tsigResponseFudge is the time tolerance (seconds) embedded in +// responses we sign. RFC 8945 §10 suggests 300s; we mirror that. +const tsigResponseFudge = 300 + +// signResponseIfSigned attaches a TSIG record to resp using the +// request's key name and algorithm. This causes the downstream +// dns.ResponseWriter to compute and serialize the MAC at WriteMsg +// time (using the secret from the server's TsigSecret map, which +// setup.go populated). Per RFC 8945 §5.4.2, the response to a +// TSIG-signed message MUST itself be signed if the server knows the +// key — otherwise the client cannot authenticate the answer and +// rejects it with "expected a TSIG or SIG(0)" (BIND nsupdate's exact +// complaint). +// +// If the request was not TSIG-signed, this is a no-op. If the key is +// not in the server's TsigSecret map (e.g. unknown key), miekg/dns +// will skip signing at write time and the response goes back +// unsigned — that's the correct shape for "I don't have your key." +func signResponseIfSigned(resp, req *dns.Msg) { + tsig := req.IsTsig() + if tsig == nil { + return + } + resp.SetTsig(tsig.Hdr.Name, tsig.Algorithm, tsigResponseFudge, time.Now().Unix()) +} + // checkTSIG verifies that the incoming UPDATE message is properly signed // with a TSIG key we know about. The actual signature math has already // been done by the underlying dns.Server (because setup.go registered diff --git a/update.go b/update.go index 7c6917e..8c9e3bd 100644 --- a/update.go +++ b/update.go @@ -29,6 +29,7 @@ import ( func (p *RFC2136) handleUpdate(w dns.ResponseWriter, r *dns.Msg) (int, error) { resp := new(dns.Msg) resp.SetReply(r) + signResponseIfSigned(resp, r) // 1. Validate the Zone section. if len(r.Question) != 1 { diff --git a/update_test.go b/update_test.go index d340a61..b6e5be9 100644 --- a/update_test.go +++ b/update_test.go @@ -260,6 +260,63 @@ func TestUpdate_NoOpAdd_DoesntRewriteFile(t *testing.T) { } } +// TestUpdate_SignedRequest_ResponseGetsTSIG verifies the RFC 8945 §5.4.2 +// requirement that a server's response to a TSIG-signed request must +// itself carry TSIG so the client can authenticate the answer. Before +// this fix, BIND nsupdate complained "expected a TSIG or SIG(0)" because +// the response went back without any TSIG record attached. +func TestUpdate_SignedRequest_ResponseGetsTSIG(t *testing.T) { + p := newTestPluginWithZone(t, "auth.example.com") + + upd := newUpdate("auth.example.com", + mustRR(t, `foo.auth.example.com. 60 IN TXT "x"`), + ) + // Attach a TSIG to the request — the values mimic what nsupdate + // or caddy-dns/rfc2136 emit. handleUpdate runs WITHOUT MAC + // verification in this test (that's ServeDNS's job and is + // covered separately), so the TSIG record's MAC contents don't + // matter — only the presence/algorithm/name do. + upd.SetTsig("acme-update-key.", dns.HmacSHA256, 300, 0) + + w := &captureWriter{} + if _, err := p.handleUpdate(w, upd); err != nil { + t.Fatalf("handleUpdate: %v", err) + } + if w.msg == nil { + t.Fatal("response was not written") + } + got := w.msg.IsTsig() + if got == nil { + t.Fatal("response missing TSIG — nsupdate would reject as 'expected a TSIG or SIG(0)'") + } + if !strings.EqualFold(got.Hdr.Name, "acme-update-key.") { + t.Errorf("response TSIG key = %q, want acme-update-key.", got.Hdr.Name) + } + if !strings.EqualFold(got.Algorithm, dns.HmacSHA256) { + t.Errorf("response TSIG alg = %q, want %s", got.Algorithm, dns.HmacSHA256) + } +} + +// TestUpdate_UnsignedRequest_ResponseStaysUnsigned guards the no-op +// branch — if the client didn't sign, we don't synthesize a TSIG on +// the response (we couldn't pick a key anyway). +func TestUpdate_UnsignedRequest_ResponseStaysUnsigned(t *testing.T) { + p := newTestPluginWithZone(t, "auth.example.com") + + upd := newUpdate("auth.example.com", + mustRR(t, `foo.auth.example.com. 60 IN TXT "x"`), + ) + // No SetTsig — request is plain. + + w := &captureWriter{} + if _, err := p.handleUpdate(w, upd); err != nil { + t.Fatalf("handleUpdate: %v", err) + } + if w.msg.IsTsig() != nil { + t.Error("response carries TSIG despite unsigned request") + } +} + func TestFindZone_LongestSuffixWins(t *testing.T) { p := &RFC2136{Zones: []string{"example.com.", "auth.example.com."}} if got := p.findZone("foo.auth.example.com."); got != "auth.example.com." {