interfaces: add zigbee-dongle interface

[jocave]

An interface that allows a usb removable zigbee dongle to be exposed through plugs & slots.

At the moment this only supports one of these devices as indicated by the udev snippet.
Any plug which is connected to a slot of this type would gain the AppArmor permissions to read&write to the device node of any and all devices the interface supports (no matter how many are attached to the system).

[zyga]

As mentioned in private, my only issue with this pull request is that it grants access to all zigbee dongles. While I recognize the issue that currently we con't have a way to create dynamic slots (nothing like hot plug) I worry that later on we may have to say the zigbee-dongle interface is now constrained to a particular dongle (in the unlikely case of there being more than one). I'd prefer if we either 1) only allowed one (say, the one with the smallest serial number) or 2) renamed the interface to something broader like zibgee-dongles.

[jocave]

Happy to do the second option here

[zyga]

Marking as blocked because we really have to figure out something better to do with udev.

[zyga]

Per the online discussion. I'd like to merge this today and address udev issues that also affect modem-manager in a separate pull request.

[zyga]

whitelist this please

[niemeyer]

Why are we doing this instead of simply using the glob itself in the apparmor profile?

[niemeyer]

Note that this will break if the device is hot-plugged afterwards.

[niemeyer]

LGTM assuming that the glob situation is fixed per comment above.

[snappy-m-o]

Can one of the admins verify this patch?

[snappy-m-o]

Can one of the admins verify this patch?

[jocave]

@niemeyer I used this approach based on the following:

• I only want to grant access to devices that matched the udev snippet i.e. items that have been identified
  as a zigbee dongle
• I won't know the device enumeration until the kernel has created the device node
• I was advised that I shouldn't use paths containing symlinks in apparmor profiles

Hence this code means I only add dereferenced paths to devices that I know are zigbee dongles in to the apparmor snippet.

[jdstrand]

So, this might be the time to use the Udev backend for one of its initial design goals: adding devices to an app-specific device cgroup. This would address the concerns of @zyga and allow fine-grained control. Here is how it could work:

1. this interface has attributes for idProduct and idVendor (feel free to name the yaml keys something different of course)
2. when attributes are used, fill in ATTRS{idProduct} and ATTRS{idVendor} appropriately, add a TAG+="snap_connecting_app" for each connected plug. udev TAGs cannot use globs so you'll need a rule for each. The TAG to add is simply the security label for the plug replacing '.' with '_'. Use /dev/** for apparmor.
3. if it makes sense, when attributes are not used, don't add TAGs and simply add all known idProducts and idVendors for zigbee devices to zigbeeDonglePermanentSlotUdev and use /dev/zigbee/* for apparmor

What does this do and why does it work? When attributes are not used, then access is granted to all known zigbee devices and this access is enforced with the /dev/zigbee/* apparmor glob rule. When attributes are used, access is granted only to those devices matching the idProduct and idVendor and this is enforced by adding only those devices that are udev TAGged for this app to the snap's device cgroup (we use a /dev/** apparmor rule since we rely on the device cgroup for security instead of apparmor). In both cases hot plugging and unplugging should work as expected.

A consideration with this approach is that using this interface with other interfaces that grant access to /dev/* via apparmor means that those other devices won't be in the device cgroup. For example, suppose we have the following yaml:

plugs:
  - camera
  - zigbee
    id-product: "0003"
    id-vendor: "10c4"

With the above we end up with apparmor policy (which while /dev/video0 is redundant, is still valid):
/dev/video0 rw,
/dev/** rw,

but only the zigbee devices are added to the device cgroup so while apparmor allows access to /dev/video0, the snap won't be able to access /dev/video0 because /dev/video0 wasn't udev TAGged and therefore snap-confine didn't add it to the per-app device cgroup. Importantly, this issue will not be unique to the zigbee interface-- it will need to be fixed to support TAGging for any interface that intends to use device cgroups and I think will be particularly important for gadget snap device labelling, etc.

OTOH to fix this we would have to abstract out the /dev/ accesses that are sprinkled through the interfaces and for each encode both an apparmor rule and a udev snippet in the interfaces, then when generating the security policy detect when a device cgroup will be used and add the udev snippets when it is and the apparmor rules when not.

Since the default with no attributes is this PR, perhaps it would make sense to commit this as is (ie, just the /dev/zigbee/* apparmor rule) and then work through attributes and specific device assignment in a future PR. As such, I agree this should not be renamed zigbee-dongles.

[jocave]

16:20 <joc_> jdstrand: thanks for response on zigbee-dongle, can i just check one point
16:21 <joc_> jdstrand: if /dev/zigbee/ only contains symbolic links will the apparmor glob /dev/zigbee/* work
16:22 <joc_> jdstrand: i was under the impression it wouldn't, and was the reason why some of the other interfaces I looked at dereference paths that include symbolic links (e.g. bool-file)
16:35 joc_: no, it won't. apparmor resolves symlinks. as such, snappy will need to resolve those and add them like you said
16:35 joc_: for the no attributes case. for the with attributes case, that wouldn't be needed (since it is the device cgroup that does the enforcement)
16:39 <joc_> jdstrand: thanks for confirmation
16:40 joc_: note, for 'as-is' it won't work til you resolve those symlinks
16:41 based on my understanding of what you said is in that dir anyway
16:43 <joc_> jdstrand: the zigbeeDevPaths function evaluates the symlinks found in that dir and adds the resolved paths to the apparmor snippet
16:45 ah, cool

[snappy-m-o]

Can one of the admins verify this patch?

[snappy-m-o]

Can one of the admins verify this patch?

[jocave]

18:23 joc_: It should at least do what I suggested in terms of using apparmor rather than globbing the FS at connection time
18:24 joc_: But it doesn't look like implementing what jdstrand suggested would be hard either

[jocave]

I've made an attempt to implement what @jdstrand suggested on another branch. Currently missing
the correct security tag as not sure how to access that at the moment:

https://github.com/jocave/snapd/commits/interface-zigbee-dongle-with-cgroups

[jdstrand]

Heh, I just noticed that the irc paste from last month was wrong (what was said was correct, but who said it got mixed up). This is what it should be:

"16:20 joc_: thanks for response on zigbee-dongle, can i just check one point
16:21 joc_: if /dev/zigbee/ only contains symbolic links will the apparmor glob /dev/zigbee/* work
16:22 joc_: i was under the impression it wouldn't, and was the reason why some of the other interfaces I looked at dereference paths that include symbolic links (e.g. bool-file)
16:35 jdstrand: no, it won't. apparmor resolves symlinks. as such, snappy will need to resolve those and add them like you said
16:35 jdstrand: for the no attributes case. for the with attributes case, that wouldn't be needed (since it is the device cgroup that does the enforcement)
16:39 joc_: thanks for confirmation
16:40 jdstrand: note, for 'as-is' it won't work til you resolve those symlinks
16:41 based on my understanding of what you said is in that dir anyway
16:43 joc_: the zigbeeDevPaths function evaluates the symlinks found in that dir and adds the resolved paths to the apparmor snippet
16:45 jdstrand> ah, cool
"

[jocave]

After discussion with @morphis and @jdstrand we decided this interface was not necessary and the function it was intended to provide could be met in other ways (using other interfaces). Withdrawing this PR.