agent/workloadattestor/docker: add rootless Podman support#6798
agent/workloadattestor/docker: add rootless Podman support#6798ChSerhiiOds wants to merge 3 commits intospiffe:mainfrom
Conversation
ChSerhiiOds
requested review from
MarcosDY,
amartinezfayo,
evan2645,
rturner3 and
sorindumitru
as code owners
Signed-off-by: ChSerhiiOds <drwolf4@gmail.com>
ChSerhiiOds
force-pushed
the
podman-support
branch
from
3adb2d6 to
c0a7786
Compare
amartinezfayo
left a comment
amartinezfayo
left a comment
There was a problem hiding this comment.
Thank you so much for this contribution, @ChSerhiiOds!
Some minor comments/suggestions.
| continue | ||
| } | ||
| if m := reUserSliceUID.FindStringSubmatch(cg.GroupPath); m != nil { | ||
| if uid, err := strconv.ParseUint(m[1], 10, 32); err == nil { |
There was a problem hiding this comment.
I'm wondering if we should log a warning when ParseUint fails here. The regex matched a user slice, so the workload is rootless, but the code would silently fall through to the rootful Podman socket.
There was a problem hiding this comment.
Good point, thanks! Added a Warn log with the raw UID and cgroup path before falling back to the rootful socket.
| ImageInspectWithRaw(ctx context.Context, imageID string) (image.InspectResponse, []byte, error) | ||
| } | ||
|
|
||
| type closeableDocker interface { |
There was a problem hiding this comment.
I think it would be cleaner to make the close obligation explicit at the type level, for example changing podmanClientFactory to return (Docker, io.Closer, error), rather than relying on a runtime type assertion in Attest.
I believe that would make the contract more discoverable. Looks like right now, to learn that the returned client should be closeable, you have to trace through Attest and find the type assertion.
There was a problem hiding this comment.
Makes sense, thanks! Replaced the runtime type assertion with a podmanDocker interface embedding Docker and Close() error, so podmanClientFactory now returns (podmanDocker, error) directly.
| podman_socket_path_template = "unix:///run/user/%s/podman/podman.sock" | ||
| `) | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "invalid podman_socket_path_template") |
There was a problem hiding this comment.
Would it make sense to also cover the trailing-% edge case? For example, "unix:///run/user/%d/podman%" or "unix:///run/user/podman%" would exercise the "trailing % at end of template" branch in validatePodmanSocketPathTemplate, which currently has no test coverage.
There was a problem hiding this comment.
Sure, thanks! Added two trailing-% cases to the invalid template test to cover that branch.
Signed-off-by: ChSerhiiOds <drwolf4@gmail.com>
2 participants
Pull Request check list
Affected functionality
Agent
WorkloadAttestor "docker"on Unix systems, specifically runtime socket selection for Podman workloads (including rootless Podman).Description of change
This change adds Podman support to the Docker workload attestor path and routes API calls to the correct runtime socket based on cgroup detection.
Key updates:
/user-<uid>.slice/and using a UID-based socket template.podman_socket_path(rootful Podman socket)podman_socket_path_template(rootless Podman socket template with%dUID placeholder)podman_socket_path_templateat configure time (must contain exactly one%d).Tests added/updated:
podman_socket_path_templaterejection test.Which issue this PR fixes
Fixes #6549