Skip to content

fix(security): scope Compose volumes per project (ExploitGym-inspired) - #4

Open
jeremy1392 wants to merge 1 commit into
SalesforceAIResearch:mainfrom
jeremy1392:fix/isolate-compose-volumes
Open

jeremy1392 wants to merge 1 commit into
SalesforceAIResearch:mainfrom
jeremy1392:fix/isolate-compose-volumes

Conversation

@jeremy1392

@jeremy1392 jeremy1392 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Problem and behavior

Independent Compose projects can attach the same backing store even when their project names differ. The current control-plane gate checks service host binds but does not inspect top-level named-volume definitions or volumes_from. This change rejects those storage declarations before image resolution, placement, rollout recording, or a node command.

For example, volumes: {cache: {name: shared-cache}} now raises KwargsPolicyViolation naming volumes.cache.name. With volumes: {cache: {}}, Compose continues allocating a project-local cache and services within that project can share it normally.

Changes

  • Reject explicit/global and external volume names, non-local volume drivers, nonempty driver options, and volume inheritance from external containers or undeclared services. Read-only consumers are checked too because another execution could write to their backing store.
  • Require literal mount sources/types and resolved includes/inheritance, so Docker cannot introduce storage that the control plane did not inspect.
  • Preserve project-local named volumes, same-project volumes_from, tmpfs, and anonymous volumes. Fix target-only anonymous mounts being mistaken for host binds.
  • Aggregate actionable field-level errors with existing policy violations, and document migration and scope.

ExploitGym connection and author's research

This PR builds on Jeremy Canale's independent case study, "Hardening the OpenAI ExploitGym environment". The study reconstructs the OpenAI / Hugging Face incident from public reports and proposes a containment architecture covering shared dependency infrastructure, per-run identities, restricted egress, and independent shutdown controls.

The principle carried into this contribution is that isolating an agent's container is insufficient if shared writable infrastructure remains available as a communication channel. This PR applies that principle to Beagle's Compose storage: keep writable volumes local to each independent project while preserving intentional sharing between services within that project. The regression tests and real Docker smoke test validate this specific implementation; the broader architecture in the case study remains a proposal requiring its own validation.

The underlying incident evidence is documented in OpenAI's account of unauthorized communication through shared infrastructure. Docker documents that explicit volume names bypass project scoping and volume drivers can attach shared backing stores.

Validation

  • 291 passed, 7 skipped under Linux/Python 3.12: Compose policy/preparation, coordinator acquire/destroy, endpoint/transport, GC reconciliation, node runner/manager, Harbor Compose helper, and kwarg policy tests. The seven skips require the external benchmark cache.
  • 1 real Linux Docker smoke passed: an intra-project sidecar reads its own cache; a second project cannot read that marker; each maintains distinct contents; tearing down the first preserves the second's data. Test resources are cleaned up.
  • Targeted Ruff, strict mypy on the changed module, and git diff --check passed.
  • Beagle's full test collection remains blocked locally by the existing missing scripts/gateway/gateway_proxy.py and Windows' lack of fcntl. The affected XRLEnv tests were run separately on Linux, including both existing POSIX process-group tests.

Scope and compatibility

Existing deployments using shared/external/custom Compose storage must migrate to project-local volumes initialized from immutable inputs. These checks have no privileged/host-path-policy override.

Leave project_name unset for independent executions; the coordinator already generates a fresh name for each acquire. Explicit project names retain existing semantics and callers must keep them unique. This does not isolate network services, operator-allowed ordinary host binds, other execution backends, or host-privileged workloads, and does not change XRLEnv's trusted-workload model.

This PR branches directly from upstream main and is independent of #3 (confirmed local-container cleanup).

@Yutong-Dai

Copy link
Copy Markdown
Contributor

Thanks for this — the storage-scoping argument holds up and most of it is ready to land. One check needs rework first: it rejects Compose shapes the benchmark corpus actually ships, in a way no operator setting can lift.

To make that concrete and reproducible, I put your commit on top of current main as the branch review/pr-4 (cherry-picked, you're still the author), plus one commit adding a corpus shape the test was missing. Work there or update this PR — whichever you prefer.

Reproducing

main now carries vendor/xrlenv/tests/unit/control/test_compose_corpus_compatibility.py, which pins compose policy against the shapes the corpus contains. Same command on both branches:

uv sync --extra all
PYTHONPATH=vendor/xrlenv .venv/bin/python -m pytest \
    vendor/xrlenv/tests/unit/control/test_compose_corpus_compatibility.py -q
main          7 passed, 1 skipped
review/pr-4   2 failed, 6 passed, 1 skipped
FAILED ...[short-form interpolated source]
FAILED ...[short-form interpolated source with a default]

AssertionError: corpus shape 'short-form interpolated source' is rejected with no
operator override available, so every task using it is offline on every cluster:
    (tier 3) services.main.volumes[0]: Mount sources and types must be resolved
             before policy vetting.

No benchmark cache, no Docker, no network needed. (With XRLENV_BENCHMARK_CACHE set, one further test stops skipping and checks those fixtures still mirror the corpus.)

What breaks and why it is unconditional

_storage_rejections rejects any mount whose source or type contains $. Nine corpus tasks declare exactly that, in three distinct shapes — and in every one the value is supplied by the harness or by a literal default, never chosen by the task author:

# seta: {890,892,973,1133,1198} — 2 mounts each
# example: https://github.com/camel-ai/seta-env/blob/683748c43e0b8ddd68ee5ceb75d28af3bea85d71/Harbor-Dataset/1198/environment/docker-compose.yaml#L12
- ${HOST_VERIFIER_LOGS_PATH}:${ENV_VERIFIER_LOGS_PATH}
- ${HOST_AGENT_LOGS_PATH}:${ENV_AGENT_LOGS_PATH}

# terminalworld: {tw_15324,tw_313581,tw_529592}
# example: https://huggingface.co/datasets/EuniAI/TerminalWorld/blob/main/artifacts/tw_15324.tar.gz
- ${DOCKER_SOCKET_PATH:-/var/run/docker.sock}:/var/run/docker.sock

# terminalworld: tw_291556
- ${WORKSPACE_DIR}:/workspace

That second shape is worth dwelling on, because it cuts both ways. Its effective value is known statically — it resolves to /var/run/docker.sock unless an operator overrides it — so rejecting it as "unresolved" is the least defensible of the nine.

But it also shows your concern is not hypothetical. Today that mount is not vetted at all. _host_binds tests whether the source starts with /, . or ~; this one starts with $, so it is filed as a named volume and allowed_host_paths never sees the Docker socket it actually mounts. So there really is a gap here, and it is a host-socket-shaped one.

The reason this is worth blocking on, rather than a tuning question, is the tier. KwargsPolicy sorts rejections by whether an operator can change the outcome:

tier operator can change it?
1 allowed, operator may restrict yes, via denied_caps etc.
2 rejected, operator may opt in yes, via allow_privileged etc.
3 never allowed no

This check emits tier 3, so there is no nodes.yaml and no pool routing under which those nine tasks still run. Compare network_mode: host, which is tier 2: several terminalworld tasks trip it and run green anyway, because they are marked in SYSBOX_TASKS and routed to a sysbox pool whose policy permits it.

What I'd suggest instead

The concern is real and I don't want it dropped — vetting a source of ${FOO} proves nothing about what the node expands FOO to, and the docker.sock case above shows that is not theoretical.

But blanket rejection is a strictly worse trade than it looks: it takes nine tasks offline and still doesn't vet the mount, it only refuses it. Resolve interpolation control-plane-side using the same environment the node will use, then vet the resolved value. That is better than both the status quo and this PR:

${DOCKER_SOCKET_PATH:-/var/run/docker.sock} the other 8 tasks
today silently unvetted (filed as a named volume) run
this PR rejected, unconditionally offline
resolve-then-vet resolved to /var/run/docker.sock, then checked against allowed_host_paths run

That closes the hole you found, and closes it more thoroughly, without the collateral.

The rest of the change is good

Verified against all 59 cached documents: volumes.*.name, volumes.*.external, driver, driver_opts, volumes_from, extends and include produce zero hits, so those rejections are safe to land as they stand. Your own 75 tests pass on review/pr-4, so the change is internally consistent — the gap is only corpus compatibility.

Also worth calling out, because I under-credited it on first read: the _host_binds target-only fix (":" not in entry) repairs a real misclassification, not just a theoretical one. tw_223822 mounts /opt/splunk/var/lib/splunk with no colon — an anonymous volume at that container path, which today is wrongly treated as a host bind. That is a genuine bug fix independent of the rest of the PR.

One documentation note: this PR removes the docstring line asserting "The corpus's 7 multi-service tasks pass under the operator's existing allow_privileged opt-in and default cap allowlist, and none mount a host path." That claim is exactly what the finding above invalidates — worth re-establishing once the interpolation handling changes, rather than dropping.

One process note

This PR touches only vendor/xrlenv/, which in this repository is a vendored copy of an upstream project rather than code this repo owns — the mirror is regenerated from that upstream on each sync, so a change merged only here would be overwritten. It needs to land in xrlenv and be re-vendored, which is a different route from #3 (this repo's own code). Nothing for you to do differently; flagging it so the merge path isn't a surprise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants