artginzburg / sudo-touchid

 Permanent TouchID support 👆 for `sudo`.
https://git.io/sudotouchid
Eclipse Public License 2.0
546 stars 15 forks source link

Service does not work after upgrade #8

Open stackptr opened 2 years ago

stackptr commented 2 years ago

This worked for me until some point recently, possibly due to an OS upgrade. I upgraded the package to see if that would fix things:

❯ brew install artginzburg/tap/sudo-touchid

Running `brew update --preinstall`...
==> Auto-updated Homebrew!
Updated 3 taps (homebrew/core, homebrew/cask and homebrew/services).
<snip>

sudo-touchid 0.2 is already installed but outdated (so it will be upgraded).
==> Downloading https://github.com/artginzburg/sudo-touchid/releases/download/0.3/sudo-touchid.sh
==> Downloading from https://objects.githubusercontent.com/github-production-release-asset-2e65be/389117398/ee
######################################################################## 100.0%
==> Upgrading artginzburg/tap/sudo-touchid
  0.2 -> 0.3 

==> Caveats
To restart artginzburg/tap/sudo-touchid after an upgrade:
  sudo brew services restart artginzburg/tap/sudo-touchid
Or, if you don't want/need a background service you can just run:
  /opt/homebrew/opt/sudo-touchid/bin/sudo-touchid
==> Summary
🍺  /opt/homebrew/Cellar/sudo-touchid/0.3: 5 files, 4.5KB, built in 1 second
==> Running `brew cleanup sudo-touchid`...
Disable this behaviour by setting HOMEBREW_NO_INSTALL_CLEANUP.
Hide these hints with HOMEBREW_NO_ENV_HINTS (see `man brew`).
Removing: /opt/homebrew/Cellar/sudo-touchid/0.2... (5 files, 3.4KB)
Warning: Directory not empty @ dir_s_rmdir - /opt/homebrew/Cellar/sudo-touchid/0.2

❯ sudo brew services start sudo-touchid

Password:
Warning: Taking root:admin ownership of some sudo-touchid paths:
  /opt/homebrew/Cellar/sudo-touchid/0.3/bin
  /opt/homebrew/Cellar/sudo-touchid/0.3/bin/sudo-touchid
  /opt/homebrew/opt/sudo-touchid
  /opt/homebrew/opt/sudo-touchid/bin
  /opt/homebrew/var/homebrew/linked/sudo-touchid
This will require manual removal of these paths using `sudo rm` on
brew upgrade/reinstall/uninstall.
/Library/LaunchDaemons/homebrew.mxcl.sudo-touchid.plist: service already bootstrapped
Bootstrap failed: 37: Operation already in progress
Error: Failure while executing; `/bin/launchctl bootstrap system /Library/LaunchDaemons/homebrew.mxcl.sudo-touchid.plist` exited with 37.

❯ sudo brew services stop sudo-touchid

Stopping `sudo-touchid`... (might take a while)
==> Successfully stopped `sudo-touchid` (label: homebrew.mxcl.sudo-touchid)

❯ sudo brew services start sudo-touchid
Warning: Taking root:admin ownership of some sudo-touchid paths:
  /opt/homebrew/Cellar/sudo-touchid/0.3/bin
  /opt/homebrew/Cellar/sudo-touchid/0.3/bin/sudo-touchid
  /opt/homebrew/opt/sudo-touchid
  /opt/homebrew/opt/sudo-touchid/bin
  /opt/homebrew/var/homebrew/linked/sudo-touchid
This will require manual removal of these paths using `sudo rm` on
brew upgrade/reinstall/uninstall.
==> Successfully started `sudo-touchid` (label: homebrew.mxcl.sudo-touchid)

After the above, sudo still requires my password. It appears the script has not changed the files in the expected ways:

❯ cat /etc/pam.d/sudo
# sudo: auth account password session
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

❯ cat /etc/pam.d/sudo.bak
2022/02/16 11:03:31 open /etc/pam.d/sudo.bak: no such file or directory

I tried uninstalling via brew (requiring manually removing /opt/homebrew/Cellar/sudo-touchid/{0.2,0.3}) but it's the same result.

Manually running /opt/homebrew/opt/sudo-touchid/bin/sudo-touchid fixes the issue.

artginzburg commented 2 years ago

To be clear, running sudo brew services start sudo-touchid just creates a daemon that runs sudo-touchid automatically on system start. So after a macOS upgrade, sudo can be used with TouchID again. The daemon also runs sudo-touchid right when it's started, so that the user doesn't have to restart macOS or run the command manually when we already know that the user's intention is to enable TouchID for sudo

What I'm not sure about is whether you're still experiencing the issue. If manually running sudo-touchid fixes the issue, but as we already know the service simply runs it automatically, does it mean that your issue is only related to the service itself? And if so, do any other brew services work after your OS upgrade?

stackptr commented 2 years ago

What I'm not sure about is whether you're still experiencing the issue. If manually running sudo-touchid fixes the issue, but as we already know the service simply runs it automatically, does it mean that your issue is only related to the service itself? And if so, do any other brew services work after your OS upgrade?

This is the only service I use through brew, so unfortunately I cannot isolate this as being an issue with the particular service or some broader issue in background services.

I updated recently to 12.2.1 and saw that sudo had again reverted to requiring text entry. sudo-touchid is still listed as a background service:

❯ brew services
Name         Status User File
sudo-touchid none   root 
unbound      none        

I again ran the command manually:

❯ /opt/homebrew/opt/sudo-touchid/bin/sudo-touchid
Password:
Created a backup file at /etc/pam.d/sudo.bak

... And this has restored the expected behavior.

stackptr commented 2 years ago

I see that com.user.sudo-touchid.plist defines a ProgramArguments invoking /usr/local/bin/sudo-touchid. I believe this fails on Apple Silicon because homebrew installs packages to a default prefix of /opt/homebrew. Therefore, resolving this issue entails writing a different value to this plist based on brew --prefix.

stackptr commented 2 years ago

Actually it seems like like the relevant plist would be generated in the formula, which is:

❯ cat /Library/LaunchDaemons/homebrew.mxcl.sudo-touchid.plist 
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
    <key>Label</key>
    <string>homebrew.mxcl.sudo-touchid</string>
    <key>ProgramArguments</key>
    <array>
        <string>/opt/homebrew/opt/sudo-touchid/bin/sudo-touchid</string>
    </array>
    <key>RunAtLoad</key>
    <true/>
</dict>
</plist>

That path is valid and points version 0.3 of this script.

artginzburg commented 2 years ago

@stackptr how is it going? I'm just interested whether the issue is resolved by Homebrew or we need to change something in the formula.

jrmann100 commented 2 years ago

@artginzburg, was this ever resolved? I am getting the same issue.

artginzburg commented 2 years ago

@jrmann100 not sure I get what's the issue. Is it that the launch daemon does not run the command right when it is started, but only at system load?

jrmann100 commented 2 years ago

It seems like my launch daemon was disabled after a macOS upgrade. It seems to have been re-bootstrapped after I manually ran the script as suggested above.

jrmann100 commented 2 years ago

Hard to isolate the situation though, will open a new issue if it happens again and let this one rest.

artginzburg commented 2 years ago

Just now realised I have the same issue. sudo brew services list gives status "stopped" for sudo-touchid, and I did not stop it. I'm assuming a macOS upgrade happened at weekends, but not sure entirely.

Maybe launch daemons are automatically stopped on system upgrade now. Or maybe it's Homebrew's logic. Need to check.

UPD: I think "stopped" as a status does not mean that the service is not active, but rather means that the specified command was executed and is not running right now. Which is expected since the command is not active, it just runs and quits. But I can confirm that sudo-touchid did not automatically run in my case.

newadventure079 commented 2 years ago

Mine isn't working after an upgrade either.

I've done multiple OS upgrades thru some of the Ventura betas and each time sudo -v prompts for my password. I then have to run sudo-touchid and then it says it made changes to the file and then after that the touchID prompt will happen

newadventure079 commented 2 years ago

I upgraded to 13.0.1 and sudo-touchid was not loaded and working. I had to manually run sudo-touchid and type in my password. Then I ran it again just for fun and it said [TouchID for sudo] seems to be enabled already

artginzburg commented 1 year ago

Tried the following property list on macOS Ventura. It was successful in executing sudo-touchid, but sudo-touchid then failed to execute the sed command. So, no luck yet.

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
    <key>Label</key>
    <string>org.artginzburg.sudoTouchIDDaemon</string>
    <key>MachServices</key>
    <dict>
        <key>org.artginzburg.sudoTouchIDDaemon</key>
        <true/>
    </dict>
    <key>Program</key>
    <string>/opt/homebrew/bin/sudo-touchid</string>
    <key>ProgramArguments</key>
    <array>
        <string>/opt/homebrew/bin/sudo-touchid</string>
    </array>
</dict>
</plist>

If anyone wants to try the same, here are the steps:

  1. I wrote the file to /Library/LaunchDaemons/org.artginzburg.sudoTouchIDDaemon.plist
  2. Changed its owner to system so that the next command can execute: sudo chown root /Library/LaunchDaemons/org.artginzburg.sudoTouchIDDaemon.plist
  3. Loaded the service: sudo launchctl load /Library/LaunchDaemons/org.artginzburg.sudoTouchIDDaemon.plist
  4. Enabled logging output to a file for the next run: sudo launchctl debug system/org.artginzburg.sudoTouchIDDaemon --stdout ~/Desktop/test-sudo-touchid.txt
  5. Forced the service to start: sudo launchctl kickstart system/org.artginzburg.sudoTouchIDDaemon

The output in the file was: [TouchID for sudo] failed to execute. Which is programmed to be output if the sed command fails to insert touch pam into sudo auth config. I tried logging what the sed command actually outputs to locate the problem, but failed to do so — the output file was just empty. Looks like I missed something important.

artginzburg commented 1 year ago

I tried outputting errors to a file by specifying service.error_log_path in the formula, installing it, and starting the service.

The error in the file was quite confusing:

sed: /etc/pam.d/sudo: Operation not permitted

I tried explicitly giving Write permissions to the wheel user, as that's the user seen in homebrew services list; that did not change anything about the error.

So, I'm guessing that macOS in the last versions prohibits any attempts to programmatically change the system files, even if the attempting script has root privileges. In other words, it seems to only allow changes directly initiated by the user. Which sounds great for safety reasons, and resembles e.g. Safari's behavior (every API that interacts with the system outside the very content of the page the user is on seems to only work via direct user initiative, like a click event). But well, would be good if there was a way to disable this protection.

artginzburg commented 1 year ago

Meanwhile, considering the message above, I propose the following:

We could make a new script store the current system version. Then it will run without root privileges on system startup and check the current major version. If it's different from the stored one, it will run open $(which sudo-touchid), which will open Terminal asking for a password and executing sudo-touchid.

This is far from automatic, but sounds like the simplest solution for now.

imgrant commented 1 year ago

I tried outputting errors to a file by specifying service.error_log_path in the formula, installing it, and starting the service.

The error in the file was quite confusing:

sed: /etc/pam.d/sudo: Operation not permitted

I tried explicitly giving Write permissions to the wheel user, as that's the user seen in homebrew services list; that did not change anything about the error.

So, I'm guessing that macOS in the last versions prohibits any attempts to programmatically change the system files, even if the attempting script has root privileges. In other words, it seems to only allow changes directly initiated by the user. Which sounds great for safety reasons, and resembles e.g. Safari's behavior (every API that interacts with the system outside the very content of the page the user is on seems to only work via direct user initiative, like a click event). But well, would be good if there was a way to disable this protection.

One solution to this is to grant Full Disk Access permission in macOS to /bin/bash — I tried this and then the LaunchAgent was able to fix the PAM file. Because sudo-touchid is a (Bash) shell script, granting Full Disk Access to /opt/homebrew/opt/sudo-touchid/bin/sudo-touchid isn't sufficient.

A better solution than granting Full Disk Access to anything via Bash might be to create a binary instead; I followed the lead of a similar post on Stack Overflow and used this C code:

#include <stdlib.h>
int main(void) {
  int status = system("/opt/homebrew/opt/sudo-touchid/bin/sudo-touchid");
  int ret = WEXITSTATUS(status);
  return ret;
}

Compiled with gcc -Wall -o sudo-touchid-wrapper sudo-touchid-wrapper.c, and then edited the LaunchAgent to use this binary instead. When first run, it appears in macOS System Settings > Privacy & Security > Full Disk Access; when toggled to on the LaunchAgent can successfully edit /etc/pam.d/sudo.

artginzburg commented 1 year ago

@imgrant thanks, this is huge! I'll check it out in depth when I can

newadventure079 commented 1 year ago

I had actually already enabled Full Disk Access previously to /bin/bash and still see the issue

imgrant commented 1 year ago

I had actually already enabled Full Disk Access previously to /bin/bash and still see the issue

Hmm. 🤷 Did you try the binary wrapper approach?

newadventure079 commented 1 year ago

No, not yet. I was just stating that I've had already given /bin/bash Full Disk Access because of other homebrew and personal scripts in the past