-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Run test to determine if sandboxing is available #3006
Conversation
a67b488
to
fff1b7e
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.
What's the exact use case for this? If it's systems that lack CLONE_NEWUSER
, it's probably better to disable only CLONE_NEWUSER
, rather than all sandboxing. So you could check whether clone
succeeds with and without CLONE_NEWUSER
.
src/libstore/build.cc
Outdated
|
||
int flags = CLONE_NEWUSER | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_PARENT | SIGCHLD; | ||
size_t stackSize = 1; | ||
char* stack = malloc(stackSize); |
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.
Probably better to use std::make_unique<unsigned char[]>(stackSize);
. Leaked mallocs pollute valgrind runs.
src/libstore/build.cc
Outdated
if (child == -1) { | ||
printError("warning: test cloning failed, sandboxing is disabled"); | ||
useChroot = false; | ||
} |
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.
Could this whole test be done inside an std::once
? Now we're forking an extra child process for every build.
src/libstore/globals.hh
Outdated
@@ -11,7 +11,7 @@ | |||
|
|||
namespace nix { | |||
|
|||
typedef enum { smEnabled, smRelaxed, smDisabled } SandboxMode; | |||
typedef enum { smEnabled, smEnabledIfAvailable, smRelaxed, smDisabled } SandboxMode; |
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.
Maybe this should be a separate flag, because you may want to use relaxed mode and disable the sandbox if unavailable.
src/libstore/build.cc
Outdated
kill(child, SIGKILL); | ||
|
||
if (child == -1) { | ||
printError("warning: test cloning failed, sandboxing is disabled"); |
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.
There is a warn
function since recently.
That sounds like a better idea, if it works right. As a plus that would avoid having an extra "sandbox" setting as sandboxing would still be enabled (just without CLONE_NEWUSER). There are at least 3 patches for this in the wild that I can find:
|
fff1b7e
to
679273c
Compare
Some kernels disable "unpriveleged user namespaces". This is unfortunate, but we can still use mount namespaces. Anyway, since each builder has its own nixbld user, we already have most of the benefits of user namespaces.
When sandbox-fallback = true (the default), the Nix builder will fall back to disabled sandbox mode when the kernel doesn’t allow users to set it up. This prevents hard errors from occuring in tricky places, especially the initial installer. To restore the previous behavior, users can set: sandbox-fallback = false in their /etc/nix/nix.conf configuration.
679273c
to
11d8534
Compare
Looks good but I would be more comfortable with |
My main concern with |
startProcess does not appear to send the exit code to the helper correctly. Not sure why this is, but it is probably safe to just fallback on all sandbox errors.
I also reported #3000 which is along the same lines. I'll run this PR through my nix install matrix. |
This looks like it should be handled (WSL2), but I'm not sure how:
|
Note that #3000 actually isn't related to WSL. |
2.2.2's default to enable the sandbox broke installation for:
each of these failed with one of two messages:
or:
After this PR, neither of these errors come up, but all six now fail with another error:
Here are the install matrix's reports:
|
The new commit fixes many (all?) of the installations https://buildkite.com/organizations/grahamc/pipelines/nix-install-matrix/builds/61/jobs/ae342503-d31f-43e6-8434-a9136b377b11/artifacts/5cdb7828-2262-44bd-bc4f-4da5cfe8da2d (built at |
adds smEnabledIfAvailable option that will run a check where possible
to determine if user namespaces are supported.