Restore default container PID namespace isolation (remove host PID mode)#6203
Restore default container PID namespace isolation (remove host PID mode)#6203
Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR removes the setPidMode("host") fallback so containers always get an isolated PID namespace, which is good for security. However, it leaves the containersPidNamespace compat flag as dead code and the test config still references it.
Issues (ranked by severity)
-
[Medium] Compat flag
containersPidNamespaceis now dead code. The flag atcompatibility-date.capnp:1383is still defined (with enable/disable strings and$experimentalannotation), and the test atcontainer-client.wd-test:11still listscontainers_pid_namespacein itscompatibilityFlags. But after this PR, no runtime code callsgetContainersPidNamespace()— the flag does nothing. This should be cleaned up: either remove the flag definition and test reference, or keep the flag wired up with the inverted logic (i.e., flag enables host PID mode as a legacy escape hatch). Leaving a dead compat flag in the schema is confusing for future readers and wastes an ordinal. -
[Low] Comment in the test is now misleading.
test.js:676says "Whencontainers_pid_namespaceis ENABLED, the container has its own isolated PID namespace" — implying that when disabled, it doesn't. After this PR, containers always get an isolated PID namespace regardless of the flag, so the comment is inaccurate.
| hostConfig.setPidMode("host"); | ||
| } | ||
| // Do not set pidMode. Docker defaults to an isolated PID namespace, which prevents | ||
| // containers from enumerating or signaling host processes. |
There was a problem hiding this comment.
The containersPidNamespace compat flag (compatibility-date.capnp:1383) is now dead code — nothing calls getContainersPidNamespace() after this change. The flag definition, its test usage in container-client.wd-test:11, and the comment in test.js:676 should all be cleaned up in this PR to avoid leaving an orphaned compat flag in the schema.
If the intent is to provide a backwards-compat escape hatch for users who somehow depend on host PID mode, consider keeping the flag but inverting the logic (e.g., use the disable flag no_containers_pid_namespace to opt into the old host-PID behavior).
|
I've posted my review on PR #6203. Here's a summary of what I found: The PR correctly removes However, I flagged two issues:
|
|
@gpanders is it necessary to have the host namespace in local dev anyway at this point? |
|
We did this intentionally to match production (warts and all) until we flip the compatibility flag on by default with a compatibility date. If we accept this change then local dev and production will have different behavior. This is probably most relevant for any applications that are expecting their ENTRYPOINT to not have PID 1. |
3 participants
Motivation
HostConfig.PidModeto"host"when thecontainers_pid_namespacecompatibility flag was not enabled, causing containers to share the host PID namespace by default and weakening container/host isolation.Description
PidMode = "host"assignment inContainerClient::createContainer()so Docker's default (an isolated PID namespace) is used instead; the change modifiessrc/workerd/server/container-client.c++and leaves other hostConfig settings (network, restart policy, extra hosts) unchanged.Testing
setPidMode("host")via a repository search (rg) and inspected the modified code region to confirm thePidModeassignment was removed; the search returned no matches.bazel test //src/workerd/server/tests/container-client:container-client-test@) but the test run could not complete in this environment because downloading the Bazel binary failed with an HTTP 403, so a full test run could not be executed here.Codex Task