cask: don't assume that sudo has write access to the caskroom #14370
Conversation
| command.run!("/bin/mv", args: [source, target], sudo: true) | ||
| # default sudo user isn't necessarily able to write to Homebrew's locations | ||
| # e.g. with runas_default set in the sudoers (5) file. | ||
| command.run!("/bin/cp", args: ["-pR", source, target], sudo: true) |
There was a problem hiding this comment.
Hmm, does this only apply here or to pretty much everywhere we use sudo: true? I suspect the latter in which case perhaps it's better fixed in the sudo logic to ensure that you're sudoing to root?
There was a problem hiding this comment.
With respect to casks, I think, it is proper to act out of assumption that current user has unrestricted access to the homebrew prefix, while sudo has access outside of it. That appears to be sufficient, as sudo is used to move files into appropriate system locations. Note that pkgs have their own way to elevate privileges and don't need sudo.
Overall, I think it's important that brew does not unnecessarily limit sysop's ability to configure sudoers. In reality, brew only needs to write to a handful of parths using a handful of commands. Thus it's quite feasibly for a sysop to craft restricted sudoers files. This is good, because it lifts responsibility from brew with respect to security considerations as policies can be freely configured elsewhere.
There was a problem hiding this comment.
I'm pretty sure the expectation is that sudo: true will have unrestricted access to the system (bar those things that SIP prevents).
Overall, I think it's important that brew does not unnecessarily limit sysop's ability to configure sudoers.
Not sure I agree with this, sorry. I think it's not on Homebrew to support more esoteric default configurations here that will cause issues for many other tools assuming e.g. sudo ... will run as root.
There was a problem hiding this comment.
I think the only place where brew needs ‘root’ specifically and not admin is where it communicates with launchctl.
Consider the following argument instead: it is already recommended that in multi-user environments ‘brew’ should have a separated dedicated user. With that in mind ‘mv’ is problematic because you will end up with improperly owned (non-admin user/group) items in system locations, such as /Applications, e.g. see the following thread on Apple’s dev forums: https://developer.apple.com/forums/thread/723134. ‘cp’ addresses that as well my esoteric sudo config :)
There was a problem hiding this comment.
it is already recommended that in multi-user environments ‘brew’ should have a separated dedicated user
We don't recommend this anywhere I'm aware of. There's many holes in our multiuser environment setup.
‘cp’ addresses that as well my esoteric sudo config :)
Sure, it just feels very weird to have this in artifact/moved when it's no longer moving and for it to only be needed with this unusual sudo config.
There was a problem hiding this comment.
@Kentzo Ok, let's try it. Note, we'll revert this ASAP if there's any issues.
There was a problem hiding this comment.
I think the rsync should be used instead of cp here: the former has flags that allow to preserve all the attributes but user-id/group-id while with the latter it's all or nothing.
I did not notice this in my testing, because in my case sudo is a non-root, thus chmod fails which cp ignores while quietly setting sudo user/group as the owner.
The command should be /usr/bin/rsync -qrltE (quiet, recursive, copy symlinks, preserve times, preserve extended attributes), i.e. same as cp -pR but without preserving user-id/group-id.
There was a problem hiding this comment.
@Kentzo rsync is overkill here. If cp isn't enough: let's just revert this.
There was a problem hiding this comment.
cp is good enough for me. It's not good enough for someone who has a separate user for brew without overriding runas_default in sudoers.
I do believe that this is a bug in macOS (that it quietly keeps quarantining the app if it's owned by a wrong user/group). Hopefully it will get fixed, but at least I wanted to mention a workaround here if someone else hits it.
There was a problem hiding this comment.
It's not good enough for someone who has a separate user for brew without overriding runas_default in sudoers.
Let's not worry about bugs that no users have reported.
| command.run!("/bin/mv", args: [source, target], sudo: true) | ||
| # default sudo user isn't necessarily able to write to Homebrew's locations | ||
| # e.g. with runas_default set in the sudoers (5) file. | ||
| command.run!("/bin/cp", args: ["-pR", source, target], sudo: true) |
There was a problem hiding this comment.
@Kentzo Ok, let's try it. Note, we'll revert this ASAP if there's any issues.
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?Currently
brew install --caskassumes thatsudohas write access to the caskroom. This is not necessarily true: a system administrator may override the default sudo user via the runas_default setting in sudoers. This user may be defined to have sufficient privileges to write to the system directory for installation but restricted from writing to Homebrew's locations.A performance enhancement would be to change unconditional
cp -pRto try commands (in order):mvcpas the resulting file will be owned by the user that has writability in the parent's directorycp -cpRcp -pRHowever, the benefit might be marginal: on my system (MBP M1 Pro) copying Xcode.app with the
-ctakes ~1min vs ~2min without it.