Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sysusers: cross-check user and group names too #25107

Merged
merged 2 commits into from Nov 6, 2022

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Oct 24, 2022

This adds an additional name check when cross-matching new group entries against existing users, which allows coalescing entries matching both ID and name.
It provides a small idempotence enhancement when creating groups in cases where matching user entries are in place.
By fine-tuning the conflict detection logic, this avoids picking up new random IDs and correctly prefers configuration values instead.

Refs:

Copy link
Member

@bluca bluca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, but could you please add a testcase covering this in test/test-sysusers.sh.in? Given it's a corner case, otherwise it will risk regressing

@poettering
Copy link
Member

hmm, can you give an example what previously wasn't allowed and now is?

src/sysusers/sysusers.c Outdated Show resolved Hide resolved
@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed good-to-merge/with-minor-suggestions labels Nov 2, 2022
This adds an additional name check when cross-matching new group
entries against existing users, which allows coalescing entries
matching both ID and name.
It provides a small idempotence enhancement when creating groups
in cases where matching user entries are in place. By fine-tuning
the conflict detection logic, this avoids picking up new random
IDs and correctly prefers configuration values instead.
@lucab lucab force-pushed the ups/sysusers-gid-check-username branch from 762b6fc to 76ad8ef Compare November 3, 2022 15:13
@lucab
Copy link
Contributor Author

lucab commented Nov 3, 2022

Rebased, const-ified, and added a new test-sysusers/test-15 entry to cover this case.

It fixes the following scenario (observed in the wild as a somehow degenerate side-effect of nss-altfiles forwarding to initrd):

$ cat etc/passwd 
root:x:0:0:root:/root:/bin/bash

$ cat etc/sysusers.d/testcase.conf 
u root    0     "Super User" /root

$ systemd-sysusers --root .
Creating group 'root' with GID 999.

@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Nov 3, 2022
@poettering
Copy link
Member

lgtm

@bluca bluca merged commit f10ad99 into systemd:main Nov 6, 2022
@lucab lucab deleted the ups/sysusers-gid-check-username branch November 7, 2022 07:53
@keszybz keszybz removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants