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/update-users-groups: read access to /etc/shadow for group shadow #116644

Merged
merged 1 commit into from Mar 18, 2021

Conversation

jluttine
Copy link
Member

@jluttine jluttine commented Mar 17, 2021

Motivation for this change

#98676 set /etc/shadow group to shadow. However, the group wasn't given read-access to the file thus defeating the purpose of the PR. This PR now adds read-access to /etc/shadow for group shadow. This fix was accepted in the following comment: #98676 (comment)

cc: @cole-h

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 pkgs 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@jluttine jluttine changed the title nixos/update-users-groups: /etc/shadow read permission to group shadow nixos/update-users-groups: read access to /etc/shadow for group shadow Mar 17, 2021
Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

Diff LGTM.

This is also consistent with the Ubuntu docs in the issue linked in the PR that introduced this (https://wiki.debian.org/SystemGroups#Groups_without_an_associated_user): "shadow: /etc/shadow is readable by this group. Some programs that need to be able to access the file are SETGID shadow."

@cole-h
Copy link
Member

cole-h commented Mar 17, 2021

@ofborg test installer.simple

@jluttine
Copy link
Member Author

jluttine commented Mar 17, 2021

I tried to reply via email, but apparently it didn't work..

I was just thinking if the permissions should be 0440 instead of 0640. Or is there a reason to have the shadow file writable on NixOS?

Also, do you know how to properly set up nginx so that it has access to /etc/shadow file? It works if I set users.extraUsers.nginx.extraGroups = [ "shadow" ] but I understood that no user should be put to that group (I don't know why) and instead setgid should be used. I tried the following but it didn't work:

security.wrappers.nginx = {
  source = "${config.services.nginx.package}/bin/nginx";
  group = "shadow";
};

Nginx service didn't seem to have access to the shadow file with this config. (I suppose the wrapper is just added to /run/wrappers/bin/nginx and the nginx service doesn't use that. Do you know how it should be set?

EDIT: Ah, found some discussions about my latter question: https://discourse.nixos.org/t/nginx-pam-access-to-etc-shadow/6218/2

@cole-h
Copy link
Member

cole-h commented Mar 17, 2021

I'd modify the nginx service and add systemd.services.nginx.serviceConfig.SupplementaryGroups = [ "shadow" ]; -- that's how I do it for services that need my keys group.


FWIW, my Pihole installation has /etc/shadow as 0640 as well. (A fresh Arch ISO has 0400, but the file is owned by root:root.)

I don't think it really matters, since it's owned by root... root can do whatever it wants with that file even if it's r/o.

@cole-h cole-h merged commit 099a9e8 into NixOS:master Mar 18, 2021
@cole-h
Copy link
Member

cole-h commented Mar 18, 2021

Thanks!


If you can find some reasoning on why 0440 is more desirable over 0640, feel free to open a PR with that justification. But, at the moment, I think it's fine to leave it as 0640 (considering it's owned by root:shadow).

@jluttine
Copy link
Member Author

Thanks! I now tried:

systemd.services.nginx.serviceConfig.SupplementaryGroups = [ "shadow" ];

instead of

users.extraUsers.nginx.extraGroups = [ "shadow" ];

but with SupplementaryGroups config, nginx isn't able to read /etc/shadow whereas with extraGroups config it is. I can see the following line in the generated systemd service file for nginx:

...
SupplementaryGroups=shadow
...

I wonder why it doesn't work.. Any ideas?

@cole-h
Copy link
Member

cole-h commented Mar 18, 2021

And you restarted the nginx unit afterwards? I've seen services need to be manually restarted despite the service definition changing underneath it.

Also try plain old serviceConfig.Group = "shadow";.

If still no dice, I have no idea :(

@jluttine
Copy link
Member Author

Ok, thanks! Yeah, I did restart the nginx unit manually. But at least setting extraGroups for user nginx works..

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nginx-pam-access-to-etc-shadow/6218/7

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

Successfully merging this pull request may close these issues.

None yet

3 participants