Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add download manager interface #2328
Conversation
All checks have failed
zyga
requested changes
Nov 22, 2016
Remove the legacy auto connect stuff (it was removed in master now) and let's see what the security team think about this.
| var buf bytes.Buffer | ||
| fmt.Fprintf(&buf, "@{SNAP_NAME}=\"%s\"\n", info.Name()) | ||
| fmt.Fprintf(&buf, "@{SNAP_REVISION}=\"%s\"\n", info.Revision) | ||
| + fmt.Fprintf(&buf, "@{SNAP_LABEL_DBUS}=\"%s\"\n", |
zyga
Nov 22, 2016
Contributor
I see why we're doing this but I don't know what @jdstrand will think about it.
jdstrand
Dec 5, 2016
Contributor
@zyga, I recommended it we bring it back. It was one of variables that was removed because nothing used it yet. We agreed then that we'd reintroduce it when Personal came along, since trusted helpers on Personal will use the DBusified security label as part of their DBus API.
That said, SNAP_LABEL_DBUS sounds a little weird. I know why it is named this way: it is the full security label of the process as a dbus string. AppArmor itself understands the '@{profile}' variable and expands it to the security label of the process. I think '@{PROFILE_DBUS} might be better in that light. What do others think?
| + allow-installation: | ||
| + slot-snap-type: | ||
| + - app | ||
| + deny-auto-connection: false |
zyga
Nov 22, 2016
Contributor
@jdstrand should this be true so that it can be overridden with a snap declaration assertion from the store?
| + return nil | ||
| +} | ||
| + | ||
| +func (iface *DownloadInterface) LegacyAutoConnect() bool { |
| + c.Assert(snippet, Not(IsNil)) | ||
| +} | ||
| + | ||
| +func (s *DownloadInterfaceSuite) TestLegacyAutoConnect(c *C) { |
Elleo
added some commits
Nov 22, 2016
Some checks were not successful
zyga
approved these changes
Nov 22, 2016
Looks better, thanks. Do get a +1 from @jdstrand before landing please.
| var buf bytes.Buffer | ||
| fmt.Fprintf(&buf, "@{SNAP_NAME}=\"%s\"\n", info.Name()) | ||
| fmt.Fprintf(&buf, "@{SNAP_REVISION}=\"%s\"\n", info.Revision) | ||
| + fmt.Fprintf(&buf, "@{SNAP_LABEL_DBUS}=\"%s\"\n", |
zyga
Nov 22, 2016
Contributor
I see why we're doing this but I don't know what @jdstrand will think about it.
jdstrand
Dec 5, 2016
Contributor
@zyga, I recommended it we bring it back. It was one of variables that was removed because nothing used it yet. We agreed then that we'd reintroduce it when Personal came along, since trusted helpers on Personal will use the DBusified security label as part of their DBus API.
That said, SNAP_LABEL_DBUS sounds a little weird. I know why it is named this way: it is the full security label of the process as a dbus string. AppArmor itself understands the '@{profile}' variable and expands it to the security label of the process. I think '@{PROFILE_DBUS} might be better in that light. What do others think?
| + allow-installation: | ||
| + slot-snap-type: | ||
| + - app | ||
| + deny-auto-connection: true |
jdstrand
Dec 5, 2016
Contributor
My understanding of UDM is that its DBus is considered completely safe for anyone to use. As such, this should drop 'deny-auto-connection: true'. Because it is is slot implementation, it should have 'deny-connection: true' to be in line with our current implementation.
Note that the use of 'deny-connection' is actively being discussed and we may change this. When developing this interface, please also note https://bugs.launchpad.net/snappy/+bug/1640874.
| + bus=session | ||
| + interface=com.canonical.applications.DownloadManager | ||
| + member=setDefaultThrottle, | ||
| +`) |
jdstrand
Dec 5, 2016
Contributor
The way snappy stitches together, explicit deny rules are problematic. Please instead remove these explicit denies and simply document that allowGSMDownload, createMmsDownload, exit and setDefaultThrottle are intentionally left out of downloadConnectedPlugAppArmor since they are privileged. Perhaps we can use an attribute or new interface (eg, download-manager-control) when snaps need access to these privileged methods.
| + interface=com.canonical.applications.DownloadManager | ||
| + member=downloadCreated | ||
| + peer=(label=unconfined), | ||
| +`) |
jdstrand
Dec 5, 2016
Contributor
I expected these to use label=###PLUG_SECURITY_TAGS### but you have label=unconfined. Can you explain? Is this for talking to applications shipped as debs on classic?
Elleo
Dec 13, 2016
Contributor
It was used for allowing the transfer-indicator to monitor downloads from other apps, but that would only have applied on a unity8 deb desktop. In the unity8 snap currently UDM and the transfer-indicator will be in the same snap so it won't matter, and if UDM gets split out at some point I think it'd be best to add a new restricted interface for "download-manager-observer" or similar; so I've removed them for now.
| + | ||
| +# dbus | ||
| +connect | ||
| +getsockname |
| + | ||
| +# dbus | ||
| +connect | ||
| +getsockname |
| +type DownloadInterface struct{} | ||
| + | ||
| +func (iface *DownloadInterface) Name() string { | ||
| + return "download" |
jdstrand
Dec 5, 2016
Contributor
I think this is far too generic for an interface name since 'network' allows you to 'download' things. Perhaps 'download-manager'? @niemeyer - what are your thoughts?
Elleo
added some commits
Dec 7, 2016
| + | ||
| +# Allow writing to app download directories | ||
| +owner @{HOME}/snap/*/common/Downloads/ rw, | ||
| +owner @{HOME}/snap/*/common/Downloads/** rwk, |
ted-gould
Dec 8, 2016
Seems like the download manager should only get access to the directories of snaps that are connected. So this should be a per-slot connection with the name of the package in it.
Elleo
added some commits
Dec 12, 2016
Some checks were not successful
Some checks were not successful
jdstrand
reviewed
Dec 13, 2016
A coupled of nit picks and a question regarding the system bus. If you remove the system bus access, +1, otherwise need to look into that further.
| + are deliberately left out of this profile due to their privileged nature. */ | ||
| +var downloadConnectedPlugAppArmor = []byte(` | ||
| +# Description: Can access the download manager. | ||
| +# Usage: common |
jdstrand
Dec 13, 2016
Contributor
With the base declaration, all these 'Usage' metatags can be dropped.
| +# Description: Can access the download manager. | ||
| +# Usage: common | ||
| + | ||
| +#include <abstractions/dbus-strict> |
jdstrand
Dec 13, 2016
Contributor
I just noticed this. Why do you need access to the system bus? All your apparmor rules are for the session bus.
| +# Usage: common | ||
| + | ||
| +# DBus accesses | ||
| +#include <abstractions/dbus-strict> |
jdstrand
Dec 13, 2016
Contributor
Why do you need access to the system bus here and in the communications with dbus-daemon?
| +# Allow writing to app download directories | ||
| +owner @{HOME}/snap/###PLUG_NAME###/common/Downloads/ rw, | ||
| +owner @{HOME}/snap/###PLUG_NAME###/common/Downloads/** rwk, | ||
| + |
|
One other thing: please get sign-off on 'download-manager' as the name of the interface from @niemeyer |
chipaca
added
Decaying
Reviewed
labels
Dec 14, 2016
Elleo
added some commits
Dec 14, 2016
Some checks were not successful
|
Thanks for making these changes. Approving, but please get signoff from @niemeyer on the interface name 'download-manager'. |
chipaca
added some commits
Jan 16, 2017
chipaca
John Lenton
Some checks were not successful
chipaca
removed
Decaying
Reviewed
labels
Jan 16, 2017
chipaca
added some commits
Jan 18, 2017
chipaca
merged commit 8251637
into
snapcore:master
Jan 18, 2017
5 of 6 checks passed
|
The name unity8-download-manager seems a bit odd to me? Surely we don't want the download-manager to always be restricted to only being a unity8 service, otherwise we're disincentivising developers to make use of it in their applications since they wouldn't want them tied to a single desktop environment? |
|
Can you please raise this topic on the snapcraft list? A similar naming scheme was used on unity8-calendar and unity8-contacts, so we need to either undo those as well or establish a different pattern which may be used for all of them. The counter argument to "download-manager" is that this is an extremely general name, rather than a noun associated with a particular piece of software. So it ends up being a poor name to the interface as well. |
|
Ah, my apologies. As you were requested to raise the issue with Gustavo a month ago and didn't, I talked with him myself and it seems I misrepresented the project. |
|
@niemeyer note the |
Elleo commentedNov 22, 2016
Add an interface that allows download manager clients to talk to a confined ubuntu-download-manager service. This also introduces the SNAP_LABEL_DBUS template variable which is required by UDM for creating application specific dbus paths.
The plug app armor rules are based on the original click rules, however the slot rules are new as UDM was previously unconfined on click systems.