i/prompting: add constraints and abstract permissions #13850

olivercalder · 2024-04-18T06:05:29Z

This PR introduces constraints and abstract permissions, originally from #13668. This PR is based on #13849, and should be continually rebased on it whenever changes are pushed there.

pedronis

did a first pass with some questions/comments

interfaces/prompting/constraints.go

pedronis · 2024-04-25T14:59:37Z

interfaces/prompting/constraints.go

+// AbstractPermissionsFromList validates the given permissions list for the
+// given interface and returns a list containing those permissions in the order
+// in which they occur in the list of available permissions for that interface.
+func AbstractPermissionsFromList(iface string, permissions []string) ([]string, error) {


the name doesn't really convey that this does also validation

True, the only use case for this right now is in constraints.ValidateForInterface, so this could be called ValidatePermissions as a method on the Constraints type.

pedronis · 2024-04-25T15:03:03Z

interfaces/prompting/constraints.go

+
+// ValidateConstraintsOutcomeLifespanExpiration returns an error if the given
+// constraints, outcome, lifespan, or duration are invalid, else returns nil.
+func ValidateConstraintsOutcomeLifespanExpiration(iface string, constraints *Constraints, outcome OutcomeType, lifespan LifespanType, expiration *time.Time, currTime time.Time) error {


this is strange, do we need a struct with the thing passed to this all together and this is a method on it?

I agree, this should be a method on the RequestRule type, likely rule.Validate(), and this is only used when looking over existing rules and making sure they are valid and consistent. Thanks!

pedronis · 2024-04-25T15:03:37Z

interfaces/prompting/constraints.go

+// ValidateConstraintsOutcomeLifespanDuration returns an error if the given
+// constraints, outcome, lifespan, or duration are invalid. Otherwise, converts
+// the given duration to an expiration timestamp and returns it and nil error.
+func ValidateConstraintsOutcomeLifespanDuration(iface string, constraints *Constraints, outcome OutcomeType, lifespan LifespanType, duration string) (*time.Time, error) {


again it seems that this should be folded in the other taking a struct and make decision based on what lifespan is?

This one is a bit more complex, as these fields are validated both when receiving a PromptReply from the client and when adding a new rule. I think the simplest/safest approach would be to continue to validate using rule.Validate() as part of PopulateNewRule, which is a helper in the requestrules called whenever a rule is being added or modified.

Rule expansion also occurs immediately after validation in requestrules, so rule.Validate() could also call ExpandPathPattern, making it all simpler.

The problem is that we want to be able to validate PromptReply contents before sending back a reply to the listener, all of which occurs before we go about creating a new rule. See: https://github.com/snapcore/snapd/pull/12964/files#diff-1bc7f26dd750993e7bf7bb0c8feb2e6d664f99313b5069f1dd68f4c3a66d51f9R234-R266

So for now, I'm inclined to remove this function, and we can re-add something like it when it is known precisely what we need, likely during the upcoming apparmorprompting package PR.

olivercalder · 2024-04-29T20:16:46Z

Now that the ValidateConstraintsOutcomeLifespan* functions are removed, this no longer depends on #13849, hopefully making reviews a bit easier as well. Rebased on master while we're at it.

pedronis

lgtm, but a bit unsure why the split out helpers?

pedronis · 2024-04-30T15:57:18Z

interfaces/prompting/constraints.go

+// AbstractPermissionsFromAppArmorPermissions returns the list of permissions
+// corresponding to the given AppArmor permissions for the given interface.
+func AbstractPermissionsFromAppArmorPermissions(iface string, permissions interface{}) ([]string, error) {
+	return abstractPermissionsFromAppArmorFilePermissions(iface, permissions)


why the call to a helper instead of having the implementation here? same for the functions below

This allows us to easily support other mediation classes in the future by adding equivalent helpers. And don't have to deal with type assertions here. Alternatively, could switch on permissions.(type), which could perhaps be better. But the original idea was to switch on iface and choose the appropriate abstractPermissionsFromAppArmor{File,Dbus,Network,...}Permissions function for that interface --- this was subsequently replaced by simply calling abstractPermissionsFromAppArmorFilePermissions, for now.

If we just switch on permissions.(type), then if the iface is not supported for the mediation class (e.g. file, network), currently we return an internal error, but we could change the error message.

The problem with this approach is that, for AbstractPermissionsToAppArmorPermissions, we don't have any indication of which mediation class is applicable unless we switch on iface, and even then this doesn't work if an interface could potentially handle more than one mediation class.

So I agree, for the time being, it would be simpler/clearer to just have the ...FilePermission implementation in the function body, no helper. Thanks!

pedronis

thanks, comments mostly about errors

interfaces/prompting/constraints.go

pedronis

thanks

Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: added function to select one interface Multiple interfaces may be included in the tag in the kernel message, and the listener passes these on to the other prompting components. This PR adds a function to decide which of those interfaces to use in prompt requests and rules. Rules only apply to a particular interface, and we don't want duplicate rules, so we must choose one interface from the list provided by the listener which we use for the prompting requests and rules associated with the listener request. It is rather arbitrary which interfaces should have priority, and in many cases interfaces do not have overlapping permissions, but we should nonetheless manually assign a priority to any interface for which we enable prompting. Any request with only interfaces which are not explicitly included in the list will be treated as having interface "other", as will any request with an empty interfaces list. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: add "constraints" field to rules and replies Adds a "constraints" field to request rules and other related structure, such as prompt replies. These constraints vary by interface, with some interfaces supporting different permissions than others, some interfaces supporting different constraints on path patterns (or non-path resources), and possibly future extensions in the future. The idea behind constraints is to allow these interface-specific variations in the future. Addionally, there are some changes to behavior which are introduced alongside the constraints changes: 1. Constraints (formerly permissions lists) are no longer duplicated when creating rules, to avoid unnecessary memory allocations. 2. Permissions are removed from constraints (formerly permission lists) in-place, rather than by creating a new list, again to avoid unnecessary memory allocations, so constraints should never be shared or reused between multiple rules. 3. Prompt reply fields are validated *before* sending back a reply to the kernel, and if any fields are invalid, or the reply constraints do not match the original request, a reply is not sent. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: abstract apparmor permissions Convert AppArmor permissions into abstract permission strings, where the available permissions are dependent on the interface associated with the prompt or rule. This allows greater flexibility to accept requests with new interfaces and/or new mediation classes from the kernel without changing the user-facing API (at least, regarding permissions), and with minimal internal code changes. In particular, the functions for parsing request permissions from AppArmor are modular, and all that is required to add a new interface with an existing mediation class is to add the mappings from abstract to AppArmor permissions. Additionally, reorganized and added more unit tests to increase coverage. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> o/i/a/common: unexport unused exported function Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: small refactors and quote variables in error messages Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: mark constraints fields as omitempty Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: move constraints and abstract permissions to interfaces/prompting Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

…terface Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

`ValidateConstraintsOutcomeLifespanExpiration` should be replaced by a `Validate` method on the forthcoming `RequestRule` type, while `ValidateConstraintsOutcomeLifespanDuration` should be unnecessary, since validation of outcomes and lifespans will occur during unmarshalling, and converting from duration to expiration should be done explicitly when necessary. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

…ppArmorPermissions Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

MiguelPires

lgtm

This PR introduces constraints and abstract permissions. * i/prompting: add constraints and abstract permissions Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: added function to select one interface Multiple interfaces may be included in the tag in the kernel message, and the listener passes these on to the other prompting components. This PR adds a function to decide which of those interfaces to use in prompt requests and rules. Rules only apply to a particular interface, and we don't want duplicate rules, so we must choose one interface from the list provided by the listener which we use for the prompting requests and rules associated with the listener request. It is rather arbitrary which interfaces should have priority, and in many cases interfaces do not have overlapping permissions, but we should nonetheless manually assign a priority to any interface for which we enable prompting. Any request with only interfaces which are not explicitly included in the list will be treated as having interface "other", as will any request with an empty interfaces list. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: add "constraints" field to rules and replies Adds a "constraints" field to request rules and other related structure, such as prompt replies. These constraints vary by interface, with some interfaces supporting different permissions than others, some interfaces supporting different constraints on path patterns (or non-path resources), and possibly future extensions in the future. The idea behind constraints is to allow these interface-specific variations in the future. Addionally, there are some changes to behavior which are introduced alongside the constraints changes: 1. Constraints (formerly permissions lists) are no longer duplicated when creating rules, to avoid unnecessary memory allocations. 2. Permissions are removed from constraints (formerly permission lists) in-place, rather than by creating a new list, again to avoid unnecessary memory allocations, so constraints should never be shared or reused between multiple rules. 3. Prompt reply fields are validated *before* sending back a reply to the kernel, and if any fields are invalid, or the reply constraints do not match the original request, a reply is not sent. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: abstract apparmor permissions Convert AppArmor permissions into abstract permission strings, where the available permissions are dependent on the interface associated with the prompt or rule. This allows greater flexibility to accept requests with new interfaces and/or new mediation classes from the kernel without changing the user-facing API (at least, regarding permissions), and with minimal internal code changes. In particular, the functions for parsing request permissions from AppArmor are modular, and all that is required to add a new interface with an existing mediation class is to add the mappings from abstract to AppArmor permissions. Additionally, reorganized and added more unit tests to increase coverage. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> o/i/a/common: unexport unused exported function Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: small refactors and quote variables in error messages Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: mark constraints fields as omitempty Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: move constraints and abstract permissions to interfaces/prompting Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: remove SelectSingleInterface and references to camera interface Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: removed switches for handlers based on interface name Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: use *time.Time for expiration Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: simplify RemovePermission Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: renamed AbstractPermissionsFromList to ValidatePermissions Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: remove ValidateConstraintsOutcomeLifespan* functions `ValidateConstraintsOutcomeLifespanExpiration` should be replaced by a `Validate` method on the forthcoming `RequestRule` type, while `ValidateConstraintsOutcomeLifespanDuration` should be unnecessary, since validation of outcomes and lifespans will occur during unmarshalling, and converting from duration to expiration should be done explicitly when necessary. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: assume file permissions in AbstractPermissions{To,From}AppArmorPermissions Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: adjust abstract permission error messages Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: use separate test suite for constraints Signed-off-by: Oliver Calder <oliver.calder@canonical.com> --------- Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder added the Skip spread Indicate that spread job should not run label Apr 18, 2024

This was referenced Apr 18, 2024

o/i/apparmorprompting/common: add prompting path patterns #13730

Open

o/i/apparmorprompting: add prompting common package #13668

Closed

olivercalder added the Needs Samuele review Needs a review from Samuele before it can land label Apr 18, 2024

olivercalder force-pushed the prompting-add-constraints branch 2 times, most recently from 838fac8 to 596f185 Compare April 18, 2024 18:30

olivercalder removed the Skip spread Indicate that spread job should not run label Apr 18, 2024

olivercalder requested review from bboozzoo, zyga, pedronis and MiguelPires April 18, 2024 18:32

olivercalder closed this Apr 18, 2024

olivercalder reopened this Apr 18, 2024

olivercalder marked this pull request as ready for review April 18, 2024 18:32

olivercalder force-pushed the prompting-add-constraints branch from 596f185 to 90d08d9 Compare April 24, 2024 06:05

pedronis reviewed Apr 25, 2024

View reviewed changes

olivercalder force-pushed the prompting-add-constraints branch from 35c4529 to 1cce32f Compare April 29, 2024 20:12

olivercalder requested a review from pedronis April 29, 2024 20:16

pedronis reviewed Apr 30, 2024

View reviewed changes

olivercalder requested a review from pedronis April 30, 2024 16:55

pedronis reviewed May 1, 2024

View reviewed changes

interfaces/prompting/constraints.go Outdated Show resolved Hide resolved

interfaces/prompting/constraints.go Outdated Show resolved Hide resolved

interfaces/prompting/constraints.go Outdated Show resolved Hide resolved

interfaces/prompting/constraints.go Outdated Show resolved Hide resolved

olivercalder requested a review from pedronis May 1, 2024 14:55

pedronis approved these changes May 1, 2024

View reviewed changes

olivercalder added 6 commits May 1, 2024 22:47

i/prompting: remove SelectSingleInterface and references to camera in…

fc39dc7

…terface Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

i/prompting: removed switches for handlers based on interface name

0c219e9

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

i/prompting: use *time.Time for expiration

1372e6e

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

i/prompting: simplify RemovePermission

c074e1a

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

i/prompting: renamed AbstractPermissionsFromList to ValidatePermissions

73baf0c

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder added 4 commits May 1, 2024 22:47

i/prompting: assume file permissions in AbstractPermissions{To,From}A…

85e2e59

…ppArmorPermissions Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

i/prompting: adjust abstract permission error messages

e8b25f8

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

i/prompting: use separate test suite for constraints

59aeeb6

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder force-pushed the prompting-add-constraints branch from 971a7d4 to 59aeeb6 Compare May 2, 2024 03:52

MiguelPires approved these changes May 2, 2024

View reviewed changes

olivercalder added the Squash-merge Please squash this PR when merging. label May 2, 2024

pedronis merged commit 5ef32b3 into snapcore:master May 2, 2024
45 of 48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i/prompting: add constraints and abstract permissions #13850

i/prompting: add constraints and abstract permissions #13850

olivercalder commented Apr 18, 2024

pedronis left a comment

pedronis Apr 25, 2024

olivercalder Apr 25, 2024

pedronis Apr 25, 2024

olivercalder Apr 25, 2024

pedronis Apr 25, 2024

olivercalder Apr 25, 2024

olivercalder Apr 25, 2024

olivercalder commented Apr 29, 2024

pedronis left a comment

pedronis Apr 30, 2024

olivercalder Apr 30, 2024

pedronis left a comment

pedronis left a comment

MiguelPires left a comment

i/prompting: add constraints and abstract permissions #13850

i/prompting: add constraints and abstract permissions #13850

Conversation

olivercalder commented Apr 18, 2024

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivercalder commented Apr 29, 2024

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

MiguelPires left a comment

Choose a reason for hiding this comment