fix(filter): guard container process-list reads - #395
Merged
Conversation
Use one non-HTML-escaping encoder configuration for rewritten object and array responses so identical values have identical raw-byte spellings while keeping compact bodies and existing response metadata handling.
…-fail-closed # Conflicts: # CHANGELOG.md
…/p8-container-top-guard # Conflicts: # CHANGELOG.md # README.md # app/internal/proxy/timeout.go # docs/content/docs/configuration.mdx # docs/content/docs/migration.mdx
…/p8-container-top-guard # Conflicts: # CHANGELOG.md
…/p8-container-top-guard # Conflicts: # CHANGELOG.md # app/internal/config/config.go # app/internal/config/validate.go # docs/content/docs/configuration.mdx
…an-event-filter-consolidation
…an-event-filter-consolidation
# Conflicts: # CHANGELOG.md
|
Deployment failed for project sockguard-website with the following error: Learn More: https://vercel.com/codeswhat?upgradeToPro=build-rate-limit |
|
Warning Review limit reachedNext included review available in 31 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (62)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Author
|
@greptileai Review exact head |
…-fail-closed # Conflicts: # CHANGELOG.md # app/internal/ownership/libpod_coverage_test.go # app/internal/ownership/libpod_paths.go # app/internal/visibility/libpod_events.go # docs/content/docs/podman.mdx
…json-encoding # Conflicts: # app/internal/responsefilter/filter_test.go
…-container-top-guard # Conflicts: # CHANGELOG.md # README.md # app/internal/cmd/rules.go # app/internal/cmd/serve.go # app/internal/cmd/serve_warn_test.go # app/internal/config/compose_example_sync_test.go # app/internal/filter/drydock_preset_conformance_test.go # app/internal/filter/podman_readonly_preset_conformance_test.go # docs/content/docs/configuration.mdx # docs/content/docs/podman.mdx # docs/content/docs/security.mdx # examples/compose/traefik/docker-compose.yml # scripts/release-metadata.test.mjs
Startup validation now reads the authored rule literals, not only a representative probe path, so the exact-name and shadowed-allow cases are refused before the policy compiles and never reach the request-time gate.
The request-time gate split the path after the first identifier segment, so
GET /containers/a/b/top missed it. Docker registers the compat route as
/containers/{name:.*}/top, so a rule like GET /containers/*/*/top admits a
slash-bearing name the gate walked straight past. Match everything before a
trailing /top with a non-empty name instead.
Startup validation has to speak the same language, so the compat top catalog
entry now carries catalogIdentifierPath. Podman's libpod routes use {name},
which is one segment, so both libpod entries keep the default shape.
The test passed config.Defaults() as the config and a disjoint compiled rule set, so exposed_endpoints=[] proved nothing: the audit walks cfg.Rules, and cfg.Rules was empty. Feed the same exact-name rule to both sides and assert the warning names GET /containers/payments/top, which is what the literal audit actually finds.
All five Portwing presets, the Portwing compose example, and both tri-tool examples already set insecure_allow_read_exfiltration: true for the logs rule, so their GET /containers/*/top rules validate unchanged. Removing them cost Portwing its process view and bought nothing. The drydock, CIS, multi-listener and multi-host removals stand: those configs carry no acknowledgment, so the rule would now refuse startup. The conformance tests that pinned Portwing top as denied move back to allowed, and the shipped-config table is renamed for what it actually pins: which configs admit top and which carry the acknowledgment they need to.
The docs credited the request-time gate with refusing exact-name and ordered process-list allows. Startup validation does that: its audit searches the catalog's whole route language against the authored rule literals, so both shapes are refused at load time and the warning names the concrete reachable path once the acknowledgment is set. The claim that an empty endpoint list could still hide such a rule was false for the same reason. The gate's real job is narrower, and now says so: it stops a warn or audit profile from passing a process-list denial through, and holds as defense in depth. README's rollout-modes row carries that carve-out. The changelog bullet gets the BREAKING marker it was missing, naming all three consequences, and drops the "unused container-top rules" reason: the drydock conformance test pinned top, so those presets lost it because they carry no acknowledgment, not because nothing called it. podman.mdx and presets.mdx now say that podman-readonly.yaml's acknowledgment is global, so a later addition to that preset gets no fresh startup refusal.
Owner isolation refused a target the daemon could not resolve with the same 403 it uses for a resource that resolves to another owner, so a client deleting something already gone read as a permissions failure and the layer disagreed with visibility, which answers 404 for a resource its policy hides. Split the denial verdict in two. verdictDenyMissing carries the unresolved case and answers 404; verdictDeny keeps 403 for a resolved resource whose labels fail the policy, for a remote image SCP source, and for a malformed local one. Both are equally fail-closed: nothing reaches the requested upstream path in enforce mode, warn and audit still log the would-be denial and forward, and an inspect error is still a 502. Denial checks now ask verdict.denied() rather than comparing against one constant, so a missing target cannot leak through a `!= verdictDeny` test. The rule covers every unresolved reference, not just the one the URL names: exec sessions, container:<ref> namespace-sharing targets, and the images, volumes, networks, secrets and configs embedded in create and update bodies. The reason vocabulary now tracks the status, "could not resolve X" for a lookup that came back empty and "denied access to X" for one that came back with the wrong owner, so the SCP malformed-source reason moved to the second wording since nothing was ever looked up for it.
The four Docker-compat prune routes have carried an injected owner label
filter since owner isolation shipped. Their libpod spellings carried nothing:
libpodNeedsOwnerFilter listed only GET reads and both call sites gated it on
GET/HEAD, so POST /libpod/{containers,images,networks,volumes}/prune went
upstream verbatim and deleted every prunable resource on the host whoever
owned it.
All four document a JSON-encoded `filters` parameter taking `label` and read
it through util.PrepareFilters at Podman v5.8.1, and containers/prune is
routed to the same compat.PruneContainers handler as the compat path, so they
take the same injection the compat routes take. libpodNeedsOwnerFilter now
takes the method, like its Docker-compat counterpart, instead of leaving the
method test to each caller.
POST /libpod/pods/prune has no filters to inject: its swagger block documents
no parameters and PodPruneHelper calls runtime.PrunePods with no options at
all. It is refused instead, through a write-side twin of the unscopeable-read
catalog, and the refusal is unconditional — warn mode buys an operator a
measurement of what enforcement would cost, and there is none to take once
another tenant's pods are gone. DELETE /libpod/pods/{name} still names a pod
the ownership layer checks.
POST /commit was the one write in the API that ownership never classified. Every other check starts from a path identifier, and commit names its container in the `container` query parameter instead, so allowPathOwnershipRequest walked straight past it. A client holding the `POST /commit` rule that configs/portainer.yaml ships (and that COMMIT=1 generates through the Tecnativa compat layer) could commit another owner's container, and the image it produced carried no owner label at all: invisible to the owner-filtered image list and outside every later ownership check. Commit is now classified off the query on all three registered spellings (moby's POST /commit, Podman's compat POST /commit, and its native POST /libpod/commit). The named container is authorized as an embedded reference, so it gets the same verdicts as the rest of the branch: 403 for a container another owner holds, 404 for one that does not resolve, 502 for a lookup failure. The owner label is stamped into the commit body's config, and a body is synthesized with a JSON content type when the client sends none, because both engines accept an empty commit body. Three request shapes are refused outright rather than authorized: - No `container` parameter. There is nothing to check, and owner isolation does not forward a write it could not classify. - A repeated or two-case-variant `container`. Moby reads the first value and Podman reads the last, so the container checked would not be the container committed. filter.FoldedScalarQueryValue, exported here for the purpose, is the same helper the container-archive policy uses for that disagreement. - A `changes` value carrying a LABEL instruction. Both engines apply changes on top of the body config (moby through dockerfile.BuildFromConfig, Podman through buildah's parseOverrideChanges), so a LABEL there overwrites the stamp. The refusal is on the instruction rather than the key it sets: deciding which key a LABEL line touches means parsing Dockerfile quoting, multi-pair lines and continuations, and getting that wrong is a silent bypass instead of a broken request. Image labels can still be set in the body config.
setPodmanEventLabelFilter serves both GET /events and GET /libpod/events, and derived its filter key from compatEventsPath, so half its callers got a key named after a path they never used. The result was right by coincidence: the only path visibilityLabelFilterKey answers differently for is the Swarm node list, which no event stream reaches. The key is not path-derived at all. It is the name of the filter Podman's event handler accepts, so it is now a named constant with the reasoning on it, and the test asserts the two things the coincidence was standing in for: the injected and sole-value-marked key is that one on both spellings, and it still matches what owner isolation derives for those paths. If those diverge the ownership layer stops seeing the marker and appends an owner value beside the visibility selector, which Podman ORs, widening the stream this path narrows. No behavior change, so no test fails before it.
…odes podman.mdx still said a local SCP source that inspects as not found "passes through so Podman can return its real error". It has not passed through since the fail-closed change, and it now answers a synthetic 404 without contacting Podman. The same page's Known Limitations entry for showmounted still said "Neither is wired today" about a refusal both isolation layers have carried since the mounted-inventory fix, so it is rewritten as the residual case it actually is: a deployment running neither layer. security.mdx's Layer 6 gets the status contract in one place. A target the daemon could not resolve is a 404, a target that resolved under another owner's label is a 403, a lookup that errored is a 502, and none of the three reaches the upstream path. The 404 half is the load-bearing part: it matches what a visibility policy already answers for a hidden resource, so the pair of layers cannot be told apart by status code, and it is what an idempotent teardown expects when it deletes something already gone. Both pages and the changelog also pick up the two surfaces this branch newly classifies: the libpod prune family, owner-filtered like its Docker-compat twin with pods/prune refused because it takes no filters at all, and container commit, owner-checked off its query parameter with the owner label stamped into the image it produces. The changelog marks both breaking, since a host-wide libpod prune and a commit carrying a LABEL change instruction both worked before and now do not under owner isolation. Two smaller corrections ride along, both about claims the commit work falsified. The Known Limitations entry for the batch image endpoints said owner isolation cannot check them because they name their images in the query; the query is not the obstacle, the set is. And commit.go's own comment said Tecnativa's COMMIT=1 generates the rule that opens the endpoint, when COMMIT=1 alone generates GET,HEAD and it takes POST=1 to widen the section. The "exhaustive" claim about the collection-route helper tests is now "modeled", which is what those tests actually pin.
…-fail-closed # Conflicts: # CHANGELOG.md # app/internal/ownership/libpod_coverage_test.go # app/internal/ownership/libpod_paths.go # docs/content/docs/podman.mdx
…iner-top-guard # Conflicts: # CHANGELOG.md # README.md # docs/content/docs/configuration.mdx # docs/content/docs/podman.mdx # docs/content/docs/security.mdx
scttbnsn
marked this pull request as ready for review
September 3, 2026 21:08
scttbnsn
requested review from
ALARGECOMPANY and
biggest-littlest
as code owners
September 3, 2026 21:08
biggest-littlest
approved these changes
Sep 3, 2026
biggest-littlest
left a comment
biggest-littlest
left a comment
Member
There was a problem hiding this comment.
Rebuilt on dev/v2.0 as a stacked chain, CI green, adversarial review findings folded in on the branch.
ALARGECOMPANY
approved these changes
Sep 3, 2026
ALARGECOMPANY
left a comment
ALARGECOMPANY
left a comment
Member
There was a problem hiding this comment.
Rebuilt on dev/v2.0 as a stacked chain, CI green, adversarial review findings folded in on the branch.
This was referenced Sep 4, 2026
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
3 participants
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.
Container and pod process-list reads now sit behind
insecure_allow_read_exfiltration. Three routes join the exfiltration catalog: Docker-compatibleGET /containers/{id}/top, nativeGET /libpod/containers/{id}/top, and nativeGET /libpod/pods/{id}/top. The compat route runs the daemon host'spswith caller-selectedps_argsand returns process command lines with no response redaction; both libpod routes expose the same class and support a streaming mode. Version-prefixed requests normalize to the same three routes.What refuses what
Startup validation does the heavy lifting. Its audit searches the catalog's whole route language against the authored rule literals, so a wildcard rule, a broad
/containers/**, an exact container name, and an ordered deny that shadows the catalog's representative path are all refused at load time without the acknowledgment. The compat catalog entry uses the slash-bearing identifier shape, because Docker registers the route as/containers/{name:.*}/topand a rule likeGET /containers/*/*/topis genuinely reachable. The two libpod entries stay single-segment, since Podman's{name}is one segment.The request-time gate is narrower than the earlier draft of this branch claimed. It matches all three routes by their
/topsuffix and it does two things validation can't: it stops awarnorauditprofile from passing a process-list denial through aswould_deny, and it holds the line as defense in depth if a rule shape ever escapes the load-time audit.sockguard matchcalls the same gate, so an offline check reports the effective decision rather than the underlying allow rule.Presets
The drydock presets and example, the CIS preset and example, the multi-listener preset, and the multi-host example drop their
GET /containers/*/toprules. None of those configs carries the acknowledgment, so keeping the rule would refuse startup. That's the reason, not that nothing calls it: drydock's integration conformance test pinned top as allowed.Portwing's five presets and its three synchronized examples keep theirs. They already set
insecure_allow_read_exfiltration: truefor the logs rule, so the top rule validates unchanged and Portwing keeps its process view.podman-readonly.yamlkeeps container top on both API surfaces and now carries the acknowledgment with the tradeoff written down. Native pod top stays denied by its narrow pod rules. Worth knowing before you extend that preset: the flag is global rather than per-rule, so anything you add later that the exfiltration catalog covers, container logs being the obvious one, is admitted with no fresh startup refusal to warn you. The Podman and presets docs both say so now.Breaking
insecure_allow_read_exfiltration: trueexits 1 at startup instead of serving it.GET /containers/{id}/top.warnorauditprofile no longer passes a process-list denial through; it refuses the request.Verification
go test -count=1 ./internal/filter ./internal/cmd ./internal/config,go test -count=1 -tags integration ./integration/,go vet ./...,golangci-lint runnode --test scripts/*.test.mjs, which pins the corrected docs prose in both the configuration reference and the changelog