interfaces: move lxd-support's use of AppArmor unconfined mode to an …

…interface attribute (#13514) * interfaces/apparmor: rework unconfined mode for dynamic enablement For an interface to use unconfined mode, it must both declare support for it as a static property and then enable it by an explicit call. This allows interfaces to enable this dynamically via an plug/slot attribute or similar as needed (or if not, it can be enabled the permanent plug/slot callback for the interface instead). Signed-off-by: Alex Murray <[email protected]> * interfaces/builtin/lxd-support: add an attr for unconfined mode The use of AppArmor unconfined mode requires a small amount of support in lxd itself, and so to ensure that we only use this when the lxd snap supports it, add a new interface attribute which the snap can set to specify that it has the required support and only enable this in the interface when that is present. Signed-off-by: Alex Murray <[email protected]> * interfaces/apparmor: check for error in unconfined mode unit test Signed-off-by: Alex Murray <[email protected]> * interfaces/apparmor: fixup comments documenting UnconfinedMode etc Signed-off-by: Alex Murray <[email protected]> * interfaces/builtin: test validation of attr in lxd-support Add a new unit test to check that the lxd-support interface validates the enable-unconfined-mode attribute as a boolean correctly. Signed-off-by: Alex Murray <[email protected]> * interfaces/builtin/lxd_support: clarify behaviour of unconfined mode Add some comments to clarify the behaviour and use of unconfined mode. Signed-off-by: Alex Murray <[email protected]> * interfaces/builtin/lxd_support: test unconfined mode enablement Signed-off-by: Alex Murray <[email protected]> * interfaces/builtin/lxd_support: explicitly ignore error Signed-off-by: Alex Murray <[email protected]> * interfaces/builtin/lxd_support: fixup gofmt in test Signed-off-by: Alex Murray <[email protected]> --------- Signed-off-by: Alex Murray <[email protected]>
canonical · Feb 16, 2024 · 1ebfd19 · 1ebfd19
1 parent 5d796f3
commit 1ebfd19
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 12 deletions.
diff --git a/interfaces/apparmor/backend.go b/interfaces/apparmor/backend.go
@@ -846,7 +846,7 @@ func (b *Backend) addContent(securityTag string, snapInfo *snap.Info, cmdName st
 		case "###FLAGS###":
 			// default flags
 			flags := []string{"attach_disconnected", "mediate_deleted"}
-			if spec.Unconfined() {
+			if spec.Unconfined() == UnconfinedEnabled {
 				// need both parser and kernel support for unconfined
 				pfeatures, _ := parserFeatures()
 				kfeatures, _ := kernelFeatures()

diff --git a/interfaces/apparmor/backend_test.go b/interfaces/apparmor/backend_test.go
@@ -1097,6 +1097,12 @@ func (s *backendSuite) TestUnconfinedFlag(c *C) {
 		"}\n")
 	defer restoreClassicTemplate()
 	s.Iface.InterfaceStaticInfo.AppArmorUnconfinedSlots = true
+	// will only be enabled if the interface also enables unconfined
+	s.Iface.AppArmorPermanentSlotCallback = func(spec *apparmor.Specification, slot *snap.SlotInfo) error {
+		err := spec.SetUnconfinedEnabled()
+		c.Assert(err, IsNil)
+		return nil
+	}
 	// test both classic and non-classic confinement
 	options := []interfaces.ConfinementOptions{
 		{},

diff --git a/interfaces/apparmor/spec.go b/interfaces/apparmor/spec.go
@@ -31,6 +31,17 @@ import (
 	"github.com/snapcore/snapd/strutil"
 )
 
+// UnconfinedMode describes the states of support for the AppArmor unconfined
+// profile mode - this is only enabled when the interface supports it as a
+// static property and it is then enabled via SetUnconfinedEnabled() method
+type UnconfinedMode int
+
+const (
+	UnconfinedIgnored UnconfinedMode = iota
+	UnconfinedSupported
+	UnconfinedEnabled
+)
+
 // Specification assists in collecting apparmor entries associated with an interface.
 type Specification struct {
 	// scope for various Add{...}Snippet functions
@@ -94,9 +105,8 @@ type Specification struct {
 	suppressPycacheDeny bool
 
 	// Unconfined profile mode allows a profile to be applied without any
-	// real confinement TODO: instead of a boolean, should this instead be a
-	// set of flags which get applied to the profile?
-	unconfined bool
+	// real confinement
+	unconfined UnconfinedMode
 }
 
 // setScope sets the scope of subsequent AddSnippet family functions.
@@ -662,7 +672,7 @@ func (spec *Specification) AddConnectedSlot(iface interfaces.Interface, plug *in
 func (spec *Specification) AddPermanentPlug(iface interfaces.Interface, plug *snap.PlugInfo) error {
 	si := interfaces.StaticInfoOf(iface)
 	if si.AppArmorUnconfinedPlugs {
-		spec.setUnconfined()
+		spec.setUnconfinedSupported()
 	}
 	type definer interface {
 		AppArmorPermanentPlug(spec *Specification, plug *snap.PlugInfo) error
@@ -679,7 +689,7 @@ func (spec *Specification) AddPermanentPlug(iface interfaces.Interface, plug *sn
 func (spec *Specification) AddPermanentSlot(iface interfaces.Interface, slot *snap.SlotInfo) error {
 	si := interfaces.StaticInfoOf(iface)
 	if si.AppArmorUnconfinedSlots {
-		spec.setUnconfined()
+		spec.setUnconfinedSupported()
 	}
 	type definer interface {
 		AppArmorPermanentSlot(spec *Specification, slot *snap.SlotInfo) error
@@ -762,14 +772,26 @@ func (spec *Specification) SuppressPycacheDeny() bool {
 	return spec.suppressPycacheDeny
 }
 
-// setUnconfined records whether a profile should be applied without any real
-// confinement
-func (spec *Specification) setUnconfined() {
-	spec.unconfined = true
+// setUnconfinedSuported records whether a profile perhaps should be applied
+// without any real confinement - this will only occur if the spec also enables
+// this by calling SetEnableUnconfined()
+func (spec *Specification) setUnconfinedSupported() {
+	spec.unconfined = UnconfinedSupported
+}
+
+// SetUnconfinedEnabled records whether a profile should be applied without any
+// real confinement - the spec must already support unconfined profiles via a
+// previous call to setUnconfinedSupported()
+func (spec *Specification) SetUnconfinedEnabled() error {
+	if spec.unconfined != UnconfinedSupported {
+		return fmt.Errorf("unconfined profiles not supported")
+	}
+	spec.unconfined = UnconfinedEnabled
+	return nil
 }
 
 // Unconfined returns whether a profile should be applied without any real
 // confinement
-func (spec *Specification) Unconfined() bool {
+func (spec *Specification) Unconfined() UnconfinedMode {
 	return spec.unconfined
 }
diff --git a/interfaces/builtin/lxd_support.go b/interfaces/builtin/lxd_support.go
@@ -20,10 +20,13 @@
 package builtin
 
 import (
+	"fmt"
+
 	"github.com/snapcore/snapd/interfaces"
 	"github.com/snapcore/snapd/interfaces/apparmor"
 	"github.com/snapcore/snapd/interfaces/seccomp"
 	apparmor_sandbox "github.com/snapcore/snapd/sandbox/apparmor"
+	"github.com/snapcore/snapd/snap"
 	"github.com/snapcore/snapd/strutil"
 )
 
@@ -75,6 +78,18 @@ func (iface *lxdSupportInterface) Name() string {
 	return "lxd-support"
 }
 
+func (iface *lxdSupportInterface) BeforePreparePlug(plug *snap.PlugInfo) error {
+	// It's fine if enable-unconfined-mode isn't specified, but if it is,
+	// it needs to be bool
+	if v, ok := plug.Attrs["enable-unconfined-mode"]; ok {
+		if _, ok = v.(bool); !ok {
+			return fmt.Errorf("lxd-support plug requires bool with 'enable-unconfined-mode'")
+		}
+	}
+
+	return nil
+}
+
 func (iface *lxdSupportInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {
 	spec.AddSnippet(lxdSupportConnectedPlugAppArmor)
 	// if apparmor supports userns mediation then add this too
@@ -87,6 +102,15 @@ func (iface *lxdSupportInterface) AppArmorConnectedPlug(spec *apparmor.Specifica
 			spec.AddSnippet(lxdSupportConnectedPlugAppArmorWithUserNS)
 		}
 	}
+	var enableUnconfinedMode bool
+	// enable-unconfined-mode was validated in BeforePreparePlug()
+	_ = plug.Attr("enable-unconfined-mode", &enableUnconfinedMode)
+	if enableUnconfinedMode {
+		// since we set appArmorUnconfinedPlugs to true in the static
+		// info below, we know that the spec will already support the
+		// unconfined mode and so this call will not fail
+		_ = spec.SetUnconfinedEnabled()
+	}
 	return nil
 }
 

diff --git a/interfaces/builtin/lxd_support_test.go b/interfaces/builtin/lxd_support_test.go
@@ -74,6 +74,24 @@ func (s *LxdSupportInterfaceSuite) TestSanitizePlug(c *C) {
 	c.Assert(interfaces.BeforePreparePlug(s.iface, s.plugInfo), IsNil)
 }
 
+func (s *LxdSupportInterfaceSuite) TestSanitizePlugInvalid(c *C) {
+	const lxdSupportInvalidConsumerYaml = `name: consumer
+version: 0
+plugs:
+  lxd-support-invalid-attr:
+    interface: lxd-support
+    enable-unconfined-mode: 1
+
+apps:
+ app:
+  plugs:
+    - lxd-support-invalid-attr
+`
+
+	_, plugInfo := MockConnectedPlug(c, lxdSupportInvalidConsumerYaml, nil, "lxd-support-invalid-attr")
+	c.Assert(interfaces.BeforePreparePlug(s.iface, plugInfo), ErrorMatches, "lxd-support plug requires bool with 'enable-unconfined-mode'")
+}
+
 func (s *LxdSupportInterfaceSuite) TestAppArmorSpec(c *C) {
 	spec := &apparmor.Specification{}
 	c.Assert(spec.AddConnectedPlug(s.iface, s.plug, s.slot), IsNil)
@@ -95,7 +113,25 @@ func (s *LxdSupportInterfaceSuite) TestAppArmorSpecUserNS(c *C) {
 func (s *LxdSupportInterfaceSuite) TestAppArmorSpecUnconfined(c *C) {
 	spec := &apparmor.Specification{}
 	c.Assert(spec.AddPermanentPlug(s.iface, s.plugInfo), IsNil)
-	c.Assert(spec.Unconfined(), Equals, true)
+	c.Assert(spec.Unconfined(), Equals, apparmor.UnconfinedSupported)
+
+	// Unconfined mode is enabled by the plug when it enables it via the
+	// enable-unconfined-mode attribute
+	const lxdSupportWithUnconfinedModeConsumerYaml = `name: consumer
+version: 0
+plugs:
+  lxd-support-with-unconfined-mode:
+    interface: lxd-support
+    enable-unconfined-mode: true
+apps:
+ app:
+  plugs: [lxd-support-with-unconfined-mode]
+`
+
+	plug, _ := MockConnectedPlug(c, lxdSupportWithUnconfinedModeConsumerYaml, nil, "lxd-support-with-unconfined-mode")
+
+	c.Assert(spec.AddConnectedPlug(s.iface, plug, s.slot), IsNil)
+	c.Assert(spec.Unconfined(), Equals, apparmor.UnconfinedEnabled)
 }
 
 func (s *LxdSupportInterfaceSuite) TestSecCompSpec(c *C) {