From 2a1fc65cd86b911d013a18e9487dd8c47f96aa9f Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 1 Sep 2026 18:48:59 -0400 Subject: [PATCH] feat(web): a remote session expires, and a wildcard bind must say what it expects (#648) Three of #648's five requirements, and the first finding is that one of them should NOT be built. NO RATE LIMITER, AND THE ARITHMETIC IS WHY. The issue asks for "issuance rate-limiting and brute-force posture". The token carries 256 bits (`secrets.token_urlsafe(32)`) -- a space of ~1.2e77, which at a billion guesses a second no `http.server` on a laptop will serve takes on the order of 1e60 years to dent. Guessing is not a threat this server has, and a limiter installed to stop it would be theatre: state, a failure mode, and a false sense that something was closed. `TOKEN_ENTROPY_BITS` records the number so it travels with the claim, and the comment says that if a limiter is ever added it must be justified by bounding log volume or making probing visible -- never by brute force. WHAT A REMOTE ORIGIN ACTUALLY CHANGES is who can use a token that leaked. On loopback that population is software already running as this operator, against which no session lifetime helps at all. Through a tunnel it becomes anyone who can reach the origin -- and the token has been in a URL, in terminal scrollback, and in whatever got pasted while asking for help. The 30-day cookie `Max-Age` is a BROWSER hint such an attacker ignores entirely, so the bound has to be enforced on this side of the wire or it is not a bound. A server configured with `--external-host` therefore enforces a 12-hour session lifetime, checked BEFORE the token so an expired session cannot be told apart from a wrong one by which refusal comes back. A loopback-only server has none, and that asymmetry is the argument rather than an exemption -- `SESSION_COOKIE_MAX_AGE_SECONDS`'s reasoning for the 30-day cookie is sound and is left completely intact. A WILDCARD BIND NOW REFUSES TO START WITHOUT A NAME. `--host 0.0.0.0` produced a server that refused EVERY request: `HostPolicy` then expects `Host: 0.0.0.0`, which no browser sends. It failed closed -- right direction, wrong explanation, and the operator's conclusion was "keel is broken" rather than "keel does not know which name to expect". A wildcard is precisely the bind where the name cannot be derived, because every interface has a different one, so it is the one bind that requires stating it. Not a blocklist: permitted the moment `--external-host` says what to expect. STILL OPEN, and #648 stays open for it: secure-context re-verification. The service worker and manifest work today because `http://127.0.0.1` is a secure context BY SPECIFICATION; over an external origin that property comes from HTTPS instead and has to be re-verified there. That needs a real deployed origin and cannot be closed from a checkout -- so an installed console reached through a tunnel is untested, not merely unsupported, and the doc says exactly that. 6 mutants, 6 killed. One is worth keeping: shrinking the lifetime to ONE SECOND passed everything, because the tests only pinned that it was shorter than the cookie. A lifetime that expires mid-use is not a session bound, it is the "refuses every request" outage the wildcard refusal exists to prevent -- so it is now pinned at both ends. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NzuKAe2RVrPt9acVAWjRyL --- docs/remote-access.md | 56 ++++++++++++----- keel/commands/serve.py | 26 ++++++++ keel/web/security.py | 30 +++++++++ keel/web/server.py | 35 ++++++++++- tests/web/test_serve_command.py | 106 ++++++++++++++++++++++++++++++++ tests/web/test_server.py | 72 ++++++++++++++++++++++ 6 files changed, 309 insertions(+), 16 deletions(-) diff --git a/docs/remote-access.md b/docs/remote-access.md index 3e31276..1e81fa1 100644 --- a/docs/remote-access.md +++ b/docs/remote-access.md @@ -3,9 +3,9 @@ `keel serve` binds loopback. That is the posture, and this document exists so that widening it is a decision someone made on purpose rather than a flag someone found. -**Status: incomplete. Nothing remote should be exposed yet.** Two of #648's five requirements -are met — the bind is configurable and a reverse proxy's hostname can be expected explicitly — -and three are not. They are named at the bottom, and they are not paperwork. +**Status: incomplete. Nothing remote should be exposed yet.** Four of #648's five requirements +are met. The last one — re-verifying the PWA's secure-context behaviour over a real HTTPS +origin — cannot be done from a checkout, and it is named at the bottom rather than waved past. ## What the defence actually defends @@ -62,21 +62,47 @@ discover it later would have failed. A mesh has no such property: WireGuard is end-to-end between devices the operator enrolled, and the coordination server distributes keys without being able to read what they protect. +## The session, once a remote host is configured + +**There is no brute-force threat, and saying so is the point.** The token carries 256 bits of +entropy (`secrets.token_urlsafe(32)`), a space of about 1.2 × 10⁷⁷. An attacker managing a +billion guesses a second — which no `http.server` on a laptop will serve — needs on the order of +10⁶⁰ years to cover a meaningful fraction. A rate limiter installed to stop guessing would be +theatre: state, a failure mode, and a false sense that something was closed. The arithmetic is +recorded beside the constant (`TOKEN_ENTROPY_BITS`) so it travels with the claim. If a limiter is +ever added it must be justified by bounding log volume or making probing visible — never by +brute force. + +**What a remote origin does change is who can use a token that leaked.** On loopback that +population is software already running as the operator, and no session lifetime helps against +it. Through a tunnel it becomes anyone who can reach the origin — and the token has been in a +URL, in terminal scrollback, and in whatever got pasted while asking for help. The 30-day cookie +`Max-Age` is a *browser* hint that such an attacker ignores entirely. + +So a server configured with `--external-host` enforces a **12-hour session lifetime on its own +side of the wire**, checked before the token so an expired session cannot be told apart from a +wrong one by which refusal comes back. A loopback-only server has none, and that asymmetry is +the argument rather than an exemption. Restarting `keel serve` remains the instant revocation +gesture in both postures. + +## Binding every interface + +`--host 0.0.0.0` (or `::`) used to produce a server that refused **every** request: `HostPolicy` +would then expect `Host: 0.0.0.0`, which no browser sends. It failed closed — the right +direction, the wrong explanation, and the operator's conclusion was "keel is broken" rather than +"keel does not know which name to expect". + +A wildcard bind is precisely the case where the name cannot be derived, because every interface +has a different one. So it is the one bind that requires stating it, and `keel serve` now refuses +to start on a wildcard with no `--external-host` — once, at the moment it can be acted on. + ## ⛔ Not done — do not expose the console yet -Three of #648's requirements remain, and each is a real gap rather than a formality: +One requirement remains, and it is not a formality: -- **Session tokens over a remote origin.** The token is generated per `keel serve` run and never - written to disk, which is right for loopback. Nothing yet states its entropy against an - attacker who can reach the origin from the open internet, and there is no issuance - rate-limiting or brute-force posture — on loopback there was no attacker to rate-limit. - **Secure-context re-verification.** The service worker and manifest work today because `http://127.0.0.1` is a secure context *by specification*. Over an external origin that property comes from HTTPS instead, and the PWA behaviours have to be re-verified there rather - than assumed from the loopback behaviour. -- **Bind opt-in beyond a mesh address.** `--host` accepts any address, and binding `0.0.0.0` - currently produces a server that refuses every request — `HostPolicy` then expects - `Host: 0.0.0.0`, which no browser sends. It fails closed, which is the safe direction, but it - fails confusingly and needs its own decision rather than this footnote. - -Until those land, `--external-host` is the mechanism waiting for the pass, not the pass. + than assumed from the loopback behaviour. That needs a real deployed origin, so it cannot be + closed from a checkout — and until it is, an installed console reached through a tunnel is + untested, not merely unsupported. diff --git a/keel/commands/serve.py b/keel/commands/serve.py index efb0be5..4e9bd79 100644 --- a/keel/commands/serve.py +++ b/keel/commands/serve.py @@ -29,6 +29,11 @@ DEFAULT_HOST = "127.0.0.1" +#: Binds that answer on EVERY interface, where no single hostname can be derived from the +#: address (#648). Not a blocklist -- each is permitted the moment `--external-host` says what +#: to expect. +_WILDCARD_BINDS = frozenset({"0.0.0.0", "::", "[::]", "*"}) + @click.command("serve") @click.option("--host", default=DEFAULT_HOST, show_default=True, help="Address to bind.") @@ -92,6 +97,27 @@ def serve_cmd( HostPolicy(bound_host=host, port=port, external_hosts=cleaned) except ValueError as exc: raise click.BadParameter(str(exc), param_hint="--external-host") from exc + + # #648, and this is a DECISION about a confusing failure rather than a new restriction. + # Binding a wildcard produced a server that refused every request: `HostPolicy` then expects + # `Host: 0.0.0.0`, which no browser ever sends, so the bind succeeded, the URL printed, and + # nothing worked with a 403 that blamed the address. It failed CLOSED, which is the right + # direction and the wrong explanation -- the operator's conclusion is "keel is broken", not + # "keel does not know what name to expect". + # + # A wildcard bind is exactly the case where the name cannot be derived, because the server + # is answering on every interface and each has a different one. So it is the one bind that + # REQUIRES the name to be stated. Refusing here says that, once, at the moment it can be + # acted on. + if host in _WILDCARD_BINDS and not cleaned: + raise click.BadParameter( + f"binding {host} answers on every interface, so keel cannot derive which hostname " + "to expect in Host: -- and a server that expects the wrong one refuses every " + "request with a 403 that looks like a fault. Name the hostname browsers will use, " + "e.g. `--external-host keel.example.com`, or bind that address directly. See " + "docs/remote-access.md before exposing this server at all.", + param_hint="--host", + ) cfg = ServeConfig( host=host, port=port, diff --git a/keel/web/security.py b/keel/web/security.py index a80367a..1a395a7 100644 --- a/keel/web/security.py +++ b/keel/web/security.py @@ -91,6 +91,36 @@ #: does not accumulate an entry with no end at all. SESSION_COOKIE_MAX_AGE_SECONDS = 30 * 24 * 60 * 60 +#: How long a session may authenticate once a REMOTE origin is configured (#648). Twelve hours: +#: long enough that a working day does not end in a re-authorisation, short enough that a token +#: which leaked this morning does not still work tomorrow. +#: +#: ⚠️ This does NOT apply to a loopback-only server, and the asymmetry is the argument rather +#: than an exemption. `SESSION_COOKIE_MAX_AGE_SECONDS`'s reasoning above is sound and unchanged: +#: the token's power is bounded by a process the browser does not control, and on loopback the +#: population who could USE a leaked copy is "software already running as this operator" -- +#: against which a shorter session buys nothing at all. +#: +#: A remote origin changes that population to "anyone who can reach the tunnel", and the token +#: has been in a URL, in terminal scrollback, and in whatever the operator pasted while asking +#: for help. The 30-day cookie is a BROWSER hint an attacker with the token ignores entirely, so +#: the bound has to be enforced on this side of the wire or it is not a bound. +REMOTE_SESSION_MAX_AGE_SECONDS = 12 * 60 * 60 + +#: ⛔ THE BRUTE-FORCE ARITHMETIC, WRITTEN DOWN SO NOBODY ADDS A RATE LIMITER FOR THE WRONG +#: REASON (#648). `_TOKEN_BYTES = 32` is 256 bits, so `secrets.token_urlsafe` draws from a space +#: of 2**256 ~ 1.2e77. An attacker managing a billion guesses per second -- which no HTTP server +#: on a laptop will serve -- needs on the order of 1e60 years to cover a meaningful fraction. +#: +#: Guessing is therefore NOT a threat this server has, and a rate limiter installed to stop it +#: would be theatre: it would add state, a failure mode, and a false sense that something was +#: closed. What bounds risk here is entropy, which is already past any margin that matters. +#: +#: What a limiter WOULD buy is unrelated to guessing -- bounding log volume from a scanner, and +#: making probing visible. Those are real, and if one is ever added it must be justified by +#: those and not by brute force. Recorded as a constant so the number travels with the claim. +TOKEN_ENTROPY_BITS = 256 + #: The request header carrying the CSRF token on a write (#540). #: #: A HEADER rather than a body field, and the difference is the whole reason this layer still diff --git a/keel/web/server.py b/keel/web/server.py index 1e04631..6559b4e 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -43,8 +43,9 @@ import json import socket import sys +import time from collections.abc import Callable -from dataclasses import dataclass +from dataclasses import dataclass, field from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path from typing import Any @@ -53,6 +54,7 @@ from keel.web import api, events, staticfiles from keel.web.security import ( CSRF_HEADER, + REMOTE_SESSION_MAX_AGE_SECONDS, SESSION_COOKIE, HostPolicy, csrf_token, @@ -105,6 +107,23 @@ class ServeConfig: #: Hostnames a reverse proxy may present that this server never bound (#648). Empty by #: default -- loopback-only is the posture, and remaining the posture is the point. external_hosts: frozenset[str] = frozenset() + #: When this run began, for the remote session lifetime (#648). Defaults to the moment the + #: config is built, which is the moment `keel serve` mints the token it bounds. + started_at: float = field(default_factory=time.time) + + @property + def session_expired_at(self) -> float | None: + """When this session stops authenticating, or `None` if it does not expire. + + `None` on a loopback-only server, and that is a decision rather than an omission -- + `REMOTE_SESSION_MAX_AGE_SECONDS` carries the argument. The population who could use a + leaked token on loopback is software already running as this operator, and no session + lifetime helps against that; a remote origin changes the population to anyone who can + reach the tunnel, and the 30-day cookie is a browser hint such an attacker ignores. + """ + if not self.external_hosts: + return None + return self.started_at + REMOTE_SESSION_MAX_AGE_SECONDS @property def host_policy(self) -> HostPolicy: @@ -485,6 +504,20 @@ def _admitted(self) -> bool: "This request did not come from the address keel is serving on.", ) return False + # #648: an ENFORCED lifetime, checked before the token so an expired session cannot be + # distinguished from a wrong one by which refusal comes back. Only a remote-configured + # server has one -- see `ServeConfig.session_expired_at`. + expires = self.cfg.session_expired_at + if expires is not None and time.time() >= expires: + self._refuse( + 403, + "Session expired", + "This session has reached its lifetime. keel enforces one when it is configured " + "to answer a remote hostname, because a token that leaked into a URL, a " + "screenshot or terminal scrollback should not still work tomorrow. Restart " + "`keel serve` and open the address it prints.", + ) + return False cookies = parse_cookie_header(self.headers.get("Cookie")) if not tokens_match(cookies.get(SESSION_COOKIE), self.cfg.token): self._refuse( diff --git a/tests/web/test_serve_command.py b/tests/web/test_serve_command.py index ca2c8a2..51d816a 100644 --- a/tests/web/test_serve_command.py +++ b/tests/web/test_serve_command.py @@ -9,6 +9,7 @@ from click.testing import CliRunner from keel.cli import cli +from keel.web import security from keel.web import server as web_server from keel.web.security import new_session_token @@ -239,3 +240,108 @@ def test_an_empty_flag_value_is_dropped_rather_than_refused( assert result.exit_code == 0, result.output assert cfg is not None assert cfg.external_hosts == frozenset() + + +# -- a wildcard bind must name what it expects (#648) --------------------------------------------- + + +@pytest.mark.parametrize("wildcard", ["0.0.0.0", "::", "[::]"]) +def test_a_wildcard_bind_without_a_named_host_refuses_to_start( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, wildcard: str +) -> None: + """The confusing failure, turned into an explanation. + + Binding a wildcard used to produce a server that refused EVERY request: `HostPolicy` then + expects `Host: 0.0.0.0`, which no browser sends, so the bind succeeded, the URL printed, and + nothing worked behind a 403 that blamed the address. It failed closed -- right direction, + wrong explanation. The operator concludes "keel is broken", not "keel does not know which + name to expect". + """ + result, _cfg = _policy_from_cli(monkeypatch, tmp_path, "--host", wildcard) + + assert result.exit_code != 0, result.output + assert "every interface" in result.output + + +def test_a_wildcard_bind_is_permitted_once_a_host_is_named( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Not a blocklist. A wildcard bind is exactly the case where the name cannot be derived -- + every interface has a different one -- so it is the one bind that requires stating it.""" + result, cfg = _policy_from_cli( + monkeypatch, tmp_path, "--host", "0.0.0.0", "--external-host", "keel.example.com" + ) + + assert result.exit_code == 0, result.output + assert cfg is not None + assert cfg.host_policy.permits("keel.example.com:8765") + + +def test_a_specific_non_loopback_bind_is_untouched( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """`--host 10.0.0.5` derives its own name and always worked. This must not become collateral + damage of the wildcard refusal.""" + result, cfg = _policy_from_cli(monkeypatch, tmp_path, "--host", "10.0.0.5") + + assert result.exit_code == 0, result.output + assert cfg is not None + assert cfg.host_policy.permits("10.0.0.5:8765") + + +# -- the remote session lifetime (#648) ----------------------------------------------------------- + + +def test_a_loopback_server_has_no_session_expiry(tmp_path: Path) -> None: + """A decision, not an omission. On loopback the population who could use a leaked token is + software already running as this operator, and no session lifetime helps against that.""" + cfg = web_server.ServeConfig( + host="127.0.0.1", + port=8765, + token=new_session_token(), + db_path=str(tmp_path / "keel.db"), + config_path=str(tmp_path / "config.yaml"), + ) + + assert cfg.session_expired_at is None + + +def test_configuring_a_remote_host_bounds_the_session(tmp_path: Path) -> None: + """A remote origin changes the population to anyone who can reach the tunnel, and the + 30-day cookie is a BROWSER hint an attacker holding the token ignores entirely -- so the + bound has to be enforced on this side of the wire or it is not a bound.""" + cfg = web_server.ServeConfig( + host="127.0.0.1", + port=8765, + token=new_session_token(), + db_path=str(tmp_path / "keel.db"), + config_path=str(tmp_path / "config.yaml"), + external_hosts=frozenset({"keel.example.com"}), + started_at=1_000_000.0, + ) + + assert cfg.session_expired_at == 1_000_000.0 + security.REMOTE_SESSION_MAX_AGE_SECONDS + + +def test_the_remote_lifetime_is_bounded_at_both_ends() -> None: + """A lifetime is only useful between two limits, and both are claims the docstring makes. + + **Shorter than the cookie** the browser keeps, or the enforced bound never bites before the + browser stops sending the cookie anyway and the mechanism is decorative. + + **Long enough for a working day**, or it is not a session lifetime but an outage: a value + small enough to expire mid-use would be indistinguishable, to the operator, from the + "refuses every request" failure the wildcard-bind refusal exists to prevent. A mutation run + set this to one second and every test still passed -- which is why the floor is here. + """ + assert security.REMOTE_SESSION_MAX_AGE_SECONDS < security.SESSION_COOKIE_MAX_AGE_SECONDS + assert security.REMOTE_SESSION_MAX_AGE_SECONDS >= 8 * 60 * 60 + + +def test_the_entropy_arithmetic_is_recorded_beside_the_token() -> None: + """#648 asked for a brute-force posture and the honest answer is that there is no brute-force + threat: 256 bits is ~1.2e77 values. A rate limiter added to stop guessing would be theatre, + and this constant exists so the number travels with that claim instead of being re-derived + by whoever proposes one.""" + assert security.TOKEN_ENTROPY_BITS == security._TOKEN_BYTES * 8 + assert security.TOKEN_ENTROPY_BITS >= 128 diff --git a/tests/web/test_server.py b/tests/web/test_server.py index 64cbe8d..fc1e35f 100644 --- a/tests/web/test_server.py +++ b/tests/web/test_server.py @@ -16,14 +16,18 @@ from __future__ import annotations +import dataclasses import http.client import json import threading +import time from collections.abc import Iterator +from contextlib import contextmanager from pathlib import Path import pytest +from keel.web import security from keel.web import server as web_server from keel.web.security import SESSION_COOKIE, new_session_token, session_cookie @@ -1357,3 +1361,71 @@ def _slow_action(_config, _db, _values): gate.set() jobs.wait(5) jobs.reset() + + +# -- an expired remote session is refused by the handler (#648) --------------------------------- + + +@contextmanager +def _serving(deployment: tuple[str, str], **changes: object): + """A live server with a config of this test's choosing. + + The `running` fixture builds one loopback-only server per test and does not expose its + handler class, and these tests need a DIFFERENT posture -- a configured remote host and a + start time in the past. Building one here is shorter than widening the fixture for two + callers, and it binds port 0 and tears down the same way. + """ + db_path, config_path = deployment + cfg = web_server.ServeConfig( + host="127.0.0.1", + port=0, + token=new_session_token(), + db_path=db_path, + config_path=config_path, + **changes, # type: ignore[arg-type] + ) + server = web_server.build_server(cfg) + bound = dataclasses.replace(cfg, port=int(server.server_address[1])) + server.RequestHandlerClass.cfg = bound # type: ignore[attr-defined] + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + yield bound + finally: + server.shutdown() + server.server_close() + thread.join(timeout=5) + + +def test_an_expired_remote_session_is_refused_even_with_a_valid_token(deployment) -> None: + """The lifetime is ENFORCED, not requested. + + `Max-Age` is a browser hint: an attacker holding the token sends the cookie whenever they + like, for as long as they like. So the bound has to be applied on this side of the wire -- + and it has to bite while the token itself is still perfectly valid, which is what makes + this a lifetime rather than a second way of spelling "wrong token". + """ + remote = frozenset({"keel.example.com"}) + stale = time.time() - security.REMOTE_SESSION_MAX_AGE_SECONDS - 1 + + with _serving(deployment, external_hosts=remote, started_at=time.time()) as fresh: + status, _headers, _body = _request(fresh, "/", cookie=f"{SESSION_COOKIE}={fresh.token}") + assert status == 200, "a fresh remote session must answer" + + with _serving(deployment, external_hosts=remote, started_at=stale) as old: + status, _headers, body = _request(old, "/", cookie=f"{SESSION_COOKIE}={old.token}") + assert status == 403 + assert "lifetime" in body + + +def test_a_loopback_session_of_the_same_age_still_answers(deployment) -> None: + """The asymmetry is the argument, not an exemption -- see `REMOTE_SESSION_MAX_AGE_SECONDS`. + + On loopback the population who could use a leaked token is software already running as this + operator, against which no session lifetime helps. Expiring there would cost the operator a + re-authorisation and buy nothing at all. + """ + with _serving(deployment, started_at=time.time() - 365 * 24 * 3600) as aged: + status, _headers, _body = _request(aged, "/", cookie=f"{SESSION_COOKIE}={aged.token}") + + assert status == 200