Answer the Docker socket on loopback, not on every address the host has - #195
Conversation
davidmckayv
left a comment
davidmckayv
left a comment
There was a problem hiding this comment.
Verified on the running stack, not from the diff.
The exposure was live on this machine. Before: openbot-supervisor-1 0.0.0.0:4500->4300/tcp, while agent-computer, agent-bot and agent-langgraph were already loopback-only. That process mounts the Docker socket, so reaching it is root-equivalent on the host behind one shared env-var token.
After recreating from this compose file:
openbot-supervisor-1 127.0.0.1:4500->4300/tcp
loopback 127.0.0.1:4500 -> 200
LAN 10.0.0.75:4500 -> 000 (connection refused)
Reachable where it needs to be, gone from the routed network.
Local development is not broken, which was my main worry. I ran scripts/start.sh end to end afterwards: it reaches the supervisor on localhost:4500, per-Bot computers are created normally (one computer per Bot), and a Bot drove a browser to completion. In-compose callers use supervisor:4300 and never touched the published port.
The compose file was also contradicting docs/architecture.md, which already said not to expose this. A template ships its defaults into every clone, so that disagreement was going to be resolved in the wrong direction by whoever read the compose file first.
Worth a follow-up, not this PR: postgres is still published on 0.0.0.0:5432 with openbot/openbot. Same class, same file, and this PR's test deliberately does not cover it. Happy to raise it separately.
Checks: stacked with ten other candidates on current main. Typecheck, lint, format clean; 1371 tests pass, 0 fail.
The supervisor's port was published with no interface in front of it, so it bound every address the machine had. That is the wrong default anywhere and worst here: this is the service that mounts /var/run/docker.sock, so reaching it is root on the host by way of four verbs, and SUPERVISOR_TOKEN is a shared secret rather than a network boundary. The computer already says exactly this about its own port, a few lines up, and binds 127.0.0.1 for it. So do the Bots. The supervisor and PostgreSQL were the two that did not, and the supervisor is the one holding the socket. Only the published mapping changes. The process still listens on every interface inside its own container, which is what a deployment running the server inside the network depends on: it sets COMPUTER_NETWORK and reaches this as supervisor:4300, never through the host mapping. Binding the process itself to loopback would have broken both paths, because Docker forwards to the container's interface address rather than its loopback. Verified against docker 29.1.3 before and after: published without a prefix answers on loopback and on the host's routable address; with the prefix it answers on loopback and refuses the routable one; a container on the same network still reaches it by service name. The test pins every service that holds a secret, not just this one, and names them rather than applying a blanket rule, so adding a service stays a decision about where it should answer. It fails on the unprefixed port for each of the four independently.
… the computer The port change on its own leaves four places describing the old behaviour, and one of them was already describing the behaviour we wanted rather than the one the compose file had. docs/architecture.md said "do not expose it outside the deployment network" and stopped there, which is the instruction the published port was breaking. It now says where Compose binds it, the way the paragraph above it already does for the computer, and names the in-network path so a deployment that needs no published port knows it has one. The README's loopback bullet named computers only. The supervisor belongs in it for a stronger reason than the computer does. supervisor/src/index.ts argued that the vocabulary is the boundary and the token is not, which is still true and was the whole argument. It now also says what sits in front of both, and that the listener stays on every interface inside its own container, because that is the part someone will otherwise "fix" and break the in-network deployment. The changelog entry is written for the person upgrading: what stops working is reaching the supervisor from another machine, which was the thing worth stopping, and it says what to do instead.
ad4af3b to
28b5d7c
Compare
2 participants
Closes #194.
What this changes
The supervisor's port was published as
"${SUPERVISOR_PORT:-4500}:4300". With no interface in front of it Docker binds every address the host has, so the service holding/var/run/docker.sockanswered anything that could route to the machine.SUPERVISOR_TOKENis a shared secret in an environment variable, not a network boundary.docs/architecture.mdalready said "do not expose it outside the deployment network", so the compose file was breaking an instruction this repository had already written down.agent-computer,agent-botandagent-langgraphare published on127.0.0.1already. The supervisor is the one where that argument is strongest and the prefix was missing.One line of
docker-compose.ymlchanges, plus the places that describe the behaviour:docs/architecture.md, the README's loopback bullet, the module comment insupervisor/src/index.ts, and aCHANGELOG.mdentry.Where it runs
supervisor:4300over the compose network otherwise, which is the path a hosted deployment uses and which needs no published port. Scaling is unchanged by this change; one supervisor owns one Docker host's socket, as before.Boundary and audit
Changelog
CHANGELOG.mdunderUnreleased, written for the person upgrading: reaching the supervisor from another machine stops working, that was the point, and here is what to do instead.Proof
Docker 29.1.3, a stand-in service on 4300, four cases:
127.0.0.1127.0.0.1(after)The third row is why this is a compose change and not a
serve()change: Docker forwards to the container's interface address rather than its loopback, so binding the process itself would have broken the published mapping and the in-network path together.tests/compose.test.tsgains a case pinning every service that holds a secret, not only this one. It was red before the fix and is green after, and it fails independently for each of the four services, checked by removing the prefix fromCOMPUTER_PORTandBOT_PORTin turn.Quality checks:
bun run format:checkclean on tracked files.bun run lintclean, 416 files.bun run typecheckclean for app, server and worker.supervisortypechecked separately withbunx tsc --noEmit, since the root script does not reach it and CI checks it in thedeployablesmatrix.bun run test1251 tests. The failing set is 111 names, byte-identical before and after the change, all pre-existing integration tests that need PostgreSQL.bun run buildexit 0.No lockfile changes. The architecture diagram renders the host port, which has not changed, so it needs no regeneration.