fix(docker): carry Compose group_add supplementary groups past s6 privilege drop (#75627) - #75632
fix(docker): carry Compose group_add supplementary groups past s6 privilege drop (#75627)#75632kyssta-exe wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for generalizing the existing socket-group repair. The premise is confirmed on current main: docker/stage2-hook.sh:127-135 states that --group-add is lost through s6-setuidgid, but the current repair loop at docker/stage2-hook.sh:147-172 only runs for mounted Docker sockets. The proposed PID 1 group loop is consistent with that existing mechanism.
Problems
- The PR changes only
docker/stage2-hook.shand adds no regression test. This behavior depends on the real s6 lifecycle; the repository's Docker harness builds and runs the image intests/docker/conftest.py:52-70, while the main command performs the relevant privilege drop indocker/main-wrapper.sh:22.
Suggested changes
- Add a Docker integration test that runs the image with
--group-add 1001and asserts the main runtime process retains GID 1001 after cont-init ands6-setuidgid.
This is an automated hermes-sweeper review.
| # supplementary groups are granted (the common case). Avoids the | ||
| # well-known groups that must NOT be granted: the socket case above | ||
| # already handles docker.sock, and we deliberately skip gid 0 (root). | ||
| pid1_groups="$(sed -n 's/^Groups:[[:space:]]*//p' /proc/1/status 2>/dev/null || true)" |
There was a problem hiding this comment.
Please add a real-image Docker regression test for this block: launch with --group-add 1001 and assert a command reached through main-wrapper.sh retains 1001 after s6-setuidgid. The existing tests/docker/conftest.py harness is designed for that lifecycle.
SummaryOne PR addresses #75627. #75632 directly targets the reported cause by carrying PID 1's Docker-granted supplementary GIDs into Related pull requests
Suggested consolidationKeep #75632 open with a salvage path: retain its focused PID 1 supplementary-group repair, but require the author to add the requested Docker integration test using Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I75627(["issue #75627 (open)"])
P75632["PR #75632 (open)"]
P75632 -->|best fix| I75627
class I75627 open
class P75632 open
class P75632 best
class P75632 target
click I75627 "https://github.com/NousResearch/hermes-agent/issues/75627"
click P75632 "https://github.com/NousResearch/hermes-agent/pull/75632"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 3 kB of PR diffs, 8 kB of issue/PR text, 1 kB of discussion (2 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
Summary
When a Hermes container is started with Docker Compose
group_add(or--group-addon the CLI), the supplementary group is granted to the container's PID 1 at the kernel level, but is silently dropped from the actual Hermes runtime process. Thes6-setuidgidprivilege drop callsinitgroups()for the target user, which rebuilds the supplementary group list from/etc/group. Without an/etc/groupentry matching the granted GID, the group is wiped between PID 1 and the dropped process — so bind-mounted directories that rely on a shared Unix group (e.g. acore.sharedRepository groupGit checkout owned by host UID 1000:GID 1001) are unwritable by Hermes.This is the same mechanism already handled for the Docker socket case (#16703) in
docker/stage2-hook.sh; this PR generalizes it.Fix
docker/stage2-hook.sh: after the existing docker.sock group block, read PID 1's supplementary group list from/proc/1/statusand, for each granted GID:/etc/groupentry for the GID,usermod -aGhermes into it.Skipped silently when no supplementary groups are granted (the common case).
Verification
Tested in a real container (
debian:bookworm-slim,--group-add 1001, hermes user UID 10000 like the Dockerfile):sh -nsyntax check passes; the block is idempotent on re-run.Closes #75627