Security audit, batch 2: hardening - #13
Merged
Merged
Conversation
The Containerfile runs `pip install -r requirements.txt` on every build, and requirements.txt named four packages with no versions. That meant two builds of the same commit could ship different code, and any compromised release anywhere in the transitive tree -- starlette, urllib3, h11, uvloop -- landed in the image with nothing in the diff to show it. The dependency change stopped being a commit and became a side effect of rebuilding. Both files are now fully pinned, transitive deps included, split into a "direct" and a "transitive" section so it's still obvious which four packages are actually chosen and which are consequences. The versions are exactly what an unpinned resolve produced today, so this is a freeze, not an upgrade: verified by installing the pinned files into a clean venv and diffing `pip freeze` against the unpinned resolution -- identical, and `pip check` clean. requirements-dev.txt is pinned for the same reason and arguably a stronger one: CI installs it on a runner holding a repo-scoped GITHUB_TOKEN. Not done, and said out loud in the file header rather than left to be assumed: `--require-hashes`. It's the version of this that also survives a hostile index, but it needs a hash per artifact and a lock regenerated per platform. PyPI's no-reupload rule covers most of what it would buy here. tests/test_dependency_pinning.py is the forcing function -- the pins only hold if the next bare `some-lib` line fails the suite.
`FROM python:3.12` is a mutable tag. It's 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 -- exactly the drift the requirements pins just closed, one layer down. deploy/pin-base-image.sh resolves the tag to its RepoDigest and, with --write, rewrites the FROM line to the immutable name@sha256:... form. Re-running it is how you bump: the move shows up as a one-line diff to review and commit, instead of happening by itself on the next rebuild. The FROM line is deliberately left as a tag in this commit rather than shipped with a digest baked in. A digest is only worth anything if it's one resolved on the build host, from the registry actually pulled from -- a digest committed from somewhere else is a number nobody verified. Running the script once on the Deck is the step that finishes this item. The test holds the script's one real assumption: a single FROM line. A second stage would make the regex pin the wrong one.
…(audit M-1)
The read-only filter was sound; the server around it was not. Three
ways an allowed request could take the proxy down, none of them
breaking a single rule in the allowlist:
- `GET /containers/{id}/logs?follow=true` is a GET on an allowed
path, and podman answers it by never closing the connection. The
handler reads the whole upstream body before replying, so one
follow request parks a handler thread and an upstream socket
permanently.
- UnixStreamServer handles one request at a time, start to finish.
A slow `podman logs` blocked everything behind it, including the
client's own /_ping.
- There was no timeout on the upstream connection at all, so a
wedged podman held the handler for good rather than for a while.
Availability is part of what a read-only proxy promises: sysadmin
losing its only view of container logs is a real outcome even though
nothing was mutated.
follow is refused on presence of the parameter, not on its value.
`?follow=false` is refused too -- deciding which spellings podman's
own decoder reads as true means reimplementing someone else's
boolean parsing and being right about it, which is the classic way a
filter and the thing it filters end up disagreeing. Forge never
sends the parameter in any form.
The server is now threaded with daemon threads, and the upstream
connection has a 30s default timeout answering 504 (kept distinct
from the filter's 403, so a wedged podman doesn't read as a policy
rejection). Client disconnects no longer print a traceback each --
that's how an operator learns to ignore this proxy's output.
One addition beyond the audit item, same failure mode: an 8 MiB cap
on the forwarded body. `?tail=all` against a large journal was an
unbounded read into the host proxy's memory. When it trips,
Content-Length is rewritten to the truncated length -- forwarding
upstream's would leave the client waiting on bytes that never
arrive, which is a hang rather than a short read.
Tests run the real thing over real Unix sockets against a fake
podman: the plumbing is where all three bugs lived, so filter-level
assertions wouldn't have caught any of them. The concurrency test
asserts "not serialised" with a loose threshold rather than a
latency budget.
Two unauthenticated surfaces, for different reasons. /docs, /redoc and /openapi.json are mounted by FastAPI itself, not by this app's routes, so Depends(require_token) cannot be attached to them -- the only available states are "published to anyone who can reach the port" and "not mounted". They published a complete, machine-readable map of every endpoint, parameter and schema. Not a vulnerability on its own; it makes the reconnaissance step free for someone who has learned nothing else about the instance. Now off unless API_DOCS_ENABLED=true, which is the switch to flip while developing against the API. /health was explicitly exempt from the rate limiter, and there was a test asserting that it always would be. That test is deliberately inverted here. The exemption read as safe while /health was "a status string nobody can abuse" -- but with FORGE_PROVIDER=llama_cpp every hit makes an outbound call to the inference server to read the loaded model name. Unmetered, one cheap anonymous request becomes load on the LLM. It stays unauthenticated (a container healthcheck and the UI's status line both call it before they have a token) and there's a test pinning that, so the limit can't quietly become a gate. The docs tests run the disabled and enabled cases in a subprocess: docs_url is read once when the FastAPI object is constructed at import time, so monkeypatching the flag afterwards proves nothing, and reloading forge.api in-process would hand the rest of the suite a different module object than the one it holds.
…ags (audit M-4) `journalctl -u <name>` and `podman logs ... <name>` both put the name in a position where a leading dash turns it into an option. A unit named `--output=cat`, or a container named `--help`, would be interpreted by the tool rather than looked up. Now `--unit=<name>` (attached: everything after the `=` is the value) and `-- <name>` (end-of-options marker). This is a second lock on a door collect_node already bolted: a name only reaches _collect_cmd if it appeared verbatim in discovery output, and unit and container names don't normally start with a dash. What it covers is the case where discovery output is no longer trustworthy -- a hostile container name, or a proxy answering with something the host never said. That assumption is exactly what the validation rests on, which makes it the one worth not resting the whole thing on. The test fake matched on `-u` being present in the argv, so it had to learn the attached form. Worth noting rather than glossing: a mock keyed on argument shape is coupled to the argument shape, and this is the kind of change that quietly turns such a mock into a test of nothing. It failed loudly here, which is the good version.
…t M-2) The counter dict was append-only: one entry per client IP that ever made a request, held for the life of the process. On a personal instance that's a slow leak; for anyone able to vary their source address it's a fast one. A rate limiter that can be made to exhaust memory is a poor trade for what it protects. Two bounds, because they cover different shapes of the same problem: - A sweep, amortised to one pass per window rather than per request, drops keys whose hits have all aged out. That bounds memory over time. - A ceiling on tracked keys, checked independently of the timer, bounds it *within* a single window -- where a flood of distinct addresses can grow the dict faster than any schedule collects it. Over the ceiling, the last resort evicts least-recently-seen entries. That does hand those clients a fresh allowance, and the comment says so: they're the ones who hammered least recently, and the alternative is the process dying, which lifts the limit for everyone at once. The guarantee the expiry must not break is that dropping a key and keeping it are observationally identical -- a key is only removed once its newest hit is older than the window, so a returning client gets exactly the allowance it would have had. Two tests hold that from both sides: an active client is never swept, and a client past its window comes back with a full budget. defaultdict is gone. Reading a key must not create one when the thing iterating the dict is the sweep meant to shrink it. Both new paths were mutation-tested: disabling the timed sweep fails the leak test, disabling the eviction fails the ceiling test.
Resolved on the Deck with ./deploy/pin-base-image.sh --write. python:3.12 is a mutable tag, rebuilt for every CPython patch release and every Debian security update, so this fixes the base image this commit actually runs on. The next bump is a one-line diff to review rather than something that happens by itself on the next rebuild.
The serving process ran as container root, so anything reaching code execution inside -- a `shell` or `files` dispatch, most obviously -- could rewrite /app/src/forge/ and everything else in the image. It never needed that: 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/`). The image now creates forge:forge at 1000:1000, owns /app/data and /home/forge to it, pre-compiles /app/src as root so no __pycache__ write is attempted later, and drops with USER after the steps that genuinely need root. HOME is set explicitly: the podman client resolves config and scratch paths from it, and /root existed while /home/forge only exists because it was just created. The part worth being blunt about: **this image without --userns=keep-id at runtime is a broken container, not a hardened one.** Under rootless podman the container's UID 0 is already mapped to the host user, and that mapping is what makes the 0660 proxy sockets and ./data reachable. A container UID of 1000 maps by default to a subuid, a stranger to all of it -- sysadmin loses both proxies and Forge loses its writable directory. keep-id puts UID 1000 back where container-root used to be. compose.example.yaml and deploy/README.md now carry that flag next to every run command, and a test asserts the two halves stay together, because the failure lands at runtime on a machine no test can reach. The CMD's mkdir becomes best-effort. sd_booted() checks only that /run/systemd/system exists -- nothing writes into it -- and the build-time mkdir covers that unless podman mounts a tmpfs over /run. In the case where it does and we can't write there, taking the whole API down over a sysadmin dependency is the wrong trade; it warns once on stderr instead, where the alternative is a confusing systemctl error much later. README documents the tmpfs-on-the-directory fix. Escape hatch, shipped commented in the compose example: `user: "0:0"` overrides the image USER with no rebuild and restores exactly the previous posture. A test keeps it commented, since uncommented it would silently undo all of this. Not verified here, and it is the one to watch on the Deck: --group-add keep-groups and --userns=keep-id are two different mechanisms acting on the same entry path, and keep-groups is what `journalctl -u` on root-owned units depends on. README's verification list checks it explicitly as step 2.
The CMD created /run/systemd/system on every start so that `systemctl` would clear its sd_booted() check before attempting a bus connection. Nothing calls systemctl any more: discovery moved to `busctl --json=short` because systemctl hardcodes a connection to /run/systemd/private and ignores DBUS_SYSTEM_BUS_ADDRESS. busctl has no such check, so the directory has had no reader for a while. That makes the warning added in the non-root commit worse than useless -- the day it fired it would send someone looking at a dependency that no longer exists. Removed along with the mkdir it guarded, and CMD goes back to plain exec form: uvicorn is PID 1 with no shell in between, which is what the `exec` in the old string was buying. Also corrects the comments that still described systemctl as the thing being run: the docstrings in sysadmin.py now say busctl, since that's what DBUS_SYSTEM_BUS_ADDRESS actually points at. The one place systemctl is still named is the returncode guard, where the history is the point -- its two-line failure message was what got parsed as two fake unit names in production. The command is gone; the failure mode isn't, so the guard and its account stay. No behaviour change: the removed code had no effect on a system where its only consumer is absent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security audit, batch 2: hardening
Follow-up to #12 (batch 1). Where batch 1 closed the things that were
exploitable, this batch closes the things that make exploitation
cheap, and the ones that let an allowed request do damage without
breaking a rule.
Eight commits, each independently revertable. Test suite goes from
406 to 439,
ruff checkandruff format --checkclean.What's in it
Dependency and image pinning (E-4).
requirements.txtnamed fourpackages with no versions, and the Containerfile installs from it on
every build — so two builds of the same commit could ship different
code, and a compromised release anywhere in the transitive tree landed
in the image with nothing in the diff to show it. Both requirement
files are now fully pinned, transitive deps included. This is a
freeze, not an upgrade: the pinned files were installed into a clean
venv and
pip freezediffed against the unpinned resolution —identical,
pip checkclean.deploy/pin-base-image.shdoes the sameone layer down, resolving
python:3.12to its RepoDigest; it was runon the build host, and the
FROMline is now a digest.podman read-only proxy (M-1). The path filter was sound; the
server around it was not. Three ways an allowed request could take
the proxy down without breaking a single allowlist rule:
?follow=trueis a GET on an allowed path that podman answers by never closing the
connection;
UnixStreamServerhandled one request at a time, so aslow
podman logsblocked everything behind it; and there was noupstream timeout at all. Now threaded, 30s timeout answering 504 (kept
distinct from the filter's 403, so a wedged podman doesn't read as a
policy rejection), and
followrefused on presence of the parameterrather than on its value — guessing which spellings podman's own
decoder reads as true means reimplementing someone else's boolean
parsing and being right about it. One addition beyond the audit item,
same failure mode: an 8 MiB cap on the forwarded body, since
?tail=allagainst a large journal was an unbounded read into thehost proxy's memory.
Unauthenticated surfaces (M-3).
/docs,/redocand/openapi.jsonare mounted by FastAPI itself, sorequire_tokencan'tcover them — the only available states are "published to anyone who
can reach the port" and "not mounted". Now not mounted, unless
API_DOCS_ENABLED=true./healthwas explicitly exempt from the ratelimiter and had a test asserting it always would be; that test is
deliberately inverted here. The exemption read as safe while
/healthwas "a status string nobody can abuse", but with
FORGE_PROVIDER=llama_cppevery hit makes an outbound call to theinference server. It stays unauthenticated — healthchecks and the UI
status line call it before they have a token — with a test pinning
that, so the limit can't quietly become a gate.
Argument shape in sysadmin (M-4).
journalctl -u <name>andpodman logs ... <name>both put the name where a leading dash turnsit into an option. Now
--unit=<name>and-- <name>. This is asecond lock on a door
collect_nodealready bolted; what it covers isthe case where discovery output is no longer trustworthy, which is
exactly the assumption the validation rests on.
Rate limiter key expiry (M-2). The counter dict was append-only:
one entry per client IP that ever made a request, held for the life of
the process. Two bounds now, covering different shapes of the same
problem — an amortised sweep (one pass per window) bounds memory over
time, and a key ceiling bounds it within a window, where a flood of
distinct addresses can grow the dict faster than any schedule collects
it. The guarantee the expiry must not break is that dropping a key and
keeping it are observationally identical; two tests hold that from both
sides. Both paths were mutation-tested.
Non-root container (E-4). The serving process ran as container
root and never needed to: uvicorn binds an unprivileged port and the
only writable path is
/app/data. Nowforge:forgeat 1000:1000.The part worth stating plainly: this image without
--userns=keep-idis a broken container, not a hardened one —under rootless podman the container's UID 0 is already mapped to the
host user, and that mapping is what makes the 0660 proxy sockets and
./datareachable.compose.example.yamlanddeploy/README.mdcarry the flag, and a test keeps the two halves together.
Cleanup. The CMD created
/run/systemd/systemon every start forsystemctl's sd_booted() check. Nothing calls systemctl any more —discovery moved to
busctl— so the directory has had no reader for awhile. Removed, CMD back to plain exec form, stale comments corrected.
Verified on the Deck
Non-root was tested on the real deployment, not just in CI:
uid_mapshows1000 0 1— keep-id's signature, container UID 1000is the host user
podman exec forge id→uid=1000(forge), writes to/app/datasucceed
journalctl -D /host-journal -u steamos-manager.servicereturns real entries — supplementary groupaccess survives the switch, because the kernel evaluates the journal
ACL against host-side GIDs rather than the names inside the container
user: "0:0"in compose reverts the non-root change with no rebuild,and ships commented for that purpose.
Not in this batch
E-2 (provenance), E-1 (
files/testsandbox scope) and F-4(
SECURITY.md) are batch 3.