systemd confinement for services using systemd.packages (e.g. mpd)

[martinetd]

Describe the bug

Trying to use confinement on packages providing their systemd units with systemd.packages, for example mpd, fails with the following error:

system-units> ln: failed to create symbolic link '/nix/store/6wrmqdvjqb0ysh8v9mjvaiydbazzy4fk-system-units/mpd.service': File exists

This is because systemd-confinement and mpd both provide a mpd.service file through systemd.packages. (mpd got updated that way recently to use upstream's service file)

Steps To Reproduce

Steps to reproduce the behavior:

1. should probably reproduce with systemd.services.mpd.confinement.enable = true; on nixpkgs >= 746e627 (Jan 11)

Expected behavior

Should work regardless of original service? Or at least try to.

Additional context

Using upstream service directly is a bit annoying with confinement, as we can't inspect it to check for e.g. ProtectSystem or other incompatible options, but it should be possible to just mash all the files together as overrides.

In nixos/lib/systemd-lib.nix for systemd.units units we check if the original unit exists and if so install the unit as foo.d/override.conf, a similar check could be added to install overlapping units from systemd.packages as override as well.
Note care would need to be taken about the override name, because in this case we'd get 2 overrides, so the overrides will also need different names, so this is not trivial.

Alternatively, the confinement code could try to generate the list of bind paths not as a separate unit file but as a list to fill in systemd.services.<servicename>.serviceConfig.Bind(ReadOnly)Paths directly; that would avoid one level of collision and solve the issue at hand. there's quite a few readink calls involved so I'm not sure if that's possible however...

Notify maintainers

@aszlig (for confinement) -- letting you pull in more people if you'd like.

[aszlig]

In nixos/lib/systemd-lib.nix for systemd.units units we check if the original unit exists and if so install the unit as foo.d/override.conf, a similar check could be added to install overlapping units from systemd.packages as override as well.
Note care would need to be taken about the override name, because in this case we'd get 2 overrides, so the overrides will also need different names, so this is not trivial.

Hm, why not just put the confinement stuff in foo.service.d/confinement.conf in the first place? It wouldn't work if there is no foo.service file but OTOH, confinement without any other unit options set wouldn't make sense anyway, right?

Alternatively, the confinement code could try to generate the list of bind paths not as a separate unit file but as a list to fill in systemd.services.<servicename>.serviceConfig.Bind(ReadOnly)Paths directly; that would avoid one level of collision and solve the issue at hand. there's quite a few readink calls involved so I'm not sure if that's possible however...

Unfortunately, this would involve an IFD because we need to already know the runtime closure of the package(s) in hand.

[martinetd]

Hm, why not just put the confinement stuff in foo.service.d/confinement.conf in the first place? It wouldn't work if there is no foo.service file but OTOH, confinement without any other unit options set wouldn't make sense anyway, right?

If we can do that it'd work, yes. I don't think we should worry about the "no foo.service" case, it doesn't make sense without Start/Stop/whatever directives anyway.
Looking at generateUnits in nixos/lib/systemd-lib.nix I don't see how we could control the name to be confinement.conf directly as is, but perhaps you had an idea?

[aszlig]

I thought more about doing this:

diff --git a/nixos/modules/security/systemd-confinement.nix b/nixos/modules/security/systemd-confinement.nix
index 0e3ec5af323..f3a2de3bf87 100644
--- a/nixos/modules/security/systemd-confinement.nix
+++ b/nixos/modules/security/systemd-confinement.nix
@@ -175,8 +175,8 @@ in {
       serviceName = "${name}.service";
       excludedPath = rootPaths;
     } ''
-      mkdir -p "$out/lib/systemd/system"
-      serviceFile="$out/lib/systemd/system/$serviceName"
+      mkdir -p "$out/lib/systemd/system/$serviceName.d"
+      serviceFile="$out/lib/systemd/system/$serviceName.d/confinement.conf"
 
       echo '[Service]' > "$serviceFile"
 

The tests still succeed with that change. Can you please check whether it works for your mpd case?

[martinetd]

Oh, for some reason I was convinced systemd.packages would not preserve subdirectories.

Just checked, it works:

nixos# systemctl cat mpd | grep '# /'
# /etc/systemd/system/mpd.service
# /nix/store/9njxrh1gliicaikdhgl4z3lgpn6k0190-system-units/mpd.service.d/confinement.conf
# /nix/store/9njxrh1gliicaikdhgl4z3lgpn6k0190-system-units/mpd.service.d/overrides.conf

I'll let you open a PR with that?

thanks!

[aszlig]

I'll let you open a PR with that?

Thanks for testing, PR coming soon in #157840.