fix(ownership): stop discarding visibility selectors on shared list endpoints - #381
Draft
scttbnsn wants to merge 10 commits into
Draft
fix(ownership): stop discarding visibility selectors on shared list endpoints#381scttbnsn wants to merge 10 commits into
scttbnsn wants to merge 10 commits into
Conversation
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 |
biggest-littlest
approved these changes
Aug 29, 2026
biggest-littlest
left a comment
biggest-littlest
left a comment
Member
There was a problem hiding this comment.
Approving on behalf of the review rotation.
ALARGECOMPANY
approved these changes
Aug 29, 2026
ALARGECOMPANY
left a comment
ALARGECOMPANY
left a comment
Member
There was a problem hiding this comment.
Approving on behalf of the review rotation.
# Conflicts: # CHANGELOG.md # app/internal/visibility/middleware.go # docs/content/docs/configuration.mdx
…rship-visibility-filter-compose
This was referenced Aug 30, 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.
Ownership was silently discarding every visibility selector on shared list endpoints.
The bug
Runtime layer order is visibility, then ownership. Visibility appends its selectors to the
labelfilter key;addOwnerLabelFilterreplaces that key. Ownership runs second, so on any endpoint where both are configured the forwarded query carried the owner selector and nothing else. The caller got owner scoping and no visibility scoping, with no log line saying so.It reproduced on every shared path:
docker_containers,docker_images,docker_volumes,docker_nodes,libpod_containers,libpod_volumes.Nothing caught it because each layer's tests configure only that layer, and the failure is silently wider results rather than an error.
Appending is correct here, and the reason is not what you'd guess
labelvalues are ANDed, not ORed, so composing narrows. Verified in source rather than assumed:api/types/filters/parse.go:132MatchKVListreturns false unless every value matches (docker/docker@v28.5.0). Call sites:daemon/list.go:463, both image backends'setupLabelFilter,daemon/network/filter.go:36,volume/service/by.go:79.pkg/filters/filters.go:105MatchLabelFiltersreturns false on the first value with no matching label.The "one filter key is a disjunction" rule is real but applies to
Args.Matchfields likenameandstatus, and to Podman's event filters. Not tolabelon list endpoints. Those are different code paths and both rules are true.The hazard the old code was actually defending against
Docker's Swarm control-plane lists (
/services,/tasks,/nodes,/secrets,/configs) foldlabelthroughconvertKVStringsToMapinto amap[string]string.Args.Getiterates a map, so duplicate label keys collapse to a nondeterministic winner. Distinct keys still AND.So client-supplied label values do still have to be dropped, but the reason is key collapse, not disjunction. Ownership keeps dropping them.
What changed
app/internal/dockerfilters/injected.gorecords, per request, which filter values sockguard itself injected. Visibility records its selectors unconditionally, including ones the client already sent (no query rewrite needed, but they're still policy-enforced). Ownership preserves those and still drops everything client-supplied. Order-independent, so it doesn't just move the clobber somewhere else.One breaking change:
response.visible_resource_labelsmay no longer select on the ownership label key whileownership.owneris set. That's now a startup error. Without it the config resolves nondeterministically on Swarm lists, so it silently loses the visibility scope today. Blast radius is narrow, since it only fires when both are set and name the same key.Verification
11/11 mutations killed. Two survived the first pass and both were real test defects, not spurious:
slices.Clipis only observable with spare capacity, so the fixture now derives a size that has itContainscheck passed off the wrong error. Counted now, not matched.go build,go vet,gofmt,golangci-lint(0 issues),go test ./...green across 3 consecutive full runs. Pre-push hooks ran in full.Also fixes a pre-existing flake found on the way:
startUnixHTTPServernamed its socket fromUnixNano(), which two parallel subtests read identically, and it collided once withbind: address already in use.Left for separate work
/libpod/secrets/jsonis broken on Podman and was before this change.IfPassesSecretsFilteraccepts onlynameandidand errors onlabel, which both layers already injected. Fail-closed, not a leak, but that endpoint errors rather than filters. There are libpod lanes in flight, so it shouldn't land here.Draft until the review queue has room.