Skip to content

relay: wire per-IP rate-limit middleware on /v1/server + /v1/client #47

@ilmoniemi

Description

@ilmoniemi

User Story

As a relay operator, I want WS upgrade attempts to /v1/server and /v1/client rate-limited per source IP so that a single misbehaving or malicious peer cannot exhaust file descriptors, goroutines, or per-connection memory by retrying connection attempts in a tight loop.

Context

docs/threat-model.md § DoS resistance flags connection floods + fork-bomb retry as in-scope concerns. Today both /v1/server and /v1/client accept unbounded WS upgrade attempts from any single IP — a misbehaving phone (or an attacker) can pin per-connection state without amortised cost.

This ticket wires the two primitives that landed in #50 and #51relay.NewIPRateLimiter (token-bucket limiter with eviction) and relay.ClientIP (RemoteAddr / X-Forwarded-For extractor) — into the upgrade pipeline as middleware, so excess attempts are rejected before websocket.Accept runs. Rejected attempts receive an HTTP 429 Too Many Requests response.

The frame-level cost is already bounded by SetReadLimit (#29) and the 1:N phone slice is bounded by max-phones (#30, landed); this ticket exclusively addresses the pre-upgrade attempt-rate vector.

Acceptance Criteria

  • Middleware wraps /v1/server and /v1/client (NOT /healthz). On a denied attempt: middleware returns 429 Too Many Requests with a generic body (no internal state leaked — docs/threat-model.md § "Error response leakage"), websocket.Accept is never invoked, and the registry is never touched.
  • A new boolean CLI flag --trust-x-forwarded-for is added to cmd/pyrycode-relay/main.go, defaults to false, and is threaded into the middleware's relay.ClientIP call. The flag's Usage string carries an explicit warning that enabling it without a trusted reverse proxy in front allows clients to spoof their source IP.
  • When relay.ClientIP returns the empty string the middleware denies with 429 before calling limiter.Allow. (Per internal/relay/ratelimit.go's doc: Allow("") treats empty as a normal map key — the empty-string guard MUST live in the middleware. Loud-failure principle, docs/PROJECT-MEMORY.md: refuse rather than admit-without-throttling.)
  • Each rate-limit denial emits exactly one slog line (level: architect's call — Warn or Info) carrying event-type and remote-host fields only. The remote-host value is strconv.Quote-wrapped before logging, per internal/relay/client_ip.go's log-injection note. The mobile/binary token (if any) is never read and never logged. Field set must conform to docs/threat-model.md § "Log hygiene".
  • Tests: (a) a sequence of upgrade attempts from a single IP at a rate above the bucket capacity returns 429 for the over-cap attempts while a request from a second IP is unaffected; (b) with --trust-x-forwarded-for=true, a forged X-Forwarded-For header counts against the forged IP's bucket (documents the trust model — enabling the flag means trusting the header).

Technical Notes

  • Compose the middleware around the existing relay.ServerHandler and relay.ClientHandler results at the mux.Handle call sites in cmd/pyrycode-relay/main.go. Do not push the limiter into the handler constructors; middleware is the right seam (composition root pattern).
  • Default rate-limit policy values live as consts at the wiring site in main.go, matching the existing maxFrameBytes pattern (see docs/PROJECT-MEMORY.md "Policy values live at the wiring site"). Starting points to inform the architect's final pick from docs/threat-model.md § "DoS resistance": ~10 attempts / IP / minute (so refillEvery ≈ 6 s), burst ~20, eviction sweep every few minutes. All three must be positive — NewIPRateLimiter panics on a zero/negative evictionInterval.
  • The limiter handle must be Close()d on shutdown — but main.go currently has no graceful-shutdown path (the listeners block forever and os.Exit on error). A defer limiter.Close() placed before the listeners block is sufficient for now; a real shutdown sequence is out of scope.
  • The 429 response body must not include any framework default that could leak internal paths. http.Error(w, "rate limited", http.StatusTooManyRequests) or an explicit w.WriteHeader(429) + minimal body — architect picks the cleaner shape.
  • No new dependencies — stdlib + internal/relay only.

Size Estimate

S — middleware composition + one CLI flag + wiring at the composition root + tests. Under 100 lines of production code.

Out of Scope

  • Rate limiting per server-id or per device token (different threat surface — separate ticket if surfaced).
  • Distributed / multi-instance rate limiting (single-instance only; multi-instance would need shared state / Redis).
  • Adaptive policy (load-aware throttling) — fixed token-bucket is sufficient for v1.

Split from #34 via #46; the rate-limit primitive originally proposed in #46 was further sliced into the limiter type (#50, landed) and the IP-extraction helper (#51, landed). This ticket consumes both.

Metadata

Metadata

Assignees

No one assigned

    Labels

    security-sensitiveTouches auth, crypto, or internet-exposed input pathssize:sSmall ticket: <100 lines production code

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions