-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
nixos: add per-user groups by default #199705
base: master
Are you sure you want to change the base?
Conversation
a474b35
to
57e7dcf
Compare
4f9c5a9
to
e464127
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, not sure if the change is good or bad, but moving with other modern distributions can't be terrible.
75b78af
to
862d960
Compare
fd21ae0
to
22bc2de
Compare
Rebased and updated to target NixOS 23.05. Please review! |
This looks great, I was just looking for an option to do exactly this. I do have a couple of questions:
|
I like it. But in a follow-up PR? Or do you feel it belongs in this one?
I think that's handled by this code: nixpkgs/nixos/modules/config/update-users-groups.pl Lines 48 to 60 in 62cace1
Correlating UID and GID would be nice. |
22bc2de
to
5b56504
Compare
Rebased to resolve conflicts. Please consider for NixOS 23.05. |
I'm not too shocked by getting this change for 23.05. But I would rather it early rather than late to smoke out any issue and so we can revert timely. |
Apologies, it completely slipped from my mind, we missed the window for this change, let's get this merged after the branch-off. |
CC @Gabriella439, author of the darwin builder (ref. #206951). Do you have any ideas/tips? |
If you could factor |
@bjornfor This is our new round for getting this in, are you able to push this to the finish line and we go for the merge early? |
@RaitoBezarius: Yes, I'll try to post an update within a week. |
Should we make |
I don't think we have opt-out for other NixOS options that change behaviour with stateVersion updates, so I guess it's odd to introduce one now. |
You've found the problem ;) |
3870d40
to
a7c297c
Compare
Updated with users.solitaryByDefault option. Please review. |
Add a new option, users.solitaryByDefault, that defaults to true for stateVersion >= 24.11. It can be opt-in before that (and opt-out after). When users.solitaryByDefault is true, each user defaults to primary group `<user_name>`, whose group id equals the user id. Before it was `users` (with group id 100) for normal users and system users required explicit setting the group -- because the default was "" (empty) which triggered an assert saying the group must be set. This change allows finer grained control over file access, and aligns NixOS with other distros. Here, the result of `useradd -m user1 && ls -ld /home/user1`: Arch Linux : drwx------ 2 user1 user1 4096 Oct 28 15:17 /home/user1 Debian 11 : drwxr-xr-x 2 user1 user1 4096 Oct 28 15:17 /home/user1 Fedora 36 : drwx------ 2 user1 user1 4096 Oct 28 15:17 /home/user1 Ubuntu 22.04 : drwxr-x--- 2 user1 user1 4096 Oct 28 15:17 /home/user1 Fixes NixOS#198296.
a7c297c
to
bad74b7
Compare
Re: CI failure:
...seems to be an unrelated Cachix failure? |
bad74b7
to
88d5929
Compare
Last rebase/force push was to trigger CI, but I also took the opportunity to improve/correct the release note entry. |
} | ||
# Create per-user primary groups | ||
(lib.optionalAttrs nixosConfig.users.solitaryByDefault | ||
(lib.mapAttrs' (_: user: { name = user.group; value = { gid = lib.mkDefault user.uid; }; }) nixosConfig.users.users)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for statically assigned UIDs, but UID could also be set to null
to mean auto-assign upon activation, and that's the default. I don't see any logic in update-users-groups.pl to ensure identical UID and GID are auto-assigned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any logic in update-users-groups.pl to ensure identical UID and GID are auto-assigned?
Correct, I didn't touch that code. Is that a blocker? You still get per-user groups and everything will work fine, except GID != UID, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users and groups with the same name should have equal uids and gids. We are careful in our static allocation: https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/misc/ids.nix#L671.
Also, currently we dynamic allocate GIDs in 400-999 range which are system IDs, and user UID/GIDs should have 1000-29999.
I think before we land this PR we need to change the allocation script to ensure the constraints are met.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I agree having GID = UID is preferable, I don't understand why that should be a blocker for this PR. Apparently we're OK with the current dynamic allocation that does not have GID = UID, so why would this PR make a difference? (I guess I should update the release notes entry to not mention the GID = UID property, since it's only valid for static UIDs.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current the non-ideal behaviour is only exposed to a services that use both a system user and an identical name groups. This does not affect normal users, nor services that use a fixed UID/GID allocation. But with this change, we're now exposing every single normal user (with state version 24.05+) with this behaviour. It's also more difficult to fix user's file permissions than service states if someone wants to change their GID, since systemd will auto fix ownerships.
Also, I think allocating normal user GIDs to <1000 is a hard blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the per-user group switch could happen together with the perlless activation... (background: systemd sysusers have provision to avoid same ID from being used by user/groups with different names)
EDIT: I think currently NixOS sysusers module doesn't look at isSystemUser at all, so it's also broken...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In perlless NixOS, what code implements the dynamic UID/GID allocations? Is systemd sysusers used for normal users too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this PR that fixes this behaviour (i.e. let sysusers only create system users): #328926
I've reached the point where the only proper solution to our user management is writing my own application that works like systemd-sysusers but also creates normal users and is able to change some data about users (e.g. passwords).
However, I believe it's worth landing this change regardless as it lays the foundation to build software that does this correctly. Since it's gated behind a flag anyway, I don't see a problem.
Btw: isSystemUser
is also fundamentally broken (see #328926 (comment))
Are there any updates on this? |
I guess I'm waiting for the systemd sysusers thing to stabilize and be the default in NixOS (open-ended thread). |
Userborn will be aiming to be the default instead of sysuser according to #332719 |
Description of changes
Each user now defaults to primary group
<user_name>
. Before it wasusers
for normal users and system users required explicit setting thegroup -- because the default was "" (empty) which triggered an assert
saying the group must be set.
This change allows finer grained control over file access, and aligns
NixOS with other distros. Here, the result of
useradd -m user1 && ls -ld /home/user1
:The new functionality is gated behind an option called
users.solitaryByDefault
, which defaults to true forsystem.stateVersion >= 24.05
.Fixes #198296.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes