interfaces/builtin: add support for content "source" section #4068

Open
wants to merge 8 commits into
from
@@ -78,6 +78,16 @@ func (iface *contentInterface) SanitizeSlot(slot *interfaces.Slot) error {
slot.Attrs["content"] = slot.Name
}
+ // Error if "read" or "write" are present alongside "source".
+ if _, ok := slot.Attrs["source"]; ok {
+ if _, ok := slot.Attrs["read"]; ok {
+ return fmt.Errorf(`move the "read" attribute into the "source" section`)
@mvo5

mvo5 Nov 8, 2017

Collaborator

Maybe a bit more context here? i.e. move the "read" attribute of slot %q into the "source" section and pass slot.Name in ? Same below.

+ }
+ if _, ok := slot.Attrs["write"]; ok {
+ return fmt.Errorf(`move the "write" attribute into the "source" section`)
+ }
+ }
+
// check that we have either a read or write path
rpath := iface.path(slot, "read")
wpath := iface.path(slot, "write")
@@ -93,7 +103,6 @@ func (iface *contentInterface) SanitizeSlot(slot *interfaces.Slot) error {
return fmt.Errorf("content interface path is not clean: %q", p)
}
}
-
return nil
}
@@ -124,7 +133,12 @@ func (iface *contentInterface) path(slot *interfaces.Slot, name string) []string
panic("internal error, path can only be used with read/write")
}
- paths, ok := slot.Attrs[name].([]interface{})
+ source, ok := slot.Attrs["source"].(map[string]interface{})
@stolowski

stolowski Oct 26, 2017

Contributor

I think it would be worth checking if the new syntax with "source" is not mixed with the old one ("read" or "write" at the same level as "source", in which case "source" would win and "old" ones would be silently ignored I think).

@zyga

zyga Oct 30, 2017

Contributor

Done in 2777b37

+ if !ok {
+ source = slot.Attrs
+ }
+
+ paths, ok := source[name].([]interface{})
if !ok {
return nil
}
@@ -165,9 +179,15 @@ func mountEntry(plug *interfaces.Plug, slot *interfaces.Slot, relSrc string, ext
options := make([]string, 0, len(extraOptions)+1)
options = append(options, "bind")
options = append(options, extraOptions...)
+ source := resolveSpecialVariable(relSrc, slot.Snap)
+ target := resolveSpecialVariable(plug.Attrs["target"].(string), plug.Snap)
+ if _, ok := slot.Attrs["source"].(map[string]interface{}); ok {
@mvo5

mvo5 Nov 8, 2017

Collaborator

Maybe a small comment like: // check if new style syntax is used ? Or maybe even a tiny helper. AIUI this check for the slot.Attr["source"].(map...) is just doing that, right?

+ _, sourceName := filepath.Split(source)
+ target = filepath.Join(target, sourceName)
+ }
return mount.Entry{
- Name: resolveSpecialVariable(relSrc, slot.Snap),
- Dir: resolveSpecialVariable(plug.Attrs["target"].(string), plug.Snap),
+ Name: source,
+ Dir: target,
Options: options,
}
}
@@ -120,6 +120,25 @@ slots:
}
}
+func (s *ContentSuite) TestSanitizeSlotSourceAndLegacy(c *C) {
+ slot := MockSlot(c, `name: snap
+slots:
+ content:
+ source:
+ write: [$SNAP_DATA/stuff]
+ read: [$SNAP/shared]
+`, nil, "content")
+ c.Assert(slot.Sanitize(s.iface), ErrorMatches, `move the "read" attribute into the "source" section`)
+ slot = MockSlot(c, `name: snap
+slots:
+ content:
+ source:
+ read: [$SNAP/shared]
+ write: [$SNAP_DATA/stuff]
+`, nil, "content")
+ c.Assert(slot.Sanitize(s.iface), ErrorMatches, `move the "write" attribute into the "source" section`)
+}
+
func (s *ContentSuite) TestSanitizePlugSimple(c *C) {
const mockSnapYaml = `name: content-slot-snap
version: 1.0
@@ -376,3 +395,209 @@ slots:
func (s *ContentSuite) TestInterfaces(c *C) {
c.Check(builtin.Interfaces(), testutil.DeepContains, s.iface)
}
+
+func (s *ContentSuite) TestModernContentInterface(c *C) {
+ plug := MockPlug(c, `name: consumer
+plugs:
+ content:
+ target: $SNAP_COMMON/import
+apps:
+ app:
+ command: foo
+`, &snap.SideInfo{Revision: snap.R(1)}, "content")
+
+ slot := MockSlot(c, `name: producer
+slots:
+ content:
+ source:
+ read:
+ - $SNAP_COMMON/read-common
+ - $SNAP_DATA/read-data
+ - $SNAP/read-snap
+ write:
+ - $SNAP_COMMON/write-common
+ - $SNAP_DATA/write-data
@stolowski

stolowski Oct 26, 2017

Contributor

What happens if both read and write have same entries?

@zyga

zyga Oct 27, 2017

Contributor

This is pretty interesting, we'll get a clash and those get de-conflicted. I'll add a test that shows what would happen.

@zyga

zyga Oct 30, 2017

Contributor

Done in 06c8556

+`, &snap.SideInfo{Revision: snap.R(2)}, "content")
+
+ // Create the mount and apparmor specifications.
+ mountSpec := &mount.Specification{}
+ c.Assert(mountSpec.AddConnectedPlug(s.iface, plug, nil, slot, nil), IsNil)
+ apparmorSpec := &apparmor.Specification{}
+ c.Assert(apparmorSpec.AddConnectedPlug(s.iface, plug, nil, slot, nil), IsNil)
+
+ // Analyze the mount specification.
+ expectedMnt := []mount.Entry{{
+ Name: "/var/snap/producer/common/read-common",
+ Dir: "/var/snap/consumer/common/import/read-common",
+ Options: []string{"bind", "ro"},
+ }, {
+ Name: "/var/snap/producer/2/read-data",
+ Dir: "/var/snap/consumer/common/import/read-data",
+ Options: []string{"bind", "ro"},
+ }, {
+ Name: "/snap/producer/2/read-snap",
+ Dir: "/var/snap/consumer/common/import/read-snap",
+ Options: []string{"bind", "ro"},
+ }, {
+ Name: "/var/snap/producer/common/write-common",
+ Dir: "/var/snap/consumer/common/import/write-common",
+ Options: []string{"bind"},
+ }, {
+ Name: "/var/snap/producer/2/write-data",
+ Dir: "/var/snap/consumer/common/import/write-data",
+ Options: []string{"bind"},
+ }}
+ c.Assert(mountSpec.MountEntries(), DeepEquals, expectedMnt)
+
+ // Analyze the apparmor specification.
+ c.Assert(apparmorSpec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
+ expected := `
+# In addition to the bind mount, add any AppArmor rules so that
+# snaps may directly access the slot implementation's files. Due
+# to a limitation in the kernel's LSM hooks for AF_UNIX, these
+# are needed for using named sockets within the exported
+# directory.
+/var/snap/producer/common/write-common/** mrwklix,
+/var/snap/producer/2/write-data/** mrwklix,
+
+# In addition to the bind mount, add any AppArmor rules so that
+# snaps may directly access the slot implementation's files
+# read-only.
+/var/snap/producer/common/read-common/** mrkix,
+/var/snap/producer/2/read-data/** mrkix,
+/snap/producer/2/read-snap/** mrkix,
+`
+ c.Assert(apparmorSpec.SnippetForTag("snap.consumer.app"), Equals, expected)
+}
+
+func (s *ContentSuite) TestModernContentInterfacePlugins(c *C) {
+ // Define one app snap and two snaps plugin snaps.
+ plug := MockPlug(c, `name: app
+plugs:
+ plugins:
+ interface: content
+ content: plugin-for-app
+ target: $SNAP/plugins
+apps:
+ app:
+ command: foo
+
+`, &snap.SideInfo{Revision: snap.R(1)}, "plugins")
+
+ // XXX: realistically the plugin may be a single file and we don't support
+ // those very well.
+ slotOne := MockSlot(c, `name: plugin-one
+slots:
+ plugin-for-app:
+ interface: content
+ source:
+ read: [$SNAP/plugin]
+`, &snap.SideInfo{Revision: snap.R(1)}, "plugin-for-app")
+
+ slotTwo := MockSlot(c, `name: plugin-two
+slots:
+ plugin-for-app:
+ interface: content
+ source:
+ read: [$SNAP/plugin]
+`, &snap.SideInfo{Revision: snap.R(1)}, "plugin-for-app")
+
+ // Create the mount and apparmor specifications.
+ mountSpec := &mount.Specification{}
+ apparmorSpec := &apparmor.Specification{}
+ for _, slot := range []*interfaces.Slot{slotOne, slotTwo} {
+ c.Assert(mountSpec.AddConnectedPlug(s.iface, plug, nil, slot, nil), IsNil)
+ c.Assert(apparmorSpec.AddConnectedPlug(s.iface, plug, nil, slot, nil), IsNil)
+ }
+
+ // Analyze the mount specification.
+ expectedMnt := []mount.Entry{{
+ Name: "/snap/plugin-one/1/plugin",
+ Dir: "/snap/app/1/plugins/plugin",
+ Options: []string{"bind", "ro"},
+ }, {
+ Name: "/snap/plugin-two/1/plugin",
+ Dir: "/snap/app/1/plugins/plugin-2",
+ Options: []string{"bind", "ro"},
+ }}
+ c.Assert(mountSpec.MountEntries(), DeepEquals, expectedMnt)
+
+ // Analyze the apparmor specification.
+ //
+ // NOTE: the paths below refer to the original locations and are *NOT*
+ // altered like the mount entries above. This is intended. See the comment
+ // below for explanation as to why those are necessary.
+ c.Assert(apparmorSpec.SecurityTags(), DeepEquals, []string{"snap.app.app"})
+ expected := `
+# In addition to the bind mount, add any AppArmor rules so that
+# snaps may directly access the slot implementation's files
+# read-only.
+/snap/plugin-one/1/plugin/** mrkix,
+
+
+# In addition to the bind mount, add any AppArmor rules so that
+# snaps may directly access the slot implementation's files
+# read-only.
+/snap/plugin-two/1/plugin/** mrkix,
+`
+ c.Assert(apparmorSpec.SnippetForTag("snap.app.app"), Equals, expected)
+}
+
+func (s *ContentSuite) TestModernContentSameReadAndWriteClash(c *C) {
+ plug := MockPlug(c, `name: consumer
+plugs:
+ content:
+ target: $SNAP_COMMON/import
+apps:
+ app:
+ command: foo
+`, &snap.SideInfo{Revision: snap.R(1)}, "content")
+
+ slot := MockSlot(c, `name: producer
+slots:
+ content:
+ source:
+ read:
+ - $SNAP_DATA/directory
+ write:
+ - $SNAP_DATA/directory
+`, &snap.SideInfo{Revision: snap.R(2)}, "content")
+
+ // Create the mount and apparmor specifications.
+ mountSpec := &mount.Specification{}
+ c.Assert(mountSpec.AddConnectedPlug(s.iface, plug, nil, slot, nil), IsNil)
+ apparmorSpec := &apparmor.Specification{}
+ c.Assert(apparmorSpec.AddConnectedPlug(s.iface, plug, nil, slot, nil), IsNil)
+
+ // Analyze the mount specification
+ expectedMnt := []mount.Entry{{
+ Name: "/var/snap/producer/2/directory",
+ Dir: "/var/snap/consumer/common/import/directory",
+ Options: []string{"bind", "ro"},
+ }, {
+ Name: "/var/snap/producer/2/directory",
+ Dir: "/var/snap/consumer/common/import/directory-2",
+ Options: []string{"bind"},
+ }}
+ c.Assert(mountSpec.MountEntries(), DeepEquals, expectedMnt)
+
+ // Analyze the apparmor specification.
+ //
+ // NOTE: Although there are duplicate entries with different permissions
+ // one is a superset of the other so they do not conflict.
+ c.Assert(apparmorSpec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
+ expected := `
+# In addition to the bind mount, add any AppArmor rules so that
+# snaps may directly access the slot implementation's files. Due
+# to a limitation in the kernel's LSM hooks for AF_UNIX, these
+# are needed for using named sockets within the exported
+# directory.
+/var/snap/producer/2/directory/** mrwklix,
+
+# In addition to the bind mount, add any AppArmor rules so that
+# snaps may directly access the slot implementation's files
+# read-only.
+/var/snap/producer/2/directory/** mrkix,
+`
+ c.Assert(apparmorSpec.SnippetForTag("snap.consumer.app"), Equals, expected)
+}
@@ -20,6 +20,8 @@
package mount
import (
+ "fmt"
+
"github.com/snapcore/snapd/interfaces"
"github.com/snapcore/snapd/snap"
)
@@ -43,6 +45,16 @@ func (spec *Specification) AddMountEntry(e Entry) error {
func (spec *Specification) MountEntries() []Entry {
result := make([]Entry, len(spec.mountEntries))
copy(result, spec.mountEntries)
+ // Number each entry, in case we get clashes this will automatically give
+ // them unique names.
+ count := make(map[string]int, len(spec.mountEntries))
@mvo5

mvo5 Nov 8, 2017

Collaborator

This is nice - but I slightly wonder about the results here. In the corresponding test I see /var/snap/consumer/common/import/directory and /var/snap/consumer/common/import/directory-2 one is ro, one is rw. This makes me wonder two things: a) is the order stable, i.e. is directory-2 always the rw one? b) will that be useful for an application? i.e. what is the advantage over just rejecting clashing names?

@zyga

zyga Nov 8, 2017

Contributor

I'll answer the ro/rw aspect separately.

The de-clashing allows us to have a system where plugin authors don't have to coordinate with each on directory names. The de-clashing aspect is also described on the forum (link is in the commit message).

+ for i := range result {
+ path := result[i].Dir
+ count[path] += 1
+ if c := count[path]; c > 1 {
+ result[i].Dir += fmt.Sprintf("-%d", c)
+ }
+ }
return result
}
@@ -39,16 +39,16 @@ var _ = Suite(&specSuite{
iface: &ifacetest.TestInterface{
InterfaceName: "test",
MountConnectedPlugCallback: func(spec *mount.Specification, plug *interfaces.Plug, plugAttrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error {
- return spec.AddMountEntry(mount.Entry{Name: "connected-plug"})
+ return spec.AddMountEntry(mount.Entry{Dir: "dir-a", Name: "connected-plug"})
},
MountConnectedSlotCallback: func(spec *mount.Specification, plug *interfaces.Plug, plugAttrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error {
- return spec.AddMountEntry(mount.Entry{Name: "connected-slot"})
+ return spec.AddMountEntry(mount.Entry{Dir: "dir-b", Name: "connected-slot"})
},
MountPermanentPlugCallback: func(spec *mount.Specification, plug *snap.PlugInfo) error {
- return spec.AddMountEntry(mount.Entry{Name: "permanent-plug"})
+ return spec.AddMountEntry(mount.Entry{Dir: "dir-c", Name: "permanent-plug"})
},
MountPermanentSlotCallback: func(spec *mount.Specification, slot *snap.SlotInfo) error {
- return spec.AddMountEntry(mount.Entry{Name: "permanent-slot"})
+ return spec.AddMountEntry(mount.Entry{Dir: "dir-d", Name: "permanent-slot"})
},
},
plug: &interfaces.Plug{
@@ -73,8 +73,8 @@ func (s *specSuite) SetUpTest(c *C) {
// AddMountEntry is not broken
func (s *specSuite) TestSmoke(c *C) {
- ent0 := mount.Entry{Name: "fs1"}
- ent1 := mount.Entry{Name: "fs2"}
+ ent0 := mount.Entry{Dir: "dir-a", Name: "fs1"}
+ ent1 := mount.Entry{Dir: "dir-b", Name: "fs2"}
c.Assert(s.spec.AddMountEntry(ent0), IsNil)
c.Assert(s.spec.AddMountEntry(ent1), IsNil)
c.Assert(s.spec.MountEntries(), DeepEquals, []mount.Entry{ent0, ent1})
@@ -88,6 +88,8 @@ func (s *specSuite) TestSpecificationIface(c *C) {
c.Assert(r.AddPermanentPlug(s.iface, s.plug.PlugInfo), IsNil)
c.Assert(r.AddPermanentSlot(s.iface, s.slot.SlotInfo), IsNil)
c.Assert(s.spec.MountEntries(), DeepEquals, []mount.Entry{
- {Name: "connected-plug"}, {Name: "connected-slot"},
- {Name: "permanent-plug"}, {Name: "permanent-slot"}})
+ {Dir: "dir-a", Name: "connected-plug"},
+ {Dir: "dir-b", Name: "connected-slot"},
+ {Dir: "dir-c", Name: "permanent-plug"},
+ {Dir: "dir-d", Name: "permanent-slot"}})
}