Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
140 changes: 140 additions & 0 deletions interfaces/prompting/prompting.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2024 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package prompting

import (
"encoding/base32"
"encoding/binary"
"fmt"
"time"
)

type OutcomeType string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs a doc string


const (
OutcomeUnset OutcomeType = ""
OutcomeAllow OutcomeType = "allow"
OutcomeDeny OutcomeType = "deny"
)

// IsAllow returns true if the outcome is OutcomeAllow, false if the outcome is
// OutcomeDeny, or an error if it cannot be parsed.
func (outcome OutcomeType) IsAllow() (bool, error) {
switch outcome {
case OutcomeAllow:
return true, nil
case OutcomeDeny:
return false, nil
default:
return false, fmt.Errorf(`outcome must be %q or %q: %q`, OutcomeAllow, OutcomeDeny, outcome)
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe what you need is MarshalJSON UnmarshalJSON methods?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

switch outcome {
case OutcomeAllow, OutcomeDeny:
return nil
default:
return fmt.Errorf(`outcome must be %q or %q: %q`, OutcomeAllow, OutcomeDeny, outcome)
}
}

type LifespanType string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same


const (
LifespanUnset LifespanType = ""
LifespanForever LifespanType = "forever"
LifespanSingle LifespanType = "single"
LifespanTimespan LifespanType = "timespan"
bboozzoo marked this conversation as resolved.
Show resolved Hide resolved
// TODO: add LifespanSession which expires after the user logs out
// LifespanSession LifespanType = "session"
)

// ValidateLifespanExpiration checks that the given lifespan is valid and that
// the given expiration is valid for that lifespan.
//
// If the lifespan is LifespanTimespan LifespanTimespan, then expiration must
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// be non-zero and be after the given currTime. Otherwise, it must be zero.
// Returns an error if any of the above are invalid.
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:
return fmt.Errorf(`invalid lifespan: %q`, lifespan)
}
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) {
var expiration time.Time
switch lifespan {
case LifespanForever, LifespanSingle:
if duration != "" {
return expiration, fmt.Errorf(`duration must be empty when lifespan is %q, but received non-empty duration: %q`, lifespan, duration)
}
case LifespanTimespan:
if duration == "" {
return expiration, fmt.Errorf(`duration must be non-empty when lifespan is %q, but received empty expiration`, lifespan)
}
parsedDuration, err := time.ParseDuration(duration)
if err != nil {
return expiration, fmt.Errorf(`error parsing duration string: %q`, duration)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
if parsedDuration <= 0 {
return expiration, fmt.Errorf(`duration must be greater than zero: %q`, duration)
}
expiration = time.Now().Add(parsedDuration)
default:
return expiration, fmt.Errorf(`invalid lifespan: %q`, lifespan)
}
return expiration, nil
}

// NewIDAndTimestamp returns a new unique ID and corresponding timestamp.
//
// The ID is the current unix time in nanoseconds encoded as a string in base32.
// 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())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when the time reports the same unixnano?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@pedronis pedronis Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's a good point

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

nowBytes := make([]byte, 8)
binary.BigEndian.PutUint64(nowBytes, nowUnix)
id = base32.StdEncoding.EncodeToString(nowBytes)
return id, now
}
153 changes: 153 additions & 0 deletions interfaces/prompting/prompting_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2024 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package prompting_test

import (
"encoding/base32"
"encoding/binary"
"testing"
"time"

. "gopkg.in/check.v1"

"github.com/snapcore/snapd/dirs"
"github.com/snapcore/snapd/interfaces/prompting"
)

func Test(t *testing.T) { TestingT(t) }

type promptingSuite struct {
tmpdir string
}

var _ = Suite(&promptingSuite{})

func (s *promptingSuite) SetUpTest(c *C) {
s.tmpdir = c.MkDir()
dirs.SetRootDir(s.tmpdir)
}

func (s *promptingSuite) TestOutcomeIsAllow(c *C) {
result, err := prompting.OutcomeAllow.IsAllow()
c.Check(err, IsNil)
c.Check(result, Equals, true)
result, err = prompting.OutcomeDeny.IsAllow()
c.Check(err, IsNil)
c.Check(result, Equals, false)
_, err = prompting.OutcomeUnset.IsAllow()
c.Check(err, ErrorMatches, `outcome must be.*`)
_, err = prompting.OutcomeType("foo").IsAllow()
c.Check(err, ErrorMatches, `outcome must be.*`)
}

func (s *promptingSuite) TestValidateOutcome(c *C) {
c.Assert(prompting.ValidateOutcome(prompting.OutcomeAllow), Equals, nil)
c.Assert(prompting.ValidateOutcome(prompting.OutcomeDeny), Equals, nil)
c.Assert(prompting.ValidateOutcome(prompting.OutcomeUnset), ErrorMatches, `outcome must be.*`)
c.Assert(prompting.ValidateOutcome(prompting.OutcomeType("foo")), ErrorMatches, `outcome must be.*`)
}

func (s *promptingSuite) TestValidateLifespanExpiration(c *C) {
var unsetExpiration time.Time
currTime := time.Now()
negativeExpiration := currTime.Add(-5 * time.Second)
validExpiration := currTime.Add(10 * time.Minute)

for _, lifespan := range []prompting.LifespanType{
prompting.LifespanForever,
prompting.LifespanSingle,
} {
err := prompting.ValidateLifespanExpiration(lifespan, unsetExpiration, currTime)
c.Check(err, IsNil)
for _, exp := range []time.Time{negativeExpiration, validExpiration} {
err = prompting.ValidateLifespanExpiration(lifespan, exp, currTime)
c.Check(err, ErrorMatches, `expiration must be omitted.*`)
}
}

err := prompting.ValidateLifespanExpiration(prompting.LifespanTimespan, unsetExpiration, currTime)
c.Check(err, ErrorMatches, `expiration must be non-zero.*`)

err = prompting.ValidateLifespanExpiration(prompting.LifespanTimespan, negativeExpiration, currTime)
c.Check(err, ErrorMatches, `expiration time has already passed.*`)

err = prompting.ValidateLifespanExpiration(prompting.LifespanTimespan, validExpiration, currTime)
c.Check(err, IsNil)
}

func (s *promptingSuite) TestValidateLifespanParseDuration(c *C) {
unsetDuration := ""
invalidDuration := "foo"
negativeDuration := "-5s"
validDuration := "10m"
parsedValidDuration, err := time.ParseDuration(validDuration)
c.Assert(err, IsNil)

for _, lifespan := range []prompting.LifespanType{
prompting.LifespanForever,
prompting.LifespanSingle,
} {
expiration, err := prompting.ValidateLifespanParseDuration(lifespan, unsetDuration)
c.Check(expiration.IsZero(), Equals, true)
c.Check(err, IsNil)
for _, dur := range []string{invalidDuration, negativeDuration, validDuration} {
expiration, err = prompting.ValidateLifespanParseDuration(lifespan, dur)
c.Check(expiration.IsZero(), Equals, true)
c.Check(err, ErrorMatches, `duration must be empty.*`)
}
}

expiration, err := prompting.ValidateLifespanParseDuration(prompting.LifespanTimespan, unsetDuration)
c.Check(expiration.IsZero(), Equals, true)
c.Check(err, ErrorMatches, `duration must be non-empty.*`)

expiration, err = prompting.ValidateLifespanParseDuration(prompting.LifespanTimespan, invalidDuration)
c.Check(expiration.IsZero(), Equals, true)
c.Check(err, ErrorMatches, `error parsing duration string.*`)

expiration, err = prompting.ValidateLifespanParseDuration(prompting.LifespanTimespan, negativeDuration)
c.Check(expiration.IsZero(), Equals, true)
c.Check(err, ErrorMatches, `duration must be greater than zero.*`)

expiration, err = prompting.ValidateLifespanParseDuration(prompting.LifespanTimespan, validDuration)
c.Check(err, IsNil)
c.Check(expiration.After(time.Now()), Equals, true)
c.Check(expiration.Before(time.Now().Add(parsedValidDuration)), Equals, true)
}

func (s *promptingSuite) TestNewIDAndTimestamp(c *C) {
before := time.Now()
id, _ := prompting.NewIDAndTimestamp()
idPaired, timestampPaired := prompting.NewIDAndTimestamp()
after := time.Now()
data1, err := base32.StdEncoding.DecodeString(id)
c.Assert(err, IsNil)
data2, err := base32.StdEncoding.DecodeString(idPaired)
c.Assert(err, IsNil)
parsedNs := int64(binary.BigEndian.Uint64(data1))
parsedNsPaired := int64(binary.BigEndian.Uint64(data2))
parsedTime := time.Unix(parsedNs/1000000000, parsedNs%1000000000)
parsedTimePaired := time.Unix(parsedNsPaired/1000000000, parsedNsPaired%1000000000)
c.Assert(parsedTime.After(before), Equals, true)
c.Assert(parsedTime.Before(after), Equals, true)
c.Assert(parsedTimePaired.After(before), Equals, true)
c.Assert(parsedTimePaired.Before(after), Equals, true)
c.Assert(parsedTimePaired.Equal(timestampPaired), Equals, true)
}