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

Don't default to nogroup for the primary group of users. #133166

Merged
merged 4 commits into from
Sep 13, 2021

Conversation

symphorien
Copy link
Member

Motivation for this change

This is unsafe. #130649 shows that most objections raised are about the UX for modules that need adaption so I added a better error message. I also fixed a lot of modules in nixpkgs. Now no nixos test fails to evaluate because of this change.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.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.

@symphorien symphorien requested a review from peti as a code owner August 8, 2021 17:17
@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 This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 8, 2021
@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 Aug 8, 2021
@symphorien symphorien force-pushed the nonogroup branch 2 times, most recently from 7aca6ed to 18f7d56 Compare August 8, 2021 17:49
@@ -486,19 +486,19 @@ in
mopidy = 130;
docker = 131;
gdm = 132;
#dhcpcd = 133; # unused
dhcpd = 133;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess changing the name here was intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

The user with the same uid is dhcpd, so I think the c in the comment was a typo.

@@ -402,14 +402,14 @@ in
fourstore = 42;
fourstorehttp = 43;
virtuoso = 44;
#rtkit = 45; # unused
rtkit = 45;
Copy link
Member

Choose a reason for hiding this comment

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

Why are these being revived?

Copy link
Member Author

Choose a reason for hiding this comment

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

because currently, user rtkit has a static uid and group nogroup. The static uid is there so that files created by this daemon have a constant numerical owner, so the group rtkit I create in this PR must also have a static gid.

It might be the case that neither the user nor the group need a static id at all, but I think this is out of scope of the PR.

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure we can let all of these just be allocated dynamically, which we should do according to https://github.com/NixOS/rfcs/blob/master/rfcs/0052-dynamic-ids.md. You can do that by just not setting a gid in the group definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

My point is merely that a static uid is required iff a static gid is required. So I'm not the one to make the decision to remove the static uid. The maintainers of each module should.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think a a static uids and gids are linked together? Or where do you see that? Is there an error if you don't assign a static gid?

Copy link
Member Author

Choose a reason for hiding this comment

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

we fix uids because filesystems retain ownership by uid, not user name (as attested by the header of ids.nix, which mentions services with many files). This rationale applies equally to group ownership.

Copy link
Member Author

Choose a reason for hiding this comment

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

does that look reasonable to you, @infinisil?

@bjornfor
Copy link
Contributor

bjornfor commented Aug 8, 2021

That last commit seems to be fixups that can be squashed.

@r-rmcgibbo
Copy link

r-rmcgibbo commented Aug 8, 2021

Result of nixpkgs-review pr 133166 at a1225538 run on x86_64-linux 1

2 packages built successfully:
  • nixos-install-tools
  • tests.trivial

Result of nixpkgs-review pr 133166 at a1225538 run on aarch64-linux 1

1 package built successfully:
  • nixos-install-tools

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Aug 16, 2021

I'm facing a similar problem with #126289.
Basically security.wrappers defaults to create files with root:nogroup or nobody:something ownership and consequently many NixOS modules are potentially unsafe. What should I do with these: create new ad-hoc groups for each module or use root when unspecified?

@symphorien
Copy link
Member Author

as far as I understand, root group for setuid and root user for setgid should be best. These should not be written at all anyway, and they only work because they are world readable/executable.

this is unsafe, as many distinct services may be running as the same
nogroup group.
@symphorien
Copy link
Member Author

I fixed the merge conflict

description = "RealtimeKit daemon";
};
users.groups.rtkit.gid = config.ids.gids.rtkit;
Copy link
Member

@infinisil infinisil Sep 3, 2021

Choose a reason for hiding this comment

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

Re #133166 (comment), you can change this to

Suggested change
users.groups.rtkit.gid = config.ids.gids.rtkit;
users.groups.rtkit = {};

Which will work just as well as the current version of this PR, but without requiring a static gid (so the rtkit entry in ids.nix can be deleted). This is exactly what NixOS/rfcs#52 is about, to not use static uid/gid's, which this PR goes against in its current state.

But also like, this PR breaks file ownership either way: Previously the files were owned by nogroup, but now the service will be run by gid rtkit (whether it be dynamically allocated or statically).

Copy link
Member Author

Choose a reason for hiding this comment

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

ok but then I should also remove the static uid

Copy link
Member

Choose a reason for hiding this comment

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

Some more info: We discussed this on matrix. @symphorien made a good argument that whether we choose static or dynamic ids, it should be the same for uid and gid, because any reason to use either static or dynamic can equally be applied to both uid's and gid's.

I suggested to then make them both dynamic, in order to go towards the RFC's direction

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@symphorien
Copy link
Member Author

I think all reviews were addressed. I would like to merge next week unless someone speaks up.

@symphorien
Copy link
Member Author

@GrahamcOfBorg test gnome

@symphorien symphorien merged commit 3592034 into NixOS:master Sep 13, 2021
@infinisil
Copy link
Member

Looking good!

@Izorkin
Copy link
Contributor

Izorkin commented Sep 14, 2021

System not building with vsftpd enabled

building Nix...
building the system configuration...
error: while evaluating the attribute 'config.system.build.toplevel' at /home/user/works/src-nix/nixpkgs/nixos/modules/system/activation/top-level.nix:298:5:
while evaluating 'foldr' at /home/user/works/src-nix/nixpkgs/lib/lists.nix:52:20, called from /home/user/works/src-nix/nixpkgs/nixos/modules/system/activation/top-level.nix:133:12:
while evaluating 'fold'' at /home/user/works/src-nix/nixpkgs/lib/lists.nix:55:15, called from /home/user/works/src-nix/nixpkgs/lib/lists.nix:59:8:

Failed assertions:
- users.users.vsftpd.group is unset. This used to default to
nogroup, but this is unsafe. For example you can create a group
for this user with:
users.users.vsftpd.group = "vsftpd";
users.groups.vsftpd = {};

After add:

  users = {
    users.vsftpd.group = "vsftpd";
    groups.vsftpd = { };
  };

...
activating the configuration...
warning: user ‘systemd-coredump’ has unknown group ‘systemd-coredump’
setting up /etc...
...

@dschrempf dschrempf mentioned this pull request Sep 20, 2021
12 tasks
@rnhmjoj rnhmjoj mentioned this pull request Sep 21, 2021
89 tasks
rnhmjoj added a commit to rnhmjoj/nixpkgs that referenced this pull request Sep 22, 2021
Add a group after the removal of the nogroup default in NixOS#133166.
rnhmjoj added a commit to rnhmjoj/nixpkgs that referenced this pull request Sep 22, 2021
The group was actually there but I didn't add it by mistake.
This fixes the evaluation after for NixOS#133166.
rnhmjoj added a commit to rnhmjoj/nixpkgs that referenced this pull request Sep 22, 2021
This fixes the evaluation after for NixOS#133166.
rnhmjoj added a commit to rnhmjoj/nixpkgs that referenced this pull request Sep 22, 2021
rnhmjoj added a commit to rnhmjoj/nixpkgs that referenced this pull request Sep 22, 2021
This fixes the evaluation after for NixOS#133166.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants