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/builtin: add online-accounts-service interface #2869
Conversation
All checks have failed
niemeyer
changed the title from
Add Online Accounts interface
to
interfaces/builtin: add interface related to online accounts (?)
Feb 23, 2017
niemeyer
requested changes
Feb 23, 2017
I'll let @jdstrand take the first pass on this, but as an early comment we need a better scoped name for the interface. "online-accounts" doesn't give any hint about what the snap is really asking for or being provided.
niemeyer
requested a review
from
jdstrand
Feb 23, 2017
|
We can prefix it with "ubuntu-" to distinguish it from other implementations, but a name is just a name, it's not a description. I don't find "bluez" or "dcdbas" very descriptive either :-) |
|
"bluez" is the well-known name of one particular software. "online accounts" is a concept. "ubuntu-online-accounts" doesn't seem so great either, as we have many of those (unfortunately). Can you provide a better idea of what this is really giving access to? |
|
The interface gives access to the online accounts API, which is described here: For a more user-oriented look, you can have a look at the UX design page: |
|
Thanks for the details. Let's go with your @jdstrand This is just waiting for your review now. |
niemeyer
changed the title from
interfaces/builtin: add interface related to online accounts (?)
to
interfaces/builtin: add ubuntu-online-accounts interface
Mar 10, 2017
|
Wait.. I propose something else here: let's establish a new fallback pattern and call this |
niemeyer
changed the title from
interfaces/builtin: add ubuntu-online-accounts interface
to
interfaces/builtin: add online-accounts-service interface
Mar 10, 2017
|
Just for emphasis, we shouldn't append the |
All checks have failed
|
I've just renamed the interface to |
|
Sorry for the trouble, but can you please rename to |
mardy
added some commits
Mar 10, 2017
All checks have failed
|
Sure, done :-) |
| +var onlineAccountsServiceConnectedPlugAppArmor = []byte(` | ||
| +# Description: Allow using Online Accounts service. Common because the access | ||
| +# to user data is actually mediated by the Online Accounts service itself. | ||
| +# Usage: common |
jdstrand
Mar 10, 2017
Contributor
Please drop 'Usage' metadata here and elsewhere. It is no longer needed with the base declaration.
| +dbus (receive, send) | ||
| + bus=session | ||
| + interface=com.ubuntu.OnlineAccounts.Manager | ||
| + peer=(label=###SLOT_SECURITY_TAGS###), |
| + peer=(label=###SLOT_SECURITY_TAGS###), | ||
| +`) | ||
| + | ||
| +var onlineAccountsServicePermanentSlotSecComp = []byte(` |
jdstrand
Mar 10, 2017
Contributor
Please use const instead of var here and elsewhere as that is the coding style for security policy.
| +sendmmsg | ||
| +sendmsg | ||
| +sendto | ||
| +shutdown |
| +send | ||
| +sendto | ||
| +sendmsg | ||
| +`) |
jdstrand
Mar 10, 2017
Contributor
This can just be dropped since it is all recv* and send* syscalls.
| + <allow send_destination="com.ubuntu.OnlineAccounts.Manager"/> | ||
| + <allow send_interface="com.ubuntu.OnlineAccounts.Manager"/> | ||
| +</policy> | ||
| +`) |
jdstrand
Mar 10, 2017
Contributor
I don't think this dbus policy is quite right because, AIUI, this is meant to be run as a session service, but the DBus backend currently only supports system bus services. Is online accounts DBus-activatable? If so, you will be interested in: #2592
| + return nil, nil | ||
| + default: | ||
| + return nil, nil | ||
| + } |
| +func (iface *OnlineAccountsServiceInterface) ConnectedPlugSnippet(plug *interfaces.Plug, slot *interfaces.Slot, securitySystem interfaces.SecuritySystem) ([]byte, error) { | ||
| + switch securitySystem { | ||
| + case interfaces.SecurityAppArmor: | ||
| + return onlineAccountsServiceConnectedPlugAppArmor, nil |
jdstrand
Mar 10, 2017
•
Edited 1 time
-
jdstrand
Mar 27, 2017
Contributor
-
Mar 27, 2017
You need to do the substitutions for ###SLOT_SECURITY_TAGS### in onlineAccountsServiceConnectedPlugAppArmor here. See the mir interface for an example.
| + case interfaces.SecuritySecComp: | ||
| + return onlineAccountsServiceConnectedPlugSecComp, nil | ||
| + case interfaces.SecurityUDev, interfaces.SecurityMount: | ||
| + return nil, nil |
| + case interfaces.SecuritySecComp: | ||
| + return onlineAccountsServicePermanentSlotSecComp, nil | ||
| + case interfaces.SecurityUDev, interfaces.SecurityMount: | ||
| + return nil, nil |
All checks have failed
|
Branch updated, I believe I addressed all points. |
|
@mardy you need to fix those test failures |
chipaca
added some commits
Mar 15, 2017
|
@mardy fixed |
mardy
and others
added some commits
Mar 15, 2017
Some checks were not successful
Some checks were not successful
|
I've updated the branch for the API changes in master. |
jdstrand
requested changes
Mar 27, 2017
Thanks for all the updates! In general the base decalaraion, the security policy and tests look good with a few more small things.
You need to change over to using the new spec implementation that landed in recent weeks and address the testsuite failures.
| + allow-installation: | ||
| + slot-snap-type: | ||
| + - app | ||
| + deny-connection: true |
jdstrand
Mar 27, 2017
Contributor
The base declaration is fine since this is a trusted helper that is safe for all snaps to auto-connect.
jdstrand
May 5, 2017
•
Edited 1 time
-
jdstrand
May 5, 2017
Contributor
-
May 5, 2017
This was mostly talking about how auto-connection is allowed (because it is omitted), but to make this clear, the base declaration is saying that:
- the slot implementation may only be supplied by app snaps
- slot implementations by default may not use this interface
- plugging snaps are auto-connected by default
If there is a slot implementation that is deemed safe and operates how the interface is designed (and safe for auto-connection), then a snap declaration can be issued to the slot implementation snap allowing it to use the interface and then all snaps plugging this interface auto-connect.
jdstrand
May 5, 2017
Contributor
This is in contrast to say, an implicit interface that is provided by core for some service. In that situation, we trust that the service is operating within the interface design constraints because the service is coming from a trusted archive (eg, the Ubuntu archive). With app snap slot implementations, that guarantee is not there so we block the slot implementation by default.
| + | ||
| +const onlineAccountsServicePermanentSlotAppArmor = ` | ||
| +# Description: Allow operating as the Online Accounts service. Reserved because | ||
| +# this gives privileged access to the system. |
jdstrand
Mar 27, 2017
Contributor
You can remove the 'Reserved' sentence since there is nothing particularly privileged in the interface beyond what the base declaration already implies.
| + | ||
| +const onlineAccountsServiceConnectedPlugAppArmor = ` | ||
| +# Description: Allow using Online Accounts service. Common because the access | ||
| +# to user data is actually mediated by the Online Accounts service itself. |
jdstrand
Mar 27, 2017
Contributor
Instead of saying 'Common' say 'Allowed to auto-connect because...'
| + iface: &builtin.OnlineAccountsServiceInterface{}, | ||
| + slot: &interfaces.Slot{ | ||
| + SlotInfo: &snap.SlotInfo{ | ||
| + Snap: &snap.Info{SuggestedName: "core", Type: snap.TypeOS}, |
jdstrand
Mar 27, 2017
Contributor
Please name this something other than "core" since "core" implies this is an implicit core interface which it is not. I suggest 'service".
| + // verify apparmor connected | ||
| + c.Assert(string(snippet), testutil.Contains, "#include <abstractions/dbus-session-strict>") | ||
| + // verify classic didn't connect | ||
| + c.Assert(string(snippet), Not(testutil.Contains), "peer=(label=unconfined),") |
| + c.Assert(snippet, Not(IsNil)) | ||
| + | ||
| + c.Check(string(snippet), testutil.Contains, "peer=(label=\"snap.other.*\")") | ||
| +} |
jdstrand
Mar 27, 2017
Contributor
Can you add another similar test for ConnectedPlugSnippetAppArmor?
Some checks were not successful
All checks have passed
|
Branch updated. |
jdstrand
approved these changes
Apr 18, 2017
There are two testsuite fixes I'd like to see, but this is fine to merge as is, so approving.
| + c.Assert(apparmorSpec.AddConnectedPlug(s.iface, s.plug, s.coreSlot), IsNil) | ||
| + snippet := apparmorSpec.SnippetForTag("snap.other.app") | ||
| + // verify apparmor connected | ||
| + c.Assert(snippet, testutil.Contains, "#include <abstractions/dbus-session-strict>") |
jdstrand
Apr 18, 2017
Contributor
Please add:
c.Check(snippet, testutil.Contains, "peer=(label=\"snap.service.oa\")")
| + // verify apparmor connected | ||
| + c.Assert(snippet, testutil.Contains, "#include <abstractions/dbus-session-strict>") | ||
| + // verify classic didn't connect | ||
| + c.Assert(snippet, Not(testutil.Contains), "peer=(label=unconfined),") |
mvo5
added some commits
Apr 20, 2017
Some checks were not successful
|
This looks good to me, I addressed the points that @jdstrand raised. I'm |
All checks have failed
zyga
reviewed
May 5, 2017
Two questions. I'm happy to iterate to address those given the answer (CC @jdstrand on this)
| + allow-installation: | ||
| + slot-snap-type: | ||
| + - app | ||
| + deny-connection: true |
jdstrand
Mar 27, 2017
Contributor
The base declaration is fine since this is a trusted helper that is safe for all snaps to auto-connect.
jdstrand
May 5, 2017
•
Edited 1 time
-
jdstrand
May 5, 2017
Contributor
-
May 5, 2017
This was mostly talking about how auto-connection is allowed (because it is omitted), but to make this clear, the base declaration is saying that:
- the slot implementation may only be supplied by app snaps
- slot implementations by default may not use this interface
- plugging snaps are auto-connected by default
If there is a slot implementation that is deemed safe and operates how the interface is designed (and safe for auto-connection), then a snap declaration can be issued to the slot implementation snap allowing it to use the interface and then all snaps plugging this interface auto-connect.
jdstrand
May 5, 2017
Contributor
This is in contrast to say, an implicit interface that is provided by core for some service. In that situation, we trust that the service is operating within the interface design constraints because the service is coming from a trusted archive (eg, the Ubuntu archive). With app snap slot implementations, that guarantee is not there so we block the slot implementation by default.
| + bus=session | ||
| + interface=com.ubuntu.OnlineAccounts.Manager | ||
| + path=/com/ubuntu/OnlineAccounts{,/**} | ||
| + peer=(label=###SLOT_SECURITY_TAGS###), |
jdstrand
May 5, 2017
Contributor
Yes. Please add:
# Allow clients to introspect the service
dbus (send)
bus=session
interface=org.freedesktop.DBus.Introspectable
path=/com/ubuntu/OnlineAccounts
member=Introspect
peer=(label=###SLOT_SECURITY_TAGS###),
I have not tested this, but based on the object path in the above rule, it should work.
zyga
added some commits
May 8, 2017
Some checks were not successful
All checks have passed
Some checks were not successful
Some checks were not successful
| @@ -42,7 +42,6 @@ var _ = Suite(&AllSuite{}) | ||
| func (s *AllSuite) TestInterfaces(c *C) { | ||
| all := builtin.Interfaces() | ||
| - c.Check(all, Contains, &builtin.StorageFrameworkServiceInterface{}) |
mvo5
and others
added some commits
May 10, 2017
Some checks were not successful
All checks have passed
All checks have passed
mardy commentedFeb 16, 2017
No description provided.