many: add support for session dbus services via snaps

[mvo5]

This adds support for dbus activation for snaps. This version is session bus only but it will be relatively easy to extend for the system bus. It implements the proposal from Jamie in #2592 (comment) and the snap.yaml for an application that implements io.snapcraft.SnapDbusService will look something like this (service: true is the key here):

name: dbus-service
slots:
  dbus-service-slot:
    interface: dbus
    bus: session
    name: io.snapcraft.SnapDbusService
    service: true
apps:
  dbus-service:
    command: bin/dbus-service
    plugs:
      - network
      - network-bind
    slots:
      - dbus-service-slot

LP: #1648990

[jdstrand]

IMO, we should remove 'desktop' here-- these aren't desktop files and this isn't limited to 'desktop' systems. Also, I know you probably modeled this name after the directory in /usr/share/dbus-1, but for clarity within snapd, I think this would be better named '/var/lib/snapd/dbus-1/session-services/'. Putting it in /var/lib/snapd/dbus-1 means we can add '/var/lib/snapd/dbus-1/system-services/' if we so choose down the line and keep snappy dbus-y things in one clear spot.

[jdstrand]

Please rename as SnapDBusSessionServicesFilesDir to make sure there is no confusion with system services.

[jdstrand]

Typo here. Should be 'Activatable'.

[jdstrand]

I don't think this should be tied to 'gui'. Most session services run in the background and don't have a gui. Some session services are dbus activatable and are started by guis accessing them, but that is a different thing.

[jdstrand]

I think here, in the MkdirAll above and in the RemoveSnapDBusServiceFiles call below you should think about when we might create a dbus service file for a system service (not that you have to implement that now, but see my later comments). That might be AddSnapDBusServiceFiles and RemoveSnapDBusServiceFiles taking an argument of 'system' or 'session' then picking the directory based on that. It might be something else. A comment in these places explicitly saying this is for session services and that we need to update these to support system services would be nice.

[jdstrand]

As written, if snapd install finds meta/gui/foo.service, it takes that and creates a session service file in /var/lib/snapd/desktop/dbus-1/services and allows the desktop file to set DBusActivatable. This is simple and it works to start the session service in devmode, but is lacking security policy updates that would allow it to run in strict mode. Also, looking at what can be declared in service files, I think that we can have snapd generate them and expose the options via yaml. The dbus specification (https://dbus.freedesktop.org/doc/dbus-specification.html) is not very clear on service files but bus/desktop-file.h from the dbus sources is. These are the supported directives:

• Name=
• Exec=
• User=
• SystemdService=

We want to use 'Name=' and 'Exec='. We can ignore 'User=' for now since session services don't use it (they are expected to run as the same user as the user's session) and system services are started as root via systemd (when we support per-snap users, we'll adjust the systemd service file for services, not a dbus serevice service file). We can ignore SystemdService= since it is used by dbus-daemon to tell systemd to start the service on its behalf. We will have dbus-daemon starting session services directly and systemd starting system services directly, so this isn't needed. It is important to remember that on series 16, we have systemd user sessions, but this isn't available in 14.04, so we should have dbus-daemon start session services itself rather than systemd.

I propose:

• move relevant parts of wrappers/service.go into interfaces/dbus/dbus.go (ie, the DBus security backend)
• have AddSnapDBusSessionServiceFile(snapInfo, name) create a service file:

[D-Bus Service]
Name=<name passed into AddSnapDBusSessionServiceFile>
Exec=<command from snap.yaml>

• keep DBusActivatable as something that is allowable in the desktop file
• remove 'daemon: dbus' from snap info since it is at this point vestigial code

With the above, we have the pieces in place to create session service files, but no way to declare they should be used or how to generate the security policy. I think we can look at the dbus interface and add a slot side attribute. We currently have:

name: (slot): well-known DBus connection name (eg, org.foo.bar)
bus (slot): DBus bus to use (ie, session or system)

We add:

service (slot): true|false. When true, the command is started as a DBus service. Defaults
to false (ie, if command uses 'daemon', started via systemd without using 'Type=dbus',
otherwise started by the user)

Then the dbus interface can be adjusted such that:

• if service is not specified, no changes to the current implementation
• if service == True and bus == 'session', call AddSnapDBusSessionServiceFile(s, name)
• if service == True and bus == 'system', when the systemd unit file is generated, use BusName=<name> and Type=dbus

With this, the dbus interface allows people to create session services started by dbus-daemon (that may be dbus activatable if the desktop file has DBusActivatable=true) and to prefer 'Type=dbus' for system services started by systemd. Future interfaces could leverage AddSnapDBusSessionServiceFile() as well for creating session services.

CC: @vosst, @niemeyer, @tyhicks, @ejratl

[mvo5]

@jdstrand Thanks for the excellent suggestions. I changed the code as you suggested.

Some limitations with this iteration:

• no dbus system service file generation yet
• the implementation is hackish (setupDbusservices), I think once #2613 lands this will become less ugly
• unit tests missing

[mvo5]

Fwiw, the 14.04 failure can be ignored, the packaging branch needs an update and then it should work.

[jhenstridge]

Would it make sense to also generate a SystemdService key in the generated D-Bus .service file?

If we're assuming systemd for system and user init, then this would reduce the number of code paths for launching confined daemons on the system.

[niemeyer]

Nice! A few details and LGTM:

[niemeyer]

Can we please use "dbus/services"? That "dbus-1" convention always feels awkward to me, and we don't have many reasons to reproduce it here I think. The compatibility issues and their solution will be the same as for every other service we hold data for.

[niemeyer]

This will need fixes elsewhere too.

[mvo5]

updated.

[niemeyer]

Our naming convention is generally:

s/Busconf/BusConf/
s/Busserv/BusServ/

Same below for remove, combineSnippets, addContent, etc.

[mvo5]

Thanks, fixed.

[niemeyer]

#2613 is now in. Seems okay to fix that in a follow up, though, assuming everything else is golden.

[mvo5]

Thanks, +1 for a followup

[niemeyer]

Shouldn't we allow "system" as well?

[mvo5]

I think we want that in the longer run, my main focus for this PR was to unblock people who need dbus activated services (I think unity and kde has some of those).

[jdstrand]

If you aren't going to support system now, can you add a TODO. Eg:

// TODO: we can eventually support 'system' by:
// 1. creating the SnapDBusSystemServicesFilesDir directory
// 2. writing the service file to SnapDBusSystemServicesFilesDir
//    when 'daemon' is set to 'dbus' (see validate.go)
// 3. add 'Type=dbus' and 'BusName=slot.Attrs["name"].(string)'
//    to the systemd unit when 'slot.Attrs["service"].(bool) == True'
//    and 'daemon' is set to 'dbus'

[niemeyer]

Even without #2613, can we please at least validate that setting there, and cross-link it here (mentioning this is being used with this purpose).

[mvo5]

I added checks that its really bool and also added a comment why we need this. Should I do more (I'm not sure if that addresses the cross-link comment).

[niemeyer]

Should be enough, thanks.

[niemeyer]

The %q/dir is probably redundant here. The error from MkdirAll will include the path name as well.

[mvo5]

thanks, removed

[niemeyer]

Why the change here? Previous version seemed cleaner than new one. Now the call site needs to know what "env" means (it's not an environment at all) and the curious place in which it will be inserted.

[mvo5]

That was from a previous iteration, this is fixed now.

[niemeyer]

Is the typo intended here?

[mvo5]

ups, I fixed that typo, the test was focused on the missing "DBus" in front, not so much on the typo :)

[jdstrand]

@jhenstridge re 'SystemdService key', I covered this in: #2592 (comment)

[jdstrand]

Thanks @mvo5 for taking my suggestions and implementing this. +1, but please add the comments I suggested for clarity and next steps.

[jdstrand]

Can you add a comment for what this function does? Eg:

// Create the DBus busconfig policy for the snap

[jdstrand]

Can you add a comment above this function. Eg

// Remove the DBus busconfig policy for the snap

[jdstrand]

@mvo5 - nitpick, this comment still didn't make it in. It looks like you added it in the wrong place (see below).

[jdstrand]

Can you rephrase this to be:

// combineSnippetsBusConf combines security snippets for busconfig
// policy collected from all the interfaces affecting a given snap into a
// content map applicable to EnsureDirState.

[jdstrand]

Can you add a comment above this. Eg:

// addContentBusConf creates/updates the xml busconfig policy for the snap

[niemeyer]

Shouldn't we allow "system" as well?

[mvo5]

I think we want that in the longer run, my main focus for this PR was to unblock people who need dbus activated services (I think unity and kde has some of those).

[jdstrand]

If you aren't going to support system now, can you add a TODO. Eg:

// TODO: we can eventually support 'system' by:
// 1. creating the SnapDBusSystemServicesFilesDir directory
// 2. writing the service file to SnapDBusSystemServicesFilesDir
//    when 'daemon' is set to 'dbus' (see validate.go)
// 3. add 'Type=dbus' and 'BusName=slot.Attrs["name"].(string)'
//    to the systemd unit when 'slot.Attrs["service"].(bool) == True'
//    and 'daemon' is set to 'dbus'

[jdstrand]

s/to get/to be/

[jdstrand]

Can you add a comment here. Eg:

// We set only 'Name' and 'Exec' for now. We may add 'User' for 'system'
// services when we support per-snap users. Don't specify 'SystemdService'
// and just let dbus-daemon launch the service since 'SystemdService' is only
// used by dbus-daemon to tell systemd to launch the service and systemd
// user sessions aren't available everywhere yet.

[jdstrand]

Please add:

// TODO: add 'dbus' when we support 'service: true' with 'bus: system' 
// in the dbus interface

[niemeyer]

Test failures are related:

2017/02/15 09:07:51 Error restoring linode:ubuntu-14.04-64:tests/main/dbus-activated : 
-----
+ killall dbus-daemon
+ apt autoremove -y dbus-x11
E: Command line option 'y' [from -y] is not known.
-----

[jdstrand]

This comment should be:

// Remove the DBus service file for the snap

[mvo5]

This will not work with re-exec, i.e. this config file needs to be in the regular deb package right now and if snapd just re-execs it won't be there. So we probably need to add code that writes it if its missing. Which has the downside that it will also mean that we leave files behind when snapd gets removed.

[niemeyer]

That's already the case with pretty much all of the security backends, and we actually already have a dbus backend that is tasked precisely with writing out that sort of file, and we ought to be removing these already when the package goes away (are we?).

[mvo5]

This is now auto-generated if missing

[mvo5]

Closing for now as this needs further work

[mvo5]

Reopening @morphis was interessted.

[jdstrand]

This is looking really good. Thanks for picking this up! Few open questions inline.

[jdstrand]

Can you add a comment along the lines of:

# Use 'dbus/services' to mirror /usr/share/dbus-1/services for session
# services. With system services, will use 'dbus/system-services'.

[mvo5]

Good point, added

[jdstrand]

I don't think "service" is the right term. Anything that slots the dbus interface is a "service", it is just that currently with system services they are automatically started and with session services they are not. If we were going to go with an attribute at all, I would suggest "activatable" and verify that it is only used when "bus" is "session".

That said, we may be able to do better. What if instead of adding an attribute we look at the surrounding yaml. For example, if "bus" is "session" and "daemon" is present, create the session service activation files, otherwise, operate how we do now. This should improve the developer experience and make things "just work" the way people expect.

[jdstrand]

Perhaps change this to be:

// System services get system bus policy

[mvo5]

Nice, updated

[jdstrand]

and change this to:

Session services get session activation bus policy

[mvo5]

👍

[jdstrand]

Perhaps use else if? (nitpick only)

[jdstrand]

This seems important before landing, no?

[morphis]

How can we reliable implement this? Is there any way to query dbus for this as I think examining available configuration files isn't enough. org.freedesktop.DBus.ListActivatableNames looks like a good candidate for this. I am adding godbus to the snapd source tree already with #3260 so would be fine to add it with this PR too.

[jdstrand]

The FIXME is for if a snap has taken it, not if anything has taken it. snapd has all the information for this: specifically all the snaps on the system with connected dbus interfaces and the attributes of those interfaces.

I don't think we want to query DBus only if we want to make this generalizable beyond activatable names because it would be useful if a non-activatable names were also checked. Right now, those snaps simply fail to start, but we could do something better if we were smarter with existing connections. snapd could additionally look at ListActivatableNames for the activated services to check for non-snap activated service collisions.

[jdstrand]

This is an interesting requirement because it forces that the snap.yaml must be of the form:

slots:
  dbus-session:
    ...
apps:
  foo:
    command: ...
    slots: [ dbus-session ]
  bar:
    command: ...

but thus far yaml has allowed specifying slots (and plugs) only in the top-level:

slots:
  dbus-session:
    ...
apps:
  foo:
    command: ...
  bar:
    command: ...

where the security policy for 'dbus-session' is given to both foo and bar. What is different here is that there can be only one activation file since we must map the 'Name' in the service file/interface attribute to a specific 'Exec' in the service file. I can't think of a way to make it cleaner than what you have, except to add a comment above saying:

# Because activation service files necessarily must map one DBus well-known
# name to one command, enforce that constraint in the interface here.

I sorta feel like the error message should be improved, but not sure to what. Perhaps add the attribute "name" so it is clearer?

[jdstrand]

This checks for snapd.conf and exits, but snapd is not creating it. Is that because snapd distro packaging will always ship this file? If so, then with how the code is written, snapd-reexec.conf will only ever be written if snapd.conf for some reason doesn't exist. While I see that in your postrm you cleanup this file on purge (great!), for systems only upgrading, if a later version of snapd then comes that provides snapd.conf, the previously created snapd-reexec.conf will never get updated or used, potentially causing confusion. Is snapd-reexec.conf meant to be preferred over snapd.conf when re-execing? If so, this code doesn't do that.

How is this supposed to work with packaging? Perhaps a comment in the code saying as much would help....

[jdstrand]

setupBusServ is too generic, no? Right now this is only for session activated services, but the name of this function implies it could be for any dbus service. Perhaps setupBusActivatedSessionServ?

[jdstrand]

Same here for removeBusServ name being too generic.

[jdstrand]

Can you adjust this comment to be:

// deriveContentBusConf combines security snippets for busconfig
// policy collected from all the interfaces affecting a given snap into a
// content map applicable to EnsureDirState.

[mvo5]

Thanks, added

[zyga]

A few things I'd like to see changed. Let me know if you need a hand, I will gladly make those changes.

[zyga]

Could we use osutil.EnsureDirState for this? This will allow us to just have a simple call here that ensures the correct content is in that file.

[mvo5]

This is done now.

[zyga]

Could you please move this to a helper similar to deriveContentBusConf?

[mvo5]

I reworked the code to do that now.

[zyga]

Are all security tag valid systemd unit file names?

[morphis]

If not we can pipe the security tag through systemd-escape, which is maybe safe in any case when we generate a systemd unit name.

[mvo5]

The files written here are dbus service files. The spec https://dbus.freedesktop.org/doc/dbus-specification.html has no restrictions on the name AFAICS.

[zyga]

Note that we have systemd.Service and the systemd backend as well. We could perhaps reuse some of those here.

[mvo5]

In this particular case we write a dbus service file which is different from a systemd unit file.

[pedronis]

see discussion in #3323 whether we need to tweak wrappers.IsService here

[mvo5]

@pedronis I think it does make sense to make wrapper.IsService() return true for dbus activated services. This way we also avoid writing the /snap/bin/svc binary and avoid polluting the namespace of PATH with those.

[mvo5]

Closing for now as this needs more work but there is no time available right now, I will reopen once I get the chance to work on it again.