interfaces: Add new systemd-user-control interface

[er-vin]

This is necessary to get a Plasma desktop to boot fully confined on an Ubuntu Core base. This is about adding a new interface for better support of a systemd + D-Bus activation session startup (requires listing and activating units)

[github-actions]

Everyone contributing to this PR have now signed the CLA. Thanks!

[er-vin]

For the record I did sign the CLA. I guess the check went through before it validated or such.

[jhenstridge]

This definitely needs careful attention from security. While it might be a bit contentious, it may still be the best way forward given the way different desktop environments are increasing their reliance on systemd as a user service manager.

Some other general points:

1. If the interface is focused on systemd access, then the name should reflect that. Maybe systemd-user-control or systemd-control.
2. It might make sense to go with systemd-control and have a plug attribute to say whether the snap wants access to the system or user instance of systemd. This might be worth doing even if we only have support for the user instance initially.

[alexmurray]

The new desktop-session-control interface allows to launch arbitrary services via the StartUnit method against org.freedesktop.systemd1.Manager (see https://www.freedesktop.org/software/systemd/man/latest/org.freedesktop.systemd1.html) - and hence run anything outside of confinement. I am not sure this fits with the existing snapd confinement model. The only similar thing we have at the moment is the desktop-launch interface - but this only allows to launch other snaps and this launching is mediated by snapd. Whereas this proposed interface is able to launch anything, outside of any mediation by snapd.

So at the bare minimum this interface needs to be super-privileged - BUT even then I am not sure if it fits in the existing paradigm for snapd. @pedronis can you comment?

The changes to shutdown, system-observe and upower-observe all seem reasonable though.

[er-vin]

This definitely needs careful attention from security. While it might be a bit contentious, it may still be the best way forward given the way different desktop environments are increasing their reliance on systemd as a user service manager.

Yes indeed. Upstream desktop projects are moving to systemd to drive session startup.

Some other general points:

1. If the interface is focused on systemd access, then the name should reflect that. Maybe `systemd-user-control` or `systemd-control`.

2. It might make sense to go with `systemd-control` and have a plug attribute to say whether the snap wants access to the system or user instance of systemd. This might be worth doing even if we only have support for the user instance initially.

So I tried going the systemd-control + attribute route and finally went back to systemd-user-control. In term of use in the snapcraft.yaml files the attribute wasn't really ergonomic I think. You'd have to declare under another name which would realistically end up being systemd-user-control (resp. systemd-system-control). Not much added value on that side.

Also on the implementation side it led to more complexity and more code than having two separate interfaces sharing the common parts. So I backtracked to just systemd-user-control.

[zyga]

I had a look at the new interface as well as at the other changes. I think the new interface should be split out to a separate PR so that security can focus on that part and we can arrive on a conclusion.

In the end the new interface also needs to be documented on the forum, if that's already done please ignore this part of the reivew.

[er-vin]

I had a look at the new interface as well as at the other changes. I think the new interface should be split out to a separate PR so that security can focus on that part and we can arrive on a conclusion.

I extracted the commits unrelated to systemd-user-control in #13947. Should I wait for it to go in before rebasing the current PR on top of the new master?

In the end the new interface also needs to be documented on the forum, if that's already done please ignore this part of the reivew.

I'll do that last once the dust settle though, it changed drastically once already.

[ZeyadYasser]

Adding to the concerns of @jhenstridge @alexmurray I am a bit worried about the impact of supporting StartUnit and StartTransientUnit in the new desktop-session-control interface.

snapd and snap run depends on StartUnit and StartTransientUnit for monitoring and resource control which would cause two annoyances:

• Naming conflicts of transient cgroups will be possible which would interfere with snapd (especially refreshes)
• Snaps will always be started in a two level nested transient cgroup (one for KDE and the other for snapd)
  • I am not sure about the performance impact or side effects of this

@pedronis opinion on this is definitely needed.

[er-vin]

For the record, I just rebased on latest master. I also made sure there was only one commit related to the introduction of the systemd-user-control interface. Should make it all easier to review.

[er-vin]

In the end the new interface also needs to be documented on the forum, if that's already done please ignore this part of the reivew.

This wasn't the case yet, it's now done:
https://forum.snapcraft.io/t/the-systemd-user-control-interface/40201

[er-vin]

* Naming conflicts of transient cgroups will be possible which would interfere with snapd (especially refreshes)

Indeed, it opens the door to this. Note however that the consensus seems to be to not make this interface widely available but only for a couple of desktop session snaps. I wouldn't expect those to actively seek such naming conflicts in practice (on the contrary).

* Snaps will always be started in a two level nested transient cgroup (one for KDE and the other for snapd)
  * I am not sure about the performance impact or side effects of this

Are you sure about this second claim? Indeed I see ways where this could be possible but I don't think this is as much a given as your "always" seems to imply. At least in practice I don't think I'm seeing this with the KDE Plasma session we've been working on. Snap related cgroups didn't get extra nesting AFAICT from systemd-cgls.

If you want me to test extra things to check this further feel free to ask I'll gladly try.

[jhenstridge]

To help with getting this approved, it would be useful to know what the various systemd methods are being used for. Looking at the AppArmor, it looks like there's roughly four categories:

1. SetEnvironment, UnsetEnvironment, and UnsetAndSetEnvironment: I'd assume this is primarily to communicate variables like DISPLAY and WAYLAND_DISPLAY to other processes in the session that are not directly run by the shell.
   2.Reload, ListUnitFiles, and ListUnitFilesByPatterns: introspect what units exist in the session.
2. ResetFailed and StartUnit: start services/targets installed on the system. Maybe to phase parts of the startup, or manage Xwayland startup? Also, would it make sense to include StopUnit and RestartUnit here?
3. StartTransientUnit: I'm guessing that this is for application launch, and is probably the most contentious as it directly supports arbitrary code execution. Would the desktop-launch interface allow you to do without this one? We've got some patches in the core-desktop snapd branch to structure desktop files for snaps so that running their Exec line will work from a snap that plugs this interface.

If we can eliminate (4) and have a rationale for the others, I think it will be much easier to sell this interface.

[er-vin]

To help with getting this approved, it would be useful to know what the various systemd methods are being used for. Looking at the AppArmor, it looks like there's roughly four categories:

1. `SetEnvironment`, `UnsetEnvironment`, and `UnsetAndSetEnvironment`: I'd assume this is primarily to communicate variables like `DISPLAY` and `WAYLAND_DISPLAY` to other processes in the session that are not directly run by the shell.

Correct. This is done by the bootstrap process to push its environment to be available to other processes (it's a bit more than DISPLAY and WAYLAND_DISPLAY it also tunes other variables.

   2.`Reload`, `ListUnitFiles`, and `ListUnitFilesByPatterns`: introspect what units exist in the session.

Correct. This is exactly what it's used for.

2. `ResetFailed` and `StartUnit`: start services/targets installed on the system. Maybe to phase parts of the startup, or manage Xwayland startup?

Yes, this is mainly for the bootstrap process to StartUnit the target describing the fully started desktop. Then it mostly gets out of the way.

Also, would it make sense to include StopUnit and RestartUnit here?

Indeed, makes sense when not on the happy path. I'll add those.

3. `StartTransientUnit`: I'm guessing that this is for application launch, and is probably the most contentious as it directly supports arbitrary code execution. Would the `desktop-launch` interface allow you to do without this one? We've got some patches in the core-desktop snapd branch to structure desktop files for snaps so that running their `Exec` line will work from a snap that plugs this interface.

Right now the StartTransientUnit path is enforced via the bootstrap process using an environment variable. I might be able to restore the old fork path, I'll test it and see how viable this is.

Here is the rub though: It's unclear how long the fork path will stay in place, if it gets fully decommissioned the workaround I'll test will be rendered useless.

If we can eliminate (4) and have a rationale for the others, I think it will be much easier to sell this interface.

Hopefully I answered what you had in mind.

[er-vin]

Alright, managed to get it working without StartTransientUnit. As mentioned earlier this might break later on though if the fork based code path gets removed.

[ZeyadYasser]

I did another pass. This looks really good, Thank you!

I have a few comments, mostly regarding tests and static check failures. ./run-checks should run both unit tests and static checks.

[ZeyadYasser]

Thank you!

[ernestl]

The approach requires additional discussion, marked block until we have feedback.

[soumyaDghosh]

Other than this, this plug has another usecases. For apps like system monitors. This plug still needs some for methods to be added though, but still this would be really helpful. Instead they would come up and ask for classic confinement as seen in the forum. cc @alexmurray

https://forum.snapcraft.io/t/classic-confinement-request-for-mission-center/40451/25

[jhenstridge]

This isn't a full review: just a few things I noticed while testing the branch.

[ernestl]

Needs more discussion

[zyga]

I don't know if there are remaining discussions to be had butfrom my point of view, this looks fine as a super-privileged interface. I am not aware of details as to which set of methods exactly is needed so I trust other reviewers and the original author on this.

I had a look at tests and test failures and nothing particular stands out.