Skip to content

docs: state the local forwarded-address lockout exposure - #4106

Merged
janicduplessis merged 1 commit into
mainfrom
docs/3722-limiter-wording
Oct 20, 2026
Merged

janicduplessis merged 1 commit into
mainfrom
docs/3722-limiter-wording

Conversation

@janicduplessis

Copy link
Copy Markdown
Collaborator

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; setupLimiter for /setup/<journal> and agentLimiter: 30) by the first X-Forwarded-For entry for serve-proxied connections (peerAddress in packages/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 local bucket 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.md authentication section and the build-approval note in website/docs/remote-machines.md now state the lockout exposure and that it is accepted.

Test plan

  • pnpm run format:check passes.
  • The wording matches peerAddress and the limiter setup in packages/server/src/server.ts as of origin/main.

Refs #3722

@janicduplessis janicduplessis left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) returns remoteAddress for non-loopback sockets and ignores the header there. For loopback sockets it returns the first X-Forwarded-For entry, 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), agentLimiter 30 (l.1310), setupLimiter 30 (l.1324). Auth timeout (l.4365) and handshake refusals other than unknown-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-secret is not counted. Matches the PR body.
  • "Tailscale rewrites the header": confirmed upstream. ipn/ipnlocal/serve.go addProxyForwardedHeaders does r.Out.Header.Set("X-Forwarded-For", c.SrcAddr.Addr().String()) inside httputil.ReverseProxy{Rewrite: ...}, which replaces any client-supplied value rather than appending.
  • Unix-socket path: upgradeLocal (l.4444) never calls limiter.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):

  1. website/docs/remote-machines.md says "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.
  2. PR body: "the Unix-socket path (Desktop, CLI) has no limiter" is true for blocking, but connection() still calls limiter.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.
  3. 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.

@janicduplessis
janicduplessis marked this pull request as ready for review October 20, 2026 22:22
@janicduplessis
janicduplessis added this pull request to the merge queue Oct 20, 2026
Merged via the queue into main with commit f00e07b Oct 20, 2026
20 checks passed
@janicduplessis
janicduplessis deleted the docs/3722-limiter-wording branch October 20, 2026 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant