fix: partition DNS cache by EDNS Client Subnet

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
2026-07-29 01:18:48 +02:00 · 2026-07-23 19:17:41 +07:00
parent 06238e5b12
commit cbef8b911c
6 changed files with 402 additions and 3 deletions
@@ -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.
@@ -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)
+	}
+}
@@ -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.
@@ -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)
+	}
+}
@@ -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])
@@ -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)