AdguardTeam / AdguardTeam/AdGuardHome

Optimistic caching serves records up to 12 hours past expiry, causing TLS errors against cloud load balancers

Aperta
#8,596 1 commento 0 reazioni 1 assegnatario Assegnata a @ainar-g Vedi su GitHub
bug Priority: P4
Lingua principale
TypeScript
Stelle
36.9k
Fork
2.5k
Metriche di merge delle PR
Nessuna PR unita negli ultimi 30g

Descrizione

### Prerequisites

- [x] I have checked the Wiki and Discussions and found no answer
- [x] I have searched other issues and found no duplicates
- [x] I want to report a bug and not ask a question or ask for help
- [x] I have set up AdGuard Home correctly and configured clients to use it

### Platform (OS and CPU architecture)

Linux, AMD64 (aka x86_64)

### Installation

Docker

### Setup

On one machine

### AdGuard Home version

v0.107.79

### Action

Enable optimistic caching. The only exposed control is the checkbox under Settings, DNS settings, DNS cache configuration. Leave `cache_optimistic_max_age` at its default.

```yaml
dns:
cache_optimistic: true
cache_ttl_max: 120
```

Query a name whose upstream address rotates, such as an AWS ALB or CloudFront endpoint, after the cache entry has sat unqueried for a few hours:

```sh
dig +noall +answer @$YOUR_AGH_ADDRESS api.example.com A
```

Deterministic version, no waiting, as an internal test in dnsproxy v0.83.2:

proxy/stale_window_test.go

```go
func TestOptimisticStaleWindow(t *testing.T) {
req := (&dns.Msg{}).SetQuestion("api.example.com.", dns.TypeA)
resp := (&dns.Msg{}).SetReply(req)
resp.Answer = []dns.RR{&dns.A{
Hdr: dns.RR_Header{
Name: "api.example.com.",
Rrtype: dns.TypeA,
Class: dns.ClassINET,
Ttl: 120,
},
A: net.IP{203, 0, 113, 10},
}}

for _, pastExpiry := range []time.Duration{
-time.Minute,
time.Second,
time.Hour,
11 * time.Hour,
12*time.Hour + time.Minute,
} {
c := &cache{
itemsLock: &sync.RWMutex{},
itemsWithSubnetLock: &sync.RWMutex{},
items: createCache(4 * 1024 * 1024),
optimistic: true,
// AdGuard Home defaults.
optimisticTTL: 30 * time.Second,
optimisticMaxAge: 12 * time.Hour,
}

packed := (&cacheItem{m: resp, ttl: 120}).pack()
stored := time.Now().Add(-pastExpiry).Unix()
binary.BigEndian.PutUint32(packed, uint32(stored))
c.items.Set(msgToKey(req), packed)

ci, expired, _ := c.get(req)
if ci == nil {
t.Logf("stale %s: entry dropped", pastExpiry)

continue
}

t.Logf(
"stale %s: served ip=%s expired=%v ttl=%d",
pastExpiry,
ci.m.Answer[0].(*dns.A).A,
expired,
ci.m.Answer[0].Header().Ttl,
)
}
}
```

### Expected result

Either a fresh answer, or a stale answer that is only slightly stale. An expired record should not outlive its TTL by hours.

### Actual result

The answer comes from cache with an address that expired up to 12 hours earlier, stamped with a 30 second TTL.

`unpackItem` returns an expired entry for as long as it is within `optimisticMaxAge` of expiry:

```go
if expired = now.After(expire); expired {
optimisticExpire := expire.Add(c.optimisticMaxAge)
if !c.optimistic || now.After(optimisticExpire) {
return nil, expired
}

ttl = uint32(c.optimisticTTL.Seconds())
}
```

`DefaultOptimisticMaxAge` is `12 * time.Hour` and `DefaultOptimisticAnswerTTL` is `30 * time.Second` (dnsproxy `proxy/config.go:25` and `:29`). AdGuard Home sets exactly those two values in `internal/home/config.go:503-504`.

Output of the test above:

```
stale -1m0s: served ip=203.0.113.10 expired=false ttl=60
stale 1s: served ip=203.0.113.10 expired=true ttl=30
stale 1h0m0s: served ip=203.0.113.10 expired=true ttl=30
stale 11h0m0s: served ip=203.0.113.10 expired=true ttl=30
stale 12h1m0s: entry dropped
```

There is no background refresh. `replyFromCache` hands the stale answer to the client first and only then starts the refresh goroutine, so the querying client always receives the stale address. Nothing walks the cache proactively, so an entry that goes unqueried for hours is served at whatever age it has reached.

Why this produces user-visible breakage: ALB, NLB, CloudFront and API Gateway publish 60 second TTLs precisely because their public addresses are recycled between tenants. An address 12 hours old usually belongs to a different customer's load balancer by then, and that load balancer terminates TLS with a different certificate. The result is a certificate mismatch, or an unrelated site where the other listener has a default rule. Route 53 failover and Aurora endpoint promotion fail the same way.

`cache_ttl_max` does not bound this. `setMinMaxTTL` clamps the record TTL before the response is cached, but the optimistic window is added after expiry and is flat regardless of TTL. Lowering `cache_ttl_max` expires entries sooner and therefore routes more queries through the optimistic path, not fewer.

### Additional information and/or screenshots

Suggested changes, in order of value:

1. Lower `DefaultOptimisticMaxAge`. Twelve hours predates the current prevalence of short-TTL cloud endpoints. A window on the order of a minute keeps the latency benefit and removes the failure mode.

2. Expose `cache_optimistic_max_age` and `cache_optimistic_answer_ttl` in the web interface and the HTTP API. Only the `cache_optimistic` boolean is in `openapi.yaml` today, so a user who ticks the checkbox has no way to see or bound the 12 hour window from the interface where they enabled it. The checkbox text, "Make AdGuard Home respond from the cache even when the entries are expired and also try to refresh them", gives no hint of a half-day window.

3. Reject `cache_optimistic_answer_ttl` below one second. `uint32(c.optimisticTTL.Seconds())` truncates a sub-second value to 0, and `filterRRSlice` treats 0 as "leave the TTL alone" (dnsproxy `proxy/cache.go:631`), so the stale answer goes out carrying the record's original TTL instead of a shortened one. Setting `500ms` produced a stale answer with TTL 60 rather than a short one.

Related: #8584 proposes reworking the refresh side of the same feature.

Workaround for anyone hitting this, since neither setting is reachable from the UI:

```yaml
dns:
cache_optimistic: true
cache_optimistic_max_age: 60s
cache_optimistic_answer_ttl: 5s
```

`0s` is treated as unset and falls back to the 12 hour default via `cmp.Or` in `proxy.go:248`, so it cannot be used to disable the window.

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.