interfaces/builtin: add bluez interface #802

ssweeny · Apr 6, 2016

First pass at adding the interface for bluez to make itself available
from a snap.

ssweeny · Apr 6, 2016

This patch comes out of a discussion with @zyga about adding dbus services to snappy.

zyga · Apr 6, 2016

zyga reviewed Apr 6, 2016

View changes

Add the copyright boilerplate please

zyga · Apr 6, 2016

whitelist this please

zyga · Apr 6, 2016

@jdstrand Can you have a look if this looks sane or not?

zyga · Apr 6, 2016

Travis complained about code formatting: https://travis-ci.org/ubuntu-core/snappy/jobs/121141259#L362

Please run go fmt to fix that. Common editors like vim and emacs have appropriate plugins for go support. Otherwise it is quite easy to miss.

jdstrand · Apr 6, 2016

jdstrand reviewed Apr 6, 2016

View changes

I'm not sure this is correct. Snappy does mediate connections between snaps but I think there is still value in differentiating between root and non-root users. I think the proper approach for now until we know more how this will work would be to require sudo for connecting to bluez.

Sounds ok to me. So lets drop the two policy statements.

jdstrand · Apr 6, 2016

I left a comment regarding the bus policy. In addition to that, this interface is going to need the following apparmor rules for the slot side (ie, what the server needs, taken from bluez.apparmor from the current 16.04 bluez5 snap):

# Description: Allow operating as the bluez service. Reserved because this
#  gives privileged access to the system.
# Usage: reserved

  network bluetooth,

  capability net_admin,
  capability net_bind_service,

  # File accesses
  /sys/bus/usb/drivers/btusb/     r,
  /sys/bus/usb/drivers/btusb/**   r,
  /sys/class/bluetooth/           r,
  /sys/devices/**/bluetooth/      rw,
  /sys/devices/**/bluetooth/**    rw,
  /sys/devices/**/id/chassis_type r,

  # TODO: use snappy hardware assignment for this once LP: #1498917 is fixed
  /dev/rfkill rw,

  # DBus accesses
  #include <abstractions/dbus-strict>
  dbus (send)
     bus=system
     path=/org/freedesktop/DBus
     interface=org.freedesktop.DBus
     member={Request,Release}Name
     peer=(name=org.freedesktop.DBus),

  dbus (send)
    bus=system
    path=/org/freedesktop/*
    interface=org.freedesktop.DBus.Properties
    peer=(label=unconfined),

  # Allow binding the service to the requested connection name
  dbus (bind)
      bus=system
      name="org.bluez",

  # Allow traffic to/from our path and interface with any method
  dbus (receive, send)
      bus=system
      path=/org/bluez{,/**}
      interface=org.bluez.*,

  # Allow traffic to/from org.freedesktop.DBus for bluez service
  dbus (receive, send)
      bus=system
      path=/
      interface=org.freedesktop.DBus.**,
  dbus (receive, send)
      bus=system
      path=/org/bluez{,/**}
      interface=org.freedesktop.DBus.**,

and the following apparmor rules for the plugs side (ie, what the client needs, taken from framework-policy/apparmor/policygroups/client from the current 16.04 bluez5 snap):

# Description: Allow using bluez service. Reserved because this gives
#  privileged access to the bluez service.
# Usage: reserved

#include <abstractions/dbus-strict>

# Allow all access to bluez service
dbus (receive, send)
    bus=system
    peer=(label=bluez5_bluez_*),

dbus (send)
    bus=system
    peer=(name=org.bluez, label=unconfined),

dbus (send)
    bus=system
    peer=(name=org.bluez.obex, label=unconfined),

dbus (receive)
    bus=system
    path=/
    interface=org.freedesktop.DBus.ObjectManager
    peer=(label=unconfined),

dbus (receive)
    bus=system
    path=/org/bluez{,/**}
    interface=org.freedesktop.DBus.*
    peer=(label=unconfined),

In addition to that, this interface is going to need the following seccomp rules for the slot side (ie, what the server needs, taken from bluez.seccomp from the current 16.04 bluez5 snap):

# Description: Allow operating as the bluez service. Reserved because this
# gives
#  privileged access to the system.
# Usage: reserved
accept
accept4
bind
connect
getpeername
getsockname
getsockopt
listen
recv
recvfrom
recvmmsg
recvmsg
send
sendmmsg
sendmsg
sendto
setsockopt
shutdown
socketpair
socket

and the following apparmor rules for the plugs side (ie, what the client needs, taken from framework-policy/seccomp/policygroups/client from the current 16.04 bluez5 snap):

# Description: Allow using bluez service. Reserved because this gives
#  privileged access to the bluez service.
# Usage: reserved

# Can communicate with DBus system service
connect
getsockname
recv
recvmsg
send
sendto
sendmsg
socket

@zyga : the slots/plugs seccomp rules are going to be common for all dbus interfaces. Perhaps these can be added automatically or at the very least have a helper function that interfaces can call out to so we can maintain the dbus seccomp list in one place for all dbus interfaces?

jdstrand · Apr 6, 2016

I fixed the formatting.

jdstrand · Apr 6, 2016

It looks like obex was left out of the bluez bus policy. If that was intentional, please don't include the obex apparmor rules.

zyga · Apr 6, 2016

@jdstrand good idea on having common bits for dbus. Right now I don't quite know how it would look like but I was thinking about letting the backends offer helpers for high-level operations so that we don't have to patch similar snippets all around.

zyga · Apr 6, 2016

@ssweeny please iterate on this and remove the reviewed label once you think this is ready for another review.

morphis · Apr 6, 2016

@jdstrand that means the apparmor/seccomp files can be shipped anymore with the snap itself? I thought we only had to add the dbus policy file within the interface definition?

jdstrand · Apr 6, 2016

@morphis - that is right. Frameworks are gone and so is old-security/security-policy. All of that is replaced with interfaces. There are plenty of examples in builtins/apparmor and builtins/seccomp that you can use.

morphis · Apr 6, 2016

@jdstrand I see.

ssweeny · Apr 6, 2016

@jdstrand two questions:

Will all of the snippets you pasted go into the Permanent{Plug|Slot} snippets?
I do want to include the obex rules. Will they need their own interface or can I add them to the bluez interface?

morphis · Apr 6, 2016

morphis reviewed Apr 6, 2016

View changes

Eventually we should put this into a static variable but can't say if that fits the Go coding guidelines for snappy. Saw that in the other interface definitions. Would make the switch statement less complex from a reader perspective.

morphis · Apr 7, 2016

@ssweeny I think we can add the obex policy bits to the bluez interface for now. Lets split that later if needed.

ssweeny · Apr 7, 2016

Looks like the Interfaces interface changed 😃

zyga · Apr 7, 2016

@ssweeny yes, sorry, I should have pinged you. Have a look at the interface (go interface). The changes are trivial:

methods got renamed to Setup and Remove
remove changed to take just the snap name
there's a new method AutoConnect (needs some tests, see other ifaces)

Please wait for #826 to land though

zyga · Apr 7, 2016

On the up side, Interfaces are about to go live so you should see your code working in practice. Track #827

niemeyer · Apr 7, 2016

@zyga @jdstrand This doesn't look right. How come we are allowing the snap to access bluez while the plug is disconnected!? The slot side is okay, as that's going to be the service, but the application itself should not be granted permissions unless the plug is connected.

Do we have this issue on any other interface?

ssweeny · Apr 8, 2016

@niemeyer I wasn't sure about that. So the plug side snippets should be Connected rather than Permanent?

zyga · Apr 8, 2016

@niemeyer you are right. We'll carefully review this before merging it

morphis · Apr 8, 2016

@ssweeny We still need to add some unit tests around this and follow the formatting rules to get this ready for a final review.

zyga · Apr 8, 2016

zyga reviewed Apr 8, 2016

View changes

As @niemeyer mentioned, using permanent plug snippets feels wrong.

This should be per-connection. Once a 3rd party snap and bluez are connected; only then; can that snap talk to bluez.

zyga · Apr 8, 2016

zyga reviewed Apr 8, 2016

View changes

Ditto

zyga · Apr 8, 2016

zyga reviewed Apr 8, 2016

View changes

Ditto

zyga · Apr 8, 2016

zyga reviewed Apr 8, 2016

View changes

Once we have snap IDs I'd like this interface to be only available to the bluez snap. Perhaps that will be done with assertions, perhaps here. Not sure yet.

ssweeny · Apr 8, 2016

@zyga thinking about it I do agree that it feels better to do those per-connection. Thanks for clarifying!

ssweeny · Apr 11, 2016

@zyga apart from the coverage issue how are we looking?

zyga · Apr 11, 2016

I'll check this out after the release tonight.

niemeyer · Apr 13, 2016

add to whitelist

niemeyer · Apr 13, 2016

retest this please

zyga · Apr 13, 2016

+1 LGTM and see how it works in practice.

niemeyer · Apr 14, 2016

@jdstrand @ssweeny Is this ready to go in?

ssweeny · Apr 14, 2016

@niemeyer It's good from my end.

niemeyer · Apr 14, 2016

@ssweeny Can you test the image ~~tomorrow~~ Thursday to ensure it works? We'll be cutting a release soon.

niemeyer · Apr 14, 2016

(by tomorrow I mean Thursday, sorry)

jdstrand · Apr 14, 2016

jdstrand reviewed Apr 14, 2016

View changes

I just noticed this label is not going to match correctly for two reasons:

bluez5_bluez_* is not how the profile name is formed any more, it should be snap.bluez5.bluez.*
this label should match the label of the slot side. Ie, perhaps there is bluez5 bluez-foo and bluez-bar in the store that all offer the bluez slot to clients. snappy should dynamically create this plug side snippet to match the label of the slot side that the client is connecting to. Ie, if 'bluez-client-snap' is connected to 'bluez-foo', then the interface code should create a rule like this on the plugs side: dbus (receive, send) bus=system peer=(label=snap.bluez-foo.service.*)

Interestingly, the slot side could have reciprocal rules to only allow the connected clients as well.

jdstrand · Apr 14, 2016

I think it was fine to commit this as is and then address my last comment in a future commit.

zyga added the Reviewed label Apr 6, 2016

Require root for bluez access

e3e748e

Add text for rules as variables to ease readability

All checks have passed
2 successful checks

continuous-integration/travis-ci/pr — The Travis CI build passed

Details

coverage/coveralls — Coverage decreased (-0.1%) to 76.717%

Details

9e9e22f

Add obex rules to the dbus policy

5d100db

Add AutoConnect() method
Return false for now

Some checks were not successful
1 failing and 1 successful checks

continuous-integration/travis-ci/pr — The Travis CI build failed

Details

coverage/coveralls — Coverage decreased (-0.5%) to 76.381%

Details

19aefe9

niemeyer removed the Reviewed label Apr 7, 2016

niemeyer changed the title from Add bluez interface to interfaces/builtin: add bluez interface Apr 13, 2016

ssweeny deleted the ssweeny:bluez-interface branch Apr 14, 2016

+        +  </policy>
+        +  <policy at_console="true">
+        +    <allow send_destination="org.bluez"/>
+        +  </policy>

+        +	switch securitySystem {
+        +	case interfaces.SecurityDBus:
+        +		return []byte(`
+        +<policy user="root">

		@@ -0,0 +1,64 @@
		+package builtin

snapcore/snapd

Join GitHub today

interfaces/builtin: add bluez interface #802

Conversation

ssweeny commented Apr 6, 2016

Some checks were not successful

ssweeny commented Apr 6, 2016

zyga reviewed Apr 6, 2016 View changes

zyga commented Apr 6, 2016

zyga commented Apr 6, 2016

zyga commented Apr 6, 2016

ssweeny added some commits Apr 6, 2016

All checks have passed

jdstrand reviewed Apr 6, 2016 View changes

jdstrand commented Apr 6, 2016

jdstrand commented Apr 6, 2016

jdstrand commented Apr 6, 2016

zyga commented Apr 6, 2016

zyga added the Reviewed label Apr 6, 2016

zyga commented Apr 6, 2016

morphis commented Apr 6, 2016

jdstrand commented Apr 6, 2016

morphis commented Apr 6, 2016

ssweeny commented Apr 6, 2016

morphis reviewed Apr 6, 2016 View changes

All checks have passed

morphis commented Apr 7, 2016

ssweeny commented Apr 7, 2016

zyga commented Apr 7, 2016

zyga commented Apr 7, 2016

Some checks were not successful

niemeyer commented Apr 7, 2016

niemeyer removed the Reviewed label Apr 7, 2016

ssweeny commented Apr 8, 2016

zyga commented Apr 8, 2016

morphis commented Apr 8, 2016

zyga reviewed Apr 8, 2016 View changes

zyga reviewed Apr 8, 2016 View changes

zyga reviewed Apr 8, 2016 View changes

zyga reviewed Apr 8, 2016 View changes

ssweeny commented Apr 8, 2016

ssweeny added some commits Apr 8, 2016

Some checks were not successful

Some checks were not successful

ssweeny commented Apr 11, 2016

zyga commented Apr 11, 2016

All checks have passed

niemeyer commented Apr 13, 2016

niemeyer changed the title from Add bluez interface to interfaces/builtin: add bluez interface Apr 13, 2016

niemeyer commented Apr 13, 2016

zyga commented Apr 13, 2016

niemeyer commented Apr 14, 2016

ssweeny commented Apr 14, 2016

niemeyer commented Apr 14, 2016

niemeyer commented Apr 14, 2016

Hide details View details niemeyer merged commit 9025e4b into snapcore:master Apr 14, 2016 4 checks passed

4 checks passed

jdstrand reviewed Apr 14, 2016 View changes

jdstrand commented Apr 14, 2016

ssweeny deleted the ssweeny:bluez-interface branch Apr 14, 2016

zyga reviewed Apr 6, 2016

View changes

jdstrand reviewed Apr 6, 2016

View changes

morphis reviewed Apr 6, 2016

View changes

zyga reviewed Apr 8, 2016

View changes

zyga reviewed Apr 8, 2016

View changes

zyga reviewed Apr 8, 2016

View changes

zyga reviewed Apr 8, 2016

View changes

niemeyer merged commit `9025e4b` into snapcore:master Apr 14, 2016
4 checks passed

jdstrand reviewed Apr 14, 2016

View changes