fix: move pam configuration to sudo_local #1020

dlubawy · 2024-07-27T04:28:12Z

Addresses #985 and #787.

Moves pam configuration to /etc/pam.d/sudo_local
Performs configuration through environment.etc
Adds a pam_reattach option to fix sudo TouchID in tmux

Implementation uses environment.etc to create the /etc/pam.d/sudo_local file and adds the pkgs.pam-reattach option provided in #662. Follows the comment by @emilazy to have nix-darwin manage the file entirely. If the file exists already, nix-darwin should handle it through the usual warning telling the user to rename the file to sudo_local.before-nix-darwin. As identified by @lilyball in their comment, the symlink approach here shouldn't impact users since it doesn't touch the main /etc/pam.d/sudo file. As long as /etc/pam.d/sudo remains a regular file than this should work fine without disrupting sudo to apply nix-darwin configurations.

I recognize this may be a duplicate PR given all the other open issues/PRs, but I haven't seen movement on them in a while. Feel free to close this if it's unnecessary.

emilazy · 2024-07-27T11:41:13Z

Thanks! This is definitely the direction I want to go, but we need to keep support for macOS < 14. That means that ensuring the auth include sudo_local line is added to /etc/pam.d/sudo if it’s not present (and cleaning up after the old # nix-darwin: security.pam.enableSudoTouchIdAuth method). So we still need to do some amount of file wrangling here, but the upside is that users of Sonoma and higher won’t have to re‐apply their configuration after every update.

dlubawy · 2024-07-27T19:51:24Z

Thanks! This is definitely the direction I want to go, but we need to keep support for macOS < 14. That means that ensuring the auth include sudo_local line is added to /etc/pam.d/sudo if it’s not present (and cleaning up after the old # nix-darwin: security.pam.enableSudoTouchIdAuth method). So we still need to do some amount of file wrangling here, but the upside is that users of Sonoma and higher won’t have to re‐apply their configuration after every update.

Good call on the support for older versions and older nix-darwin support. What do you think of the recent changes? I added back an activation script like the old one which will check for the old implementation and delete it using sed -i '/security.pam.enableSudoTouchIdAuth/d'. It will then grep to check if sudo_local is already in the /etc/pam.d/sudo file and add this line using sed -i '2i.... if any option in security.pam is set:

auth       include        sudo_local # nix-darwin: security.pam

Otherwise, if all security.pam options are disabled, then it performs sed -i '/security.pam/d' /etc/pam.d/sudo which should also act to disable it for older implementations as well. Then if enabled again they will just have the single line for including sudo_local.

For users on macOS >=14, the entire thing will just be managed through environment.etc as the /etc/pam.d/sudo file will already include sudo_local. Hopefully, there will be a point where we can remove the activation script entirely as older version support is eventually dropped.

Enzime

Sorry, it took so long to getting around to reviewing this PR

Let us know if you're still interested on working on this PR

Enzime · 2024-10-25T01:12:07Z

modules/security/pam.nix

+  {
+    environment.etc."pam.d/sudo_local" = {
+      enable = isPamEnabled;
+      text = lib.strings.concatStringsSep "\n" [


Suggested change

text = lib.strings.concatStringsSep "\n" [

text = lib.concatLines [

Enzime · 2024-10-25T01:13:39Z

modules/security/pam.nix

+        (lib.optionalString cfg.enablePamReattach "auth       optional       ${pkgs.pam-reattach}/lib/pam/pam_reattach.so")
+        (lib.optionalString cfg.enableSudoTouchIdAuth "auth       sufficient     pam_tid.so")


Suggested change

(lib.optionalString cfg.enablePamReattach "auth optional ${pkgs.pam-reattach}/lib/pam/pam_reattach.so")

(lib.optionalString cfg.enableSudoTouchIdAuth "auth sufficient pam_tid.so")

(lib.mkIf cfg.enablePamReattach "auth optional ${pkgs.pam-reattach}/lib/pam/pam_reattach.so")

(lib.mkIf cfg.enableSudoTouchIdAuth "auth sufficient pam_tid.so")

I think this will mean the output file won't have blank lines in it

Enzime · 2024-10-25T01:15:00Z

modules/security/pam.nix

+      # NOTE: this can be removed at some point when support for older versions are dropped
+      # Always clear out older implementation if it exists
+      if grep '${deprecatedOption}' ${file} > /dev/null; then
+        ${sed} -i '/${option}/d' ${file}


Suggested change

${sed} -i '/${option}/d' ${file}

${sed} -i '/${deprecatedOption}/d' ${file}

Enzime · 2024-10-25T01:15:48Z

modules/security/pam.nix

-    file   = "/etc/pam.d/sudo";
-    option = "security.pam.enableSudoTouchIdAuth";
+    file = "/etc/pam.d/sudo";
+    option = "security.pam";


Suggested change

option = "security.pam";

marker = "security.pam.sudo_local";

Enzime · 2024-10-25T01:17:13Z

modules/security/pam.nix

+      fi
+      # Check if include line is needed (macOS < 14)
+      if ! grep 'sudo_local' ${file} > /dev/null; then
+        ${sed} -i '2iauth       include        sudo_local # nix-darwin: ${option}' ${file}


Suggested change

${sed} -i '2iauth include sudo_local # nix-darwin: ${option}' ${file}

${sed} -i '2iauth include sudo_local # nix-darwin: ${marker}' ${file}

Enzime · 2024-10-25T01:21:06Z

modules/security/pam.nix

+  # for the existance of the line `auth       include        sudo_local`. This is included
+  # in macOS Sonoma and later. If the line is not there already then `sed` will add it.
+  # In those cases, the line will include the name of the option root (`security.pam`),
+  # to make it easier to identify the line that should be deleted when the option is disabled.


It would be good to add a comment about what happens to the marker line in /etc/pam.d/sudo when upgrading to Sonoma

Enzime · 2024-10-25T01:22:16Z

modules/security/pam.nix

+  # in macOS Sonoma and later. If the line is not there already then `sed` will add it.
+  # In those cases, the line will include the name of the option root (`security.pam`),
+  # to make it easier to identify the line that should be deleted when the option is disabled.
+  mkIncludeSudoLocalScript = isEnabled:


Suggested change

mkIncludeSudoLocalScript = isEnabled:

This being a function seems unnecessary

Enzime · 2024-10-25T01:25:55Z

modules/security/pam.nix

-        ${sed} -i '2i\
-      auth       sufficient     pam_tid.so # nix-darwin: ${option}
-        ' ${file}
+      # NOTE: this can be removed at some point when support for older versions are dropped


Suggested change

# NOTE: this can be removed at some point when support for older versions are dropped

# REMOVEME when macOS 13 no longer supported

emilazy · 2024-10-25T01:28:23Z

I think we can skip the isPamEnabled and just unconditionally apply this stuff too, since it’s harmless to have an empty file.

I also think that we shouldn’t include a marker on the include line: we should just attempt to make older systems match Sonoma.

Enzime · 2024-10-25T01:38:06Z

The benefit of the marker would be that if a user uninstalled nix-darwin on a pre-Sonoma version we'd be able to restore it to closer to the original state, but either way is fine as auth include sudo_local still supports sudo_local not existing

As environment.etc.<file>.copy doesn't exist, that means /etc/pam.d/sudo_local will be a symlink to a path in the /nix/store, I wonder if this is going to lead to some race conditions with FDE

emilazy · 2024-10-25T01:40:54Z

I think not being able to use touch ID very early in the user session is an acceptable failure mode if that’s all it is.

dlubawy · 2024-10-28T04:14:33Z

@Enzime, thanks for taking the time to review. I should still be able to address the points you made and will request a re-review when I get that done.

dlubawy added 7 commits July 26, 2024 18:06

Fix pam module with sudo_local and add reattach for tmux

d3fbe2b

Change sed for pam enable to toggle comment instead

c209dc4

Ensure pkgs update to pam-reattach does not break the file

2fbd748

Have nix-darwin manage the sudo_local file through environment.etc

c45dfd4

Switch to optionalString for pam sudo_local setup

1fe9d71

Update docs

e741cad

Remove implementation note a cleanup description

4f2d4a0

dlubawy added 5 commits July 27, 2024 12:04

Add back support for older versions of macOS

393686a

Fix spelling mistake

1fee417

Use variable to enable etc also

d71ff34

Add lines to clear out old implementation

52992b3

Move where to delete the older implementation

83e5c8f

emilazy mentioned this pull request Oct 21, 2024

Update security.pam.enableSudoTouchIdAuth for Sonoma+ #1109

Open

Enzime mentioned this pull request Oct 23, 2024

fix: Use sudo_local instead of sudo when changing sudo settings #1116

Closed

Enzime requested changes Oct 25, 2024

View reviewed changes

Enzime reviewed Oct 25, 2024

View reviewed changes

emilazy mentioned this pull request Nov 2, 2024

pam_reattach support #1137

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: move pam configuration to sudo_local #1020

fix: move pam configuration to sudo_local #1020

dlubawy commented Jul 27, 2024 •

edited

Loading

emilazy commented Jul 27, 2024

dlubawy commented Jul 27, 2024 •

edited

Loading

Enzime left a comment

Enzime Oct 25, 2024

Enzime Oct 25, 2024

Enzime Oct 25, 2024

Enzime Oct 25, 2024

Enzime Oct 25, 2024

Enzime Oct 25, 2024

Enzime Oct 25, 2024

Enzime Oct 25, 2024

emilazy commented Oct 25, 2024 •

edited

Loading

Enzime commented Oct 25, 2024

emilazy commented Oct 25, 2024

dlubawy commented Oct 28, 2024

	text = lib.strings.concatStringsSep "\n" [
	text = lib.concatLines [

		(lib.optionalString cfg.enablePamReattach "auth optional ${pkgs.pam-reattach}/lib/pam/pam_reattach.so")
		(lib.optionalString cfg.enableSudoTouchIdAuth "auth sufficient pam_tid.so")

	${sed} -i '/${option}/d' ${file}
	${sed} -i '/${deprecatedOption}/d' ${file}

	${sed} -i '2iauth include sudo_local # nix-darwin: ${option}' ${file}
	${sed} -i '2iauth include sudo_local # nix-darwin: ${marker}' ${file}

	# NOTE: this can be removed at some point when support for older versions are dropped
	# REMOVEME when macOS 13 no longer supported

fix: move pam configuration to sudo_local #1020

Are you sure you want to change the base?

fix: move pam configuration to sudo_local #1020

Conversation

dlubawy commented Jul 27, 2024 • edited Loading

emilazy commented Jul 27, 2024

dlubawy commented Jul 27, 2024 • edited Loading

Enzime 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

emilazy commented Oct 25, 2024 • edited Loading

Enzime commented Oct 25, 2024

emilazy commented Oct 25, 2024

dlubawy commented Oct 28, 2024

dlubawy commented Jul 27, 2024 •

edited

Loading

dlubawy commented Jul 27, 2024 •

edited

Loading

emilazy commented Oct 25, 2024 •

edited

Loading