Add option to enable sudo authentication with Touch ID #228
Conversation
malob
force-pushed
the
sudo-touchid
branch
2 times, most recently
from
0fa9f5b
to
1f2345c
Compare
|
Does this work outside of Terminal.app?? |
|
Yes. I use it with Kitty terminal. |
modules/system/sudo.nix
Outdated
|
|
||
| { | ||
| options = { | ||
| system.sudo.touchid.enable = mkEnableOption "Enable sudo authentication with Touch ID"; |
There was a problem hiding this comment.
NixOS uses security.sudo.* for options that manage sudoers, etc. Also this is more a pam option.
There was a problem hiding this comment.
Good point. Looking at NixOS options that exist, I'm now thinking that something like security.pam.enableSudoTouchIdAuth would be better. What do you think?
There was a problem hiding this comment.
Pushed a new commit moving that that name for now.
malob
force-pushed
the
sudo-touchid
branch
2 times, most recently
from
ce00b53
to
2e2a6e0
Compare
modules/security/pam.nix
Outdated
| mkSudoTouchIdAuthScript = isEnabled: | ||
| let | ||
| file = "/etc/pam.d/sudo"; | ||
| option = "security.pam.enableSudoTouchIdAuth"; | ||
| 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 | ||
| ''} | ||
| ''; |
There was a problem hiding this comment.
@emilazy, here's my go at moving the activation script to using sed for both enable and disable this functionality, including experimenting with adding the options name in a comment as a unique identifier to make removing the relevant line a little more robust.
|
@LnL7, just wanted to check in and see if there was anything I could do to help get this merged in. |
|
I would also love for this to get merged as soon as possible! @LnL7 |
|
Gentle ping. Would love to see this merged so I don’t have to |
|
this looks like ready and tested, any reason not to merge it? |
|
maybe not the best solution, but added pam module to fix touch id under tmux: ivankovnatsky/nixos-config@6b3c746 |
|
Why not merge this? |
|
FYI, you should change the use of |
|
Can this option also handle pam_reattach which is also required to make sudo work in tmux? |
|
Here is an example config that uses both. |
|
question from a newbie to Is this expected behavior? |
|
Can reproduce. Simply removing the option does not remove the addition, you have to explicitly set it to false. This probably is not expected behavior. |
|
@dsyang, @ethancedwards8, huh that is surprising to me, you are right that this is not the expected behavior, though I'm not sure what the cause would be. I tested this on my system (where I've got a copy of the module setup), and was not able to reproduce. Pretty sure the only difference between the two is that in the copy in my repo I'm setting I don't see why that would change anything though. The Any ideas? |
|
(Just rebased on |
|
I just tested this module directly by changing the With the option not specified in my config the activation script is set with the right content, and after running a rebuild and switch, the relevant line from |
|
Hmm interesting. Could this be related to flakes? My setup did not use nix-flakes. |
|
@malob I'm setting up a new computer today and idk what has changed but I'm not hitting the same bug anymore! My setup can be found at https://github.com/dsyang/nixfiles. The patch there doesn't have the new |
|
confirmed patching in the |
|
Tested and also works great for me! |
|
@domenkozar, think there's a chance we could get this merged? Any changes you'd like to see before doing so? |
|
What about devices without touchId? 🤔 |
Yes, I've stumbled on something similar when configuring a new mac (but I have a bit more complex config), so you can always enable super user and edit the sudoers file as desired to fix this. |
|
It's probably to unsafe in it's current state. See also #358 (comment) |
|
I expect |
You're probably right. At least that's the case on my machines, when my lid is closed and I use an external keyboard. However, it would be good to ensure the same goes for older machines not having any fingerprint sensors at all. |
|
For what it's worth, it is completely transparent on macOS 12 VM. It directly asks for the password. |
Co-authored-by: Peter Esselius <esselius@users.noreply.github.com>
|
(Rebased on |
|
Just wanted to chime in here with a couple of points. First off, let me say that I'd love to see this merged! However, in my case I'm using I think building the But it would be nice to also see an option for "additionalLines` or something. For example, I could image this to look something like: {
security.pam.enableSudoTouchIdAuth = true;
security.pam.additionalLines = [
"auth optional pam_reattach.so"
];
}That would be super helpful! |
|
I've extended it to handle pam_reattach https://github.com/DylanRJohnston/nixos/blob/main/common/nix-darwin/touchID.nix with pam_reattach being built as an overlay here https://github.com/DylanRJohnston/nixos/blob/main/packages/pam_reattach.nix |
See LnL7/nix-darwin#228 Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
|
@domenkozar any chance you could take a look at this PR? I've been using it and it's been working quite well for me I've also just tested it on my M1 Mac Mini without TouchID and it falls back to password seamlessly :) |
|
❤️ |
| # 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). |
There was a problem hiding this comment.
What's this about system.patches? I haven't used it before but reading through its activation script the logic appears to be "apply the patch unless it's already applied" (where "already applied" is defined as "it can be successfully reversed"). The part that looks up the previously-applied patches is just to reverse any patches that have been removed.
Which is to say, it certainly appears as though it will attempt to repatch on every activation.
There was a problem hiding this comment.
💯
system.patches is definitely the best and safest option. It will always get reapplied unless the existing contents are "unexpected" (ie. the patches don't apply cleanly).
There was a problem hiding this comment.
In fact, using system.patches to modify pam is something that the example config alludes to here:
nix-darwin/modules/examples/lnl.nix
Line 6 in f55568e
There was a problem hiding this comment.
It turns out, system.patches is actually broken. This PR should fix it: #612
There was a problem hiding this comment.
With #612 applied to my local setup, I am currently using this configuration to enable touch ID:
# Enable sudo authentication with Touch ID
system.patches = [
(pkgs.writeText "pam.patch" ''
--- a/etc/pam.d/sudo
+++ b/etc/pam.d/sudo
@@ -1,4 +1,6 @@
# sudo: auth account password session
+auth optional ${pkgs.pam-reattach}/lib/pam/pam_reattach.so # Needed for using Touch ID within tmux
+auth sufficient pam_tid.so
auth sufficient pam_smartcard.so
auth required pam_opendirectory.so
account required pam_permit.so
'')
];
This works pretty well and gets applied on every update. It is quite resilient and will continue to work even if I make changes to the patch (ex. changing the tmux related comment). This is because system.patches will reverse outdated patches before applying new ones.
There was a problem hiding this comment.
Given the potential issues with the scenario where the user edits /etc/pam.d/sudo by hand, I think system.patches is a better approach. As I already commented, the activation script for system.patches appears as though it should reapply the patch on every activation, which makes me wonder why this module claims it won't.
Alternatively you could go with environment.etc and just add your own activation script that runs before activationScripts.etc and, if the option is disabled, checks if /etc/pam.d/sudo is a symlink to /etc/static/pam.d/sudo and replaces it with /etc/pam.d/sudo.orig instead (throwing an error if the latter doesn't exist).
I can personally confirm that using a symlink for /etc/pam.d/sudo works perfectly fine, I've been doing this myself for a while.
| ${sed} -i '2i\ | ||
| auth sufficient pam_tid.so # nix-darwin: ${option} | ||
| ' ${file} |
There was a problem hiding this comment.
This sed here assumes that nobody else has edited this file. If e.g. I delete the first line comment, then it will insert the pam_tid.so after the existing pam_smartcard.so. Or if I delete the comment and pam_smartcard.so then it will insert it after the auto required pam_opendirectory.so, which will make it not work at all.
| '' else '' | ||
| # Disable sudo Touch ID authentication, if added by nix-darwin | ||
| if grep '${option}' ${file} > /dev/null; then | ||
| ${sed} -i '/${option}/d' ${file} |
There was a problem hiding this comment.
Similarly this line here assumes that pam_tid.so only exists in the auth sufficient pam_tid.so. I don't know if there's any reason for it to exist otherwise, but I might add e.g. a comment about it that would also be deleted.
| in '' | ||
| ${if isEnabled then '' | ||
| # Enable sudo Touch ID authentication, if not already enabled | ||
| if ! grep 'pam_tid.so' ${file} > /dev/null; then |
There was a problem hiding this comment.
This grep can also be tricked. For example, if I add a comment containing the string pam_tid.so that will cause nix-darwin to skip actually enabling it.
Of course, with the previous system.patches approach any such changes would make the feature not work at all, but at least it wouldn't make potentially surprising changes to the file.
| (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.) |
There was a problem hiding this comment.
Apologies for bumping this three year old PR, but I figured this is the best place to ask this.
Isn't this not actually true due to the system activation service being enabled by default, which will reapply the PAM edits upon boot?
Assuming I'm correct, I think that leaving this untrue (by default) clarification in the documentation will just confuse users.
There was a problem hiding this comment.
What you linked doesn't run the entire activation script, it just runs part of it
nix-darwin/modules/services/activate-system/default.nix
Lines 37 to 39 in eb2b9b6
I suppose this could have been updated to include the pam activation script too if someone thought about it. I actually didn't even realize this was here. Though at this point it's worth noting that #787 changes how all of this works.
There's a one line addition to
/etc/pam.d/sudothat allows Macs with Touch ID to use Touch ID to authenticatesudorequests. It's a really nice thing to have. Seem like a nice option to add tonix-darwin.I wasn't quite sure where to put the option, so definitely feel free to ask me to make changes to put it somewhere that makes more sense.
I choose to use
system.patchrather thanenvironment.etcsince I didn't want to require the user to have to delete the existing file potentially breakingsudoauthentication.