Hold NetworkingConfig to the network rule NetworkMode already had - #40
Open
bfulton wants to merge 3 commits into
Open
Hold NetworkingConfig to the network rule NetworkMode already had#40bfulton wants to merge 3 commits into
bfulton wants to merge 3 commits into
Conversation
Found while measuring a consumer report about binding a sealed network's
gateway. A container joins a network two ways at create: HostConfig
.NetworkMode, which was checked against the policy and the job's own
networks, and NetworkingConfig.EndpointsConfig, which was not checked at
all. The daemon attaches every network named in either.
So the ownership rule could simply be walked around by putting the name
in the other field:
HostConfig.NetworkMode: "someone-elses-net" -> denied
NetworkingConfig.EndpointsConfig: {"someone-elses-net"} -> ALLOWED
which reaches another job's internal network, or any network the operator
created, and whatever those reach. EndpointsConfig now goes through the
same check, case-insensitively, and `host` and `container:` are refused
there as they are for NetworkMode.
A job may still name a network it created, which is what a container
bridging two of its own networks needs - the arrangement the consumer is
trying to build.
The first cut of this refused every ordinary `docker run`: the real CLI
sends EndpointsConfig {"default": {}}, and NetworkMode normalises
"default" to bridge while the new code judged the name literally. Caught
by the e2e against a real daemon, not by the unit tests, which is the
fourth time a body assembled from the spec has disagreed with the wire.
The captured value is now a test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh
The fix permits a container joining two networks the job created, which
is the shape a sealed run actually needs on macOS: a host-side broker
cannot bind an internal network's gateway there at all - the address
lives inside Docker Desktop's VM, and binding it fails with errno 49
outside any sandbox - so the broker has to be a container with a foot in
both networks.
Measured rather than assumed, and two things came out of it. Docker
refuses to mix the default bridge with a user-defined network ("cannot
attach both user-defined and non-user-defined network-modes"), so the
routable side has to be a declared network too, not `bridge`. And the CLI
puts the first name in HostConfig.NetworkMode and the rest in
NetworkingConfig.EndpointsConfig - so this arrangement is exactly the
path that went unchecked until the previous commit.
Both networks are internal in the test because that is what this
repository's policy declares; what is being pinned is that two owned
names pass through two different fields.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh
There was a problem hiding this comment.
🟢 Approval recommended
The change closes a concrete security gap with consistent logic and includes targeted tests that cover both the bypass and real daemon/CLI behavior.
Pull request overview
This PR closes a policy bypass in Docker container creation by enforcing the repository’s run.network / “job-owned network” rule not only for HostConfig.NetworkMode, but also for NetworkingConfig.EndpointsConfig (which the daemon also uses to attach networks). It also adds unit coverage for real-wire behavior where the Docker CLI sends EndpointsConfig: {"default": {}}, ensuring ordinary docker run requests remain allowed.
Changes:
- Enforce the same network allowlist/ownership checks on
NetworkingConfig.EndpointsConfigas already applied toHostConfig.NetworkMode. - Normalize
EndpointsConfigkeys""and"default"to"bridge"to match daemon/CLI behavior and avoid rejecting ordinary runs. - Add focused unit tests covering denial of undeclared/unowned networks, allowance of job-owned networks, and case-insensitive JSON key handling for
NetworkingConfig/EndpointsConfig.
File summaries
| File | Description |
|---|---|
| src/main/docker/docker-evaluator.ts | Adds policy enforcement for networks attached via NetworkingConfig.EndpointsConfig, including normalization and host/container-mode refusal. |
| src/main/docker/docker-evaluator.test.ts | Adds unit tests proving the bypass is closed while preserving real Docker CLI behavior for "default"/"" endpoints config. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The change closes a concrete policy bypass with consistent enforcement and adds both unit and e2e regression coverage for the fixed behavior and real CLI payloads.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Both cost a consumer real time this week, and neither is fixed by changing what the sandbox grants - they are cases where the error a tool prints is not the error that happened. git shims through xcrun, which loads libxcrun from the active developer directory; where that is Xcode and the policy does not grant it, git dies naming a missing dylib rather than a permission. Pointing DEVELOPER_DIR at the Command Line Tools costs no policy change, since /Library/Developer is already on the read floor - but it belongs in the repository's workflow rather than in the runner. Which toolchain a job wants is the repository's choice, and a job that genuinely needs Xcode should declare it and keep it. A refused bind returns EPERM whatever else was wrong with the address, so "Operation not permitted" on an address that does not exist reads as a policy problem. On macOS it usually is not: Docker Desktop runs the daemon in a VM, a bridge gateway is an interface inside it, and binding one from the host fails with EADDRNOTAVAIL outside any sandbox. Measured, not inferred - 172.18.0.1 gives errno 49 unsandboxed on this machine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh
There was a problem hiding this comment.
🟢 Approval recommended
The security gap is addressed with a targeted, consistent policy check and is covered by both unit and live-daemon e2e regression tests.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
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
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.
The bypass
A container joins a network two ways at create.
HostConfig.NetworkModewas checked against the policy's declared network and the job's own networks.NetworkingConfig.EndpointsConfigwas not checked at all — and the daemon attaches every network named in either.That reaches another job's internal network, or any network the operator created, and whatever those reach.
EndpointsConfignow goes through the same check, case-insensitively, withhostandcontainer:refused there as they are forNetworkMode.A job may still name a network it created — that is deliberate, and it is what a container bridging two of its own networks needs.
How it was found
Measuring a consumer report that the seatbelt profile forbids binding the gateway of a sealed (
--internal) network. That report turns out to have a different root cause (see below), but reading the create path for it surfaced this.The wire disagreed with the spec, again
The first cut refused every ordinary
docker run. The real CLI sendsEndpointsConfig: {"default": {}}, andNetworkModenormalises"default"to bridge while the new code judged the name literally. The e2e against a real daemon caught it; the unit tests were green.That is the fourth time a body assembled from the spec has disagreed with the wire in this area. The captured value is now a test.
Two denials that name the wrong cause, written down
Neither is fixed by changing what the sandbox grants, and both cost a consumer real time, so
docs/roadmap/localmostrc.mdnow records them.xcrun, which loadslibxcrunfrom the active developer directory.DEVELOPER_DIR=/Library/Developer/CommandLineToolscosts no policy change (/Library/Developeris already on the read floor) — but it belongs in the repository's workflow, not the runner. Which toolchain a job wants is the repo's choice, and a job that genuinely needs Xcode should declare/Applications/Xcode.appand keep it.EPERMmasksEADDRNOTAVAIL. On macOS a docker bridge gateway is an interface inside Docker Desktop's VM:172.18.0.1fails with errno 49 unsandboxed on this machine. A host process cannot join a container network there; the dual-homed container above is the portable shape.Gate
1098 unit tests, tsc and eslint clean, docker e2e 7/7 against a live daemon.