Skip to content

Conversation

@mikitsu
Copy link

@mikitsu mikitsu commented Mar 2, 2025

Previously, when adding a new user when at least two users already exist, this new user was assigned the same subuid range as the second existing user.

The removal of $used->{$id} = 1; from allocId is necessary since allocSubUid is called unconditionally for auto-allocated subuids, while uid/gid assignments do not call alloc* for existing ids. As it's purpose is, as far as i can determine, to avoid repeated calls to getpwuid / getgrgid, this shouldn't have much of an impact (only when adding more than one user/group at a time).

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 2, 2025
@NixOSInfra NixOSInfra added the 12.first-time contribution This PR is the author's first one; please be gentle! label Mar 2, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 2, 2025
@mweinelt mweinelt requested a review from adisbladis March 2, 2025 23:09
@mikitsu
Copy link
Author

mikitsu commented Mar 3, 2025

For systems which currently have coinciding subuid ranges, the current code will silently assign new ranges. There are two ways I can think of to handle this:

  1. still assign new ranges, but add a warning when we detect that a subuid range has changed. This warning can include a copy-pastable config snippet to explicitly set the range for the user to the old value, making it opt-in but easy to keep the current ranges. This would require minimal code changes (just the warning).
  2. keep the old ranges, and add a warning that some ranges coincide. To get non-coinciding ranges, users will need to manually edit /var/lib/nixos/auto-subuid-map. This requires more extensive code changes.

I'd prefer the first option, since

  • I expect relatively few people to be affected: you'd have to have existing coinciding ranges (which implies at least three mot likely human users with autoSubUidGidRange = true) and use podman or other software that makes use of subuids
  • if you're affected, you'll probably want to not have coinciding ranges anyway (unless your users all trust each other)
  • choosing the non-default is easy

However, if not breaking existing systems under any circumstances is important, I'd be happy to implement the second one.

@mweinelt mweinelt added the 1.severity: security Issues which raise a security issue, or PRs that fix one label Apr 2, 2025
@mweinelt mweinelt added this to the 25.05 milestone Apr 5, 2025
@adisbladis
Copy link
Member

I agree going with option number 1 is the best option here, and gate the warning behind system.stateVersion so that new systems that would have been affected with the old behaviour does not emit warnings.
I'd like for some other people to chime in on whether that's an acceptable solution or not.

Code changes LGTM. I've been trying this PR out on my systems (all non-affected) and haven't had any issues.

@mikitsu mikitsu force-pushed the dont-overlap-subuids branch 2 times, most recently from 5183a94 to 5c3ecb9 Compare April 13, 2025 18:58
@mikitsu
Copy link
Author

mikitsu commented Apr 13, 2025

I've added the warning without the system.stateVersion gate, as it only triggers when a subuid range actually changes, which shouldn't happen on new systems (and exactly once on old systems).

Are you OK with the text?

@leona-ya leona-ya force-pushed the dont-overlap-subuids branch from 5c3ecb9 to 293018e Compare April 21, 2025 18:10
@github-actions github-actions bot added 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation labels Apr 21, 2025
@leona-ya leona-ya force-pushed the dont-overlap-subuids branch from 293018e to 73925bf Compare April 21, 2025 18:12
@leona-ya
Copy link
Member

I rebased this PR and added a release notes entry.

Copy link
Member

@leona-ya leona-ya left a comment

Choose a reason for hiding this comment

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

This looks good to me and I also tried this on an affected machine. I added a release notes entry for more clarity.

mikitsu and others added 3 commits April 21, 2025 21:09
…SubUidGidRange

Previously, when adding a new user when at least two users already exist,
this new user was assigned the same subuid range as the second existing user.
@leona-ya leona-ya marked this pull request as draft April 21, 2025 19:09
@leona-ya leona-ya force-pushed the dont-overlap-subuids branch from 73925bf to ee4fc8a Compare April 21, 2025 19:10
@leona-ya leona-ya changed the base branch from master to staging April 21, 2025 19:10
@leona-ya leona-ya marked this pull request as ready for review April 21, 2025 19:10
@leona-ya
Copy link
Member

As discussed on matrix: moving this via staging to avoid hurting hydra even more with that many nixos tests changed.

Also thank you very much for the contribution!

@leona-ya leona-ya merged commit d16472b into NixOS:staging Apr 21, 2025
39 of 42 checks passed
to review the new defaults and description of
[](#opt-services.nextcloud.poolSettings).

- In `users.users` allocation on systems with multiple users it could happen that collided with others. Now these users get new subuid ranges assigned. When this happens, a warning is issued on the first activation. If the subuids were used (e.g. with rootless container managers like podman), please change the ownership of affected files accordingly.
Copy link
Contributor

Choose a reason for hiding this comment

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

@leona-ya I feel like a few words are missing in "it could happen that collided with others" – should this say something like "it could happen that some users' allocated subuid ranges collided with others"? (I am trying to keep up with release notes and did not at first understand what this was trying to say)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you are right. A better sentence should be

In users.users subuid allocation on systems with multiple users it could happen that some users' allocated subuid ranges collided with others.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened #408018 – thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1.severity: security Issues which raise a security issue, or PRs that fix one 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants