internal/as_user: drop supplementary groups

[JasonColapietro]

Summary

set_eids() in internal/as_user/as_user.c switches the effective gid and uid
to the target user, but never touches the supplementary group list. The
privilege-dropped thread therefore keeps the caller's group memberships — in
practice root's — while acting as that user.

This drops the list with setgroups(0, NULL) before the gid and uid are
relinquished, while the thread still holds the privilege required to make that
call, following the revocation order described in
CERT POS36-C.

Reproducer

I built a probe against as_user.c before and after the change. It makes the
caller (root) a member of gid 4242, creates a directory owned root:4242 with
mode 0770 — so "other" has no access — and then asks au_open() to create a
file inside it as uid=65534 gid=65534, which is not a member of 4242:

	result
before	au_open succeeded (fd=3): the thread kept root's membership of gid 4242
after	au_open failed with EACCES, as it should

So the retained groups do grant real access during the switch, not just a
theoretical capability.

Impact

Low, and I don't want to oversell it. As #2242 notes, Ignition already runs as
root and as_user is defense-in-depth rather than a security boundary; the only
caller is writeAuthKeysFile() in internal/exec/util/passwd.go, writing SSH
authorized keys during first boot, where root's supplementary groups are
typically just {0}. This is a correctness fix that brings the privilege drop
in line with POSIX practice, and matters more for any future reuse of this code.

Notes

• <grp.h> is added for the setgroups() declaration; the cgo build uses
  -Werror=implicit-function-declaration, so a missing declaration would be
  fatal rather than silent.
• The call fails closed: if setgroups() fails, set_eids() returns an error
  rather than continuing with a partial privilege drop.
• setgroups(0, NULL) clears the list rather than installing the target user's
  own groups. Installing the real list would mean either an NSS lookup inside
  the cloned thread or widening au_ids_t to carry the group list from Go.
  Happy to do the latter if you'd prefer it — clearing seemed like the right
  minimal fix for the reported issue.

Testing

• ./test — Success, exit 0 (Fedora 44, Go 1.24, cgo, run as a non-root
  user)
• ./build ignition — exit 0
• git diff --check — clean
• The probe above, compiled against the original and patched as_user.c

Running ./test as root instead fails TestTranslateTree/translate_7 in the
seven butane/base/* packages, because that test asserts a permission-denied
error which root bypasses. That failure reproduces identically on an unmodified
origin/main, so it is unrelated to this change.

No new in-tree test: internal/as_user is Linux-, cgo- and root-only, has no
existing unit tests, and a regression test for this would have to manipulate the
test process's own credentials. I'm glad to add a root-gated one if you want it.

Fixes #2242.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: fb5d79d5-2d1e-4c6f-a686-96e26fd999b8

📥 Commits

Reviewing files that changed from the base of the PR and between c6e73c9 and 27bbd35.

📒 Files selected for processing (1)

• docs/release-notes.md

🚧 Files skipped from review as they are similar to previous changes (1)

• docs/release-notes.md

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

set_eids now removes supplementary groups before lowering effective gid and uid privileges. It returns failure if group removal fails, updates its documentation, and adds release-note entries.

Changes

Privilege-drop hardening

Layer / File(s)	Summary
Clear supplementary groups during credential changes internal/as_user/as_user.c, docs/release-notes.md	set_eids calls setgroups(0, NULL) before changing credentials and returns -1 if the call fails. Its documentation and the release notes describe the behavior.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: yasminvalim

Merge Risk: ⚪ Minimal · up to 83874

The privilege-drop path now removes supplementary groups before the requested operation. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PR adds a release-note entry for SELinux relabeling on OSTree platforms and references PR #2316. That change is unrelated to issue #2242 and is not needed to document supplementary-group removal.	Remove the unrelated #2316 release-note entry from this pull request, or move it to the pull request that implements the SELinux relabeling fix.

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title follows the required format: it uses the subsystem prefix "internal/as_user", a lowercase imperative description, and no trailing period. It accurately describes the main change.
Description check	✅ Passed	The description clearly explains the supplementary-group privilege-drop fix, its impact, implementation, testing, and linked issue. It is directly related to the changeset.
Linked Issues check	✅ Passed	The change satisfies issue #2242. internal/as_user/as_user.c includes <grp.h> and calls setgroups(0, NULL) before setregid() and setreuid(). The function returns -1 when group removal fail…
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 …
Commit Message Convention	✅ Passed	The review range contains one non-merge commit: internal/as_user: drop supplementary groups. The subsystem is a valid file path prefix, the description starts with lowercase imperative drop, and i…

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Binary size report (bin/amd64/ignition)

	Size
Base (main)	33MiB
PR (#2301)	33MiB
Delta	+16B (0.00%)

[yasminvalim]

Hey, @JasonColapietro! Nice work!
I guess you just need to rebase and handle the conflicts, then we can run the workflow again :)

[prestist]

@JasonColapietro Nice fix,lgtm.

One thought; we already have root-gated tests in internal/exec/util/user_group_lookup_test.go using the t.Skip("test requires root") pattern. Might be worth adding something similar here in internal/as_user/ to verify the groups actually get dropped. Like creating a dir that's only group-accessible (0070), then calling OpenFile as a user not in that group and checking it gets EACCES.

Not blocking, just wanted to know your thoughts?