Switch OPA and Ranger authorizers to resolveSelections - #5445
Conversation
ayushtkn
left a comment
There was a problem hiding this comment.
Thanx @zhang-arvin for the changes.
On scope: the title and Fixes #5439 read as though the synthetic-principal hack is being removed from the Resolver, but this PR only changes the two authorizers — and the hack isn't in main yet, since #5119 is still open. To be fair to the PR, #5439's body defines the fix as exactly this ("until external authorizers change their implementation of resolveAuthorizationInputs() to avoid calling resolveAll()"), so the mismatch is really between that issue's title and its body.
I'd still retitle to something like "Switch OPA and Ranger authorizers to resolveSelections", so it's clear what this delivers, and let #5439 close when #5119 lands without the workaround.
| Set.of( | ||
| Resolvable.REFERENCE_CATALOG, | ||
| Resolvable.REQUESTED_PATHS, | ||
| Resolvable.REQUESTED_TOP_LEVEL_ENTITIES)); |
There was a problem hiding this comment.
This selection set is not equivalent to resolveAll() when the manifest has no reference catalog.
ResolvePlan.fromSelections() throws IllegalArgumentException if REFERENCE_CATALOG or REQUESTED_PATHS is selected and the catalog name is null. resolveAll() already skips those in that case.
PolarisAdminService uses newResolutionManifest(null) for root ops, principal/principal-role ops, and root grants. Those paths will break for OPA.
Always resolve REQUESTED_TOP_LEVEL_ENTITIES; add REFERENCE_CATALOG and REQUESTED_PATHS only when a reference catalog is present.
Also update OpaPolarisAuthorizerTest.resolveAuthorizationInputsResolvesAll(), which still verifies resolveAll().
| Set.of( | ||
| Resolvable.REFERENCE_CATALOG, | ||
| Resolvable.REQUESTED_PATHS, | ||
| Resolvable.REQUESTED_TOP_LEVEL_ENTITIES)); | ||
| } |
There was a problem hiding this comment.
Same issue as OpaPolarisAuthorizer: this unconditional set will throw on admin calls with a null catalog. Please apply the same catalog-conditional selections here.
Would also be good to add Ranger unit tests for resolveAuthorizationInputs() covering both catalog-scoped and null-catalog manifests. testAuthzRoot only exercises authorizeOrThrow, so it would not catch this.
One caveat: a test that mocks PolarisResolutionManifest (like OpaPolarisAuthorizerTest.resolveAuthorizationInputsResolvesAll()) never reaches the real ResolvePlan.fromSelections(), so the IllegalArgumentException can't surface. To actually pin this, either capture the argument and assert the selection set excludes REFERENCE_CATALOG/REQUESTED_PATHS when there's no catalog, or use a real manifest constructed with a null catalog name.
Switch external authorizers (OPA, Ranger) from resolveAll() to resolveSelections() with REFERENCE_CATALOG, REQUESTED_PATHS, and REQUESTED_TOP_LEVEL_ENTITIES. This avoids resolving the caller principal and principal roles, which external authorizers do not need and which may not exist in the metastore for external principals. This change removes the need for synthetic principal entities in the Resolver when external principals are used, as the external authorizers no longer trigger resolution of the caller principal.
bf42e68 to
bd29a50
Compare
|
Updated per review feedback:
Let me know if anything else is needed. Thanks! |
flyingImer
left a comment
There was a problem hiding this comment.
No blocking code concerns from my side. I agree with keeping #5439 open until the synthetic-principal workaround is actually removed, so this should use Related to rather than Fixes.
Fixes #5439
Summary
Switch external authorizers (OPA, Ranger) from
resolveAll()toresolveSelections()withREFERENCE_CATALOG,REQUESTED_PATHS, andREQUESTED_TOP_LEVEL_ENTITIES.This avoids resolving the caller principal and principal roles, which external authorizers do not need and which may not exist in the metastore for external principals.
Background
PR #5119 introduces external principals and a temporary hack in the Resolver: synthetic principals and principal roles. This hack is needed because the current external authorizers call
resolveAll(), which tries to resolve the caller principal from the metastore — but external principals do not exist in the metastore.Changes
OpaPolarisAuthorizer.resolveAuthorizationInputs(): Changed fromresolveAll()toresolveSelections()withREFERENCE_CATALOG,REQUESTED_PATHS,REQUESTED_TOP_LEVEL_ENTITIES.RangerPolarisAuthorizer.resolveAuthorizationInputs(): Same change.Why this works
External authorizers (OPA, Ranger) do not use resolved caller principal/principal roles in their authorization logic. They derive actor information from the
AuthorizationRequest.principal()directly. By skippingCALLER_PRINCIPALandCALLER_PRINCIPAL_ROLESresolution, we avoid the need for synthetic principal entities in the Resolver.Checklist