daemon: stop the KX rate limiter wedging key exchange permanently - #436
Merged
Conversation
allowKxFromSource tracked one bucket per source IP in kxRateLim and never
removed any. There was no delete(tm.kxRateLim, ...) anywhere in the package —
the only deletes in tunnel.go are for relayKxLim, lastPendDropLog,
lastRekeyReq, pending and peers.
So once maxPerSrcKxEntries (4096) distinct source IPs had EVER sent a
PILA/PILK frame, this branch:
if len(tm.kxRateLim) >= maxPerSrcKxEntries {
return false
}
refused every subsequent new source IP for the life of the process. Key
exchange with any new peer was permanently dead, with no recovery short of a
restart. A peer spraying spoofed source IPs fills all 4096 slots in a single
burst, which turns a rate limiter into a remote, permanent denial of service.
The correct pattern was already thirty lines away: the sibling relay limiter
calls pruneRelayKxLocked before rejecting. This mirrors it exactly with
pruneKxRateLimLocked, reusing relayKxPruneAge (10s).
Pruning costs no rate-limiting accuracy. A bucket idle for 10s has long since
refilled — perSourceKxLimit tokens accrue every second — so dropping it is
equivalent to keeping it: the next frame from that IP recreates it with a
full budget. The cap still holds when the table is genuinely active.
Tests: a full table of stale entries must still admit a new peer (fails
against the unfixed code); a full table of ACTIVE entries must still refuse,
so the prune is a reclaim and not a hole in the cap; and the per-source
budget is unchanged. The pre-existing TestKxRateLimiterMaxEntries still
passes.
Found during a codebase-wide sweep for safety mechanisms that silently fail
to engage — the same class as the dead idle-timeout that OOM-killed the
service fleet.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remote, permanent denial of key exchange
allowKxFromSourcetracked one bucket per source IP inkxRateLimand never removed any. There is nodelete(tm.kxRateLim, ...)anywhere in the package — the only deletes intunnel.goare forrelayKxLim,lastPendDropLog,lastRekeyReq,pendingandpeers.So once
maxPerSrcKxEntries(4096) distinct source IPs had ever sent a PILA/PILK frame:…every subsequent new source IP was refused for the life of the process. Key exchange with any new peer was permanently dead, with no recovery short of a restart.
A peer spraying spoofed source IPs fills all 4096 slots in a single burst. That turns a rate limiter into a remote, permanent DoS — and it fails closed, so the daemon looks healthy while quietly refusing every new peer.
The fix was already thirty lines away
The sibling relay limiter calls
pruneRelayKxLockedbefore rejecting. This mirrors it exactly withpruneKxRateLimLocked, reusingrelayKxPruneAge(10s).Pruning costs no rate-limiting accuracy. A bucket idle for 10s has long since refilled —
perSourceKxLimittokens accrue every second — so dropping it is equivalent to keeping it: the next frame from that IP recreates it with a full budget.Tests
TestKxRateLimiterMaxEntriesstill passesFull
pkg/daemonsuite green (21.6s).Provenance
Found during a codebase-wide sweep for safety mechanisms that silently fail to engage — the same class as the dead idle-timeout that OOM-killed the service fleet, and as
connAdapter.SetReadDeadlinereturningnilwithout storing anything.🤖 Generated with Claude Code