i/p/requestrules,o/i/apparmorprompting: move request matching logic into requestrules

interfaces/prompting/requestrules/export_test.go

@@ -21,6 +21,8 @@ package requestrules
 
 import (
 	"time"
+
+	"github.com/snapcore/snapd/testutil"
 )
 
 var JoinInternalErrors = joinInternalErrors
@@ -30,3 +32,11 @@ type RulesDBJSON rulesDBJSON
 func (rule *Rule) Validate(currTime time.Time) (expired bool, err error) {
 	return rule.validate(currTime)
 }
+
+func (rdb *RuleDB) IsPathPermAllowed(user uint32, snap string, iface string, path string, permission string) (bool, error) {
+	return rdb.isPathPermAllowed(user, snap, iface, path, permission)
+}
+
+func MockIsPathPermAllowed(f func(rdb *RuleDB, user uint32, snap string, iface string, path string, permission string) (bool, error)) func() {
+	return testutil.Mock(&isPathPermAllowedByRuleDB, f)
+}

interfaces/prompting/requestrules/requestrules.go

@@ -735,10 +735,43 @@
 	return &newRule, nil
 }
 
-// IsPathAllowed checks whether the given path with the given permission is
+// IsRequestAllowed checks whether a request with the given parameters is
+// allowed or denied by existing rules.
+//
+// If any of the given permissions are allowed, they are returned as
+// allowedPerms. If any permissions are denied, then returns anyDenied as true.
+// If any of the given permissions were not matched by an existing rule, then
+// they are returned as outstandingPerms. If an error occurred, returns it.
+func (rdb *RuleDB) IsRequestAllowed(user uint32, snap string, iface string, path string, permissions []string) (allowedPerms []string, anyDenied bool, outstandingPerms []string, err error) {
+	allowedPerms = make([]string, 0, len(permissions))
+	outstandingPerms = make([]string, 0, len(permissions))
+	var errs []error
+	for _, perm := range permissions {
+		allowed, err := isPathPermAllowedByRuleDB(rdb, user, snap, iface, path, perm)
+		switch {
+		case err == nil:
+			if allowed {
+				allowedPerms = append(allowedPerms, perm)
+			} else {
+				anyDenied = true
+			}
+		case errors.Is(err, prompting_errors.ErrNoMatchingRule):
+			outstandingPerms = append(outstandingPerms, perm)
+		default:
+			errs = append(errs, err)
+		}
+	}
+	return allowedPerms, anyDenied, outstandingPerms, strutil.JoinErrors(errs...)
+}
+
+var isPathPermAllowedByRuleDB = func(rdb *RuleDB, user uint32, snap string, iface string, path string, permission string) (bool, error) {
+	return rdb.isPathPermAllowed(user, snap, iface, path, permission)
+}
+
+// isPathPermAllowed checks whether the given path with the given permission is
 // allowed or denied by existing rules for the given user, snap, and interface.
 // If no rule applies, returns prompting_errors.ErrNoMatchingRule.
-func (rdb *RuleDB) IsPathAllowed(user uint32, snap string, iface string, path string, permission string) (bool, error) {
+func (rdb *RuleDB) isPathPermAllowed(user uint32, snap string, iface string, path string, permission string) (bool, error) {
 	rdb.mutex.RLock()
 	defer rdb.mutex.RUnlock()
 	permissionMap, ok := rdb.permissionDBForUserSnapInterfacePermission(user, snap, iface, permission)

interfaces/prompting/requestrules/requestrules_test.go

@@ -968,7 +968,141 @@ func (s *requestrulesSuite) TestAddRulePartiallyExpired(c *C) {
 	c.Check(exists, Equals, true)
 }
 
-func (s *requestrulesSuite) TestIsPathAllowedSimple(c *C) {
+type isPathPermAllowedResult struct {
+	allowed bool
+	err     error
+}
+
+func (s *requestrulesSuite) TestIsRequestAllowed(c *C) {
+	rdb, err := requestrules.New(nil)
+	c.Assert(err, IsNil)
+	c.Assert(rdb, NotNil)
+
+	user := uint32(1234)
+	snap := "firefox"
+	iface := "powerful"
+	path := "/path/to/something"
+
+	for _, testCase := range []struct {
+		requestedPerms   []string
+		permReturns      map[string]isPathPermAllowedResult
+		allowedPerms     []string
+		anyDenied        bool
+		outstandingPerms []string
+		errStr           string
+	}{
+		{
+			requestedPerms: []string{"foo", "bar", "baz"},
+			permReturns: map[string]isPathPermAllowedResult{
+				"foo": {true, nil},
+				"bar": {true, nil},
+				"baz": {true, nil},
+			},
+			allowedPerms:     []string{"foo", "bar", "baz"},
+			anyDenied:        false,
+			outstandingPerms: []string{},
+			errStr:           "",
+		},
+		{
+			requestedPerms: []string{"foo", "bar", "baz"},
+			permReturns: map[string]isPathPermAllowedResult{
+				"foo": {true, prompting_errors.ErrNoMatchingRule},
+				"bar": {false, prompting_errors.ErrNoMatchingRule},
+				"baz": {true, prompting_errors.ErrNoMatchingRule},
+			},
+			allowedPerms:     []string{},
+			anyDenied:        false,
+			outstandingPerms: []string{"foo", "bar", "baz"},
+			errStr:           "",
+		},
+		{
+			requestedPerms: []string{"foo", "bar", "baz"},
+			permReturns: map[string]isPathPermAllowedResult{
+				"foo": {true, prompting_errors.ErrNoMatchingRule},
+				"bar": {true, nil},
+				"baz": {false, prompting_errors.ErrNoMatchingRule},
+			},
+			allowedPerms:     []string{"bar"},
+			anyDenied:        false,
+			outstandingPerms: []string{"foo", "baz"},
+			errStr:           "",
+		},
+		{
+			requestedPerms: []string{"foo", "bar", "baz"},
+			permReturns: map[string]isPathPermAllowedResult{
+				"foo": {false, nil},
+				"bar": {true, nil},
+				"baz": {false, prompting_errors.ErrNoMatchingRule},
+			},
+			allowedPerms:     []string{"bar"},
+			anyDenied:        true,
+			outstandingPerms: []string{"baz"},
+			errStr:           "",
+		},
+		{
+			requestedPerms: []string{"foo", "bar"},
+			permReturns: map[string]isPathPermAllowedResult{
+				"foo": {true, nil},
+				"bar": {true, nil},
+				"baz": {true, fmt.Errorf("baz")},
+			},
+			allowedPerms:     []string{"foo", "bar"},
+			anyDenied:        false,
+			outstandingPerms: []string{},
+			errStr:           "",
+		},
+		{
+			requestedPerms: []string{"foo", "bar", "baz"},
+			permReturns: map[string]isPathPermAllowedResult{
+				"foo": {true, fmt.Errorf("foo")},
+				"bar": {true, nil},
+				"baz": {true, nil},
+			},
+			allowedPerms:     []string{"bar", "baz"},
+			anyDenied:        false,
+			outstandingPerms: []string{},
+			errStr:           "foo",
+		},
+		{
+			requestedPerms: []string{"foo", "bar", "baz", "qux", "fizz", "buzz"},
+			permReturns: map[string]isPathPermAllowedResult{
+				"foo":  {true, fmt.Errorf("foo")},
+				"bar":  {true, nil},
+				"baz":  {true, prompting_errors.ErrNoMatchingRule},
+				"qux":  {false, fmt.Errorf("qux")},
+				"fizz": {false, nil},
+				"buzz": {false, prompting_errors.ErrNoMatchingRule},
+			},
+			allowedPerms:     []string{"bar"},
+			anyDenied:        true,
+			outstandingPerms: []string{"baz", "buzz"},
+			errStr:           "foo\nqux",
+		},
+	} {
+		restore := requestrules.MockIsPathPermAllowed(func(r *requestrules.RuleDB, u uint32, s string, i string, p string, perm string) (bool, error) {
+			c.Assert(r, Equals, rdb)
+			c.Assert(u, Equals, user)
+			c.Assert(s, Equals, snap)
+			c.Assert(i, Equals, iface)
+			c.Assert(p, Equals, path)
+			result := testCase.permReturns[perm]
+			return result.allowed, result.err
+		})
+		defer restore()
+
+		allowedPerms, anyDenied, outstandingPerms, err := rdb.IsRequestAllowed(user, snap, iface, path, testCase.requestedPerms)
+		c.Check(allowedPerms, DeepEquals, testCase.allowedPerms)
+		c.Check(anyDenied, Equals, testCase.anyDenied)
+		c.Check(outstandingPerms, DeepEquals, testCase.outstandingPerms)
+		if testCase.errStr == "" {
+			c.Check(err, IsNil)
+		} else {
+			c.Check(err, ErrorMatches, testCase.errStr)
+		}
+	}
+}
+
+func (s *requestrulesSuite) TestIsPathPermAllowedSimple(c *C) {
 	// Target
 	user := s.defaultUser
 	snap := "firefox"
@@ -1051,7 +1185,7 @@ func (s *requestrulesSuite) TestIsPathAllowedSimple(c *C) {
 			s.checkNewNoticesSimple(c, nil)
 		}
 
-		allowed, err := rdb.IsPathAllowed(user, snap, iface, path, permission)
+		allowed, err := rdb.IsPathPermAllowed(user, snap, iface, path, permission)
 		c.Check(err, Equals, testCase.err)
 		c.Check(allowed, Equals, testCase.allowed)
 		// Check that no notices were recorded when checking
@@ -1065,7 +1199,7 @@ func (s *requestrulesSuite) TestIsPathAllowedSimple(c *C) {
 	}
 }
 
-func (s *requestrulesSuite) TestIsPathAllowedPrecedence(c *C) {
+func (s *requestrulesSuite) TestIsPathPermAllowedPrecedence(c *C) {
 	// Target
 	user := s.defaultUser
 	snap := "firefox"
@@ -1122,13 +1256,13 @@ func (s *requestrulesSuite) TestIsPathAllowedPrecedence(c *C) {
 		mostRecentOutcome, err := ruleContents.Outcome.AsBool()
 		c.Check(err, IsNil)
 
-		allowed, err := rdb.IsPathAllowed(user, snap, iface, path, permission)
+		allowed, err := rdb.IsPathPermAllowed(user, snap, iface, path, permission)
 		c.Check(err, IsNil)
 		c.Check(allowed, Equals, mostRecentOutcome, Commentf("most recent: %+v", ruleContents))
 	}
 }
 
-func (s *requestrulesSuite) TestIsPathAllowedExpiration(c *C) {
+func (s *requestrulesSuite) TestIsPathPermAllowedExpiration(c *C) {
 	// Target
 	user := s.defaultUser
 	snap := "firefox"
@@ -1191,7 +1325,7 @@ func (s *requestrulesSuite) TestIsPathAllowedExpiration(c *C) {
 		c.Check(err, IsNil)
 
 		// Check that the outcome of the most specific unexpired rule has precedence
-		allowed, err := rdb.IsPathAllowed(user, snap, iface, path, permission)
+		allowed, err := rdb.IsPathPermAllowed(user, snap, iface, path, permission)
 		c.Check(err, IsNil)
 		c.Check(allowed, Equals, expectedOutcome, Commentf("last unexpired: %+v", rule))
 

overlord/ifacestate/apparmorprompting/prompting.go

@@ -20,7 +20,6 @@
 package apparmorprompting
 
 import (
-	"errors"
 	"fmt"
 	"sync"
 
@@ -212,54 +211,26 @@
 		return requestReply(req, nil)
 	}
 
-	outstandingPerms := make([]string, 0, len(permissions))
-	satisfiedPerms := make([]string, 0, len(permissions))
-
 	// we're done with early checks, serious business starts now, and we can
 	// take the lock
 	m.lock.Lock()
 	defer m.lock.Unlock()
 
-	matchedDenyRule := false
-	for _, perm := range permissions {
-		// TODO: move this for-loop to a helper in requestrules
-		if yesNo, err := m.rules.IsPathAllowed(userID, snap, iface, path, perm); err == nil {
-			if !yesNo {
-				matchedDenyRule = true
-			} else {
-				satisfiedPerms = append(satisfiedPerms, perm)
-			}
-		} else {
-			if !errors.Is(err, prompting_errors.ErrNoMatchingRule) {
-				logger.Noticef("error while checking request against existing rules: %v", err)
-			}
-			// No matching rule found
-			outstandingPerms = append(outstandingPerms, perm)
+	allowedPerms, matchedDenyRule, outstandingPerms, err := m.rules.IsRequestAllowed(userID, snap, iface, path, permissions)
+	if err != nil || matchedDenyRule || len(outstandingPerms) == 0 {
+		switch {
+		case err != nil:
+			logger.Noticef("error while checking request against existing rules: %v", err)
+		case matchedDenyRule:
+			logger.Debugf("request denied by existing rule: %+v", req)
+		case len(outstandingPerms) == 0:
+			logger.Debugf("request allowed by existing rule: %+v", req)
 		}
-	}
-	if matchedDenyRule {
-		logger.Debugf("request denied by existing rule: %+v", req)
-
-		// Respond with this information by allowing any requested permissions
-		// which were explicitly allowed by existing rules (there may be no
-		// such permissions) and let the listener deny all permissions which
-		// were not explicitly included in the allowed permissions.
-		allowedPermission, _ := prompting.AbstractPermissionsToAppArmorPermissions(iface, satisfiedPerms)
-		// Error should not occur, but if it does, allowedPermission is set to
-		// empty, leaving it to the listener to default deny all permissions.
-		return requestReply(req, allowedPermission)
-	}
-
-	if len(outstandingPerms) == 0 {
-		logger.Debugf("request allowed by existing rule: %+v", req)
-
-		// We don't want to just send back req.Permission() here, since that
-		// could include unrecognized permissions which were discarded, and
-		// were not matched by an existing rule. So only respond with the
-		// permissions which were matched and allowed, the listener will
-		// deny any permissions which were not explicitly included in the
-		// allowed permissions.
-		allowedPermission, _ := prompting.AbstractPermissionsToAppArmorPermissions(iface, satisfiedPerms)
+		// Allow any requested permissions which were explicitly allowed by
+		// existing rules (there may be no such permissions) and let the
+		// listener deny all permissions which were not explicitly included in
+		// the allowed permissions.
+		allowedPermission, _ := prompting.AbstractPermissionsToAppArmorPermissions(iface, allowedPerms)
 		// Error should not occur, but if it does, allowedPermission is set to
 		// empty, leaving it to the listener to default deny all permissions.
 		return requestReply(req, allowedPermission)
@@ -280,18 +251,18 @@
 		// We weren't able to create a new prompt, so respond with the best
 		// information we have, which is to allow any permissions which were
 		// allowed by existing rules, and let the listener deny the rest.
-		allowedPermission, _ := prompting.AbstractPermissionsToAppArmorPermissions(iface, satisfiedPerms)
+		allowedPermission, _ := prompting.AbstractPermissionsToAppArmorPermissions(iface, allowedPerms)
 		// Error should not occur, but if it does, allowedPermission is set to
 		// empty, leaving it to the listener to default deny all permissions.
 		return requestReply(req, allowedPermission)
 	}
+
 	if merged {
 		logger.Debugf("new prompt merged with identical existing prompt: %+v", newPrompt)
-		return nil
+	} else {
+		logger.Debugf("adding prompt to internal storage: %+v", newPrompt)
 	}
 
-	logger.Debugf("adding prompt to internal storage: %+v", newPrompt)
-
 	return nil
 }