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

dockertools: fix buildLayeredImage nix-store permissions #94243

Merged
merged 2 commits into from Jul 31, 2020

Conversation

johanot
Copy link
Contributor

@johanot johanot commented Jul 30, 2020

Motivation for this change

Fix bug in dockerTools.buildLayeredImage that causes non-root containers to be not runnable.

Pardon my python, but I was unsure how - in the prettiest way - to hook into the existing logic around /nix/store dir-creation.

example docker error message:

docker: Error response from daemon: OCI runtime create failed: container_linux.go:349: starting container process caused "exec: \"/nix/store/3avafdzgh694z5m3rgw2vvlgsfz2gmkl-coredns-1.6.6/bin/coredns\": stat /nix/store/3avafdzgh694z5m3rgw2vvlgsfz2gmkl-coredns-1.6.6/bin/coredns: permission denied": unknown.

The cause of the error is that /nix and /nix/store has wrong permissions inside the final image, i.e.:

bash-4.4# ls -dl /nix/store/
drw-r--r-- 32 root root 34 Jan  1  1970 /nix/store/

The top-level directories lack world execute permissions, which, is ok as long we run the image as root, but fails when the image is run as an unprivileged user.

Steps to reproduce
  1. cherry-pick 4fc255e from my branch onto master
  2. docker load <$(nix-build . -A dockerTools.examples.bashLayeredWithUser)
  3. docker run --rm -it -u somebody bash-layered-with-user /bin/echo "it works"
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc @LnL7 @utdemir

Copy link
Member

@utdemir utdemir left a comment

Choose a reason for hiding this comment

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

Thank you for the change!

A similar PR was submitted a few days ago too: #93811, but we were waiting for a test. Since this one has also tests, @adrian-gierakowski do you mind if we continue from this PR? The changes are almost identical save the name of the function and this one has a test case.

pkgs/build-support/docker/examples.nix Show resolved Hide resolved
@adrian-gierakowski
Copy link
Contributor

@utdemir sounds good to me

Copy link
Member

@utdemir utdemir left a comment

Choose a reason for hiding this comment

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

The tests have passed on my system, I think this PR is good to merge.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Just one little detail: "priviliged" refers to containers that run with extra capabilities that could harm the host.
Otherwise looks good. Thank you for adding a test case!

nixos/tests/docker-tools.nix Outdated Show resolved Hide resolved
pkgs/build-support/docker/examples.nix Outdated Show resolved Hide resolved
@johanot
Copy link
Contributor Author

johanot commented Jul 31, 2020

@roberth Valid point! I applied your suggestion just now using the fancy github feature. :) I'll squash those extra commits into my test-commit in a sec.

…h buildLayeredImage

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
@johanot johanot force-pushed the dockertools-fix-nixstore-perms branch from e394a11 to f5db415 Compare July 31, 2020 08:14
@srhb
Copy link
Contributor

srhb commented Jul 31, 2020

@ofborg test docker-tools

@srhb
Copy link
Contributor

srhb commented Jul 31, 2020

Ofborg is happy too! Thanks :)

@srhb srhb merged commit c5a1eaf into NixOS:master Jul 31, 2020
@johanot johanot deleted the dockertools-fix-nixstore-perms branch July 31, 2020 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants