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: Pro Plus

Run ID: 380178f3-6b5d-477c-9514-de0eb6a62d74

📥 Commits

Reviewing files that changed from the base of the PR and between 9e2b242 and f0dc24b.

📒 Files selected for processing (2)

• docs/release-notes.md
• internal/as_user/as_user.c

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

📜 Recent review details

⚠️ CI failures not shown inline (1)

Commit Status: continuous-integration/jenkins/pr-merge: continuous-integration/jenkins/pr-merge

Conclusion: failure

This commit cannot be built

🧰 Additional context used

📓 Path-based instructions (1)

docs/**

⚙️ CodeRabbit configuration file

docs/**: Documentation served via GitHub Pages/Jekyll. Every platform must be documented in supported-platforms.md. The ./test script validates doc consistency.

Files:

• docs/release-notes.md

🔇 Additional comments (2)

internal/as_user/as_user.c (1)

18-18: LGTM!

Also applies to: 58-60, 70-77

docs/release-notes.md (1)

22-23: LGTM!

---

📝 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 a release-note entry.

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.

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

Merge Risk: ⚪ Minimal · up to f0dc2

The change clears supplementary groups before dropping user and group privileges, preventing retained group memberships from granting unintended access. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: prestist

🚥 Pre-merge checks | ✅ 6

✅ Passed checks (6 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title follows the required format and accurately describes dropping supplementary groups in internal/as_user.
Description check	✅ Passed	The description clearly explains the supplementary-group bug, the fix, its impact, and validation results.
Linked Issues check	✅ Passed	The changes satisfy issue #2242 by clearing supplementary groups before changing the effective GID and UID and returning errors on failure.
Out of Scope Changes check	✅ Passed	The code and release-note changes directly support issue #2242 and contain no unrelated scope.
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 unsupported.)
Commit Message Convention	✅ Passed	The PR has one non-merge commit: internal/as_user: drop supplementary groups; its subsystem is a path prefix, drop is lowercase imperative, and there is no trailing period.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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