Skip to content

fix: narrowing a privileged custom role gives its grants to the tenant's members access the host was never asked about - #85

Open
Michael McQuade (giraffesyo) wants to merge 1 commit into
canaryfrom
fix/privileged-narrowing
Open

Michael McQuade (giraffesyo) wants to merge 1 commit into
canaryfrom
fix/privileged-narrowing

Conversation

@giraffesyo

Copy link
Copy Markdown
Member

Summary

  • Narrowing a privileged custom role gave its grants to the tenant's members access the host was never asked about.
    • The problem: a grant to the tenant's members of a privileged role gives nothing, so SetRole skipped it as "weak" when the role was widened. Whether a redefinition widens was decided from the two definitions alone (givesMore/widens), and going from privileged to unprivileged didn't count. So narrowing the role until it was no longer privileged made that grant give every member what the role gives, including permissions added while it gave them nothing. Config.ValidateGrant was asked nothing (ViaRedefinition), which broke SetRole's contract that a redefinition making grants give more asks the host about each one. For example: the catalog marks cluster:login privileged, so custom.x = [cluster:login], granted to members:t1, gives nothing. An admin widens it to [cluster:login, cluster:view] and then narrows it to [cluster:view]. Every member now views every cluster, and the host was asked 0 times.
    • The fix: SetRole now compares what each grant and eligibility gives through its principal, as checks read it, before and after the redefinition (redefinition.widensTo). A grant that gave nothing and will give something is widened however the role changed, narrowed too; a grant that gave something is widened when the role gives more, as before. The host is asked about each widened grant as the role will be, once per grant, under the same conflict and standing checks as before. A grant that still gives nothing is still not asked about, so widening a privileged role asks nothing about the members' grants of it, as TestWideningAPrivilegedRoleAsksNothingOfGrantsThatGiveNothing pins. The comparison is decided once each for named principals, the tenant's members and the public, so it costs the same however many grants the role has. The same rule covers grants to the public of a role made Public again, and grants on types the role is newly for.
  • Denies on the scopes of those grants didn't stop the narrowing.
    • The problem: a deny on the path of a scope a role's grants are on refused a redefinition for a permission the role would give only when the definition itself grew. A narrowing checked only the deny on <type>:grant there. So a deny on the workspace holding the members' grant, which keeps the admin from handing cluster:view on there, did not stop a narrowing that gave every member cluster:view there. Apply granting the same role to the members there would be refused.
    • The fix: rolesScopes reads which kind of principal each scope's grants are to, and reachedBy marks the scopes where a grant will give more (reached.widened). On those scopes, scopesDenied checks the role as it will be under the denies on their paths, before the transaction and again under the tenant's lock. On the other scopes it checks only the actor's power to change grants. So a deny where only a grant to an identity sits, which a narrowing gives nothing more, still doesn't refuse it. A scope the host no longer has (ErrNotFound) is read as stored only where no grant on it gives more, so reviving a members' grant on one fails with the host's error.
  • The docs said narrowing a role asks about no grant.
    • The problem: the SetRole doc comment, the README's custom roles and privileged roles sections, and the architecture doc described widening by definition alone.
    • The fix: they now say a grant widens by what it gives through its principal, and that narrowing a role so that it is no longer privileged widens its grants to the tenant's members. The README also says that a catalog change doing the same is the host's own change and asks nothing.

Testing

New tests, each of which fails on canary:

  • TestNarrowingARoleSoItIsNoLongerPrivilegedAsksAboutTheMembersGrant (membership_test.go): follows the reported scenario through a real catalog change. Widening while the role is privileged asks nothing. Narrowing it to unprivileged asks the host about the members' grant, ViaRedefinition, as the role will be. A host that refuses it refuses the narrowing, and the members get nothing. A host that allows it lets the narrowing through.
  • TestADenyWhereTheMembersHoldARoleNoLongerPrivilegedStopsNarrowingIt (rbac_test.go): a deny on the workspace holding the members' grant, which removes cluster:view from the actor, refuses the narrowing on that workspace, as it refuses Apply there. Moved to a cluster where only a grant to an identity sits, the deny lets the narrowing through. That second half fails if every scope of the role is checked as widened.
  • TestADenySetWhileANarrowingTheMembersGainByWaitsStopsIt (rbac_test.go): a deny set on the workspace holding the members' grant while the host is asked about that grant stops the narrowing under the tenant's lock.
  • TestARemovalReadsAScopeTheHostNoLongerHasAsUnlabeled (authority_test.go), extended: narrowing a privileged role whose members' grant sits on a scope the host no longer has fails with ErrNotFound.

I also checked that each part is needed. With the new revalidation but the old definition-level reach, the deny test and the ErrNotFound test fail. When the recheck under the lock doesn't carry the redefinition, the test of a deny set meanwhile fails.

make tidy-check lint vulncheck passes, and so does GODEBUG=fips140=only go test -race -shuffle=on -cover -timeout 60m ./... against PostgreSQL 18.

Checklist

  • make check passes
  • No exported name, stored value or table was renamed (they are contract once released)

…t'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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant