interfaces/builtin: improve the bluez interface #1078

Merged
merged 19 commits into from Apr 29, 2016
Commits
Jump to file or symbol
Failed to load files and symbols.
+79 −19
Split
Viewing a subset of changes. View all

interfaces/builtin: specialize apparmor label based on bound apps

This patch specialized the bluez label used for dbus policy based on
how apps are bound to the bluez slot.

If there is exactly one app bound to the slot the label is precise, e.g.
"snap.bluez.bluez". If a subset of apps are bound to the slot (but not a
proper subset) then the label uses alternation, e.g.
"snap.bluez.{app1,app2}". Lastly if all the apps are bound to the bluez
slot then a wildcard is used, e.g. "snap.bluez.*".

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
  • Loading branch information...
commit c1c084f186a6e747db2d03109206d387760b73a4 @zyga zyga committed Apr 26, 2016
@@ -207,21 +207,31 @@ func (iface *BluezInterface) ConnectedPlugSnippet(plug *interfaces.Plug, slot *i
switch securitySystem {
case interfaces.SecurityAppArmor:
old := []byte("@SLOT_SECURITY_TAGS@")
@niemeyer

niemeyer Apr 29, 2016

Contributor

We already have a pattern for replacements. Can we please stick to a single one of those so we're not introducing these for each independent feature? (@foo@ vs. ###FOO### vs. {{foo}} vs ...).

@zyga

zyga Apr 29, 2016

Contributor

Ah, sorry, you are completely right. I guess I was looking at apparmor for too long. I'll fix this quickly.

@zyga

zyga Apr 29, 2016

Contributor

Fixed now

- buf := bytes.NewBuffer(nil)
- fmt.Fprintf(buf, "snap.%s.{", slot.Snap.Name())
- appNames := make([]string, 0, len(slot.Apps))
- for appName := range slot.Apps {
- appNames = append(appNames, appName)
- }
- sort.Strings(appNames)
- for i, appName := range appNames {
- if i > 0 {
- fmt.Fprintf(buf, ",")
+ var new []byte
+ switch {
+ case len(slot.Apps) == 1:
+ for appName := range slot.Apps {
@mvo5

mvo5 Apr 27, 2016

Collaborator

I see len(slot.Apps) == 1 and then a range slot.Apps. That loop seems to be not required if we know the size is just one? Or am I missing something?

@zyga

zyga Apr 27, 2016

Contributor

How do I get the name of the only key in a map?

+ new = []byte(fmt.Sprintf("snap.%s.%s", slot.Snap.Name(), appName))
+ }
+ case len(slot.Apps) == len(slot.Snap.Apps):
+ new = []byte(fmt.Sprintf("snap.%s.*", slot.Snap.Name()))
+ case len(slot.Apps) != len(slot.Snap.Apps):
+ buf := bytes.NewBuffer(nil)
+ fmt.Fprintf(buf, "snap.%s.{", slot.Snap.Name())
+ appNames := make([]string, 0, len(slot.Apps))
+ for appName := range slot.Apps {
+ appNames = append(appNames, appName)
+ }
+ sort.Strings(appNames)
+ for i, appName := range appNames {
@mvo5

mvo5 Apr 27, 2016

Collaborator

I wonder if this is more readable with: fmt.Fprintf(buf, "snap.%s.{%s}",slot.Snap.Name(), strings.Join(appNames, ",")). Or is the loop doing something that I miss that strings.Join() is not doing?

@morphis

morphis Apr 27, 2016

Contributor

We should also move this code somewhere else as I will otherwise copy&paste it to the networkmanager interface where we have to do the same thing.

@zyga

zyga Apr 27, 2016

Contributor

Yes, I was thinking the same thing. I'll move that to dbus support code in the same package.

@zyga

zyga Apr 27, 2016

Contributor

@mvo5 I don't have any preference, I can use join if you think that would look better.

@zyga

zyga Apr 28, 2016

Contributor

Done

+ if i > 0 {
+ fmt.Fprintf(buf, ",")
+ }
+ fmt.Fprintf(buf, appName)
}
- fmt.Fprintf(buf, appName)
+ fmt.Fprintf(buf, "}")
+ new = buf.Bytes()
}
@jdstrand

jdstrand Apr 26, 2016

Contributor

All this logic looks fine and I fully agree with the three cases. Personal preference would be to not use a case statement here since to my eyes it isn't as clear (if/else if/else would be clearer to me), but I won't block on that.

- fmt.Fprintf(buf, "}")
- new := buf.Bytes()
snippet := bytes.Replace(bluezConnectedPlugAppArmor, old, new, -1)
return snippet, nil
case interfaces.SecuritySecComp:
@@ -41,10 +41,6 @@ var _ = Suite(&BluezInterfaceSuite{
Snap: &snap.Info{SuggestedName: "bluez"},
Name: "bluez",
Interface: "bluez",
- Apps: map[string]*snap.AppInfo{
- "app1": &snap.AppInfo{Name: "app1"},
- "app2": &snap.AppInfo{Name: "app2"},
- },
},
},
plug: &interfaces.Plug{
@@ -60,12 +56,66 @@ func (s *BluezInterfaceSuite) TestName(c *C) {
c.Assert(s.iface.Name(), Equals, "bluez")
}
-func (s *BluezInterfaceSuite) TestConnectedPlugSnippetUsesSlotLabel(c *C) {
- snippet, err := s.iface.ConnectedPlugSnippet(s.plug, s.slot, interfaces.SecurityAppArmor)
+// The label glob when all apps are bound to the bluez slot
+func (s *BluezInterfaceSuite) TestConnectedPlugSnippetUsesSlotLabelAll(c *C) {
+ app1 := &snap.AppInfo{Name: "app1"}
+ app2 := &snap.AppInfo{Name: "app2"}
+ slot := &interfaces.Slot{
+ SlotInfo: &snap.SlotInfo{
+ Snap: &snap.Info{
+ SuggestedName: "bluez",
+ Apps: map[string]*snap.AppInfo{"app1": app1, "app2": app2},
+ },
+ Name: "bluez",
+ Interface: "bluez",
+ Apps: map[string]*snap.AppInfo{"app1": app1, "app2": app2},
+ },
+ }
+ snippet, err := s.iface.ConnectedPlugSnippet(s.plug, slot, interfaces.SecurityAppArmor)
+ c.Assert(err, IsNil)
+ c.Assert(string(snippet), testutil.Contains, "peer=(label=snap.bluez.*),")
+}
+
+// The label uses alternation when some, but not all, apps is bound to the bluez slot
+func (s *BluezInterfaceSuite) TestConnectedPlugSnippetUsesSlotLabelSome(c *C) {
+ app1 := &snap.AppInfo{Name: "app1"}
+ app2 := &snap.AppInfo{Name: "app2"}
+ app3 := &snap.AppInfo{Name: "app3"}
+ slot := &interfaces.Slot{
+ SlotInfo: &snap.SlotInfo{
+ Snap: &snap.Info{
+ SuggestedName: "bluez",
+ Apps: map[string]*snap.AppInfo{"app1": app1, "app2": app2, "app3": app3},
+ },
+ Name: "bluez",
+ Interface: "bluez",
+ Apps: map[string]*snap.AppInfo{"app1": app1, "app2": app2},
+ },
+ }
+ snippet, err := s.iface.ConnectedPlugSnippet(s.plug, slot, interfaces.SecurityAppArmor)
c.Assert(err, IsNil)
c.Assert(string(snippet), testutil.Contains, "peer=(label=snap.bluez.{app1,app2}),")
}
+// The label uses short form when exactly one app is bound to the bluez slot
+func (s *BluezInterfaceSuite) TestConnectedPlugSnippetUsesSlotLabelOne(c *C) {
+ app := &snap.AppInfo{Name: "app"}
+ slot := &interfaces.Slot{
+ SlotInfo: &snap.SlotInfo{
+ Snap: &snap.Info{
+ SuggestedName: "bluez",
+ Apps: map[string]*snap.AppInfo{"app": app},
+ },
+ Name: "bluez",
+ Interface: "bluez",
+ Apps: map[string]*snap.AppInfo{"app": app},
+ },
+ }
+ snippet, err := s.iface.ConnectedPlugSnippet(s.plug, slot, interfaces.SecurityAppArmor)
+ c.Assert(err, IsNil)
+ c.Assert(string(snippet), testutil.Contains, "peer=(label=snap.bluez.app),")
+}
+
func (s *BluezInterfaceSuite) TestUnusedSecuritySystems(c *C) {
systems := [...]interfaces.SecuritySystem{interfaces.SecurityAppArmor,
interfaces.SecuritySecComp, interfaces.SecurityDBus,