From dabea716cafa00b93bf3587e5860de1514b25b5b Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Mon, 9 Dec 2024 16:40:29 -0600 Subject: [PATCH] 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 --- overlord/configstate/configcore/prompting.go | 42 +------ overlord/ifacestate/export_test.go | 4 + overlord/ifacestate/ifacemgr.go | 91 +++++++++++++- overlord/ifacestate/ifacestate_test.go | 119 ++++++++++++++++++- 4 files changed, 212 insertions(+), 44 deletions(-) diff --git a/overlord/configstate/configcore/prompting.go b/overlord/configstate/configcore/prompting.go index 20b77b300df..6fff006fd83 100644 --- a/overlord/configstate/configcore/prompting.go +++ b/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 diff --git a/overlord/ifacestate/export_test.go b/overlord/ifacestate/export_test.go index 838800234a2..b840e7e73d0 100644 --- a/overlord/ifacestate/export_test.go +++ b/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 diff --git a/overlord/ifacestate/ifacemgr.go b/overlord/ifacestate/ifacemgr.go index 64b68aa9366..1679a783746 100644 --- a/overlord/ifacestate/ifacemgr.go +++ b/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() diff --git a/overlord/ifacestate/ifacestate_test.go b/overlord/ifacestate/ifacestate_test.go index c4abb9e66aa..82359bab1f9 100644 --- a/overlord/ifacestate/ifacestate_test.go +++ b/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