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
macvim: Add sandboxProfile #69576
macvim: Add sandboxProfile #69576
Conversation
@@ -133,6 +133,9 @@ stdenv.mkDerivation { | |||
find $out/share/man \( -name eVim.1 -or -name xxd.1 \) -delete | |||
''; | |||
|
|||
# We rely on the user's Xcode install to build | |||
__noChroot = true; |
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.
I personally don't like the idea of using __noChroot
inside of nixpkgs. Have you tried to add the required paths like "/usr/bin/ibtool" to impureHostDeps
instead? That would continue to avoid purity issues with eg. homebrew packages as well as non-obvious sandbox holes.
macvim.overrideAttrs (drv: {
patches = [ ./upload-ssh-keys.patch ];
})
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.
I mean additional sandbox definitions, impureHostDeps are for the pure mode only.
sandboxProfile = ''
(allow file-read* process-exec (literal "/usr/bin/ibtool"))
'';
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.
AIUI __impureHostDeps
only works if the path exists within a directory defined using the allowed-impure-host-deps
setting, and I'm rather skeptical that anyone is going to add all of Xcode to that, plus I don't even know if Xcode relies on any system-level binaries that would also need holes punched through (it already seems to require /usr/bin/tiffutil
to be in the PATH
for some reason, so I don't know what else is required).
That also wouldn't work if I configure my system such that the developer dir is something other than /Applications/Xcode.app
either. I suppose I could explicitly set DEVELOPER_DIR
to /Library/Developer/CommandLineTools
which is the installation path for the command-line tools version of the build tooling, but I don't know how xcrun
behaves if that hasn't been installed yet (does it fall back to /Applications/Xcode.app
or does it prompt the user to install the tools?)
And even if I do manage to find a complete list of holes to request, and expect people to actually modify allowed-impure-host-deps
to include them, there's no guarantee that Xcode updates won't change the paths that it needs access to.
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.
In any case, most people on darwin aren't using sandboxing anyway, and punching holes won't allow ofBorg to build it because there's no way ofBorg will allow that (even if it has Xcode installed, which I wouldn't assume).
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.
Just saw your update. I don't know how sandboxProfile
works; does it require additional settings to enable its use like __impureHostDeps
does?
Anyway, the commentary about xcode-select
still holds, as does the questions of whether Xcode relies on other system-provided stuff. That said, if sandboxProfile
can be used without settings changes, that at least makes doing the exploration of what's required a little more compelling. Though I'm still assuming ofBorg's builders don't have Xcode installed.
I wonder if I could do a sandbox profile that allows all filesystem access but still rejects network access? That would be better than nothing.
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.
It's similar to __noChroot since it allows defining arbitrary sandbox rules, but enables only opening only filesystem or a more limited subset of paths. This is the starting point that's used for the build sandbox. https://github.com/NixOS/nix/blob/5038e1bec43a71c97ae7f8be07218a8a2edbb6a1/src/libstore/sandbox-defaults.sb
eg.
(allow file-read* process-exec (subpath "/usr/bin")
(subpath "/Applications")
(subpath "/Library/Developer"))
or
(allow file-read* process-exec (subpath "/"))
; block homebrew dependencies
(deny file-read* process-exec (subpath "/usr/local"))
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.
In an initial test, allowing full filesystem access as well as mach-lookup
allows it to build. I will continue exploring this tonight as to what I can reasonably lock down (e.g. blocking homebrew deps seems like a good idea).
The Console does show a few blocked calls to fsctl
and iokit-open
. I don't know if I should just ignore those or allow them, since I don't know what's going on (the iokit-open
calls have to do with nibs, I'm assuming it's trying to use the GPU for something; no idea about fsctl
, the target there is 0x682f
but I can't figure out what that corresponds to; the 0x68 prefix at least used to apply to HFS volumes but I can't find anything for 0x47 and most of the 0x68 definitions disappeared in a more recent version of xnu.). I'm tempted to go ahead and allow them since I don't think they're doing anything harmful, but the build does seem to work with them blocked.
ed7bb01
to
5231def
Compare
I've pushed a new commit that uses |
5231def
to
d6378fd
Compare
Updated. It still allows full filesystem access because we don't know where Xcode is located, but we're now blocking |
Actually I take it back, I'm going to remove |
d6378fd
to
e4e463b
Compare
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.
I would prefer a whitelist instead, but this addresses the main concerns I had before.
It occurs to me I should add |
This allows full filesystem access except for Homebrew. This is because we don't know where Xcode will be installed so we can't just whitelist it and its dependencies.
e4e463b
to
cf6fd91
Compare
Updated accordingly. Here's the trivial diff: diff --git a/pkgs/applications/editors/vim/macvim.nix b/pkgs/applications/editors/vim/macvim.nix
index 0951c226bb8..b639ab61784 100644
--- a/pkgs/applications/editors/vim/macvim.nix
+++ b/pkgs/applications/editors/vim/macvim.nix
@@ -139,7 +139,7 @@ stdenv.mkDerivation {
sandboxProfile = ''
(allow file-read* file-write* process-exec mach-lookup)
; block homebrew dependencies
- (deny file-read* file-write* process-exec mach-lookup (subpath "/usr/local"))
+ (deny file-read* file-write* process-exec mach-lookup (subpath "/usr/local") (with no-log))
'';
meta = with stdenv.lib; { |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
This allows full filesystem access when the sandbox is in relaxed mode.
Motivation for this change
The
macvim
package won't build in ofBorg due to sandboxing. I assume it still won't build with this change, but this should be friendlier to users who want sandboxing.Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)