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

sudo: Prioritize sudo_as_root over HOMEBREW_SUDO_THROUGH_SUDO_USER. #16368

Merged
merged 5 commits into from Dec 21, 2023

Conversation

Kentzo
Copy link
Contributor

@Kentzo Kentzo commented Dec 20, 2023

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

sudo_as_root is reserved for cases where a command must be run as root and root only. No other user can suffice regardless of group and/or ACLs. Therefore it should be preferred over HOMEBREW_SUDO_THROUGH_SUDO_USER.

Kentzo referenced this pull request Dec 20, 2023
This environment variable allows telling Homebrew to use the `SUDO_USER`
variable to `sudo` through that user when Homebrew (Cask) attempts to
run `sudo`.

While we're here, clarify in some messaging that we're running `sudo`
and that that's the password we're asking for; the specific password is
configuration dependent and not the specific password for the user.

Similarly, remove the `Package installers may write to any location`
output; it's kinda spammy and doesn't feel like the right place.
Copy link
Member

@MikeMcQuaid MikeMcQuaid 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 the PR!

This is not correct, sorry, I think you may have misunderstood what Homebrew::EnvConfig.sudo_through_sudo_user? does.

Homebrew::EnvConfig.sudo_through_sudo_user? means that if I'm a user that cannot sudo to root then it instead sudos to an intermediate user who then sudos to root. This change would break that flow.

It may be that setting Homebrew::EnvConfig.sudo_through_sudo_user? breaks what you intended with sudo_as_root? and that the -u root should be part of another if rather than an elsif but it's definitely not correct as-is, sorry.

@Kentzo
Copy link
Contributor Author

Kentzo commented Dec 20, 2023

Ah, indeed. I linked value of homebrew_sudo_user to HOMEBREW_SUDO_THROUGH_SUDO_USER.

if I'm a user that cannot sudo to root then it instead sudos to an intermediate user who then sudos to root

Could you walk me through a specific use-case, step-by-step? Because in the current implementation I don't see how this is possible

In this case I think sudo_as_root must apply on top. Because its goal is to avoid "default" sudo user.

@MikeMcQuaid
Copy link
Member

Could you walk me through a specific use-case, step-by-step? Because in the current implementation I don't see how this is possible

I can't really walk you through it all. It's possible because I use it daily 😉

…USER

System commands run with sudo_as_root must be run as  regardless of who is running sudo.
@Kentzo
Copy link
Contributor Author

Kentzo commented Dec 20, 2023

Allow me to nitpick about the prompt: sudo can be configured such that the user whose password is being requested is not the same user that is used to run command. Please consider using something like "Password of '%p' to run as '%U': ".

Is it too late to change the new env variable to something like HOMEBREW_SUDO_AS_SUDO_USER? AS being a more "standard" preposition:

  • in sudo: runaspw, runas_default
  • in brew: sudo_as_root

@Kentzo
Copy link
Contributor Author

Kentzo commented Dec 20, 2023

Updated the PR to add -u root even when HOMEBREW_SUDO_THROUGH_SUDO_USER is specified, because the sudo_as_root: true commands still want root regardless of who is running sudo.

Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
@MikeMcQuaid MikeMcQuaid merged commit 82edf71 into Homebrew:master Dec 21, 2023
24 checks passed
@MikeMcQuaid
Copy link
Member

Looks good in local testing, thanks again @Kentzo!

@Kentzo Kentzo deleted the prefer_sudo_as_root branch December 21, 2023 17:36
@github-actions github-actions bot added the outdated PR was locked due to age label Jan 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants