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

enableSudoTouchIdAuth: use a separate PAM service #591

Closed
wants to merge 2 commits into from

Conversation

antifuchs
Copy link

@antifuchs antifuchs commented Jan 4, 2023

Currently, our patching /etc/sudoers doesn't persist across macOS upgrades, which is unfortunate, and also kinda non-nix-y: Ideally, the setting could be enabled or disabled by dropping in a file managed by nix.

Thankfully, that can be done: By setting a separate (non-default) PAM service for sudo, we can redirect PAM to use a separate (nix-managed) file for its auth strategies (of which pam_tid.so is one).

Credit for discovering this fantastic trick goes to @saleemrashid, who is an expert at PAM/sudo/macOS somehow! I couldn't have found this without the marvelous knowledge he unearthed.

Error scenarios

/nix is unavailable (symlinks are broken)

There are two symlinks in play, /etc/sudoers.d/000-touchid-sudo and /etc/pam.d/touchid-sudo. Both point into the nix store, which might fail to mount or similar. Here's the three error scenarios that can happen:

  • both broken: sudo uses the sudo service, which is untouched & prompt for a password.
  • /etc/sudoers.d/000-touchid-sudo broken: sudo uses the sudo service, which is untouched & prompt for a password.
  • /etc/pam.d/touchid-sudo broken: sudo can not authenticate, needs manual intervention / rebooting into rescue system.

I think most users will only ever run into the scenario where both symlinks are broken (e.g. failed /nix store mount). If they manage to break the pam.d file, that's a problem, but that is no different from before: If that file was edited by somebody and an error were introduced, it would break their system.

Upgrade path from nix-darwin with the security.pam.enableSudoTouchIdAuth setting enabled

Landing this PR will cause the existing edit to remain in /etc/pam.d/sudo, until the admin runs a macOS upgrade again, at which point it'll neatly start using the sudo-touchid facility again. I think that should be fine... and what's more, it'll allow us to add a setting for people to customize their sudo PAM behavior in the future?

Currently, our patching /etc/sudoers doesn't persist across macOS
upgrades, which is unfortunate, and also kinda non-nix-y: Ideally, the
setting could be enabled or disabled by dropping in a file managed by
nix.

Thankfully, that can be done: By setting a separate (non-default) PAM
service for sudo, we can redirect PAM to use a separate (nix-managed)
file for its auth strategies (of which pam_tid.so is one).
@antifuchs antifuchs force-pushed the sudo-touchid-without-hacks branch from 7d1e0a2 to 681e667 Compare January 4, 2023 19:54
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

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.

'';
};
environment.etc."pam.d/sudo-touchid" = {
text = ''
Copy link
Contributor

@lockejan lockejan Jan 4, 2023

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.

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.

@malob
Copy link
Contributor

malob commented Jan 11, 2023

As the author of the original hacky implementation, I'd definitely support improvements in the vein of the ones described here.

One thing that may be worth considering to reduce the chance of someones system ending up in a broken state, would be to copy the files from the store into their appropriate locations during activation instead of using environment.etc. It's definitely less Nix-y, but might be a better solution if it makes it significantly less likely to mess up someone's system. (I haven't thought this through carefully, so there could be many obvious issues. It would definitely be important to ensure files are also removed when appropriate.)

Also, if significant changes are going to be made here, maybe it's worth taking some inspiration from the security.pam module in NixOS?

For example, we could create security.pam.services.[name].* which would manage /etc/pam.d/nix-darwin-[name] files (I've added "nix-darwin" in front of "[name]" to avoid collisions with existing files created/updated by macOS), where, e.g.,

  • security.pam.services.sudo.* would configure the contents of /etc/pam.d/nix-darwin-sudo;
  • setting security.pam.services.sudo.touchidAuth = true would enable sudo authentication with Touch ID (i.e, replacing security.pam.enableSudoTouchIdAuth); and
  • security.pam.services.sudo.enable = true would have to be explicitly set for nix-darwin to take control of managing sudo PAM authentication (where enabling this option would create, or add to, say /etc/sudoers.d/nix-darwin to get everything working, maybe by setting security.sudo.* options.)

(I expect the above has a lot of issues/mistakes/etc., but I included it mostly to gesture at the type of thing I was thinking about, as opposed to endorse/suggest specific implementation details.)

@malob
Copy link
Contributor

malob commented Jan 11, 2023

I forgot to mention in my previous comment that, doing things in a way that looks something like what I sketched out above would also provide a nice clean way to add additional functionality/features in the future. For example, one could add support for pam-reattach (as suggested by @lockejan) by,

  • adding security.pam.reattach with enable and ignoreSsh options (where setting enable to true would add pkgs.pam-reattach to environment.systemPackages);
  • adding security.pam.services.sudo.reattach which would default to the value of security.pam.reattach.enable; and
  • making it so that if security.pam.reattach.enable and security.pam.services.sudo.reattach were both set to true, the appropriate line would be added to /etc/pam.d/nix-darwin-sudo (including the ignore_ssh option at the end, if security.pam.reattach.ignoreSsh was set to true).

@emilazy
Copy link
Collaborator

emilazy commented Jul 9, 2023

I would be in favour of this with @malob's suggested change to copy directly into /etc. sudo breaking if the Nix store volume is inaccessible is too great a risk. It would also be possible to read the PAM file the sudo one is being based on during activation to avoid the possibility of desync as discussed in #591 (comment).

@LnL7
Copy link
Owner

LnL7 commented Jul 9, 2023

Hmm, I thought the current implementation used system.patches with a few variations.

@emilazy
Copy link
Collaborator

emilazy commented Jul 9, 2023

The PAM file we patch now gets overwritten on every update, so Touch ID sudo breaks until you manually reactivate the configuration. It's not the end of the world, but it's annoying, and it kind of hints that Apple don't expect/want end-users to be editing the file. Copying it with modifications and pointing to that in sudoers.d should be more robust.

@maljub01
Copy link
Contributor

maljub01 commented Jul 9, 2023

Hmm, I thought the current implementation used system.patches with a few variations.

That's done by #662 which also adds pam reattach support, but hasn't been merged yet.

@Enzime
Copy link
Collaborator

Enzime commented Feb 20, 2025

Superseded by #1344

@Enzime Enzime closed this Feb 20, 2025
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.

9 participants