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 option to pam module to use pam_reattach #662

Closed
wants to merge 1 commit into from

Conversation

vamega
Copy link

@vamega vamega commented May 17, 2023

This PR enables sudo authentication via Touch ID inside tmux as it's been described in the following article.

I've tested it on my M1 Macbook Air, and went through a MacOS upgrade with this change.
As expected the changes to /etc/pam.d/sudoers was lost on the upgrade, but a quick darwin-rebuild switch later, things were working correctly once more.

Closes #606, and mostly derived from the code of @sestrella and @maljub01

@vamega vamega marked this pull request as ready for review May 18, 2023 04:00
@LnL7
Copy link
Owner

LnL7 commented May 22, 2023

Given that there are multiple permutations for the same file, I'm not sure how well the patch module will handle switching between variants rather than just applying or rolling back the same patch. Does that work without issues?

@vamega
Copy link
Author

vamega commented May 23, 2023

I just tested every state transition (manually, but if someone knows how this could be automated please let me know), and every transition works with the latest change I pushed.

My original attempt had a mistake, where the patch for only pam-reattach, was trying to add the touchid module, the discrepancy with the line count in the patch file caused the patch to not apply.

With the current patch, every state transition works correctly. The patches module handles switching between variants just fine it seems. Table for transitions below.

original tid value original reattach value new tid value new reattach value success
false false false true y
false false true false y
false false true true y
false true true true y
false true false false y
false true true false y
true false true true y
true false false false y
true false true false y
true true false true y
true true false false y
true true true false y

@maljub01
Copy link
Contributor

The way the patch module is implemented, it will undo all "removed" patches first before applying any new ones. That's why all the tests were successful.

This works across generations too so if we were to modify the patch file later on, it will still work fine when everyone upgrades, because it will have a copy of the old version of the patch and remove it first before applying the new version

I think it should be pretty robust actually, as long as we (and the end user) don't try to juggle multiple patches for the same file at the same time.

@vamega
Copy link
Author

vamega commented Jun 12, 2023

I've now been running this for about a month on two different Macbooks (one on Apple Silicon, the other is an intel Macbook).

I've upgraded both to Ventura since using this, and outside of having to activate my profile again to reapply the patches I've run into no issues.

I think this is in a place where it's worth merging.

@stringang
Copy link

any progress?

@shinzui
Copy link

shinzui commented Jun 2, 2024

Can we please merge this?

@emilazy
Copy link
Collaborator

emilazy commented Jun 13, 2024

Thank you for your work on this, and I do think we should have this feature, but unfortunately patches are quite fragile and compose badly, so I’m not too comfortable merging this approach, especially with the tendency for system upgrades to overwrite this file. I’m going to close this with the hopes of getting an approach like #787 or #591 merged soon. I’m sorry that it took so long to get around to this.

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