Skip to content
2 changes: 1 addition & 1 deletion daemon/api_prompting.go
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ func postInterfacesRequests(c *Command, r *http.Request, user *auth.UserState) R
return errorResp
}

outcome, err := getInterfaceManager(c).InterfacesRequestsManager().Ask(reqUID, postBody.Interface, snapName, postBody.PID, cgroupPath, c.d.tomb.Dying())
outcome, err := getInterfaceManager(c).InterfacesRequestsManager().Ask(reqUID, postBody.Interface, snapName, postBody.PID, cgroupPath)
if err != nil {
return promptingError(err)
}
Expand Down
5 changes: 1 addition & 4 deletions daemon/api_prompting_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ type fakeInterfacesRequestsManager struct {
iface string
pid int32
cgroup string
snapdShuttingDown <-chan struct{}
id prompting.IDType // used for prompt ID or rule ID
ruleConstraintsJSON prompting.ConstraintsJSON
constraintsPatchJSON prompting.ConstraintsJSON
Expand All @@ -69,13 +68,12 @@ type fakeInterfacesRequestsManager struct {
clientActivity bool
}

func (m *fakeInterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int32, cgroup string, snapdShuttingDown <-chan struct{}) (prompting.OutcomeType, error) {
func (m *fakeInterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int32, cgroup string) (prompting.OutcomeType, error) {
m.userID = uid
m.iface = iface
m.snap = snap
m.pid = pid
m.cgroup = cgroup
m.snapdShuttingDown = snapdShuttingDown
return m.ask, m.err
}

Expand Down Expand Up @@ -733,7 +731,6 @@ func (s *promptingSuite) TestPostInterfacesRequestsHappy(c *C) {
c.Check(s.manager.snap, Equals, expectedSnap)
c.Check(s.manager.pid, Equals, fakePID)
c.Check(s.manager.cgroup, Equals, fakeCgroup)
c.Check(s.manager.snapdShuttingDown, NotNil)

// Check return value
responseBody, ok := rsp.Result.(daemon.PostInterfacesRequestsResponse)
Expand Down
31 changes: 23 additions & 8 deletions overlord/ifacestate/apparmorprompting/prompting.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ type listenerBackend interface {
// A Manager holds outstanding prompts and mediates their replies, further it
// stores and applies persistent rules.
type Manager interface {
Ask(uid uint32, iface, snap string, pid int32, cgroup string, snapdShuttingDown <-chan struct{}) (prompting.OutcomeType, error)
Ask(uid uint32, iface, snap string, pid int32, cgroup string) (prompting.OutcomeType, error)
Prompts(userID uint32, clientActivity bool) ([]*requestprompts.Prompt, error)
PromptWithID(userID uint32, promptID prompting.IDType, clientActivity bool) (*requestprompts.Prompt, error)
HandleReply(userID uint32, promptID prompting.IDType, replyConstraintsJSON prompting.ConstraintsJSON, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string, clientActivity bool) ([]prompting.IDType, error)
Expand Down Expand Up @@ -85,6 +85,13 @@ type InterfacesRequestsManager struct {
// listener readiness.
listenerAlreadySignalled chan struct{}

// snapdShuttingDown is closed when overlord.ShutDown is called to
// signal that the daemon is going to be stopped and the
// InterfacesRequestsManager needs to stop receiving requests and
// finish handling existing requests.
snapdShuttingDown chan struct{}
shutdown bool

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
shutdown bool
shutdownOnce sync.Once


askRequests chan *prompting.Request
}

Expand Down Expand Up @@ -140,6 +147,7 @@ func New(noticeMgr *notices.NoticeManager) (m *InterfacesRequestsManager, retErr
prompts: promptsBackend,
rules: rulesBackend,
listenerAlreadySignalled: make(chan struct{}),
snapdShuttingDown: make(chan struct{}),
askRequests: make(chan *prompting.Request),
}

Expand Down Expand Up @@ -328,21 +336,18 @@ func (m *InterfacesRequestsManager) disconnect() error {
//
// If the given channel closes or the prompting subsystem is shutting down,
// then returns [prompting_errors.ErrPromptingClosed].
// TODO: replace this ad-hoc channel with a proper shutdown handler in the
// manager itself, so this method will observe the shutdown from within the
// manager and act accordingly.
//
// The given interface must be one for which we expect requests to be created
// directly, rather than via AppArmor. The requested permissions will include
// all available permissions for the given interface.
func (m *InterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int32, cgroup string, snapdShuttingDown <-chan struct{}) (prompting.OutcomeType, error) {
func (m *InterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int32, cgroup string) (prompting.OutcomeType, error) {
replyChan := make(chan []string)

reply := func(allowedPerms []string) error {
select {
case replyChan <- allowedPerms:
return nil
case <-snapdShuttingDown:
case <-m.snapdShuttingDown:
return prompting_errors.ErrPromptingClosed
case <-m.tomb.Dying():
return prompting_errors.ErrPromptingClosed
Expand All @@ -360,7 +365,7 @@ func (m *InterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int3
select {
case m.askRequests <- req:
// request received and being processed
case <-snapdShuttingDown:
case <-m.snapdShuttingDown:
return prompting.OutcomeUnset, prompting_errors.ErrPromptingClosed
case <-m.tomb.Dying():
return prompting.OutcomeUnset, prompting_errors.ErrPromptingClosed
Expand All @@ -371,7 +376,7 @@ func (m *InterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int3
select {
case allowedPermissions = <-replyChan:
// received reply
case <-snapdShuttingDown:
case <-m.snapdShuttingDown:
return prompting.OutcomeUnset, prompting_errors.ErrPromptingClosed
case <-m.tomb.Dying():
return prompting.OutcomeUnset, prompting_errors.ErrPromptingClosed
Expand All @@ -386,6 +391,16 @@ func (m *InterfacesRequestsManager) Ask(uid uint32, iface, snap string, pid int3
return prompting.OutcomeAllow, nil
}

// ShutDown stops the listener, prompt DB, and rule DB from receiving new
// requests.
func (m *InterfacesRequestsManager) ShutDown() {
if m.shutdown {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems we might need a lock around this flag? cc @andrewphelpsj

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don't think we need a dedicated bool to mark whether this has already been done, this would be equivalent:

select {
case <-m.snapdShuttingDown:
	return
default:
	// we're about to shut down for the first time
}

But either way Samuele's right and we need some sort of synchronization. I think a sync.Once may be the most semantically clear approach, something like:

m.shutdownOnce.Do(func() {
	close(m.snapdShuttingDown)
})

then there's no need for a check at all, we can just unconditionally do this and the sync.Once guarantees the function is only invoked once. Atomics under the hood I believe.

return
}
close(m.snapdShuttingDown)
m.shutdown = true
}

// Stop closes the listener, prompt DB, and rule DB. Stop is idempotent, and
// the receiver cannot be started or used after it has been stopped.
func (m *InterfacesRequestsManager) Stop() error {
Expand Down
39 changes: 11 additions & 28 deletions overlord/ifacestate/apparmorprompting/prompting_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -652,8 +652,7 @@ func (s *apparmorpromptingSuite) testAskWithOutcome(c *C, outcome prompting.Outc
outcomeChan := make(chan prompting.OutcomeType)
errChan := make(chan error)
go func() {
snapdShuttingDown := make(chan struct{})
out, err := mgr.Ask(uid, iface, snap, pid, cgroup, snapdShuttingDown)
out, err := mgr.Ask(uid, iface, snap, pid, cgroup)
logger.WithLoggerLock(func() {
c.Check(err, IsNil, Commentf(logbuf.String()))
})
Expand Down Expand Up @@ -742,20 +741,10 @@ func (s *apparmorpromptingSuite) TestAskShutdownBeforeSending(c *C) {
)

// Stop the manager now so that it will not receive the request.
//
// Unfortunately, there's not a way to test the snapdShuttingDown channel
// closing as well, since if the manager has not stopped, there is a race
// where the run loop may receive the request. So we close the listener to
// ensure the run loop does not receive the request.
//
// XXX: in the future, when we remove the snapdShuttingDown channel in
// favor of a manager-level shutdown triggered by the daemon stopping, most
// of this comment can be removed.
mgr.ShutDown()
c.Check(mgr.Stop(), IsNil)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need to call mgr.Stop() here anymore, we can do it at the end of the test now instead, like the other tests. That's what the comment was talking about iirc.

We can change this now after adding another case to the run loop which breaks out if m.snapdShuttingDown closes.


timeoutChan := make(chan struct{})
time.AfterFunc(time.Second, func() { close(timeoutChan) })
outcome, err := mgr.Ask(uid, iface, snap, pid, cgroup, timeoutChan)
outcome, err := mgr.Ask(uid, iface, snap, pid, cgroup)
c.Check(outcome, Equals, prompting.OutcomeUnset)
c.Check(err, Equals, prompting_errors.ErrPromptingClosed)
}
Expand Down Expand Up @@ -785,12 +774,10 @@ func (s *apparmorpromptingSuite) TestAskShutdownBeforeReply(c *C) {
mgr, err := apparmorprompting.New(s.noticeMgr)
c.Assert(err, IsNil)

neverClose := make(chan struct{})

// Call Ask, then signal when response has been validated
doneChan := make(chan struct{})
go func() {
outcome, err := mgr.Ask(uid, iface, snap, pid, cgroup, neverClose)
outcome, err := mgr.Ask(uid, iface, snap, pid, cgroup)
c.Check(outcome, Equals, prompting.OutcomeUnset)
c.Check(err, Equals, prompting_errors.ErrPromptingClosed)
close(doneChan)
Expand Down Expand Up @@ -890,12 +877,10 @@ func (s *apparmorpromptingSuite) TestAskShutdownViaChannelBeforeReply(c *C) {
mgr, err := apparmorprompting.New(s.noticeMgr)
c.Assert(err, IsNil)

snapdShuttingDown := make(chan struct{})

// Call Ask, then signal when response has been validated
doneChan := make(chan struct{})
go func() {
outcome, err := mgr.Ask(uid, iface, snap, pid, cgroup, snapdShuttingDown)
outcome, err := mgr.Ask(uid, iface, snap, pid, cgroup)
c.Check(outcome, Equals, prompting.OutcomeUnset)
c.Check(err, Equals, prompting_errors.ErrPromptingClosed)
close(doneChan)
Expand All @@ -909,8 +894,8 @@ func (s *apparmorpromptingSuite) TestAskShutdownViaChannelBeforeReply(c *C) {
c.Errorf("manager failed to become ready after receiving request")
}

// Now Ask should be waiting for a reply. Close snapdShuttingDown instead.
close(snapdShuttingDown)
// Now Ask should be waiting for a reply. Shutdown the manager instead.
mgr.ShutDown()

select {
case <-doneChan:
Expand Down Expand Up @@ -1873,8 +1858,7 @@ func (s *apparmorpromptingSuite) TestListenerReadyCausesPromptsHandleReadyingIfO
// Ask for other request in the background so we can see and respond to the prompt
whenSent := time.Now()
go func() {
snapdShuttingDown := make(chan struct{})
mgr.Ask(1000, "audio-record", "firefox", 1234, "some-cgroup", snapdShuttingDown)
mgr.Ask(1000, "audio-record", "firefox", 1234, "some-cgroup")
}()
// Wait for a notice
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
Expand Down Expand Up @@ -1976,11 +1960,10 @@ func (s *apparmorpromptingSuite) TestListenerReadyNotCausesPromptsHandleReadying

// Now add remaining API requests via Ask()

shutDownChan := make(chan struct{})
outcomeChan := make(chan prompting.OutcomeType)
errChan := make(chan error)
go func() {
outcome, err := mgr.Ask(1000, "audio-record", "obs-studio", 12345, "/cgroup-path/snap.obs-studio.obs-studio-someuuid.scope", shutDownChan)
outcome, err := mgr.Ask(1000, "audio-record", "obs-studio", 12345, "/cgroup-path/snap.obs-studio.obs-studio-someuuid.scope")
outcomeChan <- outcome
errChan <- err
}()
Expand All @@ -1994,7 +1977,7 @@ func (s *apparmorpromptingSuite) TestListenerReadyNotCausesPromptsHandleReadying
}

go func() {
outcome, err := mgr.Ask(1000, "audio-record", "signal-desktop", 67890, "/cgroup-path/snap.signal-desktop.signal-desktop.someuuid.scope", shutDownChan)
outcome, err := mgr.Ask(1000, "audio-record", "signal-desktop", 67890, "/cgroup-path/snap.signal-desktop.signal-desktop.someuuid.scope")
outcomeChan <- outcome
errChan <- err
}()
Expand All @@ -2018,7 +2001,7 @@ func (s *apparmorpromptingSuite) TestListenerReadyNotCausesPromptsHandleReadying
}

// Signal that snapd is shutting down and Ask calls should return
close(shutDownChan)
mgr.ShutDown()
for i := 0; i < 2; i++ {
select {
case outcome := <-outcomeChan:
Expand Down
Loading