From 84d7ddf8f6c8d32b36032400ffb2c120e753c48a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ersin=20KO=C3=87?= Date: Wed, 4 Nov 2026 13:13:50 +0200 Subject: [PATCH] fix(server): do not cache an ECS query's SERVFAIL under the global failure key A SERVFAIL learned while a client-subnet option was in flight is not portable to other clients (resolver.go already skips failure caching under ECS), but Handle wrote it to the global key, so unscoped clients got the cached SERVFAIL for the failure TTL. --- server/handler.go | 9 +- server/handler_ecs_servfail_scope_test.go | 148 ++++++++++++++++++++++ 2 files changed, 155 insertions(+), 2 deletions(-) create mode 100644 server/handler_ecs_servfail_scope_test.go diff --git a/server/handler.go b/server/handler.go index d46bc32..a0db8bd 100644 --- a/server/handler.go +++ b/server/handler.go @@ -1251,7 +1251,11 @@ func (h *MainHandler) Handle(query []byte, clientAddr net.Addr) (resp []byte, er // RFC 9520 §3: cache the resolution failure for a few seconds // so a retry loop on a broken name does not re-run the full // iterative chain on every QPS spike. TTL is capped at 30 s. - if !bypassCache { + // F55: a failure seen while a client-subnet (ECS) option was in + // flight is not portable to clients without that subnet (the + // resolver applies the same rule), so it must not be written to + // the global key. + if !bypassCache && outboundECS == nil { h.cache.StoreFailure(q.Name, q.Type, q.Class, cache.DefaultFailureTTL) } if msg.EDNS0 != nil { @@ -1314,9 +1318,10 @@ func (h *MainHandler) Handle(query []byte, clientAddr net.Addr) (resp []byte, er if ecsKey == "" { h.cache.StoreNegativeWithStatus(q.Name, q.Type, q.Class, cache.NegNoData, result.RCODE, result.Authority, result.DNSSECStatus) } - } else if result.RCODE == dns.RCodeServFail { + } else if result.RCODE == dns.RCodeServFail && outboundECS == nil { // RFC 9520 §3 failure caching for the resolver-returned-SERVFAIL // path (DNSSEC bogus, all NS unreachable, lame delegation, etc.). + // F55: skipped under ECS, see the err != nil path above. h.cache.StoreFailure(q.Name, q.Type, q.Class, cache.DefaultFailureTTL) } } diff --git a/server/handler_ecs_servfail_scope_test.go b/server/handler_ecs_servfail_scope_test.go new file mode 100644 index 0000000..944c02b --- /dev/null +++ b/server/handler_ecs_servfail_scope_test.go @@ -0,0 +1,148 @@ +package server + +import ( + "net" + "sync/atomic" + "testing" + "time" + + "github.com/labyrinthdns/labyrinth/cache" + "github.com/labyrinthdns/labyrinth/dns" + "github.com/labyrinthdns/labyrinth/metrics" + "github.com/labyrinthdns/labyrinth/resolver" +) + +// ecsFailUpstream answers SERVFAIL to queries carrying ECS and an A record to +// queries without it. +type ecsFailUpstream struct { + queries atomic.Int64 + port string +} + +func startECSFailUpstream(t *testing.T) *ecsFailUpstream { + t.Helper() + conn, err := net.ListenPacket("udp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + _, port, _ := net.SplitHostPort(conn.LocalAddr().String()) + s := &ecsFailUpstream{port: port} + go func() { + buf := make([]byte, 4096) + for { + n, addr, err := conn.ReadFrom(buf) + if err != nil { + return + } + s.queries.Add(1) + q, err := dns.Unpack(buf[:n]) + if err != nil || len(q.Questions) == 0 { + continue + } + hasECS := false + for i := range q.Additional { + if q.Additional[i].Type != dns.TypeOPT { + continue + } + if opt, perr := dns.ParseOPT(&q.Additional[i]); perr == nil && opt != nil { + if e, eerr := dns.ExtractECSFromOPT(opt); eerr == nil && e != nil && e.SourcePrefixLen > 0 { + hasECS = true + } + } + } + resp := &dns.Message{ + Header: dns.Header{ID: q.Header.ID, QDCount: uint16(len(q.Questions))}, + Questions: q.Questions, + } + failAll := len(q.Questions[0].Name) >= 4 && q.Questions[0].Name[:4] == "bad." + fb := dns.NewFlagBuilder().SetQR(true).SetRD(true).SetRA(true) + if hasECS || failAll { + resp.Header.Flags = fb.SetRCODE(dns.RCodeServFail).Build() + } else { + resp.Header.Flags = fb.Build() + resp.Header.ANCount = 1 + resp.Answers = []dns.ResourceRecord{{Name: q.Questions[0].Name, Type: dns.TypeA, Class: dns.ClassIN, TTL: 300, RDLength: 4, RData: []byte{93, 184, 216, 34}}} + } + if packed, perr := dns.Pack(resp, make([]byte, 4096)); perr == nil { + _, _ = conn.WriteTo(packed, addr) + } + } + }() + t.Cleanup(func() { _ = conn.Close() }) + return s +} + +func newECSFailHandler(t *testing.T) (*MainHandler, *ecsFailUpstream) { + up := startECSFailUpstream(t) + m := metrics.NewMetrics() + c := cache.NewCache(1000, 1, 86400, 3600, m) + logger := discardLogger() + res := resolver.NewResolver(c, resolver.ResolverConfig{ + MaxDepth: 5, MaxCNAMEDepth: 2, UpstreamTimeout: 500 * time.Millisecond, + UpstreamRetries: 1, UpstreamPort: up.port, ECSEnabled: true, + }, m, logger) + res.SetForwardTable(resolver.NewForwardTable([]resolver.ForwardZone{{Name: "forward.example.org", Addrs: []string{"127.0.0.1"}}})) + h := NewMainHandler(res, c, nil, nil, nil, m, logger) + h.SetECS(true, 24) + return h, up +} + +// TestHandle_ECS_ServfailNotCachedGlobally pins that a SERVFAIL learned while a +// client-subnet option was in flight is not written to the global failure +// cache (it is not portable across subnets, as resolver.go already assumes), +// while an ordinary client's failure is still cached (RFC 9520). +func TestHandle_ECS_ServfailNotCachedGlobally(t *testing.T) { + rcode := func(h *MainHandler, q []byte, addr string) uint8 { + t.Helper() + resp, err := h.Handle(q, &mockAddr{network: "udp", addr: addr}) + rc, rerr := ecsScopeRcodeOf(resp, err) + if rerr != nil { + t.Fatal(rerr) + } + return rc + } + + t.Run("control: unscoped client resolves", func(t *testing.T) { + h, _ := newECSFailHandler(t) + if rc := rcode(h, unscopedQuery(t, "ctl.forward.example.org", dns.TypeA), "198.51.100.9:1111"); rc != dns.RCodeNoError { + t.Fatalf("rcode=%d", rc) + } + }) + + t.Run("ECS SERVFAIL does not leak to unscoped clients", func(t *testing.T) { + h, _ := newECSFailHandler(t) + const qname = "geo.forward.example.org" + if rc := rcode(h, ecsScopedQuery(t, qname, dns.TypeA, "1.2.3.4"), "203.0.113.4:2222"); rc != dns.RCodeServFail { + t.Fatalf("precondition: ECS client rcode=%d, want SERVFAIL", rc) + } + if rc := rcode(h, unscopedQuery(t, qname, dns.TypeA), "198.51.100.77:3333"); rc != dns.RCodeNoError { + t.Fatalf("unscoped client got rcode=%d, want NOERROR", rc) + } + }) + + t.Run("repeated ECS SERVFAIL re-asks the upstream", func(t *testing.T) { + h, up := newECSFailHandler(t) + const qname = "geo2.forward.example.org" + rcode(h, ecsScopedQuery(t, qname, dns.TypeA, "1.2.3.4"), "203.0.113.4:2222") + before := up.queries.Load() + rcode(h, ecsScopedQuery(t, qname, dns.TypeA, "1.2.3.4"), "203.0.113.4:2222") + if up.queries.Load() == before { + t.Fatal("second ECS query was answered from a cached failure") + } + }) + + t.Run("unscoped failures are still cached (RFC 9520)", func(t *testing.T) { + h, up := newECSFailHandler(t) + const qname = "bad.forward.example.org" + if rc := rcode(h, unscopedQuery(t, qname, dns.TypeA), "198.51.100.1:1"); rc != dns.RCodeServFail { + t.Fatalf("rcode=%d, want SERVFAIL", rc) + } + before := up.queries.Load() + if rc := rcode(h, unscopedQuery(t, qname, dns.TypeA), "198.51.100.2:2"); rc != dns.RCodeServFail { + t.Fatalf("rcode=%d, want SERVFAIL", rc) + } + if up.queries.Load() != before { + t.Fatal("unscoped failure was not served from the failure cache") + } + }) +}