interfaces: add an interface for use by thumbnailer

[jhenstridge]

This pull request adds a snapd interface for use by thumbnailer. On Ubuntu Phone, thumbnailer-service was a D-Bus service that provided a shared thumbnail cache to applications: both for local pictures/music/videos, and for online album art. It ran unconfined, and used the aa_query_label() API to determine whether the caller had permission to read the file it had asked to thumbnail. Moving forward to Ubuntu Personal, we want to confine the daemon.

While the generic dbus interface was enough to get the online album art feature working, but to thumbnail local files we need to be able to read files owned by the caller. So this interface does two things:

1. When connecting this interface, add an AppArmor snippet on the slot side that grants access to the plug snap's data directories.
2. Give the slot snap the permissions needed to actually use the aa_query_label() API.

This allows thumbnailer-service to read data belonging to snaps that choose to use thumbnailer (and only those snaps), which seems like an improvement over running unconfined. It is a pretty wide ranging permission though, so ideally only the thumbnailer package would be allowed to offer this slot by default. I'm not sure how easy that is though.

There are still a few open issues even with the changes in this pull request applied:

1. While this solves the problem of reading files, we're still running into some problems with reading files under the home directory due to aa_query_label() not handling rules that use the owner modifier (see bug 1620635).

2. We're also still using the generic dbus interface at the moment. I wonder if it would make sense to fold the required D-Bus permissions into this snap to reduce the number of connections that need to be made between snaps.

[mvo5]

Code looks good! This needs a review from security (e.g. @jdstrand )

[jdstrand]

Looks good, thanks! Just a couple of small changes and a decision on to use this with the 'dbus' interface or not.

[jdstrand]

Please add: deny-connection: true. This will make it so a snap declaration for slot implementations to by connected to with 'plugs: [ thumbnailer ]' which enforces your desire "only the thumbnailer package would be allowed to offer this slot by default". Only the official thumbnailer package will have this snap declaration.

[jdstrand]

It's 2017.

[jdstrand]

Can you adjust this to be:

# Description: allow the thumbnailer service to use the aa_query_label API. This
# discloses the AppArmor policy for all processes.

[jdstrand]

Is @{PROC}/@{pid}/mounts required to use libapparmor? If so, it is fine here but we also have the 'mount-observe' interface if that would be helpful.

[jhenstridge]

It is used by aa_find_mountpoint(), which is called by both aa_is_enabled() and aa_query_label:

http://bazaar.launchpad.net/~apparmor-dev/apparmor/master/view/head:/libraries/libapparmor/src/kernel.c#L50

Interestingly, there is a wrapper that falls back to a default location for securityfs in a different part of the library, but it isn't used by the APIs I'm using:

http://bazaar.launchpad.net/~apparmor-dev/apparmor/master/view/head:/libraries/libapparmor/src/kernel_interface.c#L45

[jdstrand]

This is creative and I agree it is a nice improvement over Touch. Curious, are there particular directories we could/should limit this to? Eg, I know ~/.cache/thumbnails exists OnClassic, so a rules like this could be interesting instead:

owner @{HOME}/snap/###PLUG_SNAP_NAME###/*/.cache/thumbnails/** r,

Enumerating all those paths is perhaps too cumbersome and error-prone, but I thought I'd at least ask.

[jhenstridge]

The idea of these rules is to let thumbnailer-service read the files an application might ask to for thumbnails of. An image gallery app might put jpeg images in $SNAP_USER_COMMON, so thumbnailer-service would need to be able to read that snap's directory.

The ~/.cache/thumbnails directory is not particularly interesting: it is part of the Freedesktop thumbnailer spec, which we aren't using since we didn't want many per-snap caches (instead, thumbnailer-service manages a single fixed size cache).

[jdstrand]

"We're also still using the generic dbus interface at the moment. I wonder if it would make sense to fold the required D-Bus permissions into this snap to reduce the number of connections that need to be made between snaps."

My gut feeling is that since the developer has to specify the thumbnailer interface with plugs, it would be nice if the dbus interface also didn't have to be specified so in that light, moving those accesses to this interface makes sense. That said, we would plan to allow auto-connection of this interface for the Canonical snap since it will have all the application isolation safeguards in place, so this would not be a pain point for the user.

Do you need DBus activation? #2592 should be landing soon and if you need DBus activation, we would want to either use the dbus interface or make updates to snapd so interfaces like this one could leverage it since PR#2592 is currently specific to the dbus interface.

[michihenning]

Yes, we've been hanging out for dbus activation for a service on the session bus. Currently, we have to start the service by hand because the "daemon:" entry only works for the system bus right now (AFAIK).

[jhenstridge]

thumbnailer-service is currently set up to rely on activation: we designed it to exit after a period of inactivity on the assumption that it will be started again when next needed.

Maybe it would make sense to separate out the "permission to acquire bus name XXX" feature from the "allow D-Bus communication between these two snaps" feature?

[jdstrand]

@jhenstridge, possibly, but I think it is too soon to tell. I mean, if you are acquiring a name, it is because you want to offer a service, so tying that to a connection for communicating does make sense. Let's see how people are using it in practice and see what needs refining.

[jdstrand]

Since you are currently depending on the dbus interface, there is no seccomp policy to add. If you move to using this without dbus, you would typically want to add all the syscalls for recv* and send*. However, don't; #2841 is about to land and adds all these by default.

[jdstrand]

FYI, #2841 is merged so lack of additional seccomp policy is +1

[jhenstridge]

I think this covers everything @jdstrand mentioned in his review. I'm not sure what happened with the spread tests: they don't look like they have anything to do with the changes in my branch, and may be from external factors. Each failure looks something like:

2017/02/15 10:34:20 Error executing linode:ubuntu-16.04-64:tests/main/refresh:strict_remote : 
...
+ MATCH 'All snaps up to date.'
+ snap refresh test-snapd-tools core
error: pattern not found, got:
core (edge) 16.04.1 from 'canonical' refreshed
-----

[jdstrand]

I think the interface is fine to merge as is. It sounds like for thumbnailer to be usable at all, it needs DBus activation and thumbnailer can use #2592 when it is merged (it is close).

If you decide you'd like to move all the dbus accesses and activation into this interface, you can still do that in a follow-up PR.