I’d like to be able to publish the plasma-desktop-session snap, which triggered a few problems in the automated review.
The snap name is registered to KDE, and the source is currently available at /neon/snap-packaging/plasma-desktop-session on the KDE forge. it is intended for use in an Ubuntu Core variant together with the plasma-core22-desktop base snap. The snap implements the KDE Plasma session for the user, running KWin as a Wayland compositor and a few related services.
It triggered the following failures:
human review required due to 'allow-installation' constraint (bool) declaration-snap-v2_plugs_installation (polkit-agent, polkit-agent) since the session needs to be able to request user passwords, it can’t be impersonated by wayland clients though since it’s the one in control of the compositor
human review required due to 'allow-installation' constraint (bool) declaration-snap-v2_plugs_installation (desktop-launch, desktop-launch)we want the session to be able to launch application snaps running under their own snap confinement. This interface lets us achieve that.
human review required due to 'allow-installation' constraint (bool) declaration-snap-v2_plugs_installation (shell-config-files, system-files)This grants access to some additional paths we needed to get gnome-shell and plasma to run. We’ll eventually restrict this later on. If it makes things easier, I only need to have installation allowed: I can handle connection through the matching gadget snap.
human review required due to 'allow-installation' constraint (bool) declaration-snap-v2_plugs_installation (dot-hidden, personal-files) (and similar for dot-local-share-nautilus and shell-session-local-files) likewise this is extra paths we need for now but hope to restrict later on
human review required due to 'allow-installation' constraint (bool) declaration-snap-v2_plugs_installation (snapd-control, snapd-control) This is in place to allow the terminal emulator in the snap to talk to snapd and to allow Discover (the KDE app to manage apps) to talk to snapd as well.
human review required due to 'allow-installation' constraint (snap-type) declaration-snap-v2_slots_installation (desktop, desktop) Since the snap implementing the desktop session is assumed to provide it.
human review required due to 'allow-connection' constraint (interface attributes) declaration-snap-v2_slots_connection (dbus-*, dbus)We have a number of dbus-* slots on the snap to allow it to provide various D-Bus services required to run the desktop session.
unknown plugs interface name reference 'systemd-user-control' lint-snap-v2_app_plugs_plug_reference (plasma-desktop-session, systemd-user-control) This one is still in review but will be necessary for sessions where the startup is driven by systemd. The patch in review is at snapcore/snapd/pull/13920 on GitHub.
(3) +1 for use of system-files but I recommend splitting shell-config-files into multiple related system-files groupings
(4) +1 for use of these personal-files interfaces. do they require auto-connection?
(5) I saw snapd-control is not included in the similar ubuntu-desktop-session snap. Does this snap behave in a different way requiring this interface. I just want to be thorough since it is a very privileged interface
About (3) thanks for the advice, I’ll see to improve it in later revisions.
Regarding (4), we’d need the auto-connection indeed.
As for (5) you are correct that snapd-control isn’t included in ubuntu-desktop-session. The main difference between the two is that ubuntu-desktop-session doesn’t provide anything to manage the snaps, but plasma-desktop-session comes with KDE Discover (via its base) which allows to install extra snaps. This is something we hope to be able to split out in its own snap at some point but it’s not a given, there are integration points we might miss if it gets splitted. So it’s definitely needed for now.
(5) given your reasoning and use for the kde discover store integration, I am leaning towards a +1 but would like to hear other @reviewers opinions as it is a very privileged interface.
+1 from me for granting plasma-desktop-session auto-connection to following interfaces
polkit-agent
desktop-launch
system-files as defined in shell-config-files. Usually they should be split into multiple interfaces, in this case the set of files is similar to the shell-config-files already used by ubuntu-desktop-session, so I have no strong preference in this case.
presonal-files as defined for dot-hidden, dot-local-share-nautilus and shell-session-local-files.
snapd-control, I may also be fine as it looks as a legit use case for this snap. @alexmurray thoughts?
desktop
Regarding 8) it seems to be still under development, so I would prefer to wait until it is released
Wouldn’t that mean that every app running on top and utilizing that session snap inherits access to snapd-control and as such would run essentially unconfined ?
I’m not sure what you mean about “on top” here. In any case, this is about a plug for the session snap, so indeed if connected, all the apps within the confinement would have snapd-control access. Of course, this is a limited number of applications we’re talking about since on such a system beyond what’s needed to start and interact with the session we want the applications to come from other installed snaps in their own confinements.
What i mean is that snapd-control has not been designed to be used in content snaps and has to my knowlede also never been tested in that context, what happens if you open a snapped terminal on top of a session snap plugging that interface by default, what happens if I create an application snap and preload a hacked library replacing one of the ones in the session snap on application level, will the existing confinement be enough to protect a user from such scenarios…
We used to have a policy to never grant snapd-control to any snaps in the global store unless they are actually owned by Canonical and keep this interface otherwise reserved for brand stores where the owner of the store is topically the owner of the device the application runs on…
I’m not sure if that policy has changed though. The thing is that you are trying to use this interface in a context it was not designed for and is not actually tested in…
(The decision is in the end in the hands of the reviewers team though and they should know the current policies around this interface, i personally would not grant its usage unless there are at least some regular tests on the canonical side around this particular use case)
since we have the votes, I will go ahead and grant the interfaces discussed previously barring snapd-control (because of your most recent comment) and systemd-user-control (as it is still being reviewed by snapd and will need voting).
Before doing this, is connect or auto-connect desired for shell-config-files? Thanks!
Discussed with @jamesh today, would it be possible to also grant the systemd-user-control interface? We understand it’s not in snapd yet but this is blocking us for a proper user experience for now. Having it would help things greatly.
Also we introduced new D-Bus interfaces as the confinement is getting tighter. Could you also grant those?
To support Kevin here, it’d be really useful to have this included in the snap declaration for the package, despite the interface still being in review.
Due to the scope of access it gives, the base declaration sets allow-installation: false. So without the snap declaration overriding that, we can’t test out a published version of this snap.
hi @kottens, I have gone ahead and updated those dbus slots.
As for systemd-user-control, we can do a manual review and approve the snap so that it can be downloaded from the store. But since review-tools still doesn’t know about this interface, it will still flag it for manual review
To test it from your side, snapd will need to be patched locally with the systemd-user-control MR and adjusted so that allow-installation: true is temporarily declared. So that snapd will allow installation.
As for systemd-user-control, we can do a manual review and approve the snap so that it can be downloaded from the store. But since review-tools still doesn’t know about this interface, it will still flag it for manual review
So I’m supposed to poke you here every time we will upload a new version of the package even if the interfaces don’t change? (just making sure on the process you’ll expect)
To test it from your side, snapd will need to be patched locally with the systemd-user-control MR and adjusted so that allow-installation: true is temporarily declared. So that snapd will allow installation.
Sure thing.
@jamesh Any chance you could upload a new patched snapd integrating my outstanding patches + the adjustment to allow-installation as proposed by @cav ?
hi @kottens - yeah that is right. Once it is integrated into snapd, then we will merge the relevant changes to add that interface in review-tools in Launchpad.
Let me know when everything looks good to go and I’ll go ahead and manually approve.
Thanks again!
hello @cav
You can go ahead and manually approve the latest plasma-desktop-session snap. There will be a few more iterations in the coming weeks, but this would help to have this one from the store already.
Thanks!
