Manual review of ubuntu-desktop-session snap

store-requests
#1

I’d like to be able to publish the ubuntu-desktop-session snap, which triggered a few problems in the automated review.

The snap name is registered to canonical, and the source is currently available at https://github.com/canonical/ubuntu-core-desktop/tree/main/ubuntu-desktop-session. it is intended for use in an Ubuntu Core variant together with the core22-desktop base snap. The snap implements the graphical login session for the user, running gnome-shell as a Wayland compositor and a few related services.

It triggered the following failures:

  1. interface 'polkit-agent' not found in base declaration declaration-snap-v2_plug_known (polkit-agent, polkit-agent)
    unknown interface 'polkit-agent' lint-snap-v2_plugs (polkit-agent, polkit-agent)

    this is an interface that has not yet been merged to snapd master here: https://github.com/snapcore/snapd/pull/10598. The interface should probably be treated as fairly sensitive as snaps plugging the interface will be dealing with user passwords. In this case it is appropriate, as gnome-shell is in a position to securely take input without being impersonated by Wayland clients.

  2. human review required due to 'allow-installation' constraint (bool) declaration-snap-v2_plugs_installation (desktop-launch, desktop-launch)

    we want the shell to be able to launch application snaps running under their own snap confinement. This interface lets us achieve that.

  3. 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 to run. I’d like to eventually drop this plug and add its access rules to some other interface. If it makes things easier, I only need to have installation allowed: I can handle connection through the matching gadget snap.

  4. 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. I can probably drop this for now if it makes review easier.

  5. human review required due to 'allow-installation' constraint (snap-type) declaration-snap-v2_slots_installation (desktop, desktop)

    This relies on another not-yet-merged snapd change to allow application snaps to provide a desktop interface slot: https://github.com/snapcore/snapd/pull/10753. As there is no implicit desktop interface slot on Ubuntu Core systems, the intent is that the snap implementing the desktop shell would provide it.

  6. 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.

  7. use of 'daemon: dbus' requires 'bus-name' lint-snap-v2_dbus_bus-name_required (*)

    This is a case where the review-tools checks do not line up with snapd’s behaviour. If the daemon specifies one or more dbus slots via activates-on, then the bus-name property is optional: the bus name from the last activates-on slot will be used if it is absent. That is the case for all the daemons in the snap triggering this failure.

1 Like
Request to allow connection to desktop and wayland slots of ubuntu-desktop-session snap
#2

So I am a bit wary of approving a snap which has interfaces that are not yet merged into snapd - particularly polkit-agent (1.) is still in draft and the desktop interface slot part too (5.) - how certain are we that these will see the light of day?

For the points in 7. I have submitted a MR for review-tools which should address this - https://code.launchpad.net/~alexmurray/review-tools/+git/review-tools/+merge/431324

For 2. this look reasonable.

I agree that 3. feels like it should be in some snapd interface rather than system-files.

  1. is obviously quite privileged but I think this makes sense - if this is not granted / used are users able to manage snaps otherwise?

  2. sounds reasonable too.

So whilst we can look at granting some of these, I am not sure about all of them - can you provide any more info on whether these required changes are likely to land in snapd?

#3

For (1) and (5), I’ve still got work to do to get those PRs ready to merge. They’re pretty critical to getting this Ubuntu Core variant running, so I am confident that they will make it in in some form. I also think they are likely to be “super privileged”, with the base declaration disallowing installation without an override from the store.

I don’t foresee the interface naming changing: for the desktop slot, this needs to be as is to match the plugs on existing app snaps. For polkit-agent, the name seems fairly descriptive and is orthogonal to the existing polkit interface.

For (3), using system-files was a stop gap. As I said earlier, I’d be happy if the plug passed review but didn’t auto-connect. We can make it connect in our image via the gadget.yaml file.

For (4), I’ve pushed a new revision of the snap without the snapd-control plug. It’s slightly less convenient for the initial testing phase, but not insurmountable. We’d expect regular users to install new snaps via the snap-store snap, which already has the appropriate declarations.

#4

@alexmurray: so is there anything more I can do to get this review moving? This is the main snap where being published on the store would help (so we can reference the snap ID in other parts of the image).

#5

Ok, for (1) and (5) let’s assume these will get merged - +1 from me for use-of both and auto-connect of polkit-agent.

For (7) we are waiting on an updated review-tools to be deployed to the store (ping @roadmr :slight_smile: )

For (2), +1 from me for use-of and auto-connect of desktop-launch as this is one of the primary purposes of this snap.

For (3), +1 from me for use-of but not auto-connect of system-files (whilst I don’t love the generic name shell-config-files as this gives no indication what is actually granted here, since this is transitory until these get added to some other interface, I don’t think it is worth bikeshedding over).

For (6) I have just granted the remaining DBus names.

Can other @reviewers please vote too?

2 Likes
#6
  1. +1 for auto connecting pollkit-agent a well known shell to this.
  2. +1 for allow-installation
  3. +1 for allow-installation
  4. I abstain from this one; I believe we need some form of policy around this from the security team first
  5. abstain
  6. +1 for allow-installation provided that we document the slots explicitly; I took a look at the current snapcraft.yaml and they look reasonable
  7. +1; but this looks like a bug fix request
#7

+2 votes for auto-connect of polkit-agent, system-files named shell-config-files for read access to the various paths above, desktop-launch and for slotting a desktop slot. This is now live. Note the snap still fails automated review as review-tools has one final complaint:

interface 'polkit-agent' not found in base declaration declaration-snap-v2_plug_known (polkit-agent, polkit-agent)
use of 'daemon: dbus' requires 'bus-name' lint-snap-v2_dbus_bus-name_required (gnome-terminal-server)
use of 'daemon: dbus' requires 'bus-name' lint-snap-v2_dbus_bus-name_required (nautilus-service)
use of 'daemon: dbus' requires 'bus-name' lint-snap-v2_dbus_bus-name_required (xdg-desktop-portal)
use of 'daemon: dbus' requires 'bus-name' lint-snap-v2_dbus_bus-name_required (xdg-desktop-portal-gtk)
use of 'daemon: dbus' requires 'bus-name' lint-snap-v2_dbus_bus-name_required (xdg-document-portal)
use of 'daemon: dbus' requires 'bus-name' lint-snap-v2_dbus_bus-name_required (xdg-permission-store)
unknown interface 'polkit-agent' lint-snap-v2_plugs (polkit-agent, polkit-agent)

As noted above, these dbus errors will be fixed once the store team has updated the review-tools deployment.

But the 2 polkit-agent errors can only really be fixed by getting the polkit-agent PR merged into snapd upstream, then extracting the base declaration from this into review-tools and getting that deployed to the store. So for now I have manually approved the latest snap revision (5) until that is all done.

This is now live.

2 Likes