From cbef8b911cf9d3e1a92aaac41b23fb8fb6d9a441 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Tue, 21 Jul 2026 15:54:02 +0700 Subject: [PATCH] fix: partition DNS cache by EDNS Client Subnet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With cache_enable = true, one cache entry was shared by every client asking the same name against the same upstream: the cache key ({Qtype, Qclass, Name, Upstream}) and the osResolver hot-cache/singleflight key ("name:qtype:") both ignored the EDNS Client Subnet (ECS). A response tailored for subnet A was therefore served to subnet B. A cached answer's records are scoped to the network that generated them (RFC 7871 §7.3), so sharing them across subnets returns the wrong CDN/policy answer. Rewriting only the ECS option on the shared answer is worse: forwarders that validate the echoed ECS (e.g. dnsmasq with add-subnet) then accept the wrong-subnet answer instead of rejecting it as a mismatch. Partition both cache paths by a canonical ECS tuple (family, source-prefix, masked address) via the new dnscache.CanonicalECS: the LRU key gains an ECS field and the singleflight/hot-cache key appends the canonical ECS. Same subnet still shares an entry; different subnets (or address families) never do. Only a request with no ECS option collapses to the shared empty partition; a carried /0 keeps its own family-scoped token, since it is forwarded with an ECS option and must stay distinguishable from a no-ECS query (RFC 7871 §7.3.1). SetCacheReply no longer touches ECS and only reconciles the EDNS Cookie. Adds real cache-path regression tests (LRU and osResolver hot cache) that serve a different A record per subnet and verify the second subnet never receives the first's record. Fixes https://github.com/Control-D-Inc/ctrld/issues/324 --- dns.go | 6 ++ dns_test.go | 57 ++++++++++ internal/dnscache/cache.go | 61 ++++++++++- internal/dnscache/cache_test.go | 186 ++++++++++++++++++++++++++++++++ resolver.go | 8 +- resolver_test.go | 87 +++++++++++++++ 6 files changed, 402 insertions(+), 3 deletions(-) create mode 100644 dns_test.go create mode 100644 internal/dnscache/cache_test.go diff --git a/dns.go b/dns.go index f2b71a5..f89f824 100644 --- a/dns.go +++ b/dns.go @@ -14,6 +14,12 @@ func SetCacheReply(answer, msg *dns.Msg, code int) { // See https://datatracker.ietf.org/doc/html/rfc7873#section-4 sCookie.Cookie = cCookie.Cookie[:16] + sCookie.Cookie[16:] } + // NOTE: the answer's EDNS Client Subnet (ECS) is intentionally left as the + // upstream returned it. Correctness across clients is guaranteed by + // partitioning the cache and singleflight keys by ECS (see + // dnscache.CanonicalECS), so a cache hit only ever serves an answer that was + // resolved for the requester's own subnet. Rewriting the ECS option here + // without re-scoping the Answer records would violate RFC 7871 §7.3. } // getEdns0Cookie returns Edns0 cookie from *dns.OPT if present. diff --git a/dns_test.go b/dns_test.go new file mode 100644 index 0000000..9ca7608 --- /dev/null +++ b/dns_test.go @@ -0,0 +1,57 @@ +package ctrld + +import ( + "net" + "testing" + + "github.com/miekg/dns" +) + +// Test_SetCacheReply_DoesNotRewriteECS documents the post-#564 contract: cross-client +// correctness is guaranteed by partitioning the cache/singleflight keys by ECS +// (dnscache.CanonicalECS), NOT by rewriting the cached answer's ECS option. Rewriting the +// ECS metadata while leaving the Answer records scoped to another subnet would violate +// RFC 7871 §7.3 and make forwarders accept a wrong-subnet answer. SetCacheReply must +// therefore leave the answer's ECS untouched. +func Test_SetCacheReply_DoesNotRewriteECS(t *testing.T) { + answer := new(dns.Msg) + answer.SetQuestion(dns.Fqdn("controld.com"), dns.TypeA) + answer.SetEdns0(4096, false) + cachedSubnet := &dns.EDNS0_SUBNET{ + Code: dns.EDNS0SUBNET, + Family: 2, + SourceNetmask: 64, + SourceScope: 64, + Address: net.ParseIP("2001:db8:1::"), + } + answer.IsEdns0().Option = append(answer.IsEdns0().Option, cachedSubnet) + + req := new(dns.Msg) + req.SetQuestion(dns.Fqdn("controld.com"), dns.TypeA) + req.SetEdns0(4096, true) + req.IsEdns0().Option = append(req.IsEdns0().Option, &dns.EDNS0_SUBNET{ + Code: dns.EDNS0SUBNET, + Family: 2, + SourceNetmask: 64, + Address: net.ParseIP("2001:db8:2::"), + }) + + SetCacheReply(answer, req, dns.RcodeSuccess) + + var got *dns.EDNS0_SUBNET + for _, o := range answer.IsEdns0().Option { + if e, ok := o.(*dns.EDNS0_SUBNET); ok { + got = e + break + } + } + if got == nil { + t.Fatal("SetCacheReply dropped the answer's ECS option") + } + if want := net.ParseIP("2001:db8:1::"); !got.Address.Equal(want) { + t.Fatalf("SetCacheReply rewrote the answer ECS to the requester's subnet: got %v, want %v (unchanged)", got.Address, want) + } + if got.SourceScope != 64 { + t.Fatalf("SetCacheReply altered the answer ECS scope: got %d, want 64 (unchanged)", got.SourceScope) + } +} diff --git a/internal/dnscache/cache.go b/internal/dnscache/cache.go index af8883e..1437370 100644 --- a/internal/dnscache/cache.go +++ b/internal/dnscache/cache.go @@ -1,6 +1,8 @@ package dnscache import ( + "fmt" + "net" "strings" "time" @@ -16,11 +18,17 @@ type Cacher interface { } // Key is the caching key for DNS message. +// +// ECS partitions the cache by EDNS Client Subnet so an answer resolved for one +// subnet is never served to a client in a different subnet. Answer records are +// scoped to the network that generated them (RFC 7871 §7.3), so they must not +// be shared across subnets even when the question is otherwise identical. type Key struct { Qtype uint16 Qclass uint16 Name string Upstream string + ECS string } type Value struct { @@ -60,7 +68,58 @@ func NewLRUCache(size int) (*LRUCache, error) { // NewKey creates a new cache key for given DNS message. func NewKey(msg *dns.Msg, upstream string) Key { q := msg.Question[0] - return Key{Qtype: q.Qtype, Qclass: q.Qclass, Name: normalizeQname(q.Name), Upstream: upstream} + return Key{Qtype: q.Qtype, Qclass: q.Qclass, Name: normalizeQname(q.Name), Upstream: upstream, ECS: CanonicalECS(msg)} +} + +// CanonicalECS returns a canonical string form of the EDNS Client Subnet (ECS, +// EDNS option 8) carried by msg, suitable for partitioning cache and +// singleflight keys. A request with no ECS option returns "", so all ECS-less +// queries share one partition and behave as before. +// +// A request that DOES carry an ECS option is never mapped to "", even at SOURCE +// PREFIX-LENGTH 0: the /0 query is forwarded with an ECS option, may draw +// different upstream data than an ECS-less query, and its answer (with the +// echoed option) must not be served to a client that sent no ECS — RFC 7871 +// §7.3.1 requires /0-cached data to remain distinguishable. The family is part +// of the token, so an IPv4 /0 and an IPv6 /0 stay distinct too. +// +// The address is masked to its source prefix length so only the significant +// subnet bits contribute to the key: two clients in the same subnet share a +// partition, while different subnets (or address families) never do. The +// response-only SCOPE PREFIX-LENGTH is deliberately excluded — it is not part of +// what the client asked for. +func CanonicalECS(msg *dns.Msg) string { + opt := msg.IsEdns0() + if opt == nil { + return "" + } + for _, o := range opt.Option { + e, ok := o.(*dns.EDNS0_SUBNET) + if !ok { + continue + } + bits := int(e.SourceNetmask) + total := 128 + zero := net.IPv6zero + if e.Family == 1 { + total = 32 + zero = net.IPv4zero + } + addr := e.Address + if len(addr) == 0 { + // A /0 request commonly carries an empty address; normalize it to + // the family zero so its token is stable regardless of encoding. + addr = zero + } + masked := addr + if bits <= total { + if m := addr.Mask(net.CIDRMask(bits, total)); m != nil { + masked = m + } + } + return fmt.Sprintf("%d/%d/%s", e.Family, e.SourceNetmask, masked.String()) + } + return "" } // NewValue creates a new cache value for given DNS message. diff --git a/internal/dnscache/cache_test.go b/internal/dnscache/cache_test.go new file mode 100644 index 0000000..449ed1a --- /dev/null +++ b/internal/dnscache/cache_test.go @@ -0,0 +1,186 @@ +package dnscache + +import ( + "net" + "testing" + "time" + + "github.com/miekg/dns" +) + +// msgWithECS builds an A query for name carrying an EDNS Client Subnet option, or none +// when family == 0. +func msgWithECS(name string, family uint16, prefix uint8, addr string) *dns.Msg { + m := new(dns.Msg) + m.SetQuestion(dns.Fqdn(name), dns.TypeA) + if family == 0 { + return m + } + m.SetEdns0(4096, true) + m.IsEdns0().Option = append(m.IsEdns0().Option, &dns.EDNS0_SUBNET{ + Code: dns.EDNS0SUBNET, + Family: family, + SourceNetmask: prefix, + Address: net.ParseIP(addr), + }) + return m +} + +// answerWithA builds a cached answer holding a single A record with the given address. +func answerWithA(name, a string) *dns.Msg { + m := new(dns.Msg) + m.SetQuestion(dns.Fqdn(name), dns.TypeA) + rr, err := dns.NewRR(dns.Fqdn(name) + " 300 IN A " + a) + if err != nil { + panic(err) + } + m.Answer = []dns.RR{rr} + return m +} + +func firstA(msg *dns.Msg) string { + for _, rr := range msg.Answer { + if a, ok := rr.(*dns.A); ok { + return a.A.String() + } + } + return "" +} + +// TestCanonicalECS covers the key-partitioning helper: only a request with no ECS option +// collapses to the shared empty partition, while a carried /0 keeps its own family-scoped +// token (distinct from no-ECS, per RFC 7871 §7.3.1); host bits below the source prefix do +// not fragment the key, and different subnets / families produce different keys. +func TestCanonicalECS(t *testing.T) { + tests := []struct { + name string + msg *dns.Msg + want string + }{ + {"no ecs", msgWithECS("controld.com", 0, 0, ""), ""}, + // A carried ECS option is never "" (RFC 7871 §7.3.1), and IPv4 /0 vs + // IPv6 /0 stay distinct; the address encoding must not change the token. + {"ipv4 /0", msgWithECS("controld.com", 1, 0, "0.0.0.0"), "1/0/0.0.0.0"}, + {"ipv4 /0 empty addr", msgWithECS("controld.com", 1, 0, ""), "1/0/0.0.0.0"}, + {"ipv6 /0", msgWithECS("controld.com", 2, 0, "::"), "2/0/::"}, + {"ipv6 /0 empty addr", msgWithECS("controld.com", 2, 0, ""), "2/0/::"}, + {"ipv4 /24", msgWithECS("controld.com", 1, 24, "203.0.113.0"), "1/24/203.0.113.0"}, + {"ipv4 host bits masked", msgWithECS("controld.com", 1, 24, "203.0.113.7"), "1/24/203.0.113.0"}, + {"ipv6 /64", msgWithECS("controld.com", 2, 64, "2001:db8:1::"), "2/64/2001:db8:1::"}, + {"ipv6 host bits masked", msgWithECS("controld.com", 2, 64, "2001:db8:1::dead:beef"), "2/64/2001:db8:1::"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := CanonicalECS(tt.msg); got != tt.want { + t.Fatalf("CanonicalECS = %q, want %q", got, tt.want) + } + }) + } +} + +// TestNewKey_ECSPartition verifies the cache key distinguishes subnets while collapsing +// same-subnet requests, so the LRU cache cannot serve one subnet's records to another. +func TestNewKey_ECSPartition(t *testing.T) { + const up = "https://dns.example/dns-query" + subnetA := NewKey(msgWithECS("controld.com", 2, 64, "2001:db8:1::"), up) + subnetB := NewKey(msgWithECS("controld.com", 2, 64, "2001:db8:2::"), up) + subnetAHost := NewKey(msgWithECS("controld.com", 2, 64, "2001:db8:1::5"), up) + ipv4 := NewKey(msgWithECS("controld.com", 1, 24, "203.0.113.0"), up) + noECS := NewKey(msgWithECS("controld.com", 0, 0, ""), up) + + if subnetA == subnetB { + t.Fatal("different subnets must not share a cache key") + } + if subnetA != subnetAHost { + t.Fatal("same subnet (different host bits) must share a cache key") + } + if subnetA == ipv4 || subnetA == noECS || ipv4 == noECS { + t.Fatal("different families / no-ECS must not collide") + } +} + +// TestLRUCache_ECSZeroPrefixDistinctFromNoECS is the regression test for the /0-vs-no-ECS +// collision (RFC 7871 §7.3.1). A query carrying an ECS /0 option is forwarded WITH ECS and +// may draw different upstream data, so its cached answer must never be served to a client +// that sent no ECS at all, nor may IPv4 /0 and IPv6 /0 cross-serve each other. +func TestLRUCache_ECSZeroPrefixDistinctFromNoECS(t *testing.T) { + c, err := NewLRUCache(16) + if err != nil { + t.Fatalf("NewLRUCache: %v", err) + } + const up = "https://dns.example/dns-query" + expire := time.Now().Add(time.Minute) + + noECS := msgWithECS("controld.com", 0, 0, "") + zeroV4 := msgWithECS("controld.com", 1, 0, "0.0.0.0") + zeroV6 := msgWithECS("controld.com", 2, 0, "::") + + // Only the IPv4 /0 query's answer is cached. + c.Add(NewKey(zeroV4, up), NewValue(answerWithA("controld.com", "192.0.2.1"), expire)) + + // A no-ECS client must NOT receive the ECS /0 cached answer. + if got := c.Get(NewKey(noECS, up)); got != nil { + t.Fatalf("no-ECS client received the ECS /0 cached record %q", firstA(got.Msg)) + } + // An IPv6 /0 client must NOT receive the IPv4 /0 answer. + if got := c.Get(NewKey(zeroV6, up)); got != nil { + t.Fatalf("IPv6 /0 client received the IPv4 /0 cached record %q", firstA(got.Msg)) + } + // The IPv4 /0 client still hits its own entry. + if got := c.Get(NewKey(zeroV4, up)); got == nil || firstA(got.Msg) != "192.0.2.1" { + t.Fatalf("IPv4 /0 lost its own cached record: %v", got) + } + + // The no-ECS partition is independent and does not corrupt the /0 entry. + c.Add(NewKey(noECS, up), NewValue(answerWithA("controld.com", "198.51.100.1"), expire)) + if got := c.Get(NewKey(noECS, up)); got == nil || firstA(got.Msg) != "198.51.100.1" { + t.Fatalf("no-ECS partition wrong record: %v", got) + } + if got := c.Get(NewKey(zeroV4, up)); got == nil || firstA(got.Msg) != "192.0.2.1" { + t.Fatalf("IPv4 /0 record corrupted by no-ECS insert: %v", got) + } +} + +// TestLRUCache_ECSNoCrossSubnetServe is the real cache-path regression test for #564: +// an entry populated for subnet A must never be returned to a client in subnet B, even +// though the question (name/type/class/upstream) is identical. The two subnets carry +// different A records; the second client must get its own record or a miss, never A's. +func TestLRUCache_ECSNoCrossSubnetServe(t *testing.T) { + c, err := NewLRUCache(16) + if err != nil { + t.Fatalf("NewLRUCache: %v", err) + } + const up = "https://dns.example/dns-query" + expire := time.Now().Add(time.Minute) + + reqA := msgWithECS("controld.com", 2, 64, "2001:db8:1::") + reqB := msgWithECS("controld.com", 2, 64, "2001:db8:2::") + + // Only subnet A's answer (A record 192.0.2.1) is cached. + c.Add(NewKey(reqA, up), NewValue(answerWithA("controld.com", "192.0.2.1"), expire)) + + // A client in subnet B must NOT hit subnet A's entry. + if got := c.Get(NewKey(reqB, up)); got != nil { + t.Fatalf("subnet B received subnet A's cached record %q; cache is not ECS-partitioned", firstA(got.Msg)) + } + + // Subnet A still hits its own entry. + if got := c.Get(NewKey(reqA, up)); got == nil || firstA(got.Msg) != "192.0.2.1" { + t.Fatalf("subnet A lost its own cached record: %+v", got) + } + + // Now cache subnet B's distinct answer and confirm the two never cross. + c.Add(NewKey(reqB, up), NewValue(answerWithA("controld.com", "198.51.100.1"), expire)) + if got := c.Get(NewKey(reqB, up)); got == nil || firstA(got.Msg) != "198.51.100.1" { + t.Fatalf("subnet B got the wrong record: %v", got) + } + if got := c.Get(NewKey(reqA, up)); got == nil || firstA(got.Msg) != "192.0.2.1" { + t.Fatalf("subnet A record corrupted by subnet B insert: %v", got) + } + + // A second host within subnet A shares the partition (cache hit with A's record). + reqAHost := msgWithECS("controld.com", 2, 64, "2001:db8:1::9") + if got := c.Get(NewKey(reqAHost, up)); got == nil || firstA(got.Msg) != "192.0.2.1" { + t.Fatalf("same-subnet host missed the shared cache entry: %v", got) + } +} diff --git a/resolver.go b/resolver.go index dcd2b49..8cf06da 100644 --- a/resolver.go +++ b/resolver.go @@ -17,6 +17,8 @@ import ( "golang.org/x/sync/singleflight" "tailscale.com/net/netmon" "tailscale.com/net/tsaddr" + + "github.com/Control-D-Inc/ctrld/internal/dnscache" ) const ( @@ -360,8 +362,10 @@ func (o *osResolver) Resolve(ctx context.Context, msg *dns.Msg) (*dns.Msg, error domain := strings.TrimSuffix(msg.Question[0].Name, ".") qtype := msg.Question[0].Qtype - // Unique key for the singleflight group. - key := fmt.Sprintf("%s:%d:", domain, qtype) + // Unique key for the singleflight group. The EDNS Client Subnet is part of + // the key so subnet-specific answers are neither coalesced nor hot-cached + // across different subnets (RFC 7871 §7.3). + key := fmt.Sprintf("%s:%d:%s", domain, qtype, dnscache.CanonicalECS(msg)) logger := LoggerFromCtx(ctx) Log(ctx, logger.Debug(), "OS resolver query started: %s - %s", domain, dns.TypeToString[qtype]) diff --git a/resolver_test.go b/resolver_test.go index 85b2a2e..6c94d59 100644 --- a/resolver_test.go +++ b/resolver_test.go @@ -340,6 +340,93 @@ func Test_Edns0_CacheReply(t *testing.T) { } } +// ecsAnswerHandler returns a distinct A record per EDNS Client Subnet, so a test can +// prove one subnet never receives another subnet's cached record. It counts upstream +// calls to confirm the hot cache/singleflight is partitioned by ECS rather than shared. +func ecsAnswerHandler(call *atomic.Int64) dns.HandlerFunc { + return func(w dns.ResponseWriter, msg *dns.Msg) { + call.Add(1) + a := "203.0.113.1" // no/other subnet + if opt := msg.IsEdns0(); opt != nil { + for _, o := range opt.Option { + if e, ok := o.(*dns.EDNS0_SUBNET); ok { + switch { + case e.Address.Equal(net.ParseIP("2001:db8:1::")): + a = "192.0.2.1" + case e.Address.Equal(net.ParseIP("2001:db8:2::")): + a = "198.51.100.1" + } + } + } + } + m := new(dns.Msg) + m.SetReply(msg) + rr, _ := dns.NewRR(msg.Question[0].Name + " 300 IN A " + a) + m.Answer = []dns.RR{rr} + w.WriteMsg(m) + } +} + +// Test_osResolver_HotCache_ECSPartition is the real cache-path regression test for #564 on +// the osResolver hot cache / singleflight path: the upstream returns a different A record +// per subnet, and a client in subnet B must never be served subnet A's hot-cached record. +func Test_osResolver_HotCache_ECSPartition(t *testing.T) { + lanPC, err := net.ListenPacket("udp", "127.0.0.1:0") + if err != nil { + t.Fatalf("failed to listen on LAN address: %v", err) + } + call := &atomic.Int64{} + lanServer, lanAddr, err := runLocalPacketConnTestServer(t, lanPC, ecsAnswerHandler(call)) + if err != nil { + t.Fatalf("failed to run LAN test server: %v", err) + } + defer lanServer.Shutdown() + + or := newResolverWithNameserver([]string{lanAddr}) + query := func(subnet string) string { + m := new(dns.Msg) + m.SetQuestion(dns.Fqdn("controld.com"), dns.TypeA) + m.RecursionDesired = true + m.SetEdns0(4096, true) + m.IsEdns0().Option = append(m.IsEdns0().Option, &dns.EDNS0_SUBNET{ + Code: dns.EDNS0SUBNET, + Family: 2, + SourceNetmask: 64, + Address: net.ParseIP(subnet), + }) + answer, err := or.Resolve(context.Background(), m) + if err != nil { + t.Fatal(err) + } + for _, rr := range answer.Answer { + if a, ok := rr.(*dns.A); ok { + return a.A.String() + } + } + return "" + } + + // Subnet A populates the hot cache; a repeat hits it (upstream called once). + if got := query("2001:db8:1::"); got != "192.0.2.1" { + t.Fatalf("subnet A: got %q, want 192.0.2.1", got) + } + if got := query("2001:db8:1::"); got != "192.0.2.1" { + t.Fatalf("subnet A repeat: got %q, want 192.0.2.1", got) + } + if call.Load() != 1 { + t.Fatalf("subnet A repeat did not hit the hot cache: %d upstream calls", call.Load()) + } + + // Subnet B must get ITS OWN record, not subnet A's hot-cached one, and this + // requires a fresh upstream call (the cache is partitioned, not shared). + if got := query("2001:db8:2::"); got != "198.51.100.1" { + t.Fatalf("subnet B was served the wrong record %q (want 198.51.100.1); hot cache is not ECS-partitioned", got) + } + if call.Load() != 2 { + t.Fatalf("subnet B unexpectedly served from subnet A's cache: %d upstream calls, want 2", call.Load()) + } +} + // https://github.com/Control-D-Inc/ctrld/issues/255 func Test_legacyResolverWithBigExtraSection(t *testing.T) { lanPC, err := net.ListenPacket("udp", "127.0.0.1:0") // 127.0.0.1 is considered LAN (loopback)