From 7ca0111c0cbbd10d1264006333a9b64e373036ea Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Thu, 22 Oct 2026 05:17:29 +0000 Subject: [PATCH] fix: narrowing a privileged custom role gives its grants to the tenant's members access the host was never asked about A grant to the tenant's members of a privileged role gives nothing, so SetRole skipped it when the role was widened. Whether a redefinition widened was decided from the two definitions alone, and going from privileged to unprivileged didn't count. So narrowing the role until it was no longer privileged gave every member what the role gives, what was added while it was privileged too. ValidateGrant was never asked, and the denies on the grant's scope were never checked for what it now gives. SetRole now compares what each grant and eligibility gives through its principal, as checks read it, before and after the redefinition. A grant that gave nothing and will give something is widened however the role changed: the host is asked about it as the role will be, and the role's permissions are checked under the denies on the paths of the scopes of the grants that widen, before the transaction and again under the tenant's lock. A scope the host no longer has is read as stored only where no grant on it gives more. --- README.md | 15 ++- authority_test.go | 11 +- docs/architecture.md | 7 ++ membership_test.go | 68 +++++++++++ rbac_test.go | 80 +++++++++++++ roles.go | 274 +++++++++++++++++++++++++++---------------- 6 files changed, 349 insertions(+), 106 deletions(-) diff --git a/README.md b/README.md index 6f841a7..477aae0 100644 --- a/README.md +++ b/README.md @@ -110,8 +110,12 @@ conditional ones reach, to check one by one. scope's path keeps from changing grants there, can neither change what its grants give nor delete it, nor widen it where a deny there, or where a role listing it, or listing one that does, is granted, removes a permission it would give or the power to grant it, or the label of a scope - such a role is granted on refuses it. Renaming a role or rewriting its description changes no - grant, so needs only the power to define it. + such a role is granted on refuses it. A grant widens by what it gives through its principal, as + checks read it: one to the tenant's members of a privileged role, or to the public of a role not + marked `Public`, gives nothing, so widening the role asks nothing about it, and a redefinition + that has it give something, as narrowing the role so that it is no longer privileged does, widens + it, with what was added to the role while it gave nothing. Renaming a role or rewriting its + description changes no grant, so needs only the power to define it. `DeleteRole` removes one with its grants; `Roles` lists the built-in and custom roles a scope type takes. Custom roles follow the catalog: one gives only what the catalog still has, and is privileged when it gives what only privileged roles do, or changes access. @@ -304,10 +308,13 @@ conditional ones reach, to check one by one. `rbac.ErrNotFound`, or an error wrapping it: a change that only takes access away there, as a member leaving a group with access on it or the group itself, or joining one a deny there names, a place in the group made to end sooner, `DeleteGroup`, or `DeleteRole` deleting, or `SetRole` - narrowing, a role granted on it, reads it as unlabeled, and any other change fails with the error. + narrowing so that no grant of it there gives more, a role granted on it, reads it as unlabeled, + and any other change fails with the error. - **Privileged roles:** a role the catalog marks `Privileged`, or one that changes access, is privileged: it is never granted to the tenant's members, and gives nothing through them, as - after a catalog change. + after a catalog change. Their grants of it give what it gives once it no longer is: `SetRole` + checks a redefinition doing so as widening them, but a catalog change doing so is the host's, and + asks nothing. - **Public access:** the zero `Subject` is anonymous and holds only what is granted to the public. Only a role marked `Public` can be granted to the public, and a grant to the public gives nothing once its role no longer is. diff --git a/authority_test.go b/authority_test.go index d5c4e8a..f0f2261 100644 --- a/authority_test.go +++ b/authority_test.go @@ -2069,8 +2069,11 @@ func TestARemovalReadsAScopeTheHostNoLongerHasAsUnlabeled(t *testing.T) { } }) ctx := t.Context() + // custom.priv is privileged, as it gives root, and granted to the members on gone, as a catalog + // change would leave it. if _, err := h.pool.Exec(ctx, `INSERT INTO rbac_roles (tenant, id, permissions, resource_types) VALUES ('t1', 'custom.viewer', '{cluster:view}', '{cluster}'), - ('t1', 'custom.ops', '{cluster:view,cluster:login}', '{cluster}')`); err != nil { + ('t1', 'custom.ops', '{cluster:view,cluster:login}', '{cluster}'), ('t1', 'custom.priv', '{cluster:view,cluster:root}', '{cluster}'); + INSERT INTO rbac_grants (tenant, scope_type, scope_id, principal_kind, principal_id, role) VALUES ('t1', 'cluster', 'gone', 'members', 't1', 'custom.priv')`); err != nil { t.Fatal(err) } gone, down := rbac.Scope{Type: "cluster", ID: "gone"}, rbac.Scope{Type: "cluster", ID: "down"} @@ -2105,6 +2108,12 @@ func TestARemovalReadsAScopeTheHostNoLongerHasAsUnlabeled(t *testing.T) { if err := h.authz.SetRole(ctx, admin, "t1", ops); !errors.Is(err, rbac.ErrNotFound) { t.Errorf("widening it: %v", err) } + // Narrowed so that it is no longer privileged, a role gives the members' grant there what it + // gives then, so the host must read the scope for that too. + priv := rbac.RoleDefinition{ID: "custom.priv", Permissions: []rbac.Permission{view}, ResourceTypes: []string{"cluster"}} + if err := h.authz.SetRole(ctx, admin, "t1", priv); !errors.Is(err, rbac.ErrNotFound) { + t.Errorf("narrowing a role the members hold there so that it is no longer privileged: %v", err) + } // So with a group the host no longer has: its members are removed, and their places made to end // sooner, but none added, nor any place made to last longer. if err := h.authz.ChangeMembers(ctx, admin, "t1", []rbac.Member{{Group: "lost", Principal: identity("v")}}, nil); !errors.Is(err, rbac.ErrNotFound) { diff --git a/docs/architecture.md b/docs/architecture.md index d26e796..97126f5 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -465,6 +465,13 @@ sequenceDiagram - **Custom roles.** `SetRole` redefines a role in place for every grant of it, so it asks `ValidateGrant` about every grant and eligibility the new definition widens, before its transaction, and under the tenant's lock refuses with `ErrConflict` if those changed meanwhile. + A grant widens by what it gives through its principal, as checks read it + (`redefinition.widensTo`): one to the tenant's members of a privileged role, or to the public of + a role not marked `Public`, gives nothing, and widens once the role gives it something, however + the role changed, as narrowing it so that it is no longer privileged does. On each scope a grant + that widens is on, the denies on its path that remove from the actor a permission the role will + give refuse the change, as they would refuse `Apply` granting it there; on the others, only those + removing its power to change grants there do. - **Eligibilities.** `SetEligibility` lets a principal `Activate` a role for a while, perhaps only with approvers or a `requires` condition. An activation is a grant that ends no later than its eligibility expires, or its holder's membership or place in the group the eligibility names diff --git a/membership_test.go b/membership_test.go index 6a16d74..d531067 100644 --- a/membership_test.go +++ b/membership_test.go @@ -4065,6 +4065,74 @@ func TestWideningAPrivilegedRoleAsksNothingOfGrantsThatGiveNothing(t *testing.T) } } +// A role the catalog makes privileged gives the tenant's members nothing through a grant to them, +// so widening it asks the host nothing about that grant. Narrowing it so that it is no longer +// privileged gives them what it gives then, what was added while it gave them nothing too, so the +// host is asked about that grant, as the role will be, and refusing it refuses the narrowing. +func TestNarrowingARoleSoItIsNoLongerPrivilegedAsksAboutTheMembersGrant(t *testing.T) { + viewer := rbac.RoleDefinition{ID: "cluster.viewer", Permissions: []rbac.Permission{view}, ResourceTypes: []string{"cluster"}} + h := setup(t, func(c *rbac.Config) { c.Catalog = append(slices.Clone(c.Catalog), viewer) }) + ctx := t.Context() + admin := roleAdmin(t, h) + x := rbac.RoleDefinition{ID: "custom.x", Permissions: []rbac.Permission{login}, ResourceTypes: []string{"cluster"}} + if err := h.authz.SetRole(ctx, admin, "t1", x); err != nil { + t.Fatal(err) + } + h.seed(t, grant(rbac.TenantRoot("t1"), x.ID, rbac.Members, "t1")) + member := h.as(t, "m") + // The catalog makes logging in privileged, so x is, and the members' grant gives nothing. + cfg := h.cfg + cfg.Catalog = slices.Clone(cfg.Catalog) + for i, r := range cfg.Catalog { + if r.ID == "cluster.login" { + cfg.Catalog[i].Privileged = true + } + } + var asked []rbac.GrantInput + refuseMembers := true + cfg.ValidateGrant = func(_ context.Context, _ rbac.DB, in rbac.GrantInput) error { + asked = append(asked, in) + if refuseMembers && in.Binding.Principal.Kind == rbac.Members { + return fmt.Errorf("%w: not through the members", rbac.ErrInvalid) + } + return nil + } + authz, err := rbac.New(h.pool, cfg) + if err != nil { + t.Fatal(err) + } + if err := authz.Authorize(ctx, member, login, cluster); !errors.Is(err, rbac.ErrDenied) { + t.Fatalf("a privileged role through the members: %v", err) + } + x.Permissions = []rbac.Permission{login, view} + if err := authz.SetRole(ctx, admin, "t1", x); err != nil || len(asked) != 0 { + t.Fatalf("widening a privileged role the members hold: %v, asked %+v", err, asked) + } + // Narrowed to what a role that isn't privileged gives, the members' grant would give every + // member view on every cluster. + x.Permissions = []rbac.Permission{view} + err = authz.SetRole(ctx, admin, "t1", x) + var ie *rbac.InvalidError + if !errors.As(err, &ie) || ie.Binding.Principal != rbac.TenantMembers("t1") { + t.Fatalf("narrowing a role the members hold so that it is no longer privileged: %v", err) + } + if len(asked) != 1 || asked[0].Via != rbac.ViaRedefinition || asked[0].Scope != rbac.TenantRoot("t1") || + asked[0].Role.Privileged || !slices.Equal(asked[0].Role.Permissions, x.Permissions) || asked[0].Actor.Principal != admin.Principal { + t.Fatalf("asked = %+v", asked) + } + if err := authz.Authorize(ctx, member, view, cluster); !errors.Is(err, rbac.ErrDenied) { + t.Fatalf("a refused narrowing gave the members view: %v", err) + } + // Once the host lets the members hold it, it gives them what it gives. + refuseMembers, asked = false, nil + if err := authz.SetRole(ctx, admin, "t1", x); err != nil || len(asked) != 1 { + t.Fatalf("narrowing it once the host lets the members hold it: %v, asked %+v", err, asked) + } + if err := authz.Authorize(ctx, member, view, cluster); err != nil { + t.Fatalf("the members' grant of a role no longer privileged: %v", err) + } +} + func TestAPrivilegedRoleForTheMembersIsRefusedBeforeTheHostIsAsked(t *testing.T) { var asked []rbac.GrantInput h := setup(t, func(c *rbac.Config) { diff --git a/rbac_test.go b/rbac_test.go index ae1f095..a1bbf8f 100644 --- a/rbac_test.go +++ b/rbac_test.go @@ -7576,6 +7576,86 @@ func TestADenyBelowTheTenantStopsARedefinitionHandingOnWhatItRemoves(t *testing. } } +// A grant to the tenant's members of a privileged role gives nothing, so narrowing the role so that +// it no longer is gives them what it gives, as granting it to them would: a deny on the workspace +// that grant is on, which keeps x from handing on what it gives there, stops it, as it stops Apply +// there. A deny where only a grant to an identity is, which gives no more, stops nothing. +func TestADenyWhereTheMembersHoldARoleNoLongerPrivilegedStopsNarrowingIt(t *testing.T) { + h, guardian := guarded(t) + ctx := t.Context() + x, r := definer(t, h) + member := h.as(t, "m") + elsewhere := rbac.Resource{Tenant: "t1", Type: "cluster", ID: "c2"} + // Privileged, as it gives root, and granted to the members, as a catalog change would leave it. + if _, err := h.pool.Exec(ctx, `INSERT INTO rbac_roles (tenant, id, permissions, resource_types) VALUES ('t1', 'custom.op', '{cluster:view,cluster:root}', '{cluster}'); + INSERT INTO rbac_grants (tenant, scope_type, scope_id, principal_kind, principal_id, role) VALUES + ('t1', 'workspace', 'w1', 'members', 't1', 'custom.op'), ('t1', 'cluster', 'c2', 'user', 'y', 'custom.op')`); err != nil { + t.Fatal(err) + } + noView := rbac.Deny{ID: "no-view", Permissions: []rbac.Permission{view}, Principals: []rbac.Principal{x.Principal}} + if err := h.authz.SetDeny(ctx, guardian, workspace, noView); err != nil { + t.Fatal(err) + } + if err := h.authz.Apply(ctx, x, workspace, rbac.Change{Grant: []rbac.Binding{{Role: r.ID, Principal: rbac.TenantMembers("t1")}}}); !errors.Is(err, rbac.ErrDenied) { + t.Fatalf("granting view to the members under the deny: %v", err) + } + narrowed := rbac.RoleDefinition{ID: "custom.op", Permissions: []rbac.Permission{view}, ResourceTypes: []string{"cluster"}} + var denied *rbac.DeniedError + if err := h.authz.SetRole(ctx, x, "t1", narrowed); !errors.As(err, &denied) || denied.Scope != workspace.Scope() || allowed(t, h, member, view, cluster) { + t.Fatalf("narrowing a role the members hold under the deny so that it is no longer privileged: %v", err) + } + if err := h.authz.DeleteDeny(ctx, guardian, workspace, noView.ID); err != nil { + t.Fatal(err) + } + if err := h.authz.SetDeny(ctx, guardian, elsewhere, noView); err != nil { + t.Fatal(err) + } + if err := h.authz.SetRole(ctx, x, "t1", narrowed); err != nil || !allowed(t, h, member, view, cluster) { + t.Fatalf("narrowing it under a deny only where a grant to an identity is: %v", err) + } +} + +// What the narrowing checked of the denies where the members' grant is before its transaction it +// checks again under the tenant's lock, so a deny set there while the host is asked about that +// grant stops it. A deny elsewhere binds x, so the workspace was read before the transaction. +func TestADenySetWhileANarrowingTheMembersGainByWaitsStopsIt(t *testing.T) { + var meanwhile func(context.Context) error + h, guardian := guarded(t, func(c *rbac.Config) { + validate := c.ValidateGrant + c.ValidateGrant = func(ctx context.Context, q rbac.DB, in rbac.GrantInput) error { + if do := meanwhile; do != nil && in.Via == rbac.ViaRedefinition { + meanwhile = nil + if err := do(ctx); err != nil { + return err + } + } + return validate(ctx, q, in) + } + }) + ctx := t.Context() + x, _ := definer(t, h) + member := h.as(t, "m") + if _, err := h.pool.Exec(ctx, `INSERT INTO rbac_roles (tenant, id, permissions, resource_types) VALUES ('t1', 'custom.op', '{cluster:view,cluster:root}', '{cluster}'); + INSERT INTO rbac_grants (tenant, scope_type, scope_id, principal_kind, principal_id, role) VALUES ('t1', 'workspace', 'w1', 'members', 't1', 'custom.op')`); err != nil { + t.Fatal(err) + } + elsewhere := rbac.Resource{Tenant: "t1", Type: "cluster", ID: "c2"} + if err := h.authz.SetDeny(ctx, guardian, elsewhere, rbac.Deny{ID: "no-root", Permissions: []rbac.Permission{root}, Principals: []rbac.Principal{x.Principal}}); err != nil { + t.Fatal(err) + } + meanwhile = func(ctx context.Context) error { + return h.authz.SetDeny(ctx, guardian, workspace, rbac.Deny{ID: "no-view", Permissions: []rbac.Permission{view}, Principals: []rbac.Principal{x.Principal}}) + } + narrowed := rbac.RoleDefinition{ID: "custom.op", Permissions: []rbac.Permission{view}, ResourceTypes: []string{"cluster"}} + var denied *rbac.DeniedError + if err := h.authz.SetRole(ctx, x, "t1", narrowed); !errors.As(err, &denied) || denied.Scope != workspace.Scope() || allowed(t, h, member, view, cluster) { + t.Fatalf("narrowing a role the members hold while a deny is set where they hold it: %v", err) + } + if meanwhile != nil { + t.Fatal("the deny should have been set meanwhile") + } +} + // A deny on a container, or on the resource itself, that removes x's power to change grants there // stops x changing what a role granted there gives, or revoking it by deleting the role, as it // stops Apply there. diff --git a/roles.go b/roles.go index f2f3eb6..ce94a18 100644 --- a/roles.go +++ b/roles.go @@ -365,24 +365,31 @@ func canonical(r RoleDefinition) RoleDefinition { // constraint that stops changes to grants where it can be granted or is, as narrowing it leaves // its grants on scopes of types it is no longer for, or a deny that removes the actor's power to // change them there or on the path of a scope they are on, stops it, as it stops DeleteRole and -// Apply there; so does, where they will give more, a deny on that path that removes a permission -// the role gives, which the actor would hand on there. Renaming it or rewriting its description -// changes no grant, so needs only the power to define it: a role's name and description are part -// of its definition, not of the data its grants are on. The scopes are as Config.ResourceOf gives -// them. A role another change redefined or created while this one waited for the tenant is -// refused, to be tried again: the constraints were asked about it as it was, as is one granted -// meanwhile on a scope of a type they weren't asked about, or, it or a role listing it, on a scope -// the host wasn't asked about. One deleted meanwhile is created, as its grants went with it. +// Apply there; so does, where they will give more, as a grant to the tenant's members does once +// the role is no longer privileged, however it was narrowed, a deny on that path that removes a +// permission the role gives, which the actor would hand on there. Renaming it or rewriting its +// description changes no grant, so needs only the power to define it: a role's name and +// description are part of its definition, not of the data its grants are on. The scopes are as +// Config.ResourceOf gives them. A role another change redefined or created while this one waited +// for the tenant is refused, to be tried again: the constraints were asked about it as it was, as +// is one granted meanwhile on a scope of a type they weren't asked about, or, it or a role listing +// it, on a scope the host wasn't asked about. One deleted meanwhile is created, as its grants went +// with it. // // Redefining a role so that its grants give more, or so that it becomes privileged, changes what // grants already made give: Config.ValidateGrant is asked about each of its grants and -// eligibilities in force that gives anything as the role will be (ViaRedefinition), and refusing -// any refuses the redefinition, as granting the role so would be refused. Narrowing a role asks -// about none. One made while this waited for the tenant refuses it too, to be tried again, as the -// host wasn't asked about it. Redefining it to give more label values or powers to release is -// refused as invalid where a condition of its grants on the tenant that reads the action, which -// counts once for each (Condition), would leave theirs costing more than a check may spend, as -// granting it so would be refused. +// eligibilities in force that gives more as the role will be than it gave (ViaRedefinition), as +// checks read what a grant gives through its principal, and refusing any refuses the +// redefinition, as granting the role so would be refused. A grant to the tenant's members of a +// privileged role gives nothing, nor does one to the public of a role not marked Public, so +// widening the role asks nothing about them while they still give nothing; redefining it so that +// they give something, as narrowing it so that it is no longer privileged does, asks about them as +// the role will be, what was added to it while they gave nothing too. Narrowing a role so that no +// grant of it gives more asks about none. One made while this waited for the tenant refuses it +// too, to be tried again, as the host wasn't asked about it. Redefining it to give more label +// values or powers to release is refused as invalid where a condition of its grants on the tenant +// that reads the action, which counts once for each (Condition), would leave theirs costing more +// than a check may spend, as granting it so would be refused. func (a *Authorizer) SetRole(ctx context.Context, actor Subject, tenant string, r RoleDefinition) (err error) { a = a.still() defer a.refused(ctx, &err, tenant, actor.Principal, "SetRole") @@ -413,20 +420,18 @@ func (a *Authorizer) SetRole(ctx context.Context, actor Subject, tenant string, } checked, existed := au.roles.custom[r.ID] change := fmt.Sprintf("changing what %s's grants give", r.ID) + d := a.redefining(au.roles.custom, r) var reach roleReach if existed && !sameAccess(checked, r) { - // Narrowed, it only takes access away, as deleting it does. - var grows *RoleDefinition - if givesMore(checked, r) { - grows = &r - } // Its grants are on scopes of the types it is for, and on those it was for before and is - // for again, whose grants it brings back. - if reach, err = a.roleReachOf(ctx, actor, au, tenant, r.ID, slices.Concat(checked.ResourceTypes, r.ResourceTypes), grows, change); err != nil { + // for again, whose grants it brings back. Narrowed, it takes access away, as deleting it + // does, but where a grant of it gives more, as one to the tenant's members does once it is + // no longer privileged. + if reach, err = a.roleReachOf(ctx, actor, au, tenant, r.ID, slices.Concat(checked.ResourceTypes, r.ResourceTypes), &d, change); err != nil { return err } } - asked, stand, err := a.revalidate(ctx, actor, tenant, au.roles.custom, r) + asked, stand, err := a.revalidate(ctx, actor, tenant, d) if err != nil { return err } @@ -455,16 +460,17 @@ func (a *Authorizer) SetRole(ctx context.Context, actor Subject, tenant string, // Privileged as the roles that give its permissions on its types are, or as changing // access is, so a host that confirms privileged grants asks about one that gives root // however it was defined. Derived as a read derives it. - after, widened := a.redefining(au.roles.custom, r) + redefined := a.redefining(au.roles.custom, r) + after := redefined.now r.Privileged = after.Privileged - if was := au.roles.custom[r.ID]; r.Privileged && !was.Privileged { + if r.Privileged && !redefined.was.Privileged { if err := heldByMembers(ctx, tx, tenant, r.ID, a.cfg.Now()); err != nil { return ChangeEvent{}, gains{}, err } } // The grants it widens are the ones the host was asked about, of the roles as it was asked // about them, or one was made or a role redefined meanwhile. - need, err := a.revalidations(ctx, tx, tenant, widened) + need, err := a.revalidations(ctx, tx, tenant, redefined) if err != nil { return ChangeEvent{}, gains{}, err } @@ -672,21 +678,67 @@ func (a *Authorizer) DeleteRole(ctx context.Context, actor Subject, tenant strin }) } -// redefining returns r as checks will read it once it replaces its definition among custom, as -// read, and, when its grants will give more than they gave, r as the one role that widens. A +// redefinition is a custom role as it was, the zero role where it didn't exist, and as it will be, +// both as checks read them, for a change to what its grants and eligibilities give. +type redefinition struct{ was, now RoleDefinition } + +// redefining is r replacing its definition among custom, as read, with r as checks will read it: a // copy, as current trims it. -func (a *Authorizer) redefining(custom map[Role]RoleDefinition, r RoleDefinition) (after RoleDefinition, widened map[Role]RoleDefinition) { - after = a.current([]RoleDefinition{r.clone()})[0] - if widens(custom[r.ID], after) { - return after, map[Role]RoleDefinition{r.ID: after} +func (a *Authorizer) redefining(custom map[Role]RoleDefinition, r RoleDefinition) redefinition { + return redefinition{was: custom[r.ID], now: a.current([]RoleDefinition{r.clone()})[0]} +} + +// more reports whether the role gives anything it didn't to whomever it is granted (givesMore), as +// the holders of a role listing it then hand on. It is false for no redefinition, as deleting a +// role is. +func (d *redefinition) more() bool { return d != nil && givesMore(d.was, d.now) } + +// widensTo reports whether a grant of the role to p gives more as it will be than the host was +// asked about as it was, as checks read what a grant gives through its principal +// (givesThrough): anything, where it gave nothing, as a grant to the tenant's members does once +// the role is no longer privileged, or one to the public once the role is Public again, however +// the role changed, narrowed too; or, where it gave something, more than it gave (widens). One that +// gives nothing as the role will be widens nothing, however the role grows, as a grant to the +// tenant's members of a privileged role: what was added to the role while it gave nothing is asked +// about once it gives it. +func (d redefinition) widensTo(p Principal) bool { + return givesThrough(d.now, p) && (!givesThrough(d.was, p) || widens(d.was, d.now)) +} + +// widenedTo is whether a redefinition widens grants to the tenant's members, to the public and to +// the principals named one by one, by the kind of the broad ones and "" for the others, each +// decided once however many grants each has. +type widenedTo map[PrincipalKind]bool + +func (d redefinition) widened() widenedTo { + return widenedTo{"": d.widensTo(Principal{}), Members: d.widensTo(Principal{Kind: Members}), Public: d.widensTo(Principal{Kind: Public})} +} + +// of reports whether the redefinition widens a grant or eligibility to p. +func (w widenedTo) of(p Principal) bool { + if p.broad() { + return w[p.Kind] } - return after, nil + return w[""] } -// givesMore reports whether grants of a role defined as now give anything they didn't as was, so -// that narrowing a role is never refused for grants it already has. An empty GrantableRoles lets -// holders grant any role they hold, and a listed role needn't be held, so only dropping roles from -// a list that stays non-empty gives no more. +// any reports whether it widens a grant to any principal. +func (w widenedTo) any() bool { return w[""] || w[Members] || w[Public] } + +// givesThrough reports whether a grant of a role defined as def to p gives anything, as checks read +// it (holding.carries): one to the tenant's members or the public gives nothing of a privileged +// role, nor one to the public of a role not marked Public. +func givesThrough(def RoleDefinition, p Principal) bool { + h := holding{Binding: Binding{Principal: p}} + return slices.ContainsFunc(def.Permissions, func(perm Permission) bool { return h.carries(def, perm) }) +} + +// givesMore reports whether grants of a role defined as now give anything they didn't as was, to +// whomever they are, so that narrowing a role is never refused for what its grants already give. +// What a grant to the tenant's members or the public gives turns on more than this +// (redefinition.widensTo): narrowing a privileged role so that it no longer is gives theirs what +// they didn't. An empty GrantableRoles lets holders grant any role they hold, and a listed role +// needn't be held, so only dropping roles from a list that stays non-empty gives no more. func givesMore(was, now RoleDefinition) bool { return !subset(now.Permissions, was.Permissions) || !subset(now.ResourceTypes, was.ResourceTypes) || now.Public && !was.Public || @@ -722,7 +774,9 @@ func redefinedSince(asked, now book, roles []Role) error { } // widens reports whether grants of a role defined as now give more than the host was asked about -// as was: what it gives, or newly privileged, as a catalog change can make it. +// as was: what it gives, or newly privileged, as a catalog change can make it. That is so of grants +// that gave something; of those to the tenant's members and the public, which can give nothing, +// redefinition.widensTo decides. func widens(was, now RoleDefinition) bool { return givesMore(was, now) || now.Privileged && !was.Privileged } @@ -748,16 +802,17 @@ func (v revalidation) key() revalidated { return revalidated{grantKey: keyOf(v.binding), scope: v.scope, id: v.binding.ID, note: v.binding.Note} } -// revalidations reads, on q, the grants and eligibilities in force of the roles widened, leaving -// out those that give nothing as the roles will be, as checks read them: they widen nothing. -func (a *Authorizer) revalidations(ctx context.Context, q DB, tenant string, widened map[Role]RoleDefinition) ([]revalidation, error) { - if len(widened) == 0 { +// revalidations reads, on q, the grants and eligibilities in force of the role d redefines that the +// redefinition widens (redefinition.widensTo), as the role will be, as checks read what they give: +// those that give nothing as it will be, or no more than they gave, widen nothing, but one that gave +// nothing and will give something does, however the role changed, as a grant to the tenant's +// members of a role narrowed so that it is no longer privileged. +func (a *Authorizer) revalidations(ctx context.Context, q DB, tenant string, d redefinition) ([]revalidation, error) { + w := d.widened() + if !w.any() { return nil, nil } - var ids []string - for id := range widened { - ids = append(ids, string(id)) - } + ids := []string{string(d.now.ID)} rows, err := q.Query(ctx, `SELECT `+grantColumns+` FROM (`+customRoleRows("$2", "rbac_grants", "*", inForce)+`) g ORDER BY id`, tenant, ids, a.cfg.Now()) if err != nil { return nil, err @@ -777,12 +832,8 @@ func (a *Authorizer) revalidations(ctx context.Context, q DB, tenant string, wid } need := make([]revalidation, 0, len(grants)+len(eligible)) add := func(scope Scope, b Binding) { - def := widened[b.Role] - gives := slices.ContainsFunc(def.Permissions, func(p Permission) bool { return b.Principal.carries(def, p) }) - // A privileged role gives nothing through the tenant's members or the public. - weak := def.Privileged && b.Principal.broad() - if gives && !weak && a.known(tenant, b.Principal) && a.grantableOn(def, scope.Type) { - need = append(need, revalidation{scope: scope, binding: b, role: def}) + if w.of(b.Principal) && a.known(tenant, b.Principal) && a.grantableOn(d.now, scope.Type) { + need = append(need, revalidation{scope: scope, binding: b, role: d.now}) } } for _, g := range grants { @@ -795,12 +846,10 @@ func (a *Authorizer) revalidations(ctx context.Context, q DB, tenant string, wid } // revalidate asks Config.ValidateGrant, before SetRole's transaction, about each grant and -// eligibility of a role that redefining r as actor widens, and returns those it asked about, with -// the role as it asked, and the standing of their holders. custom are the tenant's custom roles, -// as read. -func (a *Authorizer) revalidate(ctx context.Context, actor Subject, tenant string, custom map[Role]RoleDefinition, r RoleDefinition) (map[revalidated]RoleDefinition, standing, error) { - _, widened := a.redefining(custom, r) - need, err := a.revalidations(ctx, a.db, tenant, widened) +// eligibility of a role that redefining it as actor, d, widens, and returns those it asked about, +// with the role as it asked, and the standing of their holders. +func (a *Authorizer) revalidate(ctx context.Context, actor Subject, tenant string, d redefinition) (map[revalidated]RoleDefinition, standing, error) { + need, err := a.revalidations(ctx, a.db, tenant, d) if err != nil { return nil, standing{}, err } @@ -813,7 +862,7 @@ func (a *Authorizer) revalidate(ctx context.Context, actor Subject, tenant strin return nil, standing{}, err } roles := a.builtinBook() - roles.custom = widened + roles.custom = map[Role]RoleDefinition{d.now.ID: d.now} asked := make(map[revalidated]RoleDefinition, len(need)) var refused []error for _, v := range need { @@ -837,23 +886,25 @@ func (a *Authorizer) revalidate(ctx context.Context, actor Subject, tenant strin // roleReach is what a change to what a custom role's grants and eligibilities give, or deleting // the role, checked of the scopes it reaches before its transaction, which stillReaches checks // again under the tenant's lock: the types of scope the constraints were asked about, each scope as -// Config.ResourceOf gives it, where a deny or a label needed it, the role as it will be where its -// grants will give more, and the labels of the scopes they are on and, where they will, of those -// roles listing it are granted on (listing). +// Config.ResourceOf gives it, where a deny or a label needed it, the role as it was and as it will +// be where a change redefines it, and the labels of the scopes they are on and, where it will give +// more, of those roles listing it are granted on (listing). type roleReach struct { types []string on map[Scope]Resource - grows *RoleDefinition + redefined *redefinition labels, listing labeling } // reached are the scopes a change to a custom role reaches: those its grants and eligibilities are -// on, own, and where they will give more, those of the roles whose holders may grant it as it will -// be without holding it (handingOn), listed, each with the types of scope the grants there let them +// on, own, of which widened are those where one of them will give more (redefinition.widensTo), +// and where the role will give more, those of the roles whose holders may grant it as it will be +// without holding it (handingOn), listed, each with the types of scope the grants there let them // grant it on. type reached struct { - own []Scope - listed map[Scope][]string + own []Scope + widened map[Scope]bool + listed map[Scope][]string } func (r reached) any() bool { return len(r.own)+len(r.listed) > 0 } @@ -869,27 +920,37 @@ func (r reached) types() []string { return types } -// reachedBy reads, on q, the scopes a change to custom role id reaches, with grows the role as it -// will be where its grants will give more, and custom the tenant's custom roles, as read. -func (a *Authorizer) reachedBy(ctx context.Context, q DB, tenant string, id Role, custom map[Role]RoleDefinition, grows *RoleDefinition) (reached, error) { +// reachedBy reads, on q, the scopes a change to custom role id reaches, with d the role as it was +// and as it will be where the change redefines it, and custom the tenant's custom roles, as read. +func (a *Authorizer) reachedBy(ctx context.Context, q DB, tenant string, id Role, custom map[Role]RoleDefinition, d *redefinition) (reached, error) { ids := []Role{id} var handing map[listedAt][]string - if grows != nil { - handing = a.handingOn(custom, id, *grows) + if d.more() { + handing = a.handingOn(custom, id, d.now) listers := map[Role]bool{} for at := range handing { listers[at.role] = true } ids = append(ids, slices.Sorted(maps.Keys(listers))...) } + var w widenedTo + if d != nil { + w = d.widened() + } scopes, err := a.rolesScopes(ctx, q, tenant, ids) if err != nil { return reached{}, err } - r := reached{listed: map[Scope][]string{}} + r := reached{widened: map[Scope]bool{}, listed: map[Scope][]string{}} for _, s := range scopes { if s.role == id { - r.own = append(r.own, s.Scope) + // A scope's rows are read in order, one for each kind of principal they are to. + if n := len(r.own); n == 0 || r.own[n-1] != s.Scope { + r.own = append(r.own, s.Scope) + } + if w.of(Principal{Kind: s.broad}) && a.grantableOn(d.now, s.Type) { + r.widened[s.Scope] = true + } continue } for _, t := range handing[listedAt{s.role, s.Type}] { @@ -968,22 +1029,24 @@ func (a *Authorizer) handingOn(custom map[Role]RoleDefinition, id Role, def Role // tenant, removes its power to change grants on scopes of a type they can be on, as the role is for // types, or are on, as narrowing it leaves them on scopes of types it is no longer for; on a scope // they are on, as Config.ResourceOf gives it, where a deny on its path removes the actor's power to -// change grants there or, with grows, the role as it will be when they will give more, a permission -// it gives, or where its label refuses the actor (labelsHeld). With grows, it refuses too where a -// deny on the path of a scope a role whose holders may grant it without holding it is granted on +// change grants there or, where d redefines the role so that one of them gives more there +// (redefinition.widensTo), a permission it gives as it will be, or where its label refuses the +// actor (labelsHeld). Where the role will give more to whomever it is granted, it refuses too where +// a deny on the path of a scope a role whose holders may grant it without holding it is granted on // (handingOn) keeps the actor from granting it there as it will be, as they then may // (scopesDenied), or where that scope's label refuses the actor. Only a deny on a scope other than // the tenant can remove more there than the tenant's, which mayDefine and changesDenied check, so // only where one binds the actor, or a label could refuse it, as none can where the actor's label -// values satisfy every label, is the host asked about the scopes. Without grows, as for deleting -// or narrowing the role, which only takes access away, a scope the host no longer has (ErrNotFound) -// is read as stored, with no containers or label. au is the actor's authority on the tenant, which -// mayDefine checked the role on. -func (a *Authorizer) roleReachOf(ctx context.Context, actor Subject, au authority, tenant string, id Role, types []string, grows *RoleDefinition, +// values satisfy every label, is the host asked about the scopes. Where none of the grants and +// eligibilities on a scope will give more, as for deleting or narrowing the role, which there only +// takes access away, a scope the host no longer has (ErrNotFound) is read as stored, with no +// containers or label. au is the actor's authority on the tenant, which mayDefine checked the role +// on. +func (a *Authorizer) roleReachOf(ctx context.Context, actor Subject, au authority, tenant string, id Role, types []string, d *redefinition, change string, ) (roleReach, error) { - reach := roleReach{grows: grows} - scopes, err := a.reachedBy(ctx, a.db, tenant, id, au.roles.custom, grows) + reach := roleReach{redefined: d} + scopes, err := a.reachedBy(ctx, a.db, tenant, id, au.roles.custom, d) if err != nil { return roleReach{}, err } @@ -1009,7 +1072,7 @@ func (a *Authorizer) roleReachOf(ctx context.Context, actor Subject, au authorit // scope can refuse the actor handing on, as granting it there would; so it is asked about the // scopes only where the actor's values don't satisfy every label, which the constraints were // asked about for its own. - if grows != nil && reach.labels.cs != nil { + if d.more() && reach.labels.cs != nil { reach.listing = labeling{cs: reach.labels.cs} if len(scopes.listed) > 0 { if reach.listing.cleared, err = a.clearsAll(ctx, a.db, reach.listing.cs, nil); err != nil { @@ -1034,7 +1097,7 @@ func (a *Authorizer) roleReachOf(ctx context.Context, actor Subject, au authorit } r, err := a.resourceOf(ctx, tenant, s) switch { - case grows == nil && errors.Is(err, ErrNotFound): + case !scopes.widened[s] && scopes.listed[s] == nil && errors.Is(err, ErrNotFound): r = Resource{Tenant: tenant, Type: s.Type, ID: s.ID} case err != nil: return roleReach{}, err @@ -1078,7 +1141,7 @@ func scopeOrder(x, y Scope) int { return strings.Compare(x.Key(), y.Key()) } func (a *Authorizer) stillReaches(ctx context.Context, tx pgx.Tx, actor Subject, au authority, tenant string, id Role, types []string, checked roleReach, change string, ) error { - scopes, err := a.reachedBy(ctx, tx, tenant, id, au.roles.custom, checked.grows) + scopes, err := a.reachedBy(ctx, tx, tenant, id, au.roles.custom, checked.redefined) if err != nil { return err } @@ -1139,10 +1202,11 @@ func (a *Authorizer) deniedBelow(ctx context.Context, q DB, tenant string, actor // scopesDenied returns why a deny stops the actor changing a custom role, reading the denies on the // paths of the scopes the change reaches, each as reach resolved it, as one would stop Apply there: // on a scope its grants and eligibilities are on, one that removes the actor's power to change -// grants there, or where they will give more, one that removes there a permission the role as it -// will be gives, which the actor would hand on there; and on a scope of those listed, one that keeps -// the actor from granting the role as it will be there, as the holders of a role granted there then -// may. au is the actor's authority on the tenant, which mayDefine checked the role on. +// grants there, or where one of them will give more (reached.widened), as a grant to the tenant's +// members does once the role is no longer privileged, one that removes there a permission the role +// as it will be gives, which the actor would hand on there; and on a scope of those listed, one that +// keeps the actor from granting the role as it will be there, as the holders of a role granted there +// then may. au is the actor's authority on the tenant, which mayDefine checked the role on. func (a *Authorizer) scopesDenied(ctx context.Context, q DB, actor Subject, au authority, tenant string, scopes reached, reach roleReach, change string, ) error { @@ -1165,16 +1229,19 @@ func (a *Authorizer) scopesDenied(ctx context.Context, q DB, actor Subject, au a return &DeniedError{Permission: manage, Scope: on[i].Scope(), Reason: "removed by a deny, which stops " + change} } } - if reach.grows == nil { + if reach.redefined == nil || len(scopes.widened)+len(scopes.listed) == 0 { return nil } - def := *reach.grows + def := reach.redefined.now // Each scope that what the actor holds and the denies on its path can't tell apart from another // removes what that one does, so a role granted on thousands of resources is decided against a - // few. + // few. Where no grant or eligibility will give more, the role hands nothing more on. var distinct []int seen := map[string]bool{} for i, at := range own { + if !scopes.widened[scopes.own[i]] { + continue + } if k := a.alike(at, on[i]); !seen[k] { seen[k] = true distinct = append(distinct, i) @@ -1315,30 +1382,35 @@ func customRoleRows(ids, table, cols, where string) string { // inForce is SQL for whether a grant or eligibility is in force at $3. const inForce = "(expires_at IS NULL OR expires_at > $3)" -// roleScope is a scope a custom role's grants or eligibilities are on. +// roleScope is a scope a custom role's grants or eligibilities are on, to a broad principal of +// kind broad, the tenant's members or the public, or to others, "". type roleScope struct { role Role Scope + broad PrincipalKind } // rolesScopes reads, on q, the scopes the grants and eligibilities in force of custom roles ids -// are on, each once for each role, sorted by scope. +// are on, each once for each role and kind of broad principal they are to, or none, as what a +// grant gives turns on it, sorted by scope, role and kind. func (a *Authorizer) rolesScopes(ctx context.Context, q DB, tenant string, ids []Role) ([]roleScope, error) { names := make([]string, len(ids)) for i, id := range ids { names[i] = string(id) } - // Those ended give nothing, so changing what they give changes no access. - rows, err := q.Query(ctx, customRoleRows("$2", "rbac_grants", "role, scope_type, scope_id", inForce)+` + // Those ended give nothing, so changing what they give changes no access. An eligibility is + // never a broad principal's. + rows, err := q.Query(ctx, customRoleRows("$2", "rbac_grants", + "role, scope_type, scope_id, CASE WHEN principal_kind = ANY($4::text[]) THEN principal_kind ELSE '' END", inForce)+` UNION - `+customRoleRows("$2", "rbac_eligibilities", "role, scope_type, scope_id", inForce)+` - ORDER BY 2, 3, 1`, tenant, names, a.cfg.Now()) + `+customRoleRows("$2", "rbac_eligibilities", "role, scope_type, scope_id, ''::text", inForce)+` + ORDER BY 2, 3, 1, 4`, tenant, names, a.cfg.Now(), broadKinds) if err != nil { return nil, err } var scopes []roleScope var s roleScope - _, err = pgx.ForEachRow(rows, []any{&s.role, &s.Type, &s.ID}, func() error { + _, err = pgx.ForEachRow(rows, []any{&s.role, &s.Type, &s.ID, &s.broad}, func() error { scopes = append(scopes, s) return nil })