users: update properties for known users #1120

Enzime · 2024-10-24T16:16:09Z

I'm leaving gid and isHidden for later PRs

Don't allow changing home directory of current user and root

Enzime · 2024-10-25T05:21:51Z

@emilazy this is ready to review :)

modules/system/shells.nix

modules/users/default.nix

emilazy · 2024-10-25T11:47:28Z

modules/users/default.nix

+          # Always update all properties on users to keep them inline with configuration
+          dscl . -create ${dsclUser} PrimaryGroupID ${toString v.gid}
+          ${optionalString (v.description != null) "dscl . -create ${dsclUser} RealName ${lib.escapeShellArg v.description}"}
+          ${optionalString (v.shell != null) "dscl . -create ${dsclUser} UserShell ${lib.escapeShellArg (shellPath v.shell)}"}


Is dscl not deprecated for these tasks? I’m a little worried about having this long‐term split where we’re mixing sysadminctl and dscl when they might behave differently.

https://ss64.com/mac/sysadminctl.html

sysadminctl only deals with creating, deleting users, changing passwords and secure token settings, as far as I can tell updating settings is still dscl's responsibility

emilazy · 2024-10-25T11:48:35Z

modules/users/default.nix

+          ''}
+          # This command doesn't do anything if the folder already exists which should be the case
+          # for $SUDO_USER and root, so it should be safe to run unconditionally.
+          ${optionalString v.createHome "createhomedir -cu ${name}"}


I assume there’s no automated migration of files or anything, right? Do we want to be automating home directory changes, at least without a warning of some kind for this? We could split the difference by erroring out when the directory is wrong and printing commands to update it, which is an approach I like for avoiding automating “destructive” migrations while still making sure the activated configuration remains accurate.

I've changed the PR to only run createhomedir on user creation (as sysadminctl -addUser -home won't create the home directory), I've removed the logic that updates home and added a check to ensure the home directory set in the config matches the system for all users.

modules/users/user.nix

Enzime · 2024-10-26T03:57:37Z

@emilazy this should be ready to review again :)

The other change I've made is to move all the checking logic to system.checks.text to run first and avoid partial activations

Enzime · 2024-10-26T04:54:47Z

Turns out system.checks runs in user activation (presumably because it needs to run first) not root activation...

So I've updated my checks code to handle running from user activation level

Enzime · 2024-10-26T05:30:19Z

@emilazy should be good to review now :)

emilazy

Changes LGTM, thanks! Some comments but I’m happy for this to be merged as‐is.

emilazy · 2024-10-26T11:40:24Z

modules/system/activation-scripts.nix

@@ -86,6 +86,7 @@ in
      exit $_status
    '';

+    # FIXME: activationScripts.checks should be system level


Suggested change

# FIXME: activationScripts.checks should be system level

# FIXME: activationScripts should be system level

😆

emilazy · 2024-10-26T11:42:07Z

modules/users/default.nix


    users.gids = mkMerge gids;
    users.uids = mkMerge uids;

+    # NOTE: We put this in `system.checks` as we want this to run first to avoid partial activations
+    # however currently that runs at user level activation as that runs before system level activation
+    # TODO: replace `$USER` with `$SUDO_USER` when system.checks runs from system level


FWIW I plan to make sure $SUDO_USER doesn’t leak through to avoid activating‐user dependencies creeping back in when user activation is gone. But that’s okay since we can just use primaryUser or I guess check currently logged‐in users or something.

emilazy · 2024-10-26T11:43:45Z

modules/users/default.nix

+        fi
+
+        if [[ "$fullDiskAccess" != true ]]; then
+          printf >&2 '\e[1;31merror: users cannot be %s without Full Disk Access, aborting activation\e[0m\n' "$2"


Wasn’t there a separate setting to manage users and groups? I avoid granting Full Disk Access to my terminal so this feels a little unfortunate, but better to check than not.

BTW, if we work on moving things out of activation scripts to launchd daemons as NixOS is doing we might be able to avoid this.

(Edit: Sorry, I didn’t realize all this was already present when reviewing! I should have made these comments when it was added. None of this is a blocker, given that.)

After further investigation, there's a TCC service called kTCCServiceSystemPolicySysAdminFiles which shows the message: %s would like to administer your computer. Administration can include modifying passwords, networking, and system settings.

I believe this service is a subset of Full Disk Access which is the reason why granting FDA will not prompt you for this TCC service.

It turns out this permission gets triggered when you try to change a user's home directory as that can allow bypassing TCC (as TCC has a per-user database in a user's home directory) see: https://wojciechregula.blog/post/change-home-directory-and-bypass-tcc-aka-cve-2020-27937/

The easiest way to trigger this prompt:

$ sudo dscl . -change /Users/_nixbld1 NFSHomeDirectory /var/empty /var/empty

-change here is safe as it won't change the value unless the first argument matches the current value:

$ dscl . -read /Users/_nixbld1 NFSHomeDirectory NFSHomeDirectory: /var/empty $ sudo dscl . -change /Users/_nixbld1 NFSHomeDirectory /private/var/empty /tmp <main> attribute status: eDSAttributeNotFound <dscl_cmd> DS Error: -14134 (eDSAttributeNotFound) $ dscl . -read /Users/_nixbld1 NFSHomeDirectory NFSHomeDirectory: /var/empty

When running this command from a graphical session, you'll be able to see the prompt. However, if you run it from SSH, there will be no prompt and you'll get permission denied:

$ sudo dscl . -change /Users/_nixbld1 NFSHomeDirectory /var/empty <main> attribute status: eDSPermissionError <dscl_cmd> DS Error: -14120 (eDSPermissionError)

On the Remote Login modal, there's a checkbox Allow full disk access for remote users which is equivalent to toggling FDA for sshd-keygen-wrapper. I chose to suggest enabling it in the FDA menu just because it's easier to open:

$ open "x-apple.systempreferences:com.apple.preferences.sharing?Services_RemoteLogin" $ sleep 2 $ open "x-apple.systempreferences:com.apple.preferences.sharing?Services_RemoteLogin"

On newer versions of macOS, this deep link will work on the first try however on older versions it requires calling it twice with a sleep to make sure the Sharing page loaded before trying again.

With this information, I think we can change the checking logic to request kTCCServiceSystemPolicySysAdminFiles and then if that fails, give the user a relevant message:

If they're running over SSH, tell them to enable FDA for SSH or that they'll need to not use SSH with darwin-rebuild.
If they're running locally, tell them to wait 1 minute before running darwin-rebuild switch again and make sure to accept the prompt and possibly mention turning on FDA for their terminal emulator.

modules/users/default.nix

emilazy · 2024-10-26T11:45:56Z

modules/users/default.nix

+        name = lib.escapeShellArg v.name;
+        dsclUser = lib.escapeShellArg "/Users/${v.name}";
+      in ''
+        ${optionalString cfg.forceRecreate ''


We decided we could just drop this for now, right? Might be best to do now to avoid adding more logic we have to verify the correctness of when we’d like to kill it off.

I'll remove it in my next PR 👍

emilazy · 2024-10-26T11:46:30Z

modules/users/default.nix

+            if [[ ${lib.escapeShellArg v.home} != "$homeDirectory" ]]; then
+              printf >&2 '\e[1;31merror: config contains the wrong home directory for %s, aborting activation\e[0m\n' ${name}
+              printf >&2 'nix-darwin does not support changing the home directory of existing users.
+              printf >&2 '\n'
+              printf >&2 'Please set:\n'
+              printf >&2 '\n'
+              printf >&2 '    users.users.%s.home = "%s";\n' ${name} "$homeDirectory"
+              printf >&2 '\n'
+              printf >&2 'or remove it from your configuration.\n'
+              exit 1
+            fi


💜

We could print instructions on how to manually update it? Not necessary for an MVP though.

emilazy · 2024-10-26T11:49:05Z

modules/users/user.nix

-      type = types.either types.shellPackage types.path;
-      default = "/sbin/nologin";
+      type = types.nullOr (types.either types.shellPackage types.path);
+      default = "/usr/bin/false";


You can break your shell by starting to manage your user with nix-darwin unless you explicitly null this out, right? I was thinking we could just make null create users with /usr/bin/false but otherwise act the same way as the default. Not sure if that’s a good idea or not.

This is a good catch, I think this bug has been around since #818 and I'm not sure if anyone has been hit by it yet as it's currently gated behind users.knownUsers and my suspicion is that most users only add themselves to users.knownUsers when they want to update their shell as most other options are less useful.

I'll add the change to make null create users with /usr/bin/false and but still mean don't update this user's shell. This change should be superseded by adding a type option because then we could not pass the -shell flag to sysadminctl and it would set the correct shell based on whether -roleAccount was passed or not (alternatively we could determine the default passed on the type in nix-darwin as well)

Using `grep -v` without `-z` will return 0 even if there is a match found as all the non-matching lines will be matched. Instead of using `grep -vqz`, `(! grep ...)` is more readable. The brackets are necessary as `! grep` will not trigger `set -e`[0], so we run it inside a subshell to use its non-zero exit code. [0]: https://www.gnu.org/savannah-checkouts/gnu/bash/manual/bash.html#The-Set-Builtin

LnL7@2788e4f#diff-0642dcb4e551dcf07032904ee7f6b7ea645db36939f159908ccb2b85a2bbd1b8L53

Enzime · 2024-10-27T13:56:15Z

@emilazy this is ready for review :)

emilazy

Looks great, thank you for seeing this through! Feel free to merge.

My one remaining nit is that sudo dscl . -change /Users/nobody NFSHomeDirectory "$homeDirectory" "$homeDirectory" scares me a little and it’d be nice if we could just touch some meaningless NixTestWhatever field somewhere, maybe even not attached to a user at all. But I think it should probably be fine and I’m just being paranoid.

Enzime force-pushed the push-ltvtsllrqvqy branch from 346bf84 to 2b15b32 Compare October 25, 2024 05:18

emilazy requested changes Oct 25, 2024

View reviewed changes

Enzime force-pushed the push-ltvtsllrqvqy branch 2 times, most recently from 4a04398 to a9f2082 Compare October 26, 2024 03:45

Enzime force-pushed the push-ltvtsllrqvqy branch from a9f2082 to be98f2d Compare October 26, 2024 04:52

Enzime changed the title ~~users: update most properties for known users~~ users: update properties for known users Oct 26, 2024

Enzime force-pushed the push-ltvtsllrqvqy branch 2 times, most recently from 85f685b to 4b18b7b Compare October 26, 2024 05:25

emilazy previously approved these changes Oct 26, 2024

View reviewed changes

Enzime added 10 commits October 27, 2024 21:26

tests: fix old test getting messed up in refactor

13816f6

LnL7@2788e4f#diff-0642dcb4e551dcf07032904ee7f6b7ea645db36939f159908ccb2b85a2bbd1b8L53

users: update properties on known users

c9af5c2

users: allow home to be managed by macOS

bd161d6

users: change default shell to /usr/bin/false to match macOS

3712ff7

users: allow shell to be managed by macOS

dc6f754

users: move checks to system.checks

55be3e1

users: ensure all users' home directories in the config are correct

9cd3976

users: replace FDA check with more fine grained permissions check

32f0cf2

users: remove with lib;

febc3b3

Enzime dismissed emilazy’s stale review via febc3b3 October 27, 2024 13:40

Enzime force-pushed the push-ltvtsllrqvqy branch from 4b18b7b to febc3b3 Compare October 27, 2024 13:40

emilazy approved these changes Oct 27, 2024

View reviewed changes

Enzime merged commit 5c0c6aa into LnL7:master Oct 27, 2024
5 of 6 checks passed

Enzime deleted the push-ltvtsllrqvqy branch October 27, 2024 23:08

Enzime mentioned this pull request Oct 31, 2024

Can't set user shell without giving nix-darwin complete control over my user #328

Closed

This was referenced Oct 31, 2024

users.users.<name>.shell does not update the user's shell #811

Closed

environment: remove misleading environment.loginShell option #1135

Merged

ZSH does not export darwin in NIX_PATH #151

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

users: update properties for known users #1120

users: update properties for known users #1120

Enzime commented Oct 24, 2024 •

edited

Loading

Enzime commented Oct 25, 2024

emilazy Oct 25, 2024

Enzime Oct 25, 2024

emilazy Oct 25, 2024

Enzime Oct 26, 2024

Enzime commented Oct 26, 2024

Enzime commented Oct 26, 2024

Enzime commented Oct 26, 2024

emilazy left a comment

emilazy Oct 26, 2024

emilazy Oct 26, 2024

emilazy Oct 26, 2024

Enzime Oct 27, 2024

Enzime Oct 27, 2024

emilazy Oct 26, 2024

Enzime Oct 27, 2024

emilazy Oct 26, 2024

emilazy Oct 26, 2024

Enzime Oct 27, 2024

Enzime commented Oct 27, 2024

emilazy left a comment

	# FIXME: activationScripts.checks should be system level
	# FIXME: activationScripts should be system level

users: update properties for known users #1120

users: update properties for known users #1120

Conversation

Enzime commented Oct 24, 2024 • edited Loading

Enzime commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enzime commented Oct 26, 2024

Enzime commented Oct 26, 2024

Enzime commented Oct 26, 2024

emilazy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enzime commented Oct 27, 2024

emilazy left a comment

Choose a reason for hiding this comment

Enzime commented Oct 24, 2024 •

edited

Loading