diff --git a/docs/remote-access.md b/docs/remote-access.md index 5c79c846..0f496e94 100644 --- a/docs/remote-access.md +++ b/docs/remote-access.md @@ -81,16 +81,20 @@ URL, in terminal scrollback, and in whatever got pasted while asking for help. T So a server configured with `--external-host` enforces a **12-hour session lifetime on its own side of the wire**. It is checked before the token, so an expired session and a wrong one get the -same refusal. There are two exceptions, and both are for the stop. A halt that carries the -session cookie and the halt token is accepted past the lifetime (#983): a console left running -overnight must still be able to stop trading. And a read of Pulse (`/api/pulse`) that carries -this session's cookie is refused as expired, but the refusal carries the HALT control's inputs, -so a console reloaded after the lifetime still shows HALT (#985). Only the holder of the real -session token sees the difference. Nothing else is reopened: no release, no setup write, no other -read, and the release token is still served nowhere. A browser with no cached console is refused -the page itself; it can stop trading with `keel kill` at the terminal. A loopback-only server has -no lifetime, and that asymmetry is the argument rather than an exemption. Restarting `keel serve` -remains the instant revocation gesture in both postures. +same refusal. There are three exceptions, and all of them are for the stop. A halt that carries +the session cookie and the halt token is accepted past the lifetime (#983): a console left running +overnight must still be able to stop trading. A read of Pulse (`/api/pulse`) that carries this +session's cookie is refused as expired, but the refusal carries the HALT control's inputs, so a +console reloaded after the lifetime still shows HALT (#985). And the console's own files (the +page, its scripts, stylesheet and icons) are served past the lifetime to a request carrying this +session's cookie (#1006), so a browser with no cached console — or one that opens the address +`keel serve` printed, which sets the cookie — loads the page and reaches that HALT. Those files +are the package's own, unaltered: no data and no token. Only the holder of the real session token +sees any of the difference. Nothing else is reopened: no release, no setup write, no other read, +and the release token is still served nowhere. A browser without the cookie is refused the page +exactly as before the lifetime ended; it can stop trading with `keel kill` at the terminal. A +loopback-only server has no lifetime, and that asymmetry is the argument rather than an +exemption. Restarting `keel serve` remains the instant revocation gesture in both postures. ## Capability-increasing actions are refused unless the bind is loopback diff --git a/keel/web/gates.py b/keel/web/gates.py index 57963a95..7748eec0 100644 --- a/keel/web/gates.py +++ b/keel/web/gates.py @@ -203,9 +203,11 @@ class UngatedAction: #: serves a console still showing a Pulse from before the lifetime ended, whose memory holds the #: token, or any client that holds it. A RELOADED console gets the token from `/api/pulse`'s #: expiry refusal, which carries `{halt_token, resume_command}` to a request bearing this -#: session's cookie and to nothing else (`server._expired_pulse_halt`, #985); that refusal still -#: names `keel kill`. A leaked halt token can at worst halt, which `keel resume` at -#: a terminal undoes, and it opens no release (#968). +#: session's cookie and to nothing else (`server._expired_pulse_halt`, #985); the shell that +#: reads it is served past the lifetime to that cookie too, so a browser with no cached console +#: reaches it (`server._is_a_sessioned_shell_read`, #1006). That refusal still names +#: `keel kill`. A leaked halt token can at worst halt, which `keel resume` at a terminal +#: undoes, and it opens no release (#968). HALT = UngatedAction(operation=engage_kill_switch, done=KILL_ENGAGED_LINE) diff --git a/keel/web/security.py b/keel/web/security.py index a38904ef..c765545f 100644 --- a/keel/web/security.py +++ b/keel/web/security.py @@ -130,8 +130,10 @@ #: the token. A reloaded console gets the token from the expiry refusal itself: on `/api/pulse`, #: and for a request carrying this session's cookie only, that refusal carries the HALT control's #: `{halt_token, resume_command}` (`server.KeelHandler._expired_pulse_halt`, #985). It re-opens -#: no read and no other endpoint serves the token. The refusal still names `keel kill`, for a -#: browser with no cached shell, which is refused the page that would draw the button. +#: no read and no other endpoint serves the token. A browser with no cached shell is served the +#: shell's static files past the lifetime, for this session's cookie only, so it reaches that +#: refusal (`server.KeelHandler._is_a_sessioned_shell_read`, #1006); they carry no data and no +#: token. The refusal still names `keel kill`, for a browser without the cookie. REMOTE_SESSION_MAX_AGE_SECONDS = 12 * 60 * 60 #: ⛔ THE BRUTE-FORCE ARITHMETIC, WRITTEN DOWN SO NOBODY ADDS A RATE LIMITER FOR THE WRONG diff --git a/keel/web/server.py b/keel/web/server.py index b93b44de..e5a43e66 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -57,7 +57,7 @@ from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path from typing import Any -from urllib.parse import parse_qs, urlsplit +from urllib.parse import parse_qs, unquote, urlsplit from keel.web import api, events, runtime, staticfiles from keel.web.security import ( @@ -708,7 +708,10 @@ def _admitted(self, *, outlives_the_lifetime: bool = False) -> bool: `outlives_the_lifetime` skips the LIFETIME and nothing else, and exactly one caller sets it: `_post`, for a halt that carries this session's halt token (`_is_a_tokened_halt`, - #983). Every other request -- a release, a setup write, every read -- keeps it. + #983). The one other request that skips it is decided here rather than by `do_GET`, so + that handler is untouched: a read of the shell carrying this session's cookie + (`_is_a_sessioned_shell_read`, #1006). Every other request -- a release, a setup write, + every `/api/` read -- keeps it. """ if not self.cfg.host_policy.permits(self.headers.get("Host")): # DNS rebinding lands exactly here: the packet arrived on loopback, so the bind check @@ -721,13 +724,16 @@ def _admitted(self, *, outlives_the_lifetime: bool = False) -> bool: ) 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 -- with one exception since - # #985: on a GET or HEAD of `/api/pulse` a request bearing THIS session's cookie gets a - # refusal that carries `halt`, and a wrong or missing cookie gets `halt: null`. That is - # acceptable because only the holder of the real 256-bit session token can see the - # difference, and it tells them nothing they could not already use (`_is_a_tokened_halt` - # admits that halt anyway). Every other request still cannot tell the two apart. Only a - # remote-configured server has a lifetime -- see `ServeConfig.session_expired_at`. + # distinguished from a wrong one by which refusal comes back -- with two exceptions. Since + # #985, on a GET or HEAD of `/api/pulse` a request bearing THIS session's cookie gets a + # refusal that carries `halt`, and a wrong or missing cookie gets `halt: null`. Since + # #1006, a GET or HEAD of the shell bearing THIS session's cookie is served it, and a + # wrong or missing cookie is refused as expired. Both are acceptable because only the + # holder of the real 256-bit session token can see the difference, and it tells them + # nothing they could not already use (`_is_a_tokened_halt` admits that halt anyway, and + # the shell is the package's files). Every other request still cannot tell the two + # apart. Only a remote-configured server has a lifetime -- see + # `ServeConfig.session_expired_at`. # # #983, the owner's decision: the halt is exempt. A console left running past the lifetime # refused its own stop, and refusing a stop is the one failure mode this surface must not @@ -738,10 +744,17 @@ def _admitted(self, *, outlives_the_lifetime: bool = False) -> bool: # It serves a console still showing a Pulse from before the lifetime ended (the token is # in its memory), or any client that holds the token. A RELOADED console gets the token # back from this refusal itself, on `/api/pulse` and for this session's cookie only - # (#985, `_expired_pulse_halt`). The refusal still names `keel kill`: a browser with no - # cached shell is refused the page that would draw the button. + # (#985, `_expired_pulse_halt`). A browser with no cached shell gets the page from this + # server past the lifetime too, for this session's cookie only (#1006, + # `_is_a_sessioned_shell_read`), so it reaches that refusal. The refusal still names + # `keel kill`, for a browser without the cookie, which is refused the page as before. expires = self.cfg.session_expired_at - if expires is not None and not outlives_the_lifetime and time.time() >= expires: + if ( + expires is not None + and not outlives_the_lifetime + and time.time() >= expires + and not self._is_a_sessioned_shell_read() + ): self._refuse( 403, "Session expired", @@ -778,6 +791,52 @@ def _admitted(self, *, outlives_the_lifetime: bool = False) -> bool: return False return True + def _is_a_sessioned_shell_read(self) -> bool: + """Whether this request is a GET (or HEAD) of the client shell carrying this session's + cookie -- the read the remote session lifetime does not bind (#1006). + + **Why.** Since #985 an expired `/api/pulse` refusal carries the HALT control to this + session's cookie, but only a console that is already loaded can read it. A browser with no + cached console -- a first visit after the lifetime, through the printed `?token=` address, + or a browser whose cached files were cleared -- was refused the page itself, so it never + drew the button and its only stop was `keel kill` at a terminal. + + **What it re-opens: files, never data.** The shell is `staticfiles.STATIC_ROOT` byte for + byte -- `_serve_static` reads and sends it unaltered, there is no template -- so it carries + no reading of the ledger and no token; the halt token still comes only from the + cookie-matched Pulse refusal (#985), and `gates_token` is served nowhere (#968). The test + is membership in `staticfiles.shell_request_paths`, the paths `_serve_static` answers with + a file: a shipped asset or a name in `CLIENT_ROUTES` / `RETIRED_CLIENT_ROUTES`. The request + path is only compared against that package-side list, never passed to a filesystem call, + so this adds no second user-controlled path flow. The list is walked per call, which is + acceptable because this is reached only once the lifetime has expired. A spelling + `_serve_static` would still resolve (`/js/../js/main.js`) is not in the list, so it is + NOT exempted and is refused as expired: stricter, and fine. Everything under `API_PREFIX` + (`EVENTS_PATH` included) and `SWITCH_PREFIX` is excluded by name first, so no later file + under the static root can shadow one; an unmapped path keeps the lifetime and is refused as + expired, not 404ed. + + **Why this session's cookie, and not any browser (#1006's held question, ruled here).** + Before the lifetime, a request without the cookie is refused the shell + (`test_static_assets_are_behind_the_same_admission_as_every_other_page`: "not exempted + ... just because it holds no secrets today"). Serving it to anyone after expiry would make + an EXPIRED session more open to a stranger than a live one, and a browser without the + cookie gains nothing from the page: the Pulse refusal draws HALT only for the cookie. A + browser that has only the printed address gets the cookie from the token exchange, which + `do_GET` performs before admission, so the first visit is covered. So, as for #985: only + the holder of the real session token can see the difference, and it tells them nothing + they could not already use. + """ + if self.command not in ("GET", "HEAD"): + return False + path = urlsplit(self.path).path + if path.startswith((API_PREFIX, SWITCH_PREFIX)): + return False + if unquote(path) not in staticfiles.shell_request_paths(staticfiles.STATIC_ROOT): + return False + cookies = parse_cookie_header(self.headers.get("Cookie")) + return tokens_match(cookies.get(SESSION_COOKIE), self.cfg.token) + def _expired_pulse_halt(self) -> dict[str, str] | None: """The HALT control's inputs for the expired-session refusal, or `None` (#985). diff --git a/keel/web/staticfiles.py b/keel/web/staticfiles.py index 27c35c44..3b41eec5 100644 --- a/keel/web/staticfiles.py +++ b/keel/web/staticfiles.py @@ -193,7 +193,12 @@ def resolve_client_route(root: Path, url_path: str) -> Path | None: **This is a read, and it widens nothing.** It maps the names in `CLIENT_ROUTES` and `RETIRED_CLIENT_ROUTES` onto one file that is already served at `STATIC_PREFIX + CLIENT_ENTRY`; the same session cookie and the same `Host` check gate it, - because `server.do_GET` checks both before any static path is looked at. + because `server.do_GET` checks both before any static path is looked at. The one difference + from an `/api/` read is the remote session lifetime: a path this function or + `resolve_static_asset` answers is served past it to this session's cookie (#1006, + `server.KeelHandler._is_a_sessioned_shell_read`, which asks `shell_request_paths`, the list of + the paths these two functions answer), because the shell is files and the expired Pulse + refusal it reads is what carries the HALT (#985). """ if not url_path.startswith(STATIC_PREFIX): return None @@ -209,3 +214,28 @@ def resolve_client_route(root: Path, url_path: str) -> Path | None: if not shell.is_file(): return None return shell + + +def shell_request_paths(root: Path) -> frozenset[str]: + """Every request path the shell answers, spelled out (#1006). + + `STATIC_PREFIX + relpath` for each file under `root` that `content_type_for` can label (the + files `resolve_static_asset` would serve), `STATIC_PREFIX` itself, and `STATIC_PREFIX` plus + each name in `CLIENT_ROUTES` and `RETIRED_CLIENT_ROUTES` (the paths `resolve_client_route` + answers with the shell). + + **Why enumerate rather than resolve.** `server.KeelHandler._is_a_sessioned_shell_read` asks + "is this request path one the shell answers" and compares against this set, so the request + path is only ever COMPARED, never handed to a filesystem call: what is built here comes from + the package's own directory listing. Resolving the path instead would be a second + user-controlled flow into `resolve_static_asset`. The walk is per call, which is cheap + enough because its one caller is reached only once the remote session lifetime has expired. + """ + names = { + STATIC_PREFIX + path.relative_to(root).as_posix() + for path in root.rglob("*") + if path.is_file() and content_type_for(path) is not None + } + names.add(STATIC_PREFIX) + names.update(STATIC_PREFIX + name for name in (*CLIENT_ROUTES, *RETIRED_CLIENT_ROUTES)) + return frozenset(names) diff --git a/tests/web/test_server.py b/tests/web/test_server.py index c196d786..a2ace3a2 100644 --- a/tests/web/test_server.py +++ b/tests/web/test_server.py @@ -1499,18 +1499,27 @@ def test_an_expired_remote_session_is_refused_even_with_a_valid_token(deployment 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". + + Asserted on a data read. The shell's own files are served past the lifetime to this + session's cookie since #1006 (they carry no data and no token), so `/` no longer shows the + bound; `test_past_the_lifetime_only_the_shell_is_reopened` covers what else keeps it. """ 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}") + status, _headers, _body = _request( + fresh, "/api/config", 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}") + status, _headers, body = _request( + old, "/api/config", cookie=f"{SESSION_COOKIE}={old.token}" + ) assert status == 403 - assert "lifetime" in body + assert json.loads(body)["error"]["title"] == "Session expired" + assert "lifetime" in json.loads(body)["error"]["detail"] def test_a_loopback_session_of_the_same_age_still_answers(deployment) -> None: @@ -2838,8 +2847,9 @@ def test_the_session_expired_refusal_names_the_terminal_commands_it_tells_the_op ) -> None: """The refusal tells the operator how to get a working console back and how to stop trading from a terminal (#983). Since #985 a reloaded console also gets its HALT button back from this - same refusal on `/api/pulse`, but a browser with no cached shell is refused the page itself, - so the sentence naming `keel kill` stays.""" + same refusal on `/api/pulse`, and since #1006 a browser with no cached shell is served the + page for this session's cookie; a browser without the cookie is still refused the page, so the + sentence naming `keel kill` stays.""" with _serving(deployment, external_hosts=_REMOTE, started_at=_expired()) as cfg: status, _headers, body = _request(cfg, "/api/pulse", cookie=_session(cfg)) @@ -3040,6 +3050,180 @@ def test_an_expired_remote_session_is_refused_everything_but_the_halt( assert _kill_switch(deployment[0]) is True +# -- the shell outlives the remote session lifetime, for this session's cookie (#1006) ------------- + + +def _shell_paths() -> list[str]: + """Every path the client is served at: each shipped file under the static root, the mount + itself, and every client route (live and retired). Enumerated from the package rather than + listed, so a file added later is covered without editing this test.""" + from keel.web import staticfiles + + root = staticfiles.STATIC_ROOT + files = sorted( + _P(path.relative_to(root).as_posix()) + for path in root.rglob("*") + if path.is_file() and staticfiles.content_type_for(path) is not None + ) + routes = [ + _P(name) for name in ("", *staticfiles.CLIENT_ROUTES, *staticfiles.RETIRED_CLIENT_ROUTES) + ] + return files + routes + + +def _tokens(cfg) -> dict[str, str]: + return { + "session": cfg.token, + "halt": security.halt_token(cfg.token), + "gates": security.gates_token(cfg.token), + "csrf": _csrf(cfg), + } + + +def test_the_shell_is_served_to_its_own_session_past_the_lifetime(deployment) -> None: + """**#1006.** A browser with no cached console was refused the page itself after the 12 h + lifetime, so it never reached the expired-Pulse HALT that #985 serves. Every shell path -- + each shipped file, the mount, every client route -- answers 200 to THIS session's cookie past + the lifetime, with the content type it has before it, and no token of any kind is in any of + them: the shell is the package's files byte for byte, carrying no data and no credential.""" + paths = _shell_paths() + assert len(paths) >= 20, paths + assert _P("index.html") in paths and _P("js/main.js") in paths and _P("sw.js") in paths + with _serving(deployment, external_hosts=_REMOTE, started_at=_expired()) as cfg: + answers = {path: _request(cfg, path, cookie=_session(cfg)) for path in paths} + tokens = _tokens(cfg) + + for path, (status, headers, body) in answers.items(): + assert status == 200, (path, body[:200]) + assert headers["Content-Type"] != "application/json; charset=utf-8", path + for name, token in tokens.items(): + assert token not in body, (path, name) + assert answers[_P("index.html")][2] == answers[_P("")][2] == answers[_P("pulse")][2] + + +def test_a_head_of_the_shell_is_served_to_its_own_session_past_the_lifetime(deployment) -> None: + """The verb check admits HEAD as well as GET: a HEAD of a shell path with this session's + cookie answers 200 with no body, past the lifetime.""" + with _serving(deployment, external_hosts=_REMOTE, started_at=_expired()) as cfg: + status, headers, body = _request(cfg, _P("index.html"), method="HEAD", cookie=_session(cfg)) + + assert status == 200 + assert headers["Content-Type"].startswith("text/html") + assert body == "" + + +@pytest.mark.parametrize( + "cookie", + [None, f"{SESSION_COOKIE}=not-this-session"], + ids=["no-cookie", "wrong-cookie"], +) +def test_the_shell_past_the_lifetime_is_refused_without_the_session(deployment, cookie) -> None: + """The exemption is for this session's cookie only, as #985's is: a stranger past the lifetime + is refused the shell exactly as before (and before the lifetime they are refused it too, by + `test_static_assets_are_behind_the_same_admission_as_every_other_page`). Same refusal text for + every path, so a stranger cannot tell a shell path from any other by the answer.""" + paths = _shell_paths() + with _serving(deployment, external_hosts=_REMOTE, started_at=_expired()) as cfg: + answers = {path: _request(cfg, path, cookie=cookie) for path in paths} + tokens = _tokens(cfg) + + assert {status for status, _h, _b in answers.values()} == {403} + assert {body.split("\n", 1)[0] for _s, _h, body in answers.values()} == {"Session expired"} + for path, (_status, _headers, body) in answers.items(): + for name, token in tokens.items(): + assert token not in body, (path, name) + + +def test_a_token_link_past_the_lifetime_reaches_the_shell(deployment) -> None: + """The first visit in #1006's title: a browser that has never held the cookie opens the + address keel printed. The token exchange already happened before admission, so it gets the + cookie; the shell it is redirected to is now served rather than refused, and its Pulse read + then carries the HALT (#985).""" + with _serving(deployment, external_hosts=_REMOTE, started_at=_expired()) as cfg: + status, headers, _body = _request(cfg, f"/?token={cfg.token}") + assert status == 303 + cookie = headers["Set-Cookie"].split(";", 1)[0] + assert cookie == _session(cfg) + shell_status, _h, shell = _request(cfg, headers["Location"], cookie=cookie) + pulse_status, _h, pulse = _request(cfg, "/api/pulse", cookie=cookie) + + assert shell_status == 200, shell[:200] + assert '