fix(libpod)!: close native visibility gaps - #386
Draft
scttbnsn wants to merge 24 commits into
Draft
Conversation
scttbnsn
commented
Aug 29, 2026
scttbnsn
commented
Contributor
- fix(libpod): scope events, images and per-resource reads to the caller
- fix(libpod)!: refuse the two unscopeable stats collection reads
The ownership and visibility middlewares each keep their own hand-maintained
libpod path list, and both had drifted from the routes Podman actually serves
at v5.8.1. GET /libpod/events, GET /libpod/images/json and every per-image
libpod path were in neither list, so a client that switched from the
Docker-compat spelling to Podman's native one was handed the whole host's
event stream, the whole host's image list, and unchecked inspect, history,
tree, changes, export, push, tag, untag and delete on another owner's images.
podman-readonly.yaml allows the two list paths as shipped. The read-side
identifiers matched only .../json where Podman also serves /exists, /top,
/healthcheck, /archive, /tree, /changes, volume /export, and the bare
GET /libpod/networks/{id} spelling it registers on the same
libpod.InspectNetwork handler as /libpod/networks/{id}/json — /exists being
precisely the existence oracle the 404-on-hidden behavior exists to deny.
The two layers stay asymmetric on GET /libpod/events, deliberately. Podman
serves /events and /libpod/events from one handler and evaluates several
values under a single event filter key disjunctively, so a label value
injected beside a client-supplied one ORs with it. addOwnerLabelFilter
replaces the key outright and leaves exactly one value, for which disjunctive
and conjunctive evaluation coincide; addVisibilityLabelFilters appends, and a
visibility policy's selectors are ANDed with no second filter key to hold the
extra one. Visibility injects a single selector where the policy has one and
refuses with a 403 where it has more, rather than streaming a superset.
Buffering the response instead is not available: the endpoint is a long-lived
NDJSON stream that proxy.isLongLivedUpstreamRequest already exempts from the
upstream deadline. Tests in both packages pin the asymmetry so a later "fix
the inconsistency" edit fails.
GET /libpod/containers/showmounted is refused by both layers for the reason
GET /libpod/system/df is. libpod.ShowMountedContainers walks
runtime.GetAllContainers() and answers with a bare container-ID to
host-mount-path map, so one body is both a host-filesystem disclosure and a
cross-owner enumeration, with no label to classify an entry by and no query
parameter for a filter to attach to.
POST /libpod/images/scp/{name} copies an image to another host and is now
owner-checked against {name}; the scp route segment is stripped only after
the per-image action suffixes are tried, matching the order gorilla/mux
resolves Podman's own routes in. The batch image endpoints that name their
images in the query string, the two libpod stats collection routes, and what
the disjunctive event filter means for the Docker-compat GET /events on a
Podman upstream are recorded in the Podman guide's Known Limitations instead.
GET /libpod/containers/stats and GET /libpod/pods/stats are collection
endpoints, and a request that names nothing reads the whole host by design
rather than by omission: at Podman v5.8.1 abi.ContainerEngine.ContainerStats
takes its default: branch and streams GetRunningContainers when no containers
argument is given, and entities.ValidatePodStatsOptions sets All when a pod
stats request names nothing at all, commented "Podman v1 compat: if nothing's
specified get all running pods". Neither query struct has a filters member, so
the label-filter injection that scopes GET /libpod/containers/json has nothing
to attach to, and neither report shape carries labels: define.ContainerStats
and entities/types.PodStatsReport both identify their subject by ID and name
and say nothing about who owns it, so a response filter has no field to decide
an entry on either. Both layers now answer 403 before contacting the upstream,
regardless of rollout mode, the way /libpod/system/df and
/libpod/containers/showmounted already do.
/libpod/containers/stats was a live bypass rather than an uncovered gap.
libpodContainerIdentifier classified the path as a container named "stats",
the inspect came back not-found, and checkOwnedResource turns not-found into
pass-through, so an isolated caller received the host-wide stats stream; a
caller who created a container actually named "stats" turned that same check
into an outright allow. libpodPodIdentifier reserves "stats", so
/libpod/pods/stats was never classified at all.
The three refused endpoints move behind one table, filter.LibpodUnscopeableReads(),
which both middlewares read and neither owns, so an endpoint cannot be taught
to one layer while the other still forwards it. Each entry carries the reason
string both layers report and the stem their reason codes are built from.
The per-container GET /libpod/containers/{name}/stats is a different route and
stays allowed and owner-checked, but Podman's own API docs mark it deprecated
and slated for removal in the next major release, and pods have no per-pod
stats route at all. The Podman guide's Known Limitations records that, plus
why the refusal is a plain 403 rather than a truncated stream given container
stats streams by default and pod stats does not.
TestNoShippedPresetAdmitsAnUnscopeableLibpodRead now measures every preset in
app/configs/ through filter.Evaluate rather than by reading the YAML, because
the risk is a broad glob rather than an explicit rule. readonly.yaml is the
one exemption, with its reason recorded in the test: it is a single GET /**
allow that already sets insecure_allow_read_exfiltration: true and says in its
header that it exposes every read endpoint on purpose.
BREAKING CHANGE: podman-readonly.yaml no longer allows GET /libpod/pods/stats.
The preset shipped an explicit allow for a read that no isolation layer can
scope, which is a promise it could not keep, and with the refusal above the
rule could only ever have ended in a 403 for anyone running a layer. Podman
registers no per-pod stats route to narrow it to, so there is nothing to
replace it with; a single-tenant deployment that wants pod stats and runs
neither owner isolation nor a visibility policy has to add the rule back to
its own config.
Sockguard's visibility middleware narrows list and event responses by injecting a label filter into the `filters` query parameter, and that injection is only sound if the daemon ANDs the values it finds under one key. dockerd does. Podman does not, on its event stream. Telling the two apart is therefore a prerequisite for filtering correctly, and nothing in the proxy could do it. Detect probes GET /version through the caller's RoundTripper and reads the Components array. Podman's compat version handler reports a component named "Podman Engine"; moby's SystemVersion reports "Engine". The classifier scans every component before deciding rather than trusting Components[0], and Podman wins over Docker when both names are present, because a daemon that answers to both is the one whose event filter is disjunctive. Matching is case-insensitive and substring-based for Podman, exact for Docker. The failure contract is deliberately narrow: the flavor and the error are never both meaningful, so a caller can branch on the error alone, and a successful probe returns only docker or podman. "auto" is a config spelling and never comes back from a probe, so it cannot reach a consumer as a third state to handle. A non-200, an unparseable body, an unrecognized engine and a body over 1 MiB are all errors rather than a guess. FuzzClassify pins those invariants against arbitrary upstream responses; 4.2M execs, no crashers. Stdlib only, and no caller yet.
Podman registers GET /events, GET /vN/events and GET /libpod/events on one handler (pkg/api/server/register_events.go at v5.8.1 points all three at compat.GetEvents), so the Docker-compat spelling carries Podman's filter semantics on a Podman upstream. libpod/events/filters.go's applyFilters is disjunctive within a key and conjunctive across keys, and util.FiltersFromRequest flattens every value of `label` into that single key. Visibility injects its selectors by appending, and visibility selectors are ANDed by definition, so a policy with two or more visible_resource_labels selectors became an OR on /events: a client saw every event matching either selector, which is strictly more than the policy allows. Owner isolation was never exposed, because addOwnerLabelFilter replaces the value instead of appending and leaves exactly one, for which the two evaluations coincide. The fix needs to know which engine is behind the socket, so upstream adds a `flavor` field. An explicit docker or podman is taken as written and issues no request at all; auto probes GET /version once at startup, after the reachability check, bounded by a 5s timeout. Resolution is process-scoped because upstream.socket, upstream.endpoints and now upstream.flavor are all reload-immutable, so no reload can point sockguard at a different daemon. An ambiguous probe fails startup rather than picking a side. Defaulting to docker silently restores the hole on the exact deployment the field exists for, and defaulting to podman breaks working Docker installs on a transient failure; only refusing to start is wrong in a direction the operator can see, and the error names the two values that resolve it. Narrowing the failure to "only when the config has selectors" was rejected: that predicate would have to restate visibility's own notion of having selectors, and drift between the two fails open. On Podman, /events now takes a single selector as the sole `label` value, replacing whatever the client sent, because a surviving client value would sit beside it under the same disjunctive key. Two or more selectors are refused with a 403 and reason visibility_podman_events_unscopeable. Refusal rather than a superset: /events is a long-lived stream that this package cannot filter on the response side without buffering it forever, and a client watching a silently widened stream cannot tell it from a quiet host. A patterns-only policy is forwarded untouched, as before. The refusal ignores rollout warn mode on purpose; warn trades one response for one log line, which does not hold for a stream that runs as long as the client keeps the connection. Docker upstreams are byte-for-byte unchanged: the dispatch is gated on the resolved flavor, and TestDockerCompatEventsIsUnchangedByFlavorDetection drives both an explicit docker and an unset field through the real middleware against a conjunctive fake daemon.
…hly tiers FuzzClassify and FuzzConfigured (upstreamflavor) and FuzzPodmanEventLabelFilter (visibility) shipped unscheduled, which scripts/workflow-fuzz-coverage.test.mjs rejects: every in-tree target must run in at least one tier. Nightly and monthly only, not the branch-CI Tier 1 matrix. Tier 1 runs on every PR and its inventory is pinned exactly by reusable-ci-config.test.mjs, so adding targets there costs PR wall-clock for all three and edits a frozen contract. The coverage gate is satisfied either way.
The runtime chain is visibility then ownership, and both write the same
`label` filter key on the way upstream. Visibility appends its
configured selectors; addOwnerLabelFilter then replaced the key
outright. Ownership runs second, so on every list endpoint both cover
(/containers/json, /images/json, /networks, /volumes, /services,
/tasks, /secrets, /configs, /nodes, /events and the /libpod/
counterparts) the daemon received {"label":["com.sockguard.owner=..."]}
and nothing else. The caller got owner scoping and no visibility
scoping, results were wider than configured, and nothing logged it.
Reordering the two would only move the clobber, so the layers now
compose explicitly. dockerfilters records, per request, which filter
values sockguard itself injected; ownership keeps those and still drops
every client-supplied value under the key. Both scopes reach the daemon
together.
That is safe because `label` is the exception to the usual reading that
one filter key is an OR. Docker matches it with
api/types/filters.Args.MatchKVList, which returns false unless every
value matches, on containers (daemon/list.go), images
(daemon/images/image_list.go and the containerd snapshotter's
setupLabelFilter), networks (daemon/network/filter.go), volumes
(volume/service/by.go) and events (daemon/events/filter.go). Podman
matches it with containers/common's filters.MatchLabelFilters, which
returns false on the first value with no matching label. Args.Match,
the disjunctive matcher, is what `name` and `status` use, not `label`.
So the two selector sets narrow rather than widen.
Client-supplied values stay dropped for a reason the old comment got
wrong. It is not OR semantics: Docker's Swarm control-plane lists fold
`label` through convertKVStringsToMap over a randomly-ordered
Args.Get (daemon/cluster/filters.go), so a client value repeating the
owner key can displace the enforced one nondeterministically. The same
collapse is why response.visible_resource_labels may no longer select
on the ownership label key while ownership.owner is set, per profile or
globally; that combination is now a startup error rather than a filter
the daemon resolves by coin flip.
startUnixHTTPServer named its socket from time.Now().UnixNano(), which two parallel subtests can read identically. It collided during a full-suite run on 2026-08-29 and failed with "bind: address already in use". t.TempDir() is not the fix here: it derives the path from the test name and nests, and a unix socket path is capped at 104 bytes on macOS, which is why the helper avoided it in the first place. os.MkdirTemp with a short prefix is kernel-unique and keeps the path well inside the limit.
|
Deployment failed for project sockguard-website with the following error: Learn More: https://vercel.com/codeswhat?upgradeToPro=build-rate-limit |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
scttbnsn
changed the title
…lity-coverage # Conflicts: # CHANGELOG.md
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
# Conflicts: # CHANGELOG.md # app/internal/visibility/middleware.go # docs/content/docs/configuration.mdx
…rship-visibility-filter-compose
…od-visibility-coverage
This was referenced Aug 30, 2026
…od-visibility-coverage
…od-visibility-coverage
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
1 participant
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.