-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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 allow_in_sandbox!
DSL
#17734
Add allow_in_sandbox!
DSL
#17734
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.
Makes sense to me, good idea!
There is some more work to be done here with e.g. adding tests and updating Essentially, my vision is that most formulae should be not affected by this change. For the odd one that explicitly writes to In the future, we can add additional things here. For example, once the work discussed in #17703 (comment) is implemented, we might want to deny network activity by default but allow individual formulae to use it with |
@Rylan12 Often the easiest way to do this is a Homebrew/core PR changing a few formulae so people can see what it's expected to look like (and potentially check out both branches for testing).
sounds great 👍🏻
Think we already have a separate DSL for allowing sandbox network access that @alebcay worked on? |
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 great so far, nice work @Rylan12!
Library/Homebrew/formula.rb
Outdated
# @!method reduced_sandbox | ||
# @return [Array<Symbol>] | ||
# @see .reduce_sandbox | ||
def reduced_sandbox |
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.
def reduced_sandbox | |
def sandbox_allow |
or something is more obvious naming to me than reduced
Yes, in #17081. If there are other optional/configurable sandbox settings being added, maybe it makes sense to move the network controls into a new unified DSL. I haven't been actively promoting the presence of this DSL or adding into formulae, since I was hoping to improve the sandboxing situation on Linux first - it felt weird to me that network calls might be blocked on macOS but not Linux, and I felt like that might introduce another platform-specific thing to debug when troubleshooting build failures. |
while this is admirable: macOS is our majority platform and the from-source sandboxing for the majority users is primarily "in Homebrew's CI" and "on macOS" so I don't think it's worth blocking progress on Linux support |
This makes sense to me 👍🏻 |
@alebcay I'm curious what your thinking was for having both For the new dsl, I was thinking it would only be I just pushed an update renaming the DSL to This isn't quite ready yet because the DSL needs to be added separately from the actual sandbox changes so it can make it into a tag. @alebcay do you have a sense of how many formulae would need I will open some sample homebrew/core PRs later on |
/private/tmp/homebrew
as HOMEBREW_TEMP
allow_in_sandbox!
DSL
Mike provided some justification for the allow function in #17081 (comment):
With things as they currently are, any formulae that, for example, rely on a language-specific package manager to pull modules/dependencies (e.g. Rust-based I believe that @nandahkrishna is considering/investigating/working on the implementation of a new |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?This PR changes the default
HOMEBREW_TEMP
to be/private/tmp/homebrew
on macOS and/tmp/homebrew
on Linux. The sandbox permissions are also updated to not allow writing to/private/tmp
except for thehomebrew
subdirectory.I'm marking this as draft because I am not 100% sure this is the correct move.
Additionally, I'm planning on adding a formula DSL to allow us to reduce the file write sandbox restrictions for formulae that need to write to
/private/tmp
. I'm thinking about calling thisreduce_sandbox!
or something similar. That way, this change can be skipped for formulae that need to be updated upstream without blocking us from merging things. Eventually, this might be something that can be enforced so that users can choose to not install any formulae that need a reduced sandbox or something like that.