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

nixos: add per-user groups by default #199705

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Nov 5, 2022

Description of changes

Each user now defaults to primary group <user_name>. Before it was
users 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:

Ubuntu 22.04 : drwxr-x--- 2 user1 user1 4096 Oct 28 15:17 /home/user1
Fedora 36    : 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
Arch Linux   : drwx------ 2 user1 user1 4096 Oct 28 15:17 /home/user1

The new functionality is gated behind an option called
users.solitaryByDefault, which defaults to true for
system.stateVersion >= 24.05.

Fixes #198296.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Nov 5, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Nov 5, 2022
nixos/modules/config/users-groups.nix Show resolved Hide resolved
nixos/modules/config/users-groups.nix Outdated Show resolved Hide resolved
Copy link
Member

@sternenseemann sternenseemann left a 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.

nixos/modules/config/users-groups.nix Show resolved Hide resolved
@bjornfor
Copy link
Contributor Author

bjornfor commented Nov 9, 2022

CC release managers for input: @dasJ @mweinelt

@bjornfor bjornfor force-pushed the nixos-per-user-groups branch 2 times, most recently from 75b78af to 862d960 Compare December 30, 2022 13:18
@bjornfor bjornfor marked this pull request as draft December 30, 2022 13:20
@bjornfor bjornfor force-pushed the nixos-per-user-groups branch 2 times, most recently from fd21ae0 to 22bc2de Compare December 30, 2022 13:27
@bjornfor bjornfor marked this pull request as ready for review December 30, 2022 13:34
@bjornfor
Copy link
Contributor Author

Rebased and updated to target NixOS 23.05. Please review!

@qb42
Copy link

qb42 commented Jan 5, 2023

This looks great, I was just looking for an option to do exactly this.

I do have a couple of questions:

  1. Do you plan to change the default homeMode to something like 0750?
  2. How will the gid be chosen? I believe in other distros it matches the uid if possible.

@bjornfor
Copy link
Contributor Author

bjornfor commented Jan 5, 2023

1. Do you plan to change the default homeMode to something like 0750?

I like it. But in a follow-up PR? Or do you feel it belongs in this one?

2. How will the gid be chosen? I believe in other distros it matches the uid if possible.

I think that's handled by this code:

sub allocId {
my ($used, $prevUsed, $idMin, $idMax, $up, $getid) = @_;
my $id = $up ? $idMin : $idMax;
while ($id >= $idMin && $id <= $idMax) {
if (!$used->{$id} && !$prevUsed->{$id} && !defined &$getid($id)) {
$used->{$id} = 1;
return $id;
}
$used->{$id} = 1;
if ($up) { $id++; } else { $id--; }
}
die "$0: out of free UIDs or GIDs\n";
}

Correlating UID and GID would be nice.

@bjornfor
Copy link
Contributor Author

bjornfor commented Apr 2, 2023

Rebased to resolve conflicts. Please consider for NixOS 23.05.

@RaitoBezarius
Copy link
Member

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.

@RaitoBezarius RaitoBezarius self-assigned this Apr 3, 2023
@RaitoBezarius
Copy link
Member

Apologies, it completely slipped from my mind, we missed the window for this change, let's get this merged after the branch-off.

@bjornfor
Copy link
Contributor Author

About the NIXPKGS_ALLOW_UNSUPPORTED_SYSTEM=1 nix-instantiate --attr darwin.linux-builder --show-trace error.

I found that nixos/modules/profiles/macos-builder.nix disables NixOS modules that reference stateVersion, but disabling nixos/modules/config/users-groups.nix doesn't work because that results in error: The option 'boot.initrd.systemd.groups' does not exist.. It seems users/groups is a bit too low-level of a module to disable.

CC @Gabriella439, author of the darwin builder (ref. #206951). Do you have any ideas/tips?

@Gabriella439
Copy link
Contributor

You'll probably want to cc: @roberth since he added the stateVersion-related stuff to macos-builder.nix. See: #207958

@roberth
Copy link
Member

roberth commented Nov 6, 2023

If you could factor lib.versionAtLeast nixosConfig.system.stateVersion "23.11" into an option default, e.g. users.solitaryByDefault or something, we can set a fixed value for that macos-builder.nix, thereby avoiding stateVersion.

@RaitoBezarius
Copy link
Member

@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?

@bjornfor
Copy link
Contributor Author

@RaitoBezarius: Yes, I'll try to post an update within a week.

@bjornfor bjornfor marked this pull request as draft January 13, 2024 08:36
@bjornfor
Copy link
Contributor Author

Should we make users.solitaryByDefault an internal/hidden option, or make it public and document that users can have the latest stateVersion but still get the common "users" group by setting users.solitaryByDefault = false?

@bjornfor
Copy link
Contributor Author

Should we make users.solitaryByDefault an internal/hidden option, or make it public and document that users can have the latest stateVersion but still get the common "users" group by setting users.solitaryByDefault = false?

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.

@roberth
Copy link
Member

roberth commented Jan 13, 2024

I don't think we have opt-out for other NixOS options that change behaviour with stateVersion updates

You've found the problem ;)
We should have those; maybe not everywhere, but a stateVersion-free core system is valuable if you're packaging images.

@bjornfor bjornfor changed the title nixos: add per-user groups nixos: add per-user groups by default Jan 21, 2024
@bjornfor bjornfor marked this pull request as ready for review January 21, 2024 18:23
@bjornfor
Copy link
Contributor Author

Updated with users.solitaryByDefault option. Please review.

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Jan 21, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
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.
@bjornfor
Copy link
Contributor Author

bjornfor commented Apr 3, 2024

Re: CI failure:

Build NixOS manual / nixos (pull_request_target) Failing after 21s details

...seems to be an unrelated Cachix failure?

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 3, 2024
@bjornfor
Copy link
Contributor Author

bjornfor commented Apr 4, 2024

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))
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.)

Copy link
Contributor

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.

Copy link
Contributor

@nbdd0121 nbdd0121 Apr 5, 2024

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...

Copy link
Contributor Author

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?

Copy link
Contributor

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))

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 2, 2024
@RaHoni
Copy link
Contributor

RaHoni commented Jul 3, 2024

Are there any updates on this?

@bjornfor
Copy link
Contributor Author

bjornfor commented Jul 3, 2024

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).

@nyabinary
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: clean-up 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
No open projects
Status: Deferred
Development

Successfully merging this pull request may close these issues.

nixos: have per-user groups instead of common 'users' group