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

fix: move pam configuration to sudo_local #1020

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

Conversation

dlubawy
Copy link

@dlubawy dlubawy commented Jul 27, 2024

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
Copy link
Collaborator

emilazy commented Jul 27, 2024

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
Copy link
Author

dlubawy commented Jul 27, 2024

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.

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

Successfully merging this pull request may close these issues.

None yet

2 participants