Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces,interfaces/dbus: add DBus security backend #767
Conversation
zyga
added some commits
Mar 18, 2016
jdstrand
reviewed
Mar 31, 2016
| +// Particular security snippets define whole <policy>...</policy> entires. | ||
| +// | ||
| +// NOTE: This interacts with systemd. | ||
| +// TODO: Explain how this works (security). |
jdstrand
Mar 31, 2016
Contributor
Probably a link to this page is enough explanation: https://dbus.freedesktop.org/doc/dbus-daemon.1.html
jdstrand
reviewed
Mar 31, 2016
| + | ||
| +// Configure creates dbus configuration files specific to a given snap. | ||
| +// | ||
| +// NOTE: DBus has no concept of a complain mode so developer mode is not supported. |
niemeyer
Mar 31, 2016
Contributor
Should it be ignored, or should dbus be unblocked altogether? That's what that mode does for the other security systems, right? Complain just ensures we're notifying about improper attempts.
jdstrand
reviewed
Mar 31, 2016
| + // Get the snippets that apply to this snap | ||
| + snippets, err := repo.SecuritySnippetsForSnap(snapInfo.Name, interfaces.SecurityDBus) | ||
| + if err != nil { | ||
| + return fmt.Errorf("cannot obtain security snippets for snap %q: %s", snapInfo.Name, err) |
jdstrand
Mar 31, 2016
Contributor
I didn't notice this in previous reviews for other backends, but this error message seems too generic and should s/obtain security snippets/obtain dbus security snippets/ so that if there is a problem in a particular backend, we know which one. apparmor/backend.go and seccomp/backend.go need a corresponding change (can be done in a separate commit).
jdstrand
reviewed
Mar 31, 2016
| + // Get the files that this snap should have | ||
| + content, err := b.combineSnippets(snapInfo, developerMode, snippets) | ||
| + if err != nil { | ||
| + return fmt.Errorf("cannot obtain expected security files for snap %q: %s", snapInfo.Name, err) |
jdstrand
reviewed
Mar 31, 2016
| + glob := fmt.Sprintf("%s.conf", interfaces.SecurityTagGlob(snapInfo)) | ||
| + _, _, err = osutil.EnsureDirState(dirs.SnapBusPolicyDir, glob, content) | ||
| + if err != nil { | ||
| + return fmt.Errorf("cannot synchronize security files for snap %q: %s", snapInfo.Name, err) |
jdstrand
reviewed
Mar 31, 2016
| + _, _, err = osutil.EnsureDirState(dirs.SnapBusPolicyDir, glob, content) | ||
| + if err != nil { | ||
| + return fmt.Errorf("cannot synchronize security files for snap %q: %s", snapInfo.Name, err) | ||
| + } |
jdstrand
Mar 31, 2016
Contributor
I'm not 100% sure that dbus daemon doesn't need to be force-reloaded. Looking at deb packaging for bluez, it has 'invoke-rc.d dbus force-reload'. On 15.04 I don't recall needing to do anything special about this, but it might've been because we were using Type=dbus and BusName=... in the systemd unit. Also, Tony didn't complain about the bus policy not taking effect immediately with his earlier 16.04 snap.... I looked at the dbus code and it has an inotify watch on configuration and service directories.
In all, I don't think there is anything to do here, but I wanted to add a comment here for future reference.
jdstrand
reviewed
Mar 31, 2016
| +var ( | ||
| + xmlHeader = []byte(`<!DOCTYPE busconfig PUBLIC\ | ||
| + "-//freedesktop//DTD D-BUS Bus Configuration 1.0//EN" | ||
| + "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd"> |
jdstrand
Mar 31, 2016
Contributor
In 15.04 we did:
<!--
This file is autogenerated by snappy
-->
I wonder if it should be in 16.04 as well. I don't have a strong opinion either way.
jdstrand
Mar 31, 2016
Contributor
"Thanks, perhaps we should add that to all files we make?"
We could, but the difference here is that the file is in /etc as opposed to /var. I'm not opposed to adding it everywhere though.
niemeyer
Mar 31, 2016
Contributor
If adding it, let's please have just "This file is auto-generated." instead.
|
A few minor comment changes and one thing to note. LGTM |
niemeyer
reviewed
Mar 31, 2016
| +// Each configuration is an XML file containing <busconfig>...</busconfig>. | ||
| +// Particular security snippets define whole <policy>...</policy> entires. | ||
| +// | ||
| +// NOTE: This interacts with systemd. |
niemeyer
Mar 31, 2016
Contributor
The NOTE: prefix doesn't seem to be holding a consistent and useful meaning. These are comments. Everything in them are "notes". Would you mind to drop all such prefixes in this branch (there are many of them).
|
LGTM with trivials handled. |
zyga
added some commits
Mar 31, 2016
zyga commentedMar 31, 2016
This branch adds the third security backend, DBus. Unlike other backends it is not used by any of our interfaces yet but we expect to make heavy use of it when implementing support for snaps like network-manager or bluez that use DBus extensively.