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

[olivercalder]

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).

CC @sminez and @juanruitina

This addresses the issue discussed here: canonical/desktop-security-center#74

This work is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-32361

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.95%. Comparing base (2e47491) to head (3aa3632).
Report is 41 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14538      +/-   ##
==========================================
+ Coverage   78.89%   78.95%   +0.06%     
==========================================
  Files        1083     1084       +1     
  Lines      146377   146614     +237     
==========================================
+ Hits       115479   115764     +285     
+ Misses      23695    23656      -39     
+ Partials     7203     7194       -9     

Flag	Coverage Δ
unittests	78.95% <100.00%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pedronis]

why do we map to OutcomeType if we expects all "Outcome"s" to be the same? what am I missing? should outcome be stored separately? am I reading the comments wrong?

[olivercalder]

You're right, this is a bit odd. The rationale behind mapping rule ID to outcome was to less-tightly couple outcome to the entry itself, in case the rules expire, and also to allow the coexistence of expired rules for one outcome and non-expired rules for another.

But it's true, whenever we add a new rule, we clean up expired rules, so we'll never have coexisting expired and non-expired rules for different outcomes. And it's definitely clumsy to look up the outcome from a particular rule ID. So I think storing the outcome associated with a variant entry would be cleaner and clearer.

The other rationale was to make it easier to tell what the outcome of a rule was without trying to look it up (where it might not exist if there's an inconsistency). But this isn't a very good reason, as we look up the rule to check its expiration immediately before, so it's fine to look up the outcome from the rule instead of storing it in the map value. But really, we don't need this at all, since we know that the outcome of every rule in a variant entry must be equal to the entry's outcome. So this is great.

[olivercalder]

In the prompting sync today, Gustavo pushed back on the idea of allowing multiple rules with identical path patterns (as opposed to identical pattern variants), on the grounds that this creates confusion for the user about which of those rules has priority and which takes effect.

We agreed that in some cases where there is partial overlap between multiple rules (that is, the rules render to one or more identical pattern variants and have the same outcome), it's easier to permit the complexity of the overlap than require the user to navigate modifying the rules to avoid conflict (and sometimes it's not possible to modify a rule without changing the overall semantics of the ruleset). For example:

• existing rule: "allow read|write /{foo,bar}"
• new rule: "allow read|execute /{bar,baz}"

In this case, the two rules overlap on the "allow read /bar" case. Removing the /bar variant from either path pattern has the side effect of either removing write permission or execute permission for the other variant, respectively, and removing the read permission from either rule has the side effect of removing read permission from either /foo or /baz. There's no way to cover all the semantic effects of both these rules without splitting them up into more sub-rules.

However, if we treat "identical path pattern" as a special case of "identical pattern variant", we can actually combine rules which have truly identical path patterns (so long as they have the same outcome). That is: if there exists a rule with the same path pattern and same outcome, but we have different permissions, union the existing permissions with the new permissions or tell the user "hey, you already have a rule with that exact path pattern, would how would you like to set its permissions".

This sounds good, at least if we allow one "allow" rule and one "deny" rule for each path pattern, with different permissions for each, and no overlap. If we really want only one rule ever for a given path pattern, then we need to fundamentally change the way rule outcomes are handled, so a single rule can have different outcomes for different permissions.

I like and can implement the first approach, I don't think we should change the way rules store outcomes without further discussion.

I'm also wary of assuming any sort of guarantees about "identical" path patterns not having overlaps. I think we should really treat this as a special case, just for the convenience of users in the most common situations: if a user previously replied "allow read ~/Downloads/**", then later replies "allow read|write ~/Downloads/**", we can modify the existing rule to add write permission as well. For anything more complicated though, I think we need to carry on allowing overlaps:

• "allow read /{foo,bar}"
• "allow read|write /{bar,foo}"

In this example, it's trivially clear that these two path patterns render to an identical set of variants, but it's not in general possible to check this without rendering and storing that complete set of variants for both rules. (A closely-related problem, computing whether a pattern renders to duplicate variants and trying to deduplicate them in the tree before rendering, is discussed in #14319 and #14526.) We could potentially render each rule's complete set of variants, sort them lexicographically, and compare these, but I'd rather not go down that path if we can help it.

So current approach to implement: when we reply or add a rule, check for an existing rule with an identical path pattern and the same outcome, and if one exists, add any new permissions to it. If there's a rule with identical path pattern but different outcome, and no overlapping permissions, it needs to coexist, since rules can't mix "allow" and "deny" for different permissions, and we'll never have both rules apply to the same request since the permissions are disjoint.

---

Thinking more long term about this problem and how to clearly express to the user what is allowed and denied, and let the user easily change that:

Someone today mentioned the idea of splitting up rules which cover multiple permissions into discrete rules for each permission, and if there's ever an overlap with another rule, it's trivial to adjust the path pattern of a rule to remove the identical pattern variant (or remove the rule entirely) with no side effects on other permissions.

And as mentioned above, Gustavo discussed only ever having at most one rule for a given path pattern, and being able to set individual permissions as allowed or denied for that path pattern.

It sounds to me like the way to fully disambiguate situations of rule overlap is to forego the idea of discrete rules where rule X allows r|w access for pattern variants a,b,c and might overlap with some other rule Y which allows r|x access for pattern variants c,d,e. Instead all we (and the users) care about is the final rule state:

• read: allow a,b,c,d,e
• write: allow a,b,c
• execute: allow c,d,e

Then, if a user wants to add or remove a permission for a particular pattern variant, they can do so without conflicts and without any side effects on other pattern variants or permissions.

I would propose that whenever a user replies to a prompt or creates a new rule manually, the contents of that new rule are inserted into the ruleset, and if there's no conflict, the rulestate reflects the old state plus the new rule, but doesn't afterwards continue to distinguish the contents of that new rule as a discrete unit. If there's a conflict while inserting it, then it's easy to reply with exactly which pattern variant and permissions conflict, and the user can adjust accordingly.

(I'm not sure if this improves the situation where the user is trying to reply with a carefully carved-out set of permissions and pattern variants, so perhaps a change to the format for replies/rules would be required here, or else the client should create multiple rules individually or send a reply containing multiple disjoint variant/permission combinations.)

Regardless of whether we make a change like this, Innes and I discussed how storing the internal rule state in a trie would make computing precedence much simpler and faster, and make it much easier to reason about the state of the permissions granted. I think if we exposed a very similar structure of internal rules to the user in the Security Center, they'd be much better able to add/remove/adjust rules without conflict or confusion, and know exactly which rules apply to a given path, in clear precedence order in the tree depth, and also be able to reason about the rule state without needing to provide a particular path pattern (though a path can still match multiple parallel branches of the tree).

The thought is then that if we have a trie of pattern variants for each permission which we use internally for path matching because of its simplicity, wouldn't it be nice to just expose that same tree of pattern variants to users directly, rather than giving them a list of potentially-overlapping rules to parse through? The tree needs to be separate for each permission internally, but we can overlay these trees for all permissions in the UI, and let users modify permissions for particular pattern variants, much like Gustavo proposed for path patterns, but without the potential for conflicts or overlaps.

What do you think of this @sminez and @juanruitina?

[ernestl]

Changed milestone to 2.67, this needs more time.

[juanruitina]

I agree with modifying the existing rule for identical path patterns. I can't help worrying mainly about this "special case", as it will probably be the most common.

I need to ask now: To what extent is allowing braces in the custom path ({foo,bar}) making our lives harder? No consumer platform allows this kind of advanced rule, far from it, and I wonder if it's driving us into overcomplicated UX. What is the rationale for allowing it? What are the benefits over users simply adding a couple separate rules to achieve the same goal?

Regardless, it's becoming increasingly clear that we will need to show status or conflicts dynamically in the prompt, either for disabling permission checkboxes for which there are rules already (and this will be affected by which path the user has selected or provided) or giving user actionable info on conflicts (even if we only let them handle them in the Security Center). The user shouldn't need to wait to submit to get info on those conflicts. I would be very much in favour of prioritising this.

As for how to visualise the tree in the Security Center, I would need to get a better understanding of rule conflicts first. But, keeping in mind default bias, it might be preferable to keep things simple there and perhaps flag conflicts subtly, as it's likely that most users will stick to default options. It might be a bit too soon to figure this out. The user already has the option to remove all rules, in case this is blocking them.

(And I'll defer to you both on how this all should be handled/stored by snapd)

[olivercalder]

Discussed this with @pedronis, we agreed on a similar approach which should meet the following goals:

1. There can only be one rule for a given path pattern
   1. New replies/rules regarding an existing path pattern will be merged into the existing rule, or throw an error if there's a conflict
   2. Internally, snapd should be able to look up rules by path pattern as well as ID
2. Rules need to support having different outcomes for different permissions (e.g. "constraints": {"path-pattern": "/path/to/{foo,bar}", "permissions": {"read": "allow", "write": "deny"}})
3. We should decouple the internals of replies and rules, since these are not really the same thing anymore
   1. We can create/modify rules with different outcomes for different permissions (e.g. "permissions": {"read": "allow", "write": "deny"}
   2. Replies still take the form of "outcome": "allow"|"deny", "permissions": ["read", "write", "execute"] since they concern a particular request which we want to allow or deny
   3. If a reply results in a rule conflict error, the prompting client could (in the future) present the list of permissions for relevant rules along with sliders with "allow", "prompt", and "deny" options, and can then use the POST /v2/interfaces/requests/rules/{id} endpoint to modify the existing rules as needed
4. Path patterns with groups are only the result of users' custom patterns, and are the source of all the problems around conflicts and overlapping rules
   1. We still want to support groups in path patterns, as they can be very useful and convenient
   2. We can't have two identical pattern variants with different outcomes for a given permission
   3. We don't mind two identical pattern variants with the same outcome for a given permission, as prohibiting this can result in situations where one is forced to disentangle groups and permissions and create several rules to avoid conflict
5. The internal ruleset should be the "compiled" product of all the discrete rules (not dissimilar to the current permission tree, though hopefully optimized further)
   1. That "compiled" ruleset is solely used to determine whether paths are allowed or denied, rather than looking at individual rule internals
   2. When adding new rules (directly or via replies), we check that there are no identical pattern variants which have conflicting outcomes for a given permission

So the big takeaways are:

• Rule structure will change somewhat to accommodate different outcomes for different permissions @juanruitina @sminez @d-loose
• Reply structure will stay the same
• Organization of rules within snapd will change more dramatically, but that's an implementation detail

[sminez]

Thanks for the update on this @olivercalder! The changes you and @pedronis are proposing here look good to me 👍

One thing I want to double check and clarify though: when you say the rule structure needs to change to accommodate different outcomes for different permissions, are you meaning the structure that we are currently pulling in the security center? Or just the internal representation within snapd? I would have thought that the client facing rule structure would be able to remain unchanged but maybe I'm missing something 🙂

[olivercalder]

One thing I want to double check and clarify though: when you say the rule structure needs to change to accommodate different outcomes for different permissions, are you meaning the structure that we are currently pulling in the security center?

Yes this includes the structure in the security center as well, since rules will support multiple different outcomes for different permissions in the same rule, so the security center needs to reflect that if it wants to display rules accurately. That's the tough part, as this is a breaking change.

This is the current structure:

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"`
}

type Constraints struct {
        PathPattern *patterns.PathPattern `json:"path-pattern,omitempty"`
        Permissions []string              `json:"permissions,omitempty"`
}

This will be changed to be:

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"`
        Lifespan    prompting.LifespanType `json:"lifespan"`
        Expiration  time.Time              `json:"expiration,omitempty"`
}

type Constraints struct {
        PathPattern *patterns.PathPattern  `json:"path-pattern,omitempty"`
        Permissions map[string]OutcomeType `json:"permissions,omitempty"`
}

The difference being the outcome is removed from the Rule, and the permissions list is changed to a permissions map, where the value for a given permission is its outcome.

So the question is how best to do this. I think we could potentially change the name of the "permissions" field for the new map so it doesn't conflict with the old list, and make the "outcome" field in the rule optional. But you're much more of an expert on API decisions @sminez so I'd value your input about this :)

[sminez]

since rules will support multiple different outcomes for different permissions in the same rule

I think I'm not following or missing something here. My understanding was that we have (or are aiming to have) two representations of the rule state:

• user facing rules (user): these are a direct representation of the rule as created by the user and map a single path pattern (possibly containing braces) to one or more permissions (read, write, execute) and a single outcome (allow, deny)
• internal rules (internal): these are used for matching against prompts as they come in and map a single path pattern without braces to a single permission and outcome

If that is correct (and maybe this is where I am wrong?) it is a lossless conversion from user rules to internal rules if we expand out the user rules over bracing & permissions and then dedup the resulting rule set, no? In the case of the user wanting to update or remove a rule, this expansion / compilation step should be sufficiently fast that simply regenerating the internal state removes a large amount of book keeping.

In the case of error reporting when rules collide, if you also tag each internal rule with the set of user rules that expand to it you can use that to lookup the details of the user rule to report to the client for displaying the error.

[olivercalder]

We had a quick live chat about this to get on the same page :)

The distinction is that currently each rule has a single outcome and a list of permissions to which that outcome applies. But we want each rule to potentially have different outcomes for different permissions, all of which are associated with a single path pattern.

If we have an existing "allow" ["read"] /foo/bar rule and we then reply with "allow" ["read", "write"] /foo/bar, it's easy enough to merge the reply into the existing rule, ending up with a single rule which is "allow" ["read", "write"] /foo/bar.

The same thing can be done if the existing rule and new reply have the same path pattern but disjoint permissions, so long as they have the same outcome. If we have an existing "allow" ["read"] /foo/bar rule and we then reply with "allow" ["write"] /foo/bar, we can still modify the existing rule to end up with a single rule which is "allow" ["read", "write"] /foo/bar.

If we have an existing "allow" ["read"] /foo/bar rule and we then reply with "deny" ["read", "write"] /foo/bar, the allow read and deny read are clearly in conflict, and we throw an error and never let two rules like that coexist.

The problem is if we have an existing "allow" ["read"] /foo/bar rule and we then reply with "deny" ["write"] /foo/bar. Under the current system, we can't have a rule which allows one permission but denies another, even though there's no conflict between allowing read and denying write, for example. But Gustavo rightfully pointed out that it's confusing to have multiple rules pertaining to the same path pattern. So we want to support having a single rule for a given path pattern, and that rule having something like {"read": "allow", "write": "deny"}.

So regarding your thoughts above:

we have (or are aiming to have) two representations of the rule state

We really have "three" representations of the rule state:

1. The bodies of prompt replies, sent by the prompting-client, which are still "allow" or "deny" along with a list of permissions
2. The rule bodies themselves, as retrieved by the security center (or when creating/modifying rules directly, and also stored on disk), which will no longer have a single outcome for a given rule, but instead have per-permission outcomes
3. The internal "ruleset" within snapd, which is a tree designed to make matching paths and determining rule precedence fast and simple, and is purely in-memory and never serialized over the API or to disk

[olivercalder]

The remaining problem I see is how to deal with different lifespans.

For example, if we have an existing rule which has "allow" "forever" ["read"] /foo/bar, and we reply with "allow" "timespan" "5s" ["write"] /foo/bar, what the resulting rule either needs to be:

• "allow" "forever" ["read", "write"] /foo/bar, which elevates the write duration to longer than the user specified, or
• "allow" "timespan" "5s" ["read", "write"] /foo/bar, which downgrades the read duration to 5 seconds, rather than forever, which is an unintended side effect

I think the best solution is to decouple the lifespan (and duration) from the rule and move it to per-permission as well. This adds slightly more complexity than moving the outcome to per-permission, but I think it's the only way to maintain correctness of rules without side effects, and while only having at most a single rule for each path pattern.

So I think the Rule struct needs to be changed a bit more:

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"`
        Permissions map[string]*PermEntry  `json:"permissions"`
}

type PermEntry struct {
        Outcome    prompting.OutcomeType  `json:"outcome"`
        Lifespan   prompting.LifespanType `json:"lifespan"`
        Expiration time.Time              `json:"expiration,omitempty"`
}

type Constraints struct {
        // Constraints will vary by interface, hence the dedicated sub-struct...
        // But is this actually helpful/necessary vs just having a PathPattern
        // field directly in the Rule struct? It makes marshalling harder...
        PathPattern *patterns.PathPattern `json:"path-pattern"`
}

In this way, different permissions can have different lifespans. This does make rule expiration more difficult, as one needs to check through all the permissions to see if all are expired before knowing a rule as a whole is expired, but I think we can work out something reasonable here.

How does this sound to you @pedronis and @sminez ?

[olivercalder]

As this PR is currently self-contained, accomplishes what it was initially set out to accomplish, and is a stepping stone to the one-rule-per-path-pattern world we'd like to live in, I'm going to leave it as is and open a follow-up PR with the work to make rules have unique path patterns. That change requires a deeper rework of rule internals.

[pedronis]

thanks, some suggestions to simplify things a bit

[pedronis]

would it make sense to check this early in the fuction as well then?

[olivercalder]

The motivation for waiting until this point in the closure is so we can include every conflict with every variant. The RuleConflictError wraps these in addRuleToTree, so the error response has all the information for the client. I think if we checked any earlier, we would miss conflicts.

[pedronis]

setting this partially up could be done before the previous "if", same with filling newVariantEntries[variantStr]

[olivercalder]

Ah I see, yes the existing logic is convoluted, relies on lots of checks and early returns. I've cleaned this up in the latest commit. Moved this bit up before the for loop, and consolidated the two for loops together with fewer unnecessary checks.

[pedronis]

then this path could be incorporated also in the previous if

[pedronis]

this is not stored to disk, right?

[olivercalder]

Correct. The rule tree uses variantEntry as the leaves, and this whole tree is in-memory only, used for efficient lookup and precedence decision (since precedence between two rules depends on the particular pattern variant which matched a requested path).

[pedronis]

thanks, the code is cleaner/easier the follow now

[bboozzoo]

does existingEntry.RuleIDs need to be updated someplace as well?

[olivercalder]

We replace existingEntry with newEntry in its entirety at the end if we don't hit any conflicts. By leaving existingEntry unchanged, we can roll back and leave everything unchanged by simply not replacing existinglyEntry with newEntry.

[olivercalder]

Rebased on master to pull in test improvements

[bboozzoo]

LGTM

[olivercalder]

Failed tasks:

• google-distro-1:debian-sid-64:tests/main/snap-run --- problem with strace invocation on debian, unrelated to PR
• openstack:fedora-40-64:tests/main/auto-refresh-gating
• google:ubuntu-24.04-64:tests/main/document-interfaces-url --- Sergio fixed on master
• google:ubuntu-24.04-64:tests/main/upgrade-from-release --- Andrew fix imminent

Failed prepare:

• openstack:fedora-40-64:tests/main/classic-custom-device-reg
• google-core:ubuntu-core-20-64:tests/main/ --- store flakeyness, snap install hung fetching pc-kernel
• google-core:ubuntu-core-24-64:tests/main/ --- unsquashfs failures, I believe this is fixed on master
• google-core:ubuntu-core-24-64:tests/regression/ --- same
• google-core:ubuntu-core-24-64:tests/smoke/ --- same
• google-pro:ubuntu-fips-22.04-64:tests/fips/ --- problem with the FIPS build, unrelated to PR

Failed restore:

• google-distro-1:fedora-39-64:tests/main/component-from-store
• openstack:fedora-40-64:tests/main/classic-custom-device-reg
• openstack:fedora-40-64:tests/main/component-from-store