fix: move pam configuration to sudo_local
#1344
+62
−50
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,69 +1,81 @@ | ||
| { config, lib, pkgs, ... }: | ||
|
|
||
| let | ||
| cfg = config.security.pam; | ||
| in | ||
| { | ||
| options = { | ||
| security.pam = { | ||
| enable = lib.mkEnableOption "managing PAM with nix-darwin" // { | ||
| default = true; | ||
| example = false; | ||
| }; | ||
|
|
||
| enableSudoTouchIdAuth = lib.mkEnableOption "" // { | ||
|
There was a problem hiding this comment. Per the previous comment, since we’re adding more options and moving stuff around, this could be renamed to |
||
| description = '' | ||
| Whether to enable Touch ID with sudo. | ||
| This will also allow your Apple Watch to be used for sudo. If this doesn't work, | ||
| you can go into `System Settings > Touch ID & Password` and toggle the switch for | ||
| your Apple Watch. | ||
| ''; | ||
| }; | ||
|
|
||
| enableSudoPamReattach = lib.mkEnableOption "" // { | ||
|
There was a problem hiding this comment. Similarly, this could be introduced as |
||
| description = '' | ||
| Whether to enable reattaching a program to the user's bootstrap session. | ||
| This fixes Touch ID for sudo not working inside tmux and screen. | ||
| This allows programs like tmux and screen that run in the background to | ||
| survive across user sessions to work with PAM services that are tied to the | ||
| bootstrap session. | ||
| ''; | ||
| default = cfg.enableSudoTouchIdAuth; | ||
|
There was a problem hiding this comment. As I said in the previous PR I think we shouldn’t turn this on by default. It brings a third‐party PAM module into the |
||
| example = false; | ||
| }; | ||
| }; | ||
| }; | ||
|
|
||
| config = { | ||
| environment.etc."pam.d/sudo_local" = { | ||
| inherit (cfg) enable; | ||
| text = lib.concatLines ( | ||
| (lib.optional cfg.enableSudoPamReattach "auth optional ${pkgs.pam-reattach}/lib/pam/pam_reattach.so") | ||
| ++ (lib.optional cfg.enableSudoTouchIdAuth "auth sufficient pam_tid.so") | ||
|
Comment on lines
+44
to
+45
There was a problem hiding this comment. nit: I’d prefer to trim the unnecessary alignment whitespace here, but it doesn’t matter. |
||
| ); | ||
| }; | ||
|
|
||
| system.activationScripts.pam.text = | ||
| let | ||
| file = "/etc/pam.d/sudo"; | ||
| marker = "security.pam.sudo_local"; | ||
| deprecatedOption = "security.pam.enableSudoTouchIdAuth"; | ||
| sed = lib.getExe pkgs.gnused; | ||
| in | ||
| '' | ||
| # PAM settings | ||
| echo >&2 "setting up pam..." | ||
| # REMOVEME when macOS 13 no longer supported as macOS automatically | ||
| # nukes this file on system upgrade | ||
| # Always clear out older implementation if it is present | ||
| if grep '${deprecatedOption}' ${file} > /dev/null; then | ||
| ${sed} -i '/${deprecatedOption}/d' ${file} | ||
| fi | ||
| ${if cfg.enable then '' | ||
| # REMOVEME when macOS 13 no longer supported | ||
| # `sudo_local` is automatically included after macOS 14 | ||
| if ! grep 'sudo_local' ${file} > /dev/null; then | ||
| ${sed} -i '2iauth include sudo_local # nix-darwin: ${marker}' ${file} | ||
| fi | ||
| '' else '' | ||
| # Remove include line if we added it | ||
| if grep '${marker}' ${file} > /dev/null; then | ||
| ${sed} -i '/${marker}/d' ${file} | ||
| fi | ||
| ''} | ||
| ''; | ||
| }; | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it this is to accommodate users that want a custom
sudo_localfile?I’m wondering if this should be
security.pam.services.sudo_local.enable. That’s not an option that exists in NixOS, but it brings us closer to NixOS’s structure forsecurity.pam.*. We’re just managing thesudo_localconfiguration rather than PAM as a whole here.I might prefer something like @mirkolenz’s solution, which could also increase NixOS compatibility: we could have
security.pam.services.sudo_local.textwhich islib.types.linesand populate it with our settings by default; users who want extra configuration could uselib.mkAfter; users who want to replace the entire configuration could override it completely. If someone really wants a totally unmanagedsudo_localfile, we could makeenvironment.etc."pam.d/sudo_local".enablejust alib.mkDefaultso that they can disable it directly. (Or we could keep the non‐standard.enable, but I think in the presence of.textit would be niche enough that it’s fine to expect someone to toggle the file like that.)(if not for
.text, we could make itsecurity.pam.services.sudo.*since we’re effectively extending thesudoconfiguration. But if we add.text– which seems nicer than.enablehere – then we should make sure it’ssudo_local, assecurity.pam.services.sudo_local.textwill not be able to overwrite the entire PAMsudoservice configuration, as a NixOS user or configuration might expect.)