interfaces/builtin: add storage-framework-service interface

[michihenning]

This adds a snapd interface for use by storage-framework. On the slot side, it allows use of aa_is_enabled() and binding to the storage framework registry and provider DBus interfaces. On the plug side, it allows clients to talk to these DBus interfaces.

[zyga]

Just two comments, I didn't review the policy yet

[zyga]

This seems unrelated, did you change it explicitly or did go ftm decided to do so?

[michihenning]

That wasn't me. I think go fmt is responsible.

[zyga]

Storage framework is a generic name. Is there any documentation for what the canonical storage framework is doing?

[michihenning]

We have an lp project for it: lp:storage-framework. We are waiting for management to allocate time to write documentation.

[mvo5]

There is a test failure related to formating: https://travis-ci.org/snapcore/snapd/jobs/204858792#L408 - you may need to run gofmt -s -w on these file(s).

[mvo5]

I pushed the go fmt fix.

[michihenning]

I pushed the go fmt fix.

Thanks for that! My apologies for not replying sooner. Somehow, I missed the notification email for your comments.

[niemeyer]

Let's please call this interface storage-framework-service.

Rationale for this was provided in PR #2869.

[niemeyer]

@jdstrand This should be an easy one.

[michihenning]

Just checking: the thumbnailer is a service too, and its interface is called "thumbnailer". I chose "storage-framework" for consistency. I don't mind changing it to "storage-framework-service". Just let me know.

[jdstrand]

Overall the direction and implementation is good, but there is an open question on ###PLUG_SNAP_NAME### and need a few more tests

[jdstrand]

###PLUG_SNAP_NAME### isn't currently in storageFrameworkConnectedSlotAppArmor. Was it meant to be used as part of the path in this rule:

dbus (receive, send)
    bus=session
    interface=com.canonical.StorageFramework.Provider.*
    path=/provider/*
    peer=(label=###PLUG_SECURITY_TAGS###),

If so, then I like that idea a lot.

[michihenning]

The storage framework doesn't need access to any files on the plug side, so PLUG_SNAP_NAME isn't needed here.

[michihenning]

The substitution was a copy-and-paste error and was redundant. I've removed it.

[jdstrand]

It would be nice to add a test that the rules you are adding for ConnectedSlot and ConnectedPlug policy are preset similar to thumbnailer_test.go in TestSlotGrantedAccessToPlugFiles().

[michihenning]

See above. Because the service doesn't access to anything on the plug side, there is no need to test whether access is granted.

[niemeyer]

@michihenning We can't have interfaces called "online-accounts" or "storage-framework" when they are talking about very specific software implementations. snapd is widely scoped and a multi-Linux-distribution project, so we need to think about how much sense it makes for someone to be siting at a console in a completely different place and looking at these names. "thumbnailer" feels bad as well, actually. I'd prefer to rename that one too.

[michihenning]

I think I've made all the changes. The autopkg test failures are unrelated to this pull request.

I'll do a separate pull request to rename thumbnailer -> thumbnailer-service.

[jdstrand]

Getting close, just a few more changes.

[jdstrand]

The base declaration LGTM.

[jdstrand]

I think you need to run go fmt again to fix these.

[jdstrand]

Can you add a comment for this? I suggest:

# libapparmor query interface needs 'w' to perform the query and 'r' to
# read the result. This is an information leak because in addition to
# allowing querying policy for any label (precisely what
# storage-framework needs), it also allows checking the existence of
# any label.
/sys/kernel/security/apparmor/.access rw,

[jdstrand]

Can you add storageFrameworkServicePermanentSlotSecComp with the following:

const storageFrameworkServicePermanentSlotSeccomp = `
bind
`

Your storage-framework snap may be specifying 'plugs: [ network-bind ]' that is giving you this syscall, but the slots side should have it on its own to bind to the DBus well-known name.

[zyga]

This is done

[jdstrand]

Can you add:

func (iface *StorageFrameworkServiceInterface) SecCompPermanentSlot(spec *seccomp.Specification, slot *interfaces.Slot) error {
	spec.AddSnippet(storageFrameworkServicePermanentSlotSecComp)
	return nil
}

[zyga]

This is done

[jdstrand]

This should be:

new := plugAppLabelExpr(plug)

[jdstrand]

You should drop storageFrameworkServiceMockClassicSlotSnapInfoYaml because the storage-framework-service interface is not an implicit interface and the basedeclaration says only snaps of type 'app' may slot this interface.

[jdstrand]

Please remove these classic checks (see above).

[jdstrand]

This will need to be changed to "snap.client.app" due to the requested changes in AppArmorConnectedSlot() (see above).

[jdstrand]

Can you add corresponding tests for apparmorSpec.AddConnectedPlug?

[jdstrand]

Can you also check for the seccomp "bind". Eg:

seccompSpec := &seccomp.Specification{}
err := seccompSpec.AddPermanentSlot(s.iface, s.coreSlot)
c.Assert(err, IsNil)
c.Assert(seccompSpec.SecurityTags(), DeepEquals, []string{"snap.storage-framework-service.app"})
c.Check(seccompSpec.SnippetForTag("snap.storage-framework-service.app"), testutil.Contains, "bind\n")

[michihenning]

Thanks for the review Jamie! I apologize for being so clumsy. Much of this is new to me.
I take it that it is OK not to assign to classicSlot? (This is the one thing I'm not sure about. I think all the other changes you asked for are there now.)

[jdstrand]

It is fine to not assign to classicSlot AFAICS. In fact, you could probably remove it from StorageFrameworkServiceInterfaceSuite since you aren't doing anything with it. If it is decided that storage-framework-service would be provided outside of a snap, then we can start considering classic distro policy.

[zyga]

I iterated on the unit tests. Please have a look if anything is missing.

[zyga]

Hey @michihenning can you please merge master into this branch?
@niemeyer / @jdstrand can you please re-review this?

[jdstrand]

Thanks for your work on this!

[jdstrand]

Can you also check for the seccomp "bind". Eg:

seccompSpec := &seccomp.Specification{}
err := seccompSpec.AddPermanentSlot(s.iface, s.coreSlot)
c.Assert(err, IsNil)
c.Assert(seccompSpec.SecurityTags(), DeepEquals, []string{"snap.storage-framework-service.app"})
c.Check(seccompSpec.SnippetForTag("snap.storage-framework-service.app"), testutil.Contains, "bind\n")

[michihenning]

Thanks for the review Jamie! I apologize for being so clumsy. Much of this is new to me.
I take it that it is OK not to assign to classicSlot? (This is the one thing I'm not sure about. I think all the other changes you asked for are there now.)

[jdstrand]

It is fine to not assign to classicSlot AFAICS. In fact, you could probably remove it from StorageFrameworkServiceInterfaceSuite since you aren't doing anything with it. If it is decided that storage-framework-service would be provided outside of a snap, then we can start considering classic distro policy.

[zyga]

I iterated on the unit tests. Please have a look if anything is missing.

[zyga]

I did a pass over this branch and made it suitable for landing.

[jdstrand]

Can you add storageFrameworkServicePermanentSlotSecComp with the following:

const storageFrameworkServicePermanentSlotSeccomp = `
bind
`

Your storage-framework snap may be specifying 'plugs: [ network-bind ]' that is giving you this syscall, but the slots side should have it on its own to bind to the DBus well-known name.

[zyga]

This is done

[jdstrand]

Can you add:

func (iface *StorageFrameworkServiceInterface) SecCompPermanentSlot(spec *seccomp.Specification, slot *interfaces.Slot) error {
	spec.AddSnippet(storageFrameworkServicePermanentSlotSecComp)
	return nil
}

[zyga]

This is done

[jdstrand]

Can you also check for the seccomp "bind". Eg:

seccompSpec := &seccomp.Specification{}
err := seccompSpec.AddPermanentSlot(s.iface, s.coreSlot)
c.Assert(err, IsNil)
c.Assert(seccompSpec.SecurityTags(), DeepEquals, []string{"snap.storage-framework-service.app"})
c.Check(seccompSpec.SnippetForTag("snap.storage-framework-service.app"), testutil.Contains, "bind\n")

[michihenning]

Thanks for the review Jamie! I apologize for being so clumsy. Much of this is new to me.
I take it that it is OK not to assign to classicSlot? (This is the one thing I'm not sure about. I think all the other changes you asked for are there now.)

[jdstrand]

It is fine to not assign to classicSlot AFAICS. In fact, you could probably remove it from StorageFrameworkServiceInterfaceSuite since you aren't doing anything with it. If it is decided that storage-framework-service would be provided outside of a snap, then we can start considering classic distro policy.

[zyga]

I iterated on the unit tests. Please have a look if anything is missing.