You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2026/07/21 13:29:24 UTC
[PR] Support for external principals [polaris]
adutra opened a new pull request, #5119:
URL: https://github.com/apache/polaris/pull/5119
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:
- The authentication type for the realm must NOT be `internal` (i.e., an external IDP must be used).
- The authorizer type must NOT be `internal` (i.e., an external PDP must be used).
- The authenticator bypasses the metastore lookup when authenticating the principal.
- The authenticator sets the principal roles directly from the credential's claims, as-is (no prefix stripping, no "ALL" pseudo-role expansion).
- The resolver creates synthetic principal and principal role entities if asked by the authorizer to resolve the principal and/or principal roles.
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:
https://github.com/apache/polaris/pull/3250
GitHub issue:
https://github.com/apache/polaris/issues/441
<!--
π Describe what changes you're proposing, especially breaking or user-facing changes.
π See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
## Checklist
- [ ] π‘οΈ Don't disclose security issues! (contact security@apache.org)
- [ ] π Clearly explained why the changes are needed, or linked related issues: Fixes #
- [ ] π§ͺ Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] π‘ Added comments for complex logic
- [ ] π§Ύ Updated `CHANGELOG.md` (if needed)
- [ ] π Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3731778665
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -780,6 +787,16 @@ private ResolverStatus resolvePaths(
private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
List<ResolvedPolarisEntity> toValidate, boolean resolvePrincipalRoles) {
+ // External principals are not backed by the metastore: synthesize the caller principal and its
+ // roles directly from the authenticated principal instead of resolving them from the backend.
+ boolean externalPrincipal =
+ polarisPrincipal
+ .getAttribute(PolarisPrincipal.PRINCIPAL_ENTITY_ATTRIBUTE_KEY, PrincipalEntity.class)
+ .isEmpty();
+ if (externalPrincipal) {
Review Comment:
+1 to @adutra 's point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r4073079220
##########
runtime/service/build.gradle.kts:
##########
@@ -47,6 +47,8 @@ dependencies {
runtimeOnly(project(":polaris-persistence-nosql-maintenance-impl"))
runtimeOnly(project(":polaris-persistence-nosql-metastore-maintenance"))
+ runtimeOnly(project(":polaris-extensions-auth-opa"))
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3981417793
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -809,6 +826,39 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ /**
+ * Synthesize the caller principal and its principal roles for an external principal. The
+ * principal is assumed to exist and to be valid; neither it nor its roles are read from the
+ * metastore. Synthetic entities carry negative sentinel ids (so they never collide with real,
+ * positive entity ids) and empty grant records.
+ */
+ private ResolverStatus resolveExternalCallerPrincipalAndPrincipalRoles(
+ boolean resolvePrincipalRoles) {
+ PrincipalEntity syntheticPrincipal =
Review Comment:
Awesome. Thanks for the update, @sungwy !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3907664968
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/external/OidcPolarisCredentialAugmentor.java:
##########
@@ -86,11 +92,16 @@ protected SecurityIdentity setPolarisCredential(
SecurityIdentity identity,
PrincipalMapper principalMapper,
PrincipalRolesMapper rolesMapper) {
- Long principalId =
- principalMapper.mapPrincipalId(identity).stream().boxed().findFirst().orElse(null);
String principalName = principalMapper.mapPrincipalName(identity).orElse(null);
Set<String> principalRoles = rolesMapper.mapPrincipalRoles(identity);
- PolarisCredential credential = PolarisCredential.of(principalId, principalName, principalRoles);
+ PolarisCredential credential;
+ if (authConfig.principalMode() == PrincipalMode.INTERNAL) {
+ Long principalId =
+ principalMapper.mapPrincipalId(identity).stream().boxed().findFirst().orElse(null);
+ credential = InternalPolarisCredential.of(principalId, principalName, principalRoles);
+ } else {
+ credential = PolarisCredential.of(principalName, principalRoles);
Review Comment:
External mode requires a principal name, but this branch sends an empty name mapping into a non-null credential builder. PrincipalMapper allows the name to be absent (L95 above), and Immutables rejects the resulting null before DefaultAuthenticator can raise AuthenticationFailedException.
Could we reject a missing name in the external branch instead? One option is to unwrap mapPrincipalName() there and cover the ID-only/no-name case.
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -120,73 +123,109 @@ private static PolarisCredential extractPolarisCredential(SecurityIdentity ident
/**
* Resolves the principal entity based on the provided credentials.
*
- * <p>This method attempts to load the principal entity using either the principal ID or the
- * principal name from the credentials. If neither is available, nor if the principal entity can
- * be found, it throws a {@link AuthenticationFailedException}.
+ * <p>When the credentials were issued by Polaris itself ({@link InternalPolarisToken}) or when
+ * the principal mode is {@link PrincipalMode#INTERNAL}, this method attempts to load the
+ * principal entity using either the principal ID or the principal name from the credentials. If
+ * neither is available, nor if the principal entity can be found, it throws a {@link
+ * AuthenticationFailedException}.
+ *
+ * <p>When the credentials were issued by an external IDP and the principal mode is {@link
+ * PrincipalMode#EXTERNAL}, this method does not attempt to load the principal entity from the
+ * database and returns {@code null} instead. It throws {@link AuthenticationFailedException} if
+ * the principal name is not available in the credentials.
*/
+ @Nullable
protected PrincipalEntity resolvePrincipalEntity(PolarisCredential credentials) {
- PrincipalEntity principal = null;
- try {
- // If the principal id is present, prefer to use it to load the principal entity,
- // otherwise, use the principal name to load the entity.
- if (credentials.getPrincipalId() != null && credentials.getPrincipalId() > 0) {
- principal =
- metaStoreManager
- .findPrincipalById(
- callContext.getPolarisCallContext(), credentials.getPrincipalId())
- .orElse(null);
- } else if (credentials.getPrincipalName() != null) {
- principal =
- metaStoreManager
- .findPrincipalByName(
- callContext.getPolarisCallContext(), credentials.getPrincipalName())
- .orElse(null);
+ PrincipalEntity entity = null;
+
+ boolean shouldHaveBackingEntity =
+ credentials instanceof InternalPolarisToken
Review Comment:
The new InternalPolarisCredential check still changes how existing custom TokenBroker implementations behave even though TokenBroker.verify() still returns PolarisCredential:
IIUC InternalAuthenticationMechanism forwards that returned credential unchanged. If a broker returns its existing PolarisCredential implementation, DefaultAuthenticator now treats it as external, skips the metastore principal lookup, and uses the credential roles as-is. If so, could we keep those existing credentials on the internal path without requiring every custom broker to return a new type? One option would be to represent external credentials with ExternalPolarisCredential instead, plus a regression test using a custom TokenBroker that returns PolarisCredential.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3852150629
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -809,6 +826,39 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ /**
+ * Synthesize the caller principal and its principal roles for an external principal. The
+ * principal is assumed to exist and to be valid; neither it nor its roles are read from the
+ * metastore. Synthetic entities carry negative sentinel ids (so they never collide with real,
+ * positive entity ids) and empty grant records.
+ */
+ private ResolverStatus resolveExternalCallerPrincipalAndPrincipalRoles(
+ boolean resolvePrincipalRoles) {
+ PrincipalEntity syntheticPrincipal =
Review Comment:
The synthetic principal is indeed not strictly needed, since it's internal to the Resolver. Note that without the synthetic principal, calling `Resolver.getResolvedCallerPrincipal()` would throw β but that method is not called outside the Resolver currently.
The synthetic roles, however, are needed, because `Resolver.getResolvedCallerPrincipalRoles()` _is_ called by `PolarisResolutionManifest`. Also, authorization code typically does this kind of authz check:
```java
authorizer.authorizeOrThrow(
polarisPrincipal, // <- from the authenticator
resolutionManifest.getAllActivatedPrincipalRoleEntities(), // <- synthetic roles from the resolver
op,
rootContainerWrapper,
null /* secondary */);
```
I've always advocated for _not_ re-resolving principals and roles in the Resolver; but as long as the authz layer needs those, I think we should keep using synthetic principals and roles. This PR does not aim at changing `PolarisResolutionManifest` or the authz layer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra merged PR #5119:
URL: https://github.com/apache/polaris/pull/5119
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3855720807
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -120,73 +123,109 @@ private static PolarisCredential extractPolarisCredential(SecurityIdentity ident
/**
* Resolves the principal entity based on the provided credentials.
*
- * <p>This method attempts to load the principal entity using either the principal ID or the
- * principal name from the credentials. If neither is available, nor if the principal entity can
- * be found, it throws a {@link AuthenticationFailedException}.
+ * <p>When the credentials were issued by Polaris itself ({@link InternalPolarisToken}) or when
+ * the principal mode is {@link PrincipalMode#INTERNAL}, this method attempts to load the
+ * principal entity using either the principal ID or the principal name from the credentials. If
+ * neither is available, nor if the principal entity can be found, it throws a {@link
+ * AuthenticationFailedException}.
+ *
+ * <p>When the credentials were issued by an external IDP and the principal mode is {@link
+ * PrincipalMode#EXTERNAL}, this method does not attempt to load the principal entity from the
+ * database and returns {@code null} instead. It throws {@link AuthenticationFailedException} if
+ * the principal name is not available in the credentials.
*/
+ @Nullable
protected PrincipalEntity resolvePrincipalEntity(PolarisCredential credentials) {
- PrincipalEntity principal = null;
- try {
- // If the principal id is present, prefer to use it to load the principal entity,
- // otherwise, use the principal name to load the entity.
- if (credentials.getPrincipalId() != null && credentials.getPrincipalId() > 0) {
- principal =
- metaStoreManager
- .findPrincipalById(
- callContext.getPolarisCallContext(), credentials.getPrincipalId())
- .orElse(null);
- } else if (credentials.getPrincipalName() != null) {
- principal =
- metaStoreManager
- .findPrincipalByName(
- callContext.getPolarisCallContext(), credentials.getPrincipalName())
- .orElse(null);
+ PrincipalEntity entity = null;
+
+ boolean shouldHaveBackingEntity =
+ credentials instanceof InternalPolarisToken
Review Comment:
The built-in JWT brokers are safe with this impl. because they return `InternalPolarisToken`, but configurable custom brokers are silently misclassified. The RFC proposed `PolarisCrendential.isExternal()`[1] to keep the credential provenance. I believe it is a more reliable way. WDYT? cc @sungwy
1. https://docs.google.com/document/d/1VSoN1-QsAJGaM40oWTxeLIoYTp-XlfQM9nG-jqfb_-E/edit?disco=AAABxS6MQTo
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/external/mapping/PrincipalMapper.java:
##########
@@ -36,16 +36,26 @@
public interface PrincipalMapper {
/**
- * Maps the {@link SecurityIdentity} to a Polaris principal.
+ * Maps the {@link SecurityIdentity} to a Polaris principal ID.
+ *
+ * <p>When using internal principals backed by a {@link
+ * org.apache.polaris.core.entity.PrincipalEntity} in the metastore, either the principal ID or
+ * the principal name must be provided. If external principals are used instead, the principal ID
+ * is irrelevant.
*
* @param identity the {@link SecurityIdentity} of the user
- * @return the Polaris principal, or an empty optional if no mapping is available
+ * @return the Polaris principal ID, or an empty optional if no mapping is available
*/
OptionalLong mapPrincipalId(SecurityIdentity identity);
/**
* Maps the {@link SecurityIdentity} to a Polaris principal name.
*
+ * <p>When using internal principals backed by a {@link
+ * org.apache.polaris.core.entity.PrincipalEntity} in the metastore, either the principal ID or
+ * the principal name must be provided. If external principals are used instead, the principal
+ * name must be provided.
Review Comment:
Same here
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -809,6 +826,39 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ /**
+ * Synthesize the caller principal and its principal roles for an external principal. The
+ * principal is assumed to exist and to be valid; neither it nor its roles are read from the
+ * metastore. Synthetic entities carry negative sentinel ids (so they never collide with real,
+ * positive entity ids) and empty grant records.
+ */
+ private ResolverStatus resolveExternalCallerPrincipalAndPrincipalRoles(
+ boolean resolvePrincipalRoles) {
+ PrincipalEntity syntheticPrincipal =
Review Comment:
Could we avoid creating synthetic principals and roles by using the existing resolveAuthorizationInputs() hook?
OPA and Ranger already receive the authenticated identity through AuthorizationRequest.principal() and make decisions using the principal and role names. They do not use the synthetic IDs or grant records. They only need the requested resources to be resolved. The internal authorizer is the one that needs resolveAll() because it depends on persisted principals, roles, IDs, and grants.
Could OPA and Ranger resolve entities needed only ? This would remove the need for synthetic entities, negative IDs, and insertion into the shared resolver indexes, including the name-collision issue. I think `resolveAuthorizationInputs()` was introduced to avoid resolve all. cc @sungwy
##########
runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java:
##########
@@ -192,6 +193,48 @@ public ProductionReadinessCheck checkTokenBrokers(AuthenticationConfiguration co
return ProductionReadinessCheck.of(errors);
}
+ @Produces
+ public ProductionReadinessCheck checkExternalPrincipals(
+ AuthenticationConfiguration configuration) {
+ List<ProductionReadinessCheck.Error> errors = new ArrayList<>();
+ configuration
+ .realms()
+ .forEach(
+ (realm, config) -> {
+ if (config.principalMode() == PrincipalMode.EXTERNAL
+ && config.type() == AuthenticationType.INTERNAL) {
+ errors.add(
+ Error.ofSevere(
+ "Setting principal mode to EXTERNAL when the authentication type is INTERNAL is not allowed.",
+ "polaris.authentication.%sprincipal-mode"
+ .formatted(authRealmSegment(realm))));
+ }
+ });
+ return ProductionReadinessCheck.of(errors);
+ }
+
+ @Produces
+ public ProductionReadinessCheck checkExternalPrincipalsAuthorizer(
+ AuthenticationConfiguration authenticationConfiguration,
+ AuthorizationConfiguration authorizationConfiguration) {
+ List<ProductionReadinessCheck.Error> errors = new ArrayList<>();
+ if (authorizationConfiguration.type().equals("internal")) {
Review Comment:
Not a blocker: I think we can make authZ per realm as well, perhaps in a followup PRs.
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/external/mapping/PrincipalMapper.java:
##########
@@ -36,16 +36,26 @@
public interface PrincipalMapper {
/**
- * Maps the {@link SecurityIdentity} to a Polaris principal.
+ * Maps the {@link SecurityIdentity} to a Polaris principal ID.
+ *
+ * <p>When using internal principals backed by a {@link
+ * org.apache.polaris.core.entity.PrincipalEntity} in the metastore, either the principal ID or
+ * the principal name must be provided. If external principals are used instead, the principal ID
+ * is irrelevant.
Review Comment:
I'm not sure this comment is needed on a method only handle id.
##########
extensions/auth/opa/src/opaStartupAction/java/org/apache/polaris/extension/auth/opa/test/OpaStartupAction.java:
##########
@@ -18,87 +18,18 @@
*/
package org.apache.polaris.extension.auth.opa.test;
-import java.io.OutputStream;
-import java.net.HttpURLConnection;
-import java.net.URI;
-import java.net.URL;
-import java.nio.charset.StandardCharsets;
-import java.time.Duration;
-import org.apache.polaris.containerspec.ContainerSpecHelper;
import org.apache.polaris.server.test.runner.spi.PolarisServerStartupAction;
import org.apache.polaris.server.test.runner.spi.PolarisServerStartupContext;
-import org.testcontainers.containers.GenericContainer;
-import org.testcontainers.containers.wait.strategy.Wait;
+import org.apache.polaris.test.opa.OpaContainer;
/** Starts an OPA test server before the external Polaris server process starts. */
public class OpaStartupAction implements PolarisServerStartupAction {
- private static final int OPA_PORT = 8181;
+
private static final String POLICY_NAME = "polaris/authz";
private static final String POLICY_PACKAGE = POLICY_NAME.replace('/', '.');
- private GenericContainer<?> opa;
Review 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.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -809,6 +826,39 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ /**
+ * Synthesize the caller principal and its principal roles for an external principal. The
+ * principal is assumed to exist and to be valid; neither it nor its roles are read from the
+ * metastore. Synthetic entities carry negative sentinel ids (so they never collide with real,
+ * positive entity ids) and empty grant records.
+ */
+ private ResolverStatus resolveExternalCallerPrincipalAndPrincipalRoles(
+ boolean resolvePrincipalRoles) {
+ PrincipalEntity syntheticPrincipal =
+ new PrincipalEntity.Builder()
+ .setId(EXTERNAL_PRINCIPAL_ID)
+ .setName(polarisPrincipal.getName())
+ .build();
+ this.resolvedCallerPrincipal =
+ new ResolvedPolarisEntity(syntheticPrincipal, List.of(), List.of());
+ this.addToResolved(this.resolvedCallerPrincipal);
Review Comment:
`addToResolved` adds the temporary external caller to the general name lookup. Because the caller is processed first, it can hide a real stored principal with the same name.
For example, I created a stored principal named P1 with ID 28. When the external caller was also named P1, looking up the stored principal returned the temporary caller with ID -1 instead. Temporary external roles have the same problem.
As a result, management operations could read or modify a placeholder instead of the real stored entity. Could we keep the temporary caller and roles only in the dedicated caller fields and exclude them from the general name lookup? Please also add tests for principal-name and role-name collisions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3771306046
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -809,6 +826,39 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ /**
+ * Synthesize the caller principal and its principal roles for an external principal. The
+ * principal is assumed to exist and to be valid; neither it nor its roles are read from the
+ * metastore. Synthetic entities carry negative sentinel ids (so they never collide with real,
+ * positive entity ids) and empty grant records.
+ */
+ private ResolverStatus resolveExternalCallerPrincipalAndPrincipalRoles(
+ boolean resolvePrincipalRoles) {
+ PrincipalEntity syntheticPrincipal =
Review Comment:
I wonder: who relies on having this synthetic `PrincipalEntity`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3915356862
##########
runtime/service/build.gradle.kts:
##########
@@ -47,6 +47,8 @@ dependencies {
runtimeOnly(project(":polaris-persistence-nosql-maintenance-impl"))
runtimeOnly(project(":polaris-persistence-nosql-metastore-maintenance"))
+ runtimeOnly(project(":polaris-extensions-auth-opa"))
Review 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:
1. `runtime-service` is becoming a monolith grouping together unrelated functionality. Today, `events-kafka` inherits stuff about metrics, and `metrics-report` inherits stuff about events. Introducing OPA to the mix doesn't fundamentally change that.
2. `runtime/service` is 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 the `runtime-service` library instead of run against the assembled `runtime-server` module.
With all that said, I see 3 ways forward:
1. Accept the new dependency as a known tech debt, and engage in a refactor of `runtime-service` to "split the monolith" in order to avoid having unrelated functionality imported by dependent modules.
2. Accept the new dependency as a known tech debt, and engage in a refactor to move integration tests from `runtime-service` to `runtime-server`.
3. Reject the new dependency. In this case, the only solution is to move `ExternalPrincipalKeycloakOpaIT` together with its OPA `runtimeOnly` dependency and the OPA/Keycloak testcontainer deps into a dedicated leaf test module that nothing else depends on, so the extension no longer leaks onto `runtime-service`'s runtime classpath.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3981786940
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/PolarisCredential.java:
##########
@@ -20,29 +20,25 @@
import io.quarkus.security.credential.Credential;
import java.util.Set;
-import org.apache.polaris.immutables.PolarisImmutable;
import org.jspecify.annotations.Nullable;
/**
- * A Quarkus Security {@link Credential} exposing Polaris-specific attributes: the principal id,
- * name, and roles.
+ * A Quarkus Security {@link Credential} exposing Polaris-specific attributes.
+ *
+ * <p>Unless a credential implements {@link
+ * org.apache.polaris.service.auth.external.ExternalPolarisCredential}, it is treated as internal by
+ * the authenticator and requires a backing principal entity in the Polaris metastore.
+ *
+ * @see org.apache.polaris.service.auth.internal.InternalPolarisCredential
+ * @see org.apache.polaris.service.auth.external.ExternalPolarisCredential
*/
-@PolarisImmutable
public interface PolarisCredential extends Credential {
- 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();
Review 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.
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/external/OidcPolarisCredentialAugmentor.java:
##########
@@ -86,11 +92,18 @@ protected SecurityIdentity setPolarisCredential(
SecurityIdentity identity,
PrincipalMapper principalMapper,
PrincipalRolesMapper rolesMapper) {
- Long principalId =
- principalMapper.mapPrincipalId(identity).stream().boxed().findFirst().orElse(null);
String principalName = principalMapper.mapPrincipalName(identity).orElse(null);
Set<String> principalRoles = rolesMapper.mapPrincipalRoles(identity);
- PolarisCredential credential = PolarisCredential.of(principalId, principalName, principalRoles);
+ // Note: we build the credential even if it doesn't contain enough data to authenticate;
+ // DefaultAuthenticator will reject it later on.
+ PolarisCredential credential;
+ if (authConfig.principalMode() == PrincipalMode.INTERNAL) {
+ Long principalId =
+ principalMapper.mapPrincipalId(identity).stream().boxed().findFirst().orElse(null);
+ credential = InternalPolarisCredential.of(principalId, principalName, principalRoles);
+ } else {
+ credential = ExternalPolarisCredential.of(principalName, principalRoles);
Review Comment:
Not a blocker: The OIDC configuration docs still say either `id-claim-path` or `name-claim-path` is sufficient. In external mode, however, this branch ignores the mapped ID and `DefaultAuthenticator` rejects a missing name. An ID-only OIDC setup can therefore start successfully after enabling external mode but reject every OIDC request.
Could we update the mapping contract and configuration docs to require a name mapping in external mode (`name-claim-path` with the default mapper), and validate this at startup where possible?
Not a blocker, a followup should be good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3909949342
##########
runtime/service/build.gradle.kts:
##########
@@ -47,6 +47,8 @@ dependencies {
runtimeOnly(project(":polaris-persistence-nosql-maintenance-impl"))
runtimeOnly(project(":polaris-persistence-nosql-metastore-maintenance"))
+ runtimeOnly(project(":polaris-extensions-auth-opa"))
Review Comment:
This makes every downstream consumer of `runtime-service` pull in the beta OPA extension.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3909237793
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -809,6 +826,39 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ /**
+ * Synthesize the caller principal and its principal roles for an external principal. The
+ * principal is assumed to exist and to be valid; neither it nor its roles are read from the
+ * metastore. Synthetic entities carry negative sentinel ids (so they never collide with real,
+ * positive entity ids) and empty grant records.
+ */
+ private ResolverStatus resolveExternalCallerPrincipalAndPrincipalRoles(
+ boolean resolvePrincipalRoles) {
+ PrincipalEntity syntheticPrincipal =
+ new PrincipalEntity.Builder()
+ .setId(EXTERNAL_PRINCIPAL_ID)
+ .setName(polarisPrincipal.getName())
+ .build();
+ this.resolvedCallerPrincipal =
+ new ResolvedPolarisEntity(syntheticPrincipal, List.of(), List.of());
+ this.addToResolved(this.resolvedCallerPrincipal);
Review Comment:
Very good catch, fixed. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r4090134702
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/PolarisCredential.java:
##########
@@ -20,29 +20,25 @@
import io.quarkus.security.credential.Credential;
import java.util.Set;
-import org.apache.polaris.immutables.PolarisImmutable;
import org.jspecify.annotations.Nullable;
/**
- * A Quarkus Security {@link Credential} exposing Polaris-specific attributes: the principal id,
- * name, and roles.
+ * A Quarkus Security {@link Credential} exposing Polaris-specific attributes.
+ *
+ * <p>Unless a credential implements {@link
+ * org.apache.polaris.service.auth.external.ExternalPolarisCredential}, it is treated as internal by
+ * the authenticator and requires a backing principal entity in the Polaris metastore.
+ *
+ * @see org.apache.polaris.service.auth.internal.InternalPolarisCredential
+ * @see org.apache.polaris.service.auth.external.ExternalPolarisCredential
*/
-@PolarisImmutable
public interface PolarisCredential extends Credential {
- 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();
Review Comment:
Given that, should we call it `CredentialMode` instead of `PrincipalMode`? This will avoid any future confusing when `FEDERATED` principal will be introduced. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3771291959
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -780,6 +787,16 @@ private ResolverStatus resolvePaths(
private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
List<ResolvedPolarisEntity> toValidate, boolean resolvePrincipalRoles) {
+ // External principals are not backed by the metastore: synthesize the caller principal and its
+ // roles directly from the authenticated principal instead of resolving them from the backend.
+ boolean externalPrincipal =
+ polarisPrincipal
+ .getAttribute(PolarisPrincipal.PRINCIPAL_ENTITY_ATTRIBUTE_KEY, PrincipalEntity.class)
+ .isEmpty();
+ if (externalPrincipal) {
Review Comment:
I see a fundamental problem here. For internal principals the Authenticator has to make sure the Principal Entity exists upfront, before the request proceeds to contact the `Resolver`. At the same time the name Authorizer relies on the `Resolver` to provide a consistent view of the catalog _plus_ catalog and principal roles. What guarantees do we have that these lookups operate on the same data?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3909232365
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/external/OidcPolarisCredentialAugmentor.java:
##########
@@ -86,11 +92,16 @@ protected SecurityIdentity setPolarisCredential(
SecurityIdentity identity,
PrincipalMapper principalMapper,
PrincipalRolesMapper rolesMapper) {
- Long principalId =
- principalMapper.mapPrincipalId(identity).stream().boxed().findFirst().orElse(null);
String principalName = principalMapper.mapPrincipalName(identity).orElse(null);
Set<String> principalRoles = rolesMapper.mapPrincipalRoles(identity);
- PolarisCredential credential = PolarisCredential.of(principalId, principalName, principalRoles);
+ PolarisCredential credential;
+ if (authConfig.principalMode() == PrincipalMode.INTERNAL) {
+ Long principalId =
+ principalMapper.mapPrincipalId(identity).stream().boxed().findFirst().orElse(null);
+ credential = InternalPolarisCredential.of(principalId, principalName, principalRoles);
+ } else {
+ credential = PolarisCredential.of(principalName, principalRoles);
Review Comment:
Good catch, I changed `ExternalPolarisCredential` in a way that it accepts null names, which get caught and rejected later on by the authenticator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3915900329
##########
extensions/auth/opa/src/opaStartupAction/java/org/apache/polaris/extension/auth/opa/test/OpaStartupAction.java:
##########
@@ -18,87 +18,18 @@
*/
package org.apache.polaris.extension.auth.opa.test;
-import java.io.OutputStream;
-import java.net.HttpURLConnection;
-import java.net.URI;
-import java.net.URL;
-import java.nio.charset.StandardCharsets;
-import java.time.Duration;
-import org.apache.polaris.containerspec.ContainerSpecHelper;
import org.apache.polaris.server.test.runner.spi.PolarisServerStartupAction;
import org.apache.polaris.server.test.runner.spi.PolarisServerStartupContext;
-import org.testcontainers.containers.GenericContainer;
-import org.testcontainers.containers.wait.strategy.Wait;
+import org.apache.polaris.test.opa.OpaContainer;
/** Starts an OPA test server before the external Polaris server process starts. */
public class OpaStartupAction implements PolarisServerStartupAction {
- private static final int OPA_PORT = 8181;
+
private static final String POLICY_NAME = "polaris/authz";
private static final String POLICY_PACKAGE = POLICY_NAME.replace('/', '.');
- private GenericContainer<?> opa;
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3862797677
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -120,73 +123,109 @@ private static PolarisCredential extractPolarisCredential(SecurityIdentity ident
/**
* Resolves the principal entity based on the provided credentials.
*
- * <p>This method attempts to load the principal entity using either the principal ID or the
- * principal name from the credentials. If neither is available, nor if the principal entity can
- * be found, it throws a {@link AuthenticationFailedException}.
+ * <p>When the credentials were issued by Polaris itself ({@link InternalPolarisToken}) or when
+ * the principal mode is {@link PrincipalMode#INTERNAL}, this method attempts to load the
+ * principal entity using either the principal ID or the principal name from the credentials. If
+ * neither is available, nor if the principal entity can be found, it throws a {@link
+ * AuthenticationFailedException}.
+ *
+ * <p>When the credentials were issued by an external IDP and the principal mode is {@link
+ * PrincipalMode#EXTERNAL}, this method does not attempt to load the principal entity from the
+ * database and returns {@code null} instead. It throws {@link AuthenticationFailedException} if
+ * the principal name is not available in the credentials.
*/
+ @Nullable
protected PrincipalEntity resolvePrincipalEntity(PolarisCredential credentials) {
- PrincipalEntity principal = null;
- try {
- // If the principal id is present, prefer to use it to load the principal entity,
- // otherwise, use the principal name to load the entity.
- if (credentials.getPrincipalId() != null && credentials.getPrincipalId() > 0) {
- principal =
- metaStoreManager
- .findPrincipalById(
- callContext.getPolarisCallContext(), credentials.getPrincipalId())
- .orElse(null);
- } else if (credentials.getPrincipalName() != null) {
- principal =
- metaStoreManager
- .findPrincipalByName(
- callContext.getPolarisCallContext(), credentials.getPrincipalName())
- .orElse(null);
+ PrincipalEntity entity = null;
+
+ boolean shouldHaveBackingEntity =
+ credentials instanceof InternalPolarisToken
Review Comment:
That's a good remark as well. I agree that the way the authenticator currently "decides" whether to fetch the entity or not is a bit awkard.
However, instead of introducing `PolarisCredential.isExternal()` I would rather introduce a sub-interface: `InternalPolarisCredential`.
Any custom JWT broker would just need to produce tokens that implement `InternalPolarisCredential` for them to be recognized as internal principals. That's imho cleaner than a top level `isExternal()` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3981681456
##########
runtime/service/build.gradle.kts:
##########
@@ -47,6 +47,8 @@ dependencies {
runtimeOnly(project(":polaris-persistence-nosql-maintenance-impl"))
runtimeOnly(project(":polaris-persistence-nosql-metastore-maintenance"))
+ runtimeOnly(project(":polaris-extensions-auth-opa"))
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3909236137
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -120,73 +123,109 @@ private static PolarisCredential extractPolarisCredential(SecurityIdentity ident
/**
* Resolves the principal entity based on the provided credentials.
*
- * <p>This method attempts to load the principal entity using either the principal ID or the
- * principal name from the credentials. If neither is available, nor if the principal entity can
- * be found, it throws a {@link AuthenticationFailedException}.
+ * <p>When the credentials were issued by Polaris itself ({@link InternalPolarisToken}) or when
+ * the principal mode is {@link PrincipalMode#INTERNAL}, this method attempts to load the
+ * principal entity using either the principal ID or the principal name from the credentials. If
+ * neither is available, nor if the principal entity can be found, it throws a {@link
+ * AuthenticationFailedException}.
+ *
+ * <p>When the credentials were issued by an external IDP and the principal mode is {@link
+ * PrincipalMode#EXTERNAL}, this method does not attempt to load the principal entity from the
+ * database and returns {@code null} instead. It throws {@link AuthenticationFailedException} if
+ * the principal name is not available in the credentials.
*/
+ @Nullable
protected PrincipalEntity resolvePrincipalEntity(PolarisCredential credentials) {
- PrincipalEntity principal = null;
- try {
- // If the principal id is present, prefer to use it to load the principal entity,
- // otherwise, use the principal name to load the entity.
- if (credentials.getPrincipalId() != null && credentials.getPrincipalId() > 0) {
- principal =
- metaStoreManager
- .findPrincipalById(
- callContext.getPolarisCallContext(), credentials.getPrincipalId())
- .orElse(null);
- } else if (credentials.getPrincipalName() != null) {
- principal =
- metaStoreManager
- .findPrincipalByName(
- callContext.getPolarisCallContext(), credentials.getPrincipalName())
- .orElse(null);
+ PrincipalEntity entity = null;
+
+ boolean shouldHaveBackingEntity =
+ credentials instanceof InternalPolarisToken
Review Comment:
Very valid point. I introduced `ExternalPolarisCredential` and flipped the if-block in `DefaultAuthenticator`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3911412493
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/PolarisCredential.java:
##########
@@ -20,29 +20,25 @@
import io.quarkus.security.credential.Credential;
import java.util.Set;
-import org.apache.polaris.immutables.PolarisImmutable;
import org.jspecify.annotations.Nullable;
/**
- * A Quarkus Security {@link Credential} exposing Polaris-specific attributes: the principal id,
- * name, and roles.
+ * A Quarkus Security {@link Credential} exposing Polaris-specific attributes.
+ *
+ * <p>Unless a credential implements {@link
+ * org.apache.polaris.service.auth.external.ExternalPolarisCredential}, it is treated as internal by
+ * the authenticator and requires a backing principal entity in the Polaris metastore.
+ *
+ * @see org.apache.polaris.service.auth.internal.InternalPolarisCredential
+ * @see org.apache.polaris.service.auth.external.ExternalPolarisCredential
*/
-@PolarisImmutable
public interface PolarisCredential extends Credential {
- 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();
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3862061931
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -809,6 +826,39 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ /**
+ * Synthesize the caller principal and its principal roles for an external principal. The
+ * principal is assumed to exist and to be valid; neither it nor its roles are read from the
+ * metastore. Synthetic entities carry negative sentinel ids (so they never collide with real,
+ * positive entity ids) and empty grant records.
+ */
+ private ResolverStatus resolveExternalCallerPrincipalAndPrincipalRoles(
+ boolean resolvePrincipalRoles) {
+ PrincipalEntity syntheticPrincipal =
Review Comment:
Added this TODO:
```
* <p>TODO: synthetic principal and roles are a temporary workaround for external principals,
* until external authorizers change their implementation of {@link
* org.apache.polaris.core.auth.PolarisAuthorizer#resolveAuthorizationInputs(AuthorizationState,
* AuthorizationRequest)} to avoid calling {@link PolarisResolutionManifest#resolveAll()}, and
* instead only resolve the securables that are actually needed for authorization, cf. {@link
* PolarisResolutionManifest#resolveSelections(Set)}.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3771223945
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -780,6 +787,16 @@ private ResolverStatus resolvePaths(
private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
List<ResolvedPolarisEntity> toValidate, boolean resolvePrincipalRoles) {
+ // External principals are not backed by the metastore: synthesize the caller principal and its
+ // roles directly from the authenticated principal instead of resolving them from the backend.
+ boolean externalPrincipal =
+ polarisPrincipal
+ .getAttribute(PolarisPrincipal.PRINCIPAL_ENTITY_ATTRIBUTE_KEY, PrincipalEntity.class)
+ .isEmpty();
+ if (externalPrincipal) {
Review Comment:
I agree that conceptually an external principal is distinguished by not having a metastore-backed entity. My concern is that this code does not determine whether a Polaris entity actually exists; it infers external status from the absence of an optional attribute.
DefaultAuthenticator appears to be the only in-tree implementation and does attach PRINCIPAL_ENTITY_ATTRIBUTE_KEY in internal mode, so the built-in path is fine. However, Authenticator is a configurable extension point, and a downstream implementation may validly return a PolarisPrincipal without this attribute. Neither the Authenticator contract nor PolarisPrincipal requires it; the latter explicitly says callers must not assume it is present. Previously, such an implementation could rely on Resolver to perform the metastore lookup by name. After this change, the same principal would be classified as external even it is an internal one, causing the resolver to skip the metastore check and synthesize roles without grants.
Could we use an explicit marker(e.g., another attribute`EXTERNAL_PRINCIPAL`), set by the authenticator only when it deliberately takes the external-principal path? The resolver could then treat explicit true as external and a missing marker as internal. This would not be another user-facing configuration option; it would preserve the authentication decision for downstream resolution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #5119:
URL: https://github.com/apache/polaris/pull/5119#issuecomment-5782029259
FYI I had to rebase after conflicts with #5140. This should now be ready for another review round.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r4089991681
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/PolarisCredential.java:
##########
@@ -20,29 +20,25 @@
import io.quarkus.security.credential.Credential;
import java.util.Set;
-import org.apache.polaris.immutables.PolarisImmutable;
import org.jspecify.annotations.Nullable;
/**
- * A Quarkus Security {@link Credential} exposing Polaris-specific attributes: the principal id,
- * name, and roles.
+ * A Quarkus Security {@link Credential} exposing Polaris-specific attributes.
+ *
+ * <p>Unless a credential implements {@link
+ * org.apache.polaris.service.auth.external.ExternalPolarisCredential}, it is treated as internal by
+ * the authenticator and requires a backing principal entity in the Polaris metastore.
+ *
+ * @see org.apache.polaris.service.auth.internal.InternalPolarisCredential
+ * @see org.apache.polaris.service.auth.external.ExternalPolarisCredential
*/
-@PolarisImmutable
public interface PolarisCredential extends Credential {
- 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();
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3861957723
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -809,6 +826,39 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ /**
+ * Synthesize the caller principal and its principal roles for an external principal. The
+ * principal is assumed to exist and to be valid; neither it nor its roles are read from the
+ * metastore. Synthetic entities carry negative sentinel ids (so they never collide with real,
+ * positive entity ids) and empty grant records.
+ */
+ private ResolverStatus resolveExternalCallerPrincipalAndPrincipalRoles(
+ boolean resolvePrincipalRoles) {
+ PrincipalEntity syntheticPrincipal =
Review Comment:
Once the authz refactor is complete, yes that would be possible and we could skip creating synthetic principals altogether.
However, currently all 3 authorizers are still implementing `resolveAuthorizationInputs()` with a call to `resolutionManifest.resolveAll()`. As long as this will be the case, a synthetic principal will remain necessary.
I can put a TODO note on the `Resolver.resolveExternalCallerPrincipalAndPrincipalRoles()` method and call for its removal when the authz refactor is complete. Would that make the intent clear enough?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3806519120
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -780,6 +787,16 @@ private ResolverStatus resolvePaths(
private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
List<ResolvedPolarisEntity> toValidate, boolean resolvePrincipalRoles) {
+ // External principals are not backed by the metastore: synthesize the caller principal and its
+ // roles directly from the authenticated principal instead of resolving them from the backend.
+ boolean externalPrincipal =
+ polarisPrincipal
+ .getAttribute(PolarisPrincipal.PRINCIPAL_ENTITY_ATTRIBUTE_KEY, PrincipalEntity.class)
+ .isEmpty();
+ if (externalPrincipal) {
Review Comment:
> the same principal would be classified as external even it is an internal one, causing the resolver to skip the metastore check and synthesize roles without grants.
That is a fair concern, thanks for bringing an example. I agree to introduce the new attribute.
> What guarantees do we have that these lookups operate on the same data?
None. But that is an old problem: even today, the authenticator resolves the principal and principal roles, and the resolver re-resolves those, plus catalog roles and other securables. There is no guarantee that the resolver will see the same principal and roles as the authenticator.
IMHO, even for internal principals, the resolver _should not_ re-resolve the principal and its roles, it should instead what's in the `PolarisPrincipal`. I've voiced this concern many times. But this PR is not about fixing this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3622586949
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -90,21 +91,22 @@ public class DefaultAuthenticator implements Authenticator {
@Inject PolarisMetaStoreManager metaStoreManager;
@Inject CallContext callContext;
@Inject PolarisDiagnostics diagnostics;
+ @Inject AuthenticationRealmConfiguration authConfig;
@Override
public PolarisPrincipal authenticate(SecurityIdentity identity) {
PolarisCredential credentials = extractPolarisCredential(identity);
LOGGER.debug("Resolving principal for credentials: {}", credentials);
- PrincipalEntity principalEntity = resolvePrincipalEntity(credentials);
- PrincipalRoleSelection principalRoles = resolvePrincipalRoles(credentials, principalEntity);
- Map<String, Object> principalAttributes =
- resolvePrincipalAttributes(identity, principalEntity, principalRoles.allRolesRequested());
- PolarisPrincipal polarisPrincipal =
- PolarisPrincipal.of(principalEntity.getName(), principalAttributes, principalRoles.roles());
+ var entity = resolvePrincipalEntity(credentials);
+ var roleSelection = resolvePrincipalRoles(credentials, entity);
+ var attributes = resolvePrincipalAttributes(identity, entity, roleSelection);
- LOGGER.debug("Resolved principal: {}", polarisPrincipal);
+ var principalName = entity != null ? entity.getName() : credentials.getPrincipalName();
+ var polarisPrincipal = PolarisPrincipal.of(principalName, attributes, roleSelection.roles());
+
+ LOGGER.debug("Resolved internal principal: {}", polarisPrincipal);
return polarisPrincipal;
Review Comment:
The debug log message always says "Resolved internal principal", but this authenticator can now create external principals (no metastore-backed entity). This log line will be misleading for external principal mode.
##########
tools/testcontainers/opa/src/main/java/org/apache/polaris/test/opa/OpaContainer.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.test.opa;
+
+import java.io.OutputStream;
+import java.net.HttpURLConnection;
+import java.net.URI;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.time.Duration;
+import org.apache.polaris.containerspec.ContainerSpecHelper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testcontainers.containers.GenericContainer;
+import org.testcontainers.containers.output.Slf4jLogConsumer;
+import org.testcontainers.containers.wait.strategy.Wait;
+
+public class OpaContainer extends GenericContainer<OpaContainer> {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(OpaContainer.class);
+
+ private static final int OPA_PORT = 8181;
+
+ private URI externalUrl;
+
+ @SuppressWarnings("resource")
+ public OpaContainer() {
+ super(
+ ContainerSpecHelper.containerSpecHelper("opa", OpaContainer.class)
+ .dockerImageName(null)
+ .asCanonicalNameString());
+ withExposedPorts(OPA_PORT);
+ withCommand("run", "--server", "--addr=0.0.0.0:" + OPA_PORT);
+ waitingFor(
+ Wait.forHttp("/health")
+ .forPort(OPA_PORT)
+ .forStatusCode(200)
+ .withStartupTimeout(Duration.ofSeconds(120)));
+ withLogConsumer(new Slf4jLogConsumer(LOGGER));
+ }
+
+ @Override
+ public void start() {
+ super.start();
+ externalUrl = URI.create("http://" + getHost() + ":" + getMappedPort(OPA_PORT) + "/");
+ }
+
+ public URI getExternalUrl() {
+ return externalUrl;
+ }
+
+ public void createRegoPolicy(String name, String rego) {
+ try {
+ URL url = getExternalUrl().resolve("v1/policies/" + name).toURL();
+ HttpURLConnection conn = (HttpURLConnection) url.openConnection();
+ conn.setRequestMethod("PUT");
+ conn.setDoOutput(true);
+ conn.setRequestProperty("Content-Type", "text/plain");
+ try (OutputStream os = conn.getOutputStream()) {
+ os.write(rego.getBytes(StandardCharsets.UTF_8));
+ }
+ int code = conn.getResponseCode();
+ if (code < 200 || code >= 300) {
+ throw new IllegalStateException("OPA policy upload failed, HTTP " + code);
+ }
Review Comment:
createRegoPolicy() does not set connection timeouts and never disconnects the HttpURLConnection. In CI or local runs, a stalled OPA endpoint can hang the test indefinitely and the connection can hold resources longer than needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3915897082
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/PolarisCredential.java:
##########
@@ -20,29 +20,25 @@
import io.quarkus.security.credential.Credential;
import java.util.Set;
-import org.apache.polaris.immutables.PolarisImmutable;
import org.jspecify.annotations.Nullable;
/**
- * A Quarkus Security {@link Credential} exposing Polaris-specific attributes: the principal id,
- * name, and roles.
+ * A Quarkus Security {@link Credential} exposing Polaris-specific attributes.
+ *
+ * <p>Unless a credential implements {@link
+ * org.apache.polaris.service.auth.external.ExternalPolarisCredential}, it is treated as internal by
+ * the authenticator and requires a backing principal entity in the Polaris metastore.
+ *
+ * @see org.apache.polaris.service.auth.internal.InternalPolarisCredential
+ * @see org.apache.polaris.service.auth.external.ExternalPolarisCredential
*/
-@PolarisImmutable
public interface PolarisCredential extends Credential {
- 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();
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3676149911
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -780,6 +787,16 @@ private ResolverStatus resolvePaths(
private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
List<ResolvedPolarisEntity> toValidate, boolean resolvePrincipalRoles) {
+ // External principals are not backed by the metastore: synthesize the caller principal and its
+ // roles directly from the authenticated principal instead of resolving them from the backend.
+ boolean externalPrincipal =
+ polarisPrincipal
+ .getAttribute(PolarisPrincipal.PRINCIPAL_ENTITY_ATTRIBUTE_KEY, PrincipalEntity.class)
+ .isEmpty();
+ if (externalPrincipal) {
Review Comment:
Introducing another flag seems unnecessary to me: the existence (or not) of a principal entity is _precisely_ what distinguishes an external principal from an internal one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r4073527123
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/PolarisCredential.java:
##########
@@ -20,29 +20,25 @@
import io.quarkus.security.credential.Credential;
import java.util.Set;
-import org.apache.polaris.immutables.PolarisImmutable;
import org.jspecify.annotations.Nullable;
/**
- * A Quarkus Security {@link Credential} exposing Polaris-specific attributes: the principal id,
- * name, and roles.
+ * A Quarkus Security {@link Credential} exposing Polaris-specific attributes.
+ *
+ * <p>Unless a credential implements {@link
+ * org.apache.polaris.service.auth.external.ExternalPolarisCredential}, it is treated as internal by
+ * the authenticator and requires a backing principal entity in the Polaris metastore.
+ *
+ * @see org.apache.polaris.service.auth.internal.InternalPolarisCredential
+ * @see org.apache.polaris.service.auth.external.ExternalPolarisCredential
*/
-@PolarisImmutable
public interface PolarisCredential extends Credential {
- 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();
Review 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 :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3864684576
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -120,73 +123,109 @@ private static PolarisCredential extractPolarisCredential(SecurityIdentity ident
/**
* Resolves the principal entity based on the provided credentials.
*
- * <p>This method attempts to load the principal entity using either the principal ID or the
- * principal name from the credentials. If neither is available, nor if the principal entity can
- * be found, it throws a {@link AuthenticationFailedException}.
+ * <p>When the credentials were issued by Polaris itself ({@link InternalPolarisToken}) or when
+ * the principal mode is {@link PrincipalMode#INTERNAL}, this method attempts to load the
+ * principal entity using either the principal ID or the principal name from the credentials. If
+ * neither is available, nor if the principal entity can be found, it throws a {@link
+ * AuthenticationFailedException}.
+ *
+ * <p>When the credentials were issued by an external IDP and the principal mode is {@link
+ * PrincipalMode#EXTERNAL}, this method does not attempt to load the principal entity from the
+ * database and returns {@code null} instead. It throws {@link AuthenticationFailedException} if
+ * the principal name is not available in the credentials.
*/
+ @Nullable
protected PrincipalEntity resolvePrincipalEntity(PolarisCredential credentials) {
- PrincipalEntity principal = null;
- try {
- // If the principal id is present, prefer to use it to load the principal entity,
- // otherwise, use the principal name to load the entity.
- if (credentials.getPrincipalId() != null && credentials.getPrincipalId() > 0) {
- principal =
- metaStoreManager
- .findPrincipalById(
- callContext.getPolarisCallContext(), credentials.getPrincipalId())
- .orElse(null);
- } else if (credentials.getPrincipalName() != null) {
- principal =
- metaStoreManager
- .findPrincipalByName(
- callContext.getPolarisCallContext(), credentials.getPrincipalName())
- .orElse(null);
+ PrincipalEntity entity = null;
+
+ boolean shouldHaveBackingEntity =
+ credentials instanceof InternalPolarisToken
Review Comment:
Implemented my own suggestion of an `InternalPolarisCredential`. PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3861857356
##########
extensions/auth/opa/src/opaStartupAction/java/org/apache/polaris/extension/auth/opa/test/OpaStartupAction.java:
##########
@@ -18,87 +18,18 @@
*/
package org.apache.polaris.extension.auth.opa.test;
-import java.io.OutputStream;
-import java.net.HttpURLConnection;
-import java.net.URI;
-import java.net.URL;
-import java.nio.charset.StandardCharsets;
-import java.time.Duration;
-import org.apache.polaris.containerspec.ContainerSpecHelper;
import org.apache.polaris.server.test.runner.spi.PolarisServerStartupAction;
import org.apache.polaris.server.test.runner.spi.PolarisServerStartupContext;
-import org.testcontainers.containers.GenericContainer;
-import org.testcontainers.containers.wait.strategy.Wait;
+import org.apache.polaris.test.opa.OpaContainer;
/** Starts an OPA test server before the external Polaris server process starts. */
public class OpaStartupAction implements PolarisServerStartupAction {
- private static final int OPA_PORT = 8181;
+
private static final String POLICY_NAME = "polaris/authz";
private static final String POLICY_PACKAGE = POLICY_NAME.replace('/', '.');
- private GenericContainer<?> opa;
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3979876635
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -809,6 +826,39 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ /**
+ * Synthesize the caller principal and its principal roles for an external principal. The
+ * principal is assumed to exist and to be valid; neither it nor its roles are read from the
+ * metastore. Synthetic entities carry negative sentinel ids (so they never collide with real,
+ * positive entity ids) and empty grant records.
+ */
+ private ResolverStatus resolveExternalCallerPrincipalAndPrincipalRoles(
+ boolean resolvePrincipalRoles) {
+ PrincipalEntity syntheticPrincipal =
Review Comment:
Hi @adutra and @flyingImer , my apologies for being late to the thread. Just an update on the Authorizer work:
The `PolarisAuthorizer` refactoring is finally complete (https://github.com/apache/polaris/issues/3779), and the remaining work to address this problem is to update the external Authorizers extensions like OPA and Ranger to select the entities they want to resolve (which should be more straight forward given that it's more local and isolated).
I've raised an issue to track the work OPA side that @gracechen09 has picked up, and I think we could do the same for Ranger as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3981688895
##########
extensions/auth/opa/src/opaStartupAction/java/org/apache/polaris/extension/auth/opa/test/OpaStartupAction.java:
##########
@@ -18,87 +18,18 @@
*/
package org.apache.polaris.extension.auth.opa.test;
-import java.io.OutputStream;
-import java.net.HttpURLConnection;
-import java.net.URI;
-import java.net.URL;
-import java.nio.charset.StandardCharsets;
-import java.time.Duration;
-import org.apache.polaris.containerspec.ContainerSpecHelper;
import org.apache.polaris.server.test.runner.spi.PolarisServerStartupAction;
import org.apache.polaris.server.test.runner.spi.PolarisServerStartupContext;
-import org.testcontainers.containers.GenericContainer;
-import org.testcontainers.containers.wait.strategy.Wait;
+import org.apache.polaris.test.opa.OpaContainer;
/** Starts an OPA test server before the external Polaris server process starts. */
public class OpaStartupAction implements PolarisServerStartupAction {
- private static final int OPA_PORT = 8181;
+
private static final String POLICY_NAME = "polaris/authz";
private static final String POLICY_PACKAGE = POLICY_NAME.replace('/', '.');
- private GenericContainer<?> opa;
Review Comment:
We could have a single MockAuthorizer for that, more details are in https://github.com/apache/polaris/pull/5119#discussion_r3981681456.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3909933317
##########
extensions/auth/opa/src/opaStartupAction/java/org/apache/polaris/extension/auth/opa/test/OpaStartupAction.java:
##########
@@ -18,87 +18,18 @@
*/
package org.apache.polaris.extension.auth.opa.test;
-import java.io.OutputStream;
-import java.net.HttpURLConnection;
-import java.net.URI;
-import java.net.URL;
-import java.nio.charset.StandardCharsets;
-import java.time.Duration;
-import org.apache.polaris.containerspec.ContainerSpecHelper;
import org.apache.polaris.server.test.runner.spi.PolarisServerStartupAction;
import org.apache.polaris.server.test.runner.spi.PolarisServerStartupContext;
-import org.testcontainers.containers.GenericContainer;
-import org.testcontainers.containers.wait.strategy.Wait;
+import org.apache.polaris.test.opa.OpaContainer;
/** Starts an OPA test server before the external Polaris server process starts. */
public class OpaStartupAction implements PolarisServerStartupAction {
- private static final int OPA_PORT = 8181;
+
private static final String POLICY_NAME = "polaris/authz";
private static final String POLICY_PACKAGE = POLICY_NAME.replace('/', '.');
- private GenericContainer<?> opa;
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3907627247
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -809,6 +826,39 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ /**
+ * Synthesize the caller principal and its principal roles for an external principal. The
+ * principal is assumed to exist and to be valid; neither it nor its roles are read from the
+ * metastore. Synthetic entities carry negative sentinel ids (so they never collide with real,
+ * positive entity ids) and empty grant records.
+ */
+ private ResolverStatus resolveExternalCallerPrincipalAndPrincipalRoles(
+ boolean resolvePrincipalRoles) {
+ PrincipalEntity syntheticPrincipal =
+ new PrincipalEntity.Builder()
+ .setId(EXTERNAL_PRINCIPAL_ID)
+ .setName(polarisPrincipal.getName())
+ .build();
+ this.resolvedCallerPrincipal =
+ new ResolvedPolarisEntity(syntheticPrincipal, List.of(), List.of());
+ this.addToResolved(this.resolvedCallerPrincipal);
Review Comment:
The ID-only cache fix is correct, but a second same-name issue remains in the next consumer. IIUC, if an external P1 requests credential rotation for stored P1, the Resolver now returns the stored entity; PolarisAdminService then compares only the names, treats it as self-service, and skips the authorizer.
Could we make external callers ineligible for this shortcut? Off top of my head: one option is to return false from isSelfEntity() when the external marker is set, with a denied-rotation test that confirms the stored secret is unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r4074595111
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/external/OidcPolarisCredentialAugmentor.java:
##########
@@ -86,11 +92,18 @@ protected SecurityIdentity setPolarisCredential(
SecurityIdentity identity,
PrincipalMapper principalMapper,
PrincipalRolesMapper rolesMapper) {
- Long principalId =
- principalMapper.mapPrincipalId(identity).stream().boxed().findFirst().orElse(null);
String principalName = principalMapper.mapPrincipalName(identity).orElse(null);
Set<String> principalRoles = rolesMapper.mapPrincipalRoles(identity);
- PolarisCredential credential = PolarisCredential.of(principalId, principalName, principalRoles);
+ // Note: we build the credential even if it doesn't contain enough data to authenticate;
+ // DefaultAuthenticator will reject it later on.
+ PolarisCredential credential;
+ if (authConfig.principalMode() == PrincipalMode.INTERNAL) {
+ Long principalId =
+ principalMapper.mapPrincipalId(identity).stream().boxed().findFirst().orElse(null);
+ credential = InternalPolarisCredential.of(principalId, principalName, principalRoles);
+ } else {
+ credential = ExternalPolarisCredential.of(principalName, principalRoles);
Review Comment:
Since this PR changed substantially today with the addition of a new test module, I went ahead and implemented both suggestions. Hope that's OK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3625335761
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -780,6 +787,16 @@ private ResolverStatus resolvePaths(
private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
List<ResolvedPolarisEntity> toValidate, boolean resolvePrincipalRoles) {
+ // External principals are not backed by the metastore: synthesize the caller principal and its
+ // roles directly from the authenticated principal instead of resolving them from the backend.
+ boolean externalPrincipal =
+ polarisPrincipal
+ .getAttribute(PolarisPrincipal.PRINCIPAL_ENTITY_ATTRIBUTE_KEY, PrincipalEntity.class)
+ .isEmpty();
+ if (externalPrincipal) {
Review Comment:
External-vs-internal here is decided purely by whether the principal carries a `PRINCIPAL_ENTITY_ATTRIBUTE_KEY`. Can we use a more deterministic way? like a flag
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3862053181
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -809,6 +826,39 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ /**
+ * Synthesize the caller principal and its principal roles for an external principal. The
+ * principal is assumed to exist and to be valid; neither it nor its roles are read from the
+ * metastore. Synthetic entities carry negative sentinel ids (so they never collide with real,
+ * positive entity ids) and empty grant records.
+ */
+ private ResolverStatus resolveExternalCallerPrincipalAndPrincipalRoles(
+ boolean resolvePrincipalRoles) {
+ PrincipalEntity syntheticPrincipal =
+ new PrincipalEntity.Builder()
+ .setId(EXTERNAL_PRINCIPAL_ID)
+ .setName(polarisPrincipal.getName())
+ .build();
+ this.resolvedCallerPrincipal =
+ new ResolvedPolarisEntity(syntheticPrincipal, List.of(), List.of());
+ this.addToResolved(this.resolvedCallerPrincipal);
Review Comment:
That's an excellent call, you are 100% right.
TBH the collision is unlikely, but plausible: an external IDP user named "root" makes a management API call that tries to update user `root`. The resolver's `resolveByName` short-circuits and finds the synthetic entity with ID -1 instead of loading the real one from the metastore.
The real fix is to cache only by ID: this avoids collision while still allowing `getFreshlyResolved()` to retrieve entities by ID.
Skipping the by-name cache is safe as it is only used for:
1. Short-circuiting `resolveByName` but that call never happens for external principals;
2. Multi-pass resolution: each pass recreates synthetic entities from scratch anyway, so that's fine too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3861524766
##########
runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java:
##########
@@ -192,6 +193,48 @@ public ProductionReadinessCheck checkTokenBrokers(AuthenticationConfiguration co
return ProductionReadinessCheck.of(errors);
}
+ @Produces
+ public ProductionReadinessCheck checkExternalPrincipals(
+ AuthenticationConfiguration configuration) {
+ List<ProductionReadinessCheck.Error> errors = new ArrayList<>();
+ configuration
+ .realms()
+ .forEach(
+ (realm, config) -> {
+ if (config.principalMode() == PrincipalMode.EXTERNAL
+ && config.type() == AuthenticationType.INTERNAL) {
+ errors.add(
+ Error.ofSevere(
+ "Setting principal mode to EXTERNAL when the authentication type is INTERNAL is not allowed.",
+ "polaris.authentication.%sprincipal-mode"
+ .formatted(authRealmSegment(realm))));
+ }
+ });
+ return ProductionReadinessCheck.of(errors);
+ }
+
+ @Produces
+ public ProductionReadinessCheck checkExternalPrincipalsAuthorizer(
+ AuthenticationConfiguration authenticationConfiguration,
+ AuthorizationConfiguration authorizationConfiguration) {
+ List<ProductionReadinessCheck.Error> errors = new ArrayList<>();
+ if (authorizationConfiguration.type().equals("internal")) {
Review Comment:
I actually have a PR almost ready for this :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3925883441
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -809,6 +826,39 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ /**
+ * Synthesize the caller principal and its principal roles for an external principal. The
+ * principal is assumed to exist and to be valid; neither it nor its roles are read from the
+ * metastore. Synthetic entities carry negative sentinel ids (so they never collide with real,
+ * positive entity ids) and empty grant records.
+ */
+ private ResolverStatus resolveExternalCallerPrincipalAndPrincipalRoles(
+ boolean resolvePrincipalRoles) {
+ PrincipalEntity syntheticPrincipal =
Review Comment:
Sure, here it is: https://github.com/apache/polaris/issues/5439
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3909874137
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -809,6 +826,39 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ /**
+ * Synthesize the caller principal and its principal roles for an external principal. The
+ * principal is assumed to exist and to be valid; neither it nor its roles are read from the
+ * metastore. Synthetic entities carry negative sentinel ids (so they never collide with real,
+ * positive entity ids) and empty grant records.
+ */
+ private ResolverStatus resolveExternalCallerPrincipalAndPrincipalRoles(
+ boolean resolvePrincipalRoles) {
+ PrincipalEntity syntheticPrincipal =
Review Comment:
Thanks @adutra. Can we file an issue for this to track it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r4073061154
##########
runtime/service/build.gradle.kts:
##########
@@ -47,6 +47,8 @@ dependencies {
runtimeOnly(project(":polaris-persistence-nosql-maintenance-impl"))
runtimeOnly(project(":polaris-persistence-nosql-metastore-maintenance"))
+ runtimeOnly(project(":polaris-extensions-auth-opa"))
Review 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:
1. No precedent for `@QuarkusTest` + a container in this repo. Every Keycloak test today is a `@QuarkusIntegrationTest` launching Keycloak via `KeycloakTestResource`. The container-based tests that do live in `src/test` (e.g. `KafkaEventListenerTest`) use the raw JUnit `@Testcontainers` extension, not `@QuarkusTest`. This setup is technically supported though (`KeycloakTestResource` is a `QuarkusTestResourceLifecycleManager`, which works in both modes), but we'd be introducing a new pattern rather than following an established one.
2. 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 move `ExternalPrincipalKeycloakOpaIT` to `@QuarkusTest`, we end up with two nearly-identical Keycloak tests split across `test/` and `intTest/`, and the reason (one needs a test-only authorizer, the other doesn't) isn't obvious from the structure. Moving `RestCatalogKeycloakFileIT` too 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 `@QuarkusTest`s in the `src/test` forlder of `runtime-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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r4073063170
##########
runtime/service/build.gradle.kts:
##########
@@ -47,6 +47,8 @@ dependencies {
runtimeOnly(project(":polaris-persistence-nosql-maintenance-impl"))
runtimeOnly(project(":polaris-persistence-nosql-metastore-maintenance"))
+ runtimeOnly(project(":polaris-extensions-auth-opa"))
Review Comment:
FYI I went ahead and implemented my own suggestion above, but I can revert it if you prefer something else.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3961578273
##########
runtime/service/build.gradle.kts:
##########
@@ -47,6 +47,8 @@ dependencies {
runtimeOnly(project(":polaris-persistence-nosql-maintenance-impl"))
runtimeOnly(project(":polaris-persistence-nosql-metastore-maintenance"))
+ runtimeOnly(project(":polaris-extensions-auth-opa"))
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for external principals [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r4096549261
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/PolarisCredential.java:
##########
@@ -20,29 +20,25 @@
import io.quarkus.security.credential.Credential;
import java.util.Set;
-import org.apache.polaris.immutables.PolarisImmutable;
import org.jspecify.annotations.Nullable;
/**
- * A Quarkus Security {@link Credential} exposing Polaris-specific attributes: the principal id,
- * name, and roles.
+ * A Quarkus Security {@link Credential} exposing Polaris-specific attributes.
+ *
+ * <p>Unless a credential implements {@link
+ * org.apache.polaris.service.auth.external.ExternalPolarisCredential}, it is treated as internal by
+ * the authenticator and requires a backing principal entity in the Polaris metastore.
+ *
+ * @see org.apache.polaris.service.auth.internal.InternalPolarisCredential
+ * @see org.apache.polaris.service.auth.external.ExternalPolarisCredential
*/
-@PolarisImmutable
public interface PolarisCredential extends Credential {
- 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();
Review Comment:
That's a nice suggestion! Adopted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org