Repository navigation
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
Conversation
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SetRoleskipped 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.ValidateGrantwas asked nothing (ViaRedefinition), which brokeSetRole's contract that a redefinition making grants give more asks the host about each one. For example: the catalog markscluster:loginprivileged, socustom.x = [cluster:login], granted tomembers: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.SetRolenow 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, asTestWideningAPrivilegedRoleAsksNothingOfGrantsThatGiveNothingpins. 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 madePublicagain, and grants on types the role is newly for.<type>:grantthere. So a deny on the workspace holding the members' grant, which keeps the admin from handingcluster:viewon there, did not stop a narrowing that gave every membercluster:viewthere.Applygranting the same role to the members there would be refused.rolesScopesreads which kind of principal each scope's grants are to, andreachedBymarks the scopes where a grant will give more (reached.widened). On those scopes,scopesDeniedchecks 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.SetRoledoc comment, the README's custom roles and privileged roles sections, and the architecture doc described widening by definition alone.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 removescluster:viewfrom the actor, refuses the narrowing on that workspace, as it refusesApplythere. 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 withErrNotFound.I also checked that each part is needed. With the new revalidation but the old definition-level reach, the deny test and the
ErrNotFoundtest fail. When the recheck under the lock doesn't carry the redefinition, the test of a deny set meanwhile fails.make tidy-check lint vulncheckpasses, and so doesGODEBUG=fips140=only go test -race -shuffle=on -cover -timeout 60m ./...against PostgreSQL 18.Checklist
make checkpasses