asserts,registry: define confdb-control assertion #14705

st3v3nmw · 2024-11-11T09:39:10Z

Define the confdb-control assertion and add a feature flag. The bits on the API (delegating, revoking, etc) will be added later when the design discussions are finalized.

codecov · 2024-11-11T11:47:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.99%. Comparing base (96ea7b0) to head (a1c0302).
Report is 137 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14705      +/-   ##
==========================================
- Coverage   78.95%   77.99%   -0.96%     
==========================================
  Files        1084     1135      +51     
  Lines      146638   150521    +3883     
==========================================
+ Hits       115773   117403    +1630     
- Misses      23667    25854    +2187     
- Partials     7198     7264      +66

Flag	Coverage Δ
unittests	`77.99% <100.00%> (-0.96%)`	⬇️

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.

miguelpires · 2024-11-12T09:34:29Z

For posterity, this was split off of #14508

miguelpires · 2024-11-18T13:59:08Z

asserts/confdb_asserts.go

+	cc := &ConfdbControl{
+		assertionBase: assert,
+		operators:     make(map[string]*confdb.Operator),
+	}
+	if err := parseConfdbControlGroups(cc, groups); err != nil {
+		return nil, err
+	}


In general, I'd say it's better to have parseConfdbControlGroups return a map[string]*confdb.Operator which gets put into cc rather than modify a parameter directly

miguelpires · 2024-11-18T14:00:03Z

asserts/confdb_asserts.go

+
+func parseConfdbControlGroups(cc *ConfdbControl, rawGroups []interface{}) error {
+	for i, rawGroup := range rawGroups {
+		errPrefix := fmt.Sprintf("group at position %d", i+1)


Suggested change

errPrefix := fmt.Sprintf("group at position %d", i+1)

errPrefix := fmt.Sprintf("cannot parse group at position %d", i+1)

miguelpires · 2024-11-18T14:10:58Z

asserts/confdb_asserts.go

+		if err != nil {
+			return fmt.Errorf(`%s: "authentication" %w`, errPrefix, err)
+		}
+		if auth == nil {


This also needs to check that the auth method is operator-key or store

The check ends up happening inside registry/confdb_control/(AddGroup -> convertToAuthenticationMethod -> isValidAuthenticationMethod). It's a bit nested but if we do it here, we'll end up running the check twice. I could bump up the check though

miguelpires · 2024-11-18T15:26:07Z

confdb/confdb_control.go

+	if len(auth) == 0 {
+		return fmt.Errorf(`"authentication" must be a non-empty list`)
+	}


There's no "authentication" here. I know this comes from the field in the assertion but out of that context it's not clear what this is referring to. The check should be reformulated

I've changed the errors to cannot add group: "auth" must be a non-empty list & cannot add group: "views" must be a non-empty list to be more descriptive. auth & views are the parameters passed to op.AddGroup

miguelpires · 2024-11-18T15:26:30Z

confdb/confdb_control.go

+	}
+
+	if len(views) == 0 {
+		return fmt.Errorf(`"views" must be a non-empty list`)


miguelpires · 2024-11-18T15:33:25Z

confdb/confdb_control.go

+	return nil
+}
+
+func compact[T comparable](s []T) []T {


This could use a comment mentioning that s has to be sorted

miguelpires · 2024-11-18T15:54:13Z

asserts/confdb_asserts.go

Maybe this assertion can go into the file with the registry (soon to be confdb) assertion?

miguelpires · 2024-11-18T15:59:16Z

features/features.go

@@ -150,6 +154,7 @@ var featuresExported = map[SnapdFeature]bool{

 	RefreshAppAwarenessUX: true,
 	Registries:            true,
+	ConfdbControl:         true,


Do we need to export this flag? Based on the spec I don't see a reason to, at the moment, but maybe there's more to it

Yep, that flag shouldn't have been exported. Removed.

miguelpires · 2024-11-18T16:16:25Z

confdb/confdb_control.go

I'd say lets put this in registry/ for now so it's grouped up with the other code. Once I open rename, it will all get moved together

miguelpires

thank you

miguelpires · 2024-11-26T15:34:32Z

registry/confdb_control.go

+}
+
+// Group holds a set of views delegated through the given authentication.
+type Group struct {


Wondering whether DelegatedViews or ControlGroup wouldn't be better as Group is fairly generic but it's minor anyway

I was trying to match the spec but I agree, something like ControlGroup is clearer.

Thanks for the review!

given that is shows up as registry.Group I think we definitely need a more precise name, "group" in the context of the assertion is fine as it is scoped to the assertion, but the type name here is not

@pedronis, I've renamed it to ControlGroup... registry.ControlGroup or confdb.ControlGroup seems better.

pedronis

thank you, did a pass, main comment is how we store views in ControlGroup for usage

pedronis · 2024-12-03T16:01:31Z

registry/registry_test.go

@@ -142,7 +142,7 @@ func (*viewSuite) TestNewRegistry(c *C) {
 			c.Assert(err.Error(), Equals, tc.err, cmt)
 		} else {
 			c.Assert(err, IsNil, cmt)
-			c.Check(registry, Not(IsNil), cmt)
+			c.Check(registry, NotNil, cmt)


for next time: it's best to do this kind of small tweaks/fixes as their own PR or commit with something like "drive-by" in the commit message

Thanks, I'll keep this in mind for future changes.

pedronis · 2024-12-03T16:01:48Z

registry/registry.go

@@ -450,7 +450,7 @@ func getPlaceholders(viewStr string) map[string]bool {
 	return placeholders
 }

-// View returns an view from the registry.
+// View returns a view from the registry.


pedronis · 2024-12-03T16:02:53Z

registry/confdb_control.go

+	validAccountID = regexp.MustCompile("^(?:[a-z0-9A-Z]{32}|[-a-z0-9]{2,28})$")
+)
+
+type AuthenticationMethod string


this probably merits a doc comment

I've added a comment here and to the OperatorKey & Store options

pedronis · 2024-12-03T16:06:52Z

registry/confdb_control.go

+// ControlGroup holds a set of views delegated through the given authentication.
+type ControlGroup struct {
+	Authentication []AuthenticationMethod
+	Views          []string


shouldn't this contain structs already parsed into account/confdb/view ?

In the assertion, the list of groups is flat.. like so:

groups: - operator-id: john authentication: [ operator-key, store ] views: - a/b/c - d/e/f - operator-id: john authentication: [ store ] views: - x/y/z

Internally though, we're grouping by operator to make this easier to look up later since all the API operations revolve around an operator. For instance, isDelegated(operator, view), delegate(operator, views, authMethods), revoke(operator, ...).

operators: john: ID: john Groups: - Authentication: [ operator-key, store ] Views: - a/b/c - d/e/f - Authentication: [ store ] Views: - x/y/z

Edit:

Or do you mean we should store the Views as a "set" for faster lookup.. Something like map[string]struct{}?

I mean of doing something like this after the parsing:

Views []*ViewRef type ViewRef struct { Account string Confdb string View string }

or similar

does this make sense to you @miguelpires as well ?

Ah, okay. That makes sense. I've made the necessary changes to reflect this.

makes sense to me 👍

pedronis · 2024-12-03T16:07:48Z

registry/confdb_control.go

+
+// compact replaces consecutive runs of equal elements with a single copy.
+// The provided slice s should be sorted.
+func compact[T comparable](s []T) []T {


unique might be a more common name for this

pedronis · 2024-12-03T16:09:22Z

asserts/registry.go

@@ -108,3 +109,110 @@ func assembleRegistry(assert assertionBase) (Assertion, error) {
 		timestamp:     timestamp,
 	}, nil
 }
+
+// ConfdbControl holds a confdb-control assertion, which holds lists of
+// views delegated by the device to an operator.


s/to an operator/to operators/

pedronis · 2024-12-03T16:11:32Z

registry/confdb_control.go

+}
+
+// convertToAuthenticationMethod converts and validates a []string to []AuthenticationMethod
+func convertToAuthenticationMethod(methods []string) ([]AuthenticationMethod, error) {


maybe "...Methods" as it is producing a slice

pedronis · 2024-12-03T16:13:18Z

registry/confdb_control.go

+	})
+
+	// remove duplicates
+	methods = compact(methods)


I don't see code in the tests that check that this is done

It's a bit hidden but this test checks that behavior

changed enough that might be a good idea to re-review

st3v3nmw force-pushed the add-confdb-control-assertion branch from 86f5f1a to 8123a6d Compare November 11, 2024 11:28

asserts: define confdb-control assertion

eaf6576

st3v3nmw force-pushed the add-confdb-control-assertion branch from 8123a6d to eaf6576 Compare November 11, 2024 11:52

asserts: convert []string to []AuthenticationMethod in check

1b61656

st3v3nmw marked this pull request as ready for review November 11, 2024 14:21

miguelpires self-requested a review November 12, 2024 09:33

st3v3nmw changed the title ~~asserts: define confdb-control assertion~~ asserts,confdb: define confdb-control assertion Nov 14, 2024

st3v3nmw mentioned this pull request Nov 14, 2024

o/devicestate: add general method to sign no-authority assertions #14723

Open

miguelpires reviewed Nov 18, 2024

View reviewed changes

asserts,registry: fixes after review

4a865bb

st3v3nmw changed the title ~~asserts,confdb: define confdb-control assertion~~ asserts,registry: define confdb-control assertion Nov 20, 2024

registry: fix package import in confdb_control_test

d7fa30a

pedronis added the Needs Samuele review Needs a review from Samuele before it can land label Nov 20, 2024

registry,asserts: better errors

7be7007

st3v3nmw requested a review from miguelpires November 20, 2024 14:40

miguelpires previously approved these changes Nov 26, 2024

View reviewed changes

miguelpires requested a review from pedronis November 26, 2024 15:37

asserts,registry: rename Group to ControlGroup

12b25f4

pedronis reviewed Dec 3, 2024

View reviewed changes

st3v3nmw added 2 commits December 4, 2024 11:51

asserts,registry: fixes after code review

1532482

registry: convert ControlGroup.Views to a struct

a1c0302

st3v3nmw requested a review from pedronis December 4, 2024 10:06

registry: add doc comment to ViewRef

c3bdbd0

	errPrefix := fmt.Sprintf("group at position %d", i+1)
	errPrefix := fmt.Sprintf("cannot parse group at position %d", i+1)

asserts,registry: define confdb-control assertion #14705

Are you sure you want to change the base?

asserts,registry: define confdb-control assertion #14705

Conversation

st3v3nmw commented Nov 11, 2024

codecov bot commented Nov 11, 2024 • edited Loading

Codecov Report

miguelpires commented Nov 12, 2024

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

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

miguelpires 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 Dec 3, 2024 • edited Loading

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

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

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

codecov bot commented Nov 11, 2024 •

edited

Loading

pedronis Dec 3, 2024 •

edited

Loading