From eb10285ec0cd72a955c8dded776b15b17bbfb991 Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Wed, 25 Sep 2024 16:50:18 -0500 Subject: [PATCH 1/4] 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 --- .../prompting/requestrules/requestrules.go | 111 ++++++++++-------- .../requestrules/requestrules_test.go | 74 +++++++++++- .../apparmorprompting/prompting_test.go | 3 +- 3 files changed, 132 insertions(+), 56 deletions(-) diff --git a/interfaces/prompting/requestrules/requestrules.go b/interfaces/prompting/requestrules/requestrules.go index f314f394ee3..4f765db5ada 100644 --- a/interfaces/prompting/requestrules/requestrules.go +++ b/interfaces/prompting/requestrules/requestrules.go @@ -88,15 +88,16 @@ 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 map from rule IDs whose path patterns render to this +// variant to the outcome of those rules. All rules in a particular entry must +// have the same outcome. // -// 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 + RuleIDs map[prompting.IDType]prompting.OutcomeType } // permissionDB stores a map from path pattern variant to the ID of the rule @@ -435,10 +436,16 @@ 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. +// +// 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. // // Conflicts with expired rules, however, result in the expired rule being // immediately removed, and the new rule can continue to be added. @@ -452,30 +459,40 @@ 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, + RuleIDs: map[prompting.IDType]prompting.OutcomeType{rule.ID: rule.Outcome}, + } + return + } + // Construct the new entry so we can keep track of any non-expired + // rule IDs newEntry := variantEntry{ Variant: variant, - RuleID: rule.ID, + RuleIDs: make(map[prompting.IDType]prompting.OutcomeType, 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] = rule.Outcome + newVariantEntries[variantStr] = newEntry + // At least one rule with identical variant already exists, so check + // the outcome of any non-expired rules + for id, outcome := range existingEntry.RuleIDs { + if rdb.isRuleWithIDExpired(id, rule.Timestamp) { + expiredRules[id] = true + continue + } + // Preserve this non-expired rule ID + newEntry.RuleIDs[id] = outcome + if outcome == rule.Outcome { + continue + } + // Conflicting non-expired rule conflicts = append(conflicts, prompting_errors.RuleConflict{ Permission: permission, Variant: variantStr, - ConflictingID: conflictingVariantEntry.RuleID.String(), + ConflictingID: id.String(), }) } } @@ -558,10 +575,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 +778,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 +814,13 @@ 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) + for id, outcome := range matchingEntry.RuleIDs { + if !rdb.isRuleWithIDExpired(id, currTime) { + return outcome.AsBool() + } } - return matchingRule.Outcome.AsBool() + // Cannot occur, since we know the matching entry has a non-expired rule + return false, fmt.Errorf("internal error: variant entry has no non-expired rules: %q", highestPrecedenceVariant.String()) } // RuleWithID returns the rule with the given ID. diff --git a/interfaces/prompting/requestrules/requestrules_test.go b/interfaces/prompting/requestrules/requestrules_test.go index e96bdea2c5e..eb3d1f477f5 100644 --- a/interfaces/prompting/requestrules/requestrules_test.go +++ b/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) diff --git a/overlord/ifacestate/apparmorprompting/prompting_test.go b/overlord/ifacestate/apparmorprompting/prompting_test.go index 352187b3d1f..d67b0d7e027 100644 --- a/overlord/ifacestate/apparmorprompting/prompting_test.go +++ b/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) From bd296613d6a6bf8cf7a122c2b5c3181f43c8420d Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Wed, 25 Sep 2024 17:20:50 -0500 Subject: [PATCH 2/4] i/p/requestrules: add clarifying comment about match outcome Signed-off-by: Oliver Calder --- interfaces/prompting/requestrules/requestrules.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/interfaces/prompting/requestrules/requestrules.go b/interfaces/prompting/requestrules/requestrules.go index 4f765db5ada..a0df3266eec 100644 --- a/interfaces/prompting/requestrules/requestrules.go +++ b/interfaces/prompting/requestrules/requestrules.go @@ -814,6 +814,8 @@ func (rdb *RuleDB) IsPathAllowed(user uint32, snap string, iface string, path st return false, err } matchingEntry := variantMap[highestPrecedenceVariant.String()] + // All non-expired rules have the same outcome, so grab it from the first + // one we find for id, outcome := range matchingEntry.RuleIDs { if !rdb.isRuleWithIDExpired(id, currTime) { return outcome.AsBool() From bb21adc98b1b8b162075b0d89e5ead2d319f020e Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Thu, 26 Sep 2024 09:13:41 -0500 Subject: [PATCH 3/4] i/p/requestrules: associate outcome with variant entry and clarify logic Signed-off-by: Oliver Calder --- .../prompting/requestrules/requestrules.go | 80 +++++++++++-------- 1 file changed, 46 insertions(+), 34 deletions(-) diff --git a/interfaces/prompting/requestrules/requestrules.go b/interfaces/prompting/requestrules/requestrules.go index a0df3266eec..ccd2d78d815 100644 --- a/interfaces/prompting/requestrules/requestrules.go +++ b/interfaces/prompting/requestrules/requestrules.go @@ -89,15 +89,16 @@ func (rule *Rule) Expired(currentTime time.Time) bool { } // variantEntry stores the actual pattern variant struct which can be used to -// match paths, and the map from rule IDs whose path patterns render to this -// variant to the outcome of those rules. All rules in a particular entry must -// have the same outcome. +// 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. // // 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 - RuleIDs map[prompting.IDType]prompting.OutcomeType + Outcome prompting.OutcomeType + RuleIDs map[prompting.IDType]bool } // permissionDB stores a map from path pattern variant to the ID of the rule @@ -447,8 +448,11 @@ func (rdb *RuleDB) addRuleToTree(rule *Rule) *prompting_errors.RuleConflictError // call are removed from the variant map, leaving it unchanged, and the list of // conflicts is returned. If there are no conflicts, returns nil. // -// Conflicts with expired rules, however, result in the expired rule being -// immediately removed, and the new rule can continue to be added. +// 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 { @@ -464,36 +468,52 @@ func (rdb *RuleDB) addRulePermissionToTree(rule *Rule, permission string) []prom if !exists { newVariantEntries[variantStr] = variantEntry{ Variant: variant, - RuleIDs: map[prompting.IDType]prompting.OutcomeType{rule.ID: rule.Outcome}, + Outcome: rule.Outcome, + RuleIDs: map[prompting.IDType]bool{rule.ID: true}, } return } - // Construct the new entry so we can keep track of any non-expired - // rule IDs + if existingEntry.Outcome != rule.Outcome { + // Outcomes mismatch, so if there are any unexpired rules, these + // all conflict with the new rule. If all rules are expired, this + // is fine, and we can replace the existing entry with a new one. + for id := range existingEntry.RuleIDs { + if rdb.isRuleWithIDExpired(id, rule.Timestamp) { + expiredRules[id] = true + continue + } + // Conflicting non-expired rule + conflicts = append(conflicts, prompting_errors.RuleConflict{ + Permission: permission, + Variant: variantStr, + ConflictingID: id.String(), + }) + } + } + if len(conflicts) > 0 { + // If there are any conflicts (for this variant or others), all + // changes will be discarded, so don't bother building the new + // variant entry + return + } newEntry := variantEntry{ Variant: variant, - RuleIDs: make(map[prompting.IDType]prompting.OutcomeType, len(existingEntry.RuleIDs)+1), + Outcome: rule.Outcome, + RuleIDs: make(map[prompting.IDType]bool, len(existingEntry.RuleIDs)+1), } - newEntry.RuleIDs[rule.ID] = rule.Outcome + newEntry.RuleIDs[rule.ID] = true newVariantEntries[variantStr] = newEntry - // At least one rule with identical variant already exists, so check - // the outcome of any non-expired rules - for id, outcome := range existingEntry.RuleIDs { + if existingEntry.Outcome != rule.Outcome { + // We know all existing rule IDs were expired, else there would + // have been a conflict + return + } + for id := range existingEntry.RuleIDs { if rdb.isRuleWithIDExpired(id, rule.Timestamp) { expiredRules[id] = true continue } - // Preserve this non-expired rule ID - newEntry.RuleIDs[id] = outcome - if outcome == rule.Outcome { - continue - } - // Conflicting non-expired rule - conflicts = append(conflicts, prompting_errors.RuleConflict{ - Permission: permission, - Variant: variantStr, - ConflictingID: id.String(), - }) + newEntry.RuleIDs[id] = true } } rule.Constraints.PathPattern.RenderAllVariants(addVariant) @@ -814,15 +834,7 @@ func (rdb *RuleDB) IsPathAllowed(user uint32, snap string, iface string, path st return false, err } matchingEntry := variantMap[highestPrecedenceVariant.String()] - // All non-expired rules have the same outcome, so grab it from the first - // one we find - for id, outcome := range matchingEntry.RuleIDs { - if !rdb.isRuleWithIDExpired(id, currTime) { - return outcome.AsBool() - } - } - // Cannot occur, since we know the matching entry has a non-expired rule - return false, fmt.Errorf("internal error: variant entry has no non-expired rules: %q", highestPrecedenceVariant.String()) + return matchingEntry.Outcome.AsBool() } // RuleWithID returns the rule with the given ID. From ac6c4e2e7a67f492a3658912704eaf61c1b26f45 Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Mon, 7 Oct 2024 21:33:01 -0500 Subject: [PATCH 4/4] many: support mixed outcomes for permissions in prompting constraints Let rule content constraints have heterogeneous outcomes and lifespans for different permissions in the constraints. As such, convert the list of permissions to a map from permission to permission entry, where the entry holds the outcome, lifespan, and duration/expiration for that particular permission, where previous those were global to the containing rule, rule contents, or patch contents. However, the existing concept of replying "allow"/"deny" to a particular set of requested permisisons is clear and simple. We want to keep outcome, lifespan, and duration as reply-scoped values, not permission-specific, when accepting prompt replies. So we need different types of constraints for prompt replies vs. rule contents. The motivation behind this is so that we can have only a single rule for any given path pattern. We may have a situation where the user previously replied with "allow read `/path/to/foo`" and they're now prompted for write access, they need to be able to respond with "deny read `/path/to/foo`". If we only support a single outcome for any given rule, then we'd need two rules for the same path `/path/to/foo`. Thus, we need rules to support different outcomes for different permissions. The same logic applies for lifetimes and expirations, though this adds additional complexity now that the concept of rule expiration is shifted to being permission-specific. We care about expired rules in two primary places: when loading rules from disk, we want to discard any expired rules, and when adding a new rule, we want to discard any expired permisison entry for a rule which shares a pattern variant with the new rule. For cases where that expired permission entry had a conflicting outcome, we clearly can't have that, and we want to remove the expired permission entry from its containing rule as well, so as to avoid confusion for the user without them needing to check expiration timestamps. Even if the outcome of the expired entry matches that of the new rule's entry for the same permission, we still want to prune the expired permission from the old rule to avoid confusion. The complexity is around when a notice is recorded for a rule for which some permissions have expired. At the moment, the logic is that a notice is recorded in these cases: - when a rule is loaded from disk - data may be `"removed": "expired"` if all permissions are expired - when a rule is added - when a rule is patched - when a rule is removed (with data `"removed": "removed"`) - when a rule is found to be expired when attempting to add a new rule Notably, a notice is not recorded automatically when a permission entry expires. Nor is a notice recorded when a permission is found to be expired, so long as its associated rule still has at least one non-expired permission. Neither pruning an expired permission entry from the rule tree nor from the entry's containing rule results in a notice, even though technically the rule data has changed, since the expired permission has been erased. The rationale is that the semantics of the rule have not changed, since the expiration of that permission was part of the semantics of the rule. Since durations are used when adding/patching a rule and expirations are used when retrieving a rule, in addition to the differences for prompt replies vs. rule contents, we now need several different variants of constraints: - `promptConstraints`: - path, requested permissions list, available permissions list - internal to `requestprompts`, unchanged - `ReplyConstraints`: - path pattern, list of permissions - containing `PromptReply` holds outcome/lifespan/expiration - unchanged from before, though under a new name - converted to a `Constraints` if reply warrants a new rule - `Constraints`: - path pattern, map from permission to outcome, lifespan, duration - used when adding rule to the rule DB - converted to `RuleConstraints` when the new rule is created - `RuleConstraints`: - path pattern, map from permisison to outcome, lifespan, expiration - used when retrieving rules from the rule DB - never used when POSTing to the API - `PatchConstraints`: - identical to `Constraints`, but with omitempty fields - converted to `RuleConstraints` when the patched rule is created To support this, we define some new types, including `{,Rule}PermissionMap` and `{,Rule}PermissionEntry`. The latter of these is used in the leaves of the rule DB tree in place of the previous set of rule IDs of rules whose patterns render to a given pattern variant. Whenever possible, logic surrounding constraints, permissions, and expiration is pushed down to methods on these new types, thus simplifiying the logic of their callers. Signed-off-by: Oliver Calder --- daemon/api_prompting.go | 20 +- daemon/api_prompting_test.go | 180 ++++---- interfaces/prompting/constraints.go | 406 ++++++++++++++++-- interfaces/prompting/constraints_test.go | 46 +- interfaces/prompting/errors/errors.go | 33 +- interfaces/prompting/export_test.go | 4 - interfaces/prompting/prompting.go | 10 +- interfaces/prompting/prompting_test.go | 11 +- .../requestprompts/requestprompts.go | 159 ++++--- .../requestprompts/requestprompts_test.go | 81 ++-- .../prompting/requestrules/export_test.go | 2 +- .../prompting/requestrules/requestrules.go | 375 ++++++++-------- .../requestrules/requestrules_test.go | 374 ++++++++++------ .../ifacestate/apparmorprompting/prompting.go | 63 +-- .../apparmorprompting/prompting_test.go | 206 ++++++--- 15 files changed, 1293 insertions(+), 677 deletions(-) diff --git a/daemon/api_prompting.go b/daemon/api_prompting.go index cde77f6a20b..cfba4bb2619 100644 --- a/daemon/api_prompting.go +++ b/daemon/api_prompting.go @@ -283,19 +283,16 @@ var getInterfaceManager = func(c *Command) interfaceManager { } type postPromptBody struct { - Outcome prompting.OutcomeType `json:"action"` - Lifespan prompting.LifespanType `json:"lifespan"` - Duration string `json:"duration,omitempty"` - Constraints *prompting.Constraints `json:"constraints"` + Outcome prompting.OutcomeType `json:"action"` + Lifespan prompting.LifespanType `json:"lifespan"` + Duration string `json:"duration,omitempty"` + Constraints *prompting.ReplyConstraints `json:"constraints"` } type addRuleContents struct { Snap string `json:"snap"` Interface string `json:"interface"` Constraints *prompting.Constraints `json:"constraints"` - Outcome prompting.OutcomeType `json:"outcome"` - Lifespan prompting.LifespanType `json:"lifespan"` - Duration string `json:"duration,omitempty"` } type removeRulesSelector struct { @@ -304,10 +301,7 @@ type removeRulesSelector struct { } type patchRuleContents struct { - Constraints *prompting.Constraints `json:"constraints,omitempty"` - Outcome prompting.OutcomeType `json:"outcome,omitempty"` - Lifespan prompting.LifespanType `json:"lifespan,omitempty"` - Duration string `json:"duration,omitempty"` + Constraints *prompting.PatchConstraints `json:"constraints,omitempty"` } type postRulesRequestBody struct { @@ -452,7 +446,7 @@ func postRules(c *Command, r *http.Request, user *auth.UserState) Response { if postBody.AddRule == nil { return BadRequest(`must include "rule" field in request body when action is "add"`) } - newRule, err := getInterfaceManager(c).InterfacesRequestsManager().AddRule(userID, postBody.AddRule.Snap, postBody.AddRule.Interface, postBody.AddRule.Constraints, postBody.AddRule.Outcome, postBody.AddRule.Lifespan, postBody.AddRule.Duration) + newRule, err := getInterfaceManager(c).InterfacesRequestsManager().AddRule(userID, postBody.AddRule.Snap, postBody.AddRule.Interface, postBody.AddRule.Constraints) if err != nil { return promptingError(err) } @@ -529,7 +523,7 @@ func postRule(c *Command, r *http.Request, user *auth.UserState) Response { if postBody.PatchRule == nil { return BadRequest(`must include "rule" field in request body when action is "patch"`) } - patchedRule, err := getInterfaceManager(c).InterfacesRequestsManager().PatchRule(userID, ruleID, postBody.PatchRule.Constraints, postBody.PatchRule.Outcome, postBody.PatchRule.Lifespan, postBody.PatchRule.Duration) + patchedRule, err := getInterfaceManager(c).InterfacesRequestsManager().PatchRule(userID, ruleID, postBody.PatchRule.Constraints) if err != nil { return promptingError(err) } diff --git a/daemon/api_prompting_test.go b/daemon/api_prompting_test.go index 4e7a5d634cc..fbe751d8cb4 100644 --- a/daemon/api_prompting_test.go +++ b/daemon/api_prompting_test.go @@ -52,14 +52,16 @@ type fakeInterfacesRequestsManager struct { err error // Store most recent received values - userID uint32 - snap string - iface string - id prompting.IDType // used for prompt ID or rule ID - constraints *prompting.Constraints - outcome prompting.OutcomeType - lifespan prompting.LifespanType - duration string + userID uint32 + snap string + iface string + id prompting.IDType // used for prompt ID or rule ID + ruleConstraints *prompting.Constraints + patchConstraints *prompting.PatchConstraints + replyConstraints *prompting.ReplyConstraints + outcome prompting.OutcomeType + lifespan prompting.LifespanType + duration string } func (m *fakeInterfacesRequestsManager) Prompts(userID uint32) ([]*requestprompts.Prompt, error) { @@ -73,10 +75,10 @@ func (m *fakeInterfacesRequestsManager) PromptWithID(userID uint32, promptID pro return m.prompt, m.err } -func (m *fakeInterfacesRequestsManager) HandleReply(userID uint32, promptID prompting.IDType, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) ([]prompting.IDType, error) { +func (m *fakeInterfacesRequestsManager) HandleReply(userID uint32, promptID prompting.IDType, constraints *prompting.ReplyConstraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) ([]prompting.IDType, error) { m.userID = userID m.id = promptID - m.constraints = constraints + m.replyConstraints = constraints m.outcome = outcome m.lifespan = lifespan m.duration = duration @@ -90,14 +92,11 @@ func (m *fakeInterfacesRequestsManager) Rules(userID uint32, snap string, iface return m.rules, m.err } -func (m *fakeInterfacesRequestsManager) AddRule(userID uint32, snap string, iface string, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) (*requestrules.Rule, error) { +func (m *fakeInterfacesRequestsManager) AddRule(userID uint32, snap string, iface string, constraints *prompting.Constraints) (*requestrules.Rule, error) { m.userID = userID m.snap = snap m.iface = iface - m.constraints = constraints - m.outcome = outcome - m.lifespan = lifespan - m.duration = duration + m.ruleConstraints = constraints return m.rule, m.err } @@ -114,13 +113,10 @@ func (m *fakeInterfacesRequestsManager) RuleWithID(userID uint32, ruleID prompti return m.rule, m.err } -func (m *fakeInterfacesRequestsManager) PatchRule(userID uint32, ruleID prompting.IDType, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) (*requestrules.Rule, error) { +func (m *fakeInterfacesRequestsManager) PatchRule(userID uint32, ruleID prompting.IDType, constraints *prompting.PatchConstraints) (*requestrules.Rule, error) { m.userID = userID m.id = ruleID - m.constraints = constraints - m.outcome = outcome - m.lifespan = lifespan - m.duration = duration + m.patchConstraints = constraints return m.rule, m.err } @@ -700,7 +696,7 @@ func (s *promptingSuite) TestPostPromptHappy(c *C) { prompting.IDType(0xF00BA4), } - constraints := &prompting.Constraints{ + constraints := &prompting.ReplyConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/Pictures/**/*.{png,svg}"), Permissions: []string{"read", "execute"}, } @@ -718,7 +714,7 @@ func (s *promptingSuite) TestPostPromptHappy(c *C) { // Check parameters c.Check(s.manager.userID, Equals, uint32(1000)) c.Check(s.manager.id, Equals, prompting.IDType(0x0123456789abcdef)) - c.Check(s.manager.constraints, DeepEquals, contents.Constraints) + c.Check(s.manager.replyConstraints, DeepEquals, contents.Constraints) c.Check(s.manager.outcome, Equals, contents.Outcome) c.Check(s.manager.lifespan, Equals, contents.Lifespan) c.Check(s.manager.duration, Equals, contents.Duration) @@ -775,13 +771,15 @@ func (s *promptingSuite) TestGetRulesHappy(c *C) { User: 1234, Snap: "firefox", Interface: "home", - Constraints: &prompting.Constraints{ + Constraints: &prompting.RuleConstraints{ PathPattern: mustParsePathPattern(c, "/foo/bar"), - Permissions: []string{"write"}, + Permissions: prompting.RulePermissionMap{ + "write": &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + }, }, - Outcome: prompting.OutcomeDeny, - Lifespan: prompting.LifespanForever, - Expiration: time.Now(), }, } @@ -810,26 +808,34 @@ func (s *promptingSuite) TestPostRulesAddHappy(c *C) { User: 11235, Snap: "firefox", Interface: "home", - Constraints: &prompting.Constraints{ + Constraints: &prompting.RuleConstraints{ PathPattern: mustParsePathPattern(c, "/foo/bar/baz"), - Permissions: []string{"write"}, + Permissions: prompting.RulePermissionMap{ + "write": &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + }, }, - Outcome: prompting.OutcomeDeny, - Lifespan: prompting.LifespanForever, - Expiration: time.Now(), } constraints := &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/{foo,bar,baz}/**/*.{png,svg}"), - Permissions: []string{"read", "write"}, + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + "write": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, } contents := &daemon.AddRuleContents{ Snap: "thunderbird", Interface: "home", Constraints: constraints, - Outcome: prompting.OutcomeAllow, - Lifespan: prompting.LifespanForever, - Duration: "", } postBody := &daemon.PostRulesRequestBody{ Action: "add", @@ -844,10 +850,7 @@ func (s *promptingSuite) TestPostRulesAddHappy(c *C) { c.Check(s.manager.userID, Equals, uint32(11235)) c.Check(s.manager.snap, Equals, contents.Snap) c.Check(s.manager.iface, Equals, contents.Interface) - c.Check(s.manager.constraints, DeepEquals, contents.Constraints) - c.Check(s.manager.outcome, Equals, contents.Outcome) - c.Check(s.manager.lifespan, Equals, contents.Lifespan) - c.Check(s.manager.duration, Equals, contents.Duration) + c.Check(s.manager.ruleConstraints, DeepEquals, contents.Constraints) // Check return value rule, ok := rsp.Result.(*requestrules.Rule) @@ -881,7 +884,6 @@ func (s *promptingSuite) TestPostRulesRemoveHappy(c *C) { s.manager = &fakeInterfacesRequestsManager{} // Set the rules to return - var timeZero time.Time s.manager.rules = []*requestrules.Rule{ { ID: prompting.IDType(1234), @@ -889,13 +891,15 @@ func (s *promptingSuite) TestPostRulesRemoveHappy(c *C) { User: 1001, Snap: "thunderird", Interface: "home", - Constraints: &prompting.Constraints{ + Constraints: &prompting.RuleConstraints{ PathPattern: mustParsePathPattern(c, "/foo/bar/baz/qux"), - Permissions: []string{"write"}, + Permissions: prompting.RulePermissionMap{ + "write": &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + }, }, - Outcome: prompting.OutcomeDeny, - Lifespan: prompting.LifespanForever, - Expiration: timeZero, }, { ID: prompting.IDType(5678), @@ -903,13 +907,19 @@ func (s *promptingSuite) TestPostRulesRemoveHappy(c *C) { User: 1001, Snap: "thunderbird", Interface: "home", - Constraints: &prompting.Constraints{ + Constraints: &prompting.RuleConstraints{ PathPattern: mustParsePathPattern(c, "/fizz/buzz"), - Permissions: []string{"read", "execute"}, + Permissions: prompting.RulePermissionMap{ + "read": &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanTimespan, + }, + "execute": &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanTimespan, + }, + }, }, - Outcome: prompting.OutcomeAllow, - Lifespan: prompting.LifespanTimespan, - Expiration: time.Now(), }, } @@ -948,13 +958,16 @@ func (s *promptingSuite) TestGetRuleHappy(c *C) { User: 1005, Snap: "thunderbird", Interface: "home", - Constraints: &prompting.Constraints{ + Constraints: &prompting.RuleConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/Videos/**/*.{mkv,mp4,mov}"), - Permissions: []string{"read"}, + Permissions: prompting.RulePermissionMap{ + "read": &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanTimespan, + Expiration: time.Now().Add(-24 * time.Hour), + }, + }, }, - Outcome: prompting.OutcomeAllow, - Lifespan: prompting.LifespanTimespan, - Expiration: time.Now().Add(-24 * time.Hour), } rsp := s.makeSyncReq(c, "GET", "/v2/interfaces/requests/rules/000000000000012B", 1005, nil) @@ -974,31 +987,42 @@ func (s *promptingSuite) TestPostRulePatchHappy(c *C) { s.daemon(c) - var timeZero time.Time s.manager.rule = &requestrules.Rule{ ID: prompting.IDType(0x01123581321), Timestamp: time.Now(), User: 999, Snap: "gimp", Interface: "home", - Constraints: &prompting.Constraints{ + Constraints: &prompting.RuleConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/Pictures/**/*.{png,jpg}"), - Permissions: []string{"read", "write"}, + Permissions: prompting.RulePermissionMap{ + "read": &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + "write": &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, }, - Outcome: prompting.OutcomeAllow, - Lifespan: prompting.LifespanForever, - Expiration: timeZero, } - constraints := &prompting.Constraints{ + constraints := &prompting.PatchConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/Pictures/**/*.{png,jpg}"), - Permissions: []string{"read", "write"}, + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + "write": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, } contents := &daemon.PatchRuleContents{ Constraints: constraints, - Outcome: prompting.OutcomeAllow, - Lifespan: prompting.LifespanForever, - Duration: "", } postBody := &daemon.PostRuleRequestBody{ Action: "patch", @@ -1011,10 +1035,7 @@ func (s *promptingSuite) TestPostRulePatchHappy(c *C) { // Check parameters c.Check(s.manager.userID, Equals, uint32(999)) - c.Check(s.manager.constraints, DeepEquals, contents.Constraints) - c.Check(s.manager.outcome, Equals, contents.Outcome) - c.Check(s.manager.lifespan, Equals, contents.Lifespan) - c.Check(s.manager.duration, Equals, contents.Duration) + c.Check(s.manager.patchConstraints, DeepEquals, contents.Constraints) // Check return value rule, ok := rsp.Result.(*requestrules.Rule) @@ -1027,20 +1048,25 @@ func (s *promptingSuite) TestPostRuleRemoveHappy(c *C) { s.daemon(c) - var timeZero time.Time s.manager.rule = &requestrules.Rule{ ID: prompting.IDType(0x01123581321), Timestamp: time.Now(), User: 100, Snap: "gimp", Interface: "home", - Constraints: &prompting.Constraints{ + Constraints: &prompting.RuleConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/Pictures/**/*.{png,jpg}"), - Permissions: []string{"read", "write"}, + Permissions: prompting.RulePermissionMap{ + "read": &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + "write": &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, }, - Outcome: prompting.OutcomeAllow, - Lifespan: prompting.LifespanForever, - Expiration: timeZero, } postBody := &daemon.PostRuleRequestBody{ Action: "remove", diff --git a/interfaces/prompting/constraints.go b/interfaces/prompting/constraints.go index b07c04a039d..c3a8118ad55 100644 --- a/interfaces/prompting/constraints.go +++ b/interfaces/prompting/constraints.go @@ -22,6 +22,7 @@ package prompting import ( "fmt" "sort" + "time" prompting_errors "github.com/snapcore/snapd/interfaces/prompting/errors" "github.com/snapcore/snapd/interfaces/prompting/patterns" @@ -30,62 +31,128 @@ import ( "github.com/snapcore/snapd/strutil" ) -// Constraints hold information about the applicability of a rule to particular -// paths or permissions. A request matches the constraints if the requested path -// is matched by the path pattern (according to bash's globstar matching) and -// the requested permissions are contained in the constraints' permissions. +// Constraints hold information about the applicability of a new rule to +// particular paths or permissions. A request will be matched by the constraints +// if the requested path is matched by the path pattern (according to bash's +// globstar matching) and one or more requested permissions are denied in the +// permission map, or all of the requested permissions are allowed in the map. +// When snapd creates a new rule, it converts Constraints to RuleConstraints. type Constraints struct { - PathPattern *patterns.PathPattern `json:"path-pattern,omitempty"` - Permissions []string `json:"permissions,omitempty"` + PathPattern *patterns.PathPattern `json:"path-pattern"` + Permissions PermissionMap `json:"permissions"` } -// ValidateForInterface returns nil if the constraints are valid for the given -// interface, otherwise returns an error. -func (c *Constraints) ValidateForInterface(iface string) error { +// ToRuleConstraints validates the receiving Constraints and converts it to +// RuleConstraints. If the constraints are not valid with respect to the given +// interface, returns an error. +func (c *Constraints) ToRuleConstraints(iface string, currTime time.Time) (*RuleConstraints, error) { + if c.PathPattern == nil { + return nil, prompting_errors.NewInvalidPathPatternError("", "no path pattern") + } + rulePermissions, err := c.Permissions.ToRulePermissionMap(iface, currTime) + if err != nil { + return nil, err + } + ruleConstraints := &RuleConstraints{ + PathPattern: c.PathPattern, + Permissions: rulePermissions, + } + return ruleConstraints, nil +} + +// RuleConstraints hold information about the applicability of an existing rule +// to particular paths or permissions. A request will be matched by the rule +// constraints if the requested path is matched by the path pattern (according +// to bash's globstar matching) and one or more requested permissions are denied +// in the permission map, or all of the requested permissions are allowed in the +// map. +type RuleConstraints struct { + PathPattern *patterns.PathPattern `json:"path-pattern"` + Permissions RulePermissionMap `json:"permissions"` +} + +// ValidateForInterface checks that the rule constraints are valid for the given +// interface. Any permissions which have expired relative to the given current +// time are pruned. If all permissions have expired, then returns true. If the +// rule is invalid, returns an error. +func (c *RuleConstraints) ValidateForInterface(iface string, currTime time.Time) (expired bool, err error) { + if c.PathPattern == nil { + return false, prompting_errors.NewInvalidPathPatternError("", "no path pattern") + } + return c.Permissions.validateForInterface(iface, currTime) +} + +// Match returns true if the constraints match the given path, otherwise false. +// +// If the constraints or path are invalid, returns an error. +func (c *RuleConstraints) Match(path string) (bool, error) { if c.PathPattern == nil { - return prompting_errors.NewInvalidPathPatternError("", "no path pattern") + return false, prompting_errors.NewInvalidPathPatternError("", "no path pattern") } - return c.validatePermissions(iface) + match, err := c.PathPattern.Match(path) + if err != nil { + // Error should not occur, since it was parsed internally + return false, prompting_errors.NewInvalidPathPatternError(c.PathPattern.String(), err.Error()) + } + return match, nil } -// validatePermissions checks that the permissions for the given constraints -// are valid for the given interface. If not, returns an error, otherwise -// ensures that the permissions are in the order in which they occur in the -// list of available permissions for that interface. -func (c *Constraints) validatePermissions(iface string) error { +// ReplyConstraints hold information about the applicability of a reply to +// particular paths or permissions. Upon receiving the reply, snapd converts +// ReplyConstraints to Constraints. +type ReplyConstraints struct { + PathPattern *patterns.PathPattern `json:"path-pattern"` + Permissions []string `json:"permissions"` +} + +// ToConstraints validates the receiving ReplyConstraints with respect to the +// given interface, along with the given outcome, lifespan, and duration, and +// constructs an equivalent Constraints from the ReplyConstraints. +func (c *ReplyConstraints) ToConstraints(iface string, outcome OutcomeType, lifespan LifespanType, duration string) (*Constraints, error) { + if _, err := outcome.AsBool(); err != nil { + // Should not occur, as outcome is validated when unmarshalled + return nil, err + } + if _, err := lifespan.ParseDuration(duration, time.Now()); err != nil { + return nil, err + } + if c.PathPattern == nil { + return nil, prompting_errors.NewInvalidPathPatternError("", "no path pattern") + } availablePerms, ok := interfacePermissionsAvailable[iface] if !ok { - return prompting_errors.NewInvalidInterfaceError(iface, availableInterfaces()) + return nil, prompting_errors.NewInvalidInterfaceError(iface, availableInterfaces()) + } + if len(c.Permissions) == 0 { + return nil, prompting_errors.NewPermissionsListEmptyError(iface, availablePerms) } - permsSet := make(map[string]bool, len(c.Permissions)) var invalidPerms []string + permissionMap := make(PermissionMap, len(c.Permissions)) for _, perm := range c.Permissions { if !strutil.ListContains(availablePerms, perm) { invalidPerms = append(invalidPerms, perm) continue } - permsSet[perm] = true + permissionMap[perm] = &PermissionEntry{ + Outcome: outcome, + Lifespan: lifespan, + Duration: duration, + } } if len(invalidPerms) > 0 { - return prompting_errors.NewInvalidPermissionsError(iface, invalidPerms, availablePerms) - } - if len(permsSet) == 0 { - return prompting_errors.NewPermissionsListEmptyError(iface, availablePerms) + return nil, prompting_errors.NewInvalidPermissionsError(iface, invalidPerms, availablePerms) } - newPermissions := make([]string, 0, len(permsSet)) - for _, perm := range availablePerms { - if exists := permsSet[perm]; exists { - newPermissions = append(newPermissions, perm) - } + constraints := &Constraints{ + PathPattern: c.PathPattern, + Permissions: permissionMap, } - c.Permissions = newPermissions - return nil + return constraints, nil } // Match returns true if the constraints match the given path, otherwise false. // // If the constraints or path are invalid, returns an error. -func (c *Constraints) Match(path string) (bool, error) { +func (c *ReplyConstraints) Match(path string) (bool, error) { if c.PathPattern == nil { return false, prompting_errors.NewInvalidPathPatternError("", "no path pattern") } @@ -97,9 +164,9 @@ func (c *Constraints) Match(path string) (bool, error) { return match, nil } -// ContainPermissions returns true if the constraints include every one of the -// given permissions. -func (c *Constraints) ContainPermissions(permissions []string) bool { +// ContainPermissions returns true if the permission list in the constraints +// includes every one of the given permissions. +func (c *ReplyConstraints) ContainPermissions(permissions []string) bool { for _, perm := range permissions { if !strutil.ListContains(c.Permissions, perm) { return false @@ -108,6 +175,277 @@ func (c *Constraints) ContainPermissions(permissions []string) bool { return true } +// PatchConstraints hold information about the applicability of the modified +// rule to particular paths or permissions. A request will be matched by the +// constraints if the requested path is matched by the path pattern (according +// to bash's globstar matching) and one or more requested permissions are +// denied in the permission map, or all of the requested permissions are +// allowed in the map. When snapd modifies the rule, it converts +// PatchConstraints to RuleConstraints, using the rule's existing constraints +// wherever a field is omitted. +// +// Any permissions which are omitted from the new permission map are left +// unchanged from the existing rule. To remove an existing permission from the +// rule, the permission should map to null. +type PatchConstraints struct { + PathPattern *patterns.PathPattern `json:"path-pattern,omitempty"` + Permissions PermissionMap `json:"permissions,omitempty"` +} + +// PatchRuleConstraints validates the receiving PatchConstraints and uses the +// existing rule constraints to construct a new RuleConstraints. +// +// If the path pattern or permissions fields are omitted, they are left +// unchanged from the existing rule. If the permissions field is present in +// the patch constraints, then any permissions which are omitted from the +// patch constrants' permission map are left unchanged from the existing rule. +// To remove an an existing permission from the rule, the permission should map +// to null in the permission map of the patch constraints. +// +// The existing rule constraints should never be modified. +func (c *PatchConstraints) PatchRuleConstraints(existing *RuleConstraints, iface string, currTime time.Time) (*RuleConstraints, error) { + ruleConstraints := &RuleConstraints{ + PathPattern: c.PathPattern, + } + if c.PathPattern == nil { + ruleConstraints.PathPattern = existing.PathPattern + } + if c.Permissions == nil { + ruleConstraints.Permissions = existing.Permissions + return ruleConstraints, nil + } + // Permissions are specified in the patch constraints, need to merge them + newPermissions := make(RulePermissionMap, len(c.Permissions)+len(existing.Permissions)) + for perm, entry := range existing.Permissions { + if !entry.Expired(currTime) { + newPermissions[perm] = entry + } + } + availablePerms, ok := interfacePermissionsAvailable[iface] + if !ok { + // Should not occur, as we should use the interface from the existing rule + return nil, prompting_errors.NewInvalidInterfaceError(iface, availableInterfaces()) + } + var errs []error + var invalidPerms []string + for perm, entry := range c.Permissions { + if entry == nil { + delete(newPermissions, perm) + continue + } + if !strutil.ListContains(availablePerms, perm) { + invalidPerms = append(invalidPerms, perm) + continue + } + ruleEntry, err := entry.toRulePermissionEntry(currTime) + if err != nil { + errs = append(errs, err) + continue + } + newPermissions[perm] = ruleEntry + } + if len(invalidPerms) > 0 { + errs = append(errs, prompting_errors.NewInvalidPermissionsError(iface, invalidPerms, availablePerms)) + } + if len(errs) > 0 { + return nil, prompting_errors.Join(errs...) + } + ruleConstraints.Permissions = newPermissions + return ruleConstraints, nil +} + +// PermissionMap is a map from permissions to their corresponding entries, +// which contain information about the outcome and lifespan for those +// permissions. +type PermissionMap map[string]*PermissionEntry + +// ToRulePermissionMap validates the receiving PermissionMap and converts it +// to a RulePermissionMap, using the given current time to convert any included +// durations to expirations. If the permission map is not valid with respect to +// the given interface, returns an error. +func (pm PermissionMap) ToRulePermissionMap(iface string, currTime time.Time) (RulePermissionMap, error) { + availablePerms, ok := interfacePermissionsAvailable[iface] + if !ok { + return nil, prompting_errors.NewInvalidInterfaceError(iface, availableInterfaces()) + } + if len(pm) == 0 { + return nil, prompting_errors.NewPermissionsListEmptyError(iface, availablePerms) + } + var errs []error + var invalidPerms []string + rulePermissionMap := make(RulePermissionMap, len(pm)) + for perm, entry := range pm { + if !strutil.ListContains(availablePerms, perm) { + invalidPerms = append(invalidPerms, perm) + continue + } + rulePermissionEntry, err := entry.toRulePermissionEntry(currTime) + if err != nil { + errs = append(errs, err) + continue + } + rulePermissionMap[perm] = rulePermissionEntry + } + if len(invalidPerms) > 0 { + errs = append(errs, prompting_errors.NewInvalidPermissionsError(iface, invalidPerms, availablePerms)) + } + if len(errs) > 0 { + return nil, prompting_errors.Join(errs...) + } + return rulePermissionMap, nil +} + +// RulePermissionMap is a map from permissions to their corresponding entries, +// which contain information about the outcome and lifespan for those +// permissions. +type RulePermissionMap map[string]*RulePermissionEntry + +// validateForInterface checks that the rule permission map is valid for the +// given interface. Any permissions which have expired relative to the given +// current time are pruned. If all permissions have expired, then returns true. +// If the permission map is invalid, returns an error. +func (pm RulePermissionMap) validateForInterface(iface string, currTime time.Time) (expired bool, err error) { + availablePerms, ok := interfacePermissionsAvailable[iface] + if !ok { + return false, prompting_errors.NewInvalidInterfaceError(iface, availableInterfaces()) + } + if len(pm) == 0 { + return false, prompting_errors.NewPermissionsListEmptyError(iface, availablePerms) + } + var errs []error + var invalidPerms []string + var expiredPerms []string + for perm, entry := range pm { + if !strutil.ListContains(availablePerms, perm) { + invalidPerms = append(invalidPerms, perm) + continue + } + if entry.Expired(currTime) { + expiredPerms = append(expiredPerms, perm) + continue + } + if err := entry.validate(); err != nil { + errs = append(errs, err) + } + } + if len(invalidPerms) > 0 { + errs = append(errs, prompting_errors.NewInvalidPermissionsError(iface, invalidPerms, availablePerms)) + } + if len(errs) > 0 { + return false, prompting_errors.Join(errs...) + } + for _, perm := range expiredPerms { + delete(pm, perm) + } + if len(pm) == 0 { + // All permissions expired + return true, nil + } + return false, nil +} + +// Expired returns true if all permissions in the map have expired. +func (pm RulePermissionMap) Expired(currTime time.Time) bool { + for _, entry := range pm { + if !entry.Expired(currTime) { + return false + } + } + return true +} + +// PermissionEntry holds the outcome associated with a particular permission +// and the lifespan for which that outcome is applicable. +// +// PermissionEntry is used when replying to a prompt, creating a new rule, or +// modifying an existing rule. +type PermissionEntry struct { + Outcome OutcomeType `json:"outcome"` + Lifespan LifespanType `json:"lifespan"` + Duration string `json:"duration,omitempty"` +} + +// toRulePermissionEntry validates the receiving PermissionEntry and converts +// it to a RulePermissionEntry. +// +// Checks that the entry has a valid outcome, and that its lifespan is valid +// for a rule (i.e. not LifespanSingle), and that it has an appropriate +// duration for that lifespan. The duration, combined with the given current +// time, is used to compute an expiration time, and that is returned as part +// of the corresponding RulePermissionEntry. +func (e *PermissionEntry) toRulePermissionEntry(currTime time.Time) (*RulePermissionEntry, error) { + if _, err := e.Outcome.AsBool(); err != nil { + return nil, err + } + if e.Lifespan == LifespanSingle { + // We don't allow rules with lifespan "single" + return nil, prompting_errors.NewRuleLifespanSingleError(SupportedRuleLifespans) + } + expiration, err := e.Lifespan.ParseDuration(e.Duration, currTime) + if err != nil { + return nil, err + } + rulePermissionEntry := &RulePermissionEntry{ + Outcome: e.Outcome, + Lifespan: e.Lifespan, + Expiration: expiration, + } + return rulePermissionEntry, nil +} + +// RulePermissionEntry holds the outcome associated with a particular permission +// and the lifespan for which that outcome is applicable. +// +// RulePermissionEntry is derived from a PermissionEntry after it has been used +// along with the rule's timestamp to define the expiration timeouts for any +// permissions which have a lifespan of "timespan". RulePermissionEntry is what +// is returned when retrieving rule contents, but PermissionEntry is used when +// replying to prompts, creating new rules, or modifying existing rules. +type RulePermissionEntry struct { + Outcome OutcomeType `json:"outcome"` + Lifespan LifespanType `json:"lifespan"` + Expiration time.Time `json:"expiration,omitempty"` +} + +// Expired returns true if the receiving permission entry has expired and +// should no longer be considered when matching requests. +// +// This is the case if the permission has a lifespan of timespan and the +// current time is after its expiration time. +func (e *RulePermissionEntry) Expired(currTime time.Time) bool { + switch e.Lifespan { + case LifespanTimespan: + if currTime.After(e.Expiration) { + return true + } + // TODO: add lifespan session + //case LifespanSession: + // TODO: return true if the user session has changed + } + return false +} + +// validate checks that the entry has a valid outcome, and that its lifespan +// is valid for a rule (i.e. not LifespanSingle), and has an appropriate +// expiration for that lifespan. +func (e *RulePermissionEntry) validate() error { + if _, err := e.Outcome.AsBool(); err != nil { + return err + } + if e.Lifespan == LifespanSingle { + // We don't allow rules with lifespan "single" + return prompting_errors.NewRuleLifespanSingleError(SupportedRuleLifespans) + } + if err := e.Lifespan.ValidateExpiration(e.Expiration); err != nil { + // Should never error due to an API request, since rules are always + // added via the API using duration, rather than expiration. + // Error may occur when validating a rule loaded from disk. + // We don't check expiration as part of validation. + return err + } + return nil +} + var ( // List of permissions available for each interface. This also defines the // order in which the permissions should be presented. diff --git a/interfaces/prompting/constraints_test.go b/interfaces/prompting/constraints_test.go index f2f80ca6d82..9bedf74a50e 100644 --- a/interfaces/prompting/constraints_test.go +++ b/interfaces/prompting/constraints_test.go @@ -33,9 +33,15 @@ type constraintsSuite struct{} var _ = Suite(&constraintsSuite{}) -func (s *constraintsSuite) TestConstraintsValidateForInterface(c *C) { - validPathPattern, err := patterns.ParsePathPattern("/path/to/foo") +func mustParsePathPattern(c *C, patternStr string) *patterns.PathPattern { + pattern, err := patterns.ParsePathPattern(patternStr) c.Assert(err, IsNil) + return pattern +} + +/* +func (s *constraintsSuite) TestConstraintsValidateForInterface(c *C) { + validPathPattern := mustParsePathPattern(c, "/path/to/foo") // Happy constraints := &prompting.Constraints{ @@ -151,6 +157,7 @@ func (s *constraintsSuite) TestValidatePermissionsUnhappy(c *C) { c.Check(err, ErrorMatches, testCase.errStr, Commentf("testCase: %+v", testCase)) } } +*/ func (*constraintsSuite) TestConstraintsMatch(c *C) { cases := []struct { @@ -170,13 +177,19 @@ func (*constraintsSuite) TestConstraintsMatch(c *C) { }, } for _, testCase := range cases { - pattern, err := patterns.ParsePathPattern(testCase.pattern) - c.Check(err, IsNil) - constraints := &prompting.Constraints{ + pattern := mustParsePathPattern(c, testCase.pattern) + + ruleConstraints := &prompting.RuleConstraints{ PathPattern: pattern, - Permissions: []string{"read"}, } - result, err := constraints.Match(testCase.path) + result, err := ruleConstraints.Match(testCase.path) + c.Check(err, IsNil, Commentf("test case: %+v", testCase)) + c.Check(result, Equals, testCase.matches, Commentf("test case: %+v", testCase)) + + replyConstraints := &prompting.ReplyConstraints{ + PathPattern: pattern, + } + result, err = replyConstraints.Match(testCase.path) c.Check(err, IsNil, Commentf("test case: %+v", testCase)) c.Check(result, Equals, testCase.matches, Commentf("test case: %+v", testCase)) } @@ -184,14 +197,23 @@ func (*constraintsSuite) TestConstraintsMatch(c *C) { func (s *constraintsSuite) TestConstraintsMatchUnhappy(c *C) { badPath := `bad\path\` - badConstraints := &prompting.Constraints{ - Permissions: []string{"read"}, + + badRuleConstraints := &prompting.RuleConstraints{ + PathPattern: nil, + } + matches, err := badRuleConstraints.Match(badPath) + c.Check(err, ErrorMatches, `invalid path pattern: no path pattern: ""`) + c.Check(matches, Equals, false) + + badReplyConstraints := &prompting.ReplyConstraints{ + PathPattern: nil, } - matches, err := badConstraints.Match(badPath) + matches, err = badReplyConstraints.Match(badPath) c.Check(err, ErrorMatches, `invalid path pattern: no path pattern: ""`) c.Check(matches, Equals, false) } +/* func (s *constraintsSuite) TestConstraintsContainPermissions(c *C) { cases := []struct { constPerms []string @@ -240,8 +262,7 @@ func (s *constraintsSuite) TestConstraintsContainPermissions(c *C) { }, } for _, testCase := range cases { - pathPattern, err := patterns.ParsePathPattern("/arbitrary") - c.Check(err, IsNil) + pathPattern := mustParsePathPattern(c, "/arbitrary") constraints := &prompting.Constraints{ PathPattern: pathPattern, Permissions: testCase.constPerms, @@ -250,6 +271,7 @@ func (s *constraintsSuite) TestConstraintsContainPermissions(c *C) { c.Check(contained, Equals, testCase.contained, Commentf("testCase: %+v", testCase)) } } +*/ func constructPermissionsMaps() []map[string]map[string]any { var permissionsMaps []map[string]map[string]any diff --git a/interfaces/prompting/errors/errors.go b/interfaces/prompting/errors/errors.go index e2d80015258..2a999c893b7 100644 --- a/interfaces/prompting/errors/errors.go +++ b/interfaces/prompting/errors/errors.go @@ -51,9 +51,8 @@ var ( ErrRuleDBInconsistent = errors.New("internal error: interfaces requests rule database left inconsistent") // Errors which are used internally and should never be returned over the API - ErrNoMatchingRule = errors.New("no rule matches the given path") - ErrInvalidID = errors.New("invalid ID: format must be parsable as uint64") - ErrRuleExpirationInThePast = errors.New("cannot have expiration time in the past") + ErrNoMatchingRule = errors.New("no rule matches the given path") + ErrInvalidID = errors.New("invalid ID: format must be parsable as uint64") ) // Marker for UnsupportedValueError, should never be returned as an actual @@ -125,6 +124,9 @@ func NewInvalidPermissionsError(iface string, unsupported []string, supported [] } func NewPermissionsListEmptyError(iface string, supported []string) *UnsupportedValueError { + // TODO: change language to "permissions empty" rather than "permissions list empty", + // since permissions now come as a list in prompt replies but as a map when creating + // or modifying rules directly. return &UnsupportedValueError{ Field: "permissions", Msg: fmt.Sprintf("invalid permissions for %s interface: permissions list empty", iface), @@ -229,3 +231,28 @@ func (e *RuleConflictError) Error() string { func (e *RuleConflictError) Unwrap() error { return ErrRuleConflict } + +// Join returns an error that wraps the given errors. +// Any nil error values are discarded. +// Join returns nil if every value in errs is nil. +// +// TODO: replace with errors.Join() once we're on golang v1.20+ +// +// This is a lossy implementation of errors.Join, where only the first non-nil +// error is preserved in a state which can be unwrapped. +func Join(errs ...error) error { + var nonNilErrs []error + for _, e := range errs { + if e != nil { + nonNilErrs = append(nonNilErrs, e) + } + } + if len(nonNilErrs) == 0 { + return nil + } + err := nonNilErrs[0] + for _, e := range nonNilErrs[1:] { + err = fmt.Errorf("%w\n%v", err, e) + } + return err +} diff --git a/interfaces/prompting/export_test.go b/interfaces/prompting/export_test.go index 943d068930a..6a6ffa98f12 100644 --- a/interfaces/prompting/export_test.go +++ b/interfaces/prompting/export_test.go @@ -23,7 +23,3 @@ var ( InterfacePermissionsAvailable = interfacePermissionsAvailable InterfaceFilePermissionsMaps = interfaceFilePermissionsMaps ) - -func (c *Constraints) ValidatePermissions(iface string) error { - return c.validatePermissions(iface) -} diff --git a/interfaces/prompting/prompting.go b/interfaces/prompting/prompting.go index eafabbdecc5..3049a00e5e5 100644 --- a/interfaces/prompting/prompting.go +++ b/interfaces/prompting/prompting.go @@ -159,10 +159,9 @@ func (lifespan *LifespanType) UnmarshalJSON(data []byte) error { // ValidateExpiration checks that the given expiration is valid for the // receiver lifespan. // -// If the lifespan is LifespanTimespan, then expiration must be non-zero and be -// after the given currTime. Otherwise, it must be zero. Returns an error if -// any of the above are invalid. -func (lifespan LifespanType) ValidateExpiration(expiration time.Time, currTime time.Time) error { +// If the lifespan is LifespanTimespan, then expiration must be non-zero. +// Otherwise, it must be zero. Returns an error if any of the above are invalid. +func (lifespan LifespanType) ValidateExpiration(expiration time.Time) error { switch lifespan { case LifespanForever, LifespanSingle: if !expiration.IsZero() { @@ -172,9 +171,6 @@ func (lifespan LifespanType) ValidateExpiration(expiration time.Time, currTime t if expiration.IsZero() { return prompting_errors.NewInvalidExpirationError(expiration, fmt.Sprintf("cannot have unspecified expiration when lifespan is %q", lifespan)) } - if currTime.After(expiration) { - return fmt.Errorf("%w: %q", prompting_errors.ErrRuleExpirationInThePast, expiration) - } default: // Should not occur, since lifespan is validated when unmarshalled return prompting_errors.NewInvalidLifespanError(string(lifespan), supportedLifespans) diff --git a/interfaces/prompting/prompting_test.go b/interfaces/prompting/prompting_test.go index edcf8b68ddf..bcb27b8d960 100644 --- a/interfaces/prompting/prompting_test.go +++ b/interfaces/prompting/prompting_test.go @@ -169,21 +169,18 @@ func (s *promptingSuite) TestValidateExpiration(c *C) { prompting.LifespanForever, prompting.LifespanSingle, } { - err := lifespan.ValidateExpiration(unsetExpiration, currTime) + err := lifespan.ValidateExpiration(unsetExpiration) c.Check(err, IsNil) for _, exp := range []time.Time{negativeExpiration, validExpiration} { - err = lifespan.ValidateExpiration(exp, currTime) + err = lifespan.ValidateExpiration(exp) c.Check(err, ErrorMatches, `invalid expiration: cannot have specified expiration when lifespan is.*`) } } - err := prompting.LifespanTimespan.ValidateExpiration(unsetExpiration, currTime) + err := prompting.LifespanTimespan.ValidateExpiration(unsetExpiration) c.Check(err, ErrorMatches, `invalid expiration: cannot have unspecified expiration when lifespan is.*`) - err = prompting.LifespanTimespan.ValidateExpiration(negativeExpiration, currTime) - c.Check(err, ErrorMatches, `cannot have expiration time in the past.*`) - - err = prompting.LifespanTimespan.ValidateExpiration(validExpiration, currTime) + err = prompting.LifespanTimespan.ValidateExpiration(validExpiration) c.Check(err, IsNil) } diff --git a/interfaces/prompting/requestprompts/requestprompts.go b/interfaces/prompting/requestprompts/requestprompts.go index 5d8a9a050af..bbbc6a8a906 100644 --- a/interfaces/prompting/requestprompts/requestprompts.go +++ b/interfaces/prompting/requestprompts/requestprompts.go @@ -126,20 +126,63 @@ func (pc *promptConstraints) equals(other *promptConstraints) bool { return true } -// subtractPermissions removes all of the given permissions from the list of -// permissions in the constraints. -func (pc *promptConstraints) subtractPermissions(permissions []string) (modified bool) { - newPermissions := make([]string, 0, len(pc.remainingPermissions)) +// applyRuleConstraints modifies the prompt constraints, removing any remaining +// permissions which are matched by the given rule constraints. +// +// If the path pattern does not match the prompt path, or the rule constraints +// do not include any of the remaining prompt permissions, then returns false, +// and no changes are made. If any permissions are denied by the rule +// constraints, then those permissions are explicitly returned as denied +// permissions. +// +// Assumes the outcomes in the permission entries in the rule constraints have +// already been validated. +func (pc *promptConstraints) applyRuleConstraints(constraints *prompting.RuleConstraints) (matched, satisfied bool, denied []string, err error) { + pathMatched, err := constraints.Match(pc.path) + if err != nil { + // Should not occur, only error is if path pattern is malformed, + // which would have thrown an error while parsing, not now. + return false, false, nil, err + } + if !pathMatched { + return false, false, nil, nil + } + + // Path pattern matched, now check if any permissions match + + var permissionMatched bool + newRemainingPermissions := make([]string, 0, len(pc.remainingPermissions)) for _, perm := range pc.remainingPermissions { - if !strutil.ListContains(permissions, perm) { - newPermissions = append(newPermissions, perm) + entry, exists := constraints.Permissions[perm] + if !exists { + // Permission not covered by rule constraints, so permission + // should continue to be in remainingPermissions. + newRemainingPermissions = append(newRemainingPermissions, perm) + continue + } + permissionMatched = true + allow, err := entry.Outcome.AsBool() + if err != nil { + // This should not occur, as rule constraints are built internally + return false, false, nil, err + } + if !allow { + denied = append(denied, perm) } } - if len(newPermissions) != len(pc.remainingPermissions) { - pc.remainingPermissions = newPermissions - return true + if !permissionMatched { + // No permissions matched, so nothing changes + return false, false, nil, nil } - return false + + pc.remainingPermissions = newRemainingPermissions + + if len(pc.remainingPermissions) == 0 { + // All permissions satisfied + return true, true, denied, nil + } + + return true, false, denied, nil } // Path returns the path associated with the request to which the receiving @@ -310,7 +353,8 @@ func (pdb *PromptDB) AddOrMerge(metadata *prompting.Metadata, path string, reque if len(userEntry.prompts) >= maxOutstandingPromptsPerUser { logger.Noticef("WARNING: too many outstanding prompts for user %d; auto-denying new one", metadata.User) - allowedPermission := responseForInterfaceConstraintsOutcome(metadata.Interface, constraints, prompting.OutcomeDeny) + // Deny all permissions which are not already allowed by existing rules + allowedPermission := buildResponse(metadata.Interface, constraints, constraints.remainingPermissions) sendReply(listenerReq, allowedPermission) return nil, false, prompting_errors.ErrTooManyPrompts } @@ -330,21 +374,18 @@ func (pdb *PromptDB) AddOrMerge(metadata *prompting.Metadata, path string, reque return prompt, false, nil } -func responseForInterfaceConstraintsOutcome(iface string, constraints *promptConstraints, outcome prompting.OutcomeType) any { - allow, err := outcome.AsBool() - if err != nil { - // This should not occur, but if so, default to deny - allow = false - logger.Debugf("%v", err) - } +// buildResponse creates a listener response based on the given interface, +// prompt constraints, and list of denied permissions. +// +// The response is the AppArmor permission which should be allowed. This +// corresponds to the originally requested permissions from the prompt +// constraints, except with all denied permissions removed. +func buildResponse(iface string, constraints *promptConstraints, denied []string) any { allowedPerms := constraints.originalPermissions - if !allow { - // Remaining permissions were denied, so allow permissions which were - // previously allowed by prompt rules - allowedPerms = make([]string, 0, len(constraints.originalPermissions)-len(constraints.remainingPermissions)) + if len(denied) > 0 { + allowedPerms = make([]string, 0, len(constraints.originalPermissions)-len(denied)) for _, perm := range constraints.originalPermissions { - // Exclude any permissions which were remaining at time of denial - if !strutil.ListContains(constraints.remainingPermissions, perm) { + if !strutil.ListContains(denied, perm) { allowedPerms = append(allowedPerms, perm) } } @@ -408,13 +449,22 @@ func (pdb *PromptDB) promptWithID(user uint32, id prompting.IDType) (*userPrompt // // Records a notice for the prompt, and returns the prompt's former contents. func (pdb *PromptDB) Reply(user uint32, id prompting.IDType, outcome prompting.OutcomeType) (*Prompt, error) { + allow, err := outcome.AsBool() + if err != nil { + // This should not occur + return nil, err + } pdb.mutex.Lock() defer pdb.mutex.Unlock() userEntry, prompt, err := pdb.promptWithID(user, id) if err != nil { return nil, err } - allowedPermission := responseForInterfaceConstraintsOutcome(prompt.Interface, prompt.Constraints, outcome) + var denied []string + if !allow { + denied = prompt.Constraints.remainingPermissions + } + allowedPermission := buildResponse(prompt.Interface, prompt.Constraints, denied) for _, listenerReq := range prompt.listenerReqs { if err := sendReply(listenerReq, allowedPermission); err != nil { // Error should only occur if reply is malformed, and since these @@ -438,10 +488,10 @@ var sendReply = func(listenerReq *listener.Request, allowedPermission any) error // contents and, if so, sends back a decision to their listener requests. // // A prompt is satisfied by the given rule contents if the user, snap, -// interface, and path of the prompt match those of the rule, and if either the -// outcome is "allow" and all of the prompt's permissions are matched by those -// of the rule contents, or if the outcome is "deny" and any of the permissions -// match. +// interface, and path of the prompt match those of the rule, and all remaining +// permissions are covered by permissions in the rule constraints or at least +// one of the remaining permissions is covered by a permission which has an +// outcome of "deny". // // Records a notice for any prompt which was satisfied, or which had some of // its permissions satisfied by the rule contents. In the future, only the @@ -450,12 +500,11 @@ var sendReply = func(listenerReq *listener.Request, allowedPermission any) error // // Returns the IDs of any prompts which were fully satisfied by the given rule // contents. -func (pdb *PromptDB) HandleNewRule(metadata *prompting.Metadata, constraints *prompting.Constraints, outcome prompting.OutcomeType) ([]prompting.IDType, error) { - // Validate outcome before locking - allow, err := outcome.AsBool() - if err != nil { - return nil, err - } +// +// Since rule is new, we don't check the expiration timestamps for any +// permissions, since any permissions with lifespan timespan were validated to +// have a non-zero duration, and we handle this rule as it was at its creation. +func (pdb *PromptDB) HandleNewRule(metadata *prompting.Metadata, constraints *prompting.RuleConstraints) ([]prompting.IDType, error) { pdb.mutex.Lock() defer pdb.mutex.Unlock() @@ -472,39 +521,43 @@ func (pdb *PromptDB) HandleNewRule(metadata *prompting.Metadata, constraints *pr if !(prompt.Snap == metadata.Snap && prompt.Interface == metadata.Interface) { continue } - matched, err := constraints.Match(prompt.Constraints.path) + + matched, satisfied, denied, err := prompt.Constraints.applyRuleConstraints(constraints) if err != nil { + // Should not occur, only error is if path pattern is malformed, + // which would have thrown an error while parsing, not now. return nil, err } if !matched { continue } - - // Record all allowed permissions at the time of match, in case a - // permission is denied and we need to send back a response. - allowedPermission := responseForInterfaceConstraintsOutcome(metadata.Interface, prompt.Constraints, outcome) - - // See if the permission matches any of the prompt's remaining permissions - modified := prompt.Constraints.subtractPermissions(constraints.Permissions) - if !modified { - // No permission was matched + // Matched, so at least one permission was satisfied + if !satisfied && len(denied) == 0 { + // Not all permissions were satisfied, and no permissions were + // denied, so re-record a notice and move on. + pdb.notifyPrompt(metadata.User, prompt.ID, nil) continue } - id := prompt.ID - if len(prompt.Constraints.remainingPermissions) > 0 && allow == true { - pdb.notifyPrompt(metadata.User, id, nil) - continue + + // Either all permissions satisfied or at least one permission denied. + // Construct a response and send it back to the kernel, and record a + // notice that the prompt was satisfied. + if len(denied) > 0 { + // At least one permission denied by new rule, and want to send a + // response immediately, so include any remaining permissions as + // denied as well. + denied = append(denied, prompt.Constraints.remainingPermissions...) } - // All permissions of prompt satisfied, or any permission denied + allowedPermission := buildResponse(metadata.Interface, prompt.Constraints, denied) for _, listenerReq := range prompt.listenerReqs { // Reply with any permissions which were allowed, either by this // new rule or by previous rules. sendReply(listenerReq, allowedPermission) } - userEntry.remove(id) - satisfiedPromptIDs = append(satisfiedPromptIDs, id) + userEntry.remove(prompt.ID) + satisfiedPromptIDs = append(satisfiedPromptIDs, prompt.ID) data := map[string]string{"resolved": "satisfied"} - pdb.notifyPrompt(metadata.User, id, data) + pdb.notifyPrompt(metadata.User, prompt.ID, data) } return satisfiedPromptIDs, nil } diff --git a/interfaces/prompting/requestprompts/requestprompts_test.go b/interfaces/prompting/requestprompts/requestprompts_test.go index 84718555a93..a1dc851b952 100644 --- a/interfaces/prompting/requestprompts/requestprompts_test.go +++ b/interfaces/prompting/requestprompts/requestprompts_test.go @@ -20,6 +20,7 @@ package requestprompts_test import ( + "bytes" "encoding/json" "fmt" "os" @@ -347,7 +348,19 @@ func applyNotices(expectedPromptIDs []prompting.IDType, expectedData map[string] } func (s *requestpromptsSuite) checkNewNotices(c *C, expectedNotices []*noticeInfo) { - c.Check(s.promptNotices, DeepEquals, expectedNotices) + c.Check(s.promptNotices, DeepEquals, expectedNotices, Commentf("%s", func() string { + var buf bytes.Buffer + buf.WriteString("\nobtained: [\n") + for _, n := range s.promptNotices { + buf.WriteString(fmt.Sprintf(" %+v\n", n)) + } + buf.WriteString("]\nexpected: [\n") + for _, n := range expectedNotices { + buf.WriteString(fmt.Sprintf(" %+v\n", n)) + } + buf.WriteString("]\n") + return buf.String() + }())) s.promptNotices = s.promptNotices[:0] } @@ -659,14 +672,16 @@ func (s *requestpromptsSuite) TestHandleNewRuleAllowPermissions(c *C) { pathPattern, err := patterns.ParsePathPattern("/home/test/Documents/**") c.Assert(err, IsNil) - permissions := []string{"read", "write", "append"} - constraints := &prompting.Constraints{ + constraints := &prompting.RuleConstraints{ PathPattern: pathPattern, - Permissions: permissions, + Permissions: prompting.RulePermissionMap{ + "read": &prompting.RulePermissionEntry{Outcome: prompting.OutcomeAllow}, + "write": &prompting.RulePermissionEntry{Outcome: prompting.OutcomeAllow}, + "append": &prompting.RulePermissionEntry{Outcome: prompting.OutcomeAllow}, + }, } - outcome := prompting.OutcomeAllow - satisfied, err := pdb.HandleNewRule(metadata, constraints, outcome) + satisfied, err := pdb.HandleNewRule(metadata, constraints) c.Assert(err, IsNil) c.Check(satisfied, HasLen, 2) c.Check(promptIDListContains(satisfied, prompt2.ID), Equals, true) @@ -702,12 +717,13 @@ func (s *requestpromptsSuite) TestHandleNewRuleAllowPermissions(c *C) { c.Assert(stored, HasLen, 2) // Check that allowing the final missing permission allows the prompt. - permissions = []string{"execute"} - constraints = &prompting.Constraints{ + constraints = &prompting.RuleConstraints{ PathPattern: pathPattern, - Permissions: permissions, + Permissions: prompting.RulePermissionMap{ + "execute": &prompting.RulePermissionEntry{Outcome: prompting.OutcomeAllow}, + }, } - satisfied, err = pdb.HandleNewRule(metadata, constraints, outcome) + satisfied, err = pdb.HandleNewRule(metadata, constraints) c.Assert(err, IsNil) c.Check(satisfied, HasLen, 1) @@ -786,15 +802,15 @@ func (s *requestpromptsSuite) TestHandleNewRuleDenyPermissions(c *C) { pathPattern, err := patterns.ParsePathPattern("/home/test/Documents/**") c.Assert(err, IsNil) - permissions := []string{"read"} - constraints := &prompting.Constraints{ + constraints := &prompting.RuleConstraints{ PathPattern: pathPattern, - Permissions: permissions, + Permissions: prompting.RulePermissionMap{ + "read": &prompting.RulePermissionEntry{Outcome: prompting.OutcomeDeny}, + }, } - outcome := prompting.OutcomeDeny // If one or more permissions denied each for prompts 1-3, so each is denied - satisfied, err := pdb.HandleNewRule(metadata, constraints, outcome) + satisfied, err := pdb.HandleNewRule(metadata, constraints) c.Assert(err, IsNil) c.Check(satisfied, HasLen, 3) c.Check(promptIDListContains(satisfied, prompt1.ID), Equals, true) @@ -854,29 +870,38 @@ func (s *requestpromptsSuite) TestHandleNewRuleNonMatches(c *C) { pathPattern, err := patterns.ParsePathPattern("/home/test/Documents/**") c.Assert(err, IsNil) - constraints := &prompting.Constraints{ + constraints := &prompting.RuleConstraints{ PathPattern: pathPattern, - Permissions: permissions, + Permissions: prompting.RulePermissionMap{ + "read": &prompting.RulePermissionEntry{Outcome: prompting.OutcomeAllow}, + }, + } + + badOutcomeConstraints := &prompting.RuleConstraints{ + PathPattern: pathPattern, + Permissions: prompting.RulePermissionMap{ + "read": &prompting.RulePermissionEntry{Outcome: prompting.OutcomeType("foo")}, + }, } - outcome := prompting.OutcomeAllow otherUser := user + 1 otherSnap := "ldx" otherInterface := "system-files" otherPattern, err := patterns.ParsePathPattern("/home/test/Pictures/**.png") c.Assert(err, IsNil) - otherConstraints := &prompting.Constraints{ + otherConstraints := &prompting.RuleConstraints{ PathPattern: otherPattern, - Permissions: permissions, + Permissions: prompting.RulePermissionMap{ + "read": &prompting.RulePermissionEntry{Outcome: prompting.OutcomeAllow}, + }, } - badOutcome := prompting.OutcomeType("foo") stored, err := pdb.Prompts(metadata.User) c.Assert(err, IsNil) c.Assert(stored, HasLen, 1) c.Assert(stored[0], Equals, prompt) - satisfied, err := pdb.HandleNewRule(metadata, constraints, badOutcome) + satisfied, err := pdb.HandleNewRule(metadata, badOutcomeConstraints) c.Check(err, ErrorMatches, `invalid outcome: "foo"`) c.Check(satisfied, IsNil) @@ -887,7 +912,7 @@ func (s *requestpromptsSuite) TestHandleNewRuleNonMatches(c *C) { Snap: snap, Interface: iface, } - satisfied, err = pdb.HandleNewRule(otherUserMetadata, constraints, outcome) + satisfied, err = pdb.HandleNewRule(otherUserMetadata, constraints) c.Check(err, IsNil) c.Check(satisfied, IsNil) @@ -898,7 +923,7 @@ func (s *requestpromptsSuite) TestHandleNewRuleNonMatches(c *C) { Snap: otherSnap, Interface: iface, } - satisfied, err = pdb.HandleNewRule(otherSnapMetadata, constraints, outcome) + satisfied, err = pdb.HandleNewRule(otherSnapMetadata, constraints) c.Check(err, IsNil) c.Check(satisfied, IsNil) @@ -909,19 +934,19 @@ func (s *requestpromptsSuite) TestHandleNewRuleNonMatches(c *C) { Snap: snap, Interface: otherInterface, } - satisfied, err = pdb.HandleNewRule(otherInterfaceMetadata, constraints, outcome) + satisfied, err = pdb.HandleNewRule(otherInterfaceMetadata, constraints) c.Check(err, IsNil) c.Check(satisfied, IsNil) s.checkNewNoticesSimple(c, []prompting.IDType{}, nil) - satisfied, err = pdb.HandleNewRule(metadata, otherConstraints, outcome) + satisfied, err = pdb.HandleNewRule(metadata, otherConstraints) c.Check(err, IsNil) c.Check(satisfied, IsNil) s.checkNewNoticesSimple(c, []prompting.IDType{}, nil) - satisfied, err = pdb.HandleNewRule(metadata, constraints, outcome) + satisfied, err = pdb.HandleNewRule(metadata, constraints) c.Check(err, IsNil) c.Assert(satisfied, HasLen, 1) @@ -1026,7 +1051,7 @@ func (s *requestpromptsSuite) TestCloseThenOperate(c *C) { c.Check(err, Equals, prompting_errors.ErrPromptsClosed) c.Check(result, IsNil) - promptIDs, err := pdb.HandleNewRule(nil, nil, prompting.OutcomeDeny) + promptIDs, err := pdb.HandleNewRule(nil, nil) c.Check(err, Equals, prompting_errors.ErrPromptsClosed) c.Check(promptIDs, IsNil) diff --git a/interfaces/prompting/requestrules/export_test.go b/interfaces/prompting/requestrules/export_test.go index 04523754cdd..1f27b0803ae 100644 --- a/interfaces/prompting/requestrules/export_test.go +++ b/interfaces/prompting/requestrules/export_test.go @@ -27,6 +27,6 @@ var JoinInternalErrors = joinInternalErrors type RulesDBJSON rulesDBJSON -func (rule *Rule) Validate(currTime time.Time) error { +func (rule *Rule) Validate(currTime time.Time) (expired bool, err error) { return rule.validate(currTime) } diff --git a/interfaces/prompting/requestrules/requestrules.go b/interfaces/prompting/requestrules/requestrules.go index ccd2d78d815..debcb9a0667 100644 --- a/interfaces/prompting/requestrules/requestrules.go +++ b/interfaces/prompting/requestrules/requestrules.go @@ -41,64 +41,39 @@ import ( // Rule stores the contents of a request rule. type Rule struct { - ID prompting.IDType `json:"id"` - Timestamp time.Time `json:"timestamp"` - User uint32 `json:"user"` - Snap string `json:"snap"` - Interface string `json:"interface"` - Constraints *prompting.Constraints `json:"constraints"` - Outcome prompting.OutcomeType `json:"outcome"` - Lifespan prompting.LifespanType `json:"lifespan"` - Expiration time.Time `json:"expiration,omitempty"` + ID prompting.IDType `json:"id"` + Timestamp time.Time `json:"timestamp"` + User uint32 `json:"user"` + Snap string `json:"snap"` + Interface string `json:"interface"` + Constraints *prompting.RuleConstraints `json:"constraints"` } -// Validate verifies internal correctness of the rule -func (rule *Rule) validate(currTime time.Time) error { - if err := rule.Constraints.ValidateForInterface(rule.Interface); err != nil { - return err - } - if _, err := rule.Outcome.AsBool(); err != nil { - return err - } - if rule.Lifespan == prompting.LifespanSingle { - // We don't allow rules with lifespan "single" - return prompting_errors.NewRuleLifespanSingleError(prompting.SupportedRuleLifespans) - } - if err := rule.Lifespan.ValidateExpiration(rule.Expiration, currTime); err != nil { - // Should never error due to an API request, since rules are always - // added via the API using duration, rather than expiration. - // Error may occur when validating a rule loaded from disk. - return err - } - return nil +// Validate verifies internal correctness of the rule's constraints and +// permissions and prunes any expired permissions. If all permissions are +// expired, then returns true. If the rule is invalid, returns an error. +func (rule *Rule) validate(currTime time.Time) (expired bool, err error) { + return rule.Constraints.ValidateForInterface(rule.Interface, currTime) } -// Expired returns true if the receiving rule has a lifespan of timespan and -// the current time is after the rule's expiration time. -func (rule *Rule) Expired(currentTime time.Time) bool { - switch rule.Lifespan { - case prompting.LifespanTimespan: - if currentTime.After(rule.Expiration) { - return true - } - // TODO: add lifespan session - //case prompting.LifespanSession: - // TODO: return true if the user session has changed - } - return false +// expired returns true if all permissions for the receiving rule have expired. +func (rule *Rule) expired(currTime time.Time) bool { + return rule.Constraints.Permissions.Expired(currTime) } // 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. +// match paths, and a map from rule IDs whose path patterns render to this +// variant to the relevant permission entry from that rule. All non-expired +// permission entry values in the map must have the same outcome (as long as +// the entry has not expired), and that outcome is also stored directly in the +// variant entry itself. // // 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 - Outcome prompting.OutcomeType - RuleIDs map[prompting.IDType]bool + Variant patterns.PatternVariant + Outcome prompting.OutcomeType + RuleEntries map[prompting.IDType]*prompting.RulePermissionEntry } // permissionDB stores a map from path pattern variant to the ID of the rule @@ -140,10 +115,9 @@ type RuleDB struct { indexByID map[prompting.IDType]int rules []*Rule - // the incoming request queries are made in the context of a user, snap, - // snap interface, path, so this is essential a secondary compound index - // made of all those properties for being able to identify a rule - // matching given query + // Rules are stored in a tree according to user, snap, interface, and + // permission to simplify the process of checking whether a given request + // is matched by existing rules, and which of those rules has precedence. perUser map[uint32]*userDB dbPath string @@ -208,7 +182,7 @@ func (rdb *RuleDB) load() (retErr error) { rdb.rules = make([]*Rule, 0) rdb.perUser = make(map[uint32]*userDB) - expiredRules := make(map[*Rule]bool) + expiredRules := make(map[prompting.IDType]bool) f, err := os.Open(rdb.dbPath) if err != nil { @@ -227,27 +201,26 @@ func (rdb *RuleDB) load() (retErr error) { loadErr := fmt.Errorf("cannot read stored request rules: %w", err) // Save the empty rule DB to disk to overwrite the previous one which // could not be decoded. - return errorsJoin(loadErr, rdb.save()) + return prompting_errors.Join(loadErr, rdb.save()) } currTime := time.Now() var errInvalid error for _, rule := range wrapped.Rules { - if rule.Expired(currTime) { - expiredRules[rule] = true - continue - } - // If an expired rule happens to be invalid, it's fine, since we remove - // it anyway. - - if err := rule.validate(currTime); err != nil { + expired, err := rule.validate(currTime) + if err != nil { // we're loading previously saved rules, so this should not happen errInvalid = fmt.Errorf("internal error: %w", err) break } + if expired { + expiredRules[rule.ID] = true + continue + } - if conflictErr := rdb.addRule(rule); conflictErr != nil { + conflictErr := rdb.addRule(rule) + if conflictErr != nil { // Duplicate rules on disk or conflicting rule, should not occur errInvalid = fmt.Errorf("cannot add rule: %w", conflictErr) break @@ -266,13 +239,13 @@ func (rdb *RuleDB) load() (retErr error) { // Save the empty rule DB to disk to overwrite the previous one which // was invalid. - return errorsJoin(errInvalid, rdb.save()) + return prompting_errors.Join(errInvalid, rdb.save()) } expiredData := map[string]string{"removed": "expired"} for _, rule := range wrapped.Rules { var data map[string]string - if expiredRules[rule] { + if expiredRules[rule.ID] { data = expiredData } rdb.notifyRule(rule.User, rule.ID, data) @@ -306,6 +279,8 @@ func (rdb *RuleDB) lookupRuleByID(id prompting.IDType) (*Rule, error) { if !exists { return nil, prompting_errors.ErrRuleNotFound } + // XXX: should we check whether a rule is expired and throw ErrRuleNotFound + // if so? if index >= len(rdb.rules) { // Internal inconsistency between rules list and IDs map, should not occur return nil, prompting_errors.ErrRuleDBInconsistent @@ -331,22 +306,24 @@ func (rdb *RuleDB) addRuleToRulesList(rule *Rule) error { return nil } -// addRule adds the given rule to the rule DB. If there is a conflicting rule, -// returns an error, and the rule DB is left unchanged. +// addRule adds the given rule to the rule DB. +// +// If there is a conflicting rule, returns an error, and the rule DB is left +// unchanged. // // The caller must ensure that the database lock is held for writing. func (rdb *RuleDB) addRule(rule *Rule) error { if err := rdb.addRuleToRulesList(rule); err != nil { return err } - err := rdb.addRuleToTree(rule) - if err == nil { + conflictErr := rdb.addRuleToTree(rule) + if conflictErr == nil { return nil } // remove just-added rule from rules list and IDs rdb.rules = rdb.rules[:len(rdb.rules)-1] delete(rdb.indexByID, rule.ID) - return err + return conflictErr } // removeRuleByIDFromRulesList removes the rule with the given ID from the rules @@ -407,13 +384,16 @@ func (rdb *RuleDB) removeRuleByID(id prompting.IDType) (*Rule, error) { // If there are other rules which have a conflicting path pattern and // permission, returns an error with information about the conflicting rules. // +// Assumes that the rule has already been internally validated. No additional +// validation is done in this function, nor is the expiration of the permissions +// checked. +// // The caller must ensure that the database lock is held for writing. func (rdb *RuleDB) addRuleToTree(rule *Rule) *prompting_errors.RuleConflictError { addedPermissions := make([]string, 0, len(rule.Constraints.Permissions)) - var conflicts []prompting_errors.RuleConflict - for _, permission := range rule.Constraints.Permissions { - permConflicts := rdb.addRulePermissionToTree(rule, permission) + for permission, entry := range rule.Constraints.Permissions { + permConflicts := rdb.addRulePermissionToTree(rule, permission, entry) if len(permConflicts) > 0 { conflicts = append(conflicts, permConflicts...) continue @@ -448,18 +428,21 @@ func (rdb *RuleDB) addRuleToTree(rule *Rule) *prompting_errors.RuleConflictError // 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. +// Rules which are expired according to the timestamp of the rule being added, +// 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 from the tree entry (though not +// removed from the rule DB as a whole, nor is a notice recorded). 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 { +// The caller must ensure that the database lock is held for writing, and that +// the given entry is not expired. +func (rdb *RuleDB) addRulePermissionToTree(rule *Rule, permission string, permissionEntry *prompting.RulePermissionEntry) []prompting_errors.RuleConflict { permVariants := rdb.ensurePermissionDBForUserSnapInterfacePermission(rule.User, rule.Snap, rule.Interface, permission) newVariantEntries := make(map[string]variantEntry, rule.Constraints.PathPattern.NumVariants()) - expiredRules := make(map[prompting.IDType]bool) + partiallyExpiredRules := make(map[prompting.IDType]bool) var conflicts []prompting_errors.RuleConflict addVariant := func(index int, variant patterns.PatternVariant) { @@ -467,19 +450,19 @@ func (rdb *RuleDB) addRulePermissionToTree(rule *Rule, permission string) []prom existingEntry, exists := permVariants.VariantEntries[variantStr] if !exists { newVariantEntries[variantStr] = variantEntry{ - Variant: variant, - Outcome: rule.Outcome, - RuleIDs: map[prompting.IDType]bool{rule.ID: true}, + Variant: variant, + Outcome: permissionEntry.Outcome, + RuleEntries: map[prompting.IDType]*prompting.RulePermissionEntry{rule.ID: permissionEntry}, } return } - if existingEntry.Outcome != rule.Outcome { + if existingEntry.Outcome != permissionEntry.Outcome { // Outcomes mismatch, so if there are any unexpired rules, these // all conflict with the new rule. If all rules are expired, this // is fine, and we can replace the existing entry with a new one. - for id := range existingEntry.RuleIDs { - if rdb.isRuleWithIDExpired(id, rule.Timestamp) { - expiredRules[id] = true + for id, entry := range existingEntry.RuleEntries { + if entry.Expired(rule.Timestamp) { + partiallyExpiredRules[id] = true continue } // Conflicting non-expired rule @@ -496,24 +479,24 @@ func (rdb *RuleDB) addRulePermissionToTree(rule *Rule, permission string) []prom // variant entry return } - newEntry := variantEntry{ - Variant: variant, - Outcome: rule.Outcome, - RuleIDs: make(map[prompting.IDType]bool, len(existingEntry.RuleIDs)+1), + newVariantEntry := variantEntry{ + Variant: variant, + Outcome: permissionEntry.Outcome, + RuleEntries: make(map[prompting.IDType]*prompting.RulePermissionEntry, len(existingEntry.RuleEntries)+1), } - newEntry.RuleIDs[rule.ID] = true - newVariantEntries[variantStr] = newEntry - if existingEntry.Outcome != rule.Outcome { + newVariantEntry.RuleEntries[rule.ID] = permissionEntry + newVariantEntries[variantStr] = newVariantEntry + if existingEntry.Outcome != newVariantEntry.Outcome { // We know all existing rule IDs were expired, else there would // have been a conflict return } - for id := range existingEntry.RuleIDs { - if rdb.isRuleWithIDExpired(id, rule.Timestamp) { - expiredRules[id] = true + for id, entry := range existingEntry.RuleEntries { + if entry.Expired(rule.Timestamp) { + partiallyExpiredRules[id] = true continue } - newEntry.RuleIDs[id] = true + newVariantEntry.RuleEntries[id] = entry } } rule.Constraints.PathPattern.RenderAllVariants(addVariant) @@ -522,34 +505,33 @@ func (rdb *RuleDB) addRulePermissionToTree(rule *Rule, permission string) []prom return conflicts } - for ruleID := range expiredRules { - removedRule, err := rdb.removeRuleByID(ruleID) + expiredData := map[string]string{"removed": "expired"} + for ruleID := range partiallyExpiredRules { + maybeExpired, err := rdb.lookupRuleByIDForUser(rule.User, ruleID) + if err != nil { + // Error shouldn't occur. If it does, the rule was already removed + continue + } + // Already removed the rule's permission from the tree, let's remove + // it from the rule as well + delete(maybeExpired.Constraints.Permissions, permission) + if !maybeExpired.expired(rule.Timestamp) { + continue + } + _, err = rdb.removeRuleByID(ruleID) // Error shouldn't occur. If it does, the rule was already removed. if err == nil { - rdb.notifyRule(removedRule.User, removedRule.ID, - map[string]string{"removed": "expired"}) + rdb.notifyRule(maybeExpired.User, maybeExpired.ID, expiredData) } } - for variantStr, entry := range newVariantEntries { - permVariants.VariantEntries[variantStr] = entry + for variantStr, variantEntry := range newVariantEntries { + permVariants.VariantEntries[variantStr] = variantEntry } return nil } -// isRuleWithIDExpired returns true if the rule with given ID is expired with respect -// to the provided timestamp, or if it otherwise no longer exists. -// -// The caller must ensure that the database lock is held for writing. -func (rdb *RuleDB) isRuleWithIDExpired(id prompting.IDType, currTime time.Time) bool { - rule, err := rdb.lookupRuleByID(id) - if err != nil { - return true - } - return rule.Expired(currTime) -} - // removeRuleFromTree fully removes the given rule from the rules tree, even if // an error occurs. Whenever possible, it is preferred to use `removeRuleByID` // directly instead, since it ensures consistency between the rules list and the @@ -558,11 +540,11 @@ func (rdb *RuleDB) isRuleWithIDExpired(id prompting.IDType, currTime time.Time) // The caller must ensure that the database lock is held for writing. func (rdb *RuleDB) removeRuleFromTree(rule *Rule) error { var errs []error - for _, permission := range rule.Constraints.Permissions { - if es := rdb.removeRulePermissionFromTree(rule, permission); len(es) > 0 { + for permission := range rule.Constraints.Permissions { + if err := rdb.removeRulePermissionFromTree(rule, permission); err != nil { // Database was left inconsistent, should not occur. // Store the errors, but keep removing. - errs = append(errs, es...) + errs = append(errs, err) } } return joinInternalErrors(errs) @@ -577,14 +559,13 @@ func (rdb *RuleDB) removeRuleFromTree(rule *Rule) error { // errors which occurred. // // The caller must ensure that the database lock is held for writing. -func (rdb *RuleDB) removeRulePermissionFromTree(rule *Rule, permission string) []error { +func (rdb *RuleDB) removeRulePermissionFromTree(rule *Rule, permission string) error { permVariants, ok := rdb.permissionDBForUserSnapInterfacePermission(rule.User, rule.Snap, rule.Interface, permission) if !ok || permVariants == nil { err := fmt.Errorf("internal error: no rules in the rule tree for user %d, snap %q, interface %q, permission %q", rule.User, rule.Snap, rule.Interface, permission) - return []error{err} + return err } seenVariants := make(map[string]bool, rule.Constraints.PathPattern.NumVariants()) - var errs []error removeVariant := func(index int, variant patterns.PatternVariant) { variantStr := variant.String() if seenVariants[variantStr] { @@ -593,23 +574,25 @@ func (rdb *RuleDB) removeRulePermissionFromTree(rule *Rule, permission string) [ seenVariants[variantStr] = true variantEntry, exists := permVariants.VariantEntries[variantStr] 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)) + // If doesn't exist, could have been removed due to another rule's + // variant being removed and, finding all other rules' permissions + // for this variant expired, removing the variant entry. + return } - delete(variantEntry.RuleIDs, rule.ID) - if len(variantEntry.RuleIDs) == 0 { + delete(variantEntry.RuleEntries, rule.ID) + if len(variantEntry.RuleEntries) == 0 { delete(permVariants.VariantEntries, variantStr) } } rule.Constraints.PathPattern.RenderAllVariants(removeVariant) - return errs + return nil } // joinInternalErrors wraps a prompting_errors.ErrRuleDBInconsistent with the given errors. // // If there are no non-nil errors in the given errs list, return nil. func joinInternalErrors(errs []error) error { - joinedErr := errorsJoin(errs...) + joinedErr := prompting_errors.Join(errs...) if joinedErr == nil { return nil } @@ -617,28 +600,6 @@ func joinInternalErrors(errs []error) error { return fmt.Errorf("%w\n%v", prompting_errors.ErrRuleDBInconsistent, joinedErr) } -// errorsJoin returns an error that wraps the given errors. -// Any nil error values are discarded. -// errorsJoin returns nil if every value in errs is nil. -// -// TODO: replace with errors.Join() once we're on golang v1.20+ -func errorsJoin(errs ...error) error { - var nonNilErrs []error - for _, e := range errs { - if e != nil { - nonNilErrs = append(nonNilErrs, e) - } - } - if len(nonNilErrs) == 0 { - return nil - } - err := nonNilErrs[0] - for _, e := range nonNilErrs[1:] { - err = fmt.Errorf("%w\n%v", err, e) - } - return err -} - // permissionDBForUserSnapInterfacePermission returns the permission DB for the // given user, snap, interface, and permission, if it exists. // @@ -719,7 +680,7 @@ func (rdb *RuleDB) Close() error { // Creates a rule with the given information and adds it to the rule database. // If any of the given parameters are invalid, returns an error. Otherwise, // returns the newly-added rule, and saves the database to disk. -func (rdb *RuleDB) AddRule(user uint32, snap string, iface string, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) (*Rule, error) { +func (rdb *RuleDB) AddRule(user uint32, snap string, iface string, constraints *prompting.Constraints) (*Rule, error) { rdb.mutex.Lock() defer rdb.mutex.Unlock() @@ -727,11 +688,14 @@ func (rdb *RuleDB) AddRule(user uint32, snap string, iface string, constraints * return nil, prompting_errors.ErrRulesClosed } - newRule, err := rdb.makeNewRule(user, snap, iface, constraints, outcome, lifespan, duration) + newRule, err := rdb.makeNewRule(user, snap, iface, constraints) if err != nil { return nil, err } if err := rdb.addRule(newRule); err != nil { + // Cannot have expired, since the expiration is based on the lifespan, + // duration, and timestamp, all of which were validated and set in + // makeNewRule. return nil, fmt.Errorf("cannot add rule: %w", err) } @@ -750,40 +714,29 @@ func (rdb *RuleDB) AddRule(user uint32, snap string, iface string, constraints * // makeNewRule creates a new Rule with the given contents. // -// Users of requestrules should probably autofill rules from JSON and never call -// this function directly. -// -// Constructs a new rule with the given parameters as values, with the -// exception of duration. Uses the given duration, in addition to the current -// time, to compute the expiration time for the rule, and stores that as part -// of the rule which is returned. If any of the given parameters are invalid, -// returns a corresponding error. -func (rdb *RuleDB) makeNewRule(user uint32, snap string, iface string, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) (*Rule, error) { +// Constructs a new rule with the given parameters as values. The given +// constraints are converted to rule constraints, using the timestamp of the +// new rule as the baseline with which to compute an expiration from any given +// duration. If any of the given parameters are invalid, returns an error. +func (rdb *RuleDB) makeNewRule(user uint32, snap string, iface string, constraints *prompting.Constraints) (*Rule, error) { currTime := time.Now() - expiration, err := lifespan.ParseDuration(duration, currTime) + ruleConstraints, err := constraints.ToRuleConstraints(iface, currTime) if err != nil { return nil, err } + // Don't consume an ID until now, when we know the rule is valid + id, _ := rdb.maxIDMmap.NextID() + newRule := Rule{ + ID: id, Timestamp: currTime, User: user, Snap: snap, Interface: iface, - Constraints: constraints, - Outcome: outcome, - Lifespan: lifespan, - Expiration: expiration, + Constraints: ruleConstraints, } - if err := newRule.validate(currTime); err != nil { - return nil, err - } - - // Don't consume an ID until now, when we know the rule is valid - id, _ := rdb.maxIDMmap.NextID() - newRule.ID = id - return &newRule, nil } @@ -804,8 +757,8 @@ func (rdb *RuleDB) IsPathAllowed(user uint32, snap string, iface string, path st currTime := time.Now() for variantStr, variantEntry := range variantMap { nonExpired := false - for id := range variantEntry.RuleIDs { - if !rdb.isRuleWithIDExpired(id, currTime) { + for _, rulePermissionEntry := range variantEntry.RuleEntries { + if !rulePermissionEntry.Expired(currTime) { nonExpired = true break } @@ -869,7 +822,11 @@ func (rdb *RuleDB) rulesInternal(ruleFilter func(rule *Rule) bool) []*Rule { rules := make([]*Rule, 0) currTime := time.Now() for _, rule := range rdb.rules { - if rule.Expired(currTime) { + if rule.expired(currTime) { + // XXX: it would be nice if we pruned expired permissions from a + // rule before including it in the rules list, if it's not expired. + // Since we don't hold the write lock, we don't want to + // automatically prune expired permissions here. Should this change? continue } @@ -1050,16 +1007,28 @@ func (rdb *RuleDB) RemoveRulesForSnapInterface(user uint32, snap string, iface s return rules, nil } -// PatchRule modifies the rule with the given ID by updating the rule's fields -// corresponding to any of the given parameters which are set/non-empty. +// PatchRule modifies the rule with the given ID by updating the rule's +// constraints for any constraint field or permission which is set/non-empty. +// +// If the path pattern is nil, it is left unchanged from the existing rule. +// Any permissions which are omitted from the new permissions map are left +// unchanged from the existing rule. To remove an existing permission from +// the rule, the permission should map to an empty permission entry. // -// Any of the parameters which are equal to the default/unset value for their -// types are left unchanged from the existing rule. Even if the given new rule -// contents exactly match the existing rule contents, the timestamp of the rule -// is updated to the current time. If there is any error while modifying the -// rule, the rule is rolled back to its previous unmodified state, leaving the -// database unchanged. If the database is changed, it is saved to disk. -func (rdb *RuleDB) PatchRule(user uint32, id prompting.IDType, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) (r *Rule, err error) { +// Permission entries must be provided as complete units, containing both +// outcome and lifespan (and duration, if lifespan is timespan). +// XXX: does API unmarshalling ensures this, or do we need explicit checks? +// +// Even if the given new rule contents exactly match the existing rule contents, +// the timestamp of the rule is updated to the current time. If there is any +// error while modifying the rule, the rule is rolled back to its previous +// unmodified state, leaving the database unchanged. If the database is changed, +// it is saved to disk. +// +// XXX: should we just remove this method entirely? +// Clients can always delete a rule and re-add it later, which is basically what +// this method already does. +func (rdb *RuleDB) PatchRule(user uint32, id prompting.IDType, patchConstraints *prompting.PatchConstraints) (r *Rule, err error) { rdb.mutex.Lock() defer rdb.mutex.Unlock() @@ -1071,21 +1040,31 @@ func (rdb *RuleDB) PatchRule(user uint32, id prompting.IDType, constraints *prom if err != nil { return nil, err } - if constraints == nil { - constraints = origRule.Constraints - } - if outcome == prompting.OutcomeUnset { - outcome = origRule.Outcome - } - if lifespan == prompting.LifespanUnset { - lifespan = origRule.Lifespan - } - newRule, err := rdb.makeNewRule(user, origRule.Snap, origRule.Interface, constraints, outcome, lifespan, duration) + // XXX: we don't currently check whether the rule is expired or not. + // Do we want to support patching a rule for which all the permissions + // have already expired? Or say if a rule has already expired, we don't + // support patching it? Currently, we don't include fully expired rules + // in the output of Rules(), should the same be done here? + + currTime := time.Now() + + if patchConstraints == nil { + patchConstraints = &prompting.PatchConstraints{} + } + ruleConstraints, err := patchConstraints.PatchRuleConstraints(origRule.Constraints, origRule.Interface, currTime) if err != nil { return nil, err } - newRule.ID = origRule.ID + + newRule := &Rule{ + ID: origRule.ID, + Timestamp: currTime, + User: origRule.User, + Snap: origRule.Snap, + Interface: origRule.Interface, + Constraints: ruleConstraints, + } // Remove the existing rule from the tree. An error should not occur, since // we just looked up the rule and know it exists. @@ -1096,7 +1075,7 @@ func (rdb *RuleDB) PatchRule(user uint32, id prompting.IDType, constraints *prom // Try to re-add original rule so all is unchanged. if origErr := rdb.addRule(origRule); origErr != nil { // Error should not occur, but if it does, wrap it in the other error - err = errorsJoin(err, fmt.Errorf("cannot re-add original rule: %w", origErr)) + err = prompting_errors.Join(err, fmt.Errorf("cannot re-add original rule: %w", origErr)) } return nil, err } diff --git a/interfaces/prompting/requestrules/requestrules_test.go b/interfaces/prompting/requestrules/requestrules_test.go index eb3d1f477f5..2d91cb136b8 100644 --- a/interfaces/prompting/requestrules/requestrules_test.go +++ b/interfaces/prompting/requestrules/requestrules_test.go @@ -25,6 +25,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" "testing" "time" @@ -79,73 +80,12 @@ func (s *requestrulesSuite) SetUpTest(c *C) { c.Assert(os.MkdirAll(dirs.SnapdStateDir(dirs.GlobalRootDir), 0700), IsNil) } -func (s *requestrulesSuite) TestRuleValidate(c *C) { - iface := "home" - pathPattern := mustParsePathPattern(c, "/home/test/**") - - validConstraints := &prompting.Constraints{ - PathPattern: pathPattern, - Permissions: []string{"read"}, - } - invalidConstraints := &prompting.Constraints{ - PathPattern: pathPattern, - Permissions: []string{"foo"}, - } - - validOutcome := prompting.OutcomeAllow - invalidOutcome := prompting.OutcomeUnset - - validLifespan := prompting.LifespanTimespan - invalidLifespan := prompting.LifespanSingle - - currTime := time.Now() - - validExpiration := currTime.Add(time.Millisecond) - invalidExpiration := currTime.Add(-time.Millisecond) - - rule := requestrules.Rule{ - // ID is not validated - // Timestamp is not validated - // User is not validated - // Snap is not validated - Interface: iface, - Constraints: validConstraints, - Outcome: validOutcome, - Lifespan: validLifespan, - Expiration: validExpiration, - } - c.Check(rule.Validate(currTime), IsNil) - - rule.Expiration = invalidExpiration - c.Check(rule.Validate(currTime), ErrorMatches, fmt.Sprintf("%v:.*", prompting_errors.ErrRuleExpirationInThePast)) - - rule.Lifespan = invalidLifespan - c.Check(rule.Validate(currTime), ErrorMatches, prompting_errors.NewRuleLifespanSingleError(prompting.SupportedRuleLifespans).Error()) - - rule.Outcome = invalidOutcome - c.Check(rule.Validate(currTime), ErrorMatches, `invalid outcome: ""`) - - rule.Constraints = invalidConstraints - c.Check(rule.Validate(currTime), ErrorMatches, "invalid permissions for home interface:.*") -} - func mustParsePathPattern(c *C, patternStr string) *patterns.PathPattern { pattern, err := patterns.ParsePathPattern(patternStr) c.Assert(err, IsNil) return pattern } -func (s *requestrulesSuite) TestRuleExpired(c *C) { - currTime := time.Now() - rule := requestrules.Rule{ - // Other fields are not relevant - Lifespan: prompting.LifespanTimespan, - Expiration: currTime, - } - c.Check(rule.Expired(currTime), Equals, false) - c.Check(rule.Expired(currTime.Add(time.Millisecond)), Equals, true) -} - func (s *requestrulesSuite) TestNew(c *C) { rdb, err := requestrules.New(s.defaultNotifyRule) c.Assert(err, IsNil) @@ -237,6 +177,19 @@ func (s *requestrulesSuite) checkNewNotices(c *C, expectedNotices []*noticeInfo) s.ruleNotices = s.ruleNotices[:0] } +func (s *requestrulesSuite) checkNewNoticesUnordered(c *C, expectedNotices []*noticeInfo) { + sort.Slice(sortSliceParams(s.ruleNotices)) + sort.Slice(sortSliceParams(expectedNotices)) + s.checkNewNotices(c, expectedNotices) +} + +func sortSliceParams(list []*noticeInfo) ([]*noticeInfo, func(i, j int) bool) { + less := func(i, j int) bool { + return list[i].ruleID < list[j].ruleID + } + return list, less +} + func (s *requestrulesSuite) TestLoadErrorOpen(c *C) { dbPath := s.prepDBPath(c) // Create unreadable DB file to cause open failure @@ -257,10 +210,11 @@ func (s *requestrulesSuite) TestLoadErrorUnmarshal(c *C) { func (s *requestrulesSuite) TestLoadErrorValidate(c *C) { dbPath := s.prepDBPath(c) - good1 := s.ruleTemplate(c, prompting.IDType(1)) - bad := s.ruleTemplate(c, prompting.IDType(2)) + good1 := s.ruleTemplateWithRead(c, prompting.IDType(1)) + bad := s.ruleTemplateWithRead(c, prompting.IDType(2)) bad.Interface = "foo" // will cause validate() to fail with invalid constraints - good2 := s.ruleTemplate(c, prompting.IDType(3)) + good2 := s.ruleTemplateWithRead(c, prompting.IDType(3)) + good2.Constraints.Permissions["read"].Outcome = prompting.OutcomeDeny // Doesn't matter that rules have conflicting patterns/permissions, // validate() should catch invalid rule and exit before attempting to add. @@ -271,11 +225,22 @@ func (s *requestrulesSuite) TestLoadErrorValidate(c *C) { s.testLoadError(c, `internal error: invalid interface: "foo".*`, rules, checkWritten) } +// ruleTemplateWithRead returns a rule with valid contents, intended to be customized. +func (s *requestrulesSuite) ruleTemplateWithRead(c *C, id prompting.IDType) *requestrules.Rule { + rule := s.ruleTemplate(c, id) + rule.Constraints.Permissions["read"] = &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + // No expiration for lifespan forever + } + return rule +} + // ruleTemplate returns a rule with valid contents, intended to be customized. func (s *requestrulesSuite) ruleTemplate(c *C, id prompting.IDType) *requestrules.Rule { - constraints := prompting.Constraints{ + constraints := prompting.RuleConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/foo"), - Permissions: []string{"read"}, + Permissions: make(prompting.RulePermissionMap), } rule := requestrules.Rule{ ID: id, @@ -284,9 +249,6 @@ func (s *requestrulesSuite) ruleTemplate(c *C, id prompting.IDType) *requestrule Snap: "firefox", Interface: "home", Constraints: &constraints, - Outcome: prompting.OutcomeAllow, - Lifespan: prompting.LifespanForever, - // Skip Expiration } return &rule } @@ -304,12 +266,13 @@ func (s *requestrulesSuite) writeRules(c *C, dbPath string, rules []*requestrule func (s *requestrulesSuite) TestLoadErrorConflictingID(c *C) { dbPath := s.prepDBPath(c) currTime := time.Now() - good := s.ruleTemplate(c, prompting.IDType(1)) - // Expired rules should still get a {"removed": "dropped"} notice, even if they don't conflict + good := s.ruleTemplateWithRead(c, prompting.IDType(1)) + // Expired rules should still get a {"removed": "expired"} notice, even if they don't conflict expired := s.ruleTemplate(c, prompting.IDType(2)) - setPathPatternAndExpiration(c, expired, "/home/test/other", currTime.Add(-10*time.Second)) + expired.Constraints.PathPattern = mustParsePathPattern(c, "/home/test/other") + setPermissionsOutcomeLifespanExpiration(c, expired, []string{"read"}, prompting.OutcomeAllow, prompting.LifespanTimespan, currTime.Add(-10*time.Second)) // Add rule which conflicts with IDs but doesn't otherwise conflict - conflicting := s.ruleTemplate(c, prompting.IDType(1)) + conflicting := s.ruleTemplateWithRead(c, prompting.IDType(1)) conflicting.Constraints.PathPattern = mustParsePathPattern(c, "/home/test/another") rules := []*requestrules.Rule{good, expired, conflicting} @@ -319,24 +282,29 @@ func (s *requestrulesSuite) TestLoadErrorConflictingID(c *C) { s.testLoadError(c, fmt.Sprintf("cannot add rule: %v.*", prompting_errors.ErrRuleIDConflict), rules, checkWritten) } -func setPathPatternAndExpiration(c *C, rule *requestrules.Rule, pathPattern string, expiration time.Time) { - rule.Constraints.PathPattern = mustParsePathPattern(c, pathPattern) - rule.Lifespan = prompting.LifespanTimespan - rule.Expiration = expiration +func setPermissionsOutcomeLifespanExpiration(c *C, rule *requestrules.Rule, permissions []string, outcome prompting.OutcomeType, lifespan prompting.LifespanType, expiration time.Time) { + for _, perm := range permissions { + rule.Constraints.Permissions[perm] = &prompting.RulePermissionEntry{ + Outcome: outcome, + Lifespan: lifespan, + Expiration: expiration, + } + } } func (s *requestrulesSuite) TestLoadErrorConflictingPattern(c *C) { dbPath := s.prepDBPath(c) currTime := time.Now() - good := s.ruleTemplate(c, prompting.IDType(1)) - // Expired rules should still get a {"removed": "dropped"} notice, even if they don't conflict - expired := s.ruleTemplate(c, prompting.IDType(2)) - setPathPatternAndExpiration(c, expired, "/home/test/other", currTime.Add(-10*time.Second)) + good := s.ruleTemplateWithRead(c, prompting.IDType(1)) + // Expired rules should still get a {"removed": "expired"} notice, even if they don't conflict + expired := s.ruleTemplateWithRead(c, prompting.IDType(2)) + expired.Constraints.PathPattern = mustParsePathPattern(c, "/home/test/other") + setPermissionsOutcomeLifespanExpiration(c, expired, []string{"read"}, prompting.OutcomeAllow, prompting.LifespanTimespan, currTime.Add(-10*time.Second)) // Add rule with conflicting pattern and permissions but not conflicting ID. - conflicting := s.ruleTemplate(c, prompting.IDType(3)) + conflicting := s.ruleTemplateWithRead(c, prompting.IDType(3)) // Even with not all permissions being in conflict, still error - conflicting.Constraints.Permissions = []string{"read", "write"} - conflicting.Outcome = prompting.OutcomeDeny + var timeZero time.Time + setPermissionsOutcomeLifespanExpiration(c, conflicting, []string{"read", "write"}, prompting.OutcomeDeny, prompting.LifespanForever, timeZero) rules := []*requestrules.Rule{good, expired, conflicting} s.writeRules(c, dbPath, rules) @@ -349,23 +317,26 @@ func (s *requestrulesSuite) TestLoadExpiredRules(c *C) { dbPath := s.prepDBPath(c) currTime := time.Now() - good1 := s.ruleTemplate(c, prompting.IDType(1)) + good1 := s.ruleTemplateWithRead(c, prompting.IDType(1)) // At the moment, expired rules with conflicts are discarded without error, // but we don't want to test this as part of our contract expired1 := s.ruleTemplate(c, prompting.IDType(2)) - setPathPatternAndExpiration(c, expired1, "/home/test/other", currTime.Add(-10*time.Second)) + expired1.Constraints.PathPattern = mustParsePathPattern(c, "/home/test/other") + setPermissionsOutcomeLifespanExpiration(c, expired1, []string{"read"}, prompting.OutcomeAllow, prompting.LifespanTimespan, currTime.Add(-10*time.Second)) // Rules with same pattern but non-conflicting permissions do not conflict good2 := s.ruleTemplate(c, prompting.IDType(3)) - good2.Constraints.Permissions = []string{"write"} + var timeZero time.Time + setPermissionsOutcomeLifespanExpiration(c, good2, []string{"write"}, prompting.OutcomeDeny, prompting.LifespanForever, timeZero) expired2 := s.ruleTemplate(c, prompting.IDType(4)) - setPathPatternAndExpiration(c, expired2, "/home/test/another", currTime.Add(-time.Nanosecond)) + expired2.Constraints.PathPattern = mustParsePathPattern(c, "/home/test/another") + setPermissionsOutcomeLifespanExpiration(c, expired2, []string{"read"}, prompting.OutcomeAllow, prompting.LifespanTimespan, currTime.Add(-time.Nanosecond)) // Rules with different pattern and conflicting permissions do not conflict - good3 := s.ruleTemplate(c, prompting.IDType(5)) + good3 := s.ruleTemplateWithRead(c, prompting.IDType(5)) good3.Constraints.PathPattern = mustParsePathPattern(c, "/home/test/no-conflict") rules := []*requestrules.Rule{good1, expired1, good2, expired2, good3} @@ -415,14 +386,14 @@ func (s *requestrulesSuite) TestLoadExpiredRules(c *C) { func (s *requestrulesSuite) TestLoadHappy(c *C) { dbPath := s.prepDBPath(c) - good1 := s.ruleTemplate(c, prompting.IDType(1)) + good1 := s.ruleTemplateWithRead(c, prompting.IDType(1)) // Rules with different users don't conflict - good2 := s.ruleTemplate(c, prompting.IDType(2)) + good2 := s.ruleTemplateWithRead(c, prompting.IDType(2)) good2.User = s.defaultUser + 1 // Rules with different snaps don't conflict - good3 := s.ruleTemplate(c, prompting.IDType(3)) + good3 := s.ruleTemplateWithRead(c, prompting.IDType(3)) good3.Snap = "thunderbird" rules := []*requestrules.Rule{good1, good2, good3} @@ -490,16 +461,21 @@ func (s *requestrulesSuite) TestCloseSaves(c *C) { // disk when DB is closed. constraints := &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/foo"), - Permissions: []string{"read"}, + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, } - rule, err := rdb.AddRule(s.defaultUser, "firefox", "home", constraints, prompting.OutcomeAllow, prompting.LifespanForever, "") + rule, err := rdb.AddRule(s.defaultUser, "firefox", "home", constraints) c.Assert(err, IsNil) // Check that new rule is on disk s.checkWrittenRuleDB(c, []*requestrules.Rule{rule}) // Mutate rule in memory - rule.Outcome = prompting.OutcomeDeny + rule.Constraints.Permissions["read"].Outcome = prompting.OutcomeDeny // Close DB c.Check(rdb.Close(), IsNil) @@ -611,11 +587,15 @@ func addRuleFromTemplate(c *C, rdb *requestrules.RuleDB, template *addRuleConten partial.Lifespan = template.Lifespan } // Duration default is empty string, so just use partial.Duration - constraints := &prompting.Constraints{ + replyConstraints := &prompting.ReplyConstraints{ PathPattern: mustParsePathPattern(c, partial.PathPattern), Permissions: partial.Permissions, } - return rdb.AddRule(partial.User, partial.Snap, partial.Interface, constraints, partial.Outcome, partial.Lifespan, partial.Duration) + constraints, err := replyConstraints.ToConstraints(partial.Interface, partial.Outcome, partial.Lifespan, partial.Duration) + if err != nil { + return nil, err + } + return rdb.AddRule(partial.User, partial.Snap, partial.Interface, constraints) } func (s *requestrulesSuite) TestAddRuleRemoveRuleDuplicateVariants(c *C) { @@ -844,6 +824,8 @@ func (s *requestrulesSuite) TestAddRuleExpired(c *C) { c.Assert(err, IsNil) c.Assert(good, NotNil) + // TODO: ADD test which tests behavior of rules which partially expire + // Add initial rule which will expire quickly prev, err := addRuleFromTemplate(c, rdb, template, &addRuleContents{ Lifespan: prompting.LifespanTimespan, @@ -1130,7 +1112,7 @@ func (s *requestrulesSuite) TestIsPathAllowedExpiration(c *C) { for i := len(addedRules) - 1; i >= 0; i-- { rule := addedRules[i] - expectedOutcome, err := rule.Outcome.AsBool() + expectedOutcome, err := rule.Constraints.Permissions["read"].Outcome.AsBool() c.Check(err, IsNil) // Check that the outcome of the most specific unexpired rule has precedence @@ -1142,24 +1124,24 @@ func (s *requestrulesSuite) TestIsPathAllowedExpiration(c *C) { s.checkNewNoticesSimple(c, nil) // Expire the highest precedence rule - rule.Expiration = time.Now() + rule.Constraints.Permissions["read"].Expiration = time.Now() } } func (s *requestrulesSuite) TestRuleWithID(c *C) { rdb, _ := requestrules.New(s.defaultNotifyRule) - snap := "lxd" - iface := "home" - constraints := &prompting.Constraints{ - PathPattern: mustParsePathPattern(c, "/home/test/Documents/**"), + template := &addRuleContents{ + User: s.defaultUser, + Snap: "lxd", + Interface: "home", + PathPattern: "/home/test/Documents/**", Permissions: []string{"read", "write", "execute"}, + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, } - outcome := prompting.OutcomeAllow - lifespan := prompting.LifespanForever - duration := "" - rule, err := rdb.AddRule(s.defaultUser, snap, iface, constraints, outcome, lifespan, duration) + rule, err := addRuleFromTemplate(c, rdb, template, template) c.Assert(err, IsNil) c.Assert(rule, NotNil) @@ -1246,12 +1228,13 @@ func (s *requestrulesSuite) TestRulesExpired(c *C) { rules := s.prepRuleDBForRulesForSnapInterface(c, rdb) // Set some rules to be expired - rules[0].Lifespan = prompting.LifespanTimespan - rules[0].Expiration = time.Now() - rules[2].Lifespan = prompting.LifespanTimespan - rules[2].Expiration = time.Now() - rules[4].Lifespan = prompting.LifespanTimespan - rules[4].Expiration = time.Now() + // This is brittle, relies on details of the rules added by prepRuleDBForRulesForSnapInterface + rules[0].Constraints.Permissions["read"].Lifespan = prompting.LifespanTimespan + rules[0].Constraints.Permissions["read"].Expiration = time.Now() + rules[2].Constraints.Permissions["read"].Lifespan = prompting.LifespanTimespan + rules[2].Constraints.Permissions["read"].Expiration = time.Now() + rules[4].Constraints.Permissions["read"].Lifespan = prompting.LifespanTimespan + rules[4].Constraints.Permissions["read"].Expiration = time.Now() // Expired rules are excluded from the Rules*() functions c.Check(rdb.Rules(s.defaultUser), DeepEquals, []*requestrules.Rule{rules[1], rules[3]}) @@ -1604,7 +1587,7 @@ func (s *requestrulesSuite) TestPatchRule(c *C) { origRule := *rule // Check that patching with no changes works fine, and updates timestamp - patched, err := rdb.PatchRule(rule.User, rule.ID, nil, prompting.OutcomeUnset, prompting.LifespanUnset, "") + patched, err := rdb.PatchRule(rule.User, rule.ID, nil) c.Assert(err, IsNil) s.checkWrittenRuleDB(c, append(rules[:len(rules)-1], patched)) s.checkNewNoticesSimple(c, nil, rule) @@ -1617,7 +1600,16 @@ func (s *requestrulesSuite) TestPatchRule(c *C) { rule = patched // Check that patching with identical content works fine, and updates timestamp - patched, err = rdb.PatchRule(rule.User, rule.ID, rule.Constraints, rule.Outcome, rule.Lifespan, "") + newConstraints := &prompting.PatchConstraints{ + PathPattern: rule.Constraints.PathPattern, + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: rule.Constraints.Permissions["read"].Outcome, + Lifespan: rule.Constraints.Permissions["read"].Lifespan, + }, + }, + } + patched, err = rdb.PatchRule(rule.User, rule.ID, newConstraints) c.Assert(err, IsNil) s.checkWrittenRuleDB(c, append(rules[:len(rules)-1], patched)) s.checkNewNoticesSimple(c, nil, rule) @@ -1629,11 +1621,15 @@ func (s *requestrulesSuite) TestPatchRule(c *C) { rule = patched - newConstraints := &prompting.Constraints{ - PathPattern: rule.Constraints.PathPattern, - Permissions: []string{"read", "execute"}, + newConstraints = &prompting.PatchConstraints{ + Permissions: prompting.PermissionMap{ + "execute": &prompting.PermissionEntry{ + Outcome: rule.Constraints.Permissions["read"].Outcome, + Lifespan: rule.Constraints.Permissions["read"].Lifespan, + }, + }, } - patched, err = rdb.PatchRule(rule.User, rule.ID, newConstraints, prompting.OutcomeUnset, prompting.LifespanUnset, "") + patched, err = rdb.PatchRule(rule.User, rule.ID, newConstraints) c.Assert(err, IsNil) s.checkWrittenRuleDB(c, append(rules[:len(rules)-1], patched)) s.checkNewNoticesSimple(c, nil, rule) @@ -1641,12 +1637,36 @@ func (s *requestrulesSuite) TestPatchRule(c *C) { c.Check(patched.Timestamp.Equal(rule.Timestamp), Equals, false) // After making timestamp the same again, check that the rules are identical patched.Timestamp = rule.Timestamp - rule.Constraints = newConstraints + rule.Constraints = &prompting.RuleConstraints{ + PathPattern: patched.Constraints.PathPattern, + Permissions: prompting.RulePermissionMap{ + "read": &prompting.RulePermissionEntry{ + Outcome: patched.Constraints.Permissions["read"].Outcome, + Lifespan: patched.Constraints.Permissions["read"].Lifespan, + }, + "execute": &prompting.RulePermissionEntry{ + Outcome: patched.Constraints.Permissions["read"].Outcome, + Lifespan: patched.Constraints.Permissions["read"].Lifespan, + }, + }, + } c.Check(patched, DeepEquals, rule) rule = patched - patched, err = rdb.PatchRule(rule.User, rule.ID, nil, prompting.OutcomeDeny, prompting.LifespanUnset, "") + newConstraints = &prompting.PatchConstraints{ + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + "execute": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + }, + } + patched, err = rdb.PatchRule(rule.User, rule.ID, newConstraints) c.Assert(err, IsNil) s.checkWrittenRuleDB(c, append(rules[:len(rules)-1], patched)) s.checkNewNoticesSimple(c, nil, rule) @@ -1654,12 +1674,27 @@ func (s *requestrulesSuite) TestPatchRule(c *C) { c.Check(patched.Timestamp.Equal(rule.Timestamp), Equals, false) // After making timestamp the same again, check that the rules are identical patched.Timestamp = rule.Timestamp - rule.Outcome = prompting.OutcomeDeny + rule.Constraints.Permissions["read"].Outcome = prompting.OutcomeDeny + rule.Constraints.Permissions["execute"].Outcome = prompting.OutcomeDeny c.Check(patched, DeepEquals, rule) rule = patched - patched, err = rdb.PatchRule(rule.User, rule.ID, nil, prompting.OutcomeUnset, prompting.LifespanTimespan, "10s") + newConstraints = &prompting.PatchConstraints{ + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanTimespan, + Duration: "10s", + }, + "execute": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanTimespan, + Duration: "10s", + }, + }, + } + patched, err = rdb.PatchRule(rule.User, rule.ID, newConstraints) c.Assert(err, IsNil) s.checkWrittenRuleDB(c, append(rules[:len(rules)-1], patched)) s.checkNewNoticesSimple(c, nil, rule) @@ -1667,13 +1702,24 @@ func (s *requestrulesSuite) TestPatchRule(c *C) { c.Check(patched.Timestamp.Equal(rule.Timestamp), Equals, false) // After making timestamp the same again, check that the rules are identical patched.Timestamp = rule.Timestamp - rule.Lifespan = prompting.LifespanTimespan - rule.Expiration = patched.Expiration + rule.Constraints.Permissions["read"].Lifespan = prompting.LifespanTimespan + rule.Constraints.Permissions["execute"].Lifespan = prompting.LifespanTimespan + rule.Constraints.Permissions["read"].Expiration = patched.Constraints.Permissions["read"].Expiration + rule.Constraints.Permissions["execute"].Expiration = patched.Constraints.Permissions["execute"].Expiration c.Check(patched, DeepEquals, rule) rule = patched - patched, err = rdb.PatchRule(rule.User, rule.ID, origRule.Constraints, origRule.Outcome, origRule.Lifespan, "") + newConstraints = &prompting.PatchConstraints{ + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: origRule.Constraints.Permissions["read"].Outcome, + Lifespan: origRule.Constraints.Permissions["read"].Lifespan, + }, + "execute": nil, + }, + } + patched, err = rdb.PatchRule(rule.User, rule.ID, newConstraints) c.Assert(err, IsNil) s.checkWrittenRuleDB(c, append(rules[:len(rules)-1], patched)) s.checkNewNoticesSimple(c, nil, rule) @@ -1717,33 +1763,52 @@ func (s *requestrulesSuite) TestPatchRuleErrors(c *C) { rule := rules[len(rules)-1] // Wrong user - result, err := rdb.PatchRule(rule.User+1, rule.ID, nil, prompting.OutcomeUnset, prompting.LifespanUnset, "") + result, err := rdb.PatchRule(rule.User+1, rule.ID, nil) c.Check(err, Equals, prompting_errors.ErrRuleNotAllowed) c.Check(result, IsNil) s.checkWrittenRuleDB(c, rules) s.checkNewNoticesSimple(c, nil) // Wrong ID - result, err = rdb.PatchRule(rule.User, prompting.IDType(1234), nil, prompting.OutcomeUnset, prompting.LifespanUnset, "") + result, err = rdb.PatchRule(rule.User, prompting.IDType(1234), nil) c.Check(err, Equals, prompting_errors.ErrRuleNotFound) c.Check(result, IsNil) s.checkWrittenRuleDB(c, rules) s.checkNewNoticesSimple(c, nil) // Invalid lifespan - result, err = rdb.PatchRule(rule.User, rule.ID, nil, prompting.OutcomeUnset, prompting.LifespanSingle, "") + badConstraints := &prompting.PatchConstraints{ + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanSingle, + }, + }, + } + result, err = rdb.PatchRule(rule.User, rule.ID, badConstraints) c.Check(err, ErrorMatches, prompting_errors.NewRuleLifespanSingleError(prompting.SupportedRuleLifespans).Error()) c.Check(result, IsNil) s.checkWrittenRuleDB(c, rules) s.checkNewNoticesSimple(c, nil) // Conflicting rule - conflictingOutcome := prompting.OutcomeDeny - conflictingConstraints := &prompting.Constraints{ - PathPattern: mustParsePathPattern(c, template.PathPattern), - Permissions: []string{"read", "write", "execute"}, + conflictingConstraints := &prompting.PatchConstraints{ + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + "write": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + "execute": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + }, } - result, err = rdb.PatchRule(rule.User, rule.ID, conflictingConstraints, conflictingOutcome, prompting.LifespanUnset, "") + result, err = rdb.PatchRule(rule.User, rule.ID, conflictingConstraints) c.Check(err, ErrorMatches, fmt.Sprintf("cannot patch rule: %v", prompting_errors.ErrRuleConflict)) c.Check(result, IsNil) s.checkWrittenRuleDB(c, rules) @@ -1753,7 +1818,7 @@ func (s *requestrulesSuite) TestPatchRuleErrors(c *C) { func() { c.Assert(os.Chmod(prompting.StateDir(), 0o500), IsNil) defer os.Chmod(prompting.StateDir(), 0o700) - result, err = rdb.PatchRule(rule.User, rule.ID, nil, prompting.OutcomeUnset, prompting.LifespanUnset, "") + result, err = rdb.PatchRule(rule.User, rule.ID, nil) c.Check(err, NotNil) c.Check(result, IsNil) s.checkWrittenRuleDB(c, rules) @@ -1762,7 +1827,7 @@ func (s *requestrulesSuite) TestPatchRuleErrors(c *C) { // DB Closed c.Assert(rdb.Close(), IsNil) - result, err = rdb.PatchRule(rule.User, rule.ID, nil, prompting.OutcomeUnset, prompting.LifespanUnset, "") + result, err = rdb.PatchRule(rule.User, rule.ID, nil) c.Check(err, Equals, prompting_errors.ErrRulesClosed) c.Check(result, IsNil) s.checkWrittenRuleDB(c, rules) @@ -1803,11 +1868,23 @@ func (s *requestrulesSuite) TestPatchRuleExpired(c *C) { // Patching doesn't conflict with already-expired rules rule := rules[2] - newConstraints := &prompting.Constraints{ - PathPattern: mustParsePathPattern(c, template.PathPattern), - Permissions: []string{"read", "write", "execute"}, + newConstraints := &prompting.PatchConstraints{ + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + "write": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + "execute": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + }, } - patched, err := rdb.PatchRule(rule.User, rule.ID, newConstraints, prompting.OutcomeUnset, prompting.LifespanUnset, "") + patched, err := rdb.PatchRule(rule.User, rule.ID, newConstraints) c.Assert(err, IsNil) s.checkWrittenRuleDB(c, []*requestrules.Rule{patched}) expectedNotices := []*noticeInfo{ @@ -1827,11 +1904,24 @@ func (s *requestrulesSuite) TestPatchRuleExpired(c *C) { data: nil, }, } - s.checkNewNotices(c, expectedNotices) + s.checkNewNoticesUnordered(c, expectedNotices) // Check that timestamp has changed c.Check(patched.Timestamp.Equal(rule.Timestamp), Equals, false) // After making timestamp the same again, check that the rules are identical patched.Timestamp = rule.Timestamp - rule.Constraints = newConstraints + rule.Constraints.Permissions = prompting.RulePermissionMap{ + "read": &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + "write": &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + "execute": &prompting.RulePermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + } c.Check(patched, DeepEquals, rule) } diff --git a/overlord/ifacestate/apparmorprompting/prompting.go b/overlord/ifacestate/apparmorprompting/prompting.go index bef6cb609c9..2f7efb906d6 100644 --- a/overlord/ifacestate/apparmorprompting/prompting.go +++ b/overlord/ifacestate/apparmorprompting/prompting.go @@ -51,12 +51,12 @@ var ( type Manager interface { Prompts(userID uint32) ([]*requestprompts.Prompt, error) PromptWithID(userID uint32, promptID prompting.IDType) (*requestprompts.Prompt, error) - HandleReply(userID uint32, promptID prompting.IDType, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) ([]prompting.IDType, error) + HandleReply(userID uint32, promptID prompting.IDType, replyConstraints *prompting.ReplyConstraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) ([]prompting.IDType, error) Rules(userID uint32, snap string, iface string) ([]*requestrules.Rule, error) - AddRule(userID uint32, snap string, iface string, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) (*requestrules.Rule, error) + AddRule(userID uint32, snap string, iface string, constraints *prompting.Constraints) (*requestrules.Rule, error) RemoveRules(userID uint32, snap string, iface string) ([]*requestrules.Rule, error) RuleWithID(userID uint32, ruleID prompting.IDType) (*requestrules.Rule, error) - PatchRule(userID uint32, ruleID prompting.IDType, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) (*requestrules.Rule, error) + PatchRule(userID uint32, ruleID prompting.IDType, constraints *prompting.PatchConstraints) (*requestrules.Rule, error) RemoveRule(userID uint32, ruleID prompting.IDType) (*requestrules.Rule, error) } @@ -221,6 +221,7 @@ func (m *InterfacesRequestsManager) handleListenerReq(req *listener.Request) err 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 @@ -311,29 +312,7 @@ func (m *InterfacesRequestsManager) disconnect() error { m.rules = nil } - return errorsJoin(errs...) -} - -// errorsJoin returns an error that wraps the given errors. -// Any nil error values are discarded. -// errorsJoin returns nil if every value in errs is nil. -// -// TODO: replace with errors.Join() once we're on golang v1.20+ -func errorsJoin(errs ...error) error { - var nonNilErrs []error - for _, e := range errs { - if e != nil { - nonNilErrs = append(nonNilErrs, e) - } - } - if len(nonNilErrs) == 0 { - return nil - } - err := nonNilErrs[0] - for _, e := range nonNilErrs[1:] { - err = fmt.Errorf("%w\n%v", err, e) - } - return err + return prompting_errors.Join(errs...) } // Stop closes the listener, prompt DB, and rule DB. Stop is idempotent, and @@ -363,7 +342,7 @@ func (m *InterfacesRequestsManager) PromptWithID(userID uint32, promptID prompti // is not "single"). If all of these are true, sends a reply for the prompt with // the given ID, and both creates a new rule and checks any outstanding prompts // against it, if the lifespan is not "single". -func (m *InterfacesRequestsManager) HandleReply(userID uint32, promptID prompting.IDType, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) (satisfiedPromptIDs []prompting.IDType, retErr error) { +func (m *InterfacesRequestsManager) HandleReply(userID uint32, promptID prompting.IDType, replyConstraints *prompting.ReplyConstraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) (satisfiedPromptIDs []prompting.IDType, retErr error) { m.lock.Lock() defer m.lock.Unlock() @@ -372,10 +351,12 @@ func (m *InterfacesRequestsManager) HandleReply(userID uint32, promptID promptin return nil, err } - // Outcome and lifesnap are validated while unmarshalling, and duration is - // validated when the rule is being added. So only need to validate - // constraints. - if err := constraints.ValidateForInterface(prompt.Interface); err != nil { + // Validate reply constraints and convert them to Constraints, which have + // dedicated PermissionEntry values for each permission in the reply. + // Outcome and lifespan are validated while unmarshalling, and duration is + // validated against the given lifespan when constructing the Constraints. + constraints, err := replyConstraints.ToConstraints(prompt.Interface, outcome, lifespan, duration) + if err != nil { return nil, err } @@ -386,24 +367,24 @@ func (m *InterfacesRequestsManager) HandleReply(userID uint32, promptID promptin // constraints, such as check that the path pattern does not match // any paths not granted by the interface. // TODO: Should this be reconsidered? - matches, err := constraints.Match(prompt.Constraints.Path()) + matches, err := replyConstraints.Match(prompt.Constraints.Path()) if err != nil { return nil, err } if !matches { return nil, &prompting_errors.RequestedPathNotMatchedError{ Requested: prompt.Constraints.Path(), - Replied: constraints.PathPattern.String(), + Replied: replyConstraints.PathPattern.String(), } } // XXX: do we want to allow only replying to a select subset of permissions, and // auto-deny the rest? - contained := constraints.ContainPermissions(prompt.Constraints.RemainingPermissions()) + contained := replyConstraints.ContainPermissions(prompt.Constraints.RemainingPermissions()) if !contained { return nil, &prompting_errors.RequestedPermissionsNotMatchedError{ Requested: prompt.Constraints.RemainingPermissions(), - Replied: constraints.Permissions, + Replied: replyConstraints.Permissions, } } @@ -413,7 +394,7 @@ func (m *InterfacesRequestsManager) HandleReply(userID uint32, promptID promptin var newRule *requestrules.Rule if lifespan != prompting.LifespanSingle { // Check that adding the rule doesn't conflict with other rules - newRule, err = m.rules.AddRule(userID, prompt.Snap, prompt.Interface, constraints, outcome, lifespan, duration) + newRule, err = m.rules.AddRule(userID, prompt.Snap, prompt.Interface, constraints) if err != nil { // Rule conflicts with existing rule (at least one identical pattern // variant and permission). This should be considered a bad reply, @@ -451,7 +432,7 @@ func (m *InterfacesRequestsManager) applyRuleToOutstandingPrompts(rule *requestr Snap: rule.Snap, Interface: rule.Interface, } - satisfiedPromptIDs, err := m.prompts.HandleNewRule(metadata, rule.Constraints, rule.Outcome) + satisfiedPromptIDs, err := m.prompts.HandleNewRule(metadata, rule.Constraints) if err != nil { // The rule's constraints and outcome were already validated, so an // error should not occur here unless the prompt DB was already closed. @@ -484,11 +465,11 @@ func (m *InterfacesRequestsManager) Rules(userID uint32, snap string, iface stri // AddRule creates a new rule with the given contents and then checks it against // outstanding prompts, resolving any prompts which it satisfies. -func (m *InterfacesRequestsManager) AddRule(userID uint32, snap string, iface string, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) (*requestrules.Rule, error) { +func (m *InterfacesRequestsManager) AddRule(userID uint32, snap string, iface string, constraints *prompting.Constraints) (*requestrules.Rule, error) { m.lock.Lock() defer m.lock.Unlock() - newRule, err := m.rules.AddRule(userID, snap, iface, constraints, outcome, lifespan, duration) + newRule, err := m.rules.AddRule(userID, snap, iface, constraints) if err != nil { return nil, err } @@ -531,11 +512,11 @@ func (m *InterfacesRequestsManager) RuleWithID(userID uint32, ruleID prompting.I // PatchRule updates the rule with the given ID using the provided contents. // Any of the given fields which are empty/nil are not updated in the rule. -func (m *InterfacesRequestsManager) PatchRule(userID uint32, ruleID prompting.IDType, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) (*requestrules.Rule, error) { +func (m *InterfacesRequestsManager) PatchRule(userID uint32, ruleID prompting.IDType, constraints *prompting.PatchConstraints) (*requestrules.Rule, error) { m.lock.Lock() defer m.lock.Unlock() - patchedRule, err := m.rules.PatchRule(userID, ruleID, constraints, outcome, lifespan, duration) + patchedRule, err := m.rules.PatchRule(userID, ruleID, constraints) if err != nil { return nil, err } diff --git a/overlord/ifacestate/apparmorprompting/prompting_test.go b/overlord/ifacestate/apparmorprompting/prompting_test.go index d67b0d7e027..e5716a45be5 100644 --- a/overlord/ifacestate/apparmorprompting/prompting_test.go +++ b/overlord/ifacestate/apparmorprompting/prompting_test.go @@ -270,7 +270,7 @@ func (s *apparmorpromptingSuite) TestHandleReplySimple(c *C) { req, prompt := s.simulateRequest(c, reqChan, mgr, &listener.Request{}, false) // Reply to the request - constraints := prompting.Constraints{ + constraints := prompting.ReplyConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), Permissions: []string{"read"}, } @@ -418,7 +418,7 @@ func (s *apparmorpromptingSuite) TestHandleReplyErrors(c *C) { c.Check(result, IsNil) // Invalid constraints - invalidConstraints := prompting.Constraints{ + invalidConstraints := prompting.ReplyConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), Permissions: []string{"foo"}, } @@ -427,7 +427,7 @@ func (s *apparmorpromptingSuite) TestHandleReplyErrors(c *C) { c.Check(result, IsNil) // Path not matched - badPatternConstraints := prompting.Constraints{ + badPatternConstraints := prompting.ReplyConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/other"), Permissions: []string{"read"}, } @@ -436,7 +436,7 @@ func (s *apparmorpromptingSuite) TestHandleReplyErrors(c *C) { c.Check(result, IsNil) // Permissions not matched - badPermissionConstraints := prompting.Constraints{ + badPermissionConstraints := prompting.ReplyConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/foo"), Permissions: []string{"write"}, } @@ -447,11 +447,21 @@ func (s *apparmorpromptingSuite) TestHandleReplyErrors(c *C) { // Conflicting rule // For this, need to add another rule to the DB first, then try to reply // with a rule which conflicts with it. Reuse badPatternConstraints. - newRule, err := mgr.AddRule(s.defaultUser, "firefox", "home", &badPatternConstraints, prompting.OutcomeAllow, prompting.LifespanTimespan, "10s") + anotherConstraints := prompting.Constraints{ + PathPattern: mustParsePathPattern(c, "/home/test/other"), + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanTimespan, + Duration: "10s", + }, + }, + } + newRule, err := mgr.AddRule(s.defaultUser, "firefox", "home", &anotherConstraints) c.Assert(err, IsNil) c.Assert(newRule, NotNil) conflictingOutcome := prompting.OutcomeDeny - conflictingConstraints := prompting.Constraints{ + conflictingConstraints := prompting.ReplyConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/{foo,other}"), Permissions: []string{"read"}, } @@ -476,17 +486,27 @@ func (s *apparmorpromptingSuite) TestExistingRuleAllowsNewPrompt(c *C) { // Add allow rule to match read permission constraints := &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), - Permissions: []string{"read"}, - } - _, err = mgr.AddRule(s.defaultUser, "firefox", "home", constraints, prompting.OutcomeAllow, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, + } + _, err = mgr.AddRule(s.defaultUser, "firefox", "home", constraints) c.Assert(err, IsNil) // Add allow rule to match write permission constraints = &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), - Permissions: []string{"write"}, - } - _, err = mgr.AddRule(s.defaultUser, "firefox", "home", constraints, prompting.OutcomeAllow, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "write": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, + } + _, err = mgr.AddRule(s.defaultUser, "firefox", "home", constraints) c.Assert(err, IsNil) // Create request for read and write @@ -524,7 +544,7 @@ func (s *apparmorpromptingSuite) checkRecordedPromptNotices(c *C, since time.Tim After: since, }) s.st.Unlock() - c.Check(n, HasLen, count) + c.Check(n, HasLen, count, Commentf("%+v", n)) } func (s *apparmorpromptingSuite) checkRecordedRuleUpdateNotices(c *C, since time.Time, count int) { @@ -534,7 +554,7 @@ func (s *apparmorpromptingSuite) checkRecordedRuleUpdateNotices(c *C, since time After: since, }) s.st.Unlock() - c.Check(n, HasLen, count) + c.Check(n, HasLen, count, Commentf("%+v", n)) } func (s *apparmorpromptingSuite) TestExistingRulePartiallyAllowsNewPrompt(c *C) { @@ -547,9 +567,14 @@ func (s *apparmorpromptingSuite) TestExistingRulePartiallyAllowsNewPrompt(c *C) // Add rule to match read permission constraints := &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), - Permissions: []string{"read"}, - } - _, err = mgr.AddRule(s.defaultUser, "firefox", "home", constraints, prompting.OutcomeAllow, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, + } + _, err = mgr.AddRule(s.defaultUser, "firefox", "home", constraints) c.Assert(err, IsNil) // Do NOT add rule to match write permission @@ -576,9 +601,14 @@ func (s *apparmorpromptingSuite) TestExistingRulePartiallyDeniesNewPrompt(c *C) // Add deny rule to match read permission constraints := &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), - Permissions: []string{"read"}, - } - _, err = mgr.AddRule(s.defaultUser, "firefox", "home", constraints, prompting.OutcomeDeny, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + }, + } + _, err = mgr.AddRule(s.defaultUser, "firefox", "home", constraints) c.Assert(err, IsNil) // Add no rule for write permissions @@ -619,17 +649,27 @@ func (s *apparmorpromptingSuite) TestExistingRulesMixedMatchNewPromptDenies(c *C // Add deny rule to match read permission constraints := &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), - Permissions: []string{"read"}, - } - _, err = mgr.AddRule(s.defaultUser, "firefox", "home", constraints, prompting.OutcomeDeny, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + }, + } + _, err = mgr.AddRule(s.defaultUser, "firefox", "home", constraints) c.Assert(err, IsNil) // Add allow rule for write permissions constraints = &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), - Permissions: []string{"write"}, - } - _, err = mgr.AddRule(s.defaultUser, "firefox", "home", constraints, prompting.OutcomeAllow, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "write": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, + } + _, err = mgr.AddRule(s.defaultUser, "firefox", "home", constraints) c.Assert(err, IsNil) // Create request for read and write @@ -694,9 +734,14 @@ func (s *apparmorpromptingSuite) TestNewRuleAllowExistingPrompt(c *C) { whenSent := time.Now() constraints := &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), - Permissions: []string{"read"}, - } - rule, err := mgr.AddRule(s.defaultUser, "firefox", "home", constraints, prompting.OutcomeAllow, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, + } + rule, err := mgr.AddRule(s.defaultUser, "firefox", "home", constraints) c.Assert(err, IsNil) // Check that kernel received a reply @@ -767,9 +812,14 @@ func (s *apparmorpromptingSuite) TestNewRuleDenyExistingPrompt(c *C) { whenSent := time.Now() constraints := &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), - Permissions: []string{"read"}, - } - rule, err := mgr.AddRule(s.defaultUser, "firefox", "home", constraints, prompting.OutcomeDeny, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeDeny, + Lifespan: prompting.LifespanForever, + }, + }, + } + rule, err := mgr.AddRule(s.defaultUser, "firefox", "home", constraints) c.Assert(err, IsNil) // Check that kernel received two replies @@ -834,7 +884,7 @@ func (s *apparmorpromptingSuite) TestReplyNewRuleHandlesExistingPrompt(c *C) { // Reply to read prompt with denial whenSent := time.Now() - constraints := &prompting.Constraints{ + constraints := &prompting.ReplyConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), Permissions: []string{"read"}, } @@ -915,7 +965,7 @@ func (s *apparmorpromptingSuite) testReplyRuleHandlesFuturePrompts(c *C, outcome // Reply to read prompt with denial whenSent := time.Now() - constraints := &prompting.Constraints{ + constraints := &prompting.ReplyConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), Permissions: []string{"read", "write"}, } @@ -1017,9 +1067,14 @@ func (s *apparmorpromptingSuite) TestRequestMerged(c *C) { // Add rule to satisfy the read permission constraints := &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), - Permissions: []string{"read"}, - } - _, err = mgr.AddRule(s.defaultUser, prompt.Snap, prompt.Interface, constraints, prompting.OutcomeAllow, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, + } + _, err = mgr.AddRule(s.defaultUser, prompt.Snap, prompt.Interface, constraints) c.Assert(err, IsNil) // Create identical request again, it should merge even though some @@ -1077,27 +1132,42 @@ func (s *apparmorpromptingSuite) prepManagerWithRules(c *C) (mgr *apparmorprompt // Add rule for firefox and home constraints := &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/1"), - Permissions: []string{"read"}, - } - rule1, err := mgr.AddRule(s.defaultUser, "firefox", "home", constraints, prompting.OutcomeAllow, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, + } + rule1, err := mgr.AddRule(s.defaultUser, "firefox", "home", constraints) c.Assert(err, IsNil) rules = append(rules, rule1) // Add rule for thunderbird and home constraints = &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/2"), - Permissions: []string{"read"}, - } - rule2, err := mgr.AddRule(s.defaultUser, "thunderbird", "home", constraints, prompting.OutcomeAllow, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, + } + rule2, err := mgr.AddRule(s.defaultUser, "thunderbird", "home", constraints) c.Assert(err, IsNil) rules = append(rules, rule2) // Add rule for firefox and camera constraints = &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/3"), - Permissions: []string{"read"}, - } - rule3, err := mgr.AddRule(s.defaultUser, "firefox", "home", constraints, prompting.OutcomeAllow, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, + } + rule3, err := mgr.AddRule(s.defaultUser, "firefox", "home", constraints) c.Assert(err, IsNil) // Since camera interface isn't supported yet, must adjust the interface // after the rule has been created. This abuses implementation details of @@ -1108,9 +1178,14 @@ func (s *apparmorpromptingSuite) prepManagerWithRules(c *C) (mgr *apparmorprompt // Add rule for firefox and home, but for a different user constraints = &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/4"), - Permissions: []string{"read"}, - } - rule4, err := mgr.AddRule(s.defaultUser+1, "firefox", "home", constraints, prompting.OutcomeAllow, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, + } + rule4, err := mgr.AddRule(s.defaultUser+1, "firefox", "home", constraints) c.Assert(err, IsNil) rules = append(rules, rule4) @@ -1206,31 +1281,48 @@ func (s *apparmorpromptingSuite) TestAddRuleWithIDPatchRemove(c *C) { whenAdded := time.Now() constraints := &prompting.Constraints{ PathPattern: mustParsePathPattern(c, "/home/test/**"), - Permissions: []string{"write"}, - } - rule, err := mgr.AddRule(s.defaultUser, "firefox", "home", constraints, prompting.OutcomeAllow, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "write": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, + } + rule, err := mgr.AddRule(s.defaultUser, "firefox", "home", constraints) c.Assert(err, IsNil) s.checkRecordedRuleUpdateNotices(c, whenAdded, 1) + s.checkRecordedPromptNotices(c, whenAdded, 0) // Test RuleWithID + whenAccessed := time.Now() retrieved, err := mgr.RuleWithID(rule.User, rule.ID) c.Assert(err, IsNil) c.Assert(retrieved, Equals, rule) + s.checkRecordedRuleUpdateNotices(c, whenAccessed, 0) // Check prompt still exists and no prompt notices recorded since before // the rule was added retrievedPrompt, err := mgr.PromptWithID(s.defaultUser, prompt.ID) c.Assert(err, IsNil) c.Assert(retrievedPrompt, Equals, prompt) - s.checkRecordedPromptNotices(c, whenAdded, 0) + s.checkRecordedPromptNotices(c, whenAccessed, 0) // Patch rule to now cover the outstanding prompt whenPatched := time.Now() - newConstraints := &prompting.Constraints{ + newConstraints := &prompting.PatchConstraints{ PathPattern: mustParsePathPattern(c, "/home/test/{foo,bar,baz}"), - Permissions: []string{"read", "write"}, - } - patched, err := mgr.PatchRule(s.defaultUser, rule.ID, newConstraints, prompting.OutcomeAllow, prompting.LifespanForever, "") + Permissions: prompting.PermissionMap{ + "read": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + "write": &prompting.PermissionEntry{ + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + }, + }, + } + patched, err := mgr.PatchRule(s.defaultUser, rule.ID, newConstraints) c.Assert(err, IsNil) s.checkRecordedRuleUpdateNotices(c, whenPatched, 1)