many: support mixed outcomes for permissions in prompting constraints

[olivercalder]

This PR is based on #14538, and is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-32594. It addresses some of the problems discussed in that PR (such as #14538 (comment)), and more broadly in canonical/desktop-security-center#74. CC @sminez @juanruitina.

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 permissions 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 permission 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 simplifying the logic of their callers.

[olivercalder]

TODO:

• Add unit tests for new constraints types
• Add requestrules unit test for partial rule expiration
• Decide on handling of partial- and fully-expired rules when:
  • Getting rule by ID (currently no expiration checks)
  • Getting all rules (current discards fully expired rules, but does not prune expired permissions)
  • Adding rule which conflicts with expired rule permission (currently prunes expired rule permission and removes rule if all permissions are expired)

[codecov]

Codecov Report

Attention: Patch coverage is 95.50562% with 20 lines in your changes missing coverage. Please review.

Project coverage is 78.29%. Comparing base (24a0034) to head (3ad9783).
Report is 68 commits behind head on master.

Files with missing lines	Patch %	Lines
...erfaces/prompting/requestprompts/requestprompts.go	90.00%	7 Missing and 2 partials ⚠️
interfaces/prompting/constraints.go	96.80%	5 Missing and 2 partials ⚠️
interfaces/prompting/requestrules/requestrules.go	96.15%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14581      +/-   ##
==========================================
+ Coverage   78.20%   78.29%   +0.09%     
==========================================
  Files        1151     1158       +7     
  Lines      151396   152573    +1177     
==========================================
+ Hits       118402   119463    +1061     
- Misses      25662    25745      +83     
- Partials     7332     7365      +33     

Flag	Coverage Δ
unittests	78.29% <95.50%> (+0.09%)	⬆️

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]

@olivercalder this needs a rebase now?

[pedronis]

did a pass on about half of this, some initial questions/comments

[pedronis]

can't constraints be used more through the function? and if not, why?

[olivercalder]

The Match and ContainPermissions methods could be moved to the Constraints type instead of the ReplyConstraints type, but they're really about validating that the reply is well-formed, which is specific to ReplyConstraints rather than Constraints. We don't in general have a reason to check whether Constraints match a particular path or set of permissions --- that is the role of RuleConstraints. And further, I don't think it makes as much sense to check whether a rule has an entry for each permission in the list, since those entries could have mixed outcomes which wouldn't have come from a single reply, since replies always have a single outcome.

Basically, my motivation is about keeping the methods about validating replies (ToConstraints, Match, and ContainsPermissions) to ReplyConstraints, so there's no risk of accidentally mis-using them on Constraints in other situations. E.g. one never wants to match an incoming request against Constraints, as incoming requests should only be matched against RuleConstraints, and Constraints must always be converted to RuleConstraints. ReplyConstraints may be matched against existing requests to make sure they satisfy everything which was requested.

As for why replyConstraints.ToConstraints occurs before replyConstraints.Match and replyConstraints.ContainPermissions, the former validates that the reply is well-formed in the basic sense, while the latter two checks that it's semantically valid by satisfying the original request.

Does this address your question? Or is there something else I'm missing?

[pedronis]

is this just a simplicication?

[olivercalder]

Now that rules can have mixed outcomes/lifespans/expirations, it may be the case that (e.g. when reading from disk) one permission has expired and another one has not. Rather than throwing an error when an expired permission is seen, it instead is removed from the rule at a later step, and any non-expired permissions remain.

In particular: ValidateExpiration is only called once, from within RulePermissionEntry.validate(), which is in turn only called once, in RulePermissionMap.validateForInterface(). Before calling entry.validate(), validateForInterface first checks entry.Expired(), and if the permission entry has expired, the permission is removed from the permission map at the end of the function, after ensuring that no other errors occurred.

So not quite a simplification, it's a change in the distinction between expired rules and invalid rules now that rules can be "partially expired" but still valid.

[pedronis]

I'm not sure I understand the comment vs the code, matched doesn't meant that all outcomes are not deny? it seems the comment needs to be clarified/expanded

[olivercalder]

Matched means the path pattern of the rule matched the path of the prompt, and at least one permission from the rule matched at least one permission from the prompt. Whether all or just some of the permissions were matched, and whether each was allowed or denied, is what the other return values of matched, satisfied, denied, err := prompt.Constraints.applyRuleConstraints(constraints) indicates.

But I agree this is rather confusing. matched, satisfied, and denied are not the same types, and there is some implication between each.

I think the complexity comes down to the interaction between the way buildResponse, applyRuleConstraints, and their callers interact. A lot of it is because of the old way these used to work, which is no longer the case, so they can be simplified. I'll work on that.

[pedronis]

couple more comments. I haven't reviewed requestrules.go changes yet, probably would be good to improve the rest first

[olivercalder]

We need to be careful to support both the old and new rule formats during a transition period. The internals should use the new system, but we'll need to map the old format to the new structure, and provide a means of working in the old format over the API.

[olivercalder]

I'd definitely appreciate some feedback from the Desktop team about how best to handle backwards compatibility for the client :)

CC @sminez @d-loose

[olivercalder]

As discussed with Desktop, the plan is for the snapd.dart to replace the explicit constraints format (currently specific to the home interface) with a generic constraints json blob. Then it's up to the client to parse the json blob and handle it according to the interface specified in the rule.

Regarding backwards compatibility, since old-format rules can be converted to new-format rules, the plan is for clients of snapd to map from old-format to new-format and pass that on to downstream clients, so they can immediately begin working with the new format. I believe snapd.dart will be responsible for doing this mapping before passing the rules on to the security center, but I want to check with @sminez on this to confirm.

As for compatibility on the snapd side, since there are currently no clients which send rule contents directly to snapd (e.g. for creating/patching rules directly, as opposed to implicitly creating rules via prompt replies), we aren't concerned with snapd accepting old-format rule contents over the API. Any client which wishes to POST rule contents to snapd should use the new format, as we expect this PR to be merged and snapd to be using the new format well before any clients will wish to do so.

The spec for the changes to the rule format can be found here: https://docs.google.com/document/d/19m27d-YK9TdGCucZyjDzHYsQqICE2RE2yPia7sTg7Ls/edit

[olivercalder]

To clarify a bit: the only change to snapd.dart is that it will pass constraints as a json blob instead of a struct. Additionally, the rule-level "outcome", "lifespan", and "duration" fields will be marked as deprecated, and are preserved (and passed to the client) as a means of handling old-format rules from snapd. Once snapd acts with new-format rules, these fields will be omitted, and the outcome/lifespan/expiration will be included in the values of the permission map, as detailed in this PR and the spec linked above (https://docs.google.com/document/d/19m27d-YK9TdGCucZyjDzHYsQqICE2RE2yPia7sTg7Ls/edit).

[pedronis]

a comment and also this has conflicts now

[pedronis]

iiuc ReplyContraints is not really validated unless via ToConstraints which makes me think that we really want ReplyContraints to not have any methods except the conversion and move the methods even if annoying to Constraints. That should force changing some of the code I'm right now slightly uncomfortable with.

[olivercalder]

I think that makes sense: ReplyConstraints.ContainPermissions and ReplyConstraints.Match can be moved to methods on Constraints instead.

The rationale for having those methods on ReplyConstraints instead of Constraints is that one should never call such methods on any constraints except ones which are received as part of a reply. But we can add a comment to that effect instead.

[olivercalder]

You were also right about moving responseForInterfaceConstraintsOutcome in #14390, it's making the rebase much more difficult...

[pedronis]

couple of naming comments

[pedronis]

given that the doc comment itself says: hold partial rule contents

I'm not against renaming this to RuleConstraintsPatch

[pedronis]

thank you

[bboozzoo]

LGTM

[bboozzoo]

This changed from []string to map and the struct is leaking to the API. I'm assuming there's an agreement that as long as the feature is experimental we're free to make such changes?

[olivercalder]

Yes. The desktop-security-center is the only supported client at the moment, and it will accept either the old or the new format and always map the result to the new format, since the new format can express a proper superset of what the old format can express. Because the feature is experimental and the API is not yet officially documented, we feel comfortable making this breaking change, which the only "official" client already handles correctly.

[olivercalder]

Test failures:

• openstack:debian-12-64:tests/completion/ -- dpkg problem, unrelated
• openstack:opensuse -- openstack problems, unrelated
• google:ubuntu-16.04-64:tests/main/degraded -- unrelated
• google:ubuntu-20.04-64:tests/unit/go:gcc -- unrelated, fix in the works by Katie
• google-arm:ubuntu-core-22-arm-64:tests/core/snapd-refresh-undo -- unrelated, fix in the works by Katie