i/prompting{,requestrules}: merge rules which have identical lifespans

[olivercalder]

This PR is based on #14635.

When a rule is added or modified directly, or when a rule is created as a result of a prompt reply, merge it with any existing rule which has an identical path pattern. That way, a single rule represents the complete rule state for a given path pattern.

A good use case for this is if a user is prompted for read access to /foo/bar, which they grant forever, resulting in a pseudo-rule like allow read /foo/bar forever. If the user is then asked for write access to /foo/bar and they reply with allow read,write /foo/bar forever, then instead of there coexisting the following rules:

• allow read /foo/bar forever
• allow read,write /foo/bar forever
  We instead end up with a single "merged" rule for that path pattern, equal to the latter. This is simple enough, since the read part of the rule is identical.

Things get a bit more interesting when we start varying the outcome and lifespan of the rules to be merged:

• If the outcomes differ for a particular permission, throw an error, as these rules are in conflict
• If the lifespans differ for a particular permission, preserve the "longer" of the two lifespans
  • If a user previously said allow read,write /foo/bar timespan:5m and then said allow read,execute /foo/bar forever, the resulting rule constraints would look like (in actual json):

{
    "path-pattern": "/foo/bar",
    "permissions": {
        "read": {
            "outcome": "allow",
            "lifespan": "forever"
        },
        "write": {
            "outcome": "allow",
            "lifespan": "timespan",
            "expiration": "<timestamp-plus-5-minutes>"
        },
        "execute": {
            "outcome": "allow",
            "lifespan": "forever"
        }
    }

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

[codecov]

Codecov Report

Attention: Patch coverage is 94.88636% with 9 lines in your changes missing coverage. Please review.

Project coverage is 78.27%. Comparing base (24a0034) to head (024d155).
Report is 110 commits behind head on master.

Files with missing lines	Patch %	Lines
interfaces/prompting/requestrules/requestrules.go	94.40%	6 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14757      +/-   ##
==========================================
+ Coverage   78.20%   78.27%   +0.06%     
==========================================
  Files        1151     1158       +7     
  Lines      151396   153291    +1895     
==========================================
+ Hits       118402   119986    +1584     
- Misses      25662    25914     +252     
- Partials     7332     7391      +59     

Flag	Coverage Δ
unittests	78.27% <94.88%> (+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.

[olivercalder]

Question for reviewers

[pedronis]

thanks, did a first pass

[pedronis]

is emitting this very valuable? the rule is not really fully going away

[olivercalder]

The notice data as a whole, or the "merged-into" field? I think it is important include at least "removed": "merged", since the rule with the original ID no longer exists, and all that exists is now the ID of the rule into which it was merged.

Let's say rules with IDs A and B exist, and the client has retrieved these rule details from snapd. Then snapd restarts, and as part of re-loading the rules from disk, it merges rule B into rule A. The client needs to get a notice with "removed": "merged" for rule B (as opposed to just getting a notice with empty data for B), so that it knows it can drop rule B without first needing to retrieve rule details for B via a separate API request. Also including "merged-into" avoids a race condition if the notice that rule A has been updated has not yet been recorded: if the client sees that rule B has been merged but doesn't yet know into which other rule it was merged, it has to wait for the notice for rule A before it has the new merged rule contents, and even then, it has to manually check rule contents to know which rule the semantics formerly associated with rule B now belong. Including "merged-into": "A" lets the client know exactly what happened to rule B, and where to find the rule which now covers what B used to cover.

[bboozzoo]

is the client expected to keep rule IDs in its state longer (eg through persistence) than needed for the basic prompting interaction?

[olivercalder]

The security center is the only current client for rule IDs, and it displays existing rules in a table and lets users inspect or delete them. So aside from initially loading rules or notice-driven updates, it's holding the current rule state and displaying it. It could query all rules every time it receives any notice, but I think we can provide better information than that using notice data.

[bboozzoo]

LGTM

[pedronis]

thank you

[github-actions]

Wed Jan 15 16:33:02 UTC 2025

No spread failures

[olivercalder]

Rebased to pull in test fixes from master.