search

canonical / desktop-security-center

GNU General Public License v3.0
13 stars 5 forks source link

Prompting client defaults result in rule conflict #74

Open olivercalder opened 1 month ago

olivercalder commented 1 month ago

Is there an existing issue for this?

Describe the bug

The new default for the prompting client when prompted for access to a directory is to grant access to everything in the home directory's top-level folder which contains the requested directory -- or the requested directory itself, if that directory is a top-level directory.

Screenshot from 2024-09-18 19-22-16

Identically, the new default for the prompting client when prompted for access to a file is to also grant access to everything in the home directory's top-level folder which contains the requested file. Additionally, the default for the prompting client is to grant "read" access whenever "write" access is granted.

Screenshot from 2024-09-18 19-22-43

Herein lies a problem: often, applications (such as firefox) request read access to a file's parent directory before requesting write access to the file itself. However, if we answer with the defaults and "Allow always", we'll end up with a rule granting read access to ~/Downloads/**, for example. Then, when the write prompt appears, if we attempt to again "Allow always", the client will send back a reply which would create a rule granting read and write access to ~/Downloads/**. This conflicts with the existing rule, since they have identical patterns and both have the "read" permission.

Screencast from 2024-09-18 19-19-33.webm

Additionally, as we can see in the screencast, there's no error message when this occurs in the default window (as opposed to the "More options" window). Clicking "Allow always" appears to do nothing, and it takes expanding into the "More options" screen, clicking "Custom path pattern", and inputting the identical pattern before we can see that the problem is a conflicting rule.

Given the current defaults, both the client and snapd are behaving as expected (though an error message would be nice when the non-custom path pattern options result in a rule conflict). Thus, we need to adjust the default options so that a conflict is unlikely to occur, and/or adjust the behavior of the client (or possibly snapd) when a non-custom path pattern results in a conflict.

Here are a few possibilities I can think of (though there are many more):

  1. Default to granting read+write+execute all the time (I don't love this, but it's simplest)
  2. Default to only granting the requested permission (I prefer this to (1))
  3. If the client sends back one of the standard replies and sees a rule conflict, either tell the user to un-check the box for the conflicting permission $^1$ or silently un-check it and retry the request
  4. If the client sends back one of the standard replies and sees a rule conflict, it could tell snapd to modify the existing rule to cover the additional permission(s), and this should satisfy the outstanding prompt
  5. Add a new field to the reply which tells snapd that it should clobber any existing rules which have a conflicting path pattern, permissions which are a subset of the replied permissions, and an identical outcome -- then snapd deletes any such rules and results in a single unified rule, or returns an error if any conflicting rules have differing outcomes or permissions which overlap but are not a subset of the replied rules

$^1$ : the originally-requested permission should never have a conflict, since the conflicting rule would have already satisfied it, and multi-permission prompts are very rare

Steps to reproduce the behavior

  1. Purge all rules for firefox related to ~/Downloads using the security center
  2. In firefox, right click on an image and click "Save image as"
  3. Click "Allow always" without changing the defaults (this is for the read request for ~/Downloads)
  4. Choose a filename, click save
  5. Click "Allow always" without changing the defaults (this is for the write request for the file in ~/Downloads)

Expected behavior

I would expect that the defaults would not result in a rule conflict, and if they do, I would expect either an error message explaining that there's already an existing read rule, so the "Also give Read access" should be unchecked, or I'd expect the client to automatically un-check that box and retry the reply.

Ubuntu release

24.04 LTS

What architecture are you using?

amd64

App or apps where you encountered the issue

firefox

Additional context

No response

olivercalder commented 1 month ago

It's possible that this is the explanation for #70, but I'm not certain. Seems like the reporter there tried all options and none worked, and that they were getting multiple (but not infinite) prompts, which would suggest they were clicking "Allow once".

sminez commented 1 month ago

Failing to surface the error message to the user is something we need to address ASAP on the client side. @juanruitina @d-loose I've raised this internally previously but we've not tackled it yet as we thought rule conflicts would only come about from custom paths. We should prioritise sorting out a way of displaying general errors to the user.

Regarding this particular issue however, I think we need to have a design conversation about this with the wider team as the underlying issue looks like it is a mismatch between user intent and how snapd thinks about replies to prompts. That said, I also suspect that there is an internal inconsistency within snapd somewhere which is the source of how we get into this situation in the first place (see below).

User intent vs snapd rule generation

From a user POV it is reasonable to view replying to a prompt as expressing the access you would like the program to have, and for this to be cumulative over replies to multiple prompts. They are replying with permissions they wish to explicitly allow or deny, but how you describe the rules that snapd is then generating sounds like the rule is always an explicit allow / deny for all permissions available?

Taking this issue as our example, I would expect this sequence of replies to work as follows:

  1. "allow Firefox read access to ~/Downloads/**" --> create a rule allowing read
  2. "allow Firefox read and write access to ~/Downloads/**" --> create a new rule allowing both read and write that makes rule 1 obsolete
    • ideally removing rule 1 but that is an optimisation

But instead my understanding of how you describe it above is that we have something like this:

  1. "allow Firefox read access to ~/Downloads/**" --> create a rule allowing read and explicitly denying write and execute
  2. "allow Firefox read and write access to ~/Downloads/**" --> rejected as conflict because rule 1 explicitly denies write

Confusingly to me, it sounds like you are also saying that the following is expected to work?:

  1. "allow Firefox read access to ~/Downloads/**" --> create a rule allowing read and explicitly denying write and execute
  2. "allow Firefox write access to ~/Downloads/**" --> create a rule allowing write and explicitly denying read and execute(???)

I'd like to confirm with @pedronis to be sure, but I don't see any risk in having the behaviour I have outlined in the first example instead. In that case the rule is partial and we only store the explicit response from the user (we do not infer "deny X always" or "allow X always for permissions not in the reply), meaning that rule 1 simply doesn't apply when the second prompt comes in requesting write access. I don't have enough familiarity with the code in snapd to say for sure but I suspect that this is at least partly true already (see below).

Potential snapd inconsistency

The thing I really don't understand in this scenario is how we get the second prompt at all. If rule 1 is consistently being interpreted by snapd as "allow read access but deny write & execute access for ~/Downloads/**" then why does snapd emit a prompt asking the user if they wish to allow write access to a path within ~/Downloads?

This conflicts with the existing rule, since they have identical patterns and both have the "read" permission.

My understanding of how you describe things above is that the rules within snapd are interpreted differently for the purposes of applying them to prompts and determining if they overlap. We are getting the second prompt because rule 1 only explicitly allows read access, this makes sense to me. But then creating a rule that is a strict superset of rule 1 ("allow read and write") is treated as conflicting because at that point the existing rule is being interpreted as "allow read, deny write, deny execute".

Is that correct? If so then I think that this needs revisiting as it's really quite counter intuitive that "allow write" as the response to the second prompt is valid, but "allow read & write" is invalid because a pre-existing rule already allows read...

juanruitina commented 1 month ago

I think user intent is pretty clear here. My expectations match @sminez' and I think that's is how it should work:

Taking this issue as our example, I would expect this sequence of replies to work as follows:

  1. "allow Firefox read access to ~/Downloads/**" --> create a rule allowing read
  2. "allow Firefox read and write access to ~/Downloads/**" --> create a new rule allowing both read and write that makes rule 1 obsolete
  3. ideally removing rule 1 but that is an optimisation

(I do think 3 should happen)

Or out of @olivercalder's options:

  1. If the client sends back one of the standard replies and sees a rule conflict, it could tell snapd to modify the existing rule to cover the additional permission(s), and this should satisfy the outstanding prompt

(Not sure I understood your option 5)

The user is just expanding the permissions previously given, which is a perfectly normal thing to do and should happen seamlessly. We should not raise any errors because of system's quirks, it's not even an override from their perspective.

d-loose commented 1 month ago

Failing to surface the error message to the user is something we need to address ASAP on the client side. @juanruitina @d-loose I've raised this internally previously but we've not tackled it yet as we thought rule conflicts would only come about from custom paths. We should prioritise sorting out a way of displaying general errors to the user.

Ideally, since the prompt UI is asking something from the user (the user doesn't take the initiative to setup a rule here), we should make sure that none of the actions they can take result in an error (with the exception of providing a custom path). But I suspect there could always be some edge cases we didn't think about, so having a mechanism to display errors to the user as a last resort is definitely something we should implement.

Taking this issue as our example, I would expect this sequence of replies to work as follows:

1. "allow Firefox **read** access to `~/Downloads/**`" --> create a rule allowing read

2. "allow Firefox **read** and **write** access to `~/Downloads/**`" --> create a new rule allowing both read and write that makes `rule 1` obsolete

   * ideally removing `rule 1` but that is an optimisation

That's my understanding as well.

But instead my understanding of how you describe it above is that we have something like this:

1. "allow Firefox **read** access to `~/Downloads/**`" --> create a rule allowing read _and explicitly denying write and execute_

I don't think that's correct - with a rule that only grants read access to ~/Downloads/** any action that involves writing to that directory will still generate a prompt, so the rule doesn't explicitly deny write access.

2. "allow Firefox **read** and **write** access to `~/Downloads/**`" --> rejected as conflict because `rule 1` explicitly denies write

I think it's only rejected because the path patterns are completely identical. However this case should be reasonably straightforward to solve in snapd, since it doesn't involve finding the intersection of patterns. An option would be something like 5 in @olivercalder's original post, though I'm not sure what the best way of handling this is. Giving the user the explicit choice to replace or merge with the existing rule is the most transparent option, but also increases the amount of time and effort the user has to spend on the prompt.

sminez commented 1 month ago

I don't think that's correct

I agree that this does not correct for how the rule is applied (otherwise we would never see the second prompt) but it does look like this is how it is treated for the purpose of determining if rules conflict. If it was as simple as the patterns being identical then I would expect the pure "allow write" rule to also be rejected but its not.

The behaviour outlined in 5 from @olivercalder's original description of the issue feels like it should be the default. Having that be something the client opts in or out of seems unnecessary.

olivercalder commented 1 month ago

I should clarify: when I say "conflict", I mean an existing rule with an identical path pattern and one or more matching permissions, regardless of whether the outcome is the same or different.

But instead my understanding of how you describe it above is that we have something like this:

  1. "allow Firefox read access to ~/Downloads/**" --> create a rule allowing read and explicitly denying write and execute
  2. "allow Firefox read and write access to ~/Downloads/**" --> rejected as conflict because rule 1 explicitly denies write

So this is not correct. Rule (1) creates a rule allowing read, that's all. Rule (2) is rejected because it also includes the read permission, and has the same path pattern as (1), so there is a "conflict" with an existing rule, even though the new and existing rules are identical with respect to the read permission.

There are a few principles on the snapd side which might clarify what's going on here:

  1. Never allow multiple rules to exist whose precedence cannot be decided (e.g. since they have identical path pattern and overlapping permissions)
  2. Never silently modify rules without telling the user (e.g. prune/clobber)

So to apply this to @sminez 's expectations:

Taking this issue as our example, I would expect this sequence of replies to work as follows:

  1. "allow Firefox read access to ~/Downloads/**" --> create a rule allowing read
  2. "allow Firefox read and write access to ~/Downloads/**" --> create a new rule allowing both read and write that makes rule 1 obsolete
    • ideally removing rule 1 but that is an optimisation

We can't create the new rule allowing both read and write which makes rule 1 obsolete, since it is impossible to determine precedence between these rules. If one of those rules were to be modified to change the outcome to "deny", we'd have a true conflict. Snapd stores rules in a map per permission, so we can't have two rules with identical patterns and any overlapping permissions. To change this would require changes to the rule backend. Even if this were technically supported, I fear it would be confusing to users if they repeatedly create the same rule, thinking it will do something, when it actually won't. I think it's clearer to let the user know that that rule already exists, so the problem is that the rule doesn't do what they expect, rather than that the rule doesn't actually exist.

We also wouldn't want to remove rule 1. If the rule had been in place, and then the user creates rule 2 which makes that rule obsolete, but then deletes or modifies rule 2 to make rule 1 no longer obsolete, it's unclear whether rule 1 should still exist or not. In general, we decided early on that snapd should never silently prune/merge rules, as the client/user doesn't know this happened and could be confused.

The behaviour outlined in 5 from @olivercalder's original description of the issue feels like it should be the default. Having that be something the client opts in or out of seems unnecessary.

I am open to pruning/merging if the client explicitly says it expects this, hence my suggestion that the client says to snapd "clobber if necessary". The problem is when a rule exists which can't be easily clobbered. For example, let's say a rule exists which allows read and execute access to ~/{Downloads,Pictures}/**. Then, the client is asked for write access to a file in ~/Downloads, and they reply with a response which would create a new with read and write access to ~/{Downloads,Documents}/**. I don't see any way to modify the existing rule or the new rule to make them compatible without losing information.

Either we end up with:

  1. read|exec ~/Pictures/**, losing exec permission for ~/Downloads/**
  2. read|write ~/{Downloads,Documents}/**

or

  1. exec ~/{Downloads,Pictures}/**, losing read permission for ~/Pictures/**
  2. read|write ~/{Downloads,Documents}/**

or

  1. read|exec ~/{Downloads,Pictures}/**
  2. read|write ~/Documents/**, losing write permission for ~/Downloads/**

or

  1. read|exec ~/{Downloads,Pictures}/**
  2. write ~/{Downloads,Documents}/**, losing read permission for ~/Documents/**

So we have no choice but to return an error in this case, since we don't support identical path patterns for the same permission.

It's true, though, that this kind of situation can only occur when the client is responding with a path pattern which contains groups over possible variants. With the pre-populated responses, it should always be easy to reduce the new rule which would be generated by the reply so that it will not "conflict" (again, identical path pattern variant and overlapping permissions) with existing rules. I think that's the approach I'd prefer. The question is whether the client should do this (this is my suggestion (3) from above, I think this is clearer to the client if anything goes wrong), or if snapd should do this because the client asked it to (this is my suggestion (5) from above, and also solves the problem, but it's a bit less clear to the client exactly what was pruned about the new rule, and an error could still occur which the client will know less about how to handle).

I still prefer to avoid snapd modifying rules implicitly. So in the original case in question, I'd prefer that the client modify the new rule to remove the write permission, rather than to snapd doing so, or snapd deleting the old rule and retaining only the new rule.

Otherwise, snapd needs to be able to tell whether the existing and new rules fall into a case like this or not:

let's say a rule exists which allows read and execute access to ~/{Downloads,Pictures}/**. Then, the client is asked for write access to a file in ~/Downloads, and they reply with a response which would create a new with read and write access to ~/{Downloads,Documents}/**. I don't see any way to modify the existing rule or the new rule to make them compatible without losing information.

That's my reasoning for wanting the client to be in charge of how rules should be adjusted to avoid conflicts. So what I really favor is either my original suggestion (3) or (4):

  1. If the client sends back one of the standard replies and sees a rule conflict, either tell the user to un-check the box for the conflicting permission 1 or silently un-check it and retry the request
  2. If the client sends back one of the standard replies and sees a rule conflict, it could tell snapd to modify the existing rule to cover the additional permission(s), and this should satisfy the outstanding prompt

Does this answer your questions about snapd behavior @sminez, @juanruitina, @d-loose ?

olivercalder commented 1 month ago

Thanks for the discussion today, and for your patience, it's a complicated topic and we all have slightly different opinions of what is currently happening and what should be happening.

To summarize a bit, the general consensus is that the idea of a "conflict" should apply to situations like (WLOG) "tried to reply to a prompt in a way which would create a rule about a particular path pattern, but there's already a rule for that path pattern, and it says a permission should be denied but the new rule says it should be allowed".

So we want replies to be idempotent, and intent-based. If the user replies with "allow read and write for /x/y/z" but there's already a rule which states "allow read for /x/y/z", then snapd should happily adjust the existing rule to add the write permission as well, and there's no conflict.

If the user replies with "allow read and write for /x/y/z" but there's already a rule which states "deny read for /x/y/z", then we have a conflict, and we should present a pop-up to the user saying "You've already established a conflicting rule for /x/y/z, so you can adjust the permissions for it". We can have a slider for each permission, and the slider can be set to "deny", "prompt", or "allow", and they can adjust as desired and click ok. Something like this, for the situation described above:

Permission "deny" "prompt" "allow"
read X
write X
execute X

If the user instead replies with "allow write for /x/y/z" and there's already a rule which states "deny read for /x/y/z", this is fine, there's no conflict, and we end up with the state shown in the table above.

Thus, fields which are omitted from the reply are treated as unchanged by snapd, fields in the reply which are identical to the existing rule are left as unchanged as well, and we only throw a rule conflict error if there's a field in the reply which directly contradicts an existing rule.

A rule being set to "prompt" in the UI pop-up actually means that permission is omitted from the rule, and so the implicit behavior is to prompt whenever it comes up, assuming there's no other rule which matches the permission. And we don't consider a permission set to "prompt" to be a conflict if the user tries to set that permission to "allow" or "deny" in the future.

To summarize, rather than a rule being a discrete (and rather arbitrary) combination of path pattern and permissions created/modified as a unit at a particular time, and there could be multiple rules with the same path pattern so long as the permissions don't overlap, we instead want to think about a rule being identified uniquely by its path pattern, and the single rule for a particular path pattern contains all the information about the permissions which are allowed, denied, or yet unspecified ("prompt", in the UI, if that's the direction we go).

To be clear, this is not how snapd operates at the moment. Snapd currently organizes rules according to user -> interface -> snap -> permission -> path pattern variant, which points back to the rule ID. So we need to change this to be user -> interface -> snap -> path pattern variant, where the rule identified by that path pattern variant is in charge of all current and future permissions for that path pattern.

It will take some work to get snapd to operate as described above, but most of these changes should be strictly internal implementation details. From the prompt API point of view, I think the only change would be to the rule conflict error value, since the concept of multiple conflicting rules will no longer exist $^1$ , nor the distinction by permission, so we can instead just return the current permission state for that path pattern.

There may be more substantial changes to the way we create or patch rules directly via the rules API, since we need to support setting some permissions to allow and some permissions to deny. This will also need to be reflected in the structure of rules retrieved over the API, so this will require discussion with and changes to the Security Center as well.

$^1$: this isn't actually true, as it may be that the existing rule has multiple variants (due to groups) and it's only one of the variants which exactly matches the path pattern in the reply. Or the reply may be a path pattern with groups which expands to multiple variants, one or more of which conflicts with one or more existing rules. But cases like these should only occur if the user is creating custom path patterns, and we can for now just point them to the security center to try to fix whatever they've done. But the error value does likely need to include the conflicting path pattern(s), and there may be multiple.

juanruitina commented 1 month ago

So we want replies to be idempotent, and intent-based. If the user replies with "allow read and write for /x/y/z" but there's already a rule which states "allow read for /x/y/z", then snapd should happily adjust the existing rule to add the write permission as well, and there's no conflict.

I think addressing this and hiding the "Also give Read access" checkbox as @local-optimum suggested is a great quick fix for now. Conflicts would only happen when the user plays with the options under "More options...". I do think there's value in the "Also give Read access" and "Also give Write access" checkboxes and would like to bring them back later on, perhaps showing them only if there's not a preexisting rule for either.

Thus, fields which are omitted from the reply are treated as unchanged by snapd, fields in the reply which are identical to the existing rule are left as unchanged as well, and we only throw a rule conflict error if there's a field in the reply which directly contradicts an existing rule.

I'll explore what that error message could look like. I do think that the timing of a user's decision matters. There's intent in the user now deciding for the current task something different to what they previously decided in a different context. For instance, if the user intentionally checks "Also give Read access" or "Also give Write access", I'm not sure it's necessary to show a conflict error if there's a preexisting deny rule with an exact path match (as long as the checkboxes are not checked by default): they are explicitly stating what they want now.

We can inform of preexisting rules, but by blocking the user's flow by showing conflict errors or asking them to fine-tune allow/deny/ask for every permission we might be going to far. Double-checking like that might be adding too much friction, the user is being deliberate already. The sliders you are suggesting for the UI sound a bit too complex to be handled in a prompt, and maybe also in the Security Center. I'm happy to put some time into mocking this up, to see what both options could look like.

One question, how would this play with rule duration? I understand "once" is not a problem, but what about "until logout/this session" vs "always"? Could there be conflicts there?

olivercalder commented 1 month ago

One question, how would this play with rule duration? I understand "once" is not a problem, but what about "until logout/this session" vs "always"? Could there be conflicts there?

This is a great question. Let's say this is the hypothetical:

Then I think there are some relatively simple/clear cases:

When it comes to other lifespans, it gets a bit more complicated:

Thinking about it a bit more though, I think we'll still end up with cases where a user will want to have different lifespans for rules which have overlapping path pattern variants and permissions but neither is a subset of the other. And in these cases, I think we really just need to let these coexist.

So this suggests to me that we need to allow rules with identical path pattern variants, so long as they have the same outcome. However, if a path pattern is identical to an existing rule (as opposed to just one of its variants), I think we can instead replace the existing rule. Unless, in the example above, lifespan-1 was forever and lifespan-2 was timespan. But in that case I think we can let the rules coexist, again so long as the outcomes ("allow" or "deny") are the same. So perhaps that's really the simplest solution.

Then my worry is that the user could create many rules and then be confused about what exactly is allowing/denying the permissions, since there could be many near-identical rules with slightly different lifespans or combinations of permissions, for example. But I think this is unlikely, and we can do some work in snapd/security center to highlight for users what's going on in complicated situations like that.

So for now, I think I favor a structure like this within snapd:

user -> snap -> interface -> map[variant]variantEntry

type variantEntry struct {
    map[prompting.IDType]permissionEntry
}

type permissionEntry struct {
    map[permission]outcome
}

So when a new rule is added, we look through the map in the variant entry until we find a non-expired rule (if one exists), and if one does exist, we check that the new rule has the same outcome as the existing rule for the relevant permissions. As long as it does, we're free to add the new rule ID to the map from IDs to outcomes.

We only throw a conflict error if the client asks to add a new rule with an identical path pattern variant to an existing rule but with a different outcome for one or more permissions. Then we can present the user with the information and ask them what to do.

Then I think less change is required to the structure of the rules themselves. And it's kind of an implementation detail whether snapd combines the new and existing rules or if it just adds a new "duplicate" rule.

olivercalder commented 1 month ago

What it really comes down to is a gnarly case where we have something like these existing rules:

  1. allow read for /foo/bar/{a,b}
  2. deny read for /foo/bar/{c,d}

Then we reply with a response which would create a new rule:

This conflicts with (2). If we change the outcome to deny, then we conflict with (1). If we modify the path pattern of any of these rules, we end up changing the semantics of some otherwise non-conflicting variant-permission results.

This is further complicated if there are more permissions involved, or more lifespans.

I think the only true way to avoid all this complexity is if we don't have rules with path patterns which contain groups, at least within the rules backend. Then, each pattern variant can have its own permission-outcome map (e.g. "allow read, deny write, prompt for exec"), and any conflicts with an existing rule only concern the exact pattern variant which conflicts, with no impact on other variants which would otherwise still be tied to the same rule as variants of a common path pattern.

That is, when one creates a rule with a path pattern of, for example, /home/me/Pictures/**/*.{png,svg,jpg}, this would actually create three rules with the following patterns:

If there's an existing rule with a pattern which matches any of those variants, that existing rule is updated or left unchanged, if there's no conflict with one of its permission outcomes or with the lifespan, but if there's a conflict, it's much easier to simply present the offending rule with that one variant, rather than all the other variants associated with that rule, which must share the common permissions and outcomes and lifespan if the user decided to change one of those fields for that existing conflict rule.

Then, each pattern variant can uniquely identify a rule, and each can have a unique permission map (e.g. "allow read", "prompt write", "deny execute") and lifespan, and these can be easily merged with new replies if those new replies don't conflict.

So looking at the above example, we could surface to the user that there's a conflict with a rule with pattern /foo/bar/c, and we could present them with the table choosing what permissions they'd like to allow/deny:

Permission "allow" "prompt" "deny"
read X
write X
execute X

and they can adjust the permissions for the existing rule corresponding to only that particular pattern variant accordingly, without any unwanted side-effects. Or, alternatively, if there's a conflict with any of the variants related to a reply, give the user the information to adjust all the variants individually, rather than locking in their previous decision. But not sure if this is a job for snapd or the client to decide what (if anything) to lock in on first pass vs allow adjusting in the event of a conflict.

The obvious problem with this approach is that then there are potentially many rules created as a result of a single reply or rule creation POST, and if the user wants to modify an existing rule, they potentially have to track down all of the rules which were created as the result of that single POST with a path pattern containing groups. But, arguably, it's already the case that the user has to sort through many rules which can have groups and would thus be hard to read, and this would be even harder if we allowed rules with overlapping identical path pattern variants. And if we were to allow rules with path patterns which expand to identical pattern variants, (e.g. with "deny" taking precedence over "allow" for identical variants), then this would be even more challenging, since there could be duplicate variants but they're buried within groups for different rules.

So I think allowing groups in path patterns when replying to a prompt or directly creating a rule are fine, and snapd then creates/modifies separate rules for each variant of the path pattern, so conflict resolution is clear and it's easier to see exactly which rules apply to a path without having to read through any groups in the path patterns. And we get all the nice idempotency benefits for permissions as discussed above, without any of the nasty conflict handling with unintended side effects.

How does this approach sound to you @sminez and @juanruitina compared to the alternatives discussed above and in the previous comment?

sminez commented 1 month ago

@juanruitina and I had a chat about this earlier today and I think the underlying issue to call out with everything discussed above is that we have to different world views that we to satisfy:

  1. On the snapd side, the logic for applying rules needs to be as efficient as possible. This places constraints on what the internal representation of rules can look like.
  2. On the UI side, managing rules needs to be as intuitive as possible. The user should be able to create and manage rules using patterns containing globs and groups where they determine the complexity they want to interact with (create lots of simple rules or a smaller number of complicated rules).

Design and implementation decisions for each side of this shouldn't have to impact the other I don't think, so long as we're careful with how we set things up. I've tried tackling each of the topics individually below as there's a lot being discussed here at this point :sweat_smile:

The tl;dr is as follows:

For more context, see below :slightly_smiling_face:

The gnarly example

We have something like these existing rules:

  • allow read for /foo/bar/{a,b}
  • deny read for /foo/bar/{c,d}

Then we reply with a response which would create a new rule:

  • allow read for /foo/bar/{a,c,e}

As you call out, this clearly conflicts with rule 2. As such I think the only thing we should do in this situation is reject the rule, calling out the conflict (see Surfacing conflicts to the client below). I don't think we (snapd or the prompting-client) should attempt to modify the rule in any way to see if we can make it work.

Rule lifetimes

I think I'm pretty confident in arguing for the lifetime of a rule only being used to determine whether or not it should currently be applied in handling prompts. If rules are in conflict due to the path pattern and permissions they set then that is a definite semantic conflict from the user's point of view and really only they can decide which rule they want to keep and which they want to remove.

Surfacing conflicts to the client

With that in mind, I think that all conflicts should be returned to the client in the same way: the structured error you implemented recently with the semantics of "existing rule A conflicts with rule B that you are trying to create". From the client side we can handle implementing and iterating on the UX of how that conflict is presented to the user for them to review and resolve.

I don't think snapd should attach any assumed semantics to what happens in the case of a conflict arising as this gets fiddly pretty quickly. It'll be a lot easier to iterate on the UX purely from the client side if snapd continues to treat conflicts in the way it does now: the prompt reply (and accompanying rule creation) are rejected with a structured error letting us know which existing rule we are in conflict with.

Exploding out rules

I think your suggestion of expanding /home/me/Pictures/**/*.{png,svg,jpg} to three rules covering each of the braced suffixes sounds like a good way of simplifying the internal representation of the rules and probably the way we want to handle this? The problem you call out (multiple rules being created instead of one) I think is easily addressed if you treat this as an expansion of the "raw" rules into the format that you use for resolution and application along with an ID to track which raw rule each expanded rule was derived from. That way you can report the raw rule to the client when a conflict arises rather than something they didn't actually create.

Ultimately, this all feels a lot like trying to write a little regex / matching engine where the outcome is a tri-state of "allow / deny / prompt" rather than a boolean. While I am now trying really hard to not take a stab at implementing this as a compiled state machine, I think there are probably a number of tricks and techniques we could adapt from that as an approach.