o/c/configcore,o/ifacestate: check for presence of handler-service on…

… startup

During snapd startup, if the "apparmor-prompting" flag is enabled but
there is no snap with an interfaces-requests-control connection and an
app app declared as a "handler-service", then do not start the prompting
backends, and record a warning that prompting will be inactive until
snapd is restarted.

This avoids a situation where prompting is enabled but there is no
client installed, resulting in snaps blocking while waiting for prompt
replies which will never be received. Now, if there's no client
installed, prompting will be inactive until a client is installed and
snapd is restarted.

There is a similar check for the presence of a handler-service client
when one attempts to enable the "apparmor-prompting" feature flag in the
first place. If there is no handler-service app present, or it cannot be
successfully started, then the flag is left disabled. Before this
commit, however, there are no subsequent checks for the presence of a
handler-service, so snapd can end up in the situation described above if
the user uninstalls the client snap without disabling the
"apparmor-prompting" flag.

We don't want killing the handler-service client to be sufficient to
disable prompting. Instead, prompting can only be simultaneously enabled
but inactive if the client snap either has disconnected the plug for the
interfaces-requests-control interface, or has been uninstalled entirely.
Both of these operations operations require elevated permissions, so
confined snaps which lack the snapd-control interface should be unable
to disable prompting.

Signed-off-by: Oliver Calder <[email protected]>

overlord/configstate/configcore/prompting.go

@@ -26,12 +26,10 @@ import (
 
 	"github.com/snapcore/snapd/client"
 	"github.com/snapcore/snapd/features"
-	"github.com/snapcore/snapd/interfaces"
 	"github.com/snapcore/snapd/overlord/configstate/config"
 	"github.com/snapcore/snapd/overlord/ifacestate"
 	"github.com/snapcore/snapd/overlord/restart"
 	"github.com/snapcore/snapd/overlord/servicestate"
-	"github.com/snapcore/snapd/overlord/snapstate"
 	"github.com/snapcore/snapd/overlord/state"
 	"github.com/snapcore/snapd/release"
 	"github.com/snapcore/snapd/snap"
@@ -88,45 +86,7 @@ func findPromptingRequestsHandlers(st *state.State) ([]*snap.AppInfo, error) {
 	st.Lock()
 	defer st.Unlock()
 
-	conns, err := ifacestate.ConnectionStates(st)
-	if err != nil {
-		return nil, fmt.Errorf("internal error: cannot get connections: %w", err)
-	}
-
-	var handlers []*snap.AppInfo
-
-	for connId, connState := range conns {
-		if connState.Interface != "snap-interfaces-requests-control" || !connState.Active() {
-			continue
-		}
-
-		connRef, err := interfaces.ParseConnRef(connId)
-		if err != nil {
-			return nil, err
-		}
-
-		handler, ok := connState.StaticPlugAttrs["handler-service"].(string)
-		if !ok {
-			// does not have a handler service
-			continue
-		}
-
-		sn := connRef.PlugRef.Snap
-		si, err := snapstate.CurrentInfo(st, sn)
-		if err != nil {
-			return nil, err
-		}
-
-		// this should not fail as plug's before prepare should have validated that such app exists
-		app := si.Apps[handler]
-		if app == nil {
-			return nil, fmt.Errorf("internal error: cannot find app %q in snap %q", app, sn)
-		}
-
-		handlers = append(handlers, app)
-	}
-
-	return handlers, nil
+	return ifacestate.InterfacesRequestsControlHandlerServices(st)
 }
 
 // Trigger a security profile regeneration by restarting snapd if the

overlord/ifacestate/export_test.go

@@ -140,6 +140,10 @@ func MockAssessAppArmorPrompting(new func(m *InterfaceManager) bool) (restore fu
 	return testutil.Mock(&assessAppArmorPrompting, new)
 }
 
+func MockInterfacesRequestsControlHandlerServicePresent(new func(m *InterfaceManager) (bool, error)) (restore func()) {
+	return testutil.Mock(&interfacesRequestsControlHandlerServicePresentImpl, new)
+}
+
 func MockUDevInitRetryTimeout(t time.Duration) (restore func()) {
 	old := udevInitRetryTimeout
 	udevInitRetryTimeout = t

overlord/ifacestate/ifacemgr.go

@@ -205,7 +205,23 @@ func (m *InterfaceManager) StartUp() error {
 	}
 
 	if m.useAppArmorPrompting {
+		// Check if there is at least one snap on the system which has a
+		// connection using the "snap-interfaces-requests-control" plug
+		// with a "handler-service" attribute declared.
+		present, err := m.interfacesRequestsControlHandlerServicePresent()
+		if err != nil {
+			// Internal error, should not occur
+			logger.Noticef("failed to check the presence of a interfaces-requests-control handler service: %v", err)
+			m.useAppArmorPrompting = false
+		} else if !present {
+			m.useAppArmorPrompting = false
+			m.state.AddWarning(`"apparmor-prompting" feature flag enabled but no prompting client is present; prompting will be inactive until a prompting client is installed and snapd is restarted`, nil)
+		}
+
 		func() {
+			if !m.useAppArmorPrompting {
+				return
+			}
 			// Must not hold state lock while starting interfaces requests
 			// manager, so that notices can be recorded if needed.
 			m.state.Unlock()
@@ -552,8 +568,79 @@ func (m *InterfaceManager) initUDevMonitor() error {
 	return nil
 }
 
-// initInterfacesRequestsManager should only be called if prompting is
-// supported and enabled.
+// interfacesRequestsControlHandlerServicePresent returns true if there is at
+// least one snap which has a "snap-interfaces-requests-control" connection
+// with an app declared by the "handler-service" attribute.
+//
+// The caller must ensure that the state lock is held.
+func (m *InterfaceManager) interfacesRequestsControlHandlerServicePresent() (bool, error) {
+	return interfacesRequestsControlHandlerServicePresentImpl(m)
+}
+
+var interfacesRequestsControlHandlerServicePresentImpl = func(m *InterfaceManager) (bool, error) {
+	handlers, err := InterfacesRequestsControlHandlerServices(m.state)
+	if err != nil {
+		return false, err
+	}
+	return len(handlers) > 0, nil
+}
+
+// InterfacesRequestsControlHandlerServices returns the list of all apps which
+// are defined as "handler-service" for a snap which has a connected plug for
+// the "snap-interfaces-requests-control" interface.
+//
+// The caller must ensure that the state lock is held.
+func InterfacesRequestsControlHandlerServices(st *state.State) ([]*snap.AppInfo, error) {
+	conns, err := ConnectionStates(st)
+	if err != nil {
+		return nil, fmt.Errorf("internal error: cannot get connections: %w", err)
+	}
+
+	var handlers []*snap.AppInfo
+
+	for connId, connState := range conns {
+		if connState.Interface != "snap-interfaces-requests-control" || !connState.Active() {
+			continue
+		}
+
+		connRef, err := interfaces.ParseConnRef(connId)
+		if err != nil {
+			return nil, err
+		}
+
+		handler, ok := connState.StaticPlugAttrs["handler-service"].(string)
+		if !ok {
+			// does not have a handler service
+			continue
+		}
+
+		sn := connRef.PlugRef.Snap
+		si, err := snapstate.CurrentInfo(st, sn)
+		if err != nil {
+			return nil, err
+		}
+
+		// this should not fail as plug's before prepare should have validated that such app exists
+		app := si.Apps[handler]
+		if app == nil {
+			return nil, fmt.Errorf("internal error: cannot find app %q in snap %q", app, sn)
+		}
+
+		handlers = append(handlers, app)
+	}
+
+	return handlers, nil
+}
+
+// initInterfacesRequestsManager initializes the prompting backends which make
+// up the interfaces requests manager.
+//
+// This function should only be called if prompting is supported and enabled,
+// and at least one installed snap has a "snap-interfaces-requests-control"
+// connection with the "handler-service" attribute declared.
+//
+// The state lock must not be held when this function is called, so that
+// notices can be recorded if necessary.
 func (m *InterfaceManager) initInterfacesRequestsManager() error {
 	m.interfacesRequestsManagerMu.Lock()
 	defer m.interfacesRequestsManagerMu.Unlock()

overlord/ifacestate/ifacestate_test.go

@@ -374,6 +374,12 @@ func (s *interfaceManagerSuite) TestSmokeAppArmorPromptingEnabled(c *C) {
 		return true
 	})
 	defer restore()
+	checkCount := 0
+	restore = ifacestate.MockInterfacesRequestsControlHandlerServicePresent(func(m *ifacestate.InterfaceManager) (bool, error) {
+		checkCount++
+		return true, nil
+	})
+	defer restore()
 	createCount := 0
 	fakeManager := &apparmorprompting.InterfacesRequestsManager{}
 	restore = ifacestate.MockCreateInterfacesRequestsManager(func(s *state.State) (*apparmorprompting.InterfacesRequestsManager, error) {
@@ -397,6 +403,7 @@ func (s *interfaceManagerSuite) TestSmokeAppArmorPromptingEnabled(c *C) {
 	defer restore()
 
 	mgr := s.manager(c)
+	c.Check(checkCount, Equals, 1)
 	c.Check(createCount, Equals, 1)
 	c.Check(mgr.AppArmorPromptingRunning(), Equals, true)
 	c.Check(mgr.InterfacesRequestsManager(), Equals, fakeManager)
@@ -411,11 +418,19 @@ func (s *interfaceManagerSuite) TestSmokeAppArmorPromptingDisabled(c *C) {
 		return false
 	})
 	defer restore()
+	checkCount := 0
+	restore = ifacestate.MockInterfacesRequestsControlHandlerServicePresent(func(m *ifacestate.InterfaceManager) (bool, error) {
+		c.Errorf("unexpectedly called m.interfacesRequestsControlHandlerServicePresent")
+		checkCount++
+		return true, nil
+	})
+	defer restore()
 	createCount := 0
 	fakeManager := &apparmorprompting.InterfacesRequestsManager{}
 	restore = ifacestate.MockCreateInterfacesRequestsManager(func(s *state.State) (*apparmorprompting.InterfacesRequestsManager, error) {
+		c.Errorf("unexpectedly called m.initInterfacesRequestsManager")
 		createCount++
-		return fakeManager, fmt.Errorf("should not have been called")
+		return fakeManager, nil
 	})
 	defer restore()
 	stopCount := 0
@@ -426,6 +441,7 @@ func (s *interfaceManagerSuite) TestSmokeAppArmorPromptingDisabled(c *C) {
 	defer restore()
 
 	mgr := s.manager(c)
+	c.Check(checkCount, Equals, 0)
 	c.Check(createCount, Equals, 0)
 	c.Check(mgr.AppArmorPromptingRunning(), Equals, false)
 	c.Check(mgr.InterfacesRequestsManager(), testutil.IsInterfaceNil)
@@ -6985,12 +7001,109 @@ func (s *interfaceManagerSuite) TestConnectHandlesAutoconnect(c *C) {
 	})
 }
 
+func (s *interfaceManagerSuite) TestInterfacesRequestsManagerNoHandlerService(c *C) {
+	restore := ifacestate.MockAssessAppArmorPrompting(func(m *ifacestate.InterfaceManager) bool {
+		return true
+	})
+	defer restore()
+
+	checkCount := 0
+	restore = ifacestate.MockInterfacesRequestsControlHandlerServicePresent(func(m *ifacestate.InterfaceManager) (bool, error) {
+		checkCount++
+		return false, nil
+	})
+	defer restore()
+
+	createCount := 0
+	fakeManager := &apparmorprompting.InterfacesRequestsManager{}
+	restore = ifacestate.MockCreateInterfacesRequestsManager(func(s *state.State) (*apparmorprompting.InterfacesRequestsManager, error) {
+		c.Errorf("unexpectedly called m.initInterfacesRequestsManager")
+		createCount++
+		return fakeManager, nil
+	})
+	defer restore()
+
+	mgr, err := ifacestate.Manager(s.state, nil, s.o.TaskRunner(), nil, nil)
+	c.Assert(err, IsNil)
+
+	logbuf, restore := logger.MockLogger()
+	defer restore()
+
+	err = mgr.StartUp()
+	c.Check(err, IsNil)
+
+	// Check that error caused AppArmorPromptingRunning() to now return false
+	running := mgr.AppArmorPromptingRunning()
+	c.Check(running, Equals, false)
+
+	c.Check(checkCount, Equals, 1)
+	c.Check(createCount, Equals, 0)
+
+	logger.WithLoggerLock(func() {
+		logStr := logbuf.String()
+		c.Check(logStr, Not(testutil.Contains), "failed to check the presence of a interfaces-requests-control handler service")
+		c.Check(logStr, Not(testutil.Contains), "failed to start interfaces requests manager")
+	})
+
+	s.state.Lock()
+	defer s.state.Unlock()
+	warns := s.state.AllWarnings()
+	c.Check(warns, HasLen, 1)
+	c.Check(warns[0].String(), Matches, `"apparmor-prompting" feature flag enabled but no prompting client is present; prompting will be inactive until a prompting client is installed and snapd is restarted`)
+}
+
+func (s *interfaceManagerSuite) TestInterfacesRequestsManagerHandlerServicePresentError(c *C) {
+	restore := ifacestate.MockAssessAppArmorPrompting(func(m *ifacestate.InterfaceManager) bool {
+		return true
+	})
+	defer restore()
+
+	checkError := fmt.Errorf("custom error")
+	restore = ifacestate.MockInterfacesRequestsControlHandlerServicePresent(func(m *ifacestate.InterfaceManager) (bool, error) {
+		return false, checkError
+	})
+	defer restore()
+
+	createCount := 0
+	fakeManager := &apparmorprompting.InterfacesRequestsManager{}
+	restore = ifacestate.MockCreateInterfacesRequestsManager(func(s *state.State) (*apparmorprompting.InterfacesRequestsManager, error) {
+		c.Errorf("unexpectedly called m.initInterfacesRequestsManager")
+		createCount++
+		return fakeManager, nil
+	})
+	defer restore()
+
+	mgr, err := ifacestate.Manager(s.state, nil, s.o.TaskRunner(), nil, nil)
+	c.Assert(err, IsNil)
+
+	logbuf, restore := logger.MockLogger()
+	defer restore()
+
+	err = mgr.StartUp()
+	c.Check(err, IsNil)
+
+	// Check that error caused AppArmorPromptingRunning() to now return false
+	running := mgr.AppArmorPromptingRunning()
+	c.Check(running, Equals, false)
+
+	c.Check(createCount, Equals, 0)
+
+	logger.WithLoggerLock(func() {
+		c.Check(logbuf.String(), testutil.Contains, "failed to check the presence of a interfaces-requests-control handler service")
+	})
+}
+
 func (s *interfaceManagerSuite) TestInitInterfacesRequestsManagerError(c *C) {
 	restore := ifacestate.MockAssessAppArmorPrompting(func(m *ifacestate.InterfaceManager) bool {
 		return true
 	})
 	defer restore()
 
+	restore = ifacestate.MockInterfacesRequestsControlHandlerServicePresent(func(m *ifacestate.InterfaceManager) (bool, error) {
+		return true, nil
+	})
+	defer restore()
+
 	createError := fmt.Errorf("custom error")
 	restore = ifacestate.MockCreateInterfacesRequestsManager(func(s *state.State) (*apparmorprompting.InterfacesRequestsManager, error) {
 		return nil, createError
@@ -7020,6 +7133,10 @@ func (s *interfaceManagerSuite) TestStopInterfacesRequestsManagerError(c *C) {
 		return true
 	})
 	defer restore()
+	restore = ifacestate.MockInterfacesRequestsControlHandlerServicePresent(func(m *ifacestate.InterfaceManager) (bool, error) {
+		return true, nil
+	})
+	defer restore()
 	fakeManager := &apparmorprompting.InterfacesRequestsManager{}
 	restore = ifacestate.MockCreateInterfacesRequestsManager(func(s *state.State) (*apparmorprompting.InterfacesRequestsManager, error) {
 		return fakeManager, nil