-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Improve Mac package for enterprise install scenarios #16073
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nstrauss, great work so far ❤️! A bunch of comments here but I think we're pretty close. The TL;DR is that I think we want to avoid as many edge-cases here as possible. I think HOMEBREW_PKG_USER
is a good addition but trying to guess the correct user or ending up with a root
-owned installation gets a fairly big 👎🏻 from me, I'm afraid.
We definitely have seen cases where it's not been present 🤷🏻. Would rather just leave this as it guarantees we have the newest version and adds minimal overhead.
I'm game for some additional configuration e.g. |
@nstrauss Oh also: just a process note to avoid any surprises: when this PR looks good to go, we probably won't merge it but instead close and cherry-pick to a branch for testing. This isn't our usual process but want to avoid merging something that breaks the installer creation CI and can only test that with a maintainer-created non-fork PR on this repository (so the secrets are exposed and accessible). |
@MikeMcQuaid Thank you for comments and feedback 🙏 Made a few changes in response, but would like more direction on a few other items. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes here, good progress!
package/scripts/postinstall
Outdated
# get last created account UID over 500 | ||
logged_in_user=$(dscl . list /Users UniqueID | awk '$2 > 500 { print $1 }' | tail -n 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4. Specify a user (
HOMEBREW_PKG_USER
).
This seems the best option here.
Not everyone will agree.
Yup, but that doesn't mean Homebrew needs to offer multiple options 😁
When Homebrew.pkg is installed at the login window with no console user, it's owned by
root
or another system account.
Yeh, this should be considered a bug. I agree with detecting these cases and handling it, I just think that we should fail if either a user isn't specified or logged-in.
Considering the vast majority of enterprise managed Macs have a 1:1 relationship with an assigned user, it's generally safe to say UID 501 will be the intended owner.
This feels too much like unexpected magic to me. Requiring an explicit user feels like a better fit here.
@nstrauss I merged #16077 so I've merged that in here to resolve merge conflicts for you. A few related notes:
Feels like the next step, as discussed would be to:
|
@nstrauss I was testing this out and couldn't seem to get it to work. https://stackoverflow.com/questions/55511165/pass-variables-to-the-installer-on-osx-package-installer makes it look like it may be more involved, unfortunately. |
Tested this myself and the linked article is correct. No longer working as I thought. May have another idea. |
@MikeMcQuaid Alright, thanks for bearing with me. Here's where we're at.
So slightly dismayed the PR has been hollowed out from its original intent. My main goal is still to get login window installations working. Your preference seems to be to explicitly define a user when one is not logged in. I respect that approach as it means admins can bring their own logic for which user to specify instead of Homebrew being heavily opinionated by shelling out to I'm still of the opinion falling back to first/last user created UID isn't all that bad, but it does mean Homebrew is taking a position, and with no configurable way to define user could end up with unexpected behavior. |
I think I have a better solution in place. 2c57881 adds support for reading a defined user from
I think this meets both of our requirements. Also fixed a few edge bugs in postinstall for Intel machines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Couple more tiny nits and I think we're good to go with this. As mentioned before: that will likely be me squashing and cherry-picking this into another non-fork branch for testing the CI job with all secrets etc.
Thanks again @nstrauss!
@MikeMcQuaid 🙏Thank you for your help and guidance along the way. I'm happy with where it's at now. Will leave the rest up to you. Otherwise let me know how else I can help. |
- Allow specifying user through `/var/tmp/.homebrew_pkg_user.plist` - Improve permission handling - Correctly write API cache - Add `preinstall` script to fail if we can't get a valid user Co-authored-by: Mike McQuaid <[email protected]>
8560827
to
e0ac545
Compare
Thanks so much for your first contribution to Homebrew! Without people like you submitting PRs we couldn't run this project. You rock, @nstrauss! |
Thank you for investing time into a package! This PR suggests a few improvements to use with large scale enterprise deployments.
The Mac package as is assumes there's a logged in console user in order to set permissions on
/opt/homebrew
and copy cache files to~/Library/Caches/Homebrew/api
. Usually when a person installs the package manually. However, in an enterprise environment that's often not the case.For organizationally managed Macs using software management tools like Jamf or Munki which initiate installs, it's common for there to be no logged in user or that a user hasn't even been created yet during install. These solutions and similar often run at the login window via a launch daemon before a valid console user has logged in. It's also possible to use Apple's Automated Device Enrollment to deliver packages via MDM before setup assistant is complete. Introducing fallbacks in cases where there's no active console user should cover most enterprise focused edge cases.
loginwindow
is the current console user, infer the intended user by looking at the most recently created UID over 500.HOMEBREW_PKG_USER
environment variable for admins to have better control over the target user where required (a lab environment). For example -HOMEBREW_PKG_USER="nathaniel.strauss" installer -pkg Homebrew-4.1.15 -target /
/opt/homebrew/cache_api
to user directory. This previously did not work.gh
from workflow. GitHub CLI is baked into the runner. https://github.com/actions/runner-images/blob/375e9a1c270ec81a3ffd5b429dcdee9508b46124/images/macos/macos-13-Readme.md?plain=1#L57