fix(dns): expire cached responses with their answers

dns/cache.go

@@ -90,12 +90,38 @@ func (c *LruCache) Set(k string, v []byte, ttl int) {
 	}
 }
 
+// remove deletes a cached response without changing custom records.
+func (c *LruCache) remove(k string) {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+
+	it, ok := c.cache[k]
+	if !ok {
+		return
+	}
+	if it.prev != nil {
+		it.prev.next = it.next
+	} else {
+		c.head = it.next
+	}
+	if it.next != nil {
+		it.next.prev = it.prev
+	} else {
+		c.tail = it.prev
+	}
+	delete(c.cache, k)
+}
+
 // putToHead puts a new item to cache's head.
 func (c *LruCache) putToHead(k string, v []byte, exp int64) {
 	it := &item{key: k, val: v, exp: exp, prev: nil, next: c.head}
 	it.prev = nil
 	it.next = c.head
-	c.head.prev = it
+	if c.head != nil {
+		c.head.prev = it
+	} else {
+		c.tail = it
+	}
 	c.head = it
 
 	c.cache[k] = it
@@ -122,6 +148,10 @@ func (c *LruCache) moveToHead(it *item) {
 func (c *LruCache) removeTail() {
 	delete(c.cache, c.tail.key)
 
-	c.tail.prev.next = nil
 	c.tail = c.tail.prev
+	if c.tail != nil {
+		c.tail.next = nil
+	} else {
+		c.head = nil
+	}
 }

dns/client.go

@@ -125,8 +125,11 @@ func (c *Client) handleAnswer(respBytes []byte, clientAddr, dnsServer, network,
 		ttl = c.config.MinTTL
 	}
 
-	if ttl <= 0 { // we got a null result
-		ttl = 1800
+	if ttl <= 0 {
+		// A zero TTL must not become a permanent cache entry. Responses
+		// without answers have no usable cache lifetime either.
+		c.cache.remove(qKey(resp.Question))
+		return nil
 	}
 
 	c.cache.Set(qKey(resp.Question), valCopy(respBytes), ttl)
@@ -138,8 +141,13 @@ func (c *Client) handleAnswer(respBytes []byte, clientAddr, dnsServer, network,
 
 func (c *Client) extractAnswer(resp *Message) ([]string, int) {
 	var ips []string
-	ttl := c.config.MinTTL
-	for _, answer := range resp.Answers {
+	ttl := 0
+	for i, answer := range resp.Answers {
+		// The entire response is cached, so it must expire with its
+		// shortest-lived answer, including CNAMEs and zero TTLs.
+		if i == 0 || int(answer.TTL) < ttl {
+			ttl = int(answer.TTL)
+		}
 		if answer.TYPE == QTypeA || answer.TYPE == QTypeAAAA {
 			if answer.IP.IsValid() && !answer.IP.IsUnspecified() {
 				for _, h := range c.handlers {
@@ -149,9 +157,6 @@ func (c *Client) extractAnswer(resp *Message) ([]string, int) {
 				}
 				ips = append(ips, answer.IP.String())
 			}
-			if answer.TTL != 0 {
-				ttl = int(answer.TTL)
-			}
 		}
 	}
 

dns/client_test.go

@@ -0,0 +1,101 @@
+package dns
+
+import (
+	"bytes"
+	"net/netip"
+	"testing"
+	"time"
+)
+
+func TestHandleAnswerCacheTTL(t *testing.T) {
+	cname := func(ttl uint32) *RR {
+		var target bytes.Buffer
+		if _, err := MarshalDomainTo(&target, "target.example"); err != nil {
+			t.Fatal(err)
+		}
+		return &RR{NAME: "alias.example", TYPE: 5, CLASS: ClassINET, TTL: ttl, RDLENGTH: uint16(target.Len()), RDATA: target.Bytes()}
+	}
+	address := func(ttl uint32) *RR {
+		ip := netip.MustParseAddr("192.0.2.1")
+		return &RR{NAME: "target.example", TYPE: QTypeA, CLASS: ClassINET, TTL: ttl, RDLENGTH: 4, RDATA: ip.AsSlice()}
+	}
+	tests := []struct {
+		name                    string
+		answers                 []*RR
+		minTTL, maxTTL, wantTTL int
+	}{
+		{"CNAME only", []*RR{cname(30)}, 0, 1800, 30},
+		{"CNAME before longer address", []*RR{cname(10), address(300)}, 0, 1800, 10},
+		{"CNAME after longer address", []*RR{address(300), cname(10)}, 0, 1800, 10},
+		{"address expires first", []*RR{cname(300), address(10)}, 0, 1800, 10},
+		{"zero TTL CNAME", []*RR{cname(0), address(300)}, 0, 1800, 0},
+		{"zero TTL address", []*RR{cname(300), address(0)}, 0, 1800, 0},
+		{"no answers", nil, 0, 1800, 0},
+		{"minimum override", []*RR{cname(10)}, 60, 1800, 60},
+		{"maximum override", []*RR{cname(300)}, 0, 60, 60},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := &Client{config: &Config{MinTTL: tt.minTTL, MaxTTL: tt.maxTTL}, cache: NewLruCache(10)}
+			resp := NewMessage(1, ResponseMsg)
+			resp.SetQuestion(&Question{QNAME: "alias.example", QTYPE: QTypeA, QCLASS: ClassINET})
+			resp.Answers = tt.answers
+			wire, err := resp.Marshal()
+			if err != nil {
+				t.Fatal(err)
+			}
+			// A refresh without a cache lifetime must also discard a stale entry.
+			c.cache.Set(qKey(resp.Question), []byte("stale response"), 60)
+			before := time.Now().Unix()
+			if err := c.handleAnswer(wire, "fixture", "fixture", "udp", "DIRECT"); err != nil {
+				t.Fatal(err)
+			}
+			after := time.Now().Unix()
+			key := qKey(resp.Question)
+			entry, ok := c.cache.cache[key]
+			if tt.wantTTL == 0 {
+				if ok {
+					t.Fatalf("response with no positive TTL was cached until %d", entry.exp)
+				}
+				if _, ok := c.cache.store[key]; ok {
+					t.Fatal("response was cached permanently")
+				}
+				return
+			}
+			if !ok {
+				t.Fatal("response was not cached")
+			}
+			if entry.exp < before+int64(tt.wantTTL) || entry.exp > after+int64(tt.wantTTL) {
+				t.Fatalf("cache expiration = %d, want %d..%d", entry.exp, before+int64(tt.wantTTL), after+int64(tt.wantTTL))
+			}
+			got, expired := c.cache.Get(key)
+			if expired || !bytes.Equal(got, wire) {
+				t.Fatalf("cached response = %x, expired = %v", got, expired)
+			}
+		})
+	}
+}
+
+func TestUncacheableAnswersLeaveCacheUsable(t *testing.T) {
+	c := &Client{config: &Config{MaxTTL: 1800}, cache: NewLruCache(2)}
+	var responses [][]byte
+	for _, name := range []string{"first.example", "second.example"} {
+		resp := NewMessage(1, ResponseMsg)
+		resp.SetQuestion(&Question{QNAME: name, QTYPE: QTypeA, QCLASS: ClassINET})
+		wire, err := resp.Marshal()
+		if err != nil {
+			t.Fatal(err)
+		}
+		responses = append(responses, wire)
+		c.cache.Set(qKey(resp.Question), []byte("stale"), 60)
+	}
+	for _, wire := range responses {
+		if err := c.handleAnswer(wire, "fixture", "fixture", "udp", "DIRECT"); err != nil {
+			t.Fatal(err)
+		}
+	}
+	c.cache.Set("next", []byte("response"), 30)
+	if got, expired := c.cache.Get("next"); expired || string(got) != "response" {
+		t.Fatalf("cache after removing all responses: %q, expired = %v", got, expired)
+	}
+}