New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
interfaces: support D-Bus activation #6258
Conversation
094458f
to
28e83b3
Compare
1bd1a83
to
a5a59f7
Compare
This branch relies on changes to the test-snapd-dbus-service snap. Those changes have been merged to the bzr branch and the build recipe has run, but I think they're either stuck in manual review, or need manual review requested: https://launchpad.net/~mvo/+snap/test-snapd-dbus-service I can't see the dashboard for the snap. |
I've run into a few problems trying to get my spread tests running on Ubuntu 14.04. I get the following from the
On 14.04, the system bus is run by Upstart so it is starting the service manually via the [note that session services work fine, since it doesn't use I suspect the same will happen on newer distros for system services presented as "apps" rather than "daemons" in
|
5de375a
to
990033d
Compare
There's currently a few failures in the spread tests:
It's not clear that the opensuse failures have anything to do with my branch: it looks like the Go compiler is being invoked with The The The The |
With the test modified to use a confined D-Bus client, I'm seeing failures on 18.04 and 18.10: when the service is inactive, the AppArmor rules block the method call before activation can occur. It's fine on 16.04, so I suspect there is something different in the kernel or dbus-daemon. |
From what I can tell, dbus-daemon passes an empty destination security label to the kernel when checking a method call to a not-yet-activated service. Manually adjusting the rule to
... seems to allow the client to activate the service. That doesn't seem to allow communication with unconfined peers, or activation of names other than the one specified in the rule. @jdstrand: can you think of any problems with this change to the rule? |
With the changes to the AppArmor rule, we're down to two failures:
The 14.04 failure is the I've got no idea why centos-7-64 is now working. I wouldn't have thought any of the changes I made would affect that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment for a way to assign a label to the activated service.
2a5d3c3
to
91dda01
Compare
my key point was about AssumedAppArmorLabel, which was addressed. I've not reviewed the remaining PR to block on it
1398dbb
to
a606b1c
Compare
I realised that with some small changes, my code for managing service files could be made to avoid service files not managed by snapd. The last commit on this branch makes use of this to place system services to However, this does introduce a potential problem with upgrades. What if I install a snap providing an activatable system service on 14.04, then upgrade the host to xenial or bionic? It will likely leave a stale .service file in /usr. I can see three options here:
I'm not sure which option is best. |
0aeb093
to
0d16b43
Compare
0d16b43
to
6cc6092
Compare
interfaces/dbus/backend.go
Outdated
@@ -55,6 +56,27 @@ func (b *Backend) Name() interfaces.SecuritySystem { | |||
return "dbus" | |||
} | |||
|
|||
// setupHostDBusConf will ensure that we have a dbus configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of other similar files we create we the trigger point is installation of either core or snapd. I wonder if we could behave similarly there, so that at least all that code is in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the relation of this code vs the one in wrappers/core18.go ? the latter will be used only core, this one is for all systems, but wondering what are the various scenarios, will the files not be there early enough in some cases? also should we share code between the two paths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd written this code way back before the wrappers/core18.go
code. The intention was the same as the code installing user'd service activation files: ensure things work on a system with an old .deb installed snapd functions if it receives a new version via the core
snap.
If we don't need that any more, then I guess we can remove this. I wasn't sure the wrappers/core18.go
code did anything on systems without the snapd snap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, wrappers/core18.go is for core >= 18 only, but maybe there's a way to have a helper in wrappers.go that is used in both cases
interfaces/dbus/backend.go
Outdated
"session.d/snapd.session-services.conf", | ||
"system.d/snapd.system-services.conf", | ||
} { | ||
dst := filepath.Join("/usr/share/dbus-1/", conf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not using the root directory from dirs.go
so it will be harder to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review. Will continue after the call today.
interfaces/dbus/backend.go
Outdated
case "system": | ||
servicesDir = dirs.SnapDBusSystemServicesDir | ||
default: | ||
panic(bus) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could just return an error here.
wrappers/services.go
Outdated
if appInfo.Daemon == "dbus" && len(appInfo.ActivatesOn) > 0 { | ||
slot := appInfo.Slots[appInfo.ActivatesOn[len(appInfo.ActivatesOn)-1]] | ||
if err := slot.Attr("name", &busName); err != nil { | ||
logger.Noticef("could not get 'name' attribute of dbus slot %q: %v", slot.Name, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.Noticef("could not get 'name' attribute of dbus slot %q: %v", slot.Name, err) | |
logger.Noticef("Cannot get 'name' attribute of dbus slot %q: %v", slot.Name, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be validated earlier and essentially impossible here?
@@ -0,0 +1,39 @@ | |||
summary: D-Bus session services support activation without a user of instance systemd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to test how this behaves on Ubuntu 16.04?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Ubuntu 16.04, CentOS 7, and anything else where the session bus is not managed by systemd.
. "$TESTSLIB"/snaps.sh | ||
|
||
echo "Set up a private D-Bus session bus" | ||
eval "$(dbus-launch --sh-syntax)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super fond of seeing this code. Could we run it in a systemd service so that, at least, we can reliably shut it down, along with all children?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having gone through the exercise I would like you to do something like this instead: https://github.com/zyga/snapd/blob/tweak/detect-stray-dbus-daemon/tests/main/invariant-tool/task.yaml#L64
snap set system experimental.dbus-activation=true | ||
#shellcheck source=tests/lib/pkgdb.sh | ||
. "$TESTSLIB"/pkgdb.sh | ||
distro_install_package dbus-user-session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now preinstalled in all systems. I think you can drop it.
In addition, the restore side is missing.
wrappers/core18.go
Outdated
@@ -485,6 +490,53 @@ func undoSnapdUserServicesOnCore(s *snap.Info, inter interacter) error { | |||
return nil | |||
} | |||
|
|||
func writeSnapdDbusConfigOnCore(s *snap.Info) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this similar to setupHostDBusConf
?
snap/info_snap_yaml.go
Outdated
@@ -355,7 +355,7 @@ func setAppsFromSnapYaml(y snapYaml, snap *Info, strk *scopedTracker) error { | |||
PostStopCommand: yApp.PostStopCommand, | |||
RestartCond: yApp.RestartCond, | |||
RestartDelay: yApp.RestartDelay, | |||
BusName: yApp.BusName, | |||
ActivatesOn: yApp.ActivatesOn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is still pending
@@ -1,2 +1,4 @@ | |||
../../usr/lib/snapd/snap-device-helper /lib/udev/snappy-app-dev | |||
usr/lib/snapd/snapctl usr/bin/snapctl | |||
usr/share/dbus-1/session.d/snapd.session-services.conf etc/dbus-1/session.d/snapd.session-services.conf | |||
usr/share/dbus-1/system.d/snapd.system-services.conf etc/dbus-1/system.d/snapd.system-services.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not ship this on 14.04?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And a related question, if we choose not to ship it on 14.04 - should we make it so that on re-exec, this feature is not supported?
Perhaps we should add a function to the each feature, that checks if this is supported by the running system? Not sure
} | ||
} | ||
|
||
func (s *backendSuite) _TestUpdatingSnapToOneWithMoreServices(c *C) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the idea with _Test
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a first pass, some initial comments/questions mostly concentrating on the snap package, but also with some questions about other bits.
dirs/dirs.go
Outdated
@@ -288,6 +292,10 @@ func SetRootDir(rootdir string) { | |||
// freedesktop.org specifications | |||
SnapDesktopFilesDir = filepath.Join(rootdir, snappyDir, "desktop", "applications") | |||
SnapDesktopIconsDir = filepath.Join(rootdir, snappyDir, "desktop", "icons") | |||
// Use 'dbus/services' and `dbus/system-services' to mirror | |||
// '/usr/share/dbus-1' hierarchy. | |||
SnapDBusSessionServicesDir = filepath.Join(rootdir, snappyDir, "dbus", "services") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will there be dbus-2 ? should this be dbus-1? or no chance of that being an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was some forward thinking by DBus authors. There might be dbus-2 eventually or never, depending on how things evolve.
interfaces/dbus/backend.go
Outdated
@@ -55,6 +56,27 @@ func (b *Backend) Name() interfaces.SecuritySystem { | |||
return "dbus" | |||
} | |||
|
|||
// setupHostDBusConf will ensure that we have a dbus configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the relation of this code vs the one in wrappers/core18.go ? the latter will be used only core, this one is for all systems, but wondering what are the various scenarios, will the files not be there early enough in some cases? also should we share code between the two paths?
snap/info.go
Outdated
// things vs refactor | ||
// https://github.com/snapcore/snapd/pull/794#discussion_r58688496 | ||
BusName string | ||
ActivatesOn []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be close to the definition of Sockets I think, also I would kind of expect ti to be a ActivatesOn []*SlotInfo
@@ -90,8 +90,8 @@ type appYaml struct { | |||
SlotNames []string `yaml:"slots,omitempty"` | |||
PlugNames []string `yaml:"plugs,omitempty"` | |||
|
|||
BusName string `yaml:"bus-name,omitempty"` | |||
CommonID string `yaml:"common-id,omitempty"` | |||
ActivatesOn []string `yaml:"activates-on,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should go close to sockets stuff
snap/info_snap_yaml.go
Outdated
@@ -355,7 +355,7 @@ func setAppsFromSnapYaml(y snapYaml, snap *Info, strk *scopedTracker) error { | |||
PostStopCommand: yApp.PostStopCommand, | |||
RestartCond: yApp.RestartCond, | |||
RestartDelay: yApp.RestartDelay, | |||
BusName: yApp.BusName, | |||
ActivatesOn: yApp.ActivatesOn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if Info.ActivatesOn is a []*SlotInfo then this should be probably be setup close to where we set AppInfo.Slots.
As we discussed we should make activates-on imply these slots as being listed as if they were in the "slots" field for the app. Repetition between the two should be ok though.
snap/validate.go
Outdated
@@ -666,6 +665,15 @@ func ValidateApp(app *AppInfo) error { | |||
} | |||
} | |||
|
|||
// ActivatesOn is a list of slot names that use the "dbus" type | |||
for _, slotName := range app.ActivatesOn { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we check somehow that the app is a daemon: dbus or is that orthogonal?
snap/validate.go
Outdated
if slot, ok := app.Slots[slotName]; !ok { | ||
return fmt.Errorf("invalid activates-on value %q: slot not found", slotName) | ||
} else if slot.Interface != "dbus" { | ||
return fmt.Errorf("invalid activates-on value %q: slot does not use dbus interface", slotName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps:
"..: slot interface is not of the currently supported type (dbus) for activation"
there's a chance will add more types that are supported in activates-on
interfaces/builtin/dbus.go
Outdated
if len(apps) == 1 { | ||
app := apps[0] | ||
if !app.IsService() { | ||
return fmt.Errorf("only daemons can be activatable D-Bus services") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This made me realize that the proper spelling is indeed D-Bus
and not DBus. We could later do a pass and make the tree consistent.
interfaces/builtin/dbus.go
Outdated
} | ||
switch app.DaemonScope { | ||
case snap.SystemDaemon: | ||
if bus != "system" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a constant in dbusutil
somewhere? Might avoid some chance of typos.
@@ -37,6 +42,8 @@ type DbusInterfaceSuite struct { | |||
testutil.BaseTest | |||
iface interfaces.Interface | |||
|
|||
RootDir string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really used in practice?
interfaces/builtin/dbus.go
Outdated
return fmt.Errorf("only daemons can be activatable D-Bus services") | ||
} | ||
switch app.DaemonScope { | ||
case snap.SystemDaemon: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should all of this not be done by validation in the snap
package instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more complete pass.
My main worry: using non-state disk files in interfaces/backend/dbus
Minor comments throughout the code, on validation, 14.04 support, some testing.
apps: | ||
app-dbus-slot: | ||
daemon: simple | ||
daemon-scope: user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super late design nitpick time: could we use daemon-scope: user
to derive that a snap uses the session bus? Is there a valid reason for a user session app to grab a name on the system bus?
interfaces/builtin/dbus_test.go
Outdated
info := snaptest.MockInfo(c, mockSnapYaml, nil) | ||
slot := info.Slots["dbus-service-slot"] | ||
err := interfaces.BeforePrepareSlot(s.iface, slot) | ||
c.Check(err, ErrorMatches, "only daemons can be activatable D-Bus services") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This highlights my suggestion to handle this via snap validation. It would come up early (at snapcraft time) and does not require any dynamic connection state to determine.
|
||
err := os.MkdirAll(dirs.SnapDBusSystemServicesDir, 0755) | ||
c.Assert(err, IsNil) | ||
err = ioutil.WriteFile(filepath.Join(dirs.SnapDBusSystemServicesDir, "org.dbus-snap.system.service"), []byte(`[D-BUS Service] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this makes me somewhat worried we are duplicating state facilities. Instead of looking at the disk snapd should determine this via the state. This also frees us from parsing D-Bus service files as a authoritative source of information.
This also probably suggests that bus names should be something snapd models with regards to name clashes, similar to aliases and eventually man pages. I'm not sure how to structure this yet, though.
@@ -104,10 +126,24 @@ func (b *Backend) Setup(snapInfo *snap.Info, opts interfaces.ConfinementOptions, | |||
if err := setupDbusServiceForUserd(snapInfo); err != nil { | |||
logger.Noticef("cannot create host `snap userd` dbus service file: %s", err) | |||
} | |||
if err := setupHostDBusConf(snapInfo); err != nil { | |||
logger.Noticef("cannot create host dbus config: %s", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a snap warning instead?
} | ||
if err := b.removeServiceActivation(snapName); err != nil { | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to tell DBus daemon that something has changed or is it entirely based on file system notification? Similar question for the case above, where we alter content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a ReloadConfig
method on the bus, but dbus-daemon also performs an inotify watch on its config and service directories and automatically reload.
Debian packaging currently issues a ReloadConfig method call to the system bus when installing/removing services. It leaves session buses to fend for themselves though. At this point, it could all be seen as hold over from pre-inotify Linux systems, or to support Debian's non-Linux ports.
wrappers/services.go
Outdated
if appInfo.Daemon == "dbus" && len(appInfo.ActivatesOn) > 0 { | ||
slot := appInfo.Slots[appInfo.ActivatesOn[len(appInfo.ActivatesOn)-1]] | ||
if err := slot.Attr("name", &busName); err != nil { | ||
logger.Noticef("could not get 'name' attribute of dbus slot %q: %v", slot.Name, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be validated earlier and essentially impossible here?
session-tool -u test systemctl --user is-active snap.test-snapd-dbus-service.session.service | ||
|
||
echo "Removing the snap also removes the service activation file" | ||
snap remove test-snapd-dbus-service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've since learned that removing user session service keeps that service in the memory of systemd --user
, while the test
user is managed automatically, I wonder if snapd should orchestrate communication with snapd.session-agent.service
of each user to ensure this happens.
. "$TESTSLIB"/snaps.sh | ||
|
||
echo "Install a snap containing an activatable D-Bus session service" | ||
snap install --edge test-snapd-dbus-service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the snap need to come from the store for some assertions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snap is a Python daemon making use of packages not included in the base snap, so couldn't simply be built with snap pack
. So it is instead built with Snapcraft and the result published to the store.
|
||
systems: | ||
# Ubuntu Core does not contain the D-Bus user session systemd units | ||
- -ubuntu-core-* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could expand that to support core20 now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In PR #8943, I've made this test conditional on tests.session has-session-systemd-and-dbus
.
. "$TESTSLIB"/snaps.sh | ||
|
||
echo "Set up a private D-Bus session bus" | ||
eval "$(dbus-launch --sh-syntax)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having gone through the exercise I would like you to do something like this instead: https://github.com/zyga/snapd/blob/tweak/detect-stray-dbus-daemon/tests/main/invariant-tool/task.yaml#L64
Also implicitly bind the named slot to the app.
cdfb1a2
to
f2f3485
Compare
* Apps with an activates-on clause must be daemons * The slot's bus must match the daemon scope * The slot must only be activatable on a single app Corresponding checks removed from the dbus interface's BeforePrepareSlot implementation.
[ -f /var/lib/snapd/dbus/services/io.snapcraft.SnapDbusService.service ] | ||
|
||
echo "The service is not initially running" | ||
not session-tool -u test systemctl --user is-active snap.test-snapd-dbus-service.session.service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please update the spread test to the new command syntax:
session-tool --prepare
=> tests.session prepare
session-tool <cmd>
=> tests.session exec <cmd>
session-tool --restore
=> tests.session restore
I've created PR #8837 that splits out the yaml changes from this branch as a first step towards merging this. |
After #8943 was merged to master, and I merged master into this old branch, the remaining changes are:
So I'm marking this PR as closed. |
This is a reworking of PR #2592, expanded to work for both system and session services. In addition to that generalisation, I've made a few changes to how things are handled:
The service activation files are now named after the bus names rather than snap names. This is required for system bus services, but it had the side effect of making it a lot easier to check for existing name ownership.
I renamed the new interface attribute from
service
toactivatable
. This was a suggested change on the old PR.If the app associated with a dbus interface slot is a daemon, we add a
SystemdService
key to the service activation file, which causes the daemon to be launched via systemd. This currently only benefits system services, but it will be easy to generalise it to session services with PR wrappers: allow user mode systemd daemons #5822.I do wonder whether it would make sense to limit activatable services to "daemons" and not "apps". Given that the snap needs to ask for activation support, there is no backward compatibility issue.
I'm still working through getting the spread tests ported forward, which is why I've marked this WIP.