i/prompting: add package for prompting common types/functions #13849

olivercalder · 2024-04-18T06:01:46Z

This PR includes some type definitions and helper functions for apparmor prompting, originally from #13668, which do not relate to constraints or abstract permissions.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: make `TimestampToTime()` return a local time, matching `time.Now()` and `time.Unix()` Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: make duration a string parsable by `time.ParseDuration()` Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: reorder functions Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: remove "app" from prompts and rules AppArmor permissions are granted by interfaces according to plugs/slots which may or may not differ between apps within a given snap. For the home interface, for example, there is never an instance where one app within a snap is granted a permission that another app in that same snap is not. Thus, it does not make sense for a rule to apply to only a single app within a snap. Instead, the interface field should optionally include the name of the plug or slot which granted the permissions, which should be received from the kernel using message tagging. The result should be of the form: `<interface name>[/<plug or slot>]` As such, rules should no longer be associated with a particular app, and in order to match, neither should request prompts. Adjustments are made to the prompting API to remove the "app" parameter when querying rules. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: add validation functions for expiration Adds expiration-related analogues for `ValidateLifespanParseDuration` and `ValidateConstraintsOutcomeLifespanDuration`. Additionally, improves error messages by including the contents of the invalid field in the message. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> i/prompting: move prompting common types/functions to interfaces/prompting Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

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

snap/info.go

snap/info_test.go

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

pedronis

bunch more comments

snap/info.go

pedronis · 2024-04-23T08:34:53Z

interfaces/prompting/prompting.go

+}
+
+// ValidateOutcome returns nil if the given outcome is valid, otherwise an error.
+func ValidateOutcome(outcome OutcomeType) error {


this is a bit strange, shouldn't it be a parse function taking a string and returning the OutcomeType or an error?

or maybe what you need is MarshalJSON UnmarshalJSON methods?

Since OutcomeType is an alias for string, the json {,un}marshaller works on OutcomeType directly, so potentially malformed outcomes are already OutcomeType variables which need to be validated, rather than strings. Theoretically, this could be built into custom {M,Unm}arshalJSON functions, but then there's no way to validate values when explicitly defined manually (i.e. myOutcome := OutcomeType("allow") instead of OutcomeAllow). There should probably never be a case where custom strings are cast to OutcomeTypes, so perhaps this negative isn't really relavant and a custom {,un}marshaller for OutcomeType would be ergonomic.

Hm if we don't care about defining new values of OutcomeType manually, then I think a trivial UnmarshalJSON would be preferred. You do not need MarshalJSON in such case.

pedronis · 2024-04-23T08:35:03Z

interfaces/prompting/prompting.go

+// If the lifespan is LifespanTimespan LifespanTimespan, then expiration must
+// be a string parsable as time.Duration with RFC3339 format. Otherwise, it must
+// be empty. Returns an error if any of the above are invalid.
+func ValidateLifespanExpiration(lifespan LifespanType, expiration string, currTime time.Time) error {


same question

More strongly than for OutcomeType, I do think this needs to be an explicit standalone function (rather than a custom json {,un}marshaller), since it's important that we're able to pass in the current time as a variable. Additionally, this is about validating the compatibility of the expiration for the given lifespan, so I don't believe this can belong in the {,un}marshaller for the LifespanType. It could be in that of the containing struct (e.g. PromptReply and RequestRule), but then there is duplicate code in these places, so I'm not sure it makes sense.

interfaces/prompting/prompting.go

Since `time.Time` values are marshalled as RFC3339Nano by default, there is no reason to explicitly save timestamps and expirations as strings and convert them to `time.Time` on the fly as necessary. Let the marshaller do this automatically. As expirations should only be populated when lifespan is "timespan", expirations can be stored as `*time.Time`, which should be encoded as `nil` when empty and an expiration timestamp in RFC3339Nano format when non-empty. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder · 2024-04-24T05:55:36Z

Regarding the timestamp-related helpers: timestamps and expirations can be stored as time.Time instead of a string, so these can be removed -- see olivercalder#34

olivercalder

Comments after live review with @zyga and @bboozzoo -- thanks!

interfaces/prompting/prompting.go

olivercalder · 2024-04-24T14:29:57Z

interfaces/prompting/prompting.go

+// should be valid. Otherwise, it must be empty. Returns an error if any of the
+// above are invalid, otherwise computes the expiration time of the rule based
+// on the current time and the given duration and returns it.
+func ValidateLifespanParseDuration(lifespan LifespanType, duration string) (*time.Time, error) {


me: should probably take currTime as well, for consistency

interfaces/prompting/prompting.go

olivercalder · 2024-04-24T14:45:03Z

interfaces/prompting/prompting.go

+// The timestamp is the same time, encoded as a string in time.RFC3339Nano.
+func NewIDAndTimestamp() (id string, timestamp time.Time) {
+	now := time.Now()
+	nowUnix := uint64(now.UnixNano())


What happens when the time reports the same unixnano?

Just use a counter. Store with rules storage? And use with request prompts as well.

Or combine boot ID with monotonic boot time. (look for helper, reads from /proc)

CLOCK_MONOTONIC
A nonsettable system-wide clock that represents monotonic time since—as described by POSIX—"some unspecified point in the past". On Linux, that point corresponds to the number of seconds that the system has been running since it was booted.
The CLOCK_MONOTONIC clock is not affected by discontinuous jumps in the system time (e.g., if the system administrator manually changes the clock), but is affected by frequency adjustments.

According to the time package documentation (https://pkg.go.dev/time#hdr-Monotonic_Clocks), time.Now() automatically includes both monotonic and wall time:

If Times t and u both contain monotonic clock readings, the operations t.After(u), t.Before(u), t.Equal(u), t.Compare(u), and t.Sub(u) are carried out using the monotonic clock readings alone, ignoring the wall clock readings. If either t or u contains no monotonic clock reading, these operations fall back to using the wall clock readings.

Because the monotonic clock reading has no meaning outside the current process, the serialized forms generated by t.GobEncode, t.MarshalBinary, t.MarshalJSON, and t.MarshalText omit the monotonic clock reading, and t.Format provides no format for it. Similarly, the constructors time.Date, time.Parse, time.ParseInLocation, and time.Unix, as well as the unmarshalers t.GobDecode, t.UnmarshalBinary. t.UnmarshalJSON, and t.UnmarshalText always create times with no monotonic clock reading.

Thus, the monotonic time is only stripped when rules/prompts are read from disk. This occurs when snapd is restarted, including on system start. Unfortunately, this means time.Now() is not guaranteed to be unique between snapd restarts, so we likely need to include both boot ID and system monotonic time when creating rule/prompt IDs.

where and what are this IDs used for ? in general snapd prefers to use sequential increasing IDs? at least for norification from the kernel don't we get them in a single goroutine? in that context using sequential IDs would be cheap, no?

AFAIU the IDs are stored within the prompting namespace in notices and we need them to be unique across reboots too (@olivercalder please correct me if I'm wrong), so to keep them sequential we'd either need to store some state.

well, we could restart counting from the max of the pending ones when we restart

yes, that's a good point

We could start from the max of the pending ones, though that would allow for the potential that IDs are reused for different rules, if a rule is removed, then snapd restarted. This isn't necessarily a problem, as we don't guarantee that IDs are never reused, but I'd prefer to keep them unique if possible.

I think storing a count as part of the rule storage would solve this problem, and it can be atomically incremented whenever a new rule is added. For prompt requests, which are not written to disk (because of listener/notify socket behavior, but that can be discussed in a later PR), we can just keep a count in memory and atomically increment it.

So, my opinion is that we should remove this function for now and add a NewID function when necessary in the requestrules and requestprompts packages.

snap/info.go

Additionally, adjust related error messages to remove "invalid ...:" prefix. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

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

Instead, use the existing `ParseSecurityTag` function and then call `InstanceName` on the result. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

There is not currently a good way to identify when a session ends when a user has logged in via multiple methods (e.g. tty, ssh). Until it is deemed to be necessary and a good implementation is developed, do not include LifespanSession. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

this is still under heavy changes

interfaces/prompting/prompting.go

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

IDs used for prompting (rules and prompt requests) must be unique across reboots, and thus cannot rely on timestamps or even monotonic time. We also want IDs to be sorted, so we cannot use boot ID along with monotonic time either. Lastly, since we need IDs to be unique across reboots, we can't simply use a counter based on the maximum of the existing rule/prompt IDs. Instead, we should store the current maximum ID in memory and on disk alongside the storage of rules and prompts. This should be implemented in the respective forthcoming packages and used at the former callsights of NewIDAndTimestamp. Since IDs should no longer be tied to timestamps, we can simply use `time.Now()` where the timestamps from `NewIDAndTimestamp` were formerly used. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

pedronis · 2024-04-29T16:21:46Z

interfaces/prompting/prompting.go

+func ValidateLifespanExpiration(lifespan LifespanType, expiration time.Time, currTime time.Time) error {
+	switch lifespan {
+	case LifespanForever, LifespanSingle:
+		if !expiration.IsZero() {
+			return fmt.Errorf(`expiration must be omitted when lifespan is %q, but received non-zero expiration: %q`, lifespan, expiration)
+		}
+	case LifespanTimespan:
+		if expiration.IsZero() {
+			return fmt.Errorf(`expiration must be non-zero when lifespan is %q, but received empty expiration`, lifespan)
+		}
+		if currTime.After(expiration) {
+			return fmt.Errorf("expiration time has already passed: %q", expiration)
+		}
+	default:
+		// Should not occur, since lifespan is validated when unmarshalled
+		return fmt.Errorf(`lifespan must be %q, %q, or %q`, LifespanForever, LifespanSingle, LifespanTimespan)
+	}
+	return nil
+}
+
+// ValidateLifespanParseDuration checks that the given lifespan is valid and
+// that the given duration is valid for that lifespan.
+//
+// If the lifespan is LifespanTimespan, then duration must be a string parsable
+// by time.ParseDuration(), representing the duration of time for which the rule
+// should be valid. Otherwise, it must be empty. Returns an error if any of the
+// above are invalid, otherwise computes the expiration time of the rule based
+// on the current time and the given duration and returns it.
+func ValidateLifespanParseDuration(lifespan LifespanType, duration string) (time.Time, error) {


if we keep them they should be methods on LifespanType I think

I agree, thanks!

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

bboozzoo

LGTM with some error message tweaks

bboozzoo · 2024-04-30T07:26:56Z

interfaces/prompting/prompting.go

+// ValidateExpiration checks that the given expiration is valid for the
+// receiver lifespan.
+//
+// If the lifespan is LifespanTimespan LifespanTimespan, then expiration must


Suggested change

// If the lifespan is LifespanTimespan LifespanTimespan, then expiration must

// If the lifespan is LifespanTimespan , then expiration must

bboozzoo · 2024-04-30T07:30:15Z

interfaces/prompting/prompting.go

+		}
+		parsedDuration, err := time.ParseDuration(duration)
+		if err != nil {
+			return expiration, fmt.Errorf(`error parsing duration string: %q`, duration)


FWIW, errors usually follow the pattern of cannot do sth... ParseDuration error already includes the duration string, no need to repeat it again.

Suggested change

return expiration, fmt.Errorf(`error parsing duration string: %q`, duration)

return expiration, fmt.Errorf(`cannot parse duration: %w`, duration, err)

that's correct, errors should try to be of the form "cannot ..." whenever possible

bboozzoo · 2024-04-30T07:33:56Z

interfaces/prompting/prompting.go

+		expiration = currTime.Add(parsedDuration)
+	default:
+		// Should not occur, since lifespan is validated when unmarshalled
+		return expiration, fmt.Errorf(`lifespan must be %q, %q, or %q`, LifespanForever, LifespanSingle, LifespanTimespan)


we already know what the valid values are, so should this occur at runtime, it's actually nice to know what the invalid value is:

Suggested change

return expiration, fmt.Errorf(`lifespan must be %q, %q, or %q`, LifespanForever, LifespanSingle, LifespanTimespan)

return expiration, fmt.Errorf("internal error: invalid lifespan %q", lifespan)

bboozzoo · 2024-04-30T07:34:21Z

interfaces/prompting/prompting.go

+		}
+	default:
+		// Should not occur, since lifespan is validated when unmarshalled
+		return fmt.Errorf(`lifespan must be %q, %q, or %q`, LifespanForever, LifespanSingle, LifespanTimespan)


same here about returning the invalid value rather than the valid ones (which we know already); should prob count as internal error too (as inputs from JSON go through unmarshalling and so would be checked anyway)

bboozzoo · 2024-04-30T07:35:23Z

interfaces/prompting/prompting.go

+	case LifespanForever, LifespanSingle, LifespanTimespan:
+		*lifespan = value
+	default:
+		return fmt.Errorf(`lifespan must be %q, %q, or %q`, LifespanForever, LifespanSingle, LifespanTimespan)


Suggested change

return fmt.Errorf(`lifespan must be %q, %q, or %q`, LifespanForever, LifespanSingle, LifespanTimespan)

return fmt.Errorf(`lifespan must be %q, %q, or %q, not %q`, LifespanForever, LifespanSingle, LifespanTimespan, value)

pedronis

looks good, but couple of small comments about doc comments and agreeing with Maciej about being careful with error messages

pedronis · 2024-04-30T14:13:40Z

interfaces/prompting/prompting.go

+	"time"
+)
+
+type OutcomeType string


this needs a doc string

pedronis · 2024-04-30T14:13:47Z

interfaces/prompting/prompting.go

+	}
+}
+
+type LifespanType string


pedronis · 2024-04-30T14:14:32Z

interfaces/prompting/prompting.go

+		}
+		parsedDuration, err := time.ParseDuration(duration)
+		if err != nil {
+			return expiration, fmt.Errorf(`error parsing duration string: %q`, duration)


that's correct, errors should try to be of the form "cannot ..." whenever possible

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

pedronis

thanks

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

This was referenced Apr 18, 2024

i/prompting: add constraints and abstract permissions #13850

Merged

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

i/prompting,snap: replace LabelToSnap with SplitSecurityTag

6a71217

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

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 16:45

olivercalder closed this Apr 18, 2024

olivercalder reopened this Apr 18, 2024

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

olivercalder added 3 commits April 18, 2024 13:07

i/prompting: removed unused NewID function

74cc30b

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

i/prompting: removed unused CurrentTimestamp function

a428025

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

i/prompting: rearrange functions

29920e0

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

MiguelPires previously approved these changes Apr 19, 2024

View reviewed changes

snap/info.go Outdated Show resolved Hide resolved

snap/info_test.go Outdated Show resolved Hide resolved

snap: check that app is non-empty if it exists in security label

0f9b8d2

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

pedronis requested changes Apr 23, 2024

View reviewed changes

olivercalder commented Apr 24, 2024

View reviewed changes

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

olivercalder commented Apr 24, 2024

View reviewed changes

olivercalder added 4 commits April 24, 2024 13:49

i/prompting: store expiration as time.Time instead of *time.Time

617e192

Additionally, adjust related error messages to remove "invalid ...:" prefix. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

i/prompting: rename AsBool to IsAllow and adjust related error messages

a9031f6

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

snap: remove SplitSecurityTag function

01d2a87

Instead, use the existing `ParseSecurityTag` function and then call `InstanceName` on the result. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder force-pushed the prompting-common-types-helpers branch from 69a3fc7 to 4adc766 Compare April 24, 2024 20:18

bboozzoo reviewed Apr 25, 2024

View reviewed changes

interfaces/prompting/prompting.go Show resolved Hide resolved

olivercalder added 2 commits April 26, 2024 21:14

i/prompting: validate outcome and lifespan when unmarshalling

4fe163d

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

olivercalder requested review from bboozzoo, pedronis and MiguelPires April 29, 2024 13:03

pedronis reviewed Apr 29, 2024

View reviewed changes

i/prompting: replaced ValidateLifespan* functions with methods on lif…

6a8609a

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

olivercalder requested a review from pedronis April 29, 2024 19:47

bboozzoo approved these changes Apr 30, 2024

View reviewed changes

pedronis approved these changes Apr 30, 2024

View reviewed changes

i/prompting: add doc string and adjusted error messages and comments

9875909

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

olivercalder requested a review from pedronis April 30, 2024 18:23

pedronis approved these changes May 1, 2024

View reviewed changes

MiguelPires approved these changes May 1, 2024

View reviewed changes

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

Meulengracht merged commit 356d7c1 into snapcore:master May 1, 2024
45 of 48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i/prompting: add package for prompting common types/functions #13849

i/prompting: add package for prompting common types/functions #13849

olivercalder commented Apr 18, 2024 •

edited

pedronis left a comment

pedronis Apr 23, 2024

pedronis Apr 23, 2024

olivercalder Apr 24, 2024 •

edited

bboozzoo Apr 25, 2024

pedronis Apr 23, 2024

olivercalder Apr 24, 2024

olivercalder commented Apr 24, 2024

olivercalder left a comment

olivercalder Apr 24, 2024

olivercalder Apr 24, 2024

olivercalder Apr 24, 2024

olivercalder Apr 24, 2024

pedronis Apr 25, 2024

bboozzoo Apr 25, 2024

pedronis Apr 25, 2024 •

edited

bboozzoo Apr 25, 2024

olivercalder Apr 25, 2024

pedronis Apr 29, 2024

olivercalder Apr 29, 2024

bboozzoo left a comment

bboozzoo Apr 30, 2024

bboozzoo Apr 30, 2024

pedronis Apr 30, 2024

bboozzoo Apr 30, 2024

bboozzoo Apr 30, 2024

bboozzoo Apr 30, 2024

pedronis left a comment

pedronis Apr 30, 2024

pedronis Apr 30, 2024

pedronis Apr 30, 2024

pedronis left a comment

	// If the lifespan is LifespanTimespan LifespanTimespan, then expiration must
	// If the lifespan is LifespanTimespan , then expiration must

	return expiration, fmt.Errorf(`error parsing duration string: %q`, duration)
	return expiration, fmt.Errorf(`cannot parse duration: %w`, duration, err)

	return expiration, fmt.Errorf(`lifespan must be %q, %q, or %q`, LifespanForever, LifespanSingle, LifespanTimespan)
	return expiration, fmt.Errorf("internal error: invalid lifespan %q", lifespan)

	return fmt.Errorf(`lifespan must be %q, %q, or %q`, LifespanForever, LifespanSingle, LifespanTimespan)
	return fmt.Errorf(`lifespan must be %q, %q, or %q, not %q`, LifespanForever, LifespanSingle, LifespanTimespan, value)

i/prompting: add package for prompting common types/functions #13849

i/prompting: add package for prompting common types/functions #13849

Conversation

olivercalder commented Apr 18, 2024 • edited

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

olivercalder Apr 24, 2024 • edited

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 24, 2024

olivercalder 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

pedronis Apr 25, 2024 • edited

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

bboozzoo 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

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

pedronis left a comment

Choose a reason for hiding this comment

olivercalder commented Apr 18, 2024 •

edited

olivercalder Apr 24, 2024 •

edited

pedronis Apr 25, 2024 •

edited