diff --git a/README.md b/README.md index e58c5a0..55ffca7 100644 --- a/README.md +++ b/README.md @@ -102,25 +102,37 @@ header, decided *before* the body is read so an abusive caller costs nothing. "rate_limit": { "enabled": true, "trust_forwarded_for": false, + "trusted_proxies": [], "rules": {"/v1/link": [5, 3600], "/v1/repos": [60, 3600]} } ``` -* A malformed rule **refuses startup** rather than silently meaning - unlimited; `[0, N]` disables an endpoint outright. +* Anything malformed **refuses startup** rather than silently meaning + unlimited: a bad rule, a non-boolean `enabled`/`trust_forwarded_for` (JSON + `null`, `0`, or the *string* `"false"` — every non-empty string is truthy), + or a `rate_limit` that is not an object. `[0, N]` disables an endpoint + outright. * Denied requests are *not* recorded, so a client that keeps hammering cannot push its own window forward and lock itself out permanently. * State is an in-process dict behind a lock — granthi-link is one `ThreadingHTTPServer`, so that is the entire store. **If this ever runs - multi-process or multi-host, the limiter must move with it.** The key store - is capped (`MAX_RATE_KEYS`); at capacity it drops least-recent windows and - logs loudly, which is fail-open, chosen over an unbounded dict that is a - memory DoS. -* `trust_forwarded_for` is **off** by default. Turn it on only behind - cloudflared, where every request otherwise arrives from the tunnel and one - abuser would starve everyone. A caller can *prepend* anything to - `X-Forwarded-For`; a trusted proxy *appends* the peer it actually saw, so - the service reads the **last** entry, never the first. + multi-process or multi-host, the limiter must move with it.** The clock is + read *inside* the lock; taken outside, racing threads append out of order + and both `Retry-After` and window reclamation silently go wrong. +* The key store is capped (`MAX_RATE_KEYS`). At capacity it reclaims expired + windows, and if every window is still live it **refuses the new key** — + fail closed. Evicting a live window would let an attacker who can mint many + distinct keys clear their *own* limit on demand, which is worse than + turning a request away on an invite-only endpoint. +* `trust_forwarded_for` is **off** by default, and turning it on **requires a + non-empty `trusted_proxies`** — the header is honored only when the socket + peer is in that list. Without it, anyone reaching the origin directly (it + also listens on the tailnet) could pick and rotate their own rate-limit key + just by sending a header. Turn it on when exposing behind cloudflared, + where every request otherwise arrives from the tunnel and one abuser would + starve everyone. A caller can *prepend* anything to `X-Forwarded-For`; a + trusted proxy *appends* the peer it actually saw, so the service reads the + **last** entry, never the first, and requires it to parse as a real IP. #### Identity binding (`state.json`) diff --git a/server/granthi_link.py b/server/granthi_link.py index 5ac9560..4befa8b 100644 --- a/server/granthi_link.py +++ b/server/granthi_link.py @@ -52,6 +52,7 @@ Stdlib only. Python 3.9+. """ import base64 +import ipaddress import json import logging import os @@ -120,52 +121,93 @@ class RateLimiter: limit, window = rule if limit <= 0: # 0 = endpoint disabled entirely return False, window - now = self._clock() key = (route, client) with self._lock: + # Clock read INSIDE the lock: taken outside, two racing threads + # can append out of order, and both hits[0] (retry_after) and + # v[-1] (eviction age) assume the list is chronological. + now = self._clock() hits = [t for t in self._hits.get(key, ()) if now - t < window] if len(hits) >= limit: self._hits[key] = hits return False, max(1, int(window - (now - hits[0])) + 1) + if key not in self._hits and len(self._hits) >= self.max_keys: + # At capacity, reclaim expired keys first... + self._reclaim_expired(now) + if len(self._hits) >= self.max_keys: + # ...and if every window is still live, this is an + # identity flood, not organic load. Evicting here would + # let an attacker reset their own limit on demand, so + # refuse the NEW key instead. Fail closed: /v1/link is + # invite-only and low-volume, so hitting this cap is an + # attack, and turning tokens away beats minting them. + LOG.error("rate limiter at capacity (%d keys) with no " + "expired windows: refusing new client %r on %s", + self.max_keys, client, route) + return False, window hits.append(now) self._hits[key] = hits - if len(self._hits) > self.max_keys: - self._prune(now) return True, 0 - def _prune(self, now): - """Caller holds the lock. Drop expired keys; if still over the cap, - drop the least-recently-hit. Dropping RESETS those clients' windows, - which is fail-open -- but an unbounded dict is a memory DoS, and a - bounded one is the lesser failure. Say so loudly either way.""" - for k in [k for k, v in self._hits.items() - if not v or now - v[-1] >= self.rules[k[0]][1]]: + def _reclaim_expired(self, now): + """Caller holds the lock. Drop only keys whose window has fully + expired -- never a live one, or eviction becomes the bypass.""" + dead = [k for k, v in self._hits.items() + if not v or now - v[-1] >= self.rules[k[0]][1]] + for k in dead: del self._hits[k] - if len(self._hits) > self.max_keys: - over = len(self._hits) - self.max_keys - for k in sorted(self._hits, key=lambda k: self._hits[k][-1])[:over]: - del self._hits[k] - LOG.warning("rate limiter at capacity (%d keys): dropped %d " - "least-recent windows; those clients start fresh", - self.max_keys, over) + if dead: + LOG.info("rate limiter reclaimed %d expired windows", len(dead)) -def client_ip(handler, trust_forwarded_for): +def client_ip(handler, trust_forwarded_for, trusted_proxies=()): """The address to rate-limit on. Behind cloudflared every request arrives from the tunnel, so limiting on the socket peer would let one abuser starve everyone. X-Forwarded-For is - client-controlled, though: a caller can prepend anything. A trusted proxy - APPENDS the peer it actually saw, so the LAST entry is the only one we did - not let the client choose -- take that, never the first. Off by default; - only enable when something trustworthy really is in front. + client-controlled, though: a caller can prepend anything. Two guards: + + 1. The header is honored ONLY when the socket peer is itself a configured + trusted proxy. Without that, anyone who can reach the origin directly + -- and the origin also listens on the tailnet -- picks their own + rate-limit key and rotates it at will. + 2. A trusted proxy APPENDS the peer it actually saw, so the LAST entry is + the only one the client did not choose. Take that, never the first. + + The value must parse as a real IP; junk falls back to the socket peer + rather than becoming a key of its own. """ - if trust_forwarded_for: - xff = handler.headers.get("X-Forwarded-For", "") - parts = [p.strip() for p in xff.split(",") if p.strip()] - if parts: - return parts[-1] - return handler.client_address[0] + peer = handler.client_address[0] + if not trust_forwarded_for: + return peer + if not _ip_in_any(peer, trusted_proxies): + LOG.warning("X-Forwarded-For ignored: peer %s is not a trusted proxy", + peer) + return peer + xff = handler.headers.get("X-Forwarded-For", "") + parts = [p.strip() for p in xff.split(",") if p.strip()] + if not parts: + return peer + try: + return str(ipaddress.ip_address(parts[-1])) + except ValueError: + LOG.warning("X-Forwarded-For last hop %r is not an IP; using peer", + parts[-1][:60]) + return peer + + +def _ip_in_any(addr, networks): + try: + ip = ipaddress.ip_address(addr) + except ValueError: + return False + for net in networks: + try: + if ip in ipaddress.ip_network(net, strict=False): + return True + except ValueError: + LOG.error("ignoring malformed trusted_proxies entry %r", net) + return False # -------------------------------------------------------------------------- @@ -294,8 +336,26 @@ class LinkService: # Rate limiting is ON by default: this endpoint creates accounts and # mints tokens, so the safe default is limited, and disabling it has # to be a deliberate config act rather than an omission. - rl = config.get("rate_limit") or {} - self.trust_forwarded_for = bool(rl.get("trust_forwarded_for", False)) + rl = config.get("rate_limit", {}) + if rl is None: + rl = {} + if not isinstance(rl, dict): + raise SystemExit("config rate_limit must be an object") + # Real JSON booleans only. `"enabled": null` or `0` must not quietly + # turn limiting off, and the string "false" must not turn XFF trust + # ON (every non-empty string is truthy). + for flag, default in (("enabled", True), ("trust_forwarded_for", False)): + if flag in rl and not isinstance(rl[flag], bool): + raise SystemExit( + f"config rate_limit.{flag} must be true or false, " + f"got {rl[flag]!r}") + self.trust_forwarded_for = rl.get("trust_forwarded_for", False) + self.trusted_proxies = tuple(rl.get("trusted_proxies") or ()) + if self.trust_forwarded_for and not self.trusted_proxies: + raise SystemExit( + "config rate_limit.trust_forwarded_for requires a non-empty " + "trusted_proxies list -- trusting the header from any peer " + "lets callers choose their own rate-limit key") if rl.get("enabled", True): rules = dict(DEFAULT_RATE_RULES) for route, spec in (rl.get("rules") or {}).items(): @@ -308,9 +368,10 @@ class LinkService: f"[max_requests, window_seconds] with window > 0") rules[route] = (spec[0], spec[1]) self.limiter = RateLimiter(rules) - LOG.info("rate limiting active: %s (trust_forwarded_for=%s)", + LOG.info("rate limiting active: %s (trust_forwarded_for=%s, " + "trusted_proxies=%s)", {k: f"{v[0]}/{v[1]}s" for k, v in rules.items()}, - self.trust_forwarded_for) + self.trust_forwarded_for, list(self.trusted_proxies)) else: self.limiter = None LOG.warning("rate limiting DISABLED by config -- /v1/link will " @@ -571,7 +632,8 @@ class Handler(BaseHTTPRequestHandler): # to drain a body we already decided to reject. limiter = getattr(self.service, "limiter", None) if limiter is not None: - who = client_ip(self, self.service.trust_forwarded_for) + who = client_ip(self, self.service.trust_forwarded_for, + self.service.trusted_proxies) allowed, retry_after = limiter.check(self.path, who) if not allowed: LOG.warning("rate limited %s %s (retry after %ss)", diff --git a/tests/test_server.py b/tests/test_server.py index da3a883..9411a15 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -585,19 +585,43 @@ class _FakeHandler: class TestClientIp(unittest.TestCase): + PROXY = ("10.0.0.0/8",) + def test_socket_peer_by_default(self): h = _FakeHandler("5.5.5.5", xff="1.2.3.4") - self.assertEqual(granthi_link.client_ip(h, False), "5.5.5.5") + self.assertEqual(granthi_link.client_ip(h, False, self.PROXY), "5.5.5.5") def test_trusted_proxy_uses_the_last_hop_not_the_client_supplied_first(self): """A caller can PREPEND anything to X-Forwarded-For; a trusted proxy appends the peer it really saw. Only the last entry is trustworthy.""" h = _FakeHandler("10.0.0.1", xff="1.2.3.4, 203.0.113.9") - self.assertEqual(granthi_link.client_ip(h, True), "203.0.113.9") + self.assertEqual(granthi_link.client_ip(h, True, self.PROXY), + "203.0.113.9") + + def test_untrusted_peer_cannot_choose_its_own_key(self): + """The origin also listens on the tailnet. Anyone reaching it + directly must not be able to pick (and rotate) their rate-limit key + just by sending a header.""" + h = _FakeHandler("203.0.113.50", xff="9.9.9.9") + self.assertEqual(granthi_link.client_ip(h, True, self.PROXY), + "203.0.113.50") + + def test_non_ip_last_hop_falls_back_to_peer(self): + for junk in ("not-an-ip", "', OR 1=1", "x" * 500, "10.0.0.1:8080"): + with self.subTest(junk=junk): + h = _FakeHandler("10.0.0.1", xff=f"1.2.3.4, {junk}") + self.assertEqual( + granthi_link.client_ip(h, True, self.PROXY), "10.0.0.1") def test_trusted_proxy_falls_back_when_header_absent(self): h = _FakeHandler("10.0.0.1") - self.assertEqual(granthi_link.client_ip(h, True), "10.0.0.1") + self.assertEqual(granthi_link.client_ip(h, True, self.PROXY), + "10.0.0.1") + + def test_malformed_trusted_proxy_entry_does_not_grant_trust(self): + h = _FakeHandler("10.0.0.1", xff="9.9.9.9") + self.assertEqual( + granthi_link.client_ip(h, True, ("not-a-cidr",)), "10.0.0.1") class TestRateLimitConfig(ServiceTestBase): @@ -655,3 +679,94 @@ class TestRateLimitOverHttp(HandlerTestBase): conn.request("GET", "/health") self.assertEqual(conn.getresponse().status, 200) conn.close() + + +class TestRateLimitHardening(unittest.TestCase): + """The four codex [P2] findings, each pinned by a test.""" + + def test_capacity_fails_closed_instead_of_resetting_a_live_window(self): + """Evicting a live window would let an attacker who can mint many + distinct keys clear their OWN limit on demand. New keys are refused + instead while every window is still live.""" + now = [1000.0] + rl = granthi_link.RateLimiter(rules={"/v1/link": (1, 3600)}, + max_keys=5, clock=lambda: now[0]) + for i in range(5): + self.assertTrue(rl.check("/v1/link", f"10.0.0.{i}")[0]) + victim_hits = list(rl._hits[("/v1/link", "10.0.0.0")]) + for i in range(100, 140): # identity flood + allowed, retry = rl.check("/v1/link", f"10.0.1.{i}") + self.assertFalse(allowed) + self.assertGreater(retry, 0) + # the earlier client's window survived the flood untouched + self.assertEqual(rl._hits[("/v1/link", "10.0.0.0")], victim_hits) + self.assertLessEqual(len(rl._hits), 5) + + def test_capacity_recovers_once_windows_expire(self): + now = [1000.0] + rl = granthi_link.RateLimiter(rules={"/v1/link": (1, 60)}, + max_keys=3, clock=lambda: now[0]) + for i in range(3): + rl.check("/v1/link", f"10.0.0.{i}") + self.assertFalse(rl.check("/v1/link", "10.0.9.9")[0]) + now[0] += 61 + self.assertTrue(rl.check("/v1/link", "10.0.9.9")[0]) + + def test_hits_stay_chronological_under_thread_contention(self): + """retry_after uses hits[0] and reclamation uses v[-1]; both assume + the list is ordered. Reading the clock outside the lock let racing + threads append out of order.""" + rl = granthi_link.RateLimiter(rules={"/v1/link": (500, 3600)}) + ts = [threading.Thread(target=lambda: rl.check("/v1/link", "7.7.7.7")) + for _ in range(200)] + for t in ts: + t.start() + for t in ts: + t.join() + hits = rl._hits[("/v1/link", "7.7.7.7")] + self.assertEqual(len(hits), 200) + self.assertEqual(hits, sorted(hits), "timestamps out of order") + + +class TestRateLimitConfigTypes(ServiceTestBase): + def _svc(self, rl): + cfg = dict(self.svc.cfg) + cfg["rate_limit"] = rl + return granthi_link.LinkService(cfg) + + def test_non_boolean_enabled_refuses_startup(self): + """`"enabled": null` or `0` must not quietly mean unlimited.""" + for bad in (None, 0, "", "false", "no", []): + with self.subTest(enabled=bad): + with self.assertRaises(SystemExit): + self._svc({"enabled": bad}) + + def test_string_false_does_not_enable_forwarded_trust(self): + """Every non-empty string is truthy -- "false" used to mean True.""" + with self.assertRaises(SystemExit): + self._svc({"trust_forwarded_for": "false", + "trusted_proxies": ["10.0.0.0/8"]}) + + def test_rate_limit_must_be_an_object(self): + for bad in ("yes", 5, ["/v1/link"]): + with self.subTest(rl=bad): + with self.assertRaises(SystemExit): + self._svc(bad) + + def test_null_rate_limit_means_defaults_not_disabled(self): + svc = self._svc(None) + self.assertIsNotNone(svc.limiter) + + def test_forwarded_trust_without_trusted_proxies_refuses_startup(self): + """Trusting the header from ANY peer lets callers choose their own + rate-limit key -- that must not be reachable by omission.""" + with self.assertRaises(SystemExit): + self._svc({"trust_forwarded_for": True}) + with self.assertRaises(SystemExit): + self._svc({"trust_forwarded_for": True, "trusted_proxies": []}) + + def test_forwarded_trust_with_proxies_is_accepted(self): + svc = self._svc({"trust_forwarded_for": True, + "trusted_proxies": ["10.0.0.0/8", "127.0.0.1/32"]}) + self.assertTrue(svc.trust_forwarded_for) + self.assertEqual(len(svc.trusted_proxies), 2)