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/users-groups: move home dir creation to systemd tmpfiles #223932

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

Conversation

jsoo1
Copy link
Contributor

@jsoo1 jsoo1 commented Mar 30, 2023

Fixes #6481

When the home directory is on a separate mount the user home directories were not created.

Using systemd tmpfiles solves the race condition.

Description of changes
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/)
  • 23.05 Release Notes (or backporting 22.11 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.

@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 30, 2023
@jian-lin
Copy link
Contributor

I tried to solve the same issue in #204290 and @ElvishJerricco wrote some very helpful advice in #204290 (comment). He may be interested in reviewing this.

Copy link
Contributor

@ElvishJerricco ElvishJerricco left a comment

Choose a reason for hiding this comment

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

I don't think I like having one unit per user like this. Seems very cluttered.

I was looking at systemd-sysusers, hoping it had something for automatic home directory creation that we could use for inspiration, and the man page for sysusers.d says this:

systemd-sysusers only sets the home directory record in the user database. To actually create the directory, consider adding a corresponding tmpfiles.d(5) fragment.

I wonder if a tmpfiles thing is correct here? Sounds odd given the "tmp" in the name, but I think would get the job done.

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

jsoo1 commented Mar 30, 2023

I don't think I like having one unit per user like this. Seems very cluttered.

I was looking at systemd-sysusers, hoping it had something for automatic home directory creation that we could use for inspiration, and the man page for sysusers.d says this:

systemd-sysusers only sets the home directory record in the user database. To actually create the directory, consider adding a corresponding tmpfiles.d(5) fragment.

I wonder if a tmpfiles thing is correct here? Sounds odd given the "tmp" in the name, but I think would get the job done.

Ah right. tmpfiles was mentioned in the issue. I am not sure how that would be accomplished. Do you have an example to point me to?

edit: after some perusal of the manpage, I am also concerned about the "volatile" description of the files meant to be managed by tmpfiles.d.

@jsoo1
Copy link
Contributor Author

jsoo1 commented Mar 30, 2023

... but it seems like tmpfiles.d should do the trick. Pushed the change @ElvishJerricco

@github-actions github-actions bot added 8.has: changelog 8.has: documentation This PR adds or changes documentation labels Mar 30, 2023
@jsoo1 jsoo1 changed the title nixos/users-groups: move home dir creation to systemd services nixos/users-groups: move home dir creation to systemd tmpfiles Mar 30, 2023
Fixes NixOS#6481

When the home directory is on a separate mount the user home
directories were not created.

Using systemd tmpfiles solves the race condition.
@ElvishJerricco
Copy link
Contributor

ElvishJerricco commented Mar 30, 2023

Well that's certainly much simpler :) Though, I'm intrigued by the ordering relationships on systemd-tmpfiles-setup.service.

DefaultDependencies=no
After=local-fs.target systemd-sysusers.service systemd-journald.service
Before=sysinit.target

So it will wait for all fstab FSes to be mounted, but is still early enough to be before sysinit.target. Are these semantics good enough for NixOS? I wonder if there's any services that also have DefaultDependencies=no and a user with a home directory that needs creation... I doubt it. A service really shouldn't assume that things like home directories are available before at least sysinit.target is reached, if not basic.target or even multi-user.target.

@jsoo1
Copy link
Contributor Author

jsoo1 commented Mar 30, 2023

Well that's certainly much simpler :) Though, I'm intrigued by the ordering relationships on systemd-tmpfiles-setup.service.

DefaultDependencies=no
After=local-fs.target systemd-sysusers.service systemd-journald.service
Before=sysinit.target

So it will wait for all fstab FSes to be mounted, but is still early enough to be before sysinit.target. Are these semantics good enough for NixOS? I wonder if there's any services that also have DefaultDependencies=no and a user with a home directory that needs creation... I doubt it. A service really shouldn't assume that things like home directories are available before at least sysinit.target is reached, if not basic.target or even multi-user.target.

True. A quick grep also says that no service with DefaultDependencies = false and a user with home directories.

@jsoo1
Copy link
Contributor Author

jsoo1 commented Mar 30, 2023

Another question: does local-fs.target complete after network file systems? What if a home directory is on nfs?

ElvishJerricco
ElvishJerricco previously approved these changes Mar 30, 2023
Copy link
Contributor

@ElvishJerricco ElvishJerricco left a comment

Choose a reason for hiding this comment

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

That's great then! Assuming we can find some tests to pass (or make one), I like the look of this! Solves a noticeable problem that if there's a mountpoint between / and $HOME, the activation script doesn't create the directory correctly.

As a tangential addendum, I wonder if we could have the activation script generate a systemd-sysusers config file instead of doing what it does. I dunno if that would actually have much benefit, but it'd be interesting to look into.

@ElvishJerricco
Copy link
Contributor

Another question: does local-fs.target complete after network file systems? What if a home directory is on nfs?

Oh that's an interesting question. It looks like remote-fs.target is ordered before only multi-user.target in man bootup.... and it looks like NixOS doesn't even honor that:

$ systemctl list-dependencies --before remote-fs.target
remote-fs.target
○ ├─libvirtd.service
● ├─systemd-user-sessions.service
○ ├─virtlxcd.service
○ ├─virtqemud.service
○ └─virtvboxd.service

@jsoo1
Copy link
Contributor Author

jsoo1 commented Mar 30, 2023

Seems like nfs could be a problem: systemd/systemd#1959

@ElvishJerricco
Copy link
Contributor

I think that specific issue is not exactly related, but I think there is a sort of issue. That seems to be more of an issue with automount home directories, which is another thing to consider.

systemd will take care of it in most cases, because of the implicit ordering of mount units and because it does create intermediate directories when needed, but here's the problem that could come up: systemd-tmpfiles-setup.service can run before a remote FS in the user's $HOME path is mounted. This can cause two things to happen.

  1. If the remote FS is mounted at $HOME directly, systemd-tmpfiles-setup.service will just make that directory exist beforehand, which is fine, although homeMode won't be set correctly once the FS is mounted.
  2. If the remote FS is a parent directory of $HOME, systemd-tmpfiles-setup.service will make the home directory in the mountpoint directory and not in the FS, and then when the remote FS is mounted, it won't have the home directory in it.

@ElvishJerricco ElvishJerricco self-requested a review March 30, 2023 22:43
@jsoo1
Copy link
Contributor Author

jsoo1 commented Mar 30, 2023

I think that specific issue is not exactly related, but I think there is a sort of issue. That seems to be more of an issue with automount home directories, which is another thing to consider.

systemd will take care of it in most cases, because of the implicit ordering of mount units and because it does create intermediate directories when needed, but here's the problem that could come up: systemd-tmpfiles-setup.service can run before a remote FS in the user's $HOME path is mounted. This can cause two things to happen.

1. If the remote FS is mounted at `$HOME` directly, `systemd-tmpfiles-setup.service` will just make that directory exist beforehand, which is fine, although `homeMode` won't be set correctly once the FS is mounted.

2. If the remote FS is a parent directory of `$HOME`, `systemd-tmpfiles-setup.service` will make the home directory _in the mountpoint directory_ and not in the FS, and then when the remote FS is mounted, it won't have the home directory in it.

I see... 2 is I think the original root cause of the issue and 1 also seems a problem, but less egregious.

Perhaps the oneshot services are the best way to go, here... what do you think?

@ElvishJerricco ElvishJerricco dismissed their stale review March 30, 2023 22:46

New issues found

@ElvishJerricco
Copy link
Contributor

FWIW, this is still objectively an improvement over the existing code. This problem also existed, but more severely, before this change.

@jsoo1
Copy link
Contributor Author

jsoo1 commented Mar 30, 2023

Hm. Would oneshot services fix both the automount and nfs problems @ElvishJerricco ?

@ElvishJerricco
Copy link
Contributor

Well, one thing that concerned me was that neither solution would recreate user home directories if they were deleted and then a system config was activated. So I added this to test it out, expecting for it to fail with this PR but succeed without it:

diff --git a/nixos/tests/mutable-users.nix b/nixos/tests/mutable-users.nix
index ebe32e6487e..d9025295934 100644
--- a/nixos/tests/mutable-users.nix
+++ b/nixos/tests/mutable-users.nix
@@ -69,5 +69,9 @@ import ./make-test-python.nix ({ pkgs, ...} : {
         for file in files_to_check:
             assert machine.succeed(f"sha256sum {file}") == expected_hashes[file]
             assert machine.succeed(f"stat {file}") == expected_stats[file]
+
+    with subtest("activate does change things"):
+        machine.succeed("/run/current-system/bin/switch-to-configuration test")
+        machine.succeed('test -e /home/dry-test') # home WAS recreated
   '';
 })

Above this diff, the test removes the home directory so that it can make sure dry-activate doesn't recreate it. To my surprise, it succeed even with this PR. Turns out switch-to-configuration has logic for tmpfiles which automatically covers it with the tmpfiles solution. I think we'd have to add a bunch of logic if we wanted to do this with oneshots.

Between that, the fact that creating a oneshot per user seems excessive, and the fact that this problem has always existed for users of remote-fs homes, I think the tmpfiles solution is probably the right way to go.

I would also recommend adding that diff to this PR. Once that's done I'll ask ofborg to tests mutableUsers and user-home-mode, and we'll be looking pretty good.

@jsoo1
Copy link
Contributor Author

jsoo1 commented Mar 31, 2023

Adding one formatting commit along with your patch. Thank you very much @ElvishJerricco !

@ElvishJerricco
Copy link
Contributor

@ofborg test mutableUsers user-home-mode

ElvishJerricco
ElvishJerricco previously approved these changes Apr 1, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/2018

systemd.tmpfiles.rules = lib.concatLists (lib.mapAttrsToList
(_: user:
lib.optionals user.createHome [
"d ${lib.escapeShellArg user.home} ${user.homeMode} ${user.name} ${user.group}"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is incorrect. tmpfile paths do not respect shell escaping rules, in fact the escaping rules documented in the manpage are not even correct (systemd/systemd#26955). until that is sorted out we'd better be very careful with using tmpfiles to create arbitrary paths. user names themselves are fine, but there is no restriction on the parent directory. until systemd is fixed to actually follow its escaping rules we had better leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok good call, thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a library function to c-escape a string that you know of, on that topic?

Copy link
Contributor

Choose a reason for hiding this comment

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

don't think so, almost certainly not by the rules systemd requires (tmpfiles also processes %<x> directives, so those need escaped as well)

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 see. Well it's probably not impossible...

Copy link
Contributor

Choose a reason for hiding this comment

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

Well crap. Lets hold off on this until we have a solution or at least an answer to that bug. I'll be looking into it.

@ElvishJerricco ElvishJerricco dismissed their stale review April 2, 2023 04:30

tmpfiles bug could break things

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/users-users-name-createhome-not-creating-home-directory/30779/4

@CarlHMitchell
Copy link

Looks like that systemd bug is fixed at least as of Systemd v254. This PR merged & fixed it. Nixpkgs 23.11 includes that version of Systemd, so this should be unblocked.

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

mutable-users.nix and user-home-mounts.nix tests do pass, is there anything else here in need of fixup before being merged?

Comment on lines +365 to +366
- `users.users.<name>.home` directories are created with systemd tmpfiles rules instead of activation scripts. This fixes a bug where home directories were not created when home directories were on a separate mount. (See issue [#6481](https://github.com/NixOS/nixpkgs/issues/6481))

Copy link
Member

Choose a reason for hiding this comment

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

Should be moved to new release notes.

@ElvishJerricco
Copy link
Contributor

ElvishJerricco commented Mar 1, 2024

@jsoo1 Can you rebase on unstable? Personally, I'd prefer to see the PR without the reformat to make it easier to review. I think there are plans to auto-format all of nixpkgs once the nixfmt RFC 166 lands.

@nikstur Is there anything about this PR that would conflict with or need adaptation for the merge of #270727 and its very similar approach to the home directory?

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@nyabinary
Copy link
Contributor

@jsoo1 Can you rebase on unstable? Personally, I'd prefer to see the PR without the reformat to make it easier to review. I think there are plans to auto-format all of nixpkgs once the nixfmt RFC 166 lands.

@nikstur Is there anything about this PR that would conflict with or need adaptation for the merge of #270727 and its very similar approach to the home directory?

Think it would be better to reopen clone this & rebase it and then open another PR since it doesn't seem the author is responsive,

@jsoo1
Copy link
Contributor Author

jsoo1 commented Apr 2, 2024

Sorry I am listening but I don't have bandwidth for this. @ixmatus and @bacchanalia can you help please?

@nyabinary
Copy link
Contributor

bump
cc @ixmatus @bacchanalia

@ixmatus
Copy link
Contributor

ixmatus commented Jun 11, 2024

It might be some time before I have the personal bandwidth to push this forward. If I have some free-time in $JOB to push this along I'll try, maybe @bacchanalia and I can try working on it together a bit.

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
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 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 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: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating home happens before /home is mounted
10 participants