interfaces/builtin: add dbus interface #1613

jdstrand · 2016-08-01T22:04:12Z

This implements dbus in support of 'Apps can't own session bus names'. Specifically, snaps may specify a slot using this interface which grants access to bind. Eg:

name: gnome-logs-udt
slots:
  org.gnome.Logs:
    interface: dbus
    bus: session
    name: org.gnome.Logs

This will allow the snap to own the well-known bus name 'org.gnome.Logs' and also integrate into the classic environment.

This also implements the plugs side and allows other snaps to connect to the interface with something like:

name: foo
plugs:
  org.gnome.Logs:
    interface: dbus
    bus: session
    name: org.gnome.Logs

Upon snap connect, this allows foo to introspect gnome-logs-udt and use its entire DBus API.

Please see #1613 (comment) for complete details.

LP: #1590679

Test with: go build -tags=excludeintegration -v github.com/snapcore/snapd/... && go test -tags=excludeintegration -v github.com/snapcore/snapd/interfaces/builtin

…nup)

…ppet()

pedronis · 2016-12-13T21:10:17Z

interfaces/builtin/dbus.go

+	dot_re := regexp.MustCompile("\\.")
+	var pathBuf bytes.Buffer
+	pathBuf.WriteString(`"/`)
+	pathBuf.WriteString(dot_re.ReplaceAllString(name, "/"))


the bit stands that this could be just strings.Replace(name, ".", "/", -1)

pedronis · 2016-12-13T21:11:30Z

interfaces/builtin/dbus.go

+}
+
+// Obtain yaml-specified bus well-known name
+func (iface *DbusInterface) getAttribs(attribs map[string]interface{}) (string, string, error) {


would be better to use named results here: (bus, name string, err error)

pedronis · 2016-12-13T21:14:02Z

interfaces/builtin/dbus.go

+	}
+
+	// ensure that we only connect to slot with matching attributes
+	if bus != busPlug || name != namePlug {


@jdstrand could you replace that TODO there with use interface/policy Checkers

pedronis · 2016-12-13T21:17:53Z

interfaces/builtin/dbus.go

+
+func (iface *DbusInterface) AutoConnect(*interfaces.Plug, *interfaces.Slot) bool {
+	// allow what declarations allowed
+	return true


the check // ensure that we only connect to slot with matching attributes

if bus != busPlug || name != namePlug {

should probably be done here? instead?

that's what content is doing atm btw, until we don't have support for specifying attribute matching in the basedecl

AutoConnect() isn't the right place for the check for the dbus interface because the check needs to happen at connection, not auto connection and at connection we only connect when bus and name match. The content interface is different in that while the content attribute must match for auto-connect, the content attribute is not used as part of the connected snippets, only read and write which aren't part of the content interface's plugging attributes.

Note that the dbus interface design and its attributes are approved.

pedronis · 2016-12-13T21:21:02Z

interfaces/builtin/dbus.go

+		return "", "", fmt.Errorf("DBus bus name must not end with -NUMBER")
+	}
+
+	if bus == "" {


I'm confused by these checks, it doesn't seem that bus == "" would pass the previous check being equal session or system, same for name == "" and ValidateDBusName ?

@pedronis, you are right. These are a holdover from a refactor. Removed.

jdstrand · 2016-12-13T22:26:00Z

@pedronis, I believe I addressed your comments.

jdstrand · 2016-12-13T23:28:48Z

@pedronis and I discussed the 'if bus != busPlug || name != namePlug' part on irc and agreed that what is implemented is the best we can do without the attribute matching. The code will ensure that the correct security policy is generated. I updated the comment to reflect this.

jdstrand · 2016-12-13T23:29:44Z

@pedronis and @tyhicks - I believe I addressed all your comments. Can you give a +1 or more feedback if needed?

Thanks for your reviews!

pedronis

looks reasonable given the current infra limitations

tyhicks · 2016-12-13T23:39:51Z

The security policy looks good to me. Thanks for making the adjustments we spoke about over IRC.

jdstrand · 2016-12-13T23:48:30Z

@niemeyer - I believe this is mergeable now based on my incorporating feedback from you, @pedronis, and @tyhicks. Please consider approving the changes pending the testsuite run.

jdstrand · 2016-12-14T01:51:59Z

The travis testsuite failure is unrelated:
2016/12/14 00:37:47 Failed tasks: 1
- linode:ubuntu-core-16-64:tests/main/revert-devmode:production

snap refresh --devmode --edge test-snapd-tools
error: cannot refresh "test-snapd-tools": cannot get refresh information for snap "test-snapd-tools": Post https://search.apps.ubuntu.com/api/v1/snaps/metadata: EOF

jdstrand · 2016-12-14T01:52:30Z

The trusty failure is unrelated:
Exit request sent.
autopkgtest [01:12:08]: ERROR: erroneous package: Test dependencies are unsatisfiable. A common reason is that your testbed is out of date with respect to the archive, and you need to use a current testbed or run apt-get update or use -U.
blame: https://github.com/snapcore/snapd.git#refs/pull/1613/head
badpkg: Test dependencies are unsatisfiable. A common reason is that your testbed is out of date with respect to the archive, and you need to use a current testbed or run apt-get update or use -U.

jdstrand · 2016-12-14T01:55:31Z

The zesty failures are unrelated:

2016/12/14 01:22:14 Error executing autopkgtest:ubuntu-16.04-64:tests/main/server-snap:pythonServer : 
-----
++ nc -4 localhost 80
+ response=
+ statusPattern='(?s)HTTP\/1\.0 200 OK\n*'
+ grep -Pzq '(?s)HTTP\/1\.0 200 OK\n*'
+ echo ''

2016/12/14 01:11:22 Error executing autopkgtest:ubuntu-16.04-64:tests/main/interfaces-network-bind : 
-----
+ . /home/gopath/src/github.com/snapcore/snapd/tests/lib/names.sh
+++ sed -n 's/^\(pc\|pi[23]\|dragonboard\) .*/\1/p'
+++ snap list
++ gadget_name=
++ kernel_name=-kernel
+++ awk '/^(ubuntu-)?core / {print $1; exit}'
+++ snap list
++ core_name=core
++ '[' -kernel = pi3-kernel ']'
+ CONNECTED_PATTERN='(?s)Slot +Plug\n.*?\n:network-bind +network-bind-consumer'
+ DISCONNECTED_PATTERN='(?s)Slot +Plug\n.*?\n- +network-bind-consumer:network-bind'
+ echo 'Then the snap is listed as connected'
Then the snap is listed as connected
+ grep -Pzq '(?s)Slot +Plug\n.*?\n:network-bind +network-bind-consumer'
+ snap interfaces
+ echo ============================================
============================================
+ echo 'When the plug is disconnected'
When the plug is disconnected
+ snap disconnect network-bind-consumer:network-bind core:network-bind

[|] Disconnect network-bind-consumer:network-bind from core:network-bind�[K
�[K+ grep -Pzq '(?s)Slot +Plug\n.*?\n- +network-bind-consumer:network-bind'
+ snap interfaces
+ echo 'Then the plug can be connected again'
Then the plug can be connected again
+ snap connect network-bind-consumer:network-bind core:network-bind

[|] Connect network-bind-consumer:network-bind to core:network-bind�[K
�[K+ grep -Pzq '(?s)Slot +Plug\n.*?\n:network-bind +network-bind-consumer'
+ snap interfaces
+ echo ============================================
============================================
+ echo 'When the plug is connected'
When the plug is connected
+ snap connect network-bind-consumer:network-bind core:network-bind

[|] Connect network-bind-consumer:network-bind to core:network-bind�[K
�[K+ grep -Pzq '(?s)Slot +Plug\n.*?\n:network-bind +network-bind-consumer'
+ snap interfaces
+ echo 'Then the service is accessible by a client'
Then the service is accessible by a client
+ grep -Pqz 'ok\n'
+ nc -w 2 localhost 8081

2016/12/14 01:22:18 Error executing autopkgtest:ubuntu-16.04-64:tests/main/server-snap:goServer : 
-----
++ nc -6 ip6-localhost 8081
+ response=
+ statusPattern='(?s)HTTP\/1\.0 200 OK\n*'
+ grep -Pzq '(?s)HTTP\/1\.0 200 OK\n*'
+ echo ''

jdstrand · 2016-12-14T01:57:17Z

Retrying the travis tests since there seem to be intermittent failures.

jhenstridge · 2016-12-14T09:00:03Z

interfaces/builtin/dbus.go

+    bus=###DBUS_BUS###
+    path=/org/freedesktop/DBus
+    interface=org.freedesktop.DBus
+    member="GetConnectionUnix{ProcessID,User}"


Would it make sense to also include org.freedesktop.DBus.GetConnectionCredentials here? That seems to be the replacement for the above two if you need both.

That is a good point. Done

jhenstridge · 2016-12-14T09:00:21Z

interfaces/builtin/dbus.go

+	var ifaceBuf bytes.Buffer
+	ifaceBuf.WriteString(`"`)
+	ifaceBuf.WriteString(name)
+	ifaceBuf.WriteString(`{,.*}"`)


This might make sense as a default value, but interface names are not the same as bus names in the general case. For my work on storage-framework, we have a number of daemons that all implement the same D-Bus interface, but will register on the bus with different well known names.

I believe MPRIS works similarly.

Also, what if the service exposes method calls or signals on more than one interface? I imagine it'd be quite common for an app to want to respond to method calls on the org.freedesktop.DBus.Properties interface, but I think it'd be a mistake to simply special case it.

As mentioned on the mailing list: https://lists.ubuntu.com/archives/snapcraft/2016-December/001998.html this initial implementation of the interface intentionally does not address complex dbus services. It is meant to address leaf applications (eg, GNOME and KDE) that bind to a well-known name.

I'd prefer not to delay this PR any longer by adding more functionality to it. @niemeyer and I agreed in this PR that we would implement what we have and then see how people are using it and potentially add DBus paths, interfaces, activation, complex bus policy, etc down the line. Do keep in mind that generic interfaces like this one are a new concept in snappy that is in some ways counter to the initial goals of interfaces, and we want to make sure we design and implement them in a controlled fashion.

As for storage framework in particular, if you haven't already, I suggest reading the aforementioned thread. At this time, it will need to be its own interface (and that's ok-- it's easy getting interfaces into snappy :). Perhaps at a future date the dbus interface will get to the point to support all DBus services generically, but not just yet.

If you don't want to make it configurable, then I'd strongly consider allowing at least org.freedesktop.DBus.Properties as an extra one unconditionally. Without it, it is impossible to access APIs using the standard D-Bus property system.

To get property "A" on interface "I", you call org.freedesktop.DBus.Properties.Get("I", "A") -- not a method call on the interface declaring the property.

To be clear, we do want to make it configurable-- at some point. There is an urgency to this PR as it is blocking GNOME and KDE devs from targeting stable. This PR specifically addresses that use case. Other use cases can come in other PRs.

As for Properties, this is already handled by the path rule without an interface:

# allow connected snaps to all interfaces via ###DBUS_PATH### (eg, # org.freedesktop.*, org.gtk.Application, etc) to allow full integration with # connected snaps. dbus (receive, send) bus=###DBUS_BUS### path=###DBUS_PATH### peer=(label=###SLOT_SECURITY_TAGS###),

This was a tested condition for the leaf applications we are trying to address.

jhenstridge · 2016-12-14T09:50:11Z

interfaces/builtin/dbus.go

+	var pathBuf bytes.Buffer
+	pathBuf.WriteString(`"/`)
+	pathBuf.WriteString(strings.Replace(name, ".", "/", -1))
+	pathBuf.WriteString(`{,/**}"`)


Similar to the comment about interface names, you can't necessarily derive the paths a D-Bus service uses from the bus name it will try to acquire. If you're going to restrict object paths (which seems a bit dubious), shouldn't it be configurable on the slot?

See above comment. We intentionally left out path configuration in this initial implementation.

…nstridge)

jdstrand · 2016-12-14T19:10:58Z

Travis failure is unrelated: linode:ubuntu-core-16-64-fixme:tests/main/ubuntu-core-upgrade-no-gc

Trusty autopkgtest failure is unrelated

Zesty autopkgtest failures are unrelated (and same as before):
- autopkgtest:ubuntu-16.04-64:tests/main/interfaces-network-bind
- autopkgtest:ubuntu-16.04-64:tests/main/server-snap:goServer
- autopkgtest:ubuntu-16.04-64:tests/main/server-snap:pythonServer

jdstrand · 2016-12-14T19:14:06Z

From IRC:

12:58 < niemeyer> So do we support any interface on the well known name?
13:01 < jdstrand> we allow a connected snap to introspect our dbus api
13:01 < jdstrand> we allow a connected snap to use any dbus interface and any dbus 
                  path if they are connecting to the well-known name
13:02 < jdstrand> if they are using a non-well-known name, we allow connecting to any 
                  dbus path if the dbus interface is regular according to our 
                  well-known name
13:02 < niemeyer> Okay, that sounds reasonable
13:02 < niemeyer> Thanks for the long explanation
13:02 < jdstrand> if they are using a non-well-known name, we allow connecting with 
                  any dbus interface if the dbus path is regular accoriding to our 
                  well-known name
13:03 < niemeyer> Happy to go forward with it
13:03 < jdstrand> \o/
13:03 < niemeyer> Is the PR blocked on anything else at this point?
13:04 < jdstrand> niemeyer: no. tyhicks reviewed the security policy. pedronis 
                  reviewed the code. I incorporated all applicable feedback from 
                  everywhere
13:04 < niemeyer> So let's get it in
13:04 < jdstrand> thanks!
13:04 < niemeyer> Thank you!
13:06 < jdstrand> niemeyer: so with that, I plan to work with the personal folks on 
                  designing their interfaces (eg, storage framework) and will be 
                  reviewing tvoss' work for session services and will be thinking 
                  about how all of this relates to this interface, improvements we 
                  can make to this interface, etc and when I have coherent thoughts 
                  on it, will present phase ii, phase iii, etc, etc iterations to you

jdstrand · 2016-12-15T14:29:35Z

Thanks especially @niemeyer for your time on this and thank you to all reviewers and commenters for your input and reviews! :)

Jamie Strandboge added 30 commits June 22, 2016 16:29

interfaces: add preliminary dbus-bind (TODO: attributes)

a289901

implement SanitizeSlot with tests

94736b4

perform substitutions in PermanentSlotAppArmor policy

57fb199

Merge remote-tracking branch 'upstream/master' into dbus-bind

8b7e9f3

update comments and documentation

98c39de

Merge remote-tracking branch 'upstream/master' into dbus-bind

413e19c

update receive and send rules for talking with dbus-daemon

a624250

add connected plug and connected slot policy. improve tests

34dd46f

Merge remote-tracking branch 'upstream/master' into dbus-bind

eb2d79b

add interfaces.SecurityMount to PermanentSlotSnippet

2345399

BROKEN TEST BRANCH

969be86

Test with: go build -tags=excludeintegration -v github.com/snapcore/snapd/... && go test -tags=excludeintegration -v github.com/snapcore/snapd/interfaces/builtin

revert last commit

b743fe7

Merge remote-tracking branch 'upstream/master' into dbus-bind

af16d22

start conversion to proposed yaml

2f73a16

update SanitizeSlot and tests for getBusNames()

24fd251

remove dbusBindBusNames, iterate through names for policy (TODO: clea…

12721f0

…nup)

abstract out shared and individual dbus name rules, fix testsuite

0a62fc2

update TestPermanentSlotAppArmorSystem() to test abstraction

a978d37

abstract out AppArmor abstraction calculation

22aeba5

abstract out individual policy generation to getAppArmorIndividualSni…

6a865ce

…ppet()

update comments and shuffle code around

4861623

update docs, implement plugs, refactor tests, add tests

a6af67f

Merge remote-tracking branch 'upstream/master' into dbus-bind

ab91e06

add Introspectable policy

c2da833

go fmt changes

7954f99

fix a couple typos and a the mocked test yaml

ce961f0

ensure that connections specify the same bus and name

11d768c

non-existent plug should skip. non-existent slot should error

4915bd9

don't autoconnect

28577f9

Merge remote-tracking branch 'upstream/master' into dbus-bind

26b8e2e

pedronis reviewed Dec 13, 2016

View reviewed changes

Jamie Strandboge added 5 commits December 13, 2016 21:31

address feedback from tyhicks (thanks!) regarding comments

a372d97

fix testsuite for last commit

0ea491d

remove redundant checks (thanks pedronis)

d9a1dce

simplify pathBuf (thanks pedronis)

38b3feb

add TODO for using interface/policy checkers (thanks pedronis)

bb7cc54

Jamie Strandboge added 3 commits December 13, 2016 22:48

remove 'send' with classic policy and clarify comments (thanks tyhicks)

b070992

update parser error with peer=(name=... for KDE plug rule

3f5acbf

update the comment for attributes not matching to be more clear

ead951b

pedronis reviewed Dec 13, 2016

View reviewed changes

jdstrand mentioned this pull request Dec 14, 2016

interfaces: support network namespaces via 'ip netns' in network-control (LP: #1624675) #2450

Merged

jhenstridge reviewed Dec 14, 2016

View reviewed changes

Jamie Strandboge added 2 commits December 14, 2016 14:06

Merge remote-tracking branch 'upstream/master' into dbus-app

758cda9

dbusPermanentSlotAppArmor: allow GetConnectionCredentials (thanks jhe…

50e6cf1

…nstridge)

niemeyer changed the title ~~interfaces/builtin: add dbus interface (LP: #1590679)~~ interfaces/builtin: add dbus interface Dec 15, 2016

mvo5 merged commit c1ff793 into snapcore:master Dec 15, 2016

jdstrand deleted the dbus-app branch January 9, 2017 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interfaces/builtin: add dbus interface #1613

interfaces/builtin: add dbus interface #1613

jdstrand commented Aug 1, 2016 •

edited by niemeyer

pedronis Dec 13, 2016

jdstrand Dec 13, 2016

pedronis Dec 13, 2016

pedronis Dec 13, 2016

pedronis Dec 13, 2016

jdstrand Dec 13, 2016

pedronis Dec 13, 2016

jdstrand Dec 13, 2016

jdstrand commented Dec 13, 2016

jdstrand commented Dec 13, 2016

jdstrand commented Dec 13, 2016

pedronis left a comment

tyhicks commented Dec 13, 2016

jdstrand commented Dec 13, 2016 •

edited

jdstrand commented Dec 14, 2016

jdstrand commented Dec 14, 2016

jdstrand commented Dec 14, 2016

jdstrand commented Dec 14, 2016

jhenstridge Dec 14, 2016

jdstrand Dec 14, 2016

jhenstridge Dec 14, 2016

jdstrand Dec 14, 2016 •

edited

jhenstridge Dec 14, 2016

jdstrand Dec 14, 2016 •

edited

jhenstridge Dec 14, 2016

jdstrand Dec 14, 2016

jdstrand commented Dec 14, 2016

jdstrand commented Dec 14, 2016

jdstrand commented Dec 15, 2016

interfaces/builtin: add dbus interface #1613

interfaces/builtin: add dbus interface #1613

Conversation

jdstrand commented Aug 1, 2016 • edited by niemeyer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand commented Dec 13, 2016

jdstrand commented Dec 13, 2016

jdstrand commented Dec 13, 2016

pedronis left a comment

Choose a reason for hiding this comment

tyhicks commented Dec 13, 2016

jdstrand commented Dec 13, 2016 • edited

jdstrand commented Dec 14, 2016

jdstrand commented Dec 14, 2016

jdstrand commented Dec 14, 2016

jdstrand commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand Dec 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand Dec 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand commented Dec 14, 2016

jdstrand commented Dec 14, 2016

jdstrand commented Dec 15, 2016

jdstrand commented Aug 1, 2016 •

edited by niemeyer

jdstrand commented Dec 13, 2016 •

edited

jdstrand Dec 14, 2016 •

edited

jdstrand Dec 14, 2016 •

edited