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.

Sign up for GitHub

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

enableSudoTouchIdAuth: use a separate PAM service #591

Closed
wants to merge 2 commits into from
Closed

enableSudoTouchIdAuth: use a separate PAM service #591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
681e667
enableSudoTouchIdAuth: use a separate PAM service
antifuchs Jan 4, 2023
eeea23e
Remove the pam script from the system activation script
antifuchs Jan 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 20 additions & 42 deletions modules/security/pam.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,59 +4,37 @@ with lib;

let
cfg = config.security.pam;

# Implementation Notes
#
# We don't use `environment.etc` because this would require that the user manually delete
# `/etc/pam.d/sudo` which seems unwise given that applying the nix-darwin configuration requires
# sudo. We also can't use `system.patchs` since it only runs once, and so won't patch in the
# changes again after OS updates (which remove modifications to this file).
#
# As such, we resort to line addition/deletion in place using `sed`. We add a comment to the
# added line that includes the name of the option, to make it easier to identify the line that
# should be deleted when the option is disabled.
mkSudoTouchIdAuthScript = isEnabled:
let
file = "/etc/pam.d/sudo";
option = "security.pam.enableSudoTouchIdAuth";
sed = "${pkgs.gnused}/bin/sed";
in ''
${if isEnabled then ''
# Enable sudo Touch ID authentication, if not already enabled
if ! grep 'pam_tid.so' ${file} > /dev/null; then
${sed} -i '2i\
auth sufficient pam_tid.so # nix-darwin: ${option}
' ${file}
fi
'' else ''
# Disable sudo Touch ID authentication, if added by nix-darwin
if grep '${option}' ${file} > /dev/null; then
${sed} -i '/${option}/d' ${file}
fi
''}
'';
in

{
options = {
security.pam.enableSudoTouchIdAuth = mkEnableOption ''
Enable sudo authentication with Touch ID

When enabled, this option adds the following line to /etc/pam.d/sudo:
When enabled, this option changes sudo to use a separate pam
service, sudo-touchid, which contains the entire "sudo" service's
authentication methods, plus the line:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we have to worry about Apple changing the default contents, and our copy running out of sync? (Say, they remove a module and its .so file from disk in a new macOS version, possibly breaking this setup.)

Copy link
Author

Choose a reason for hiding this comment

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

While I can't rule it out, the contents of this file have remained stable for I think 2 major OS releases now... it would be pretty surprising if that changed from under us, but yep, catastrophic. One thing that I'd love here is another option that allows customizing the remaining contents of the file, which would easily allow adding @lockejan's suggestion as well (:

Copy link
Contributor

@stephank stephank Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But then, customizing a file is no better than the old mechanism? Or did you have something different in mind?

I was thinking, the current mechanism to protect against this sort of thing is knownSha256Hashes, but that can only check the target file, and not another file. We could expand it to do that, making the two new links conditional on having a known /etc/pam.d/sudo. But unlike knownSha256Hashes, it'd also have to remove existing links if the hash fails, or we may still end up with a broken config after such a macOS upgrade.


auth sufficient pam_tid.so

(Note that macOS resets this file when doing a system update. As such, sudo
authentication with Touch ID won't work after a system update until the nix-darwin
configuration is reapplied.)
'';
};

config = {
system.activationScripts.pam.text = ''
# PAM settings
echo >&2 "setting up pam..."
${mkSudoTouchIdAuthScript cfg.enableSudoTouchIdAuth}
'';
config = lib.mkIf (cfg.enableSudoTouchIdAuth) {
environment.etc."sudoers.d/000-sudo-touchid" = {
text = ''
Defaults pam_service=sudo-touchid
Defaults pam_login_service=sudo-touchid
'';
};
environment.etc."pam.d/sudo-touchid" = {
text = ''
Copy link
Contributor

@lockejan lockejan Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Really cool. Already using this. If you are keen to make it even better, there's also pam-reattach available via nixpkgs.
This will make sudo touchID also available for tmux sessions.

It's just this line:
auth optional ${pkgs.pam-reattach}/lib/pam/pam_reattach.so ignore_ssh

The downside is, this way everyone would end up having it in their config, also non-tmux users.

Copy link

Choose a reason for hiding this comment

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

I would not want this to be the default. Perhaps as a follow up PAM modules could be made configurable, so those who do want it could set up up, or a module could then build off it.

auth sufficient pam_tid.so
auth sufficient pam_smartcard.so
auth required pam_opendirectory.so
account required pam_permit.so
password required pam_deny.so
session required pam_permit.so
'';
};
};
}
1 change: 0 additions & 1 deletion modules/system/activation-scripts.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ in
${cfg.activationScripts.groups.text}
${cfg.activationScripts.users.text}
${cfg.activationScripts.applications.text}
${cfg.activationScripts.pam.text}
${cfg.activationScripts.patches.text}
${cfg.activationScripts.etc.text}
${cfg.activationScripts.defaults.text}
Expand Down