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

Use doas instead of sudo if it's installed (fixed) #364

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

StratusFearMe21
Copy link

Fixes the issues in #323. Same concept as the original

Copy link
Contributor

@morgant morgant left a comment

Choose a reason for hiding this comment

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

Thanks for your improvements, @StratusFearMe21! I'm not an official maintainer here or anything, but I've been maintaining a personal OpenBSD port and have proposed some changes to increase portability to BSDs (see Issue #344 & PR #348).

So, the following suggestions come from using doas under OpenBSD and my efforts to improve portability, but also to minimize changes for @graysky2 to review and later maintain:

  • I'd lean toward using only SUDO and just setting it to either sudo or doas and using it as the binary to execute
  • It seems like a reasonable requirement that $SUDO should be in PATH and could be confirmed with which in dep_check()
  • doas does support the -n option, so probably best to retain that so that profile-sync-daemon doesn't try to prompt for authentication interactively
  • The change to use runuser instead of sudo is not portable, so I'd suggest continuing to use $SUDO -u in its stead

Of course, that still leaves the sudo -kn situation. I submit the following for consideration:

  • While doas does not support sudo's -k option, it does have a -L option which appears to be equivalent to sudo -k without a command (doas -L exits without running a command)
  • While it would change the functionality by invalidating cached credentials for the session, instead of just ignoring cached credentials while executing the command, by separately calling sudo -k before sudo -n [...] we could more easily & cleanly conditionally execute either $SUDO -k or $SUDO -L depending on the value of $SUDO
  • That would also allow $SUDO -n [...] to be used without adding further conditionals

I'm curious regarding your thoughts. I'm happy to put together an alternate PR, but wouldn't be able to until Sunday at the earliest.

@dorsiflexion
Copy link

dorsiflexion commented Apr 18, 2024

@graysky2 Any chance this gets merged? This seems like not a big change and thus doable. I could help make this PR even shorter and more in line with the rest of the code, if you wish.

@graysky2
Copy link
Owner

I never used doas before. Need to test. Do you mind squash/rebase into a single commit?

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.

5 participants