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.

Sign up for GitHub

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

Jump to bottom

systemd-confinement: use /var/empty as chroot mountpoint #108028

Merged
merged 1 commit into from Jul 1, 2021
Merged

systemd-confinement: use /var/empty as chroot mountpoint #108028

merged 1 commit into from Jul 1, 2021

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Dec 31, 2020 •
edited

bind mounting directories into the nix-store breaks nix commands.
In particular it introduces character devices that are not supported
by nix-store as valid files in the nix store. Use /var/empty instead
which is designated for these kind of use cases. We won't create any
files beause of the tmpfs mounted.

Motivation for this change
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.

@Mic92 Mic92 requested review from dasJ and aszlig December 31, 2020 09:41
@Mic92 Mic92 changed the title systemd-confinment: move to /var/empty systemd-confinment: use to /var/empty as chroot mountpoint Dec 31, 2020
@Mic92 Mic92 changed the title systemd-confinment: use to /var/empty as chroot mountpoint systemd-confinment: use /var/empty as chroot mountpoint Dec 31, 2020
@aszlig
Copy link
Member

aszlig commented Dec 31, 2020

In particular it introduces character devices that are not supported by nix-store as valid files in the nix store.

Since those are within the tmpfs as you point out, can you give a specific example where this is a problem?

@Mic92
Copy link
Member Author

Mic92 commented Dec 31, 2020 •
edited

In particular it introduces character devices that are not supported by nix-store as valid files in the nix store.

Since those are within the tmpfs as you point out, can you give a specific example where this is a problem?

It becomes a problem if the service tries to use nix commands. i.e. I use systemd confinement to run my ci runner in a chroot: https://github.com/Mic92/dotfiles/blob/279819a5d20d281555ab679bae471faa4875eec6/nixos/eve/modules/drone.nix#L73

It is general unsound to mount stuff into store paths because it can also confuse nix-store when checking the integrity of derivations or even worse garbage collection. Also it is a bit faster when building since it saves one derivation and we create this directory with systemd.

@aszlig
Copy link
Member

aszlig commented Dec 31, 2020

It becomes a problem if the service tries to use nix commands. i.e. I use systemd confinement to run my ci runner in a chroot: https://github.com/Mic92/dotfiles/blob/279819a5d20d281555ab679bae471faa4875eec6/nixos/eve/modules/drone.nix#L73

Okay, granted, this is indeed a problem, for example if you want to use it to confine Hydra.

It is general unsound to mount stuff into store paths because it can also confuse nix-store when checking the integrity of derivations or even worse garbage collection.

Those bind mounts are private mounts, so applications not sharing the same namespace are unaffected.

Also it is a bit faster when building since it saves one derivation and we create this directory with systemd.

Originally I was even re-using the merged systemd units so it would be as "fast" but probably would confuse everyone, so I eventually went for a dedicated empty store path.

The reason why I refrained use /var/empty or even temporary paths was that I wanted to make sure that the path is read-only in the first place, even for the root user to prevent accidentally dropping files in that directory. Given that the service will fail whenever the tmpfs couldn't be mounted, we can however reasonably assume that something like that will never happen.

@aszlig
Copy link
Member

aszlig commented Dec 31, 2020

@ofborg test systemd-confinement

@aszlig aszlig changed the title systemd-confinment: use /var/empty as chroot mountpoint systemd-confinement: use /var/empty as chroot mountpoint Dec 31, 2020
aszlig
aszlig requested changes Dec 31, 2020
View reviewed changes
Copy link
Member

@aszlig aszlig left a comment

Choose a reason for hiding this comment

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

Temporarily blocked, since the test currently fails and the status by ofborg isn't very visible here.

@SuperSandro2000
Copy link
Member 

--- /nix/store/nfyjixayfsfrr79gx7drijlvkfbvx7db-nixos-test-driver-systemd-confinement/test-script       2020-12-31 11:21:39.900680 +0000
+++ /nix/store/nfyjixayfsfrr79gx7drijlvkfbvx7db-nixos-test-driver-systemd-confinement/test-script       2020-12-31 11:21:40.426911 +0000
@@ -13,11 +13,12 @@
         "chroot-exec ls -l /etc",
         "chroot-exec ls -l /run",
         "chroot-exec chown 65534 /bin",
     )
     machine.succeed(
-        'test "$(chroot-exec id -u)" = 0', "chroot-exec chown 0 /bin",
+        'test "$(chroot-exec id -u)" = 0',
+        "chroot-exec chown 0 /bin",
     )

@aszlig
Copy link
Member

aszlig commented Dec 31, 2020 •
edited

@SuperSandro2000: That's not the main issue because the test fails with the APIVFS changes of the last systemd update.

edit: Ah, maybe I wasn't clear here, with "visibility" I was referring to the fact that ofborg didn't mark the failures "red enough" =)

@aszlig
Copy link
Member

aszlig commented Dec 31, 2020

@Mic92: Just ran a working version of the test with this change and it floods the logs with Failed to create directory at /var/empty/usr: Operation not permitted. While it didn't create that directory even when running as root, the fact that systemd even tries to create it is something why we should be paranoid also add an additional check in the test to make sure /var/empty stays that way.

Now the only ugliness is the (non-fatal) failure message, which for some reason does not occur when mounting the tmpfs inside a store path.

@Mic92
Copy link
Member Author

Mic92 commented Dec 31, 2020

@Mic92: Just ran a working version of the test with this change and it floods the logs with Failed to create directory at /var/empty/usr: Operation not permitted. While it didn't create that directory even when running as root, the fact that systemd even tries to create it is something why we should be paranoid also add an additional check in the test to make sure /var/empty stays that way.

Now the only ugliness is the (non-fatal) failure message, which for some reason does not occur when mounting the tmpfs inside a store path.

How about mounting it read-only?

@stale
Copy link

stale bot commented Jun 30, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 30, 2021
@Mic92
nixos/systemd-confinment: use /var/empty as chroot mountpoint
e12188c 
bind mounting directories into the nix-store breaks nix commands.
In particular it introduces character devices that are not supported
by nix-store as valid files in the nix store. Use `/var/empty` instead
which is designated for these kind of use cases. We won't create any
files beause of the tmpfs mounted.
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 1, 2021
@Mic92
Copy link
Member Author

Mic92 commented Jul 1, 2021

@ofborg test systemd-confinement

@Mic92
Copy link
Member Author

Mic92 commented Jul 1, 2021

Tests are working now!

@Mic92 Mic92 merged commit 8737aa9 into NixOS:master Jul 1, 2021
@Mic92 Mic92 deleted the confinment branch July 1, 2021 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aszlig aszlig aszlig requested changes

@dasJ dasJ Awaiting requested review from dasJ

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Mic92 @aszlig @SuperSandro2000