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

Add pam-watchid authentication to sudo command #1216

Merged
merged 1 commit into from
Feb 27, 2025
Merged

Conversation

Coosis
Copy link
Contributor

@Coosis Coosis commented Dec 5, 2024

With projects like pam-watchid, it's now possible to enable apple watches to authenticate sudo commands, alongside with TouchID. This is mostly a raw idea, but I would love to see something like this in nix-darwin.

Because this is used to present an idea, there are couple things to note:

  1. I did not add any tests
  2. I straight up embedded a package in the source code
  3. The x86 part fails to compile for some reason so that's now commented out so I'll need some help with that

I would love for others to pick this up as this is my first time contributing to the project, and I am unfamiliar with:

  1. How to use apple's toolchains when building packages
  2. Which packages should I include
  3. The best practices

@Enzime
Copy link
Collaborator

Enzime commented Dec 5, 2024

The package should live in Nixpkgs not in nix-darwin

Also, I don’t think pam-watchid is necessary. I have pam_tid enabled through security.pam.enableSudoTouchIdAuth and I went into System Settings and allowed my Apple Watch to unlock my laptop and every time I’m prompted for my fingerprint for sudo it also asks on my Apple Watch

@Coosis
Copy link
Contributor Author

Coosis commented Dec 5, 2024

The package should live in Nixpkgs not in nix-darwin

Also, I don’t think pam-watchid is necessary. I have pam_tid enabled through security.pam.enableSudoTouchIdAuth and I went into System Settings and allowed my Apple Watch to unlock my laptop and every time I’m prompted for my fingerprint for sudo it also asks on my Apple Watch

Simply adding it to Nixpkgs will not put the .pam file in place. For some reason my machine doesn't let me use my watch when security.pam.enableSudoTouchIdAuth is enabled, I believe this happens to machines that doesn't have a touchid(also laptops that have lids closed).

@Enzime
Copy link
Collaborator

Enzime commented Dec 5, 2024

First you’ll need to add the package to Nixpkgs, then you can use it through pkgs from a nix-darwin module

@Enzime Enzime changed the title Petition to add apple watch authentication to sudo command Add pam-watchid authentication to sudo command Dec 5, 2024
@Enzime
Copy link
Collaborator

Enzime commented Dec 5, 2024

Have you enabled your Apple Watch to unlock your applications and your Mac in System Settings? It's under Login Password on non-Touch ID devices

@Coosis
Copy link
Contributor Author

Coosis commented Dec 6, 2024

Have you enabled your Apple Watch to unlock your applications and your Mac in System Settings? It's under Login Password on non-Touch ID devices

Yes, I have. No pop up to use apple watch.

@Samasaur1
Copy link
Contributor

There was a discussion on Matrix about this and we're waiting on Logicer16/pam-watchid#5 before adding that repo to nixpkgs which will then allow this PR to progress.

It hasn't been that long since I opened that PR, but if it really drags on for a while I might just add my fork to nixpkgs instead and then this PR will be able to move forwards

@Samasaur1
Copy link
Contributor

This also probably depends on #1205 for linking the built .so file into /usr/local/lib/pam

@Samasaur1
Copy link
Contributor

cc: @Coosis to confirm, but iirc we confirmed on Matrix that it works to symlink the .so file into place and we don't have to copy it

@Enzime
Copy link
Collaborator

Enzime commented Dec 15, 2024

This also probably depends on #1205 for linking the built .so file into /usr/local/lib/pam

PAM libraries can be used directly from the Nix Store I believe

@Coosis
Copy link
Contributor Author

Coosis commented Dec 15, 2024

cc: @Coosis to confirm, but iirc we confirmed on Matrix that it works to symlink the .so file into place and we don't have to copy it

Yes symlink will work just fine. Still waiting for #1205 and I definitely think you should just fork it and add fork to nixpkgs.

@Enzime
Copy link
Collaborator

Enzime commented Dec 16, 2024

You can just make a PR to nixpkgs and use your pull request as a patch on top of their repo

@Samasaur1
Copy link
Contributor

PAM libraries can be used directly from the Nix Store I believe

oh great, even easier! (i have no experience with PAM stuff, just the Swift side)

You can just make a PR to nixpkgs and use your pull request as a patch on top of their repo

Mostly I'm just wondering if the repo is being supported at all — it seems like it's a fork of a fork of a fork and I'm wondering if any in the chain are maintained or if they were made to fix one thing and then essentially abandoned. If so, I may just point the nixpkgs package to my repository so I can maintain it more easily

@Logicer16
Copy link

I'm wondering if any in the chain are maintained

For the record, I intend to maintain my fork for the foreseeable future.

@Samasaur1
Copy link
Contributor

Okay, yay! I'll open a PR to nixpkgs when I get a chance and link it here

@Samasaur1
Copy link
Contributor

NixOS/nixpkgs#368071

@tymscar
Copy link

tymscar commented Dec 31, 2024

Now that nixpkgs has merged @Samasaur1's PR, this should go fairly well, right?

@Samasaur1
Copy link
Contributor

Depends on whether the library can be used directly from the Nix store or whether it needs to be linked into place. If it can be used directly, then there's no blockers. If it needs to be linked, we're waiting on #1205.

@Coosis
Copy link
Contributor Author

Coosis commented Dec 31, 2024

Depends on whether the library can be used directly from the Nix store or whether it needs to be linked into place. If it can be used directly, then there's no blockers. If it needs to be linked, we're waiting on #1205.

I actually don't know how a pam module can be used from nix store directly.

@Samasaur1
Copy link
Contributor

Depends on whether the library can be used directly from the Nix store or whether it needs to be linked into place. If it can be used directly, then there's no blockers. If it needs to be linked, we're waiting on #1205.

I actually don't know how a pam module can be used from nix store directly.

My guess is that you'd put the full path to the .so file in one of the files under /etc/pam.d? but again I haven't messed around with PAM at all

@jmartindf
Copy link

I was able to use the pam module directly from the store:

  etc = {
    "pam.d/sudo_local".text = ''
      auth sufficient pam_tid.so
      auth sufficient ${pkgs.pam-watchid}/lib/pam_watchid.so
    '';
  };

@Coosis
Copy link
Contributor Author

Coosis commented Feb 7, 2025

I was able to use the pam module directly from the store:

  etc = {
    "pam.d/sudo_local".text = ''
      auth sufficient pam_tid.so
      auth sufficient ${pkgs.pam-watchid}/lib/pam_watchid.so
    '';
  };

Nice! I would say this could be the closest thing possible. But i do think this could override other pam modules if handled incorrectly. My laziness is kicking in and telling me to take this and call it a day

@tymscar
Copy link

tymscar commented Feb 7, 2025

Would this need to be added manually?

@Samasaur1
Copy link
Contributor

At the moment, yes, you'll need to do it manually

@Enzime
Copy link
Collaborator

Enzime commented Feb 25, 2025

Now that #1344 is merged, you can rebase this PR and add an option similar to security.pam.services.sudo_local.reattach

@Enzime
Copy link
Collaborator

Enzime commented Feb 25, 2025

It seems the PR got closed, if that wasn't your intention, you'll need to push some new commits to this branch before you can reopen the branch

@Coosis
Copy link
Contributor Author

Coosis commented Feb 25, 2025

It seems the PR got closed, if that wasn't your intention, you'll need to push some new commits to this branch before you can reopen the branch

I originally used master, and I was thinking about opening a feature branch for actual merging. Is that not necessary? Should I push to my fork/main directly?

@Enzime
Copy link
Collaborator

Enzime commented Feb 25, 2025

Yeah, it should be fine to keep using your master branch. GitHub ties the pull request to the branch so if you want to switch branches you’ll need to make a new PR but it would probably be best to keep using this PR

@Coosis Coosis reopened this Feb 25, 2025
@Coosis
Copy link
Contributor Author

Coosis commented Feb 25, 2025

Inside sudo_local, I added the text after reattach and right before touchId. The idea is that watchId will be prompted first if touchId is not available. The description can be re-written if desired.

@Enzime
Copy link
Collaborator

Enzime commented Feb 25, 2025

I think it should be after the Touch ID if you want it to run it if Touch ID fails

@Coosis
Copy link
Contributor Author

Coosis commented Feb 25, 2025

done, waiting on checks

Copy link
Collaborator

@Enzime Enzime left a comment

Choose a reason for hiding this comment

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

After you make your changes, can you squash the changes into one commit and change the commit message to:

pam: add `pam_watchid` support

Copy link
Collaborator

@Enzime Enzime left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for the contribution

@Enzime Enzime merged commit 665cc04 into LnL7:master Feb 27, 2025
3 checks passed
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.

6 participants