From 14b1428f9fd5300bef9c91cb8a2127a5413abb92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Bene=C5=A1?= Date: Thu, 6 Aug 2026 10:07:36 +0200 Subject: [PATCH] A connection that cannot point at the machine it is running on "Nothing runs on the LLeMbas host" is the sentence the absent sandbox and the absent local MCP rest on, and an SSH profile aimed at 127.0.0.1 walked straight past it -- through a real login, with every gate in policy.py still applying, onto the machine holding the database and the Fernet key. From the SSH layer down it is indistinguishable from a container on the network, so nothing here could have noticed. One switch, three positions: never, one named port, anywhere. The middle one is the one with a real use -- a container that published its SSH port on the loopback interface is genuinely somewhere else -- and port 22 is refused even there, because that one is this host's own sshd. Enforced in five places, because a row can predate a setting: saving a profile, `session.resolve` (the control every agent tool, the terminal and the canvas go through), the composer's picker, browsing, and the draft the panels open against before a chat exists. Check refuses before it opens its socket rather than after. And the recognition never resolves a name on the request path. `refusal` runs several times per page render; the first version of this looked names up inline and the suite went from two minutes to not finishing. Literal forms are decided from the string, a name is settled where a network call is already expected, and the answer lives on the row. The gap that leaves is written down rather than discovered. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 29 ++- PLAN.md | 5 + src/lembas/__init__.py | 2 +- src/lembas/api/admin_agents.py | 30 ++- src/lembas/api/agents.py | 46 +++- src/lembas/api/pages.py | 13 +- src/lembas/db/models/agent.py | 14 + src/lembas/services/agent/hosts.py | 241 ++++++++++++++++++ src/lembas/services/agent/session.py | 8 +- src/lembas/services/settings_store.py | 18 ++ src/lembas/web/templates/admin/agents.html | 33 +++ src/lembas/web/templates/agents/detail.html | 7 + src/lembas/web/templates/agents/index.html | 15 +- tests/conftest.py | 13 + tests/test_agent_hosts.py | 269 ++++++++++++++++++++ 15 files changed, 731 insertions(+), 12 deletions(-) create mode 100644 src/lembas/services/agent/hosts.py create mode 100644 tests/test_agent_hosts.py diff --git a/CLAUDE.md b/CLAUDE.md index 6bac36e..70895d7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -20,7 +20,7 @@ lembas info # paths + counts, useful when confused lembas secret-key # generate LEMBAS_SECRET_KEY lembas create-admin # create or promote an admin -pytest # 1828 tests, ~2min +pytest # 1854 tests, ~2min # PLAN.md tracks what is and is not built ruff check . # lint (line length 100) python scripts/build_artwork.py # regenerate artwork (SVG + PWA icons; @@ -791,6 +791,33 @@ meet it. Exactly what `--header-height` already does at the top of the shell. A composer that grows past it as somebody types is expected; nothing is pretending the sidebar should follow. +**An SSH connection may not point at this machine unless an administrator says +so.** "Nothing runs on the LLeMbas host" is the sentence the absent sandbox, the +absent local MCP and the whole security story rest on — and a profile pointed at +`127.0.0.1` walked straight past it, looking from the SSH layer down exactly +like a container on the network. `services/agent/hosts.py` is one switch in +three positions: **off** (the default, and the default on upgrade), **port** +(one named port, for a container that published SSH on the loopback interface — +22 is refused there regardless, being this host's own sshd), and **on**. + +Enforced in five places because a row can predate a setting: saving a profile, +`session.resolve` (the control — every agent tool, the terminal and the canvas +go through it), the composer's picker, `browse`, and the draft endpoint that the +panels open against before a chat exists. Check refuses *before* opening its +socket. + +**And the recognition never resolves a name on the request path.** +`is_loopback` reads the string only. A *name* pointing at loopback needs +`getaddrinfo`, which blocks, and `refusal` is called several times per page +render — the first version resolved inline and the suite went from two minutes +to not finishing. So resolution happens where a network call is already expected +(saving, and Check) and is written to `SshProfile.resolves_here`, which the +request path reads for free. The gap this leaves is stated rather than +discovered: a name whose DNS moves *after* it was saved is not noticed until it +is saved or checked again. `tests/conftest.py` runs the suite with the switch +**open**, because the tests that stand up a real asyncssh server can only listen +on loopback; `tests/test_agent_hosts.py` closes it explicitly. + **An empty htmx verb is a request, not a no-op.** htmx looks for the *attribute* — `if(s(t,"hx-"+r))` is `hasAttribute` — so `hx-get=""` is a real request for the empty path, which the browser resolves against the current diff --git a/PLAN.md b/PLAN.md index cc50427..939452d 100644 --- a/PLAN.md +++ b/PLAN.md @@ -400,6 +400,11 @@ seen working. the outer container does, and switching to a shorter panel drops the reader at the bottom of the page - [ ] Sidebar scroll no longer chains to the document +- [x] **A connection may not point at this machine** unless an administrator + says so, in one of three positions — never, one named port, or anywhere. + An SSH profile aimed at `127.0.0.1` walked past the sentence the whole + security story rests on, looking from the SSH layer down exactly like a + container on the network ### Phase 1 — the scheduling tools (`0.9.0`) - [ ] **A model can schedule.** There was no tool for it — the seam was left diff --git a/src/lembas/__init__.py b/src/lembas/__init__.py index bb7289c..3a17f3b 100644 --- a/src/lembas/__init__.py +++ b/src/lembas/__init__.py @@ -1,3 +1,3 @@ """LLeMbas - a Middle-earth themed web UI for OpenAI-compatible LLM endpoints.""" -__version__ = "0.8.2" +__version__ = "0.8.3" diff --git a/src/lembas/api/admin_agents.py b/src/lembas/api/admin_agents.py index ae9b5be..9d1b683 100644 --- a/src/lembas/api/admin_agents.py +++ b/src/lembas/api/admin_agents.py @@ -19,7 +19,7 @@ from sqlalchemy import func, select from lembas.api.deps import AdminUser, Db from lembas.db.models import SshProfile from lembas.services import settings_store -from lembas.services.agent import policy +from lembas.services.agent import hosts, policy from lembas.services.agent import ssh as ssh_service from lembas.services.agent import terminal as terminal_service from lembas.web.templating import render @@ -48,6 +48,18 @@ async def agents_page(request: Request, db: Db, user: AdminUser, saved: bool = F "profile_count": db.scalar(select(func.count()).select_from(SshProfile)) or 0, "terminal_count": terminal_service.count(), "modes": [(m, policy.MODE_LABELS[m], policy.MODE_HINTS[m]) for m in policy.MODES], + "loopback_modes": [ + (m, hosts.MODE_LABELS[m], hosts.MODE_HINTS[m]) for m in hosts.MODES + ], + # How many of this instance's connections the current position would + # stop. The number is the point of the card: "3 connections" beside + # a switch somebody is about to move is the difference between an + # informed change and a surprise. + "loopback_count": sum( + 1 + for p in db.scalars(select(SshProfile)) + if hosts.is_loopback(p.host) or p.resolves_here + ), "saved": saved, }, ) @@ -58,6 +70,8 @@ async def save_agents( db: Db, user: AdminUser, enabled: bool = Form(False), + loopback: str = Form("off"), + loopback_port: int = Form(0), default_timeout: int = Form(60), max_timeout: int = Form(600), max_output_bytes: int = Form(64 * 1024), @@ -88,6 +102,13 @@ async def save_agents( db, { "enabled": enabled, + # Anything unrecognised means off, here as well as on read: the one + # direction safe to get wrong is refusing a connection somebody has + # to re-allow, and the other is a shell on this host. + "loopback": loopback if loopback in hosts.MODES else hosts.MODE_OFF, + # Zero means "none named", which is what `port` needs in order to + # refuse rather than to allow. 22 is refused wherever it is stored. + "loopback_port": loopback_port if 1 <= loopback_port <= 65535 else 0, # Clamped here as well as on read. A number with no bound is a way # to break the instance from a form, which is the same reasoning # the search settings carry. @@ -124,4 +145,11 @@ async def save_agents( key=settings_store.AGENTS, ) log.info("agent execution %s by %s", "enabled" if enabled else "disabled", user.email) + if loopback != hosts.MODE_OFF: + log.warning( + "ssh connections to this machine allowed (%s%s) by %s", + loopback, + f", port {loopback_port}" if loopback == hosts.MODE_PORT else "", + user.email, + ) return RedirectResponse("/admin/agents?saved=1", status_code=status.HTTP_303_SEE_OTHER) diff --git a/src/lembas/api/agents.py b/src/lembas/api/agents.py index 1f49485..63dc6b0 100644 --- a/src/lembas/api/agents.py +++ b/src/lembas/api/agents.py @@ -25,6 +25,7 @@ from lembas.api.pages import sidebar_context from lembas.db.models import AUTH_METHODS, AUTH_PASSWORD, SshProfile from lembas.services import settings_store from lembas.services.agent import draft as draft_service +from lembas.services.agent import hosts from lembas.services.agent import index as index_service from lembas.services.agent import jobs as jobs_service from lembas.services.agent import ssh as ssh_service @@ -119,6 +120,9 @@ def _detail( else "", "has_key": bool(profile.private_key_encrypted), "problem": ssh_service.available(), + # Empty on the new-connection page, where there is no host yet to + # ask about -- the answer arrives when it is submitted. + "refused": hosts.refusal_for(db, profile) if profile.host else "", }, ) @@ -130,7 +134,11 @@ async def agents_page(request: Request, db: Db, user: RequiredUser, saved: str = "agents/index.html", { **sidebar_context(db, user), - "profiles": _owned(db, user.id), + "profiles": (owned := _owned(db, user.id)), + # Keyed by id rather than resolved in the template, because the + # template has no session and this is a question about instance + # settings, not about the row. + "refusals": {p.id: hosts.refusal_for(db, p) for p in owned}, "saved": saved, "problem": ssh_service.available(), "enabled": bool(settings_store.agents(db).get("enabled")), @@ -179,6 +187,18 @@ def _problem(db: Db, profile: SshProfile, owner_id: str, *, existing_id: str = " if not profile.username: return "A connection needs a username to log in as." + # Saving is one of the two moments a DNS lookup is affordable, so this is + # where a *name* pointing at loopback is settled and written to the row for + # every later request to read for free. See services/agent/hosts.py. + # + # Not the last word -- `session.resolve` refuses one that was saved before an + # administrator moved the switch, and has to, because a row can predate a + # setting. This is here so the refusal arrives while somebody is looking at + # the form that caused it rather than at an agent chat with no tools. + resolved = hosts.restamp(profile) + if refused := hosts.refusal(db, profile.host, profile.port, resolved=resolved): + return refused + clash = db.scalar( select(SshProfile).where( SshProfile.owner_id == owner_id, SshProfile.name == profile.name @@ -224,7 +244,11 @@ async def browse_profile( entries: list = [] error = "" - if hint := ssh_service.available(): + if refused := hosts.refusal_for(db, profile): + # First, because this one opens a connection and the others only explain + # why one would fail. + error = refused + elif hint := ssh_service.available(): error = hint elif not profile.host_key: # connect_kwargs would raise the same thing, but a picker that opens on @@ -300,6 +324,12 @@ async def draft_target(db: Db, user: RequiredUser, profile_id: str, dir: str = " finds the shell already running there rather than opening a second one. """ profile = _profile(db, user, profile_id) + # A draft is what the terminal and the canvas open against before a chat + # exists, so refusing here is refusing the whole new-chat path. `resolve` + # would refuse it anyway once a chat existed; this stops the panel opening + # on a target it will not be allowed to use. + if refused := hosts.refusal_for(db, profile): + raise HTTPException(status.HTTP_403_FORBIDDEN, refused) draft = draft_service.remember(user.id, profile.id, dir or profile.default_dir or "") return {"id": draft.id, "dir": draft.project_dir} @@ -406,6 +436,18 @@ async def check_profile(request: Request, db: Db, user: RequiredUser, profile_id """ profile = _profile(db, user, profile_id) + # Before anything is sent. Check is the one button here that opens a socket, + # so a refused connection must not get one -- and the reason belongs in the + # place somebody just pressed rather than in a log. + # + # The other moment a lookup is affordable, and the one that catches a name + # whose DNS moved after it was saved: this button is how somebody finds out + # a connection has stopped working, so it is the right place to find out why. + hosts.restamp(profile) + db.commit() + if refused := hosts.refusal_for(db, profile): + return render(request, "agents/_check.html", {"profile": profile, "error": refused}) + try: line, fingerprint = await ssh_service.capture_host_key( profile.host, profile.port, timeout=profile.connect_timeout diff --git a/src/lembas/api/pages.py b/src/lembas/api/pages.py index 09296ba..d0fccc7 100644 --- a/src/lembas/api/pages.py +++ b/src/lembas/api/pages.py @@ -206,17 +206,24 @@ def _agent_context(db: DBSession, user: User, chat: Chat | None) -> dict: would lead anywhere. """ from lembas.db.models import SshProfile + from lembas.services.agent import hosts from lembas.services.agent import policy as agent_policy profiles: list[SshProfile] = [] if settings_store.agents(db).get("enabled") and permissions.has(db, user, "tools.agent"): - profiles = list( - db.scalars( + profiles = [ + profile + for profile in db.scalars( select(SshProfile) .where(SshProfile.owner_id == user.id, SshProfile.enabled.is_(True)) .order_by(SshProfile.name) ) - ) + # A connection pointing at this machine that an administrator has not + # allowed is not offered at all. `session.resolve` refuses it too and + # is the control; this is so it never appears in a picker whose only + # outcome is an agent chat with no tools and nothing said about why. + if hosts.usable(db, profile) + ] current = None if chat is not None and chat.ssh_profile_id: diff --git a/src/lembas/db/models/agent.py b/src/lembas/db/models/agent.py index c391f88..718904d 100644 --- a/src/lembas/db/models/agent.py +++ b/src/lembas/db/models/agent.py @@ -53,6 +53,20 @@ class SshProfile(UUIDPrimaryKey, Timestamps, Base): port: Mapped[int] = mapped_column(Integer, default=22, nullable=False) username: Mapped[str] = mapped_column(String(120), nullable=False) + # Whether `host` resolved to loopback the last time anybody looked. Written + # where a network call is already happening -- saving this connection, and + # Check -- and read on every request that asks whether this connection may + # be used at all. A column rather than a lookup because that question is + # asked several times per page render, and `getaddrinfo` on the request path + # makes an agent page wait out a DNS timeout for a host nobody is talking + # to. A literal `127.0.0.1` needs none of this and is decided from the + # string. See services/agent/hosts.py. + # + # False on every row an upgrade brings in, which is correct for the literal + # case (decided from the string anyway) and optimistic for a *name* until it + # is next saved or checked. + resolves_here: Mapped[bool] = mapped_column(Boolean, default=False, nullable=False) + auth: Mapped[str] = mapped_column(String(16), default=AUTH_KEY, nullable=False) password_encrypted: Mapped[str] = mapped_column(Text, default="") private_key_encrypted: Mapped[str] = mapped_column(Text, default="") diff --git a/src/lembas/services/agent/hosts.py b/src/lembas/services/agent/hosts.py new file mode 100644 index 0000000..28ed51b --- /dev/null +++ b/src/lembas/services/agent/hosts.py @@ -0,0 +1,241 @@ +"""Whether an SSH connection is allowed to point back at this machine. + +The whole design of agent chats rests on one sentence: nothing runs on the host +LLeMbas is installed on. That is why there is no local sandbox, why local MCP +over stdio is absent, and why "the security of an agent chat is the security of +the host behind its profile" is a statement anybody can check. + +An SSH profile pointed at `127.0.0.1` walks straight past it. The commands go +over SSH, through a real login, and every gate in `policy.py` still applies -- +and they land on the machine holding the database, the Fernet key and every +other user's encrypted credentials. Nothing else in the codebase can tell that +apart from a container on the network, because from the SSH layer's point of +view it is not different. + +So it is a decision an administrator makes deliberately, in one of three +positions: + +- **off** (the default, including on an instance upgrading into this) -- no + connection may point at loopback, and one that already does is refused rather + than quietly kept working. +- **port** -- allowed on exactly one port. This is the position that has a real + use: a container that publishes its SSH port on the host's loopback interface + is genuinely somewhere else, and `127.0.0.1:2222` is how you reach it. Port 22 + is refused even here, because that is the host's own sshd. +- **on** -- allowed anywhere. For somebody who has read the paragraph above and + means it. + +## Literal or resolved, and never resolved on the request path + +Both are checked, at two different moments, and the split is not tidiness. + +The literal forms -- `127.0.0.1`, `::1`, `localhost`, anything in +`127.0.0.0/8` -- are decided from the string with no I/O at all. That is the +check `refusal` makes, and it is why `refusal` can be called from a page render, +from `resolve_tools` and from the composer's profile listing. + +A *name* that resolves to loopback needs `getaddrinfo`, which is a blocking +network call, and putting one of those behind a check that runs several times +per request is how a page render comes to wait out a DNS timeout for a host +nobody is even talking to. The first version of this file did exactly that and +the test suite went from two minutes to not finishing. So resolution happens +**only where a network call is already expected and already awaited** -- saving +a connection, and pressing Check -- and the answer is written to +`SshProfile.resolves_here`, which the request path reads for free. + +The consequence, stated rather than discovered: a name whose DNS changes to +point here after it was saved is not noticed until it is saved or checked again. +That is a real gap and it is the right trade. The alternative is a DNS lookup in +front of every agent page load, and a guard that makes the application feel +broken is a guard somebody turns off. + +A refusal is never silent. Every caller that has somewhere to put a sentence +puts this one there, because "this connection cannot be used" with no reason is +indistinguishable from a bug. +""" + +from __future__ import annotations + +import ipaddress +import logging +import socket +from typing import TYPE_CHECKING + +if TYPE_CHECKING: # pragma: no cover - typing only + from sqlalchemy.orm import Session as DBSession + + from lembas.db.models import SshProfile + +log = logging.getLogger(__name__) + +MODE_OFF = "off" +MODE_PORT = "port" +MODE_ON = "on" +MODES = (MODE_OFF, MODE_PORT, MODE_ON) + +MODE_LABELS = { + MODE_OFF: "Never", + MODE_PORT: "Only on one port", + MODE_ON: "Anywhere", +} +MODE_HINTS = { + MODE_OFF: ( + "A connection to this machine is refused, and an existing one stops " + "working. This is what keeps “nothing runs on the LLeMbas host” true." + ), + MODE_PORT: ( + "For a container that publishes its SSH port on this machine's loopback " + "interface. Name that port; everything else here is still refused, and " + "port 22 is refused regardless, because that one is this host's own sshd." + ), + MODE_ON: ( + "Any port on this machine. Commands then run beside the database and the " + "encryption key, with whatever the login account can reach." + ), +} + +# The host's own sshd, and never what somebody means by "the container on 2222". +HOST_SSH_PORT = 22 + + +def _literal(host: str) -> bool | None: + """True/False when the host decides itself, None when it needs resolving.""" + text = (host or "").strip().strip("[]").lower() + if not text: + return False + # Not a real hostname anywhere, and the one everybody types. + if text in ("localhost", "localhost.localdomain", "ip6-localhost", "ip6-loopback"): + return True + try: + return ipaddress.ip_address(text).is_loopback + except ValueError: + return None + + +def is_loopback(host: str) -> bool: + """Whether this host *string* reaches the machine LLeMbas is running on. + + No I/O, ever. A name is answered False here and settled by `resolves_here` + at the two moments a lookup is affordable -- see the module docstring; the + version of this that resolved inline made every agent page wait on DNS. + """ + return bool(_literal(host)) + + +def resolves_here(host: str) -> bool: + """The same question for a name, by resolving it. Blocking; call sparingly. + + Resolution failure is answered **False**: a name that does not resolve is not + a name pointing here, and refusing it would turn every DNS hiccup into "your + connection is on this machine", which is both wrong and confusing. The + connection itself will fail on its own terms a moment later. + """ + decided = _literal(host) + if decided is not None: + return decided + + try: + for entry in socket.getaddrinfo((host or "").strip().lower(), None): + if _literal(str(entry[4][0])): + return True + except OSError: + return False + return False + + +def policy(db: DBSession) -> tuple[str, int]: + """The configured position, and the port that goes with `port`.""" + from lembas.services import settings_store + + values = settings_store.agents(db) + mode = str(values.get("loopback") or MODE_OFF) + if mode not in MODES: + mode = MODE_OFF + try: + port = int(values.get("loopback_port") or 0) + except (TypeError, ValueError): + port = 0 + return mode, port + + +def refusal(db: DBSession, host: str, port: int, *, resolved: bool = False) -> str: + """Why this host and port may not be used, or "" if they may. + + A sentence rather than a boolean, because every caller has somewhere to show + one and a connection that is unavailable for no stated reason reads as a + fault in the application. + + `resolved` is what a stored profile's `resolves_here` column carries in: the + string said nothing, and a lookup made earlier said yes. + """ + if not (resolved or is_loopback(host)): + return "" + + mode, allowed = policy(db) + if mode == MODE_ON: + return "" + if mode == MODE_PORT: + if allowed and port == allowed and port != HOST_SSH_PORT: + return "" + if allowed: + return ( + f"This connection points at this machine, which is only allowed " + f"on port {allowed}. An administrator sets that on the Agents page." + ) + return ( + "This connection points at this machine, which is allowed only on a " + "port an administrator has named — and none has been." + ) + return ( + "This connection points at the machine LLeMbas itself runs on, which an " + "administrator has not allowed. Agent chats are meant to reach a " + "different host; running here would put the commands beside the database " + "and the encryption key." + ) + + +def refusal_for(db: DBSession, profile: SshProfile | None) -> str: + """The same answer for a stored profile, with no lookup. + + `resolves_here` is the verdict recorded the last time somebody saved or + checked this connection. Reading it is what keeps this callable from a page + render. + """ + if profile is None: + return "" + return refusal( + db, profile.host, profile.port, resolved=bool(getattr(profile, "resolves_here", False)) + ) + + +def usable(db: DBSession, profile: SshProfile | None) -> bool: + return not refusal_for(db, profile) + + +def restamp(profile: SshProfile) -> bool: + """Record whether this profile's host resolves to loopback, and return it. + + Called where a network call is already happening -- saving a connection, and + Check. The column is the request path's only way of knowing about a *name*, + so a save that skips this leaves the guard reading a stale answer. + """ + profile.resolves_here = resolves_here(profile.host) + return profile.resolves_here + + +__all__ = [ + "HOST_SSH_PORT", + "MODES", + "MODE_HINTS", + "MODE_LABELS", + "MODE_OFF", + "MODE_ON", + "MODE_PORT", + "is_loopback", + "policy", + "refusal", + "refusal_for", + "resolves_here", + "restamp", + "usable", +] diff --git a/src/lembas/services/agent/session.py b/src/lembas/services/agent/session.py index 44d7529..68ade35 100644 --- a/src/lembas/services/agent/session.py +++ b/src/lembas/services/agent/session.py @@ -21,7 +21,7 @@ from sqlalchemy.orm import Session as DBSession from lembas.db.models import KIND_AGENT, Chat, SshProfile, User from lembas.services import settings_store -from lembas.services.agent import policy +from lembas.services.agent import hosts, policy from lembas.services.agent import ssh as ssh_service from lembas.services.agent.base import Executor from lembas.services.agent.policy import Limits @@ -146,6 +146,12 @@ def profile_for(db: DBSession, chat: Chat, user: User | None) -> SshProfile | No return None if user is not None and profile.owner_id != user.id: return None + # A row can predate a setting, so this is asked here rather than trusted + # from when the profile was saved: an administrator moving the switch to + # `off` has to stop the chats already pointed at loopback, not only the next + # one somebody tries to create. See services/agent/hosts.py. + if not hosts.usable(db, profile): + return None return profile diff --git a/src/lembas/services/settings_store.py b/src/lembas/services/settings_store.py index b0f0217..bf7b91a 100644 --- a/src/lembas/services/settings_store.py +++ b/src/lembas/services/settings_store.py @@ -89,6 +89,13 @@ def _agents_defaults() -> dict[str, Any]: # a model reads web pages, files and command output, all of them # untrusted, so a shell is a capability somebody chooses on purpose. "enabled": False, + # Whether a connection may point back at this machine. Off, and off on + # an instance upgrading into this too: a loopback profile walks straight + # past "nothing runs on the LLeMbas host", and that sentence is what the + # absence of a sandbox rests on. See services/agent/hosts.py for the + # three positions and why the middle one exists. + "loopback": "off", + "loopback_port": 0, # Per command. "default_timeout": 60, "max_timeout": 600, @@ -422,6 +429,17 @@ def agents(db: DBSession) -> dict[str, Any]: max(int(values.get("max_completion_tokens") or 0), 0), 5_000_000 ) values["background_max_jobs"] = min(max(int(values.get("background_max_jobs") or 0), 1), 100) + # Anything unrecognised means off. A stored value this version does not know + # must fail closed here: the one direction that is safe to get wrong is + # refusing a connection somebody has to re-allow, and the other direction is + # a shell on this host. + if values.get("loopback") not in ("off", "port", "on"): + values["loopback"] = "off" + try: + port = int(values.get("loopback_port") or 0) + except (TypeError, ValueError): + port = 0 + values["loopback_port"] = port if 1 <= port <= 65535 else 0 return values diff --git a/src/lembas/web/templates/admin/agents.html b/src/lembas/web/templates/admin/agents.html index c9c4600..3fd7d84 100644 --- a/src/lembas/web/templates/admin/agents.html +++ b/src/lembas/web/templates/admin/agents.html @@ -54,6 +54,39 @@

+
+

Connections to this machine

+

+ Agent chats reach a machine over SSH, and the point of that is that it is + not this one — nothing runs on the host holding the database and the + encryption key. A connection pointed at 127.0.0.1 walks past + that, and from the SSH layer's point of view it looks like any other host. + {% if loopback_count %} + {{ loopback_count }} saved connection{{ '' if loopback_count == 1 else 's' }} + point{{ 's' if loopback_count == 1 else '' }} here. + {% endif %} +

+
+ {% for value, label, hint in loopback_modes %} + + {% endfor %} +
+
+ + +

+ Only read when the position above is Only on one port. + Port 22 is refused whatever is typed here — that one is this host's own + sshd, not a container that published its port on the loopback interface. +

+
+
+

The modes

diff --git a/src/lembas/web/templates/agents/detail.html b/src/lembas/web/templates/agents/detail.html index bb35a6f..91b1d7d 100644 --- a/src/lembas/web/templates/agents/detail.html +++ b/src/lembas/web/templates/agents/detail.html @@ -17,6 +17,13 @@ {% if problem %}

{{ icon("warning", "icon--sm") }} {{ problem }}
{% endif %} +{# A connection saved before an administrator moved the switch. It is still here + and still editable -- pointing it at another host is the way out -- but + nothing will use it, and saying so here is the only place somebody looking at + this row would find out. #} +{% if refused %} +
{{ icon("shield", "icon--sm") }} {{ refused }}
+{% endif %} {% if saved %}
{{ icon("check", "icon--sm") }} {{ saved }}
{% endif %} diff --git a/src/lembas/web/templates/agents/index.html b/src/lembas/web/templates/agents/index.html index 4838985..673a1d6 100644 --- a/src/lembas/web/templates/agents/index.html +++ b/src/lembas/web/templates/agents/index.html @@ -62,11 +62,18 @@
{% for profile in profiles %} -
+ {# `refused` is why this connection cannot be used at all, which is a stronger + statement than `disabled` -- that one is the owner's own choice and this one + is not theirs to make. Greyed out with the same class, because "you cannot + use this" is one visual idea however it came about. #} + {% set refused = refusals.get(profile.id, "") %} +
{{ profile.name }} - {% if profile.verified %} + {% if refused %} + not allowed + {% elif profile.verified %} key confirmed {% else %} not checked @@ -77,7 +84,9 @@ {{ profile.address }}{% if profile.default_dir %} · {{ profile.default_dir }}{% endif %} - {% if profile.last_error %} + {% if refused %} +

{{ refused }}

+ {% elif profile.last_error %}

{{ profile.last_error }}

{% elif profile.server_info.system %}

{{ profile.server_info.system }}

diff --git a/tests/conftest.py b/tests/conftest.py index 7570d91..599ba43 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -54,6 +54,19 @@ def fresh_database(tmp_path: Path) -> Iterator[None]: Base.metadata.create_all(bind=get_engine()) sync_schema(get_engine()) + + # An SSH connection to loopback is refused by default -- see + # services/agent/hosts.py, and `tests/test_agent_hosts.py` for the guard + # itself. Almost every agent test has to point at 127.0.0.1 anyway, because + # the ones that stand up a real asyncssh server can only listen there, and + # the rest were written beside them. So the suite runs with the switch open + # and the tests that care about it close it explicitly. + from lembas.db.session import session_scope + from lembas.services import settings_store + + with session_scope() as db: + settings_store.update(db, {"loopback": "on"}, key=settings_store.AGENTS) + yield reset_engine() diff --git a/tests/test_agent_hosts.py b/tests/test_agent_hosts.py new file mode 100644 index 0000000..bb9b1a6 --- /dev/null +++ b/tests/test_agent_hosts.py @@ -0,0 +1,269 @@ +"""An SSH connection pointed at the machine LLeMbas itself runs on. + +The whole design of agent chats is one sentence -- nothing runs on this host -- +and a profile pointed at 127.0.0.1 walks past it while looking, from the SSH +layer down, exactly like a container on the network. So it is an administrator's +decision in three positions, and what is pinned here is that every one of them is +enforced where it has to be rather than only in the form. + +Note the conftest: the suite runs with the switch **open**, because the tests +that stand up a real asyncssh server can only listen on loopback. Every test in +this file closes it explicitly, which is also why they are together in one file. +""" + +from __future__ import annotations + +import pytest +from fastapi.testclient import TestClient +from sqlalchemy import select + +from lembas.db.models import KIND_AGENT, Chat, SshProfile, User +from lembas.services import settings_store +from lembas.services.agent import hosts +from lembas.services.agent import session as agent_session +from lembas.services.crypto import encrypt + + +def _set(db, mode: str, port: int = 0) -> None: + settings_store.update( + db, {"enabled": True, "loopback": mode, "loopback_port": port}, key=settings_store.AGENTS + ) + db.commit() + + +def _profile(db, host: str = "127.0.0.1", port: int = 22) -> SshProfile: + profile = SshProfile( + owner_id=db.scalars(select(User)).first().id, + name=f"box-{host}-{port}", + host=host, + port=port, + username="deploy", + auth="password", + password_encrypted=encrypt("hunter2"), + host_key="ssh-ed25519 AAAA", + host_fingerprint="SHA256:x", + enabled=True, + ) + db.add(profile) + db.commit() + return profile + + +# --- Recognising the machine -------------------------------------------------- +@pytest.mark.parametrize( + "host", + ["127.0.0.1", "127.0.0.53", "127.1.2.3", "::1", "[::1]", "localhost", "LOCALHOST"], +) +def test_the_obvious_spellings_are_recognised(host: str): + """All of them, and from the string alone. `127.0.0.53` is the one worth + listing: the whole of 127.0.0.0/8 is loopback, and a check written against + the single address `127.0.0.1` misses every other way of writing it.""" + assert hosts.is_loopback(host) + + +@pytest.mark.parametrize("host", ["10.0.0.9", "192.168.1.5", "example.test", ""]) +def test_anything_else_is_not(host: str): + """Including a private address. This guard is about *this machine*, not + about the network it is on -- refusing 192.168.x would refuse the container + on the shelf, which is the intended use of the whole feature.""" + assert not hosts.is_loopback(host) + + +def test_recognising_a_host_never_makes_a_network_call(monkeypatch): + """`getaddrinfo` blocks. `is_loopback` is called several times per page + render -- the composer's picker, `resolve_tools`, the canvas -- so a lookup + behind it makes an agent page wait out a DNS timeout for a host nobody is + talking to. The first version of this did exactly that and the suite went + from two minutes to not finishing.""" + import socket + + def explode(*args, **kwargs): # pragma: no cover - the point is it is unused + raise AssertionError("is_loopback resolved a name on the request path") + + monkeypatch.setattr(socket, "getaddrinfo", explode) + + assert hosts.is_loopback("127.0.0.1") + assert not hosts.is_loopback("build.example") + + +def test_a_name_is_settled_by_resolving_it(monkeypatch): + """Which is where a hosts-file entry gets caught. Blocking, and therefore + called only where a network call is already expected.""" + import socket + + monkeypatch.setattr( + socket, + "getaddrinfo", + lambda *a, **k: [(2, 1, 6, "", ("127.0.0.1", 22))], + ) + assert hosts.resolves_here("build.example") + + monkeypatch.setattr( + socket, + "getaddrinfo", + lambda *a, **k: [(2, 1, 6, "", ("93.184.216.34", 22))], + ) + assert not hosts.resolves_here("build.example") + + +def test_a_name_that_does_not_resolve_is_not_this_machine(monkeypatch): + """Refusing it would turn every DNS hiccup into "your connection is on this + machine", which is both wrong and unactionable. The connection fails on its + own terms a moment later.""" + import socket + + def fail(*args, **kwargs): + raise OSError("no such host") + + monkeypatch.setattr(socket, "getaddrinfo", fail) + assert not hosts.resolves_here("nowhere.invalid") + + +# --- The three positions ------------------------------------------------------ +def test_off_refuses_every_port(db, registered): + _set(db, hosts.MODE_OFF) + assert hosts.refusal(db, "127.0.0.1", 22) + assert hosts.refusal(db, "127.0.0.1", 2222) + + +def test_on_allows_every_port(db, registered): + _set(db, hosts.MODE_ON) + assert hosts.refusal(db, "127.0.0.1", 22) == "" + assert hosts.refusal(db, "127.0.0.1", 2222) == "" + + +def test_one_port_allows_that_one_and_nothing_else(db, registered): + _set(db, hosts.MODE_PORT, 2222) + assert hosts.refusal(db, "127.0.0.1", 2222) == "" + assert hosts.refusal(db, "127.0.0.1", 2200) + # And a host that is not this machine is not this switch's business. + assert hosts.refusal(db, "10.0.0.9", 22) == "" + + +def test_port_22_is_refused_even_when_it_is_the_named_one(db, registered): + """The position exists for a container that published its SSH port on the + loopback interface. Port 22 is not that -- it is this host's own sshd, and + naming it is either a mistake or the thing the switch exists to stop.""" + _set(db, hosts.MODE_PORT, 22) + assert hosts.refusal(db, "127.0.0.1", 22) + + +def test_naming_no_port_refuses_rather_than_allows(db, registered): + """`port` with nothing named is an unfinished setting, and the safe reading + of an unfinished setting is the one that refuses.""" + _set(db, hosts.MODE_PORT, 0) + assert hosts.refusal(db, "127.0.0.1", 2222) + + +def test_an_unrecognised_stored_position_means_off(db, registered): + """Fail closed. The direction that is safe to get wrong is refusing a + connection somebody has to re-allow; the other direction is a shell on the + host holding the database and the encryption key.""" + settings_store.update(db, {"enabled": True, "loopback": "sometimes"}, key=settings_store.AGENTS) + db.commit() + assert hosts.refusal(db, "127.0.0.1", 2222) + + +# --- Where it is enforced ----------------------------------------------------- +def test_a_refused_connection_cannot_be_saved(client: TestClient, db, registered): + _set(db, hosts.MODE_OFF) + + response = client.post( + "/api/agents", + data={ + "name": "Here", + "host": "127.0.0.1", + "port": "22", + "username": "deploy", + "auth": "password", + "password": "hunter2", + "enabled": "on", + }, + follow_redirects=False, + ) + + assert response.status_code == 200 # the form again, with the reason on it + assert "points at the machine LLeMbas itself runs on" in response.text + assert db.scalar(select(SshProfile).where(SshProfile.name == "Here")) is None + + +def test_a_connection_saved_before_the_switch_moved_stops_working(db, registered): + """The one that matters. A row can predate a setting, so refusing only at + save time would leave every existing loopback profile working exactly as it + did -- which is the whole of the flaw, untouched. + + `session.resolve` is the control: it is what every agent tool, the terminal + and the canvas go through. + """ + _set(db, hosts.MODE_ON) + profile = _profile(db) + chat = Chat( + user_id=profile.owner_id, kind=KIND_AGENT, ssh_profile_id=profile.id, title="t" + ) + db.add(chat) + db.commit() + + owner = db.get(User, profile.owner_id) + assert agent_session.resolve(db, chat, owner) is not None + + _set(db, hosts.MODE_OFF) + assert agent_session.resolve(db, chat, owner) is None + + +def test_a_refused_connection_is_not_offered_in_the_composer( + client: TestClient, db, registered +): + """Or the reader picks it, starts an agent chat, and finds a chat with no + tools and nothing said about why.""" + _set(db, hosts.MODE_ON) + profile = _profile(db) + + assert profile.name in client.get("/chat").text + + _set(db, hosts.MODE_OFF) + assert profile.name not in client.get("/chat").text + + +def test_the_listing_says_why_rather_than_hiding_it(client: TestClient, db, registered): + """The connection is still the owner's and still editable -- pointing it at + another host is the way out. A row that silently stopped working would be + indistinguishable from a fault.""" + _set(db, hosts.MODE_OFF) + profile = _profile(db) + + body = client.get("/agents").text + + assert profile.name in body + assert "not allowed" in body + assert "points at the machine LLeMbas itself runs on" in body + + +def test_check_opens_no_socket_for_a_refused_connection( + client: TestClient, db, registered, monkeypatch +): + """Check is the one button here that connects, so it must refuse before it + does rather than after.""" + _set(db, hosts.MODE_OFF) + profile = _profile(db) + + async def explode(*args, **kwargs): # pragma: no cover - the point is it is unused + raise AssertionError("a refused connection was contacted") + + from lembas.services.agent import ssh as ssh_service + + monkeypatch.setattr(ssh_service, "capture_host_key", explode) + + body = client.post(f"/api/agents/{profile.id}/check").text + + assert "points at the machine LLeMbas itself runs on" in body + + +def test_a_draft_cannot_be_opened_against_a_refused_connection( + client: TestClient, db, registered +): + """The draft is what the terminal and the canvas open against before a chat + exists, so this is the new-chat path's own gate.""" + _set(db, hosts.MODE_OFF) + profile = _profile(db) + + assert client.get(f"/api/agents/{profile.id}/draft").status_code == 403