feat(visibility): scope /events to the policy on a Podman upstream - #380
Draft
scttbnsn wants to merge 7 commits into
Draft
feat(visibility): scope /events to the policy on a Podman upstream#380scttbnsn wants to merge 7 commits into
scttbnsn wants to merge 7 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.
|
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Scopes
GET /eventsto the visibility policy when the upstream is Podman, and adds the upstream-flavor detection needed to know when that applies.Why this needs flavor detection at all
Podman serves
/eventsand/libpod/eventsfrom the same handler (pkg/api/server/register_events.golines 39, 41, 73 in v5.8.1), so the Docker-compat spelling carries Podman's semantics. And Podman's event filters are disjunctive within a key:libpod/events/filters.go:112says "Filters under the same key are disjunctive while each key must match". Docker's are not.So appending a second
labelselector, which is what the visibility layer does everywhere else, widens the stream on Podman instead of narrowing it. There's no way to tell from the request which daemon is on the other end, henceupstream.flavor.Behavior
autoprobesGET /versiononce at startup and reads the engine name (Podman EnginevsEngine). An ambiguous result fails startup rather than guessing:dockersilently restores the exact hole on the exact deployment the field exists forpodmanbreaks working Docker installs on a transient blipFailing is the only outcome that's wrong in a direction the operator can see. The probe runs after the reachability check, so an unreachable daemon reports as unreachable rather than ambiguous.
upstream.flavoris reload-immutable alongsideupstream.socketandupstream.endpoints, so nothing can repoint at a different daemon mid-process.On the
/eventspath itself: zero selectors forwards untouched, one selector is written as the solelabelvalue (replacing what the client sent, since a surviving client value would sit beside it under the same disjunctive key), and two or more returns 403visibility_podman_events_unscopeablewithout contacting the upstream.Refusing beats streaming a superset here.
/eventsis long-lived NDJSON and is already exempt from the upstream deadline, and this package filters responses by buffering the whole body, which would hang the stream forever. A client watching a silently widened stream can't tell it apart from a quiet host. This deliberately ignores rollout warn mode, which trades one response for one log line: that trade doesn't hold for a connection-lifetime stream.Docker is provably untouched.
TestDockerCompatEventsIsUnchangedByFlavorDetectiondrives both explicitdockerand the unset zero value through the real middleware against a conjunctive fake daemon, and the dispatch is a single gated condition.Verification
17/17 mutations killed. Two initially died on a compile error rather than a test failure, so I re-ran them as behavior-only mutations to get real
--- FAILlines. Explicit-wins uses a counting round-tripper that fails if it's ever called, so "no HTTP request at all" is checked rather than assumed.go build,go test,golangci-lint(0 issues),gofmt,-raceon the five changed packages.FuzzClassify4.2M execs,FuzzPodmanEventLabelFilter1.9M execs, no crashers. The three new fuzz targets are scheduled in the nightly and monthly tiers, not branch-CI Tier 1, whose inventory is pinned exactly byreusable-ci-config.test.mjs.Merge notes
Two things need cleanup when this and #379's sibling branch both land, tracked as P23:
docs/content/docs/podman.mdxwill carry both the Known Limitation describing this hole and the paragraph describing the fix. One has to go.setLibpodEventsLabelFilterandsetPodmanEventLabelFilterwill coexist. They're named apart on purpose so they don't collide at compile time, but they should collapse into one.Found along the way, not fixed here
Ownership's
addOwnerLabelFilterreplaces thelabelkey while visibility appends to it, and ownership runs second. So on any list endpoint with both configured, ownership silently discards every visibility selector. I confirmed it with a throwaway test: the forwarded query came back asfilters={"label":["com.sockguard.owner=o1"]}with both visibility selectors gone.That's a live bug on Docker, independent of this PR, and it's being fixed separately. It cuts in this PR's favor: ownership always writes last and leaves exactly one value, so the replace semantics here introduce no ownership bypass.
Draft until the review queue has room.