Support for external principals - #5119
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a configurable “principal mode” per realm to support external principals (principals not backed by Polaris metastore entities), enabling authentication purely from external IDP credentials and synthetic principal/role resolution for external authorizers (e.g., OPA).
Changes:
- Introduces
PrincipalMode(INTERNAL/EXTERNAL) and wires it into realm authentication config, readiness checks, and default auth flow. - Updates
DefaultAuthenticatorandResolverto support external principals (skip metastore lookup; synthesize principal and role entities during resolution). - Adds OPA testcontainer support plus an end-to-end Keycloak+OPA integration test, and updates related docs/config reference.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/testcontainers/opa/src/main/resources/org/apache/polaris/test/opa/Dockerfile-opa-version | Adds Renovate-managed OPA image tag source for tests. |
| tools/testcontainers/opa/src/main/java/org/apache/polaris/test/opa/OpaTestResource.java | Quarkus test resource to start OPA and inject Polaris authz config. |
| tools/testcontainers/opa/src/main/java/org/apache/polaris/test/opa/OpaContainer.java | Testcontainers wrapper for OPA plus policy upload helper. |
| tools/testcontainers/opa/build.gradle.kts | New Gradle module for the OPA testcontainer utilities. |
| site/content/in-dev/unreleased/managing-security/external-idp/idp-dev-notes.md | Documents how external principal mode changes the auth flow. |
| site/content/in-dev/unreleased/managing-security/external-idp/_index.md | Adds user-facing documentation for enabling/configuring external principals. |
| site/content/in-dev/unreleased/configuration/config-sections/smallrye-polaris_authentication.md | Updates generated config reference for principal-mode. |
| runtime/service/src/test/java/org/apache/polaris/service/config/ProductionReadinessChecksTest.java | Adds tests for external principal readiness constraints. |
| runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java | Adds unit coverage for external principal authentication behavior. |
| runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java | Adds readiness checks enforcing external principal configuration constraints. |
| runtime/service/src/main/java/org/apache/polaris/service/auth/PrincipalMode.java | New enum modeling internal vs external principal mode. |
| runtime/service/src/main/java/org/apache/polaris/service/auth/internal/broker/InternalPolarisToken.java | Makes internal token type public for cross-package detection. |
| runtime/service/src/main/java/org/apache/polaris/service/auth/external/mapping/PrincipalMapper.java | Clarifies mapping contract docs for external principals. |
| runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java | Implements external principal authentication path and role handling changes. |
| runtime/service/src/main/java/org/apache/polaris/service/auth/AuthenticationRealmConfiguration.java | Adds principalMode() realm configuration option. |
| runtime/service/src/intTest/java/org/apache/polaris/service/it/ExternalPrincipalKeycloakOpaIT.java | New E2E integration test for Keycloak-authenticated external principals with OPA authz. |
| runtime/service/build.gradle.kts | Wires in OPA auth extension and OPA testcontainer dependency. |
| runtime/defaults/src/main/resources/application.properties | Adds default polaris.authentication.principal-mode setting and per-realm example. |
| polaris-core/src/test/java/org/apache/polaris/core/persistence/ResolverTest.java | Adds coverage for synthetic resolution of external principals/roles. |
| polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java | Synthesizes caller principal and principal roles for external principals. |
| gradle/projects.main.properties | Registers the new polaris-opa-testcontainer module. |
| bom/build.gradle.kts | Exposes the new OPA testcontainer module in the BOM. |
Comments suppressed due to low confidence (1)
runtime/service/src/main/java/org/apache/polaris/service/auth/external/mapping/PrincipalMapper.java:49
- Javadoc return description for mapPrincipalId() still refers to returning a "Polaris principal" even though the method returns an OptionalLong principal ID. This is confusing for implementers and readers.
*
* @param identity the {@link SecurityIdentity} of the user
* @return the Polaris principal, or an empty optional if no mapping is available
*/
OptionalLong mapPrincipalId(SecurityIdentity identity);
a37ae41 to
aa24ef2
Compare
flyrain
left a comment
There was a problem hiding this comment.
Can we add a changelog for this PR?
2b22b08 to
a1f3d8c
Compare
86141a5 to
8517735
Compare
| private static final String POLICY_NAME = "polaris/authz"; | ||
| private static final String POLICY_PACKAGE = POLICY_NAME.replace('/', '.'); | ||
|
|
||
| private GenericContainer<?> opa; |
There was a problem hiding this comment.
Looks like this refactor is not related to the external principal effort. We may not conflate it with the principal change, but correct me if I'm wrong.
There was a problem hiding this comment.
The problem is that this PR introduces a new OpaContainer, a required container for external principal testing.
As a result, the code in this class is now largely duplicating what OpaContainer does, hence the refactor. If you prefer, I can revert the changes in this class, and fix the duplication in a follow-up PR.
There was a problem hiding this comment.
Would it be simpler to move ExternalPrincipalKeycloakOpaIT into the OPA module’s integration tests? The OPA module already owns the OPA extension, its container lifecycle, and the external-server test setup. It would only need Keycloak as a test-scoped dependency.
This would keep runtime/service independent of a concrete authorizer, remove the new production runtimeOnly dependency on polaris-extensions-auth-opa, and avoid introducing the shared OpaContainer infrastructure in this PR. The reusable container refactor could still be done separately if it is useful beyond this test.
There was a problem hiding this comment.
What you suggest was actually my first design, but then I moved to the current design. Let me explain why:
The system under test here is the external-principals feature itself: it lives in polaris-core (Resolver) and runtime/service (DefaultAuthenticator, ExternalPolarisCredential). OPA and Keycloak are just a concrete authorizer + IDP used to exercise it end-to-end.
Moving the test into the OPA module inverts that relationship: OPA becomes the SUT and Polaris the fixture. It would also force Keycloak (as a test dependency) and a Keycloak-specific PolarisServerStartupAction into the OPA extension module, which is unrelated coupling
The natural neighbor for this new test is really RestCatalogKeycloakFileIT, which already tests Keycloak/OIDC external auth in runtime/service.
That said, I agree with the underlying concern about the runtimeOnly OPA dependency (your other comment); see my reply there.
There was a problem hiding this comment.
We could have a single MockAuthorizer for that, more details are in #5119 (comment).
8517735 to
e9f3eba
Compare
flyingImer
left a comment
There was a problem hiding this comment.
I think two issues need to be fixed before merge: an external principal can hit the stored-principal self-rotation shortcut, and existing custom brokers are silently reclassified as external. The missing-name path should also return a normal authentication failure. I'm fine with the synthetic Resolver entities as a temporary bridge once those are addressed.
| runtimeOnly(project(":polaris-persistence-nosql-maintenance-impl")) | ||
| runtimeOnly(project(":polaris-persistence-nosql-metastore-maintenance")) | ||
|
|
||
| runtimeOnly(project(":polaris-extensions-auth-opa")) |
There was a problem hiding this comment.
This makes every downstream consumer of runtime-service pull in the beta OPA extension.
There was a problem hiding this comment.
Note that the production distribution already declares OPA correctly and independently in runtime/server, so the line here exists purely to satisfy the @QuarkusIntegrationTest requirements, which packages the app from runtime-service's own runtimeClasspath. IOW, intTestRuntimeOnly wouldn't work here since @QuarkusIntegrationTest builds from the main runtime classpath.
But I agree that runtime-service is also consumed as a library by many modules: semantic-models, metrics-reports, openlineage, events-kafka, etc. All of them would now pull in the OPA extension.
Is that a problem? I'm not sure. Of course, OPA is of no use in a module like events-kafka or metrics-report. But that is imho a symptom of 2 pathological situations that predate this change:
-
runtime-serviceis becoming a monolith grouping together unrelated functionality. Today,events-kafkainherits stuff about metrics, andmetrics-reportinherits stuff about events. Introducing OPA to the mix doesn't fundamentally change that. -
runtime/serviceis playing two conflicting roles at once: it's a reusable library (consumed by ~10 modules), and a standalone Quarkus app that it builds only to run its own@QuarkusIntegrationTests. In fact, having a Quarkus app module consumed as a library is in itself a code smell. the OPA leak is a symptom of end-to-end tests being co-built inside theruntime-servicelibrary instead of run against the assembledruntime-servermodule.
With all that said, I see 3 ways forward:
- Accept the new dependency as a known tech debt, and engage in a refactor of
runtime-serviceto "split the monolith" in order to avoid having unrelated functionality imported by dependent modules. - Accept the new dependency as a known tech debt, and engage in a refactor to move integration tests from
runtime-servicetoruntime-server. - Reject the new dependency. In this case, the only solution is to move
ExternalPrincipalKeycloakOpaITtogether with its OPAruntimeOnlydependency and the OPA/Keycloak testcontainer deps into a dedicated leaf test module that nothing else depends on, so the extension no longer leaks ontoruntime-service's runtime classpath.
There was a problem hiding this comment.
IMO, the separate test module proposal makes sense to me. It would keep OPA in the test application without adding it to every runtime-service consumer. I'm fine with that move being a follow-up.
There was a problem hiding this comment.
Thanks for laying out the options, @adutra! I prefer option 3 over adding OPA to runtime-service’s production dependencies.
Could we also consider a test-only authorizer? As you noted, this test exercises external principals, with OPA acting as a fixture. A simple test authorizer could check the principal name and roles, exercise the resolver, and cover both allow and deny cases.
That would let us keep the test in runtime/service, though we would likely need to switch it to @QuarkusTest so the test authorizer is available. If testing the packaged application is required, I’m happy with the separate test module.
WDYT?
There was a problem hiding this comment.
I considered your suggestion initially. You're right that with a test authorizer, Polaris is unambiguously the system under test and OPA is no longer needed as a fixture at all.
Two things made me avoid this route though:
- No precedent for
@QuarkusTest+ a container in this repo. Every Keycloak test today is a@QuarkusIntegrationTestlaunching Keycloak viaKeycloakTestResource. The container-based tests that do live insrc/test(e.g.KafkaEventListenerTest) use the raw JUnit@Testcontainersextension, not@QuarkusTest. This setup is technically supported though (KeycloakTestResourceis aQuarkusTestResourceLifecycleManager, which works in both modes), but we'd be introducing a new pattern rather than following an established one. - Asymmetry with
RestCatalogKeycloakFileIT. That test is a close sibling: same base class, same Keycloak setup, also external OIDC; but it uses the default internal authorizer with real metastore principals, so it doesn't suffer from the OPA dependency problem. If we moveExternalPrincipalKeycloakOpaITto@QuarkusTest, we end up with two nearly-identical Keycloak tests split acrosstest/andintTest/, and the reason (one needs a test-only authorizer, the other doesn't) isn't obvious from the structure. MovingRestCatalogKeycloakFileITtoo would be extra churn for no clear benefit.
How about mixing both approaches then? Here is what I can suggest:
- Let's create the test module and move both Keycloak integration tests there; the new module could serve for more resource-intensive integration tests in the future, and it keeps the Keycloak tests consistent.
- Let's also introduce a test-only authorizer, and mirror the two integration tests as
@QuarkusTests in thesrc/testforlder ofruntime-service. These tests could be lightweight versions of the current Keycloak tests: for instance, they could not only use a test-only authorizer, but also a test-only OIDC provider (instead of Keycloak) to avoid the container overhead.
This way, we get the best of both worlds: we have the consistent @QuarkusIntegrationTest coverage for the real packaged server, and we also have lightweight @QuarkusTest coverage for faster feedback during development.
WDYT?
There was a problem hiding this comment.
FYI I went ahead and implemented my own suggestion above, but I can revert it if you prefer something else.
There was a problem hiding this comment.
And as a side note: I still think we need to have this conversation about splitting runtime-service into smaller modules; the fact that more and more modules depend on it will only make it harder and harder to add or update dependencies in runtime-service without breaking something downstream.
| static PolarisCredential of( | ||
| @Nullable Long principalId, @Nullable String principalName, Set<String> principalRoles) { | ||
| return ImmutablePolarisCredential.builder() | ||
| .principalId(principalId) | ||
| .principalName(principalName) | ||
| .principalRoles(principalRoles) | ||
| .build(); | ||
| } | ||
|
|
||
| /** The principal id, or null if unknown. Used for principal lookups by id. */ | ||
| @Nullable Long getPrincipalId(); |
There was a problem hiding this comment.
We just removed two public method in an interface. It potentially break the downstream.
How about add a default method like this:
default boolean isExternal() {
return false;
}
This way, we preserve the existing PolarisCredential API, and existing logic for internal credential doesn't need to change, and there is no need to introduce the new interface InternalPolarisCredential. We just need to have external OIDC credentials return true for that default method. WDYT?
There was a problem hiding this comment.
Good point on binary compatibility, but that's not a concern for us here: Polaris allows breaking binary compatibility, so preserving the exact PolarisCredential surface isn't a goal in itself.
On the design: I did consider a default boolean isExternal() flag (that was also suggested in the original RFC), but I think the sub-interfaces model this better. The key is getPrincipalId(): it's only meaningful for internally-managed principals. InternalPolarisCredential scopes it correctly, and since we already need that sub-interface for the id, making ExternalPolarisCredential its sibling (as suggested by @flyingImer) keeps the discrimination symmetric and type-driven.
So I'd prefer to keep the sub-interfaces; happy to revisit if you feel strongly about it.
There was a problem hiding this comment.
Since getPrincipalId() already allows null, could we keep the existing credential API and add isExternal(), defaulting to false? That seems simpler while still making the metastore-backed versus external distinction explicit. I think the additional type hierarchy doesn't give us enough benefit here.
There was a problem hiding this comment.
Fair enough, let's remove the type hierarchy. I'm still slightly bothered by the exposure of getPrincipalId() in the parent interface, since a principal ID simply doesn't make any sense for an external principal. (It also doesn't make sense for a custom token broker that wouldn't use/need principal IDs.)
I'm also slightly worried that if more principal types are added in the future (e.g. the proposed "FEDERATED" type), the we would need another boolean flag.
But I could live with that :-)
There was a problem hiding this comment.
Thanks for the fix, @adutra! For a potential FEDERATED principal in the future, could we keep the federation-specific information on PrincipalEntity without introducing another property on PolarisCredential? That would keep the credential model independent of the different kinds of persisted principals.
There was a problem hiding this comment.
Given that, should we call it CredentialMode instead of PrincipalMode? This will avoid any future confusing when FEDERATED principal will be introduced. WDYT?
There was a problem hiding this comment.
That's a nice suggestion! Adopted.
a4f9744 to
7042109
Compare
|
FYI I had to rebase after conflicts with #5140. This should now be ready for another review round. |
flyrain
left a comment
There was a problem hiding this comment.
Thanks @adutra for the continuous work on the PR! Great job! LGTM overall. Just want to get your input on this question: #5119 (comment).
This change introduces support for fully external principals, that is, principals that are not backed by an entity in Polaris metastore.
The new "principal mode" is configurable on a per-realm basis, and by default, internal mode is used (no changes).
When a realm is configured to use external principals instead:
internal(i.e., an external IDP must be used).internal(i.e., an external PDP must be used).Note: in theory, an external authorizer shouldn't attempt to resolve principal roles, but for now it can happen, until the Authorizer SPI is fully refactored to allow more fine-grained resolutions.
See design doc:
https://docs.google.com/document/d/1VSoN1-QsAJGaM40oWTxeLIoYTp-XlfQM9nG-jqfb_-E/edit
Prior work:
#3250
GitHub issue:
#441
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)