Repository navigation
docs: state the local forwarded-address lockout exposure - #4106
Merged
Merged
Conversation
janicduplessis
commented
Oct 20, 2026
janicduplessis
left a comment
Collaborator
Author
There was a problem hiding this comment.
Review of the wording against origin/main (71a9f97): the claim is accurate. Two optional wording points and one precision note, nothing blocking.
Verified in packages/server/src/server.ts:
peerAddress(l.858) returnsremoteAddressfor non-loopback sockets and ignores the header there. For loopback sockets it returns the firstX-Forwarded-Forentry, or null when there is none. So only loopback connections can supply the key, and a header-less loopback connection falls into the shared'local'bucket (peer ?? 'local'at l.1312, 1329, 4359, 4388).- Limiters: main WebSocket
FailureLimiter(5, ...)(l.1105),agentLimiter30 (l.1310),setupLimiter30 (l.1324). Auth timeout (l.4365) and handshake refusals other thanunknown-secret(l.4394, 4411) record against the key, so an unauthenticated local process can fill a named peer's bucket with idle or garbage connections.unknown-secretis not counted. Matches the PR body. - "Tailscale rewrites the header": confirmed upstream.
ipn/ipnlocal/serve.goaddProxyForwardedHeadersdoesr.Out.Header.Set("X-Forwarded-For", c.SrcAddr.Addr().String())insidehttputil.ReverseProxy{Rewrite: ...}, which replaces any client-supplied value rather than appending. - Unix-socket path:
upgradeLocal(l.4444) never callslimiter.blocked, so no lockout applies there.
No stale statements found: git grep over docs, website, READMEs, AGENTS.md and packages/*/src finds no other place that says the forwarded address "grants nothing" or "limits load". The remaining mentions are consistent: AGENTS.md:314 ("label records and key rate limits; they never authenticate"), README l.32 and 460, spec l.341, and the l.4379 comment. pnpm run format:check passes and the diff is ASCII. The PR body matches the diff and the code.
Suggestions (non-blocking):
website/docs/remote-machines.mdsays "lock a peer out temporarily". The attacker can repeat it every window, so the lockout is renewable for as long as the process runs. "Temporarily" understates that; "repeatably" or "for as long as it keeps trying" is closer. The README and spec say "for the lockout window", which is accurate per window but does not mention renewal either. The acceptance rationale (needs local code execution) still holds.- PR body: "the Unix-socket path (Desktop, CLI) has no limiter" is true for blocking, but
connection()still callslimiter.record('local')for Unix-socket failures (l.1555, 1560). A same-user process on the Unix socket can therefore fill the shared'local'bucket and block header-less loopback TCP clients. This is already covered by "requires local code execution", so no doc change is needed. Say "no lockout check" if you want it exact. - Optional: AGENTS.md:314 (the machine-to-machine authentication rule) still reads as if the rate-limit keying is exposure-free. It is not wrong, and the new text lives in the spec it points to, so leave it unless you want the exposure stated in the agent rules too.
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.
Description
The pluggable-transports spec said the forwarded address "limits load, it grants nothing". That understates one case. stim-server keys its failed-authentication limiters (main WebSocket: 5 failures;
setupLimiterfor/setup/<journal>andagentLimiter: 30) by the firstX-Forwarded-Forentry for serve-proxied connections (peerAddressinpackages/server/src/server.ts).Tailscale serve rewrites that header to the real tailnet source, so remote clients cannot forge it and are counted per peer. Any local process on the host, including another OS user, can connect to the loopback TCP port and forge it. It can lock a named tailnet peer out for the lockout window, or the shared
localbucket if it omits the header. It cannot authenticate or read anything: secrets are 32 bytes, unknown secrets are not counted, and the Unix-socket path (Desktop, CLI) has no limiter.The maintainer accepted this on 2026-10-20 because it requires local code execution on the host. Alternatives considered: never refusing handshake-authenticated connections, and a Unix-socket serve backend.
Solution
Docs only. The spec's Decisions entry, the
packages/server/README.mdauthentication section and the build-approval note inwebsite/docs/remote-machines.mdnow state the lockout exposure and that it is accepted.Test plan
pnpm run format:checkpasses.peerAddressand the limiter setup inpackages/server/src/server.tsas oforigin/main.Refs #3722