From 032e2671214108628b51d3847a55ebe33e80f46c Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 1 Sep 2026 09:25:44 -0400 Subject: [PATCH] feat(web): expect a tunnel's own hostname, one name at a time (#648, partial) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⚠️ THIS DOES NOT COMPLETE #648 AND NOTHING REMOTE SHOULD SHIP ON IT. Two of the five requirements are met; three are not, and they are named at the top of `docs/remote-access.md` rather than left to be discovered by someone who read a changelog line and exposed a console. WHAT IS DONE. A Cloudflare Tunnel forwards to loopback and presents the app's PUBLIC domain in `Host:`. The DNS-rebinding defence refuses it -- correctly, and for exactly the same reason it refuses `evil.example`: from inside the process the two are indistinguishable, both being names that resolve to a machine this server did not bind. Nothing in the request can tell them apart. Only the operator can, so only the operator can name one: keel serve --external-host keel.example.com EXTENDS, NEVER REPLACES. Checked before the bind-derived rules and changing neither: every loopback spelling still answers, every unlisted name is still refused, and the port check still applies to a proxied request. Matching is case-insensitive because DNS is, and an operator whose correct-looking config silently refused everything would have no way to see why. NO WILDCARDS, REFUSED AT STARTUP. `*`, `*.example.com`, `.example.com`, `0.0.0.0`, `::`, `any`, `all` and the empty string all raise. A wildcard is not a wider expectation, it is the ABSENCE of one -- `*` answers `evil.example` exactly as readily as the operator's own domain, which is the attack. A leading-dot suffix admits every subdomain an attacker can provision, and a tunnel presents one name. THE BUG ITS OWN TEST FOUND, kept because it is the whole argument for testing a control rather than reading it. `ServeConfig.host_policy` is a lazy property, so `HostPolicy.__post_init__`'s wildcard guard fired on the first REQUEST rather than at startup: `--external-host '*.example.com'` started cleanly, said nothing, and would have raised somewhere inside a handler. The command now builds a policy eagerly and turns the refusal into `click.BadParameter`. THE THREAT MODEL, WRITTEN DOWN. `docs/remote-access.md` states what each transport actually guarantees rather than "it's encrypted". A WireGuard mesh is end-to-end between the operator's own devices. A Cloudflare Tunnel TERMINATES TLS AT CLOUDFLARE -- so in that mode the operator is not trusting Cloudflare to route traffic, they are handing it the readable contents of a console showing positions, balances and attestations. That may be an acceptable trade. It is not a neutral one, and a document that let someone find out later would have failed. STILL OPEN, and each is a real gap: session-token entropy and rate-limiting against an attacker who can reach the origin at all (on loopback there was no attacker to rate-limit); secure-context re-verification of the service worker and manifest over HTTPS rather than assumed from `http://127.0.0.1`'s by-specification secure context; and a decision about `--host 0.0.0.0`, which today produces a server that refuses every request because `HostPolicy` then expects `Host: 0.0.0.0` -- fails closed, which is the safe direction, but fails confusingly. 10 mutants, 10 killed. The last was CLI-side normalisation, invisible until a test typed the name the way a shell or a config file hands it over. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NzuKAe2RVrPt9acVAWjRyL --- docs/architecture.md | 10 ++- docs/remote-access.md | 82 +++++++++++++++++++++ keel/commands/serve.py | 34 ++++++++- keel/web/security.py | 45 ++++++++++++ keel/web/server.py | 7 +- tests/web/test_security.py | 83 +++++++++++++++++++++ tests/web/test_serve_command.py | 123 ++++++++++++++++++++++++++++++++ 7 files changed, 378 insertions(+), 6 deletions(-) create mode 100644 docs/remote-access.md diff --git a/docs/architecture.md b/docs/architecture.md index ca7a9690..18ea4f88 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -73,9 +73,13 @@ is a load-bearing property across the codebase. ## 4. Security architecture -- **Loopback bind by design** with Host/bind checks against DNS rebinding; remote - exposure is deliberately absent until #648 lands the opt-in bind, tunnel Host - allowlist, and off-loopback token posture. +- **Loopback bind by design** with Host/bind checks against DNS rebinding. The tunnel + Host allowlist is in (`keel serve --external-host`, one specific name each, wildcards + refused at startup) and the threat model for each transport is written down in + [`docs/remote-access.md`](remote-access.md) — including that a Cloudflare Tunnel + terminates TLS at its edge. **Remote exposure is still deliberately absent**: #648's + off-loopback token posture and secure-context re-verification are not done, and that + document says so at the top rather than leaving it to be discovered. - **One-time session token** URLs; CSP `default-src 'self'` with no `unsafe-inline` (pinned by test); the key-parity scanner ensures the payload emits every key the client reads. diff --git a/docs/remote-access.md b/docs/remote-access.md new file mode 100644 index 00000000..3e31276c --- /dev/null +++ b/docs/remote-access.md @@ -0,0 +1,82 @@ +# Reaching the console from another device + +`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. + +## What the defence actually defends + +`keel serve` answers on `127.0.0.1`, and a browser treats `http://127.0.0.1` as a secure +context by specification, which is what makes the service worker and the manifest work over +plain HTTP. Binding loopback is not, by itself, protection: any page in the operator's browser +can issue requests to `127.0.0.1`, and an attacker who points `evil.example` at `127.0.0.1` +gets a connection that arrives on loopback with every network-level check satisfied. That is +DNS rebinding, and the `Host:` header is the only place the lie is visible. + +So `HostPolicy` refuses any `Host:` the bind does not account for. `evil.example` is refused +even though the packet came from loopback, because the name is not one this server bound. + +## Why a tunnel is refused by default, and why that is correct + +A Cloudflare Tunnel forwards to loopback and presents the app's **public domain** in `Host:`. +From inside the process that is indistinguishable from the rebinding attempt above: both are +names that resolve to a machine this server did not bind. Nothing in the request tells them +apart, and nothing can — only the operator knows which name is theirs. + +That is why the name has to be typed: + +``` +keel serve --external-host keel.example.com +``` + +One specific name, repeatable for more than one. Wildcards are refused **when the server +starts**, not per request: `*`, `*.example.com`, `.example.com`, `0.0.0.0`, `::`, `any`, `all` +and the empty string all raise. A wildcard is not a wider expectation, it is the absence of +one — `*` answers `evil.example` exactly as readily as the operator's own domain, which is the +attack itself. A leading-dot suffix is refused for the same reason: it admits every subdomain +an attacker can provision, and a tunnel presents one name. + +Naming a host **extends** the defence and never replaces it. Every loopback spelling still +answers, every unlisted name is still refused, and the port must still match — a proxied +request is not exempt from being addressed to this server. + +## What each transport actually guarantees + +Stated plainly, because "it's encrypted" is not a threat model. + +| | what protects the traffic | who can read it | what the operator is trusting | +| :-- | :-- | :-- | :-- | +| **Loopback only** | nothing leaves the machine | anything running as the operator | the machine itself | +| **WireGuard mesh** (Tailscale/Headscale) | end-to-end between the operator's own devices | only those devices | the mesh's coordination server for key distribution — not for content | +| **Cloudflare Tunnel** | TLS from the browser to Cloudflare, then Cloudflare to the machine | **Cloudflare, in the clear at its edge** | Cloudflare with the plaintext of every page and every session token | + +The third row is the one that needs saying out loud. Cloudflare terminates TLS. In that mode +the operator is not merely trusting Cloudflare to route traffic; they are handing it the +readable contents of a console that displays positions, balances and attestations. That may be +an acceptable trade for convenience. It is not a neutral one, and a document that let someone +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. + +## ⛔ Not done — do not expose the console yet + +Three of #648's requirements remain, and each is a real gap rather than 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. diff --git a/keel/commands/serve.py b/keel/commands/serve.py index 0964f95c..efb0be56 100644 --- a/keel/commands/serve.py +++ b/keel/commands/serve.py @@ -20,7 +20,7 @@ import click from keel.commands._common import default_config_path, default_db_path -from keel.web.security import new_session_token +from keel.web.security import HostPolicy, new_session_token from keel.web.server import ServeConfig, serve #: Not 8080. Freqtrade's FreqUI and Jesse's dashboard both sit there, and an operator running one @@ -40,8 +40,25 @@ show_default=True, help="Open the URL in your default browser.", ) +@click.option( + "--external-host", + "external_hosts", + multiple=True, + metavar="HOSTNAME", + help=( + "A hostname a reverse proxy (e.g. a Cloudflare Tunnel) may present in Host:. " + "Repeatable. One specific name each -- wildcards are refused. Default: none, and " + "leaving it that way keeps this server loopback-only." + ), +) @click.pass_context -def serve_cmd(ctx: click.Context, host: str, port: int, open_browser: bool) -> None: +def serve_cmd( + ctx: click.Context, + host: str, + port: int, + open_browser: bool, + external_hosts: tuple[str, ...], +) -> None: """Serve keel's read-only view on localhost and open it in your browser. Read-only, by construction: the server implements GET and HEAD and nothing else, so there is @@ -63,9 +80,22 @@ def serve_cmd(ctx: click.Context, host: str, port: int, open_browser: bool) -> N # forks a subprocess to answer "which build is this" would make the cheapest question on the # server the most expensive one. build = _build_info() + # #648. Normalised HERE rather than in `HostPolicy`, so the policy compares two values that + # are already in the same case and the comparison stays a plain set membership. + cleaned = frozenset(name.strip().lower() for name in external_hosts if name.strip()) + # BUILT EAGERLY, and the reason is a bug this caught in its own first draft: `host_policy` is + # a lazy property, so `HostPolicy.__post_init__`'s wildcard guard fired on the first REQUEST + # rather than at startup -- a `--external-host '*.example.com'` server started cleanly, said + # nothing, and would have raised somewhere inside a handler. Constructing one here turns that + # into the command refusing to start, which is what the guard was written to mean. + try: + HostPolicy(bound_host=host, port=port, external_hosts=cleaned) + except ValueError as exc: + raise click.BadParameter(str(exc), param_hint="--external-host") from exc cfg = ServeConfig( host=host, port=port, + external_hosts=cleaned, token=new_session_token(), db_path=obj.get("db_path") or default_db_path(), config_path=obj.get("config_path") or default_config_path(), diff --git a/keel/web/security.py b/keel/web/security.py index 5e1bcf77..a80367a8 100644 --- a/keel/web/security.py +++ b/keel/web/security.py @@ -108,6 +108,29 @@ #: 127.0.0.1 -- is rejected, which is the entire point of checking the header at all. _LOOPBACK_NAMES = frozenset({"127.0.0.1", "localhost", "::1", "[::1]"}) +#: Spellings that mean "stop checking". Refused at construction rather than at request time so a +#: configuration that would disable the defence fails when the server STARTS -- visibly, once -- +#: instead of quietly answering everything for as long as it runs. +_WILDCARD_NAMES = frozenset({"*", "", "any", "all", "0.0.0.0", "::", "[::]"}) + + +def _reject_wildcard(name: str) -> None: + """Refuse an external host that is not one specific name. + + The DNS-rebinding defence works by naming what is expected. A wildcard is not a wider + expectation, it is the absence of one, and a `*` here would answer `evil.example` exactly as + readily as the operator's own domain -- which is the whole attack. A leading-dot suffix + (`.example.com`) is refused for the same reason: it admits every subdomain an attacker can + provision, and a tunnel presents ONE name. + """ + cleaned = name.strip().lower() + if cleaned in _WILDCARD_NAMES or "*" in cleaned or cleaned.startswith("."): + raise ValueError( + f"external host {name!r} is a wildcard, not a name. The DNS-rebinding defence works " + "by naming exactly what is expected; a wildcard removes the check rather than " + "widening it. List the tunnel's own hostname." + ) + def new_session_token() -> str: """A fresh token for one `keel serve` run. Never written to disk: a token that outlives the @@ -192,6 +215,23 @@ class HostPolicy: bound_host: str port: int + #: Names a REVERSE PROXY may legitimately present that this server never bound (#648). + #: + #: A Cloudflare Tunnel forwards to loopback and passes the app's PUBLIC domain in `Host:`, so + #: the rebinding check refuses it -- correctly, and for exactly the same reason it refuses + #: `evil.example`. From inside the process the two are indistinguishable: both are names that + #: resolve to a machine this server did not bind. Only the OPERATOR can tell them apart, so + #: only the operator can name one, one at a time, in configuration. + #: + #: ⚠️ This EXTENDS the defence and never replaces it. Empty by default; a name is admitted + #: only by being listed; there is no wildcard and no "any" -- `_reject_wildcard` refuses the + #: spellings someone reaches for when a specific name is inconvenient. The port check still + #: applies to a proxied request the same as to a direct one. + external_hosts: frozenset[str] = frozenset() + + def __post_init__(self) -> None: + for name in self.external_hosts: + _reject_wildcard(name) @property def is_loopback(self) -> bool: @@ -207,6 +247,11 @@ def permits(self, host_header: str | None) -> bool: name, port = split_host_header(host_header) if port is not None and port != str(self.port): return False + # Checked BEFORE the bind-derived rules, and it changes neither: an allowlisted name is + # an addition to what the bind permits, so the loopback and explicit-address branches + # below answer exactly what they answered before this field existed. + if name.lower() in self.external_hosts: + return True if self.is_loopback: return name in _LOOPBACK_NAMES return name == self.bound_host diff --git a/keel/web/server.py b/keel/web/server.py index 9651635d..1e046313 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -102,10 +102,15 @@ class ServeConfig: #: `Any` rather than the real type for the same reason `api.load_config` returns `Any` -- this #: module names service objects loosely so that importing `keel/web/` stays cheap. build_info: Any = None + #: 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() @property def host_policy(self) -> HostPolicy: - return HostPolicy(bound_host=self.host, port=self.port) + return HostPolicy( + bound_host=self.host, port=self.port, external_hosts=self.external_hosts + ) def url(self) -> str: host = f"[{self.host}]" if ":" in self.host else self.host diff --git a/tests/web/test_security.py b/tests/web/test_security.py index 96bdf275..481edd13 100644 --- a/tests/web/test_security.py +++ b/tests/web/test_security.py @@ -182,3 +182,86 @@ def test_the_cookie_outlives_the_browser_and_dies_with_the_run() -> None: "two serve runs handed out the same cookie -- the session token is being reused across " "processes, which is the persisted-secret design #634 declined" ) + + +# -- external hosts: a tunnel's own name, and nothing wider (#648) -------------------------------- + + +def test_an_allowlisted_external_host_is_admitted() -> None: + """A Cloudflare Tunnel forwards to loopback and presents the app's PUBLIC domain. + + From inside the process that is indistinguishable from a rebinding attempt -- both are names + that resolve to a machine this server did not bind. Only the operator can tell them apart, + which is why the name has to be configured rather than inferred. + """ + policy = HostPolicy( + bound_host="127.0.0.1", port=8765, external_hosts=frozenset({"keel.example.com"}) + ) + assert policy.permits("keel.example.com:8765") + assert policy.permits("keel.example.com") + + +def test_allowlisting_one_name_admits_no_other() -> None: + """**The pin that matters.** The defence is not weakened, it is extended by exactly one name.""" + policy = HostPolicy( + bound_host="127.0.0.1", port=8765, external_hosts=frozenset({"keel.example.com"}) + ) + assert not policy.permits("evil.example:8765") + assert not policy.permits("keel.example.com.evil.example:8765") + assert not policy.permits("sub.keel.example.com:8765") + + +def test_the_loopback_rules_are_unchanged_by_an_allowlist() -> None: + """Adding an external name must not disturb what the bind already permitted, in either + direction: every loopback spelling still answers, and a rebinding attempt still does not.""" + policy = HostPolicy( + bound_host="127.0.0.1", port=8765, external_hosts=frozenset({"keel.example.com"}) + ) + for host in ("127.0.0.1:8765", "localhost:8765", "[::1]:8765"): + assert policy.permits(host), host + assert not policy.permits("evil.example:8765") + assert not policy.permits(None) + + +def test_the_port_check_still_applies_to_a_proxied_request() -> None: + """A proxied request is not exempt from being addressed to THIS server.""" + policy = HostPolicy( + bound_host="127.0.0.1", port=8765, external_hosts=frozenset({"keel.example.com"}) + ) + assert not policy.permits("keel.example.com:9000") + + +def test_an_external_host_is_matched_case_insensitively() -> None: + """DNS is case-insensitive and a proxy may present any casing. Refusing on case would be a + defence that fails open in the operator's head -- they configured the name, it looks right, + and requests are refused for a reason nothing reports.""" + policy = HostPolicy( + bound_host="127.0.0.1", port=8765, external_hosts=frozenset({"keel.example.com"}) + ) + assert policy.permits("KEEL.Example.COM:8765") + + +@pytest.mark.parametrize( + "wildcard", ["*", "*.example.com", ".example.com", "0.0.0.0", "::", "any", "all", ""] +) +def test_a_wildcard_external_host_is_refused_at_construction(wildcard: str) -> None: + """A wildcard is not a wider expectation -- it is the ABSENCE of one. + + The rebinding defence works by naming what is expected, so `*` would answer `evil.example` + exactly as readily as the operator's own domain, which is the attack itself. A leading-dot + suffix is refused for the same reason: it admits every subdomain an attacker can provision, + and a tunnel presents one name. + + Refused at CONSTRUCTION, not per request, so a configuration that would disable the defence + fails when the server starts -- visibly, once -- instead of quietly answering everything for + as long as it runs. + """ + with pytest.raises(ValueError, match="wildcard"): + HostPolicy(bound_host="127.0.0.1", port=8765, external_hosts=frozenset({wildcard})) + + +def test_no_external_hosts_is_the_default() -> None: + """Loopback-only is the posture, and remaining the posture unless someone types a name is + the point. A default that admitted anything would make every other test here decoration.""" + assert HostPolicy(bound_host="127.0.0.1", port=8765).external_hosts == frozenset() + assert not HostPolicy(bound_host="127.0.0.1", port=8765).permits("keel.example.com:8765") diff --git a/tests/web/test_serve_command.py b/tests/web/test_serve_command.py index 1ce5e325..ca2c8a22 100644 --- a/tests/web/test_serve_command.py +++ b/tests/web/test_serve_command.py @@ -116,3 +116,126 @@ def _boom(_cfg: Any) -> None: ) assert web_server.serve(cfg, echo=lines.append) == 1 assert "could not bind 127.0.0.1:8765" in "\n".join(lines) + + +# -- --external-host reaches the policy (#648) ---------------------------------------------------- + + +def _policy_from_cli(monkeypatch: pytest.MonkeyPatch, tmp_path: Path, *args: str): + """Invoke `keel serve` for real and hand back the `HostPolicy` it built. + + Captured from the `ServeConfig` the command actually constructs, because the option being + parsed proves nothing about it reaching the check -- a flag wired to a field nothing reads + is a security control that exists only in `--help`. + """ + captured: dict[str, object] = {} + + class _Stub(_StubServer): + pass + + def build(cfg): # type: ignore[no-untyped-def] + captured["cfg"] = cfg + return _Stub((cfg.host, cfg.port)) + + monkeypatch.setattr(web_server, "ensure_schema", lambda _path: None) + monkeypatch.setattr(web_server, "build_server", build) + result = CliRunner().invoke( + cli, + ["--db", str(tmp_path / "keel.db"), "serve", "--no-open", *args], + ) + return result, captured.get("cfg") + + +def test_external_host_from_the_cli_reaches_the_host_policy( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + result, cfg = _policy_from_cli( + monkeypatch, tmp_path, "--external-host", "keel.example.com" + ) + + assert result.exit_code == 0, result.output + assert cfg is not None + policy = cfg.host_policy + assert policy.permits("keel.example.com:8765"), ( + "--external-host was accepted by Click and never reached the Host check" + ) + assert not policy.permits("evil.example:8765") + assert policy.permits("127.0.0.1:8765"), "the loopback rules must be untouched" + + +def test_no_external_host_flag_leaves_the_server_loopback_only( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """The default is the posture. If this ever passes an external name, the flag has grown a + default and every other test in this section is decoration.""" + result, cfg = _policy_from_cli(monkeypatch, tmp_path) + + assert result.exit_code == 0, result.output + assert cfg is not None + assert cfg.external_hosts == frozenset() + assert not cfg.host_policy.permits("keel.example.com:8765") + + +def test_a_wildcard_external_host_stops_the_server_starting( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Refused at startup, visibly, once -- not per request. A server that answered everything + for an hour before anyone noticed is the failure the defence exists to prevent.""" + result, _cfg = _policy_from_cli(monkeypatch, tmp_path, "--external-host", "*.example.com") + + assert result.exit_code != 0, result.output + + +def test_repeating_the_flag_admits_each_name( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + result, cfg = _policy_from_cli( + monkeypatch, + tmp_path, + "--external-host", + "a.example.com", + "--external-host", + "b.example.com", + ) + + assert result.exit_code == 0, result.output + assert cfg is not None + assert cfg.host_policy.permits("a.example.com:8765") + assert cfg.host_policy.permits("b.example.com:8765") + assert not cfg.host_policy.permits("c.example.com:8765") + + +def test_the_flag_normalises_case_and_whitespace_before_the_policy_sees_it( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A name typed with the casing DNS allows, or pasted with a stray space, must still work. + + `HostPolicy.permits` lowercases the incoming `Host:` and then compares by set membership, so + an un-normalised entry silently never matches: the operator configured the name, it looks + right in `--help`, and every request is refused for a reason nothing reports. Normalising at + the boundary is what keeps the comparison a plain membership test rather than a loop. + """ + result, cfg = _policy_from_cli( + monkeypatch, tmp_path, "--external-host", " KEEL.Example.COM " + ) + + assert result.exit_code == 0, result.output + assert cfg is not None + assert cfg.external_hosts == frozenset({"keel.example.com"}) + assert cfg.host_policy.permits("keel.example.com:8765") + + +def test_an_empty_flag_value_is_dropped_rather_than_refused( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """`--external-host ''` is an unset variable expanding, not a request to serve everything. + + It must not reach `HostPolicy`, where the empty string is a wildcard spelling and would stop + the server -- and it must not become an allowlist entry either. Dropping it leaves the + default posture, which is what an empty value meant. + """ + result, cfg = _policy_from_cli(monkeypatch, tmp_path, "--external-host", "") + + assert result.exit_code == 0, result.output + assert cfg is not None + assert cfg.external_hosts == frozenset()