internal/exec/util: handle existing group in EnsureGroup by onurhanak · Pull Request #2158 · coreos/ignition

onurhanak · 2025-11-17T20:16:17Z

When groupadd is called with a group name that already exists, it returns exit code 9. This causes ignition to fail. This change catches exit code 9 and treats it as success to make EnsureGroup idempotent.

gemini-code-assist

Code Review

This pull request makes EnsureGroup idempotent by handling the exit code from groupadd when a group already exists. While the change prevents a crash, it introduces a more subtle issue. If a group exists, any configuration changes for that group (like GID or password) will be silently ignored, leading to potential configuration drift. My review comment details this problem and suggests a more robust implementation that uses groupmod for existing groups, similar to how EnsureUser is implemented. This would ensure that group configurations are correctly applied whether the group is being created or modified.

gemini-code-assist · 2025-11-17T20:17:34Z

internal/exec/util/passwd.go

+	if err != nil && exitCode == 9 {
+		return nil
+	}


While this change correctly makes EnsureGroup idempotent for group creation and prevents a crash, it introduces a potential issue of silent configuration drift. If a group already exists, this change will cause the function to return success without applying any specified modifications like Gid or PasswordHash. This can lead to the system being in an incorrect state without any warning.

To address this properly, EnsureGroup should be updated to handle existing groups by modifying them, similar to how EnsureUser uses usermod. I recommend the following approach:

Use the exists variable (which is already available) to differentiate between creating and modifying a group.

If the group exists, use groupmod to apply any changes to GID or password hash. This will require adding a GroupmodCmd to internal/distro/distro.go.

If the group does not exist, use groupadd as is currently done.

This will make EnsureGroup robust and ensure the system state matches the desired configuration.

This is actually the SAME design limitation that EnsureUser has, and is documented in config/doc/ignition.yaml

- name: system desc: "whether or not the group should be a system group. This only has an effect if the group doesn't exist yet."

You are right. That is a limitation of groupmod and usermod. I opted to maintain consistency. Do you think that should be handled differently?

Nah, I am pretty sure keeping it as it has been is correct. Its been documented as a limitation for a while. I wanted to just comment on it so there is a trail in-case we ever need to ask why it was done this way.

onurhanak · 2025-11-19T20:53:54Z

Looks like I forgot to update release notes, should I revise the PR?

Edit: updated.

prestist

Great find, thank you for the fix, the changes make sense and LGTM

edit: sorry

One request, would you mind adding blackbox tests that follow the ensureUser example but for ensureGroup. as there are no create/addition tests that I can see.

You can find the user tests and group tests mixed in => ignition/tests/positive/passwd/users.go

It looks like we can expand the group coverage for creation and modification now, you might need to expand the stubs in => ignition/tests/stubs/groupdel-stub/main.go

Noticed the need for test coverage.

Add groupadd-stub and groupmod-stub test helpers to test the groupadd and groupmod commands. Add test cases for group creation and group idempotency to verify EnsureGroup uses groupmod when a group already exists.

prestist · 2025-12-03T15:46:25Z

I think the only thing we should do is re-base rather then the merges from main, its not terrible but it does add noise to the git history. Otherwise lgtm.

When a group already exists, modify it using groupmod instead of attempting to create it with groupadd. This makes EnsureGroup idempotent and prevents failure when the group already exists (exit code 9).

onurhanak · 2025-12-03T15:57:36Z

Does this look better?

prestist · 2025-12-03T16:08:30Z

@onurhanak perfect! thank you.

prestist

LGTM

prestist · 2025-12-04T03:26:24Z

@onurhanak unfortunately looks like the blackbox tests are failing

You can test them locally by

./build_blackbox_tests
sudo sh -c 'PATH=$PWD/bin/amd64:$PATH ./tests.test -test.run <testname>'

so if you want to test groups.add_3.6.0-experimental it would look like:

sudo sh -c 'PATH=$PWD/bin/amd64:$PATH ./tests.test -test.run TestIgnitionBlackBox/groups.add_3.6.0-experimental

onurhanak · 2025-12-04T12:49:37Z

@prestist Theblackbox tests pass locally on my end, any idea why they are failing here? This is from the Jenkins log:

passwd.go:173: "group1" wasn't correctly created: got "group1:!::", expected "group1:*::"

I cannot see any reference to group1 in the codebase when I look through the tests. Also, when I check /etc/gpasswd I see it uses !, not * for empty passwords. Any suggestions on how to proceed?

prestist · 2025-12-04T14:40:58Z

@prestist Theblackbox tests pass locally on my end, any idea why they are failing here? This is from the Jenkins log:

passwd.go:173: "group1" wasn't correctly created: got "group1:!::", expected "group1:*::"

I cannot see any reference to group1 in the codebase when I look through the tests. Also, when I check /etc/gpasswd I see it uses !, not * for empty passwords. Any suggestions on how to proceed?

Thats soo weird, at first glance no, but looking into it currently.

prestist · 2025-12-04T17:01:41Z

@onurhanak Ah, its a kola test this time. No longer blackbox tests

Our kola tests are mostly in fedora-coreos-config here is an example of one for passwd on users https://github.com/coreos/fedora-coreos-config/blob/c20db91bd5780a3da22204677b251048cf91b8d7/tests/kola/security/passwd/test.sh

I cant seem to find the one that is failing but its likely in this repo, still investigating.

Ah this kola test is in coreos-asembler's repo https://github.com/coreos/coreos-assembler/blob/bf8b944d6fd211cb3f9071820934db65138b80b6/mantle/kola/tests/ignition/passwd.go#L144

Looks like the testgroup section is not passing due to the changes here. The * makes me think about the else you removed from passwd.. but not sure.

onurhanak · 2025-12-04T17:10:19Z

@prestist Thanks. I think the error is caused by the EnsureGroup function in internal/exec/util/passwd.go . Specifically these lines:

	if util.NotEmpty(g.PasswordHash) {
		args = append(args, "--password", *g.PasswordHash)
	}

I see that other functions in the file default to "*" when password is nil. The tests seem to expect this behavior.

I can implement that, but groupadd man page specifies that it defaults to "!" when password is not supplied:

-p, --password PASSWORD
           defines an initial password for the group account. PASSWORD is expected to be encrypted, as returned by crypt (3).

           Without this option, the group account will be locked and with no password defined, i.e. a single exclamation mark in
           the respective field of ths system account file /etc/group or /etc/gshadow.

           Note: This option is not recommended because the password (or encrypted password) will be visible by users listing the
           processes.

           You should make sure the password respects the system's password policy.

How do you think we should proceed?

prestist · 2025-12-04T18:51:51Z

Hmm, it makes my hands itch to give the final verdict. Let me reach out to a more knowledgeable party => @travier for their view.

So from my perspective, we should probably maintain what has been true in the past and what has been enforced by the kola test. Mostly to avoid unintentional configuration drift. I.e bring back the default of * when not specified.

onurhanak · 2025-12-09T10:42:44Z

@travier what do you think?

travier · 2025-12-10T09:33:06Z

This PR adds groupadd-stub & groupmod-stub binaries that I don't think should be included.

How do you think we should proceed?

For the * vs !, gshadow says:

If the password field contains some string that is not a valid
result of crypt(3), for instance ! or *, users will not be
able to use a unix password to access the group (but group
members do not need the password).

So I think we should update our kola test to accept both.

travier · 2025-12-10T09:34:15Z

I have not looked too closely to the tests but the code change looks OK from a quick look. Thanks

onurhanak · 2025-12-10T13:22:35Z

This PR adds groupadd-stub & groupmod-stub binaries that I don't think should be included.

You are right. My mistake, should be fixed now.

@prestist What are the next steps? I think the kola tests are in another repository right? Should I make a PR there too?

prestist · 2025-12-10T14:07:59Z

@onurhanak yeah the kola tests that are being effected are here. And yeah we would want to create a PR which accepts both * and !. If you would like @onurhanak I can make those changes?

onurhanak · 2025-12-10T15:24:21Z

@prestist Sure, if you do not mind, please go ahead and make the changes.

prestist · 2025-12-11T19:57:40Z

@onurhanak Thank you for working on this, just landed it!

onurhanak · 2025-12-11T19:59:29Z

@prestist That's great news, thanks for the help along the way.

gemini-code-assist bot reviewed Nov 17, 2025

View reviewed changes

onurhanak marked this pull request as draft November 17, 2025 20:22

onurhanak force-pushed the ensure-groupadd-idempotency branch from ecdf347 to 1b3a2d2 Compare November 17, 2025 20:36

onurhanak marked this pull request as ready for review November 17, 2025 20:37

prestist previously approved these changes Dec 3, 2025

View reviewed changes

prestist self-requested a review December 3, 2025 02:48

tests: add blackbox tests for group creation and modification

5c9e7c9

Add groupadd-stub and groupmod-stub test helpers to test the groupadd and groupmod commands. Add test cases for group creation and group idempotency to verify EnsureGroup uses groupmod when a group already exists.

onurhanak marked this pull request as draft December 3, 2025 13:00

onurhanak marked this pull request as ready for review December 3, 2025 13:03

onurhan-ak added 2 commits December 3, 2025 10:53

internal/exec/util: handle existing group in EnsureGroup

634020e

When a group already exists, modify it using groupmod instead of attempting to create it with groupadd. This makes EnsureGroup idempotent and prevents failure when the group already exists (exit code 9).

docs/release-notes: add entry for EnsureGroup fix

7572fa8

onurhanak force-pushed the ensure-groupadd-idempotency branch from c3a467c to 7572fa8 Compare December 3, 2025 15:55

prestist approved these changes Dec 3, 2025

View reviewed changes

build_blackbox_tests: add gldflags for groupadd-stub and groupmod-stubs

8b4499f

internal/exec/util: only pass password if not empty

87da603

onurhanak force-pushed the ensure-groupadd-idempotency branch from ddbc66e to 87da603 Compare December 10, 2025 13:20

prestist mentioned this pull request Dec 10, 2025

kola/tests/ignition/passwd: accept both * and ! for group passwords coreos/coreos-assembler#4387

Merged

prestist merged commit 966af01 into coreos:main Dec 11, 2025
10 checks passed

This was referenced Jan 8, 2026

groupmod executable is not found during ignition phase #2186

Closed

dracut/30ignition/module-setup.sh: include groupmod binary in initramfs #2190

Merged

Conversation

onurhanak commented Nov 17, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

prestist Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

onurhanak Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

prestist Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

onurhanak commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prestist left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prestist commented Dec 3, 2025

Uh oh!

onurhanak commented Dec 3, 2025

Uh oh!

prestist commented Dec 3, 2025

Uh oh!

prestist left a comment

Choose a reason for hiding this comment

Uh oh!

prestist commented Dec 4, 2025

Uh oh!

onurhanak commented Dec 4, 2025

Uh oh!

prestist commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prestist commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onurhanak commented Dec 4, 2025

Uh oh!

prestist commented Dec 4, 2025

Uh oh!

onurhanak commented Dec 9, 2025

Uh oh!

travier commented Dec 10, 2025

Uh oh!

travier commented Dec 10, 2025

Uh oh!

onurhanak commented Dec 10, 2025

Uh oh!

prestist commented Dec 10, 2025

Uh oh!

onurhanak commented Dec 10, 2025

Uh oh!

Uh oh!

prestist commented Dec 11, 2025

Uh oh!

onurhanak commented Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

onurhanak commented Nov 19, 2025 •

edited

Loading

prestist left a comment •

edited

Loading

prestist commented Dec 4, 2025 •

edited

Loading

prestist commented Dec 4, 2025 •

edited

Loading