didericis/bot-bottle
1
0
Fork 0
Code Issues 34 Pull Requests 7 Actions Packages Releases Wiki Activity

refactor(gateway): remove the single-tenant data-plane paths (audit #400 finding 3) #402

Merged
didericis merged 4 commits from refactor/remove-single-tenant-data-plane into main 2026-07-17 22:56:48 -04:00
Conversation 2 Commits 4 Files Changed 14
+439 -435
didericis-claude commented 2026-07-17 18:03:27 -04:00
Collaborator
Copy Link
Copy Source

What

Removes the legacy single-tenant (resolver is None) data-plane paths from the shared gateway, closing finding #3 of the audit in #400. All three backends (docker, firecracker, macos-container) now launch through the consolidated orchestrator and every production gateway sets BOT_BOTTLE_ORCHESTRATOR_URL, so these branches were unreachable dead code — a second, security-relevant path to keep correct in parallel with the live one (finding #2 was exactly a case where one such path had rotted).

Confirmed before cutting: the docker backend's launch.py calls launch_consolidated()/teardown_consolidated() (its docstring was stale); orchestrator/gateway.py, firecracker/infra_vm.py, and macos_container/infra.py all set the orchestrator URL. No production gateway runs single-tenant.

Scope (full teardown, one PR — as requested)

egress_addon.py — drops the static routes file entirely: EGRESS_ROUTES, _reload, the SIGHUP handler, self.config, and the SUPERVISE_BOTTLE_SLUG env slug. The per-request /resolve is the only policy source; __init__ fail-closes without an orchestrator URL. Introspection now reports the calling bottle's resolved routes. The block/redact log gates and _req_ctx redaction now read the per-flow config/env from the request-time stash (they silently used the empty static config before).

git_http_backend.py — resolver mandatory, no flat-root fallback; main() refuses to start without an orchestrator URL; an unattributed source 404s.

supervise_server.py — resolver mandatory; every proposal attributed to the source-IP-resolved bottle. Removes handle_list_egress_routes (the proxy-fetch introspection that only worked when the proxy carried one bottle's identity); list-egress-routes is answered from the resolved policy.

Safety

Nothing sends docker kill --signal HUP to the gateway in the consolidated model — the egress applicators (docker + macOS) raise rather than signal — so removing the SIGHUP reload can't strand a signal that would now kill the egress process. Verified.

Tests

A host-side fake resolver serves each test's Config through the real parse path (a small YAML-subset emitter round-trips route_to_yaml_dict); the response/websocket hooks stash it as request() would. Deletes tests for the removed static-config / SIGHUP-reload / single-tenant-passthrough paths; adds fail-closed-without-orchestrator coverage for all three entry points (smoke-tested: git-http exits 1, supervise exits 2, egress __init__ raises).

  • pyright: 0 errors. pylint: source 9.90/10, tests 9.47/10. All egress (197), git-http (18), and supervise (97) tests green. Net −57 lines.
  • The 13 test_gateway_init failures on the dev host are the pre-existing /bin/sleep-on-NixOS issue, unrelated.

Follow-ups (out of the data-plane teardown, noted in the commit)

  • gateway_init still forwards SIGHUP to the egress child (now dormant — nothing sends it).
  • The README still describes the docker backend's per-bottle topology.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ

## What Removes the legacy **single-tenant** (`resolver is None`) data-plane paths from the shared gateway, closing finding #3 of the audit in #400. All three backends (docker, firecracker, macos-container) now launch through the consolidated orchestrator and every production gateway sets `BOT_BOTTLE_ORCHESTRATOR_URL`, so these branches were unreachable dead code — a second, security-relevant path to keep correct in parallel with the live one (finding #2 was exactly a case where one such path had rotted). Confirmed before cutting: the docker backend's `launch.py` calls `launch_consolidated()`/`teardown_consolidated()` (its docstring was stale); `orchestrator/gateway.py`, `firecracker/infra_vm.py`, and `macos_container/infra.py` all set the orchestrator URL. No production gateway runs single-tenant. ## Scope (full teardown, one PR — as requested) **`egress_addon.py`** — drops the static routes file entirely: `EGRESS_ROUTES`, `_reload`, the SIGHUP handler, `self.config`, and the `SUPERVISE_BOTTLE_SLUG` env slug. The per-request `/resolve` is the only policy source; `__init__` fail-closes without an orchestrator URL. Introspection now reports the calling bottle's *resolved* routes. The block/redact log gates and `_req_ctx` redaction now read the per-flow config/env from the request-time stash (they silently used the empty static config before). **`git_http_backend.py`** — resolver mandatory, no flat-root fallback; `main()` refuses to start without an orchestrator URL; an unattributed source 404s. **`supervise_server.py`** — resolver mandatory; every proposal attributed to the source-IP-resolved bottle. Removes `handle_list_egress_routes` (the proxy-fetch introspection that only worked when the proxy carried one bottle's identity); `list-egress-routes` is answered from the resolved policy. ## Safety Nothing sends `docker kill --signal HUP` to the gateway in the consolidated model — the egress applicators (docker + macOS) **raise** rather than signal — so removing the SIGHUP reload can't strand a signal that would now kill the egress process. Verified. ## Tests A host-side fake resolver serves each test's `Config` through the real parse path (a small YAML-subset emitter round-trips `route_to_yaml_dict`); the response/websocket hooks stash it as `request()` would. Deletes tests for the removed static-config / SIGHUP-reload / single-tenant-passthrough paths; adds fail-closed-without-orchestrator coverage for all three entry points (smoke-tested: git-http exits 1, supervise exits 2, egress `__init__` raises). - **pyright**: 0 errors. **pylint**: source 9.90/10, tests 9.47/10. All egress (197), git-http (18), and supervise (97) tests green. Net **−57 lines**. - The 13 `test_gateway_init` failures on the dev host are the pre-existing `/bin/sleep`-on-NixOS issue, unrelated. ## Follow-ups (out of the data-plane teardown, noted in the commit) - `gateway_init` still forwards SIGHUP to the egress child (now dormant — nothing sends it). - The README still describes the docker backend's per-bottle topology. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ
didericis-claude added the Kind/Enhancement label 2026-07-17 18:03:27 -04:00
didericis-claude reviewed 2026-07-17 21:31:41 -04:00
didericis-claude left a comment
Author
Collaborator
Copy Link
Copy Source

Review — remove the single-tenant data-plane paths

Verdict: approve. The teardown is coherent, genuinely fail-closed at every entry point, and the PR body's claims held up under verification. No blocking bugs found. (All 130 tests in the touched suites pass locally against f26579c.)

What I verified

  • All launch paths set the env: orchestrator/gateway.py:226, firecracker/infra_vm.py:434, macos_container/infra.py:103 all inject BOT_BOTTLE_ORCHESTRATOR_URL; the only no-URL DockerGateway() construction (docker/launch.py:148) just reads the CA from an already-running container, it never starts one.
  • No stranded SIGHUP sender: both egress applicators confirm the docker kill --signal HUP path was removed in #385; gateway_init.py's forward is dormant as stated.
  • Log-gate call ordering: every _block/_flow_log call in request() comes after the _stash_flow_ctx at egress_addon.py:347, so the new per-flow log level is always available; the _flow_ctx deny-all fallback (Config(routes=()), log=LOG_OFF) makes hooks on unresolved flows a safe no-op.
  • Supervise: _resolver_or_fail's _RpcInternalError is caught and mapped by _dispatch's existing handler, so a misconfigured server returns a JSON-RPC error rather than crashing the thread.
  • Fake resolver fidelity: _StaticResolver.resolve_policy_and_bottle_id matches the real PolicyResolver signature, and the YAML-subset emitter drives the real parse path as claimed.
  • Only caller of serve() is supervise_server.main() — the bottle_slug → resolver signature change breaks nobody else.

One belt-and-braces suggestion (non-blocking)

The egress fail-closed guarantee rests on mitmdump exiting when addons = [EgressAddon()] raises at script load. mitmproxy 11.1.3's errorcheck addon does exit dump tools on startup errors, so this should hold — but the smoke test covered the raise itself, not mitmdump's reaction to it. If a future mitmproxy bump ever softened that to "log and continue", the failure mode is the worst one: a bare TLS-bumping open proxy with no policy. A two-line guard in egress_entrypoint.sh before the exec mitmdump:

[ -n "$BOT_BOTTLE_ORCHESTRATOR_URL" ] || { echo "egress: BOT_BOTTLE_ORCHESTRATOR_URL required" >&2; exit 1; }

makes the fail-closed property independent of mitmproxy's error handling.

Stale-comment fallout in untouched files (follow-up material)

  • orchestrator/gateway.py:129 — "Empty → single-tenant." Now false: empty means all three daemons refuse to start. Could also just make orchestrator_url a required kwarg now.
  • egress_entrypoint.sh:18-19 — "loads the addon that reads /etc/egress/routes.yaml" — that read no longer exists.
  • Dockerfile.gateway:23 — layout comment still lists /etc/egress/routes.yaml bind-mounted at run time.

These fit naturally with the README follow-up already listed in the PR body.

Nice touches

  • Moving introspection after resolution closes a small info-leak: _egress.local/allowlist now shows only the calling bottle's resolved routes, and an unattributed caller sees an empty list rather than any global table.
  • _req_ctx redacting with the per-flow env overlay fixes a real (if quiet) gap — block logs previously scrubbed only os.environ, not the bottle's provisioned tokens.

🤖 Review by Claude Code (Opus 4.8)

## Review — remove the single-tenant data-plane paths **Verdict: approve.** The teardown is coherent, genuinely fail-closed at every entry point, and the PR body's claims held up under verification. No blocking bugs found. (All 130 tests in the touched suites pass locally against `f26579c`.) ### What I verified - **All launch paths set the env**: `orchestrator/gateway.py:226`, `firecracker/infra_vm.py:434`, `macos_container/infra.py:103` all inject `BOT_BOTTLE_ORCHESTRATOR_URL`; the only no-URL `DockerGateway()` construction (`docker/launch.py:148`) just reads the CA from an already-running container, it never starts one. - **No stranded SIGHUP sender**: both egress applicators confirm the `docker kill --signal HUP` path was removed in #385; `gateway_init.py`'s forward is dormant as stated. - **Log-gate call ordering**: every `_block`/`_flow_log` call in `request()` comes after the `_stash_flow_ctx` at `egress_addon.py:347`, so the new per-flow log level is always available; the `_flow_ctx` deny-all fallback (`Config(routes=())`, log=`LOG_OFF`) makes hooks on unresolved flows a safe no-op. - **Supervise**: `_resolver_or_fail`'s `_RpcInternalError` is caught and mapped by `_dispatch`'s existing handler, so a misconfigured server returns a JSON-RPC error rather than crashing the thread. - **Fake resolver fidelity**: `_StaticResolver.resolve_policy_and_bottle_id` matches the real `PolicyResolver` signature, and the YAML-subset emitter drives the real parse path as claimed. - **Only caller of `serve()`** is `supervise_server.main()` — the `bottle_slug` → `resolver` signature change breaks nobody else. ### One belt-and-braces suggestion (non-blocking) The egress fail-closed guarantee rests on mitmdump **exiting** when `addons = [EgressAddon()]` raises at script load. mitmproxy 11.1.3's `errorcheck` addon does exit dump tools on startup errors, so this should hold — but the smoke test covered the raise itself, not mitmdump's reaction to it. If a future mitmproxy bump ever softened that to "log and continue", the failure mode is the worst one: a bare TLS-bumping **open proxy** with no policy. A two-line guard in `egress_entrypoint.sh` before the `exec mitmdump`: ```sh [ -n "$BOT_BOTTLE_ORCHESTRATOR_URL" ] || { echo "egress: BOT_BOTTLE_ORCHESTRATOR_URL required" >&2; exit 1; } ``` makes the fail-closed property independent of mitmproxy's error handling. ### Stale-comment fallout in untouched files (follow-up material) - `orchestrator/gateway.py:129` — "`Empty → single-tenant.`" Now false: empty means all three daemons refuse to start. Could also just make `orchestrator_url` a required kwarg now. - `egress_entrypoint.sh:18-19` — "loads the addon that reads /etc/egress/routes.yaml" — that read no longer exists. - `Dockerfile.gateway:23` — layout comment still lists `/etc/egress/routes.yaml bind-mounted at run time`. These fit naturally with the README follow-up already listed in the PR body. ### Nice touches - Moving introspection **after** resolution closes a small info-leak: `_egress.local/allowlist` now shows only the calling bottle's resolved routes, and an unattributed caller sees an empty list rather than any global table. - `_req_ctx` redacting with the per-flow env overlay fixes a real (if quiet) gap — block logs previously scrubbed only `os.environ`, not the bottle's provisioned tokens. --- 🤖 Review by Claude Code (Opus 4.8)
didericis-codex removed the Kind/Enhancement label 2026-07-17 21:36:34 -04:00
didericis force-pushed refactor/remove-single-tenant-data-plane from 121c3e82eb to ec3b85ac1b 2026-07-17 22:03:43 -04:00 Compare
didericis approved these changes 2026-07-17 22:06:37 -04:00
didericis added 4 commits 2026-07-17 22:34:30 -04:00
refactor(gateway): remove the single-tenant data-plane paths (audit #400 finding 3) b601b663e2 
All three backends (docker, firecracker, macos-container) now launch through
the consolidated orchestrator, and every production gateway sets
BOT_BOTTLE_ORCHESTRATOR_URL — so the legacy single-tenant (`resolver is None`)
branches in the shared gateway's data plane were unreachable dead code, a second
security-relevant path to keep correct in parallel with the live one. Make the
orchestrator resolver mandatory and delete the single-tenant paths from the
three data-plane modules.

egress_addon.py: drop the static routes file entirely — EGRESS_ROUTES, _reload,
the SIGHUP handler, self.config, and the SUPERVISE_BOTTLE_SLUG env slug. The
per-request /resolve is the only policy source; __init__ fail-closes if
BOT_BOTTLE_ORCHESTRATOR_URL is unset. Introspection (`_egress.local/allowlist`)
now reports the calling bottle's *resolved* routes. The block/redact log gates
and _req_ctx redaction now read the per-flow config/env from the request-time
stash, so they use each bottle's log level and token set (they silently used the
empty static config before). Nothing sends `docker kill --signal HUP` to the
gateway in the consolidated model (the egress applicators fail closed), so
removing the SIGHUP reload is safe.

git_http_backend.py: resolver mandatory; no flat-root fallback. main() refuses
to start without an orchestrator URL; a request whose source resolves to no
bottle 404s.

supervise_server.py: resolver mandatory; every proposal is attributed to the
source-IP-resolved bottle. Remove handle_list_egress_routes (the proxy-fetch
introspection that only worked when the proxy carried one bottle's identity) —
list-egress-routes is answered from the resolved policy. main() refuses to start
without an orchestrator URL.

Tests: a host-side fake resolver serves each test's Config through the real
parse path (a small YAML-subset emitter round-trips route_to_yaml_dict); the
response/websocket hooks stash it as request() would. Deletes the tests for the
removed static-config, SIGHUP-reload, and single-tenant-passthrough paths; adds
fail-closed-without-orchestrator coverage.

Follow-up: gateway_init still forwards SIGHUP to the egress child (now dormant —
no one sends it); the README still describes the docker backend's per-bottle
topology. Both are outside the data-plane teardown.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ
refactor(gateway): fail closed without an orchestrator URL; drop stale single-tenant comments ea1fbeeaa0 
Follow-ups from the #402 review of the single-tenant data-plane teardown.

- egress_entrypoint.sh: refuse to launch mitmdump when
  BOT_BOTTLE_ORCHESTRATOR_URL is unset, so the fail-closed guarantee no
  longer rests solely on mitmproxy's errorcheck addon exiting on the
  addon's load-time raise. A misconfigured gateway can never come up as
  a bare TLS-bumping open proxy with no policy.
- orchestrator/gateway.py: ensure_running() raises GatewayError on an
  empty orchestrator URL — a URL-less launch would only crash-loop the
  now-resolver-only daemons (egress raises, git-http exits 1, supervise
  exits 2). The env-injection branch is now unconditional.
- Drop stale "single-tenant" / "reads routes.yaml" comments in
  gateway.py and egress_entrypoint.sh, and the /etc/egress/routes.yaml
  layout line in Dockerfile.gateway.
- Tests: gateway fixtures supply an orchestrator URL; add a
  refuse-without-URL test and assert the URL env is injected.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ
test(egress): set orchestrator URL for entrypoint tests; cover fail-closed guard d0b7de119f 
The egress_entrypoint.sh fail-closed guard (this branch) exits 1 when
BOT_BOTTLE_ORCHESTRATOR_URL is unset, which broke the argv-construction
tests that ran the script without it. Set the URL in the shared
_run_entrypoint helper (a precondition for reaching mitmdump now, like
PATH) and add a test asserting the guard fails closed when it's absent.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ
test(git-http): wire the resolver into the access-hook-503 regression test
test / unit (pull_request) Successful in 1m9s
Details
test / integration (pull_request) Successful in 23s
Details
test / coverage (pull_request) Successful in 1m21s
Details
lint / lint (push) Successful in 2m16s
Details
test / unit (push) Successful in 1m9s
Details
test / integration (push) Successful in 31s
Details
test / coverage (push) Successful in 1m33s
Details
Update Quality Badges / update-badges (push) Successful in 1m21s
Details
d8e3947bd3 
b1850be's fail-closed-503 test (rebased in from main) built a git-http
server with a flat repo.git and no policy_resolver. The resolver-only
data plane on this branch denies an unattributed request with 404 before
it reaches the access-hook path the test exercises, so it saw 404 != 503.

Nest the bare repo under <root>/<_BID>/ and set _FixedResolver(_BID) on
the server, matching every other test in this module, so the request is
attributed and reaches the access-hook (mocked to raise PermissionError)
that the 503 fail-closed behavior guards.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ
didericis force-pushed refactor/remove-single-tenant-data-plane from 99cc146cb8 to d8e3947bd3 2026-07-17 22:34:30 -04:00 Compare
didericis merged commit d8e3947bd3 into main 2026-07-17 22:56:48 -04:00
didericis deleted branch refactor/remove-single-tenant-data-plane 2026-07-17 22:56:49 -04:00
Sign in to join this conversation.
Reviewers
No Reviewers
didericis-claude
didericis
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
Status/Needs Triage
Awaiting initial classification
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
didericis-claude (didericis (claude)) didericis-codex (didericis (codex)) didericis-gemma (gemma) didericis
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: didericis/bot-bottle#402
Delete Branch "refactor/remove-single-tenant-data-plane"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?