diff --git a/.env.example b/.env.example index 1e04ec2..8d79700 100644 --- a/.env.example +++ b/.env.example @@ -149,10 +149,18 @@ # Set to true only for a genuinely local-only instance you want open on # purpose. Never set it on anything reachable over WireGuard/LAN. # API_ALLOW_UNAUTHENTICATED=false +# +# Interactive docs: /docs, /redoc and /openapi.json. FastAPI mounts +# these itself, so API_TOKEN above cannot cover them -- they are either +# published to anyone who can reach the port, or not mounted at all. +# Not mounted by default; turn on while developing against the API. +# API_DOCS_ENABLED=false # --- API rate limiting ------------------------------------------------------- # In-memory sliding window, per client IP, single-process only. -# Applies to the same routes as API_TOKEN above. +# Applies to the same routes as API_TOKEN above, plus /health -- which +# stays open (healthchecks have no token) but is metered, because each +# hit costs an outbound call to the inference server. # RATE_LIMIT_ENABLED=true # RATE_LIMIT_REQUESTS=30 # RATE_LIMIT_WINDOW_SECONDS=60 diff --git a/Containerfile b/Containerfile index ef5d9de..2f93a33 100644 --- a/Containerfile +++ b/Containerfile @@ -1,4 +1,20 @@ -FROM python:3.12 +# Audit E-4: this tag is mutable. `python:3.12` is rebuilt for every +# CPython patch release and every Debian security update, so two +# builds of the same Forge commit can land on two different base +# images -- the same drift the requirements.txt pins just closed, one +# layer down. Pin it to a digest with: +# +# ./deploy/pin-base-image.sh --write +# +# which rewrites this line to FROM ...python:3.12@sha256: and +# leaves a one-line diff to commit. Re-run it to bump, deliberately, +# with the move visible in git history instead of happening silently +# on the next rebuild. +# +# Left as a tag here rather than a digest baked in blind: a digest is +# only meaningful if it's one you resolved yourself, on your own +# build host, from the registry you actually pull from. +FROM docker.io/library/python@sha256:cfdcc988c45d6a933e0ec3fd9ce46e6f78174d3f082eea8f2f4d6f1f72f32b89 WORKDIR /app @@ -20,23 +36,53 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ podman \ && rm -rf /var/lib/apt/lists/* +# --- Non-root runtime user (audit E-4) -------------------------------- +# Everything above runs as root because installing packages needs to; +# nothing below does. uvicorn binds 8000, which is unprivileged, and +# Forge's only writable path is /app/data (MEMORY_FILE, TRACE_FILE, +# RAG_DB_FILE and WORKSPACE_DIR all default under `data/`, and /app is +# WORKDIR). So there is no reason for the serving process to be able +# to rewrite /app/src/forge/ -- which is exactly what a `shell` or +# `files` dispatch could do while it was root. +# +# UID/GID 1000 is not arbitrary and not cosmetic. Under rootless +# podman the container's UID 0 is already mapped to your host UID, and +# that mapping is what makes the 0660 proxy sockets and the mounted +# journal readable at all. Dropping to a container UID that maps to a +# *subuid* would silently break every one of those. Running this image +# non-root therefore requires --userns=keep-id at runtime, so that +# container UID 1000 maps back to host UID 1000. See deploy/README.md, +# "Running non-root" -- the image half without the runtime half is a +# broken container, not a safer one. +# +# python:3.12 (Debian) ships no user at 1000, so this claims it +# cleanly; useradd fails the build loudly if that ever stops being +# true rather than quietly picking a different id. +RUN groupadd --gid 1000 forge \ + && useradd --uid 1000 --gid 1000 --create-home --shell /usr/sbin/nologin forge \ + && mkdir -p /app/data \ + && chown -R forge:forge /app/data /home/forge \ + && python -m compileall -q /app/src + +# HOME matters here in a way it didn't as root: the podman client +# resolves its config and scratch paths from it, and /root exists +# while /home/forge only does because it was just created. +ENV HOME=/home/forge + +USER forge:forge + EXPOSE 8000 # Default: HTTP API (accessible from browser / other machines) # Override for REPL: podman run -it ... forge-core python -m forge.main # -# `mkdir -p /run/systemd/system` -- NOT running real systemd, just -# satisfying its own sd_booted() check (per systemd's own man page: -# "Internally, this function checks whether the directory -# /run/systemd/system/ exists" -- nothing more). Without this, -# `systemctl` inside sysadmin bails out with "System has not been -# booted with systemd... Failed to connect to system scope bus... -# Host is down" *before ever attempting* the bus connection -- -# happens regardless of DBUS_SYSTEM_BUS_ADDRESS being set correctly, -# confirmed in production on 2026-08-11 (the filtered D-Bus proxy -# worked fine when tested directly with busctl; systemctl inside the -# container still refused). /run is a fresh tmpfs at container start, -# so this can't be done at build time -- it has to happen here, in -# CMD, every time the container starts. `exec` keeps uvicorn as PID 1 -# so it still receives SIGTERM directly from `podman stop`. -CMD ["sh", "-c", "mkdir -p /run/systemd/system && exec uvicorn forge.api:app --host 0.0.0.0 --port 8000"] +# Plain exec form: uvicorn is PID 1 and receives SIGTERM directly from +# `podman stop`, with no shell in between. +# +# This used to be wrapped in `sh -c "mkdir -p /run/systemd/system && +# ..."`, to satisfy sd_booted() before `systemctl` would attempt a bus +# connection. Nothing calls systemctl any more -- graphs/sysadmin.py +# discovers units with `busctl --json=short`, which has no such check +# -- so the directory served no purpose and the mkdir was doing +# nothing but adding a shell and a failure mode. +CMD ["uvicorn", "forge.api:app", "--host", "0.0.0.0", "--port", "8000"] diff --git a/deploy/README.md b/deploy/README.md index 0c4c0c4..96996af 100644 --- a/deploy/README.md +++ b/deploy/README.md @@ -100,6 +100,7 @@ logging to the user session journal, not root services like ```bash podman run -d --name forge \ + --userns=keep-id \ --group-add keep-groups \ --env-file .env.local \ -v $(pwd)/data:/app/data \ @@ -122,6 +123,7 @@ services: image: forge-core container_name: forge restart: unless-stopped + userns_mode: "keep-id" annotations: run.oci.keep_original_groups: "1" ports: @@ -141,6 +143,73 @@ rootless setup; the paths on the right of each `:` are what in `.env.local` above actually refer to, since those are seen from *inside* the container.) +## Running non-root (and why `--userns=keep-id` is not optional) + +The image sets `USER forge` (UID/GID 1000) so the serving process +can't rewrite `/app/src/forge/` or anything else in the container. +That half is in the Containerfile. The other half is at runtime, and +**an image built non-root without `--userns=keep-id` is a broken +container, not a hardened one.** + +Why: under rootless podman the container's UID 0 is already mapped to +your host UID. That mapping is what lets the container read the two +proxy sockets (mode 0660, owned by your host user) and write to +`./data`. A container UID of 1000 maps by default to a *subuid* +instead -- a stranger to all of those -- so `sysadmin` loses both +proxies and Forge loses its writable directory. `--userns=keep-id` +maps your host UID to the same UID inside, putting `forge` back where +container-root used to be. + +Check your host UID first: + +```bash +id -u # expected: 1000 +``` + +If it isn't 1000, use the explicit form -- `--userns=keep-id:uid=1000,gid=1000` +on the CLI, or `userns_mode: "keep-id:uid=1000,gid=1000"` in compose +-- which pins the mapping to the image's user regardless of the host +id. + +### What to verify after switching + +Each of these has a distinct failure mode, so run them in order and +stop at the first surprise: + +```bash +# 1. Non-root, and mapped to your host user. +podman exec forge id +# expect: uid=1000(forge) gid=1000(forge) + +# 2. Supplementary groups survived the user namespace. +# This is the one to watch: --group-add keep-groups and +# --userns=keep-id are two different mechanisms touching the same +# thing, and keep-groups is what journalctl -u depends on. +podman exec forge id +# expect: extra groups listed, not just 1000(forge) + +# 3. The writable path is actually writable. +podman exec forge touch /app/data/.write-test && echo OK + +# 4. Both proxies still reachable. Use a *root-owned* unit here: +# it's the one subject to the wheel ACL on system.journal, so it +# tests group access rather than just "no entries". +podman exec forge podman --url unix:///run/forge-podman-ro-proxy.sock ps +podman exec forge journalctl -D /host-journal -u steamos-manager.service -n 5 +``` + +Confirmed working on the Deck (2026-08-13): `uid_map` shows +`1000 0 1`, which is keep-id's signature -- container UID 1000 is the +host user. Supplementary groups survive the switch, because the +kernel evaluates the journal ACL against the host-side GIDs, not the +names visible inside the container. + +### If something breaks and you need the old behaviour now + +`user: "0:0"` in compose (or `--user 0:0` on the CLI) overrides the +image's `USER` without a rebuild. That is the whole revert: one line, +no image work, and it restores exactly the previous posture. + ## Group access for `journalctl -u` on root-owned system services Confirmed root cause: rootless podman does NOT pass the host user's diff --git a/deploy/compose.example.yaml b/deploy/compose.example.yaml index 6cdb78d..b1fdf7a 100644 --- a/deploy/compose.example.yaml +++ b/deploy/compose.example.yaml @@ -76,6 +76,17 @@ services: image: forge-core container_name: forge restart: unless-stopped + # L'image tourne en UID 1000 (utilisateur `forge`), pas root. + # keep-id est la moitie obligatoire cote runtime : sans elle, + # l'UID 1000 du container est mappe sur un subuid et perd l'acces + # aux sockets des proxies (0660, propriete de l'utilisateur hote) + # ET a ./data. Voir deploy/README.md, section "Running non-root". + # Si l'UID hote n'est pas 1000 : keep-id:uid=1000,gid=1000. + # Echappatoire immediate si quelque chose casse, sans rebuild : + # decommenter `user: "0:0"` ci-dessous, qui restaure l'ancien + # comportement root. + userns_mode: "keep-id" + # user: "0:0" annotations: run.oci.keep_original_groups: "1" ports: diff --git a/deploy/pin-base-image.sh b/deploy/pin-base-image.sh new file mode 100755 index 0000000..5fac5b9 --- /dev/null +++ b/deploy/pin-base-image.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash +# +# Resolve the Containerfile's base image tag to an immutable digest +# and (with --write) pin the FROM line to it. Audit E-4. +# +# Why this is a script and not just a digest committed by hand: a +# digest is only correct for the moment it was resolved, and the +# whole point of pinning is that bumping it is a deliberate, visible +# act. `python:3.12` is a moving tag -- it gets rebuilt for every +# CPython patch release and every Debian security update, so two +# builds of the same Forge commit can sit on two different base +# images. That's the same problem the requirements pins fixed, one +# layer down. +# +# Usage: +# ./deploy/pin-base-image.sh # show the digest, change nothing +# ./deploy/pin-base-image.sh --write # rewrite the FROM line +# +# To bump later: put the tag back (or edit the tag in TAG below), +# re-run with --write, and commit the one-line diff. The diff is the +# record of what you moved onto and when. + +set -euo pipefail +cd "$(dirname "${BASH_SOURCE[0]}")/.." # repo root, regardless of cwd + +TAG="${FORGE_BASE_TAG:-docker.io/library/python:3.12}" +WRITE=0 +[[ "${1:-}" == "--write" ]] && WRITE=1 + +if ! command -v podman >/dev/null 2>&1; then + echo "podman not found -- this needs to run on the build host." >&2 + exit 1 +fi + +echo "Pulling ${TAG} to resolve its digest..." +podman pull "${TAG}" >/dev/null + +# RepoDigests is the registry-side content address: pulling by it +# gets the exact same bytes forever, where the tag does not. Take the +# first entry -- an image pulled from one registry has one. +DIGEST_REF="$(podman image inspect --format '{{index .RepoDigests 0}}' "${TAG}")" + +if [[ -z "${DIGEST_REF}" ]]; then + echo "No RepoDigest on ${TAG} -- was it built locally rather than pulled?" >&2 + exit 1 +fi + +echo +echo "Resolved: ${DIGEST_REF}" +echo + +if [[ "${WRITE}" -eq 0 ]]; then + echo "Nothing written. Re-run with --write to pin the Containerfile:" + echo " FROM ${DIGEST_REF}" + exit 0 +fi + +# Matches both the tag form and an already-pinned digest form, so +# re-running to bump works the same as running it the first time. +python3 - "$DIGEST_REF" <<'PY' +import re +import sys +from pathlib import Path + +digest_ref = sys.argv[1] +path = Path("Containerfile") +text = path.read_text(encoding="utf-8") +new_text, count = re.subn( + r"^FROM\s+\S+$", f"FROM {digest_ref}", text, count=1, flags=re.MULTILINE +) +if count != 1: + sys.exit("Could not find a single FROM line to rewrite -- edit by hand.") +path.write_text(new_text, encoding="utf-8") +print(f"Containerfile FROM pinned to {digest_ref}") +PY + +echo +echo "Now review and commit the diff:" +echo " git diff Containerfile" diff --git a/deploy/podman_ro_proxy.py b/deploy/podman_ro_proxy.py index 825fb7b..96a0f18 100755 --- a/deploy/podman_ro_proxy.py +++ b/deploy/podman_ro_proxy.py @@ -18,6 +18,14 @@ what gets mounted into Forge's container (via SYSADMIN_PODMAN_URL), never the real one. +`?follow=true` on the logs path is refused too, even though it's a +GET on an allowed path: it asks podman for a stream that never ends, +which parks a handler thread and an upstream connection for good. +Availability is part of the read-only guarantee -- a proxy that can +be wedged with three allowed requests isn't much of a proxy. Same +reasoning behind the upstream timeout and the response cap below, +and behind serving requests on threads instead of one at a time. + Stdlib only, deliberately -- consistent with the rest of Forge (web_fetch's HTML parser, the markdown renderer, etc.): one more moving part with no extra dependency to audit. @@ -40,6 +48,8 @@ import re import socket import socketserver +import sys +import urllib.parse from http.server import BaseHTTPRequestHandler # Matches podman's actual REST paths, tolerant of the /vX.Y.Z API @@ -59,26 +69,72 @@ ] +def _mentions_follow(path: str) -> bool: + """True if the query string mentions `follow` at all. + + `GET /containers/{id}/logs?follow=true` never returns: podman + holds the connection open and keeps writing. This proxy reads the + whole upstream body before answering, so a single follow request + parks a handler thread and an upstream connection permanently. + Repeat it a few times and sysadmin's log collection is dead -- + with no exploit, no mutation, and nothing in the allowlist + violated. + + The test is presence of the key, not whether its value looks + true. `?follow`, `?follow=1`, `?follow=TRUE` and `?follow=false` + are all refused, because deciding which of those podman's own + decoder reads as true means reimplementing someone else's + boolean parsing and being right about it -- the classic way a + filter and the thing it filters end up disagreeing. Forge never + sends the parameter in any form (graphs/sysadmin.py runs + `podman logs --tail N`), so refusing all of them costs nothing. + """ + query = urllib.parse.urlsplit(path).query + keys = urllib.parse.parse_qs(query, keep_blank_values=True).keys() + return any(key.lower() == "follow" for key in keys) + + def _is_allowed(method: str, path: str) -> bool: if method != "GET": return False - return any(p.match(path) for p in _ALLOWED_GET_PATTERNS) + if not any(p.match(path) for p in _ALLOWED_GET_PATTERNS): + return False + return not _mentions_follow(path) + + +# Defaults for the two limits below. Both are here because this proxy +# reads the whole upstream response into memory before answering: with +# no timeout a wedged podman parks a handler thread forever, and with +# no cap a `?tail=all` against a large journal is an OOM on the host, +# not in the container. +DEFAULT_UPSTREAM_TIMEOUT = 30.0 # seconds +DEFAULT_MAX_RESPONSE_BYTES = 8 * 1024 * 1024 class _UnixHTTPConnection(http.client.HTTPConnection): """http.client talks TCP by default; podman.sock is a Unix socket, so the connect step is overridden to dial that instead.""" - def __init__(self, unix_path: str): - super().__init__("localhost") + def __init__(self, unix_path: str, timeout: float = DEFAULT_UPSTREAM_TIMEOUT): + super().__init__("localhost", timeout=timeout) self._unix_path = unix_path def connect(self): self.sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + # HTTPConnection.timeout is honoured for TCP by its own + # connect(); overriding connect() means applying it here by + # hand, or the socket inherits the global default (None = + # block forever) and the timeout argument silently does + # nothing. + self.sock.settimeout(self.timeout) self.sock.connect(self._unix_path) -def make_handler(upstream_path: str): +def make_handler( + upstream_path: str, + timeout: float = DEFAULT_UPSTREAM_TIMEOUT, + max_bytes: int = DEFAULT_MAX_RESPONSE_BYTES, +): class Handler(BaseHTTPRequestHandler): def _forward(self): if not _is_allowed(self.command, self.path): @@ -87,22 +143,55 @@ def _forward(self): self.end_headers() self.wfile.write( b"forbidden: podman_ro_proxy only allows GET on " - b"/containers/json and /containers/{id}/logs\n" + b"/containers/json and /containers/{id}/logs, " + b"and never with follow\n" ) return - conn = _UnixHTTPConnection(upstream_path) + conn = _UnixHTTPConnection(upstream_path, timeout=timeout) try: conn.request(self.command, self.path) resp = conn.getresponse() - body = resp.read() + # Read one byte past the cap so an exactly-at-cap body + # isn't mislabelled as truncated. + body = resp.read(max_bytes + 1) + truncated = len(body) > max_bytes + if truncated: + body = body[:max_bytes] + print( + f"podman_ro_proxy: truncated response for {self.path} " + f"at {max_bytes} bytes", + file=sys.stderr, + ) self.send_response(resp.status) for header, value in resp.getheaders(): - if header.lower() == "transfer-encoding": + lowered = header.lower() + if lowered == "transfer-encoding": continue # avoid double-chunking; we already read the full body + if lowered == "content-length" and truncated: + # Forwarding upstream's length after cutting the + # body would leave the client waiting on bytes + # that never arrive -- a hang instead of a short + # read, which is worse than the truncation. + continue self.send_header(header, value) + if truncated: + self.send_header("Content-Length", str(len(body))) self.end_headers() self.wfile.write(body) + except TimeoutError: + # Distinct from 403: the request was allowed, podman + # just didn't answer in time. Saying so keeps a wedged + # upstream from looking like a policy rejection. + self.send_response(504) + self.send_header("Content-Type", "text/plain") + self.end_headers() + self.wfile.write(b"gateway timeout: podman did not respond in time\n") + except OSError as exc: + self.send_response(502) + self.send_header("Content-Type", "text/plain") + self.end_headers() + self.wfile.write(f"bad gateway: {exc}\n".encode()) finally: conn.close() @@ -121,8 +210,33 @@ def log_message(self, fmt, *args): # quieter default logging return Handler -class _UnixSocketHTTPServer(socketserver.UnixStreamServer): +class _UnixSocketHTTPServer(socketserver.ThreadingMixIn, socketserver.UnixStreamServer): + """Threaded on purpose (audit M-1). + + UnixStreamServer alone handles one request at a time, start to + finish. sysadmin's collect step is a blocking `podman logs` that + can legitimately take seconds on a busy container, and while it + runs nothing else -- not even the client's own /_ping -- gets + served. One slow request became a queue for every request. + + daemon_threads means a hung handler can't keep the process alive + at shutdown; combined with the upstream timeout above, a stuck + podman costs one thread for at most that long rather than + permanently. + """ + allow_reuse_address = True + daemon_threads = True + + def handle_error(self, request, client_address): + """A client that hangs up mid-response is normal (podman's own + CLI does it on ^C) and shouldn't print a full traceback per + occurrence -- that's how the operator learns to ignore this + proxy's output, which is where real errors go to hide.""" + exc = sys.exception() + if isinstance(exc, (BrokenPipeError, ConnectionResetError)): + return + super().handle_error(request, client_address) def main() -> None: @@ -133,12 +247,31 @@ def main() -> None: parser.add_argument( "--listen", required=True, help="path for this proxy's own socket" ) + parser.add_argument( + "--upstream-timeout", + type=float, + default=DEFAULT_UPSTREAM_TIMEOUT, + help="seconds to wait on podman before answering 504 (default: %(default)s)", + ) + parser.add_argument( + "--max-response-bytes", + type=int, + default=DEFAULT_MAX_RESPONSE_BYTES, + help="cap on a forwarded response body (default: %(default)s)", + ) args = parser.parse_args() if os.path.exists(args.listen): os.remove(args.listen) - server = _UnixSocketHTTPServer(args.listen, make_handler(args.upstream)) + server = _UnixSocketHTTPServer( + args.listen, + make_handler( + args.upstream, + timeout=args.upstream_timeout, + max_bytes=args.max_response_bytes, + ), + ) os.chmod(args.listen, 0o660) # group-readable only, not world print( f"podman_ro_proxy: {args.listen} -> {args.upstream} (GET-only, containers/json + logs)" diff --git a/requirements-dev.txt b/requirements-dev.txt index e906922..073f602 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,5 +1,23 @@ -pytest -ruff -httpx -requests-mock -httpx2 +# Dev/CI dependencies, pinned for the same reason as requirements.txt +# (audit E-4) -- and arguably a stronger one: CI installs these on a +# runner that holds a repo-scoped GITHUB_TOKEN. An unpinned dev +# dependency is a supply-chain path straight into the workflow. +# +# Install alongside the runtime pins: +# pip install -r requirements.txt -r requirements-dev.txt + +# --- Direct ------------------------------------------------------- +httpx==0.28.1 +httpx2==2.10.0 +pytest==9.1.1 +requests-mock==1.12.1 +ruff==0.16.2 + +# --- Transitive --------------------------------------------------- +Pygments==2.20.0 +httpcore==1.0.9 +httpcore2==2.10.0 +iniconfig==2.3.0 +packaging==26.3 +pluggy==1.6.0 +truststore==0.10.4 diff --git a/requirements.txt b/requirements.txt index b789909..9039209 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,51 @@ -requests -fastapi -uvicorn[standard] -sqlite-vec +# Runtime dependencies, fully pinned (audit E-4). +# +# Why pinned, transitive deps included: the Containerfile runs +# `pip install -r requirements.txt` on every image build. Unpinned, +# that means "whatever PyPI serves today" -- two builds of the same +# commit can produce different images, and a compromised release of +# any transitive dependency (starlette, urllib3, h11...) lands in the +# image silently, with nothing in the diff to notice. Pinning makes a +# dependency change a reviewable commit instead of a side effect of +# rebuilding. +# +# What this does NOT buy: protection against a package being replaced +# at the same version. PyPI forbids re-uploading a version, so that's +# largely covered upstream, but a compromised mirror or index could +# still serve something else. `pip install --require-hashes` closes +# that too, at the cost of a hash per artifact and a lock that has to +# be regenerated per platform. Not done here -- noted so the choice is +# visible rather than assumed. +# +# Regenerate after changing a direct dependency: +# python3 -m venv /tmp/pin && /tmp/pin/bin/pip install +# /tmp/pin/bin/pip freeze | sort +# then paste the result below, keeping the two sections apart. + +# --- Direct ------------------------------------------------------- +fastapi==0.141.1 +requests==2.34.2 +sqlite-vec==0.1.9 +uvicorn[standard]==0.52.1 + +# --- Transitive (pulled by the above; listed so builds are exact) -- +PyYAML==6.0.3 +annotated-doc==0.0.5 +annotated-types==0.8.0 +anyio==4.14.2 +certifi==2026.7.22 +charset-normalizer==3.5.0 +click==8.4.2 +h11==0.16.0 +httptools==0.8.0 +idna==3.18 +pydantic==2.13.4 +pydantic_core==2.46.4 +python-dotenv==1.2.2 +starlette==1.6.0 +typing-inspection==0.4.4 +typing_extensions==4.16.0 +urllib3==2.7.0 +uvloop==0.22.1 +watchfiles==1.2.0 +websockets==17.0.1 diff --git a/src/forge/api.py b/src/forge/api.py index b8f4647..99ee48a 100644 --- a/src/forge/api.py +++ b/src/forge/api.py @@ -20,9 +20,15 @@ posture is intentional. See check_auth_configuration() below. Rate limiting: in-memory sliding window, per client IP, on every -endpoint except / and /health. RATE_LIMIT_REQUESTS per -RATE_LIMIT_WINDOW_SECONDS (default: 30 per 60s). Set -RATE_LIMIT_ENABLED=false to disable. +endpoint except /. RATE_LIMIT_REQUESTS per RATE_LIMIT_WINDOW_SECONDS +(default: 30 per 60s). Set RATE_LIMIT_ENABLED=false to disable. +/health stays unauthenticated (healthchecks and the UI status line +have no token yet) but is metered like the rest: each hit costs an +outbound call to llama.cpp. + +Interactive docs (/docs, /redoc, /openapi.json) are off unless +API_DOCS_ENABLED=true. FastAPI mounts them itself, so they cannot be +put behind require_token. Run: uvicorn forge.api:app --host 0.0.0.0 --port 8000 @@ -45,6 +51,7 @@ from forge import rag, ratelimit, trace from forge.config import ( API_ALLOW_UNAUTHENTICATED, + API_DOCS_ENABLED, API_TOKEN, FORGE_PROVIDER, LLAMA_CPP_URL, @@ -90,7 +97,20 @@ async def lifespan(_app: FastAPI): yield -app = FastAPI(title="Forge", version="3.3.0", docs_url="/docs", lifespan=lifespan) +# docs_url/redoc_url/openapi_url are None unless API_DOCS_ENABLED says +# otherwise (audit M-3). These three routes are mounted by FastAPI +# itself, so they can't take Depends(require_token) the way this app's +# own routes do -- there is no version of them that is behind the +# token. Off by default; API_DOCS_ENABLED=true turns them back on for +# development. +app = FastAPI( + title="Forge", + version="3.3.0", + docs_url="/docs" if API_DOCS_ENABLED else None, + redoc_url="/redoc" if API_DOCS_ENABLED else None, + openapi_url="/openapi.json" if API_DOCS_ENABLED else None, + lifespan=lifespan, +) _executor = ThreadPoolExecutor(max_workers=2) _orchestrator = Orchestrator() @@ -232,7 +252,14 @@ def _graph_registry() -> dict: # ─── Endpoints ───────────────────────────────────────────────────── -@app.get("/health") +# Rate-limited but deliberately still unauthenticated (audit M-3): +# it's what a container healthcheck and the UI's own status line call +# before there's a token to send. The limit is the point -- every hit +# makes an outbound HTTP call to llama.cpp to read the loaded model +# name, so an unmetered /health turns one cheap request from an +# anonymous caller into load on the inference server. That's an +# amplifier, not just a chatty endpoint. +@app.get("/health", dependencies=[Depends(rate_limit)]) async def health(): model = LLM_MODEL if FORGE_PROVIDER == "llama_cpp": diff --git a/src/forge/config.py b/src/forge/config.py index c72c99c..c0137e5 100644 --- a/src/forge/config.py +++ b/src/forge/config.py @@ -215,6 +215,18 @@ def _bool(name: str, default: str = "false") -> bool: # perfectly good reason to set it; forgetting isn't. API_ALLOW_UNAUTHENTICATED = _bool("API_ALLOW_UNAUTHENTICATED", "false") +# Interactive docs (/docs, /redoc, /openapi.json). FastAPI mounts all +# three by default and none of them can carry a Depends(require_token) +# -- they're wired up by the framework, not by this app's routes. So +# on an instance reachable by anything other than you, they publish a +# complete, machine-readable map of every endpoint, its parameters and +# its schemas to an unauthenticated caller. That's not a +# vulnerability by itself; it's the reconnaissance step made free, and +# it's free for an attacker who has learned nothing else about the +# instance. Off by default, on where it's useful (audit M-3): set +# API_DOCS_ENABLED=true while developing against the API. +API_DOCS_ENABLED = _bool("API_DOCS_ENABLED", "false") + # --- API rate limiting ---------------------------------------------------- # In-memory sliding window, per client IP, single-process only (see # forge/ratelimit.py). Defaults are generous for interactive/UI use diff --git a/src/forge/graphs/sysadmin.py b/src/forge/graphs/sysadmin.py index 2e305b7..8c0bf94 100644 --- a/src/forge/graphs/sysadmin.py +++ b/src/forge/graphs/sysadmin.py @@ -8,7 +8,7 @@ Security model (read-only, always): - discover_node runs two fixed, parameter-free commands - (systemctl list-units, podman ps) -- nothing here can be + (a busctl ListUnits call, podman ps) -- nothing here can be influenced by user input, so there is no injection surface at this step. - collect_node only ever runs a command built from a fixed template @@ -151,17 +151,35 @@ def _collect_cmd(kind: str, name: str) -> list[str]: collect_node has verified it against discover_node's own output -- see collect_node's docstring. `kind` selects journalctl-by-unit, podman-logs, or journalctl-kernel; the journal dir / podman URL - flags are added only when the matching proxy is configured.""" + flags are added only when the matching proxy is configured. + + The name is passed as `--unit=` and after a `--` separator + respectively (audit M-4), never as a bare argument following a + short flag. `-u foo` and `foo` are both positions where a value + starting with `-` is read as an option instead: `-u --output=cat` + or a container literally named `--help` would be interpreted by + journalctl/podman rather than treated as a name. The attached form + and the end-of-options marker remove that reading entirely. + + This is the second lock on a door that collect_node already + bolted: a name only reaches here if it appeared verbatim in + discovery output, and unit/container names don't normally start + with a dash. What it defends is the case where discovery output is + no longer trustworthy -- a hostile container name, or a proxy + returning something the host didn't say -- which is exactly the + assumption the validation rests on and therefore the one worth not + resting the whole thing on. + """ if kind == "unit": cmd = ["journalctl"] if SYSADMIN_JOURNAL_DIR: cmd += ["-D", SYSADMIN_JOURNAL_DIR] - return cmd + ["-u", name, "--no-pager", "-n", str(SYSADMIN_MAX_LOG_LINES)] + return cmd + [f"--unit={name}", "--no-pager", "-n", str(SYSADMIN_MAX_LOG_LINES)] if kind == "container": cmd = ["podman"] if SYSADMIN_PODMAN_URL: cmd += ["--url", SYSADMIN_PODMAN_URL] - return cmd + ["logs", "--tail", str(SYSADMIN_MAX_LOG_LINES), name] + return cmd + ["logs", "--tail", str(SYSADMIN_MAX_LOG_LINES), "--", name] if kind == "kernel": cmd = ["journalctl"] if SYSADMIN_JOURNAL_DIR: @@ -174,7 +192,7 @@ def _subprocess_env() -> dict[str, str]: """Same minimal-env posture as tools/shell.py: no host env variables reach the subprocess except what's explicitly listed. DBUS_SYSTEM_BUS_ADDRESS is added only when SYSADMIN_DBUS_ADDRESS - is configured, pointing systemctl at the filtered proxy socket + is configured, pointing busctl at the filtered proxy socket from deploy/forge-dbus-proxy.sh -- never the real system bus.""" env = {"PATH": "/usr/local/bin:/usr/bin:/bin", "TERM": "dumb"} if SYSADMIN_DBUS_ADDRESS: @@ -225,7 +243,7 @@ def _run_fixed(cmd: list[str], timeout: int) -> str: Never shell=True, never a hand-built string -- same posture as tools/shell.py's allowlisted subprocess.run(parts, ...). Uses _subprocess_env() so DBUS_SYSTEM_BUS_ADDRESS (when configured) - points systemctl at the filtered proxy, not the host bus.""" + points busctl at the filtered proxy, not the host bus.""" try: result = subprocess.run( cmd, @@ -248,16 +266,19 @@ def _run_fixed(cmd: list[str], timeout: int) -> str: if result.returncode != 0: # The command RAN (no Python-level exception above) but the - # target itself failed -- e.g. systemctl unable to reach the - # bus, podman unable to reach its socket. This must carry the - # same "[error]" prefix as the exception-based cases above: + # target itself failed -- e.g. busctl unable to reach the bus, + # podman unable to reach its socket. This must carry the same + # "[error]" prefix as the exception-based cases above: # without it, a real production case slipped straight through - # as if it were valid data -- systemctl's two-line failure - # message ("System has not been booted with systemd...\n - # Failed to connect to bus...") got parsed as two fake unit - # names ("System", "Failed") by _discover_node, and podman's - # connection-refused text got parsed as a fake container name - # the same way. Caught in production on 2026-08-11. + # as if it were valid data. The case that taught this was + # systemctl, back when discovery still used it: its two-line + # failure message ("System has not been booted with + # systemd...\nFailed to connect to bus...") got parsed as two + # fake unit names ("System", "Failed") by _discover_node, and + # podman's connection-refused text got parsed as a fake + # container name the same way. Caught in production on + # 2026-08-11. The systemctl path is gone; the failure mode it + # exposed is not, which is why the guard stays. return f"[error] {cmd[0]} exited {result.returncode}: {joined}" return joined diff --git a/src/forge/ratelimit.py b/src/forge/ratelimit.py index 2f712c2..2df2aad 100644 --- a/src/forge/ratelimit.py +++ b/src/forge/ratelimit.py @@ -9,11 +9,18 @@ limit by worker count. Fine for the single-worker deployment this project documents (see the Containerfile); worth knowing if that ever changes. + +Keys are expired as well as counted (audit M-2). The dict used to be +append-only: one entry per client IP that ever made a request, kept +for the life of the process. That's a slow leak on any instance +reachable by more than one address, and a fast one for anyone who can +vary their source IP -- the rate limiter itself becoming the thing +that exhausts memory is a poor trade for what it protects. """ import threading import time -from collections import defaultdict, deque +from collections import deque from forge.config import ( RATE_LIMIT_ENABLED, @@ -21,8 +28,47 @@ RATE_LIMIT_WINDOW_SECONDS, ) +# Ceiling on tracked keys, enforced independently of the timed sweep +# below. The sweep bounds memory over time; this bounds it during a +# single window, when a flood of distinct source addresses could +# otherwise grow the dict faster than any schedule collects it. +_MAX_TRACKED_KEYS = 10_000 + _lock = threading.Lock() -_hits: dict[str, deque] = defaultdict(deque) +_hits: dict[str, deque] = {} +_last_sweep = 0.0 + + +def _sweep_locked(now: float) -> None: + """Drop every key whose hits have all aged out. Caller holds _lock. + + A key is only removed when its most recent hit is older than the + window, i.e. when keeping it and dropping it are observationally + identical: a client that comes back gets a fresh deque and the + same allowance it would have had. Expiry here is bookkeeping, not + policy -- it must never hand anyone a budget they hadn't earned. + """ + global _last_sweep + window_start = now - RATE_LIMIT_WINDOW_SECONDS + for key in [k for k, hits in _hits.items() if not hits or hits[-1] < window_start]: + del _hits[key] + _last_sweep = now + + +def _evict_oldest_locked(target: int) -> None: + """Last resort when a sweep didn't get the dict under the ceiling. + + Only reachable if _MAX_TRACKED_KEYS distinct clients are all + genuinely active inside one window, which on a personal instance + means something is wrong rather than busy. Evicting the + least-recently-seen entries does give those clients a fresh + allowance -- but they are, by construction, the ones who have + hammered least recently, and the alternative is the process dying, + which lifts the limit for everyone at once. + """ + by_age = sorted(_hits.items(), key=lambda item: item[1][-1] if item[1] else 0.0) + for key, _ in by_age[: max(len(_hits) - target, 0)]: + del _hits[key] def check(key: str) -> tuple[bool, int]: @@ -39,7 +85,24 @@ def check(key: str) -> tuple[bool, int]: window_start = now - RATE_LIMIT_WINDOW_SECONDS with _lock: - hits = _hits[key] + # Amortised: one pass over the dict per window, not per + # request. The ceiling check is what makes a burst inside a + # single window bounded too. + if now - _last_sweep >= RATE_LIMIT_WINDOW_SECONDS: + _sweep_locked(now) + if len(_hits) > _MAX_TRACKED_KEYS: + _sweep_locked(now) + if len(_hits) > _MAX_TRACKED_KEYS: + _evict_oldest_locked(_MAX_TRACKED_KEYS) + + hits = _hits.get(key) + if hits is None: + # Plain dict, not defaultdict: reading a key must not + # create one. The sweep above iterates this dict, and a + # container that grows on read is the kind of thing that + # makes a leak fix leak. + hits = _hits[key] = deque() + while hits and hits[0] < window_start: hits.popleft() @@ -51,7 +114,16 @@ def check(key: str) -> tuple[bool, int]: return True, 0 +def tracked_keys() -> int: + """Number of client keys currently held. Exposed for tests and for + anyone wanting to confirm the dict isn't growing without bound.""" + with _lock: + return len(_hits) + + def reset() -> None: """Test helper: clear every counter.""" + global _last_sweep with _lock: _hits.clear() + _last_sweep = 0.0 diff --git a/tests/test_api.py b/tests/test_api.py index e3a03b7..19bf93c 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -180,14 +180,40 @@ def test_rate_limit_disabled_allows_unlimited_requests(monkeypatch): assert client.post("/chat", json={"message": "hi"}).status_code == 200 -def test_health_is_never_rate_limited(monkeypatch): +def test_health_is_rate_limited_like_every_other_endpoint(monkeypatch): + """Deliberately inverts the earlier test_health_is_never_rate_limited + (audit M-3). + + Exempting /health looked free when it was read as "a status string + nobody can abuse". It isn't one: with FORGE_PROVIDER=llama_cpp, + every hit makes an outbound HTTP call to the inference server to + read the loaded model name. Unmetered, that turns one cheap + anonymous request into load on the LLM -- an amplifier reachable + without a token, which is the combination that matters. + + Still unauthenticated (see the test below) -- metered, not gated. + """ monkeypatch.setattr(api_mod.ratelimit, "RATE_LIMIT_ENABLED", True) - monkeypatch.setattr(api_mod.ratelimit, "RATE_LIMIT_REQUESTS", 1) + monkeypatch.setattr(api_mod.ratelimit, "RATE_LIMIT_REQUESTS", 2) monkeypatch.setattr(api_mod.ratelimit, "RATE_LIMIT_WINDOW_SECONDS", 60) monkeypatch.setattr(api_mod, "FORGE_PROVIDER", "ollama") client = _client() - for _ in range(5): - assert client.get("/health").status_code == 200 + + assert client.get("/health").status_code == 200 + assert client.get("/health").status_code == 200 + + r = client.get("/health") + assert r.status_code == 429 + assert "Retry-After" in r.headers + + +def test_health_stays_unauthenticated_even_with_a_token_set(monkeypatch): + """The rate limit must not have quietly turned into an auth gate: + a container healthcheck and the UI's own status line both call + /health before they have a token to send.""" + monkeypatch.setattr(api_mod, "API_TOKEN", "s3cret") + monkeypatch.setattr(api_mod, "FORGE_PROVIDER", "ollama") + assert _client().get("/health").status_code == 200 def test_different_clients_have_independent_limits(monkeypatch): diff --git a/tests/test_api_docs_switch.py b/tests/test_api_docs_switch.py new file mode 100644 index 0000000..cc2a60d --- /dev/null +++ b/tests/test_api_docs_switch.py @@ -0,0 +1,87 @@ +""" +Tests for the interactive docs switch (security audit, M-3). + +/docs, /redoc and /openapi.json are mounted by FastAPI itself rather +than by this app's routes, so there is no way to hang +Depends(require_token) on them -- the only two states available are +"published to anyone who can reach the port" and "not mounted". +API_DOCS_ENABLED picks between them, defaulting to not mounted. + +The enabled case needs a separate interpreter: docs_url is read once, +when the FastAPI object is constructed at import time, so +monkeypatching the flag afterwards changes nothing. Reloading +forge.api in-process would hand the rest of the suite a different +module object than the one it holds a reference to, so a subprocess +is the honest way to exercise that branch. +""" + +import json +import subprocess +import sys +import textwrap + +from fastapi.testclient import TestClient + +import forge.api as api_mod + +_DOC_ROUTES = ["/docs", "/redoc", "/openapi.json"] + + +def _statuses_in_subprocess(env_value: str | None) -> dict[str, int]: + script = textwrap.dedent( + f""" + import json + from fastapi.testclient import TestClient + import forge.api as api_mod + + client = TestClient(api_mod.app) + routes = {_DOC_ROUTES!r} + print(json.dumps({{route: client.get(route).status_code for route in routes}})) + """ + ) + env = { + "PATH": "/usr/bin:/bin", + "PYTHONPATH": ":".join(sys.path), + # The app refuses to start unauthenticated; this subprocess only + # builds the app object, but keep it configured either way. + "API_TOKEN": "s3cret", + } + if env_value is not None: + env["API_DOCS_ENABLED"] = env_value + + result = subprocess.run( + [sys.executable, "-c", script], + capture_output=True, + text=True, + env=env, + timeout=120, + check=True, + ) + return json.loads(result.stdout.strip().splitlines()[-1]) + + +def test_docs_are_not_mounted_by_default(): + """The default has to be checked against a fresh interpreter with + no API_DOCS_ENABLED in the environment -- asserting it against the + already-imported app would only prove what this test run happens + to be configured as.""" + statuses = _statuses_in_subprocess(None) + assert all(status == 404 for status in statuses.values()), statuses + + +def test_docs_are_mounted_when_explicitly_enabled(): + """The off switch is only defensible if the on switch works; + otherwise the next person needing the schema turns off something + else instead.""" + statuses = _statuses_in_subprocess("true") + assert all(status == 200 for status in statuses.values()), statuses + + +def test_the_app_under_test_matches_its_own_configuration(): + """Ties the running app object to the flag it was built from, so + this file can't pass while the real app quietly publishes the + schema.""" + client = TestClient(api_mod.app) + expected = 200 if api_mod.API_DOCS_ENABLED else 404 + for route in _DOC_ROUTES: + assert client.get(route).status_code == expected, route diff --git a/tests/test_container_user.py b/tests/test_container_user.py new file mode 100644 index 0000000..1d636ca --- /dev/null +++ b/tests/test_container_user.py @@ -0,0 +1,84 @@ +""" +Tests for the non-root container user (security audit, E-4). + +Nothing here builds or runs an image -- that needs a real podman host +and is checked by hand against deploy/README.md's verification list. +What these hold is the pair of facts that make the change coherent, +because either one alone is worse than neither: + + - the image drops privileges (USER, non-root, after the steps that + genuinely need root) + - the documented runtime keeps the UID mapped to the host user + (--userns=keep-id) + +An image running as UID 1000 without keep-id maps to a subuid, which +loses the 0660 proxy sockets and the writable data directory. That +isn't a hardened container, it's a broken one, and it fails at +runtime on a machine none of the tests can reach -- so the coupling +is asserted here instead. +""" + +import re +from pathlib import Path + +_ROOT = Path(__file__).resolve().parents[1] +_CONTAINERFILE = (_ROOT / "Containerfile").read_text(encoding="utf-8") +_COMPOSE = (_ROOT / "deploy" / "compose.example.yaml").read_text(encoding="utf-8") +_DEPLOY_README = (_ROOT / "deploy" / "README.md").read_text(encoding="utf-8") + + +def _directive_lines(prefix: str) -> list[str]: + return [ + line.strip() + for line in _CONTAINERFILE.splitlines() + if line.startswith(f"{prefix} ") + ] + + +def test_the_image_declares_a_user(): + assert _directive_lines("USER"), ( + "Containerfile has no USER directive, so the serving process runs " + "as container root and can rewrite /app/src/forge/ (audit E-4)." + ) + + +def test_the_declared_user_is_not_root(): + for line in _directive_lines("USER"): + user = line.removeprefix("USER ").strip().strip('"') + assert user.split(":")[0] not in ("0", "root"), line + + +def test_privileges_are_dropped_after_the_steps_that_need_them(): + """apt-get and pip must still run as root; only the CMD needs to be + unprivileged. A USER placed too early breaks the build instead of + the runtime, which is at least loud -- but it also tempts the next + person to move it back rather than fix the ordering.""" + lines = _CONTAINERFILE.splitlines() + user_index = next(i for i, line in enumerate(lines) if line.startswith("USER ")) + for i, line in enumerate(lines): + if line.startswith("RUN ") and ("apt-get" in line or "pip install" in line): + assert i < user_index, f"privileged step after USER: {line}" + + +def test_the_compose_example_keeps_the_uid_mapped_to_the_host_user(): + """The runtime half. Without this the image's UID 1000 lands on a + subuid and loses both proxy sockets and ./data.""" + assert re.search(r"^\s*userns_mode:\s*\"keep-id", _COMPOSE, re.MULTILINE), ( + "deploy/compose.example.yaml must set userns_mode: keep-id -- a " + "non-root image without it is a broken container, not a safer one." + ) + + +def test_the_escape_hatch_is_present_and_commented_out(): + """`user: "0:0"` restores the previous behaviour with no rebuild. + It has to be shipped commented, and stay commented: uncommented it + would silently undo the whole change.""" + assert '# user: "0:0"' in _COMPOSE + assert not re.search(r"^\s*user:\s*\"0:0\"", _COMPOSE, re.MULTILINE) + + +def test_the_runtime_requirement_is_documented_next_to_the_run_commands(): + """A flag this load-bearing can't live only in a commit message -- + the person hitting the failure is reading deploy/README.md.""" + assert "--userns=keep-id" in _DEPLOY_README + assert "Running non-root" in _DEPLOY_README diff --git a/tests/test_dependency_pinning.py b/tests/test_dependency_pinning.py new file mode 100644 index 0000000..103a71c --- /dev/null +++ b/tests/test_dependency_pinning.py @@ -0,0 +1,73 @@ +""" +Forcing function for audit E-4: the requirement files stay pinned. + +Pinning is only worth anything if it survives the next person (or the +next Evolution Runtime patch) adding a bare `some-lib` line. The +Containerfile installs from these files on every build, so an +unpinned line there is a silent, unreviewable change to the shipped +image. This test makes that a red suite instead. + +Deliberately not checked here: whether the pins are current, or +whether they match what's installed in this venv. Freshness is a +judgement call for a human bumping versions on purpose; the shape of +the file is what a test can hold. +""" + +from pathlib import Path + +import pytest + +_ROOT = Path(__file__).resolve().parents[1] +_REQUIREMENT_FILES = ["requirements.txt", "requirements-dev.txt"] + + +def _requirement_lines(filename: str) -> list[str]: + text = (_ROOT / filename).read_text(encoding="utf-8") + return [ + line.strip() + for line in text.splitlines() + if line.strip() and not line.lstrip().startswith("#") + ] + + +@pytest.mark.parametrize("filename", _REQUIREMENT_FILES) +def test_every_requirement_is_pinned(filename): + unpinned = [line for line in _requirement_lines(filename) if "==" not in line] + assert not unpinned, ( + f"{filename} has unpinned requirements: {unpinned}. " + "Pin them (see the header of the file for how to regenerate)." + ) + + +@pytest.mark.parametrize("filename", _REQUIREMENT_FILES) +def test_no_range_specifiers_alongside_the_pin(filename): + """`foo==1.2,>=1.0` is a pin in appearance only -- it still lets + the resolver move. Same for the `!=` / `~=` families.""" + loose = [ + line + for line in _requirement_lines(filename) + if any(op in line for op in (">=", "<=", "~=", "!=", ">", "<")) + ] + assert not loose, f"{filename} mixes range specifiers into pins: {loose}" + + +def test_containerfile_has_exactly_one_from_line(): + """deploy/pin-base-image.sh --write rewrites the FROM line with a + single regex substitution and refuses to guess if it matches + anything other than once. A second FROM (a multi-stage build, say) + would make the pinning script silently pin the wrong stage, so + that assumption is held here rather than discovered later.""" + lines = (_ROOT / "Containerfile").read_text(encoding="utf-8").splitlines() + from_lines = [line for line in lines if line.startswith("FROM ")] + assert len(from_lines) == 1, ( + f"expected one FROM line, found {len(from_lines)}: {from_lines}. " + "If this became a multi-stage build, teach pin-base-image.sh " + "which stage to pin before adding the stage." + ) + + +def test_the_files_are_not_empty(): + """Guards the guard: a truncated or renamed file would make both + tests above pass on an empty list.""" + for filename in _REQUIREMENT_FILES: + assert _requirement_lines(filename), f"{filename} has no requirements at all" diff --git a/tests/test_podman_ro_proxy.py b/tests/test_podman_ro_proxy.py index 1606ef5..5ef731e 100644 --- a/tests/test_podman_ro_proxy.py +++ b/tests/test_podman_ro_proxy.py @@ -6,7 +6,11 @@ """ import importlib.util +import socketserver import sys +import threading +import time +from http.server import BaseHTTPRequestHandler from pathlib import Path _PROXY_PATH = Path(__file__).resolve().parents[1] / "deploy" / "podman_ro_proxy.py" @@ -70,3 +74,196 @@ def test_rejects_mutation_paths(): def test_rejects_path_traversal_style_attempts(): assert not _is_allowed("GET", "/containers/json/../../start") assert not _is_allowed("GET", "/containers/abc/logs/../start") + + +def test_rejects_follow_in_any_form(): + """A follow request is a GET on an allowed path, so the allowlist + alone lets it straight through -- and podman then never closes + the connection, parking a handler thread and an upstream socket + permanently. Three of those and sysadmin's log collection is dead + without a single rule being broken (audit M-1). + + Every spelling is refused, including the ones podman itself would + read as false: guessing which strings someone else's decoder + calls true is how a filter and its target end up disagreeing.""" + for query in ( + "?follow=true", + "?follow=1", + "?follow=TRUE", + "?follow", + "?follow=false", + "?stdout=true&follow=true", + "?FOLLOW=true", + ): + path = f"/v4.9.0/libpod/containers/abc123/logs{query}" + assert not _is_allowed("GET", path), f"unexpectedly allowed: GET {path}" + + +def test_still_allows_the_query_parameters_sysadmin_actually_sends(): + """The follow check must not turn into a blanket ban on query + strings -- `podman logs --tail N` is the whole point of this + endpoint.""" + assert _is_allowed("GET", "/v4.9.0/libpod/containers/abc123/logs?tail=200") + assert _is_allowed("GET", "/containers/abc123/logs?stdout=true&stderr=true") + + +# ─── Forwarding behaviour (real sockets, real HTTP) ───────────────── +# The tests above cover the filter in isolation. These start the proxy +# in front of a fake podman on a real Unix socket, because the M-1 +# failures -- serialised handling, an upstream that never answers, an +# unbounded body -- only exist in the plumbing the filter never sees. + + +class _FakeUpstream: + """Minimal stand-in for podman.sock. `delay` simulates a slow + podman, `body` a large one.""" + + def __init__(self, tmpdir: Path, body: bytes = b"ok", delay: float = 0.0): + self.path = str(tmpdir / "upstream.sock") + self._body = body + self._delay = delay + + outer = self + + class Handler(BaseHTTPRequestHandler): + protocol_version = "HTTP/1.0" + + def do_GET(self): + if outer._delay: + time.sleep(outer._delay) + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(outer._body))) + self.end_headers() + self.wfile.write(outer._body) + + def log_message(self, fmt, *args): + pass + + class Server(socketserver.ThreadingMixIn, socketserver.UnixStreamServer): + daemon_threads = True + + def handle_error(self, request, client_address): + # The timeout test deliberately makes the proxy hang up + # on this fake podman mid-response; a broken pipe here + # is the expected outcome, not a failure to report. + pass + + self._server = Server(self.path, Handler) + self._thread = threading.Thread(target=self._server.serve_forever, daemon=True) + + def __enter__(self): + self._thread.start() + return self + + def __exit__(self, *exc): + self._server.shutdown() + self._server.server_close() + + +class _ProxyUnderTest: + def __init__(self, tmpdir: Path, upstream_path: str, **handler_kwargs): + self.path = str(tmpdir / "proxy.sock") + handler = podman_ro_proxy.make_handler(upstream_path, **handler_kwargs) + self._server = podman_ro_proxy._UnixSocketHTTPServer(self.path, handler) + self._thread = threading.Thread(target=self._server.serve_forever, daemon=True) + + def __enter__(self): + self._thread.start() + return self + + def __exit__(self, *exc): + self._server.shutdown() + self._server.server_close() + + def get(self, path: str, timeout: float = 10.0): + conn = podman_ro_proxy._UnixHTTPConnection(self.path, timeout=timeout) + try: + conn.request("GET", path) + resp = conn.getresponse() + return resp.status, resp.read(), dict(resp.getheaders()) + finally: + conn.close() + + +def test_forwards_an_allowed_request_unchanged(tmp_path): + with ( + _FakeUpstream(tmp_path, body=b'[{"Names":["forge"]}]') as upstream, + _ProxyUnderTest(tmp_path, upstream.path) as proxy, + ): + status, body, _ = proxy.get("/v4.9.0/libpod/containers/json") + assert status == 200 + assert body == b'[{"Names":["forge"]}]' + + +def test_answers_504_when_podman_does_not_respond_in_time(tmp_path): + """Without a timeout on the upstream connection the handler blocks + forever on read() and the thread never comes back. 504 also has to + be distinguishable from the 403 the filter returns, or a wedged + podman looks like a policy rejection (audit M-1).""" + with ( + _FakeUpstream(tmp_path, delay=2.0) as upstream, + _ProxyUnderTest(tmp_path, upstream.path, timeout=0.2) as proxy, + ): + status, body, _ = proxy.get("/containers/json") + assert status == 504 + assert b"timeout" in body.lower() + + +def test_caps_an_oversized_response_and_corrects_content_length(tmp_path): + """`?tail=all` against a large journal is an unbounded read into + the host proxy's memory. Truncating without rewriting + Content-Length would be worse than the truncation: the client + waits on bytes that never arrive.""" + with ( + _FakeUpstream(tmp_path, body=b"x" * 5000) as upstream, + _ProxyUnderTest(tmp_path, upstream.path, max_bytes=1000) as proxy, + ): + status, body, headers = proxy.get("/containers/abc/logs") + assert status == 200 + assert len(body) == 1000 + assert headers["Content-Length"] == "1000" + + +def test_a_body_exactly_at_the_cap_is_not_truncated(tmp_path): + """Off-by-one guard: the read asks for cap+1 bytes precisely so an + exactly-at-cap body isn't reported as cut short.""" + with ( + _FakeUpstream(tmp_path, body=b"x" * 1000) as upstream, + _ProxyUnderTest(tmp_path, upstream.path, max_bytes=1000) as proxy, + ): + status, body, _ = proxy.get("/containers/abc/logs") + assert status == 200 + assert len(body) == 1000 + + +def test_slow_requests_do_not_queue_behind_each_other(tmp_path): + """The single-threaded server served one request start to finish + before looking at the next, so one slow `podman logs` stalled + everything -- including the client's own /_ping. Four concurrent + half-second requests should take about half a second, not two.""" + with ( + _FakeUpstream(tmp_path, delay=0.5) as upstream, + _ProxyUnderTest(tmp_path, upstream.path) as proxy, + ): + results: list[int] = [] + lock = threading.Lock() + + def hit(): + status, _, _ = proxy.get("/containers/json") + with lock: + results.append(status) + + threads = [threading.Thread(target=hit) for _ in range(4)] + start = time.monotonic() + for thread in threads: + thread.start() + for thread in threads: + thread.join() + elapsed = time.monotonic() - start + + assert results == [200, 200, 200, 200] + # Serialised would be ~2.0s; concurrent is ~0.5s. The threshold is + # deliberately loose -- this asserts "not serialised", not a + # latency budget, so a slow CI runner doesn't turn it red. + assert elapsed < 1.5, f"requests appear to be serialised: {elapsed:.2f}s for 4" diff --git a/tests/test_ratelimit_expiry.py b/tests/test_ratelimit_expiry.py new file mode 100644 index 0000000..8ac08f1 --- /dev/null +++ b/tests/test_ratelimit_expiry.py @@ -0,0 +1,110 @@ +""" +Tests for the rate limiter's key lifecycle (security audit, M-2). + +Counting was already correct; keeping was not. Every client IP that +ever made a request got a dict entry that lived as long as the +process. These tests are about what gets thrown away and, just as +importantly, what doesn't -- an over-eager expiry would hand a +throttled client a fresh allowance, which is worse than the leak it +replaced. +""" + +import time + +import pytest + +from forge import ratelimit + + +@pytest.fixture(autouse=True) +def _clean_limiter(monkeypatch): + monkeypatch.setattr(ratelimit, "RATE_LIMIT_ENABLED", True) + ratelimit.reset() + yield + ratelimit.reset() + + +def test_keys_of_departed_clients_are_dropped(monkeypatch): + """The leak itself: 50 one-shot clients used to leave 50 entries + behind for good.""" + monkeypatch.setattr(ratelimit, "RATE_LIMIT_REQUESTS", 5) + monkeypatch.setattr(ratelimit, "RATE_LIMIT_WINDOW_SECONDS", 0.05) + + for i in range(50): + ratelimit.check(f"10.0.0.{i}") + assert ratelimit.tracked_keys() == 50 + + time.sleep(0.06) + ratelimit.check("10.0.0.99") # any request triggers the due sweep + + assert ratelimit.tracked_keys() == 1 + + +def test_an_active_client_is_never_swept(monkeypatch): + """Expiry must be observationally invisible. A client still inside + its window keeps its counter -- otherwise the sweep is a way to + reset the limit by waiting.""" + monkeypatch.setattr(ratelimit, "RATE_LIMIT_REQUESTS", 3) + monkeypatch.setattr(ratelimit, "RATE_LIMIT_WINDOW_SECONDS", 60) + + for _ in range(3): + assert ratelimit.check("192.168.1.5")[0] is True + + for i in range(20): # traffic from elsewhere, sweeps may run + ratelimit.check(f"172.16.0.{i}") + + allowed, retry_after = ratelimit.check("192.168.1.5") + assert allowed is False + assert retry_after >= 1 + + +def test_a_returning_client_gets_the_full_allowance(monkeypatch): + """The other half of the same guarantee: once a key has aged out, + dropping it and keeping it are the same thing from the client's + side.""" + monkeypatch.setattr(ratelimit, "RATE_LIMIT_REQUESTS", 2) + monkeypatch.setattr(ratelimit, "RATE_LIMIT_WINDOW_SECONDS", 0.05) + + assert ratelimit.check("10.1.1.1")[0] is True + assert ratelimit.check("10.1.1.1")[0] is True + assert ratelimit.check("10.1.1.1")[0] is False + + time.sleep(0.06) + assert ratelimit.check("10.1.1.1")[0] is True + + +def test_the_key_ceiling_bounds_a_burst_inside_one_window(monkeypatch): + """The timed sweep bounds memory over time; it does nothing about + a flood of distinct addresses arriving faster than one window. The + ceiling is what covers that case.""" + monkeypatch.setattr(ratelimit, "RATE_LIMIT_REQUESTS", 5) + monkeypatch.setattr(ratelimit, "RATE_LIMIT_WINDOW_SECONDS", 3600) + monkeypatch.setattr(ratelimit, "_MAX_TRACKED_KEYS", 20) + + for i in range(200): + ratelimit.check(f"10.2.{i // 256}.{i % 256}") + + assert ratelimit.tracked_keys() <= 21 # ceiling, plus the key just added + + +def test_reading_a_key_does_not_create_one(monkeypatch): + """The dict was a defaultdict, so anything iterating or inspecting + it by key could grow it -- including, eventually, the sweep meant + to shrink it.""" + monkeypatch.setattr(ratelimit, "RATE_LIMIT_REQUESTS", 5) + monkeypatch.setattr(ratelimit, "RATE_LIMIT_WINDOW_SECONDS", 60) + + ratelimit.check("10.3.3.3") + before = ratelimit.tracked_keys() + ratelimit._hits.get("never-seen") + assert ratelimit.tracked_keys() == before + + +def test_disabled_limiter_stores_nothing(monkeypatch): + """RATE_LIMIT_ENABLED=false should cost nothing at all, memory + included -- it's the documented escape hatch for anyone fronting + this with a proxy that already rate-limits.""" + monkeypatch.setattr(ratelimit, "RATE_LIMIT_ENABLED", False) + for i in range(100): + assert ratelimit.check(f"10.4.0.{i}") == (True, 0) + assert ratelimit.tracked_keys() == 0 diff --git a/tests/test_sysadmin.py b/tests/test_sysadmin.py index a5b8842..ebf0e0a 100644 --- a/tests/test_sysadmin.py +++ b/tests/test_sysadmin.py @@ -48,8 +48,9 @@ def _fake_run_fixed(cmd, timeout): return _fake_busctl_units_json(["searxng.service", "forge.service"]) if cmd == sysadmin_mod._DISCOVER_CONTAINERS_CMD(): return "test-container" - if cmd[0] == "journalctl" and "-u" in cmd: - return f"log line for unit {cmd[cmd.index('-u') + 1]}" + unit_flags = [arg for arg in cmd if arg.startswith("--unit=")] + if cmd[0] == "journalctl" and unit_flags: + return f"log line for unit {unit_flags[0].removeprefix('--unit=')}" if cmd[0] == "podman": return f"log line for container {cmd[-1]}" if cmd[0] == "journalctl" and "-k" in cmd: @@ -671,3 +672,46 @@ def test_sysadmin_cleans_json_wrapped_response_like_review(monkeypatch): ) assert state.final_output == '{"tool":"chat","content":"query"}' + + +# ─── Argument shape (audit M-4) ───────────────────────────────────── + + +def test_collect_cmd_attaches_the_unit_name_to_its_flag(): + """`-u foo` puts the name in a position where a leading dash is + read as an option: `-u --output=cat` would be journalctl's flag, + not a unit name. The attached `--unit=` form has no such + position -- everything after the `=` is the value, dash or not.""" + cmd = sysadmin_mod._collect_cmd("unit", "--output=cat") + assert "--unit=--output=cat" in cmd + assert "-u" not in cmd + # And nothing that looks like a loose option was introduced. + assert "--output=cat" not in cmd + + +def test_collect_cmd_puts_a_container_name_after_the_options_marker(): + """podman takes the container as a positional argument, so a name + starting with a dash is parsed as a flag. `--` ends option parsing: + whatever follows is a name, even if it's spelled `--help`.""" + cmd = sysadmin_mod._collect_cmd("container", "--help") + assert cmd[-2:] == ["--", "--help"] + + +def test_ordinary_names_are_unaffected(): + """The hardening must not change what the normal path actually + runs -- these two commands are what production executes.""" + unit_cmd = sysadmin_mod._collect_cmd("unit", "searxng.service") + assert "--unit=searxng.service" in unit_cmd + assert unit_cmd[0] == "journalctl" + + container_cmd = sysadmin_mod._collect_cmd("container", "forge") + assert container_cmd[0] == "podman" + assert container_cmd[-1] == "forge" + + +def test_kernel_collection_takes_no_name_at_all(): + """The kernel branch never interpolates anything, which is why it + is the safe fallback for an unrecognised target_hint.""" + cmd = sysadmin_mod._collect_cmd("kernel", "ignored") + assert "ignored" not in cmd + assert "-k" in cmd