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

Fix auto-uid-allocation in Docker containers #7692

Merged
merged 6 commits into from
Feb 7, 2023

Conversation

edolstra
Copy link
Member

@edolstra edolstra commented Jan 25, 2023

Motivation

Fixes

# nix build --extra-experimental-features 'auto-allocate-uids nix-command flakes' --auto-allocate-uids --impure --expr 'with import <nixpkgs> {}; runCommand "foo" {} "mkdir $out"'
foo> /tmp/nix-build-foo.drv-0/.attr-0l2nkwhif96f51f4amnlf414lhl4rv9vh8iffyp431v6s28gsr90: line 1: /nix/store/2fhbn9q5q3r96p9jmrnwh36a7m3iwvar-foo: Permission denied

This didn't work because sandboxing doesn't work in Docker. However, the sandboxing check is done lazily - after clone(CLONE_NEWNS) fails, we retry with sandboxing disabled. But at that point, we've already done UID allocation under the assumption that user namespaces are enabled.

So let's get rid of the "goto fallback" logic and just detect early whether user / mount namespaces are enabled.

This commit also gets rid of a compatibility hack for some ancient Linux kernels (<2.13).

It also adds a check to automatically disable sandboxing in unprivileged podman containers, since that doesn't work.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or bug fix: updated release notes

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall. Beyond the actual bugfix, it feels like this is untangling this piece of code quite a lot, which is quite pleasant :)

src/libutil/namespaces.cc Outdated Show resolved Hide resolved
src/libstore/build/local-derivation-goal.cc Outdated Show resolved Hide resolved
src/libstore/build/local-derivation-goal.cc Show resolved Hide resolved
This didn't work because sandboxing doesn't work in Docker. However,
the sandboxing check is done lazily - after clone(CLONE_NEWNS) fails,
we retry with sandboxing disabled. But at that point, we've already
done UID allocation under the assumption that user namespaces are
enabled.

So let's get rid of the "goto fallback" logic and just detect early
whether user / mount namespaces are enabled.

This commit also gets rid of a compatibility hack for some ancient
Linux kernels (<2.13).
In unprivileged podman containers, /proc is not fully visible (there
are other filesystems mounted on subdirectories of /proc). Therefore
we can't mount a new /proc in the sandbox that matches the PID
namespace of the sandbox. So this commit automatically disables
sandboxing if /proc is not fully visible.
@edolstra edolstra force-pushed the fix-docker-auto-uid-allocation branch from e2eaeb9 to 0a70b41 Compare February 7, 2023 22:03
@edolstra edolstra dismissed thufschmitt’s stale review February 7, 2023 22:03

Comments addressed.

@edolstra edolstra enabled auto-merge February 7, 2023 22:03
@edolstra edolstra merged commit ae6de01 into NixOS:master Feb 7, 2023
@edolstra edolstra deleted the fix-docker-auto-uid-allocation branch February 17, 2023 10:36
@abathur
Copy link
Member

abathur commented Jun 9, 2023

@edolstra #8481 may be related to this change? It sounds like some of the old lack-of-sandboxing-support issues, but it looks like reports of that problem dried up a few years ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants