diff --git a/internal/api/auth/api_test.go b/internal/api/auth/api_test.go index d9029ffba..ab8c8d321 100644 --- a/internal/api/auth/api_test.go +++ b/internal/api/auth/api_test.go @@ -578,7 +578,7 @@ func TestIssueToken(t *testing.T) { ctx := t.Context() // setup RBAC policies for test - rbacPoliciesJSONStr := "" + rbacPoliciesJSONStr := "[]" if c.RBACPolicy != nil { buf := must.ReturnT(json.Marshal([]keppel.RBACPolicy{*c.RBACPolicy}))(t) rbacPoliciesJSONStr = string(buf) diff --git a/internal/api/keppel/accounts_test.go b/internal/api/keppel/accounts_test.go index a83e2de0f..80fe5c51a 100644 --- a/internal/api/keppel/accounts_test.go +++ b/internal/api/keppel/accounts_test.go @@ -238,7 +238,7 @@ func TestAccountsAPI(t *testing.T) { }) tr.DBChanges().AssertEqual(` INSERT INTO accounts (name, auth_tenant_id) VALUES ('first', 'tenant1'); - INSERT INTO accounts (name, auth_tenant_id, gc_policies_json, rbac_policies_json, tag_policies_json) VALUES ('second', 'tenant1', '[{"match_repository":".*/database","except_repository":"archive/.*","time_constraint":{"on":"pushed_at","newer_than":{"value":10,"unit":"d"}},"action":"protect"},{"match_repository":".*","only_untagged":true,"action":"delete"}]', '[{"match_repository":"library/.*","permissions":["anonymous_pull"]},{"match_repository":"library/alpine","match_username":".*@tenant2","permissions":["pull","push"]}]', '[{"match_repository":"library/.*","block_overwrite":true},{"match_repository":"library/alpine","block_delete":true}]'); + INSERT INTO accounts (name, auth_tenant_id, gc_policies_json, rbac_policies_json, tag_policies_json, anon_rbac_policies_json) VALUES ('second', 'tenant1', '[{"match_repository":".*/database","except_repository":"archive/.*","time_constraint":{"on":"pushed_at","newer_than":{"value":10,"unit":"d"}},"action":"protect"},{"match_repository":".*","only_untagged":true,"action":"delete"}]', '[{"match_repository":"library/.*","permissions":["anonymous_pull"]},{"match_repository":"library/alpine","match_username":".*@tenant2","permissions":["pull","push"]}]', '[{"match_repository":"library/.*","block_overwrite":true},{"match_repository":"library/alpine","block_delete":true}]', '[{"r":"library/.*","p":"p"}]'); `) // check editing of RBAC policies @@ -291,6 +291,43 @@ func TestAccountsAPI(t *testing.T) { }, }, ) + tr.DBChanges().AssertEqual(` + UPDATE accounts SET gc_policies_json = '[]', rbac_policies_json = '[{"match_repository":"library/alpine","match_username":".*@tenant2","permissions":["pull"]},{"match_repository":"library/alpine","match_username":".*@tenant3","permissions":["pull","delete"]}]', tag_policies_json = '[]', anon_rbac_policies_json = '[]' WHERE name = 'second'; + `) + + // check length restriction for accounts.anon_rbac_policies_json: once the payload would grow too large, + // the field is left empty instead and AuthZ for anonymous users needs to inspect accounts.rbac_policies_json + // (this protects against unbounded growth of ReducedAccount contents) + newRBACPoliciesJSON = []jsonmatch.Object{ + { + "match_repository": "verylongverylongverylongverylong", + "permissions": []string{"anonymous_pull"}, + }, + { + "match_repository": "evenlongerevenlongerevenlongerevenlonger", + "permissions": []string{"anonymous_pull"}, + }, + } + s.RespondTo(ctx, "PUT /keppel/v1/accounts/second", + withPerms("change:tenant1"), + httptest.WithJSONBody(map[string]any{ + "account": map[string]any{ + "auth_tenant_id": "tenant1", + "rbac_policies": newRBACPoliciesJSON, + }, + }), + ).ExpectJSON(t, http.StatusOK, jsonmatch.Object{ + "account": jsonmatch.Object{ + "name": "second", + "auth_tenant_id": "tenant1", + "metadata": nil, + "rbac_policies": newRBACPoliciesJSON, + }, + }) + // in the diff below, it is noteworthy that accounts.anon_rbac_policies_json remains at its previous value of "[]" + tr.DBChanges().AssertEqual(` + UPDATE accounts SET rbac_policies_json = '[{"match_repository":"verylongverylongverylongverylong","permissions":["anonymous_pull"]},{"match_repository":"evenlongerevenlongerevenlongerevenlonger","permissions":["anonymous_pull"]}]' WHERE name = 'second'; + `) // test POST /keppel/v1/:accounts/sublease success case (error cases are in // TestPutAccountErrorCases and TestGetPutAccountReplicationOnFirstUse) @@ -299,9 +336,7 @@ func TestAccountsAPI(t *testing.T) { ExpectJSON(t, http.StatusOK, jsonmatch.Object{ "sublease_token": makeSubleaseToken("second", "registry.example.org", "this-is-the-token"), }) - tr.DBChanges().AssertEqual(` - UPDATE accounts SET gc_policies_json = '[]', rbac_policies_json = '[{"match_repository":"library/alpine","match_username":".*@tenant2","permissions":["pull"]},{"match_repository":"library/alpine","match_username":".*@tenant3","permissions":["pull","delete"]}]', tag_policies_json = '[]' WHERE name = 'second'; - `) + tr.DBChanges().AssertEmpty() } func TestAccountValidationPolicies(t *testing.T) { diff --git a/internal/api/keppel/api_test.go b/internal/api/keppel/api_test.go index 0aa7e9ce2..c808ab7bb 100644 --- a/internal/api/keppel/api_test.go +++ b/internal/api/keppel/api_test.go @@ -37,7 +37,7 @@ func TestAlternativeAuthSchemes(t *testing.T) { ) s.RespondTo(ctx, "GET /keppel/v1/accounts/test1/repositories/foo/_manifests"). ExpectJSON(t, http.StatusOK, jsonmatch.Object{"manifests": []jsonmatch.Object{}}) - test.MustExec(t, s.DB, `UPDATE accounts SET rbac_policies_json = $2 WHERE name = $1`, "test1", "") + test.MustExec(t, s.DB, `UPDATE accounts SET rbac_policies_json = $2 WHERE name = $1`, "test1", "[]") // test bearer token auth: obtain a bearer token on the Auth API while // authenticating with Keppel API Auth, then use the bearer token on the diff --git a/internal/api/registry/blobs_test.go b/internal/api/registry/blobs_test.go index 03267ed65..e5f839fcf 100644 --- a/internal/api/registry/blobs_test.go +++ b/internal/api/registry/blobs_test.go @@ -133,7 +133,7 @@ func TestBlobMonolithicUpload(t *testing.T) { s.RespondTo(ctx, "GET /v2/test1/foo/blobs/"+blob.Digest.String()). ExpectBody(t, http.StatusOK, blob.Contents) - test.MustExec(t, s.DB, `UPDATE accounts SET rbac_policies_json = $2 WHERE name = $1`, "test1", "") + test.MustExec(t, s.DB, `UPDATE accounts SET rbac_policies_json = $2 WHERE name = $1`, "test1", "[]") }) } diff --git a/internal/api/registry/manifests_test.go b/internal/api/registry/manifests_test.go index 0a1855a20..9e4083e54 100644 --- a/internal/api/registry/manifests_test.go +++ b/internal/api/registry/manifests_test.go @@ -318,7 +318,7 @@ func TestImageManifestLifecycle(t *testing.T) { s.RespondTo(ctx, "GET /v2/test1/foo/manifests/"+image.Manifest.Digest.String()). Expect(containsManifest(t, image.Manifest)) - test.MustExec(t, s.DB, `UPDATE accounts SET rbac_policies_json = $2 WHERE name = $1`, "test1", "") + test.MustExec(t, s.DB, `UPDATE accounts SET rbac_policies_json = $2 WHERE name = $1`, "test1", "[]") // DELETE failure case: no delete permission s.RespondTo(ctx, "DELETE /v2/test1/foo/manifests/"+image.Manifest.Digest.String(), diff --git a/internal/keppel/database.go b/internal/keppel/database.go index dd2603687..5cd4e055f 100644 --- a/internal/keppel/database.go +++ b/internal/keppel/database.go @@ -330,6 +330,25 @@ var sqlMigrations = map[int64]string{ ALTER TABLE accounts ADD CONSTRAINT platform_filter_sync_on_replicas CHECK ((upstream_peer_hostname = '') = (next_platform_filter_sync_at IS NULL)); `, + 58: ` + ALTER TABLE accounts + ADD COLUMN anon_rbac_policies_json TEXT NOT NULL DEFAULT '[]'; + ALTER TABLE accounts + ADD CONSTRAINT anon_rbac_policies_not_empty CHECK (anon_rbac_policies_json != ''); + `, + 59: ` + ALTER TABLE accounts + ALTER COLUMN rbac_policies_json SET DEFAULT '[]'; + UPDATE accounts SET rbac_policies_json = '[]' WHERE rbac_policies_json = ''; + UPDATE accounts SET gc_policies_json = '[]' WHERE gc_policies_json = ''; + UPDATE accounts SET tag_policies_json = '[]' WHERE tag_policies_json = ''; + UPDATE accounts SET security_scan_policies_json = '[]' WHERE security_scan_policies_json = ''; + ALTER TABLE accounts + ADD CONSTRAINT rbac_policies_not_empty CHECK (rbac_policies_json != ''), + ADD CONSTRAINT gc_policies_not_empty CHECK (gc_policies_json != ''), + ADD CONSTRAINT tag_policies_not_empty CHECK (tag_policies_json != ''), + ADD CONSTRAINT security_scan_policies_not_empty CHECK (security_scan_policies_json != ''); + `, } // DBInterface is implemented by both [*gsql.DB] and [*gsql.Tx]. diff --git a/internal/keppel/gc_policy.go b/internal/keppel/gc_policy.go index 538f20744..654dfb21c 100644 --- a/internal/keppel/gc_policy.go +++ b/internal/keppel/gc_policy.go @@ -182,7 +182,7 @@ func (g GCPolicy) Validate() error { // ParseGCPolicies parses the GC policies for the given account. func ParseGCPolicies(account models.Account) ([]GCPolicy, error) { - if account.GCPoliciesJSON == "" || account.GCPoliciesJSON == "[]" { + if account.GCPoliciesJSON == "[]" { return nil, nil } var policies []GCPolicy diff --git a/internal/keppel/rbac_policy.go b/internal/keppel/rbac_policy.go index d6ab6b5ce..7cf8be75b 100644 --- a/internal/keppel/rbac_policy.go +++ b/internal/keppel/rbac_policy.go @@ -5,12 +5,15 @@ package keppel import ( "bytes" - "encoding/json" + "encoding/json/jsontext" + "encoding/json/v2" "errors" "fmt" "net" + "strings" "github.com/sapcc/go-bits/regexpext" + . "go.xyrillian.de/gg/option" "github.com/sapcc/keppel/internal/models" ) @@ -64,18 +67,23 @@ func (r RBACPolicy) Matches(ip, repoName, userName string) bool { return true } -// ValidateAndNormalize performs some normalizations and returns an error if -// this policy is invalid. -func (r *RBACPolicy) ValidateAndNormalize(strategy ReplicationStrategy) error { +// ValidateAndNormalize performs some normalizations and returns an error if this policy is invalid. +// On success, if the policy governs access for anonymous users, the respective [AnonymousRBACPolicy] is returned. +// Otherwise, if the policy governs access for authenticated users, [None] is returned. +// +// [None]: https://pkg.go.dev/go.xyrillian.de/gg/option#None +func (r *RBACPolicy) ValidateAndNormalize(strategy ReplicationStrategy) (Option[AnonymousRBACPolicy], error) { + var none Option[AnonymousRBACPolicy] // for use in error returns + if r.CidrPattern != "" { _, network, err := net.ParseCIDR(r.CidrPattern) if err != nil { // err.Error() sadly does not contain any useful information why the cidr is invalid - return fmt.Errorf("%q is not a valid CIDR", r.CidrPattern) + return none, fmt.Errorf("%q is not a valid CIDR", r.CidrPattern) } r.CidrPattern = network.String() if network.String() == "0.0.0.0/0" { - return errors.New("0.0.0.0/0 cannot be used as CIDR because it matches everything") + return none, errors.New("0.0.0.0/0 cannot be used as CIDR because it matches everything") } } @@ -84,7 +92,7 @@ func (r *RBACPolicy) ValidateAndNormalize(strategy ReplicationStrategy) error { refersToPerm := make(map[RBACPermission]bool) // set of permissions named in either `r.Permissions` or `r.NegativePermissions` for _, perm := range r.Permissions { if !isRBACPermission[perm] { - return fmt.Errorf("%q is not a valid RBAC policy permission", perm) + return none, fmt.Errorf("%q is not a valid RBAC policy permission", perm) } grantsPerm[perm] = true forbidsPerm[perm] = false @@ -92,10 +100,10 @@ func (r *RBACPolicy) ValidateAndNormalize(strategy ReplicationStrategy) error { } for _, perm := range r.ForbiddenPermissions { if !isRBACPermission[perm] { - return fmt.Errorf("%q is not a valid RBAC policy permission", perm) + return none, fmt.Errorf("%q is not a valid RBAC policy permission", perm) } if grantsPerm[perm] { - return fmt.Errorf("%q cannot be granted and forbidden by the same RBAC policy", perm) + return none, fmt.Errorf("%q cannot be granted and forbidden by the same RBAC policy", perm) } grantsPerm[perm] = false forbidsPerm[perm] = true @@ -103,28 +111,28 @@ func (r *RBACPolicy) ValidateAndNormalize(strategy ReplicationStrategy) error { } if len(r.Permissions) == 0 && len(r.ForbiddenPermissions) == 0 { - return errors.New(`RBAC policy must grant at least one permission`) + return none, errors.New(`RBAC policy must grant at least one permission`) } if r.CidrPattern == "" && r.UserNamePattern == "" && r.RepositoryPattern == "" { - return errors.New(`RBAC policy must have at least one "match_..." attribute`) + return none, errors.New(`RBAC policy must have at least one "match_..." attribute`) } if (refersToPerm[RBACAnonymousPullPermission] || refersToPerm[RBACAnonymousFirstPullPermission]) && r.UserNamePattern != "" { - return errors.New(`RBAC policy with "anonymous_pull" or "anonymous_first_pull" may not have the "match_username" attribute`) + return none, errors.New(`RBAC policy with "anonymous_pull" or "anonymous_first_pull" may not have the "match_username" attribute`) } if refersToPerm[RBACPullPermission] && r.UserNamePattern == "" { - return errors.New(`RBAC policy with "pull" must have the "match_username" attribute`) + return none, errors.New(`RBAC policy with "pull" must have the "match_username" attribute`) } if grantsPerm[RBACPushPermission] && !grantsPerm[RBACPullPermission] { - return errors.New(`RBAC policy with "push" must also grant "pull"`) + return none, errors.New(`RBAC policy with "push" must also grant "pull"`) } if grantsPerm[RBACAnonymousFirstPullPermission] && !grantsPerm[RBACAnonymousPullPermission] { - return errors.New(`RBAC policy with "anonymous_first_pull" must also grant "anonymous_pull"`) + return none, errors.New(`RBAC policy with "anonymous_first_pull" must also grant "anonymous_pull"`) } if refersToPerm[RBACDeletePermission] && r.UserNamePattern == "" { - return errors.New(`RBAC policy with "delete" must have the "match_username" attribute`) + return none, errors.New(`RBAC policy with "delete" must have the "match_username" attribute`) } if refersToPerm[RBACAnonymousFirstPullPermission] && strategy == NoReplicationStrategy { - return errors.New(`RBAC policy with "anonymous_first_pull" may only be for replica accounts`) + return none, errors.New(`RBAC policy with "anonymous_first_pull" may only be for replica accounts`) } if len(r.Permissions) == 0 { @@ -132,7 +140,18 @@ func (r *RBACPolicy) ValidateAndNormalize(strategy ReplicationStrategy) error { r.Permissions = []RBACPermission{} } - return nil + if r.UserNamePattern == "" { + return Some(AnonymousRBACPolicy{ + cidrPattern: r.CidrPattern, + repositoryPattern: string(r.RepositoryPattern), + grantsPull: grantsPerm[RBACAnonymousPullPermission], + forbidsPull: forbidsPerm[RBACAnonymousPullPermission], + grantsFirstPull: grantsPerm[RBACAnonymousFirstPullPermission], + forbidsFirstPull: forbidsPerm[RBACAnonymousFirstPullPermission], + }), nil + } else { + return None[AnonymousRBACPolicy](), nil + } } // ParseRBACPolicies parses the RBAC policies for the given account. @@ -152,3 +171,78 @@ func ParseRBACPoliciesField(buf []byte) ([]RBACPolicy, error) { err := json.Unmarshal(buf, &policies) return policies, err } + +// AnonymousRBACPolicy is a trimmed-down version of [RBACPolicy] that only covers access control for anonymous users: +// +// - Policies matching on user name cannot be converted into this format. +// - Policies granting permissions other than [RBACAnonymousPullPermission] and [RBACAnonymousFirstPullPermission] cannot be converted into this format. +// +// When serialized into JSON, this type yields an extremely compact encoding. +// Anonymous RBAC policies are meant for reading from the DB even during extremely hot paths, +// if doing so can avoid issuing tokens with cryptographic signatures and incurring the performance penalty of verifying these signatures. +type AnonymousRBACPolicy struct { + cidrPattern string + repositoryPattern string + grantsPull bool + grantsFirstPull bool + forbidsPull bool + forbidsFirstPull bool +} + +// serializedAnonymousRBACPolicy defines how [AnonymousRBACPolicy] gets serialized as JSON. +type serializedAnonymousRBACPolicy struct { + CidrPattern string `json:"c,omitempty"` + RepositoryPattern string `json:"r,omitempty"` + Permissions string `json:"p"` +} + +// MarshalJSONTo implements the [json.MarshalerTo] interface. +func (a AnonymousRBACPolicy) MarshalJSONTo(enc *jsontext.Encoder) error { + var perms []string + if a.grantsPull { + perms = append(perms, "p") + } + if a.forbidsPull { + perms = append(perms, "!p") + } + if a.grantsFirstPull { + perms = append(perms, "f") + } + if a.forbidsFirstPull { + perms = append(perms, "!f") + } + return json.MarshalEncode(enc, serializedAnonymousRBACPolicy{ + CidrPattern: a.cidrPattern, + RepositoryPattern: a.repositoryPattern, + Permissions: strings.Join(perms, ","), + }) +} + +// UnmarshalJSONFrom implements the [json.UnmarshalerFrom] interface. +func (a *AnonymousRBACPolicy) UnmarshalJSONFrom(dec *jsontext.Decoder) error { + var s serializedAnonymousRBACPolicy + err := json.UnmarshalDecode(dec, &s) + if err != nil { + return err + } + + *a = AnonymousRBACPolicy{ + cidrPattern: s.CidrPattern, + repositoryPattern: s.RepositoryPattern, + } + for perm := range strings.SplitSeq(s.Permissions, ",") { + switch perm { + case "p": + a.grantsPull = true + case "!p": + a.forbidsPull = true + case "f": + a.grantsFirstPull = true + case "!f": + a.forbidsFirstPull = true + default: + return &json.SemanticError{Err: fmt.Errorf("invalid permission code: %q", perm)} + } + } + return nil +} diff --git a/internal/keppel/tag_policy.go b/internal/keppel/tag_policy.go index 7eae180a0..fcf23700e 100644 --- a/internal/keppel/tag_policy.go +++ b/internal/keppel/tag_policy.go @@ -17,7 +17,7 @@ type TagPolicy struct { // ParseTagPolicies parses the Tag policies for the given account. func ParseTagPolicies(tagPoliciesJSON string) ([]TagPolicy, error) { - if tagPoliciesJSON == "" || tagPoliciesJSON == "[]" { + if tagPoliciesJSON == "[]" { return nil, nil } var policies []TagPolicy diff --git a/internal/models/account.go b/internal/models/account.go index 5556a316b..23a174fb0 100644 --- a/internal/models/account.go +++ b/internal/models/account.go @@ -36,13 +36,16 @@ type Account struct { // IsManaged indicates if the account was created by AccountManagementDriver IsManaged bool `db:"is_managed"` - // RBACPoliciesJSON contains a JSON string of []keppel.RBACPolicy, or the empty string. + // RBACPoliciesJSON contains a JSON string of []keppel.RBACPolicy. RBACPoliciesJSON string `db:"rbac_policies_json"` - // GCPoliciesJSON contains a JSON string of []keppel.GCPolicy, or the empty string. + // AnonymousRBACPoliciesJSON contains a JSON string of []keppel.AnonymousRBACPolicy. + // If the list is empty, AuthZ for anonymous users must fall back to the full set of RBAC policies instead. + AnonymousRBACPoliciesJSON string `db:"anon_rbac_policies_json"` + // GCPoliciesJSON contains a JSON string of []keppel.GCPolicy. GCPoliciesJSON string `db:"gc_policies_json"` - // SecurityScanPoliciesJSON contains a JSON string of []keppel.SecurityScanPolicy, or the empty string. + // SecurityScanPoliciesJSON contains a JSON string of []keppel.SecurityScanPolicy. SecurityScanPoliciesJSON string `db:"security_scan_policies_json"` - // TagPoliciesJSON contains a JSON string of []keppel.TagPolicy, or the empty string. + // TagPoliciesJSON contains a JSON string of []keppel.TagPolicy. TagPoliciesJSON string `db:"tag_policies_json"` NextBlobSweepedAt Option[time.Time] `db:"next_blob_sweep_at"` // see tasks.BlobSweepJob @@ -80,6 +83,27 @@ func (a Account) IsReplica() bool { return a.UpstreamPeerHostName != "" || a.ExternalPeerURL != "" } +// ApplyDefaultsToAccount fills default values for various fields of type Account +// in order to simplify writing down struct literals in tests. +func ApplyDefaultsToAccount(a Account) Account { + if a.RBACPoliciesJSON == "" { + a.RBACPoliciesJSON = "[]" + } + if a.GCPoliciesJSON == "" { + a.GCPoliciesJSON = "[]" + } + if a.SecurityScanPoliciesJSON == "" { + a.SecurityScanPoliciesJSON = "[]" + } + if a.TagPoliciesJSON == "" { + a.TagPoliciesJSON = "[]" + } + if a.AnonymousRBACPoliciesJSON == "" { + a.AnonymousRBACPoliciesJSON = "[]" + } + return a +} + // ReducedAccount contains just the fields from type Account that the Registry API is most interested in. // This type exists to avoid loading the large payload fields in type Account when we don't need to, // which is a significant memory optimization for the keppel-api process. @@ -87,6 +111,8 @@ type ReducedAccount struct { Name AccountName `db:"name"` AuthTenantID string `db:"auth_tenant_id"` + // TODO: add AnonymousRBACPoliciesJSON (when adding light-weight tokens for anonymous users) + // replication policy UpstreamPeerHostName string `db:"upstream_peer_hostname"` ExternalPeerURL string `db:"external_peer_url"` @@ -112,3 +138,9 @@ var ReducedAccountStore = oblast.MustNewStore[ReducedAccount]( func (a ReducedAccount) IsReplica() bool { return a.UpstreamPeerHostName != "" || a.ExternalPeerURL != "" } + +// AnonymousRBACPoliciesJSONMaxLength is the maximum length of the [Account.AnonymousRBACPoliciesJSON] field. +// If this length is exceeded, the field will be left empty (only holding an empty array) +// and AuthZ for anonymous users needs to inspect the full set of RBAC policies. +// This protects [ReducedAccount] from growing beyond a reasonable size. +const AnonymousRBACPoliciesJSONMaxLength = 64 diff --git a/internal/processor/accounts.go b/internal/processor/accounts.go index 6386a7e0e..50f281320 100644 --- a/internal/processor/accounts.go +++ b/internal/processor/accounts.go @@ -160,19 +160,32 @@ func (p *Processor) CreateOrUpdateAccount(ctx context.Context, account keppel.Ac replicationStrategy = rp.Strategy } - // validate RBAC policies + // validate RBAC policies, and fill AnonymousRBACPoliciesJSON with just the RBAC policies for anonymous users if len(account.RBACPolicies) == 0 { - targetAccount.RBACPoliciesJSON = "" + targetAccount.RBACPoliciesJSON = "[]" + targetAccount.AnonymousRBACPoliciesJSON = "[]" } else { + anonPolicies := []keppel.AnonymousRBACPolicy{} for idx, policy := range account.RBACPolicies { - err := policy.ValidateAndNormalize(replicationStrategy) + anonPolicy, err := policy.ValidateAndNormalize(replicationStrategy) if err != nil { return models.Account{}, keppel.AsRegistryV2Error(err).WithStatus(http.StatusUnprocessableEntity) } account.RBACPolicies[idx] = policy + + if policy, ok := anonPolicy.Unpack(); ok { + anonPolicies = append(anonPolicies, policy) + } } buf, _ := json.Marshal(account.RBACPolicies) targetAccount.RBACPoliciesJSON = string(buf) + + buf, err := json.Marshal(anonPolicies) + if err == nil && len(buf) <= models.AnonymousRBACPoliciesJSONMaxLength { + targetAccount.AnonymousRBACPoliciesJSON = string(buf) + } else { + targetAccount.AnonymousRBACPoliciesJSON = "[]" + } } // validate validation policy diff --git a/internal/tasks/account_management_test.go b/internal/tasks/account_management_test.go index 65e09c395..0b560b349 100644 --- a/internal/tasks/account_management_test.go +++ b/internal/tasks/account_management_test.go @@ -48,7 +48,7 @@ func TestAccountManagementBasic(t *testing.T) { // since we are enforcing that account, no error is returned assert.ErrEqual(t, managedAccountsJob.ProcessOne(s.Ctx), sql.ErrNoRows) tr.DBChanges().AssertEqualf(` - INSERT INTO accounts (name, auth_tenant_id, external_peer_url, gc_policies_json, security_scan_policies_json, rbac_policies_json, is_managed, next_enforcement_at, rule_for_manifest) VALUES ('abcde', '12345', 'registry-tertiary.example.org', '[{"match_repository":".*/database","except_repository":"archive/.*","time_constraint":{"on":"pushed_at","newer_than":{"value":6,"unit":"h"}},"action":"protect"},{"match_repository":".*","only_untagged":true,"action":"delete"}]', '[{"match_repository":".*","match_vulnerability_id":".*","except_fix_released":true,"action":{"assessment":"risk accepted: vulnerabilities without an available fix are not actionable","ignore":true}}]', '[{"match_repository":"library/.*","permissions":["anonymous_pull"]},{"match_repository":"library/alpine","match_username":".*@tenant2","permissions":["pull","push"]}]', TRUE, %d, '''important-label'' in labels && ''some-label'' in labels'); + INSERT INTO accounts (name, auth_tenant_id, external_peer_url, gc_policies_json, security_scan_policies_json, rbac_policies_json, is_managed, next_enforcement_at, rule_for_manifest, anon_rbac_policies_json) VALUES ('abcde', '12345', 'registry-tertiary.example.org', '[{"match_repository":".*/database","except_repository":"archive/.*","time_constraint":{"on":"pushed_at","newer_than":{"value":6,"unit":"h"}},"action":"protect"},{"match_repository":".*","only_untagged":true,"action":"delete"}]', '[{"match_repository":".*","match_vulnerability_id":".*","except_fix_released":true,"action":{"assessment":"risk accepted: vulnerabilities without an available fix are not actionable","ignore":true}}]', '[{"match_repository":"library/.*","permissions":["anonymous_pull"]},{"match_repository":"library/alpine","match_username":".*@tenant2","permissions":["pull","push"]}]', TRUE, %d, '''important-label'' in labels && ''some-label'' in labels', '[{"r":"library/.*","p":"p"}]'); `, s.Clock.Now().Add(1*time.Hour).Unix()) @@ -91,7 +91,8 @@ func TestAccountManagementWithReplicaCreation(t *testing.T) { // The setup already includes an account "test1" set up on both ends, but we // want to test the setup of a managed replica account, so we will use a // fresh account called "managed" instead. - must.SucceedT(t, models.AccountStore.Insert(ctx, s1.DB, &models.Account{Name: "managed", AuthTenantID: "managedauthtenant"})) + managedAccount := models.ApplyDefaultsToAccount(models.Account{Name: "managed", AuthTenantID: "managedauthtenant"}) + must.SucceedT(t, models.AccountStore.Insert(ctx, s1.DB, &managedAccount)) s1.FD.NextSubleaseTokenSecretToIssue = "thisisasecret" s2.FD.ValidSubleaseTokenSecrets["managed"] = "thisisasecret" diff --git a/internal/tasks/accounts_test.go b/internal/tasks/accounts_test.go index 7026e2350..33a4dfe3a 100644 --- a/internal/tasks/accounts_test.go +++ b/internal/tasks/accounts_test.go @@ -37,7 +37,7 @@ func TestAnnounceAccountsToFederation(t *testing.T) { // setup another account; only that one should need announcing initially s.Clock.StepBy(5 * time.Minute) - account2 := models.Account{Name: "test2", AuthTenantID: "test2authtenant"} + account2 := models.ApplyDefaultsToAccount(models.Account{Name: "test2", AuthTenantID: "test2authtenant"}) must.SucceedT(t, models.AccountStore.Insert(ctx, s.DB, &account2)) assert.ErrEqual(t, accountJob.ProcessOne(s.Ctx), nil) expectAccountsAnnouncedJustNow(t, s, account2.Reduced()) @@ -71,17 +71,18 @@ func TestAccountPlatformFilterSync(t *testing.T) { // set up another replica account s2.Clock.StepBy(65 * time.Minute) - account2 := models.Account{ + account2 := models.ApplyDefaultsToAccount(models.Account{ Name: "test2", AuthTenantID: "test2authtenant", UpstreamPeerHostName: "registry.example.org", NextPlatformFilterSyncAt: Some(s2.Clock.Now().Add(1 * time.Hour)), - } + }) must.SucceedT(t, models.AccountStore.Insert(ctx, s2.DB, &account2)) - must.SucceedT(t, models.AccountStore.Insert(ctx, s1.DB, &models.Account{ + account2 = models.ApplyDefaultsToAccount(models.Account{ Name: "test2", AuthTenantID: "test2authtenant", - })) + }) + must.SucceedT(t, models.AccountStore.Insert(ctx, s1.DB, &account2)) assert.ErrEqual(t, syncJob.ProcessOne(s2.Ctx), nil) assert.ErrEqual(t, syncJob.ProcessOne(s2.Ctx), sql.ErrNoRows) diff --git a/internal/test/setup.go b/internal/test/setup.go index 90bf2fc9b..fd5f0b426 100644 --- a/internal/test/setup.go +++ b/internal/test/setup.go @@ -123,17 +123,8 @@ func WithRateLimitEngine(rle *keppel.RateLimitEngine) SetupOption { // WithAccount is a SetupOption that adds the given keppel.Account to the DB during NewSetup(). func WithAccount(account models.Account) SetupOption { return func(params *setupParams) { - // some field have default values that's not the zero value - if account.GCPoliciesJSON == "" { - account.GCPoliciesJSON = "[]" - } - if account.SecurityScanPoliciesJSON == "" { - account.SecurityScanPoliciesJSON = "[]" - } - if account.TagPoliciesJSON == "" { - account.TagPoliciesJSON = "[]" - } - params.Accounts = append(params.Accounts, account) + // some fields have default values that are not the zero value + params.Accounts = append(params.Accounts, models.ApplyDefaultsToAccount(account)) } }