[caixingcheng]

No description provided.

[caixingcheng]

How to flush a name from the cache

Enable the control channel in dnscrypt-proxy.toml with an absolute socket path. It is a unix socket only, created with mode 0600, so only the proxy's own user can use it. The channel has no authentication, so there is no TCP option.

control_socket = '/run/dnscrypt/control.sock'

From the command line, against the proxy running with that configuration file:

dnscrypt-proxy -config dnscrypt-proxy.toml -flush example.com        # all common record types
dnscrypt-proxy -config dnscrypt-proxy.toml -flush example.com,AAAA   # one record type

The reply is printed. The exit code is 0 for ok N (N = cache entries removed) and 1 otherwise.

Over the socket directly, one line-based command per connection and one line of reply:

printf 'flush example.com\n'           | nc -U -q1 /run/dnscrypt/control.sock   # -> ok 2
printf 'flush_type example.com AAAA\n' | nc -U -q1 /run/dnscrypt/control.sock   # -> ok 1
printf 'purge example.com\n'           | nc -U -q1 /run/dnscrypt/control.sock   # -> error unknown command purge

Notes:

• flush removes the same record types as unbound-control flush: A, AAAA, NS, SOA, CNAME, DNAME, MX, PTR, SRV, NAPTR, SVCB, HTTPS and TXT.
• Every entry for the name is removed, fresh or stale, under all four DNSSEC/CD flag combinations that are part of the cache key. A request that was already in flight when the name was flushed cannot write its answer back into the cache.
• Only the exact name is flushed, not its subdomains. Case and a trailing dot do not matter.
• Limits: a command line is at most 512 bytes (error line too long otherwise), at most 4 connections are served at a time, and an idle connection is closed after 2s.
• On startup, a leftover socket is replaced only if nothing answers on it. If another instance still listens on that path, the control channel fails to start with ... is in use by another process.
• The intended caller is a health check that probes a fixed name through the proxy. Flushing that name first makes the probe go to the upstream, instead of being answered from the cache while the upstream is down.

Updated for bf26622: unix socket only, control_listen_address renamed to control_socket.

[MelvinTo]

Summary

This PR adds an optional, unauthenticated control channel that removes names from the cache, so health probes go to the upstream. Some parts are done right: the channel is off by default, the Unix socket is created with mode 0600, the 2s idle timeout works, and cache key, case, and DNSSEC/CD handling match the cache plugin. Verdict: please fix the issues below before merging. The line length is not actually limited and the number of connections is not capped. A request already in flight when the flush runs can write the stale answer back into the cache. The TCP listener can also bind to non-loopback addresses. Each finding below was reproduced in isolated tests against this head, with the race detector on.

Findings

1. High: no real line-length limit and no cap on connections (resource exhaustion), dnscrypt-proxy/control.go:93-98 (see also line 59). bufio.NewReaderSize(..., 512) only sets the buffer size. ReadString keeps growing the line. A single ~4 MiB line was still accepted and returned ok 1, with ~12.7 MiB of extra allocation for one request. With maxClients=1, 64 idle control connections could still be open at once, each with its own handler goroutine. The 2s timeout does not limit how many bytes arrive in that window or how many connections exist. Suggested fix: enforce a hard line limit (e.g. wrap the conn in io.LimitReader or use bufio.Scanner with a max token size) and cap concurrent control connections.

2. Medium: an in-flight request can re-cache a stale answer after a flush, so the health probe still hits the cache, dnscrypt-proxy/control.go:184-189. Reproduced with the real cache plugin and a failing mock upstream: an earlier request gets the stale fallback, the flush returns ok 1 and removes the entry, then the earlier request fails upstream and the response plugins write that fallback back as fresh. The next probe gets cacheHit=true for about 60s. This is a logic race, so the race detector does not catch it. Suggested fix: stop requests that started before the flush from re-populating that name (e.g. a per-name generation or epoch check), or give health probes an explicit cache-bypass path.

3. Medium: TCP is not limited to loopback, so the unauthenticated endpoint can bind to every interface, dnscrypt-proxy/control.go:65-67. listenControl("0.0.0.0:0") opens a wildcard listener and accepts unauthenticated flush commands. If the setting is a wildcard or non-loopback address and the port is reachable, outside clients can flush any name and use finding 1. The comment says to use loopback, but the code does not enforce it. Suggested fix: reject any TCP address that does not resolve to loopback.

4. Low: a live Unix socket is deleted as if it were stale, dnscrypt-proxy/control.go:71-79. The code only checks that the path is a socket, not whether another listener is still using it. In a repro, a second listen on the same path succeeded while the first listener was still running, and new connections went to the second one. Overlapping restarts or two instances with the same path can take over the control endpoint. Suggested fix: try to dial the existing socket first and only remove it if the connect fails (ECONNREFUSED). Otherwise return an error.

[caixingcheng]

@MelvinTo thanks for the review. All four findings are addressed in bf26622. Each fix has a test in control_test.go, and the whole package passes with -race.

1. No line-length limit / no connection cap. Fixed. The line is now read through io.LimitReader(conn, 513), so no more than 513 bytes are ever read. An over-long line gets error line too long and the connection is closed. A semaphore caps concurrent control connections at 4; a connection over the cap is closed right away. Tests: a 4 MiB line reads at most 513 bytes and is answered with the error, a 5th connection is closed while 4 idle ones hold the slots, and the slots are released afterwards.

2. An in-flight request re-caching a flushed name. Fixed, using the epoch approach you suggested. Every flush records the flush time of each key it touches; entries are kept for 1 minute. PluginCacheResponse skips the write when pluginsState.requestStart is earlier than that key's last flush. When nothing has been flushed, the hot path costs one atomic load.

I did not change the stale fallback so that it is never re-cached. That would change serve-stale for every name: during an outage each query would wait for the upstream timeout before getting its stale answer.

TestFlushFenceStopsInFlightRewrite reproduces your scenario with the real cache plugins: the stale fallback is taken before the flush and goes through the response plugins after it. The test fails without the fence and passes with it. A request that starts after the flush still caches as usual.

One addition to your analysis: the re-cached stale answer gets its TTL raised by cache_min_ttl. With the 2400 s we deploy, it would have lived 40 minutes, not ~60 s.

3. TCP not limited to loopback. Fixed by removing TCP altogether. The setting is now control_socket and must be an absolute path. 127.0.0.1:9054, 0.0.0.0:0, [::1]:9054 and relative paths are all rejected. We only use the unix socket, and dropping TCP removes the exposure entirely.

4. A live unix socket deleted as stale. Fixed. An existing socket is dialed first. If it connects, startup fails with ... is in use by another process and the socket is left alone. Only ECONNREFUSED (nothing listening) leads to removing it. A non-socket file is never removed.

I updated the usage comment above for the new setting name.

[MelvinTo]

Summary

This PR adds a cache-flush control socket so health checks can reach the upstream resolver. The hardening commit bf26622 fixes three of the four earlier findings (the resource-limit High, the TCP-exposure Medium, and the live-socket replacement Low). The in-flight cache-rewrite Medium is only partly fixed: a flush can still be undone by a response that passed the fence check just before it. Please fix that before merging. (Reviewed via GitHub reads; tests were inspected but not rerun.)

Findings

Medium: the flush fence check and cache insertion are not atomic

Previous finding #2 is still partly unresolved.

• dnscrypt-proxy/plugin_cache.go L155-L168
• dnscrypt-proxy/control.go L225-L226

writtenBeforeFlush() releases its mutex before the response is inserted, and the cache operations use a separate mutex, so this interleaving is still possible:

1. An older response passes the fence check at line 155.
2. The control handler records the fence, removes the entry, and returns ok.
3. The older response resumes and inserts its answer at line 168.

If that response carries the stale fallback, the next health probe gets a freshly cached answer despite the successful flush, which masks the upstream outage.

TestFlushFenceStopsInFlightRewrite finishes the flush before entering the response plugin, so it doesn't cover this window. Suggest doing the fence check and the insertion under the same synchronization as cache removal, and adding a test where a writer is paused between the check and the insertion.

[caixingcheng]

@MelvinTo you're right. The fence check released its own lock before the insert, so a flush could land between the two. This is fixed in e0cdc8b.

Fix. The fence check and the cache insert now share one sync.RWMutex with the flush:

• The cache response plugin holds the read lock while it checks the fence and inserts (flushGuard.insert).
• A flush holds the write lock while it records the fences and removes the entries (flushGuard.flush).

A flush therefore either runs before a store, and the store sees the fence and skips, or after it, and removes what was stored. It can no longer fall between the check and the insert. The previous fenceSet with its own mutex and atomic counter is gone. The hot path pays one RLock/RUnlock per cached response.

Test. TestFlushWaitsForInFlightInsert covers the window you described. A test hook pauses a writer after it has passed the check and before it inserts; the hook is nil outside tests. The test then:

1. starts a flush, and asserts that the flush stays blocked while the writer is paused;
2. releases the writer;
3. checks that the flush returns ok 1 and that no entry is left in the cache.

With the check reverted to unlock before the insert, the test fails with flush completed ("ok 0") while a writer was between its check and its insert. With the fix, the control tests pass 3 runs in a row under -race, and so does the whole package.

[MelvinTo]

Summary

The shared read/write lock fixes the previously reported gap between checking the flush fence and inserting a response. But the fence timestamp is now captured before the flush takes its lock, so a request that arrives while the flush is waiting can read stale data and cache it again afterwards. That leaves one more Medium flush race to fix.

Findings

Medium: A pre-lock timestamp lets waiting-period requests undo the flush

dnscrypt-proxy/control.go#L225, dnscrypt-proxy/control.go#L265-L279

The remaining interleaving:

1. An existing writer holds the guard's read lock.
2. A flush captures timestamp T0, then waits for the write lock.
3. Another DNS request starts at T1 > T0 and reads the old stale entry (cache reads don't take the guard).
4. The flush gets its lock, records T0, removes the entry, and returns ok.
5. The newer request's upstream fails. Its insertion passes because requestStart.Before(T0) is false, restoring the stale answer as fresh.

The new regression test covers an older writer, but not a request that reads stale data while the flush is waiting.

Fix: remove the entries under the write lock, then sample and record the fence timestamp before releasing that lock. Add a regression test that gets a stale fallback during the flush's lock wait and checks that its later insertion is rejected.

---

Reviewed at e0cdc8b6b3d7c676895d4e489546c9c54d01e6dc (GitHub reads only; tests not run).

[caixingcheng]

@MelvinTo agreed. The fence time was taken before the flush acquired the write lock, so a request that started during the wait could read the stale entry and later get past the fence. This is fixed in 6094ced.

Fix. flushGuard.flush now does all of its work under the write lock, in this order:

1. remove the entries;
2. take the fence time;
3. record the fence time.

Any request that could still have read a removed entry read it before the removal. That means its requestStart is at or before the fence time, so its store is rejected. The check is now !requestStart.After(flushed), so a request that started at exactly the fence time is rejected too. A request that starts after the fence time can no longer find the entry. flush() no longer takes a time from its caller.

Test. TestFlushFencesRequestsStartedWhileFlushWaits follows your interleaving:

1. Another store holds the read lock, paused by the test hook.
2. The flush starts and waits for the write lock.
3. A request starts during that wait and gets the stale fallback from the real cache plugin.
4. The store is released and the flush returns ok 1.
5. The request's stale fallback goes through PluginCacheResponse and must not be cached.

With the fence time moved back before the lock, the test fails with a request that read the entry while the flush waited wrote it back. With the fix, the control tests pass 3 runs in a row under -race, and so does the whole package.

The one remaining bound is flushFenceTTL (1 minute): a request that takes longer than that to reach the cache store would not be fenced. Requests are bounded by the upstream timeout (5 s in our config), so this does not occur in practice.