Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 38 additions & 4 deletions internal/api/keppel/accounts_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -291,6 +291,42 @@ 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,
},
})
tr.DBChanges().AssertEqual(`
UPDATE accounts SET rbac_policies_json = '[{"match_repository":"verylongverylongverylongverylong","permissions":["anonymous_pull"]},{"match_repository":"evenlongerevenlongerevenlongerevenlonger","permissions":["anonymous_pull"]}]', anon_rbac_policies_json = '' WHERE name = 'second';
`)

// test POST /keppel/v1/:accounts/sublease success case (error cases are in
// TestPutAccountErrorCases and TestGetPutAccountReplicationOnFirstUse)
Expand All @@ -299,9 +335,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) {
Expand Down
4 changes: 4 additions & 0 deletions internal/keppel/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,10 @@ 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 '';
`,
}

// DBInterface is implemented by both [*gsql.DB] and [*gsql.Tx].
Expand Down
130 changes: 112 additions & 18 deletions internal/keppel/rbac_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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")
}
}

Expand All @@ -84,55 +92,66 @@ 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
refersToPerm[perm] = true
}
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
refersToPerm[perm] = true
}

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 {
// the "permissions" field is not documented as optional, so `null` values should be avoided and empty lists should only be represented as `[]`
r.Permissions = []RBACPermission{}
}

return nil
if r.UserNamePattern == "" {
Comment thread
majewsky marked this conversation as resolved.
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.
Expand All @@ -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
}

Comment thread
SuperSandro2000 marked this conversation as resolved.
*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
}
10 changes: 10 additions & 0 deletions internal/models/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ type Account struct {

// RBACPoliciesJSON contains a JSON string of []keppel.RBACPolicy, or the empty string.
RBACPoliciesJSON string `db:"rbac_policies_json"`
// AnonymousRBACPoliciesJSON contains a JSON string of []keppel.AnonymousRBACPolicy.
// If 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, or the empty string.
GCPoliciesJSON string `db:"gc_policies_json"`
// SecurityScanPoliciesJSON contains a JSON string of []keppel.SecurityScanPolicy, or the empty string.
Expand Down Expand Up @@ -87,6 +90,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"`
Expand All @@ -112,3 +117,8 @@ 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 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
17 changes: 15 additions & 2 deletions internal/processor/accounts.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.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)
Comment thread
majewsky marked this conversation as resolved.
if err == nil && len(buf) <= models.AnonymousRBACPoliciesJSONMaxLength {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we use some other estimation to end the loop early if it is likely to big? Right now we would build the entire string up to all to only throw it away if it is longer than 64 chars. Maybe we use an amount and say no more than 5 entries?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is during PutAccount, which is a rare operation, so I find this acceptable. (Also, we're handling all the RBAC policies anyway, so it's only causing more work proportional to the existing amount of work.)

The optimization target is to avoid loading lots of data in ReducedAccount when not necessary.

targetAccount.AnonymousRBACPoliciesJSON = string(buf)
} else {
targetAccount.AnonymousRBACPoliciesJSON = ""
}
}

// validate validation policy
Expand Down
2 changes: 1 addition & 1 deletion internal/tasks/account_management_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())

Expand Down