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/confinement: add i18n glibcLocales to confined services #87484

Closed
wants to merge 1 commit into from

Conversation

@martinetd
Copy link
Contributor

martinetd commented May 10, 2020

Motivation for this change

Programs within the chroot will have pkgs.glibcLocale but not the
environment-defined $LOCALE_ARCHIVE, leading to subtle bugs that can be
hard to figure.

Always include i18n locales to avoid thoses, there already is another
locales anyway, and add a test to make sure 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)
    New test fails without patch and works with patch.
  • 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 @aszlig

Copy link
Member

aszlig left a comment

Thanks a lot for your contribution.

However, I'm personally not in favour of this, since at least I tend towards having only the Nix runtime closure available and be explicit about the additional paths required.

That said, we already had requests to include more stuff into the chroot by default already, which is for example why the fullUnit option came to be. While glibcLocales isn't something I'd consider very critical - compared to such things like secrets passed via environment - I'd still opt for being as minimal as possible. My original implementation even had binSh set to null and mode to chroot-only, but since most people would probably prefer things to "mostly just work" by enabling confinement, they're set to "reasonable" defaults.

Which brings me back to glibcLocales: If we really want to include them by default, we really should make it possible to easily override it and also make it clear with a changelog entry that it's now included by default.

So with this change then we'd now have three options to set for the most restrictive confinement:

{ lib, ... }:

{
  systemd.services.foo.confinement = {
    binSh = null;
    packages = lib.mkForce [];
    mode = "chroot-only";
  };
}

As you can probably see, the amount of options you need to take care are increasing, but if it makes things less painful for people while also not annoying people wanting more restrictive settings too much, I'm somewhat okay with it. Needing to go the mkForce [] route on packages can also be used in a backwards-compatible way, so at least for my deployments it's just a matter of adding it everywhere without breaking existing deployments.

@@ -194,6 +195,9 @@ in {
echo "BindReadOnlyPaths=$storePath"
fi
done < "$closureInfo/store-paths" >> "$serviceFile"
# add i18n locale as well ($LOCAL_ARCHIVE)
echo "BindReadOnlyPaths=$i18nGlibcLocales" >> "$serviceFile"

This comment has been minimized.

Copy link
@aszlig

aszlig May 10, 2020

Member

This makes it very hard to exclude glibcLocales from the chroot and at least for my deployment I specifically want to avoid adding anything that is not explicitly needed, which includes locales.

Instead I'd rather provide a default definition for systemd.services.*.confinement.packages instead of adding it here unconditionally. If one would like to get rid of locales, it's just a matter of using mkForce or mkOverride on that option.

This comment has been minimized.

Copy link
@martinetd

martinetd May 10, 2020

Author Contributor

Sure, can do. Note that if you have anything linked to glibc you pull in pkgs.glibcLocales already by default so this really isn't adding anything to the chroot (just you now have two versions instead of one); but I can understand not having locales at all.

FWIW all my confinements are binSh = null and mode = "chroot-only" and I now have quite a few :/ Some services like postgres need /proc/self/fd or /dev/null but I'd rather just mount the required directory rather than the whole thing. fullUnit is that same that kind of makes confinement a bit moot if you expose all of this, ProtectSystem is the just about the same... (btw see #87420 about ProtectSystem, forgot to cc you)

Anyway, updating with default package set.

Programs within the chroot will have pkgs.glibcLocale but not the
environment-defined $LOCALE_ARCHIVE, leading to subtle bugs that can be
hard to figure.

Always include i18n locales to avoid thoses, there already is another
locales anyway, and add a test to make sure it works.
@martinetd martinetd force-pushed the martinetd:confinement-locales branch from 9a08afa to 231a8f5 May 10, 2020
@martinetd
Copy link
Contributor Author

martinetd commented May 10, 2020

Ok so force pushed an update.

  • I take it back, pkgs.glibcLocales doesn't seem to be pulled in by default, but it does get pulled instead of config.i18n.glibcLocales if a package uses it. Maybe remapping if it's found would make more sense instead?
  • I was mislead by options.confinement.packages which has a default value that does not seem to do anything!? So had to modify the list of packages by default built from service's exec lines, which means mkForce won't be as easy to use as you suggest. I would prefer some dynamic check that detects if pkgs.glibcLocales is included and also add i18n's glibcLocales if that is the case -- what do you think?
@martinetd
Copy link
Contributor Author

martinetd commented May 10, 2020

I was mislead by options.confinement.packages which has a default value that does not seem to do anything!? So had to modify the list of packages by default built from service's exec lines, which means mkForce won't be as easy to use as you suggest. I would prefer some dynamic check that detects if pkgs.glibcLocales is included and also add i18n's glibcLocales if that is the case -- what do you think?

Hmm I've got to take that back as well, I don't know why I was so sure some locales were included. I'm happy to drop this PR then, my basis for it really was that since there are locales we might want to add the normal ones, but I don't see locales in any of my chroot except postgres that I manually adjusted for it.

I'll let you close as you see fit :/ Thanks for the feedback and playing rubberduck.

@aszlig
Copy link
Member

aszlig commented May 10, 2020

* I was mislead by `options.confinement.packages` which has a default value that does not seem to do anything!? So had to modify the list of packages by default built from service's exec lines, which means mkForce won't be as easy to use as you suggest. I would prefer some dynamic check that detects if pkgs.glibcLocales is included and also add i18n's glibcLocales if that is the case -- what do you think?

Actually you're right, the packages option definition already is prepopulated by the exec options, so mkForce unfortunately won't do it either.

I'll let you close as you see fit :/ Thanks for the feedback and playing rubberduck.

Mhm, I think actually accomodating for both variants is probably not going to work without some hackery or adding an new dedicated option, or even add another mode besides chroot-only and full-apivfs.

I'll close this for now, feel free to reopen if either there is a compelling reason (like having locales in the chroot being very common) to add complexity or you've found a better way.

@aszlig aszlig closed this May 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.