i/p/requestrules,o/i/apparmorprompting: allow overlapping rules (cano…

…nical#14538)

* i/p/requestrules,o/i/apparmorprompting: allow overlapping rules

Allow several rules which render to the same variant to coexist in the
tree without conflict, so long as the outcome of all those overlapping
rules is identical.

This allows the client to reply with "allow read forever for /foo/bar"
and then later say "allow read|write forever for /foo/bar" without the
latter being treated as a rule conflict error. Clearly, the second rule
is a superset of the first, and there's no intent-based reason that
these two rules couldn't coexist, it was just an implementation detail
that we previously only allowed a pattern variant to be associated with
a single rule ID.

Now, each pattern variant in the tree for a particular snap, interface,
and permission can be associated with a set of rule IDs. Any non-expired
rules in that set must have the same outcome. Any expired rules in the
set are ignored (and removed when convenient).

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* i/p/requestrules: add clarifying comment about match outcome

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* i/p/requestrules: associate outcome with variant entry and clarify logic

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* i/p/requestrules: simplify closure which adds rule to tree

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

---------

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

interfaces/prompting/requestrules/requestrules.go

@@ -88,15 +88,17 @@ func (rule *Rule) Expired(currentTime time.Time) bool {
 	return false
 }
 
-// variantEntry stores the variant struct and the ID of its corresponding rule.
+// variantEntry stores the actual pattern variant struct which can be used to
+// match paths, and the set of rule IDs whose path patterns render to this
+// variant. All rules in a particular entry must have the same outcome, and
+// that outcome is stored directly in the variant entry itself.
 //
-// This is necessary since multiple variants might render to the same string,
-// and it would be necessary to make a deep comparison of two variants to tell
-// that they are the same. Since we want to map from variant to rule ID, we need
-// to use the variant string as the key.
+// Use the rendered string as the key for this entry, since pattern variants
+// cannot otherwise be easily checked for equality.
 type variantEntry struct {
 	Variant patterns.PatternVariant
-	RuleID  prompting.IDType
+	Outcome prompting.OutcomeType
+	RuleIDs map[prompting.IDType]bool
 }
 
 // permissionDB stores a map from path pattern variant to the ID of the rule
@@ -435,13 +437,22 @@ func (rdb *RuleDB) addRuleToTree(rule *Rule) *prompting_errors.RuleConflictError
 // addRulePermissionToTree adds all the path pattern variants for the given
 // rule to the map for the given permission.
 //
-// If there are conflicting pattern variants from other non-expired rules,
-// all variants which were previously added during this function call are
-// removed from the path map, leaving it unchanged, and the list of conflicts
-// is returned. If there are no conflicts, returns nil.
+// If there are identical pattern variants from other non-expired rules and the
+// outcomes of all those rules match the outcome of the new rule, then the ID
+// of the new rule is added to the set of rule IDs in the existing variant
+// entry.
 //
-// Conflicts with expired rules, however, result in the expired rule being
-// immediately removed, and the new rule can continue to be added.
+// If there are identical pattern variants from other non-expired rules and the
+// outcomes of those rules differ from that of the new rule, then there is a
+// conflict, and all variants which were previously added during this function
+// call are removed from the variant map, leaving it unchanged, and the list of
+// conflicts is returned. If there are no conflicts, returns nil.
+//
+// Expired rules, whether their outcome conflicts with the new rule or not, are
+// ignored and never treated as conflicts. If there are no conflicts with non-
+// expired rules, then all expired rules are removed. If there is a conflict
+// with a non-expired rule, then nothing about the rule DB state is changed,
+// including expired rules.
 //
 // The caller must ensure that the database lock is held for writing.
 func (rdb *RuleDB) addRulePermissionToTree(rule *Rule, permission string) []prompting_errors.RuleConflict {
@@ -452,36 +463,47 @@ func (rdb *RuleDB) addRulePermissionToTree(rule *Rule, permission string) []prom
 	var conflicts []prompting_errors.RuleConflict
 
 	addVariant := func(index int, variant patterns.PatternVariant) {
+		variantStr := variant.String()
+		existingEntry, exists := permVariants.VariantEntries[variantStr]
+		if !exists {
+			newVariantEntries[variantStr] = variantEntry{
+				Variant: variant,
+				Outcome: rule.Outcome,
+				RuleIDs: map[prompting.IDType]bool{rule.ID: true},
+			}
+			return
+		}
 		newEntry := variantEntry{
 			Variant: variant,
-			RuleID:  rule.ID,
+			Outcome: rule.Outcome,
+			RuleIDs: make(map[prompting.IDType]bool, len(existingEntry.RuleIDs)+1),
 		}
-		variantStr := variant.String()
-		conflictingVariantEntry, exists := permVariants.VariantEntries[variantStr]
-		switch {
-		case !exists:
-			newVariantEntries[variantStr] = newEntry
-		case rdb.isRuleWithIDExpired(conflictingVariantEntry.RuleID, rule.Timestamp):
-			expiredRules[conflictingVariantEntry.RuleID] = true
-			newVariantEntries[variantStr] = newEntry
-		default:
-			// XXX: If we ever switch to adding variants directly to the real
-			// variant entries map instead of a new map, check that the
-			// "conflicting" entry does not belong to the same rule that we're
-			// in the process of adding, which is possible if the rule's path
-			// pattern can render to duplicate variants. Ignore self-conflicts.
-
-			// Exists and is not expired, so there's a conflict
+		newEntry.RuleIDs[rule.ID] = true
+		newVariantEntries[variantStr] = newEntry
+		for id := range existingEntry.RuleIDs {
+			if rdb.isRuleWithIDExpired(id, rule.Timestamp) {
+				// Don't preserve expired rules, and don't care if they conflict
+				expiredRules[id] = true
+				continue
+			}
+			if existingEntry.Outcome == rule.Outcome {
+				// Preserve non-expired rule which doesn't conflict
+				newEntry.RuleIDs[id] = true
+				continue
+			}
+			// Conflicting non-expired rule
 			conflicts = append(conflicts, prompting_errors.RuleConflict{
 				Permission:    permission,
 				Variant:       variantStr,
-				ConflictingID: conflictingVariantEntry.RuleID.String(),
+				ConflictingID: id.String(),
 			})
 		}
 	}
 	rule.Constraints.PathPattern.RenderAllVariants(addVariant)
 
 	if len(conflicts) > 0 {
+		// If there are any conflicts, discard all changes, and do nothing
+		// about any expired rules.
 		return conflicts
 	}
 
@@ -495,6 +517,9 @@ func (rdb *RuleDB) addRulePermissionToTree(rule *Rule, permission string) []prom
 	}
 
 	for variantStr, entry := range newVariantEntries {
+		// Replace the old variant entries with the new ones.
+		// This removes any expired rules from the entries, since these were
+		// not preserved in the new variant entries.
 		permVariants.VariantEntries[variantStr] = entry
 	}
 
@@ -558,10 +583,9 @@ func (rdb *RuleDB) removeRulePermissionFromTree(rule *Rule, permission string) [
 		if !exists {
 			// Database was left inconsistent, should not occur
 			errs = append(errs, fmt.Errorf(`internal error: path pattern variant not found in the rule tree: %q`, variant))
-		} else if variantEntry.RuleID != rule.ID {
-			// Database was left inconsistent, should not occur
-			errs = append(errs, fmt.Errorf(`internal error: path pattern variant maps to different rule ID: %q: %s`, variant, variantEntry.RuleID.String()))
-		} else {
+		}
+		delete(variantEntry.RuleIDs, rule.ID)
+		if len(variantEntry.RuleIDs) == 0 {
 			delete(permVariants.VariantEntries, variantStr)
 		}
 	}
@@ -762,22 +786,19 @@ func (rdb *RuleDB) IsPathAllowed(user uint32, snap string, iface string, path st
 		return false, prompting_errors.ErrNoMatchingRule
 	}
 	variantMap := permissionMap.VariantEntries
-	matchingVariants := make([]patterns.PatternVariant, 0)
+	var matchingVariants []patterns.PatternVariant
 	// Make sure all rules use the same expiration timestamp, so a rule with
 	// an earlier expiration cannot outlive another rule with a later one.
 	currTime := time.Now()
 	for variantStr, variantEntry := range variantMap {
-		matchingRule, err := rdb.lookupRuleByID(variantEntry.RuleID)
-		if err != nil {
-			logger.Noticef("internal error: inconsistent DB when fetching rule %v", variantEntry.RuleID)
-			// Database was left inconsistent, should not occur
-			delete(variantMap, variantStr)
-			// Record a notice for the offending rule, just in case
-			rdb.notifyRule(user, variantEntry.RuleID, nil)
-			continue
+		nonExpired := false
+		for id := range variantEntry.RuleIDs {
+			if !rdb.isRuleWithIDExpired(id, currTime) {
+				nonExpired = true
+				break
+			}
 		}
-
-		if matchingRule.Expired(currTime) {
+		if !nonExpired {
 			continue
 		}
 
@@ -801,13 +822,7 @@ func (rdb *RuleDB) IsPathAllowed(user uint32, snap string, iface string, path st
 		return false, err
 	}
 	matchingEntry := variantMap[highestPrecedenceVariant.String()]
-	matchingID := matchingEntry.RuleID
-	matchingRule, err := rdb.lookupRuleByID(matchingID)
-	if err != nil {
-		// Database was left inconsistent, should not occur
-		return false, fmt.Errorf("internal error: while looking for rule %v: %w", matchingID, prompting_errors.ErrRuleNotFound)
-	}
-	return matchingRule.Outcome.AsBool()
+	return matchingEntry.Outcome.AsBool()
 }
 
 // RuleWithID returns the rule with the given ID.

interfaces/prompting/requestrules/requestrules_test.go

@@ -336,6 +336,7 @@ func (s *requestrulesSuite) TestLoadErrorConflictingPattern(c *C) {
 	conflicting := s.ruleTemplate(c, prompting.IDType(3))
 	// Even with not all permissions being in conflict, still error
 	conflicting.Constraints.Permissions = []string{"read", "write"}
+	conflicting.Outcome = prompting.OutcomeDeny
 
 	rules := []*requestrules.Rule{good, expired, conflicting}
 	s.writeRules(c, dbPath, rules)
@@ -725,6 +726,7 @@ func (s *requestrulesSuite) TestAddRuleErrors(c *C) {
 			&addRuleContents{
 				PathPattern: "/home/test/Pictures/**/*.{svg,jpg}",
 				Permissions: []string{"read", "write"},
+				Outcome:     prompting.OutcomeDeny,
 			},
 			fmt.Sprintf("cannot add rule: %v", prompting_errors.ErrRuleConflict),
 		},
@@ -764,6 +766,64 @@ func (s *requestrulesSuite) TestAddRuleErrors(c *C) {
 	s.checkNewNoticesSimple(c, nil)
 }
 
+func (s *requestrulesSuite) TestAddRuleOverlapping(c *C) {
+	rdb, err := requestrules.New(s.defaultNotifyRule)
+	c.Assert(err, IsNil)
+
+	template := &addRuleContents{
+		User:        s.defaultUser,
+		Snap:        "lxd",
+		Interface:   "home",
+		PathPattern: "/home/test/Pictures/**/*.png",
+		Permissions: []string{"write"},
+		Outcome:     prompting.OutcomeAllow,
+		Lifespan:    prompting.LifespanForever,
+		Duration:    "",
+	}
+
+	var addedRules []*requestrules.Rule
+
+	// Add one rule matching template, then various overlapping rules, and
+	// check that all rules add without error.
+	for _, ruleContents := range []*addRuleContents{
+		{}, // use template
+		{PathPattern: "/home/test/Pictures/**/*.{jpg,png,svg}"},
+		{Permissions: []string{"read", "write"}},
+		{PathPattern: "/home/test/Pictures/**/*.{jp,pn,sv}g"},
+		{PathPattern: "/home/test/{Documents,Pictures}/**/*.{jpg,png,svg}", Permissions: []string{"read", "write", "execute"}},
+		{}, // template again, for good measure
+	} {
+		rule, err := addRuleFromTemplate(c, rdb, template, ruleContents)
+		c.Check(err, IsNil)
+		c.Check(rule, NotNil)
+		addedRules = append(addedRules, rule)
+		s.checkWrittenRuleDB(c, addedRules)
+		s.checkNewNoticesSimple(c, nil, rule)
+	}
+
+	// Lastly, add a conflicting rule, and check that it conflicts with all
+	// the prior rules
+	rule, err := addRuleFromTemplate(c, rdb, template, &addRuleContents{
+		Outcome: prompting.OutcomeDeny,
+	})
+	c.Check(err, NotNil)
+	c.Check(rule, IsNil)
+	var ruleConflictErr *prompting_errors.RuleConflictError
+	if !errors.As(err, &ruleConflictErr) {
+		c.Fatalf("cannot cast error as RuleConflictError: %v", err)
+	}
+	c.Check(ruleConflictErr.Conflicts, HasLen, len(addedRules))
+outer:
+	for _, existing := range addedRules {
+		for _, conflict := range ruleConflictErr.Conflicts {
+			if conflict.ConflictingID == existing.ID.String() {
+				continue outer
+			}
+		}
+		c.Errorf("conflict error does not include existing rule: %+v", existing)
+	}
+}
+
 func (s *requestrulesSuite) TestAddRuleExpired(c *C) {
 	rdb, err := requestrules.New(s.defaultNotifyRule)
 	c.Assert(err, IsNil)
@@ -800,16 +860,17 @@ func (s *requestrulesSuite) TestAddRuleExpired(c *C) {
 	// Add rules which all conflict but each expire before the next is added,
 	// thus causing the prior one to be removed and not causing a conflict error.
 	for _, ruleContents := range []*addRuleContents{
-		{}, // use template
-		{PathPattern: "/home/test/{**/secret,**/private}/**"},
-		{PathPattern: "/home/test/**/{sec,priv}{ret,ate}/**", Permissions: []string{"read", "write"}},
-		{Permissions: []string{"write", "execute"}},
+		{Outcome: prompting.OutcomeDeny},
+		{Outcome: prompting.OutcomeAllow, PathPattern: "/home/test/{**/secret,**/private}/**"},
+		{Outcome: prompting.OutcomeDeny, PathPattern: "/home/test/**/{sec,priv}{ret,ate}/**", Permissions: []string{"read", "write"}},
+		{Outcome: prompting.OutcomeAllow, Permissions: []string{"write", "execute"}},
+		{Outcome: prompting.OutcomeDeny, PathPattern: "/home/test/*{*/secret/*,*/private/*}*"},
 	} {
 		ruleContents.Lifespan = prompting.LifespanTimespan
 		ruleContents.Duration = "1ms"
 		newRule, err := addRuleFromTemplate(c, rdb, template, ruleContents)
 		c.Check(err, IsNil)
-		c.Check(newRule, NotNil)
+		c.Assert(newRule, NotNil)
 		s.checkWrittenRuleDB(c, []*requestrules.Rule{good, newRule})
 		expectedNoticeInfo := []*noticeInfo{
 			{
@@ -1677,11 +1738,12 @@ func (s *requestrulesSuite) TestPatchRuleErrors(c *C) {
 	s.checkNewNoticesSimple(c, nil)
 
 	// Conflicting rule
+	conflictingOutcome := prompting.OutcomeDeny
 	conflictingConstraints := &prompting.Constraints{
 		PathPattern: mustParsePathPattern(c, template.PathPattern),
 		Permissions: []string{"read", "write", "execute"},
 	}
-	result, err = rdb.PatchRule(rule.User, rule.ID, conflictingConstraints, prompting.OutcomeUnset, prompting.LifespanUnset, "")
+	result, err = rdb.PatchRule(rule.User, rule.ID, conflictingConstraints, conflictingOutcome, prompting.LifespanUnset, "")
 	c.Check(err, ErrorMatches, fmt.Sprintf("cannot patch rule: %v", prompting_errors.ErrRuleConflict))
 	c.Check(result, IsNil)
 	s.checkWrittenRuleDB(c, rules)

overlord/ifacestate/apparmorprompting/prompting_test.go

@@ -450,11 +450,12 @@ func (s *apparmorpromptingSuite) TestHandleReplyErrors(c *C) {
 	newRule, err := mgr.AddRule(s.defaultUser, "firefox", "home", &badPatternConstraints, prompting.OutcomeAllow, prompting.LifespanTimespan, "10s")
 	c.Assert(err, IsNil)
 	c.Assert(newRule, NotNil)
+	conflictingOutcome := prompting.OutcomeDeny
 	conflictingConstraints := prompting.Constraints{
 		PathPattern: mustParsePathPattern(c, "/home/test/{foo,other}"),
 		Permissions: []string{"read"},
 	}
-	result, err = mgr.HandleReply(s.defaultUser, prompt.ID, &conflictingConstraints, prompting.OutcomeAllow, prompting.LifespanForever, "")
+	result, err = mgr.HandleReply(s.defaultUser, prompt.ID, &conflictingConstraints, conflictingOutcome, prompting.LifespanForever, "")
 	c.Check(err, ErrorMatches, "cannot add rule.*")
 	c.Check(result, IsNil)