Skip to content
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

nixos/tests: Use a patched QEMU for testing #20500

Merged
merged 1 commit into from
Dec 15, 2016

Conversation

aszlig
Copy link
Member

@aszlig aszlig commented Nov 17, 2016

Our NixOS test runner is using virtio to make the store available to the VMs within the test. Since NixOS/nix@5e51ffb we now build with user namespaces and we have a mapping from the uid/gid of the current build user to a uid/gid within the newly created user namespace. In NixOS/nix#1131 this is going to be 0/0 again but even using 1000/100 before (in NixOS/nix@ff0c0b6) doesn't avoid the following problem.

So the problem here is that the bind-mounted file systems within the build process will have a different uid/gid mapping and this in turn leads to test failures like these:

What happens here is that the programs tested here (sudo and cups) are doing strict permission checks and bail out because the paths that are mounted within the builder sandbox now have uid/gid 65534 and thus don't match with the user that the process is running (uid/gid 0) as.

From the perspective of namespacing this makes sense, because the uid 0 within the user namespace isn't the same as uid 0 from the upper namespace.

Right now it seems that the official Hydra doesn't yet run with a recent enough Nix version to be hitting this problem, but it will eventually get there as well.

So the following options come in mind to address this:

  • Patch programs like sudo and cups to not do restrictive checks on paths within the Nix store.
  • Use something like bindfs to override the uid/gid (very slow, because it's implemented as FUSE).
  • Prevent the Nix builder from creating user namespaces on a selective basis (this however also prevents relocated stores).
  • Patch qemu to always report uid 0 and gid 0 to the VM on stat().

With this PR I've chosen the latter, because it's the most generic and least invasive approach. However we still need to fix build failures of some programs (for example go's tests fail because of this https://headcounter.org/hydra/build/1455527/nixlog/1/raw, but I haven't looked into it in detail yet).

Cc: @edolstra

The reason to patch QEMU is that with latest Nix, tests like "printing"
or "misc" fail because they expect the store paths to be owned by uid 0
and gid 0.

Starting with NixOS/nix@5e51ffb, Nix
builds inside of a new user namespace. Unfortunately this also means
that bind-mounted store paths that are part of the derivation's inputs
are no longer owned by uid 0 and gid 0 but by uid 65534 and gid 65534.

This in turn causes things like sudo or cups to fail with errors about
insecure file permissions.

So in order to avoid that, let's make sure the VM always gets files
owned by uid 0 and gid 0 and does a no-op when doing a chmod on a store
path.

In addition, this adds a virtualisation.qemu.program option so that we
can make sure that we only use the patched version if we're *really*
running NixOS VM tests (that is, whenever we have imported
test-instrumentation.nix).

Tested against the "misc" and "printing" tests.

Signed-off-by: aszlig <aszlig@redmoonstudios.org>
@mention-bot
Copy link

@aszlig, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @globin and @fpletz to be potential reviewers.

@aszlig
Copy link
Member Author

aszlig commented Nov 17, 2016

Cc: @domenkozar because this should surface on Hydra deployments in the wild with #19396.

@aszlig
Copy link
Member Author

aszlig commented Nov 17, 2016

Also I'm open for ideas to solve this in a better way.

@edolstra
Copy link
Member

Well, the easy solution is not to use user namespaces. I'd prefer not to have to patch qemu (and who knows how many other packages) to work around bugs caused by user namespaces.

@aszlig
Copy link
Member Author

aszlig commented Nov 17, 2016

@edolstra: These problems are not only surfacing with user namespaces but also on non-NixOS without user namespaces. I guess the reason we didn't get bug reports yet is that the amount of people running NixOS tests on non-NixOS systems is close to zero. For the issue related to go, I expect it to surface in a few days or weeks.

@aszlig
Copy link
Member Author

aszlig commented Nov 17, 2016

@edolstra: Also, there is another option to fix this in a generic way, which I forgot above: Use seccomp to force st_uid/st_gid to be 0 on *stat() in NixOS/nix#1131. The problem however is that this is more complicated, because we need to do this only for stat() calls on store paths, so implementing this for fstat() will be challenging.

@edolstra
Copy link
Member

Why does it have to be 0 only for store paths? Can't we force everything to be 0?

@aszlig
Copy link
Member Author

aszlig commented Nov 17, 2016

@edolstra: Hm, good point... going to implement it.

@aszlig
Copy link
Member Author

aszlig commented Nov 28, 2016

@edolstra: Okay, this is very tricky to implement, because if we want to emulate the stat syscall, we need to use either SIGSYS or ptrace to intercept the call from seccomp. Unfortunately using SIGSYS we can't really do this properly, because we need to call the real stat syscall from within the signal handler which in turn is limited by seccomp.

This would lead to an infinite recursion, but usually the way to properly implement this with seccomp is to match only on specific arguments of stat, but we do have two pointers here which we can't dereference via the seccomp BPF program, so there is no way to avoid this recursion.

If we go for the second implementation, we could use a tracer to pause and re-inject the emulated syscall from outside the sandbox. But having a tracer attached to each process not only has an impact on performance but also can cause side-effects in terms of ptrace.

Both methods however have another big drawback: We'd lose cross-platform compatibility of libseccomp, because we need to alter the syscall's registers ourselves so it gets even more complicated: For example on x86_64 we need to call stat based on the values of RDI, RSI, de-reference RSI, write the altered struct stat into it, write the new return code to RAX and also take care of errno (as in restoring it to the value just after stat).

On 32bit x86 we can just swap R* with E*, but on other platforms this is much more complicated (for example MIPS has indirect syscalls).

And on top of all that, we still have a build environment that's different than we have in older Nix, so we will see builds failing on newer Nix where they have succeeded in older Nix versions.

So all in all, I think this is not worth it, so it might be a good idea to either remove user namespaces altogether or make it the new standard for Nix builders (which also means, that we need to do patches like this very PR).

Speaking of builders, using user namespaces indeed has one particular advantage: We could implement a real nix-shell --pure which ends up in a real builder environment.

@edolstra edolstra merged commit 705829b into NixOS:master Dec 15, 2016
@c0bw3b c0bw3b added the 6.topic: testing Tooling for automated testing of packages and modules label Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug 6.topic: nixos 6.topic: testing Tooling for automated testing of packages and modules 9.needs: community feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants