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/systemd-confinement: Fix DynamicUser support #64405

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

arianvp
Copy link
Member

@arianvp arianvp commented Jul 7, 2019

Problem was that if you enabled DynamicUser, it would remount
/ as read-only which wouldn't work, as we were using
the fact that / was read-write to set up all the mounts.

We work around this by actually making sure all the directories
that will be mount points exist apriori. For things
for which mount points are dynamically generated on the fly,
we use TemporaryFileSystem (cache, log, state).

We also use TemporaryFileSystm for /nix/store. However,
we might not actually need it, as we know all the mount points
apriori.

TODO:

  • See how we interact with APIVFS
  • See if we can get rid of the TemporaryFileSystem for /nix/store,
    because currently /nix/store itself is read/write in the container (it
    already was) and that's surely not something we want!
Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.

Problem was that if you enabled DynamicUser, it would remount
/ as read-only which wouldn't work, as we were using
the fact that / was read-write to set up all the mounts.

We work around this by actually making sure all the directories
that will be mount points exist apriori.  For things
for which mount points are dynamically generated on the fly,
we use TemporaryFileSystem (cache, log, state).

We also use TemporaryFileSystm for /nix/store. However,
we might not actually need it, as we know all the mount points
apriori.

TODO:

* See how we interact with APIVFS
* See if we can get rid of the TemporaryFileSystem for /nix/store,
because currently /nix/store _itself_ is read/write in the container (it
already was) and that's surely not something we want!
Make sure that /nix/store, /usr, /bin /var and friends are read-only
inside the container.

Especially for /nix/store this should defenitely be read-only as
otherwise we can break invariants (not good!).

/bin and /usr/bin being read-only also makes sense to me; however
it currently breaks some tests.  The test differentiated between
chroot-only mode and fullapivfs mode by testing whether it could
chown /bin.  We should test this in another way?

/var/{lib,cache,log} are also mount read only.  This means
the only way to create directories here is to use
StateDirectory/CacheDirectory/LogDirectory directives. This
is a tradeoff. Perhaps we should keep them read/write, as not
all our services use these options yet, and it might hinder
adoption of confinement. However when you enable confinement you
could as well just add these options.  We defenitely need to give
this some thought. Perhaps put it behind an option? I think
the others being read-only makes sense; but this one might
be a bit controversial
@arianvp arianvp force-pushed the fix-confinement-statedirectory branch from fad4c9d to d978228 Compare July 7, 2019 13:41
@gazally
Copy link
Contributor

gazally commented Jul 7, 2019

I rebased #63863 on this and enabled confinement with mode = "chroot-only". I had to add yggdrasil's config file to BindReadOnlyPaths, and then got this error:

Jul 07 08:23:13 sockeye systemd[1]: Starting Yggdrasil Network Service...
Jul 07 08:23:13 sockeye systemd[8035]: yggdrasil.service: Failed to set up mount namespacing: No such file or directory
Jul 07 08:23:13 sockeye systemd[8035]: yggdrasil.service: Failed at step NAMESPACE spawning /nix/store/xwbhik24q3f909mhygk7cyr9grnbxmas-unit-script-yggdrasil-pre-start: No such file or directory
Jul 07 08:23:13 sockeye systemd[1]: yggdrasil.service: Control process exited, code=exited status=226
Jul 07 08:23:13 sockeye systemd[1]: yggdrasil.service: Failed with result 'exit-code'.
Jul 07 08:23:13 sockeye systemd[1]: Failed to start Yggdrasil Network Service.

Here's the rebased branch and the only configuration needed is service.yggdrasil.enable = true if you want to try it out.

@arianvp
Copy link
Member Author

arianvp commented Jul 7, 2019 via email

@aszlig
Copy link
Member

aszlig commented Jul 8, 2019

@arianvp: Just came back from vacation and need to catch up on a few other things, can you ping me again once this is no longer WIP?

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 3, 2020
@stale
Copy link

stale bot commented Jun 5, 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 5, 2021
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

4 participants