Consolidated per-host gateway for the macOS (Apple container) backend #399

Merged

didericis merged 9 commits from macos-consolidated-gateway into main 2026-07-17 17:14:03 -04:00

Conversation 1 Commits 9 Files Changed 29

+2273 -101

didericis-claude commented 2026-07-17 01:01:00 -04:00

Collaborator

Copy Link

Copy Source

Part of #351 (PRD 0070). The last backend in the roadmap — macOS (Apple container), which #385 left failing closed after the per-bottle companion container was removed.

Summary

Re-enables macos-container on the shared per-host orchestrator + gateway. The agent attaches to one shared host-only network and proxies egress through the single gateway, attributed by its source address.

Apple Container 1.0.0 forced three departures from the docker shape. Each is a platform constraint I verified against the live CLI on this host, not a style choice — they're recorded with transcripts in the networking spike.

No --ip. --network takes only <name>[,mac=…][,mtu=…]; the address comes from vmnet's DHCP and is knowable only once the container runs. So the order inverts:

docker:  allocate IP -> pin with --ip -> register -> launch
macOS:   gateway up  -> run agent -> read its address -> register

That is why consolidated_launch.py exposes ensure_gateway() + register_agent() where docker has one function — the caller starts the agent in between.

No container DNS. Containers can't resolve each other by name, so the gateway is handed the control plane's IP. That IP doesn't exist until the orchestrator runs, so orchestrator_service.py starts the orchestrator first and then points the gateway at it — the reverse of docker's order. It also means no --publish: the host reaches the host-only network directly, so the host CLI and the gateway share one URL.

No network connect. Networks are fixed at container run, so the shared network is created up front. Per-bottle networks would force a gateway restart per launch and defeat the consolidation.

The identity token moved

Registration mints the token after the container exists, so it can't be baked into the run-time env the way docker's compose spec does. It's applied at container exec time instead, which works because exec inherits run-time env and --env overrides it. This is load-bearing rather than cosmetic: /resolve requires a matching (source_ip, identity_token) pair and fail-closes with no source-IP-only fallback (#366 has landed). The agent's init is a bare sleep and every real command arrives via exec, so everything carries the token; anything that didn't would be denied, which is the safe direction.

The token rides bare --env names so its value never lands on argv (ps is world-readable).

Security: dropping CAP_NET_RAW

Apple grants NET_RAW by default. It does not grant NET_ADMIN, so an agent can't change its own address or route:

$ container exec agent ip addr add 192.168.128.99/24 dev eth0
ip: RTNETLINK answers: Operation not permitted

But NET_RAW permits raw sockets — i.e. forging a neighbour's source address on the shared segment, directly against PRD 0070's invariant ("a packet's source address, as seen by the orchestrator, provably identifies the originating bottle"). The agent is therefore run with --cap-drop CAP_NET_RAW, which I confirmed clears the bit and blocks raw sockets.

Worth noting for the PRD: 0070 lists the Apple invariant as simply "the host-only network". That isn't sufficient on its own — the host-only network alone leaves NET_RAW spoofing open. The cap drop is what closes it.

Verification

Driven end-to-end against real Apple Container 1.0.0 (container 1.0.0, macOS 26.5.1, arm64), not just mocked:

• Both Dockerfile.orchestrator and Dockerfile.gateway build under container build.
• Control plane comes up healthy; the gateway reaches it by IP; the gateway's default route is the NAT leg (NAT-network-first ordering) and it egresses fine.
• A real agent container on the shared network got its DHCP address, registered by it, and received a token.
• Policy enforcement: with the token, a host in the bottle's routes returned 200; a host outside them returned 403.
• Bring-up is idempotent — a second call does not churn the singletons (that property is what keeps N launches from dropping other bottles' data plane).

Two real bugs were caught during review and fixed, both with regression tests:

1. The gateway wasn't recreated when the orchestrator's address changed. Docker is immune (it uses a stable container name); on Apple the URL is an IP baked into the gateway's env, so a recreated orchestrator on a new address would have silently denied egress for every bottle on the host.
2. Bottle.exec() didn't carry the token, so a provider whose provision step fetches anything would have egressed token-less and been denied.

1815 unit tests pass, pyright clean, pylint 9.92.

Notes for review

• Not verified: the full test_sandbox_escape suite on macOS. I proved the egress-policy path end-to-end; the git-gate/DLP attacks aren't exercised here.
• Live egress route-apply fails closed, matching the docker backend's current posture — not a macOS-specific regression.
• The macOS orchestrator shares the one host bot-bottle.db with the docker/fc orchestrators, consistent with the existing multi-backend coexistence the docker lifecycle already documents.

Part of #351 (PRD 0070). The last backend in the roadmap — macOS (Apple container), which #385 left failing closed after the per-bottle companion container was removed. ## Summary Re-enables `macos-container` on the shared per-host orchestrator + gateway. The agent attaches to one shared host-only network and proxies egress through the single gateway, attributed by its source address. Apple Container 1.0.0 forced three departures from the docker shape. Each is a platform constraint I verified against the live CLI on this host, not a style choice — they're recorded with transcripts in [the networking spike](https://gitea.dideric.is/didericis/bot-bottle/src/commit/e2107f4bb33e7c5f152022cec46a5d7561559ad4/docs/research/apple-container-networking-spike.md#addendum-consolidated-gateway-findings-2026-07-17-prd-0070). **No `--ip`.** `--network` takes only `<name>[,mac=…][,mtu=…]`; the address comes from vmnet's DHCP and is knowable only once the container runs. So the order inverts: ``` docker: allocate IP -> pin with --ip -> register -> launch macOS: gateway up -> run agent -> read its address -> register ``` That is why [`consolidated_launch.py`](https://gitea.dideric.is/didericis/bot-bottle/src/commit/e2107f4bb33e7c5f152022cec46a5d7561559ad4/bot_bottle/backend/macos_container/consolidated_launch.py) exposes `ensure_gateway()` + `register_agent()` where docker has one function — the caller starts the agent in between. **No container DNS.** Containers can't resolve each other by name, so the gateway is handed the control plane's **IP**. That IP doesn't exist until the orchestrator runs, so [`orchestrator_service.py`](https://gitea.dideric.is/didericis/bot-bottle/src/commit/e2107f4bb33e7c5f152022cec46a5d7561559ad4/bot_bottle/backend/macos_container/orchestrator_service.py) starts the orchestrator *first* and then points the gateway at it — the reverse of docker's order. It also means no `--publish`: the host reaches the host-only network directly, so the host CLI and the gateway share one URL. **No `network connect`.** Networks are fixed at `container run`, so the shared network is created up front. Per-bottle networks would force a gateway restart per launch and defeat the consolidation. ## The identity token moved Registration mints the token *after* the container exists, so it can't be baked into the run-time env the way docker's compose spec does. It's applied at `container exec` time instead, which works because exec inherits run-time env and `--env` overrides it. This is load-bearing rather than cosmetic: `/resolve` requires a matching `(source_ip, identity_token)` pair and fail-closes with no source-IP-only fallback (#366 has landed). The agent's init is a bare `sleep` and every real command arrives via exec, so everything carries the token; anything that didn't would be denied, which is the safe direction. The token rides bare `--env` names so its value never lands on argv (`ps` is world-readable). ## Security: dropping CAP_NET_RAW Apple grants **NET_RAW by default**. It does not grant NET_ADMIN, so an agent can't change its own address or route: ```console $ container exec agent ip addr add 192.168.128.99/24 dev eth0 ip: RTNETLINK answers: Operation not permitted ``` But NET_RAW permits raw sockets — i.e. forging a neighbour's source address on the shared segment, directly against PRD 0070's invariant ("a packet's source address, as seen by the orchestrator, provably identifies the originating bottle"). The agent is therefore run with `--cap-drop CAP_NET_RAW`, which I confirmed clears the bit and blocks raw sockets. Worth noting for the PRD: 0070 lists the Apple invariant as simply "the host-only network". That isn't sufficient on its own — the host-only network alone leaves NET_RAW spoofing open. The cap drop is what closes it. ## Verification Driven end-to-end against real Apple Container 1.0.0 (`container 1.0.0`, macOS 26.5.1, arm64), not just mocked: - Both `Dockerfile.orchestrator` and `Dockerfile.gateway` build under `container build`. - Control plane comes up healthy; the gateway reaches it by IP; the gateway's default route is the NAT leg (NAT-network-first ordering) and it egresses fine. - A real agent container on the shared network got its DHCP address, registered by it, and received a token. - **Policy enforcement:** with the token, a host in the bottle's routes returned **200**; a host outside them returned **403**. - Bring-up is idempotent — a second call does not churn the singletons (that property is what keeps N launches from dropping other bottles' data plane). Two real bugs were caught during review and fixed, both with regression tests: 1. The gateway wasn't recreated when the orchestrator's **address** changed. Docker is immune (it uses a stable container name); on Apple the URL is an IP baked into the gateway's env, so a recreated orchestrator on a new address would have silently denied egress for *every* bottle on the host. 2. `Bottle.exec()` didn't carry the token, so a provider whose provision step fetches anything would have egressed token-less and been denied. `1815` unit tests pass, pyright clean, pylint 9.92. ## Notes for review - **Not verified:** the full `test_sandbox_escape` suite on macOS. I proved the egress-policy path end-to-end; the git-gate/DLP attacks aren't exercised here. - **Live egress route-apply** fails closed, matching the docker backend's current posture — not a macOS-specific regression. - The macOS orchestrator shares the one host `bot-bottle.db` with the docker/fc orchestrators, consistent with the existing multi-backend coexistence the docker lifecycle already documents.

didericis-claude referenced this pull request 2026-07-17 01:01:17 -04:00

Per-host orchestrator service (consolidate per-bottle sidecars; virtualized, backend-native) #351

didericis added 2 commits 2026-07-17 01:27:44 -04:00

feat(macos): consolidated per-host gateway for the Apple backend (PRD 0070) ... c69642e568

Re-enables the macos-container backend on the shared per-host orchestrator +
gateway, replacing the per-bottle companion container removed in #385. This is
the last backend in PRD 0070's roadmap.

Apple Container 1.0.0 forced three departures from the docker shape, each
verified against the live CLI (findings recorded in the networking spike):

- No `--ip`. The address is DHCP-assigned and knowable only once the container
  runs, so the order inverts: gateway up -> run agent -> read its address ->
  register. The identity token is minted by registration and therefore cannot
  be in the agent's run-time env; it rides the proxy URL applied at
  `container exec` time (bare `--env` names keep it off argv).
- No container DNS. The gateway can only be handed the control plane's IP, so
  the orchestrator starts first and the gateway is pointed at its address.
- No `network connect`. Networks are fixed at run time, so the shared host-only
  network is created up front; per-bottle networks would restart the gateway
  on every launch and defeat the consolidation.

The agent runs with `--cap-drop CAP_NET_RAW`: Apple grants NET_RAW by default,
which would let an agent forge a neighbour's source address on the shared
segment. NET_ADMIN is already absent, so this closes the source-address half of
PRD 0070's attribution invariant.

Verified end-to-end on real Apple Container 1.0.0: both images build, the
control plane comes up healthy, the gateway reaches it by IP, and a registered
agent gets 200 for a host in its routes and 403 for one outside them. Bring-up
is idempotent — a second launch does not churn the singletons.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

fix(test): stop the macOS unit tests shelling out to the container CLI ... a5910696a5

CI's unit + coverage jobs failed with `FileNotFoundError: 'container'`: three
tests reached the real Apple CLI, which exists on a macOS dev host but not on
the Linux runner. They passed locally for that reason alone — and two of them
were quietly creating real Apple networks on the dev host as a side effect.

- `test_enumerate_active_is_empty_while_disabled` asserted the disabled-era
  stub and called `enumerate_active()` unmocked. The backend launches bottles
  again, so it now covers the real enumeration: slug parsing, exclusion of the
  shared gateway/orchestrator singletons, and the CLI-failure path.
- The two orchestrator tests patched `orchestrator_service.container_mod`, but
  `_run_orchestrator_container` reaches the CLI through `ensure_networks`,
  which is imported from the gateway module and resolves `container_mod` in
  *its* namespace. Patch the imported name instead.

Adds a test that the networks exist before the orchestrator runs — the
ordering the escaped call was hiding.

Verified by reproducing the CI environment locally (`PATH` without the
`container` binary): 3 failures before, 1818 passing after.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

didericis force-pushed macos-consolidated-gateway from 965ee67c46 to a5910696a5 2026-07-17 01:27:44 -04:00 Compare

didericis added 1 commit 2026-07-17 03:27:13 -04:00

fix(macos): review fixes — token on plan, self-heal, symmetric digest, DHCP poll ... e24b62b6b9

Addresses findings from a high-effort review of the PRD 0070 macOS backend.

Correctness:
- Stamp identity_token onto MacosContainerBottlePlan after registration. git's
  gitconfig extraHeader and the supervise MCP --header read
  getattr(plan,"identity_token","") at provision time, and both reach the
  gateway on NO_PROXY (bypassing the egress proxy that carries the token). The
  plan never carried it, so /resolve fail-closed and every git fetch/push and
  supervise call from a macOS bottle would have been denied. Registration
  precedes provision(), so — unlike the run-time env — the plan can carry it.
- Self-heal the orchestrator: recreate when it is not (source-current AND
  answering /health), not on the source-hash label alone. A container running
  current code but with a wedged HTTP server was left alone and polled to
  death, failing every launch until manual deletion.
- image_digest and container_image_digest now read the same descriptor.digest
  field; dropped image_digest's id/tag fallback that could yield a value the
  container side can't produce — a permanent mismatch would have recreated the
  shared gateway on every launch (severing every live bottle's egress, since
  the replacement gets a new DHCP address).
- Poll for the agent's and gateway's DHCP address instead of a fatal read
  right after `container run` (there is no --ip; the address can lag start).

Cleanup:
- One _inspect_first + _descriptor_digest behind the four inspect readers.
- Shared bind_mount_spec (util) and host_db_dir (paths) replace per-module
  copies; _GIT_HTTP_PORT now imports git_http_backend.DEFAULT_PORT.
- Drop the dead _url cache / url property and the write-only agent_proxy_url.

Deferred (noted on the PR, not fixed here): the gateway image rebuilding on
every launch (needs source-hash-labeled build), SQLite shared across VM
guests, and the sh -lc profile-override edge — each is design-level or
behavior-risk beyond a review fix.

Verified: real Apple Container bring-up is green and idempotent; 1826 unit
tests pass with `container` absent (CI parity), pyright clean, pylint 9.86.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

didericis added 1 commit 2026-07-17 04:14:18 -04:00

refactor(macos): one infra container (control plane + gateway), fixes shared-DB races ... 4a607ad098

Adopts the firecracker infra-VM pattern for macOS: the orchestrator control
plane and the gateway data plane now run in a SINGLE Apple container instead of
two. Apple Containers are lightweight VMs with separate kernels, so the prior
two-container design had both guests writing one bot-bottle.db over virtiofs,
where fcntl locks are not coherent across kernels — concurrent writes (the
orchestrator's registry vs the gateway supervise daemon's queue) could corrupt
it. One container = one kernel = coherent locking.

The DB moves onto a container-only Apple volume (bot-bottle-mac-db), never
bind-mounted from the host, so no host process opens the live file either. The
host CLI already reaches registry + supervise state over the control-plane HTTP
surface (cli/supervise.py uses OrchestratorClient), exactly as firecracker's
VM-only DB requires.

Two simplifications fall out of the single container:
- No DNS dance: the control plane and gateway daemons reach each other over
  127.0.0.1, so the orchestrator-before-gateway ordering (a workaround for
  Apple having no container DNS) is gone, along with the moved-IP recreate
  logic it needed.
- Net -243 lines.

Mechanics: the infra container runs from the gateway image with the
control-plane source bind-mounted read-only (like the docker orchestrator, so a
code change needs no rebuild) and a small sh -c init that starts both processes
(mirrors firecracker's _infra_init). Also implements the macOS backend's
ensure_orchestrator() and adds it to discover_orchestrator_url, so operator
tools (supervise) can bring up / find the control plane on demand — previously
the macOS backend died with "no orchestrator control plane".

Verified end-to-end on real Apple Container 1.0.0: the single infra container
comes up healthy (one address for control plane + gateway), both processes run,
the DB is written on the container-only volume, host-side supervise works over
HTTP, and a registered agent gets 200 for an allowed host / 403 for a denied
one. 1824 unit tests pass with `container` absent (CI parity), pyright clean,
pylint 9.89.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

didericis-claude referenced this pull request 2026-07-17 04:43:58 -04:00

Audit: control-plane exposure on macOS consolidated backend + WebSocket DLP gap (security/architecture/quality) #400

didericis-claude referenced this pull request 2026-07-17 04:51:52 -04:00

Audit: control-plane exposure on macOS consolidated backend + WebSocket DLP gap (security/architecture/quality) #400

didericis added 1 commit 2026-07-17 05:15:23 -04:00

fix(security): authenticate the orchestrator control plane (issue #400 Critical) ... ca91fc4d91

The control plane had no caller authentication — dispatch() routed every
request straight through, and the only guards anywhere were the
(source_ip, identity_token) checks inside /resolve and /attribute, which
authenticate the *bottle* a request is about, not the *caller*. On the
consolidated backends the agent shares a network with the control-plane port
(macOS: the infra container runs 8099 on the agent's --internal network;
docker: the orchestrator container sits on the shared gateway network), so a
prompt-injected agent could GET /bottles (enumerate siblings + policies),
PUT /bottles/<id>/policy (rewrite its own egress allowlist to allow-all),
POST /resolve (read the injected upstream credentials it's never meant to
see), and POST /supervise/respond (self-approve its own proposals). Every
README guarantee collapsed to the agent's discretion.

Fix: a per-host control-plane secret required on every route but GET /health,
compared with hmac.compare_digest. It is held only by the trusted callers and
never handed to an agent:
- minted + persisted 0600 at <root>/control-plane-token (paths.host_control_plane_token);
- injected as $BOT_BOTTLE_CONTROL_PLANE_TOKEN into the orchestrator + gateway
  containers via bare `--env NAME` (value inherited from the launch process,
  so it never lands on argv or in `container/docker inspect`);
- presented by the gateway's PolicyResolver (reads the env) on /resolve, and by
  the host CLI's OrchestratorClient (reads the host file) on every call.

The agent container is never given the env var or the host file, so from a
bottle every /bottles*, /resolve, /attribute, and /supervise/* call now
returns 401 — closing the enumeration, allowlist-rewrite, credential-lift, and
self-approval. The existing (source_ip, identity_token) checks stay as
defense-in-depth.

Enforced when configured: macOS + docker inject the secret (→ enforced). With
no secret set the server runs open and warns loudly at startup — a
fail-visible fallback for the unit suite and for Firecracker, whose
port-scoped nft already blocks agents from 8099 (wiring the secret into its
infra-VM init is a clean fast-follow, left out here to avoid churning the
prebuilt-artifact hash).

Verified end-to-end on real Apple Container: infra comes up healthy, the host
CLI (with the secret) lists bottles while an unauthenticated GET /bottles gets
401, all five issue-#400 attacks from inside the agent get 401, and egress
policy still works (200 allowed / 403 denied) — proving the gateway
authenticates to /resolve with the secret. 1829 unit tests pass, pyright
clean, pylint 9.91.

Refs #400.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

didericis-claude referenced this pull request 2026-07-17 05:20:18 -04:00

Audit: control-plane exposure on macOS consolidated backend + WebSocket DLP gap (security/architecture/quality) #400

didericis added 1 commit 2026-07-17 16:17:46 -04:00

test(orchestrator): live Docker coverage for the control-plane auth fix (#400) ... 4f36b919b2

ca91fc4 fixed the control plane's missing caller authentication but only
verified it end-to-end on Apple Container; Docker had the same code change
(docker_cmd.py's env injection) backed only by the in-process dispatch()
unit tests, which never exercise the real HTTP server or a real container.

Ran it for real first: brought up the actual orchestrator + gateway via
Docker, hit the published control-plane port directly. Confirms /health
stays open, /bottles and /resolve both 401 with no token or a wrong one
(the enumeration and credential-lift vectors from #400), and /bottles
succeeds with the real per-host secret.

Added as a proper integration test so this stays covered: a subclass of
OrchestratorService giving the gateway its own unique name too (the base
class only parameterizes the orchestrator's), plus a throwaway
BOT_BOTTLE_ROOT and a random port, so a run can never collide with a real
host's orchestrator or gateway. Gated on a reachable Docker daemon, same
as the existing docker gateway/broker integration tests.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

didericis added 2 commits 2026-07-17 16:45:55 -04:00

fix(test): actually isolate the docker control-plane auth test's root + network ... d32f36bb2b

Two bugs surfaced by a review of the previous commit:

- host_control_plane_token() resolves its path via the ambient
  BOT_BOTTLE_ROOT env var, not the host_root kwarg passed to
  OrchestratorService (that kwarg only controls the DB bind-mount
  destination). The test's isolation claim was false as a result: running
  it read/wrote the developer's real ~/.bot-bottle/control-plane-token
  instead of the throwaway temp dir — confirmed directly on disk. Fixed
  by pointing the env var at the same temp dir for the test's duration
  and restoring it via addCleanup.

- ensure_running() creates a per-bottle Docker network but stop() only
  ever removes containers, never the network — every run of this test
  leaked one bridge network permanently (found and removed 5 from prior
  runs via `docker network ls`). Fixed with an explicit `docker network
  rm` in addCleanup.

Verified: re-ran the suite twice: 5/5 pass both times, the real
~/.bot-bottle/control-plane-token timestamp is unchanged across both runs
(proving isolation), and `docker network ls` shows zero leaked
bot-bottle-net-itest-* networks afterward. pyright clean, pylint 10.00/10.

Remaining findings from the same review (missing GITEA_ACTIONS skip
guard, root-owned bind-mount cleanup on native Linux, no setUpClass,
reinvented OrchestratorClient, gateway_name should be a constructor
param rather than a subclassed private-method override) are left for a
follow-up — each is a real, separate design/scope call, not a
mechanical fix like these two.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

fix(test): skip the control-plane auth test under act_runner (CI) ... 492669e620

Pushing the previous two fixes surfaced a real, currently-failing CI run
(gitea actions run 2164, jobs "integration" and "coverage"): every one of
the 5 new tests errored in setUp with

  docker: Error response from daemon: error while creating mount source
  path '/workspace/didericis/bot-bottle': mkdir /workspace: read-only
  file system

This is finding #4 from the review of the previous commit, now confirmed
live rather than just plausible: act_runner's job-container topology
can't satisfy the orchestrator's host-path bind mount, the same
constraint test_multitenant_isolation.py, test_gateway_image.py, and
test_sandbox_escape.py already skip around. Add the identical
skip_unless_docker + GITEA_ACTIONS guard so this test degrades the same
way its siblings do instead of failing the job.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

didericis added 1 commit 2026-07-17 17:00:36 -04:00

fix(test): resolve the remaining review findings on the control-plane auth test ... 5c526860bc

Addresses the 5 lower-priority findings left as follow-up in the earlier
review, now that each has a concrete answer:

- Add gateway_name: str = GATEWAY_NAME to OrchestratorService.__init__
  (mirrors the existing orchestrator_name param) and thread it through
  _gateway(). Deletes the test's _IsolatedOrchestratorService subclass,
  which existed only to override a private method for this one kwarg —
  any caller needing gateway-name isolation can now use the public
  constructor. Backward compatible: every existing caller constructs
  OrchestratorService with keyword args and a sensible default is kept.

- Give the test its own fixed image tags (bot-bottle-orchestrator:itest,
  bot-bottle-gateway:itest) instead of the production :latest ones.
  _running_image_is_current() keys gateway staleness off the image tag's
  ID, not per-instance identity, so rebuilding the shared :latest tag from
  whatever's on disk during a test run could make a real host's running
  production gateway look stale and get force-recreated. Fixed tags (not
  per-run-suffixed, so they don't accumulate) fully decouple the two.

- setUp -> setUpClass/tearDownClass: all 5 tests are read-only checks
  against the same running control plane, so one shared container
  lifecycle replaces 5 (each of which paid its own container-start +
  image-build + health-poll cycle). Cuts the file's wall-clock roughly
  4x (11.5s -> 2.9-4.3s) and, combined with the network-rm cleanup from
  the previous commit, means one cleanup instead of five.

- Reuse OrchestratorClient (bot_bottle/orchestrator/client.py) instead of
  a hand-rolled urllib helper — the test now exercises the same
  request/response code path the real host CLI uses, rather than a
  private copy that could silently drift from it.

- Add the chown workaround test_multitenant_isolation.py already needed
  for this exact bind-mount: the orchestrator container has no USER
  directive, so it writes the registry DB as root into the throwaway
  host_root; chown it back before tempdir cleanup so that doesn't raise
  PermissionError on native Linux Docker (no UID remap, unlike Docker
  Desktop's macOS VM).

Verified: ran the suite twice in a row (idempotency — fixed image tags
don't accumulate, 5/5 pass both times, 2.99-4.33s each), the real
~/.bot-bottle/control-plane-token is untouched, zero leaked networks or
containers after either run, exactly 2 :itest images (not growing), the
full orchestrator unit suite (93 tests) and the sibling docker
gateway/broker integration tests still pass. pyright clean, pylint
10.00/10 on both changed files.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

didericis-claude commented 2026-07-17 17:09:46 -04:00

Author

Collaborator

Copy Link

Copy Source

Ran a review of the new integration test (test_orchestrator_docker_control_plane_auth.py) and fixed everything it found. Summary for anyone reviewing:

Verified the fix itself first, live, before writing anything: brought up a real Docker orchestrator + gateway, hit the control-plane port directly. /health stays open with no token; /bottles and /resolve both return 401 with no token or a wrong one (the enumeration and credential-lift attacks from #400); /bottles succeeds with the real per-host secret. Docker backend now has the same live verification macOS got.

Then reviewed the new test itself and fixed 8 findings:

1. Isolation was broken — host_control_plane_token() reads/writes the ambient BOT_BOTTLE_ROOT env var, not the host_root constructor kwarg (that kwarg only controls the DB bind-mount destination). The "isolated" test was actually reading/writing my real ~/.bot-bottle/control-plane-token — confirmed directly on disk. Fixed by pointing the env var at the same throwaway dir for the test's duration.
2. Shared-image force-recreate risk — _running_image_is_current() keys gateway staleness off the image tag's ID, not per-instance identity, and the test didn't override image/gateway_image. A test run rebuilding the shared :latest tag from local disk state could make a real host's running production gateway look stale and get force-recreated mid-flight. Fixed with fixed (non-per-run) test-only tags (:itest), decoupled from production, that don't accumulate across runs.
3. Leaked a Docker network every run — stop() only removes containers, never the network ensure_running() creates. Found and removed 5 leaked networks from earlier runs. Fixed with explicit docker network rm in cleanup.
4. Broke CI — missing the GITEA_ACTIONS skip guard every sibling Docker-bind-mount integration test has (test_multitenant_isolation.py, test_gateway_image.py, test_sandbox_escape.py). This one wasn't theoretical: pushing surfaced a real failing run (mkdir /workspace: read-only file system, all 5 tests erroring in setUp). Added the same guard.
5. Root-owned cleanup on native Linux — the orchestrator container has no USER directive, so it writes the registry DB as root into the throwaway host_root; TemporaryDirectory.cleanup() would then hit PermissionError on real Linux Docker (not reproducible on macOS, where Docker Desktop's VM remaps ownership). Added the same chown workaround test_multitenant_isolation.py already uses.
6. Reinvented OrchestratorClient — the test hand-rolled its own urllib request helper instead of reusing the real host-side client, so it wasn't actually exercising the code path production callers use. Swapped to OrchestratorClient.
7. 5x redundant container startups — no setUpClass, so all 5 read-only test methods each paid their own container-start + image-build + health-poll cycle. Converted to setUpClass/addClassCleanup; cut the file's wall-clock from ~11.5s to ~3-4s.
8. Private-method subclass workaround — the test subclassed OrchestratorService and overrode its private _gateway() just to give the gateway a unique name. Added a real gateway_name: str = GATEWAY_NAME constructor param (mirrors the existing orchestrator_name) so any caller needing this can use the public constructor — backward compatible, deleted the subclass entirely.

Verified end to end: ran the suite twice in a row (idempotency — fixed tags don't accumulate), confirmed zero leaked networks/containers and an untouched real token file after each run, exactly 2 :itest images (not growing), the full orchestrator unit suite (93 tests) and the sibling docker gateway/broker integration tests still green, pyright clean, pylint 10.00/10 on every changed file. CI is green on the current head.

Ran a review of the new integration test (`test_orchestrator_docker_control_plane_auth.py`) and fixed everything it found. Summary for anyone reviewing: **Verified the fix itself first**, live, before writing anything: brought up a real Docker orchestrator + gateway, hit the control-plane port directly. `/health` stays open with no token; `/bottles` and `/resolve` both return 401 with no token or a wrong one (the enumeration and credential-lift attacks from #400); `/bottles` succeeds with the real per-host secret. Docker backend now has the same live verification macOS got. **Then reviewed the new test itself and fixed 8 findings:** 1. **Isolation was broken** — `host_control_plane_token()` reads/writes the *ambient* `BOT_BOTTLE_ROOT` env var, not the `host_root` constructor kwarg (that kwarg only controls the DB bind-mount destination). The "isolated" test was actually reading/writing my real `~/.bot-bottle/control-plane-token` — confirmed directly on disk. Fixed by pointing the env var at the same throwaway dir for the test's duration. 2. **Shared-image force-recreate risk** — `_running_image_is_current()` keys gateway staleness off the image *tag's* ID, not per-instance identity, and the test didn't override `image`/`gateway_image`. A test run rebuilding the shared `:latest` tag from local disk state could make a real host's running production gateway look stale and get force-recreated mid-flight. Fixed with fixed (non-per-run) test-only tags (`:itest`), decoupled from production, that don't accumulate across runs. 3. **Leaked a Docker network every run** — `stop()` only removes containers, never the network `ensure_running()` creates. Found and removed 5 leaked networks from earlier runs. Fixed with explicit `docker network rm` in cleanup. 4. **Broke CI** — missing the `GITEA_ACTIONS` skip guard every sibling Docker-bind-mount integration test has (`test_multitenant_isolation.py`, `test_gateway_image.py`, `test_sandbox_escape.py`). This one wasn't theoretical: pushing surfaced a real failing run (`mkdir /workspace: read-only file system`, all 5 tests erroring in `setUp`). Added the same guard. 5. **Root-owned cleanup on native Linux** — the orchestrator container has no `USER` directive, so it writes the registry DB as root into the throwaway `host_root`; `TemporaryDirectory.cleanup()` would then hit `PermissionError` on real Linux Docker (not reproducible on macOS, where Docker Desktop's VM remaps ownership). Added the same `chown` workaround `test_multitenant_isolation.py` already uses. 6. **Reinvented `OrchestratorClient`** — the test hand-rolled its own urllib request helper instead of reusing the real host-side client, so it wasn't actually exercising the code path production callers use. Swapped to `OrchestratorClient`. 7. **5x redundant container startups** — no `setUpClass`, so all 5 read-only test methods each paid their own container-start + image-build + health-poll cycle. Converted to `setUpClass`/`addClassCleanup`; cut the file's wall-clock from ~11.5s to ~3-4s. 8. **Private-method subclass workaround** — the test subclassed `OrchestratorService` and overrode its private `_gateway()` just to give the gateway a unique name. Added a real `gateway_name: str = GATEWAY_NAME` constructor param (mirrors the existing `orchestrator_name`) so any caller needing this can use the public constructor — backward compatible, deleted the subclass entirely. Verified end to end: ran the suite twice in a row (idempotency — fixed tags don't accumulate), confirmed zero leaked networks/containers and an untouched real token file after each run, exactly 2 `:itest` images (not growing), the full orchestrator unit suite (93 tests) and the sibling docker gateway/broker integration tests still green, pyright clean, pylint 10.00/10 on every changed file. CI is green on the current head.

didericis merged commit 5c526860bc into main 2026-07-17 17:14:03 -04:00

didericis deleted branch macos-consolidated-gateway 2026-07-17 17:14:03 -04:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

Compat/Breaking

Breaking change that won't be backward compatible

Kind/Bug

Something is not working

Kind/Documentation

Documentation changes

Kind/Enhancement

Improve existing functionality

Kind/Feature

New functionality

Kind/Security

This is security issue

Kind/Testing

Issue or pull request related to testing

Priority

Critical

1

The priority is critical

Priority

High

2

The priority is high

Priority

Low

4

The priority is low

Priority

Medium

3

The priority is medium

Reviewed

Confirmed

1

Issue has been confirmed

Reviewed

Duplicate

2

This issue or pull request already exists

Reviewed

Invalid

3

Invalid issue

Reviewed

Won't Fix

3

This issue won't be fixed

Status

Abandoned

3

Somebody has started to work on this but abandoned work

Status

Blocked

1

Something is blocking this issue or pull request

Status

Need More Info

2

Feedback is required to reproduce issue or to continue work

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

didericis-claude (didericis (claude)) didericis-codex (didericis (codex)) didericis-gemma (gemma) didericis

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: didericis/bot-bottle#399