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 2025/04/18 11:15:45 UTC
[PR] Support for external identity providers [polaris]
adutra opened a new pull request, #1397:
URL: https://github.com/apache/polaris/pull/1397
Fixes #336 and #976.
### Overview
This change is globally backwards-compatible, there are no configuration breaking changes and the default server behavior is the same as today (internal authentication only).
The main interfaces are also unchanged – but some implementations changed.
### TODOs
Things to do in follow-up PRs:
- [ ] Integration tests (these will be tricky to implement, therefore I prefer to tackle that later)
- [ ] Helm chart changes
- [ ] Documentation
### Authentication Types
To support external identity providers, the notion of "authentication type" has been introduced. The following values are supported:
- `internal`: Polaris internal authentication only (default)
- `external`: Polaris external authentication only (using Quarkus OIDC)
- When this type is enabled globally or for a realm, the realm's internal token endpoint _is disabled and returns 501_.
- `mixed`: Polaris internal and external authentication (using Quarkus OIDC)
- internal is tried first, then external
The following authentication type is also present, but I would be in favor of removing it:
- `test`: The old `TestInlineBearerTokenAuthenticator`, currently unused in Polaris
The default authentication type is `internal`, but can be either changed globally or overridden per realm.
The authentication type is configured in the `application.properties` file as below:
```properties
# Default authentication type
polaris.authentication.type=internal
# Authentication type overrides per realm
polaris.authentication.realm1.type=external
polaris.authentication.realm2.type=mixed
```
See below for details of each authentication type.
### Configuration Changes
The `polaris.authentication` configuration section remains backwards-compatible, but everything now can be overridden per realm.
The following options are effective only when using internal auth (can be overridden in per realm):
- `polaris.authentication.token-service.*`
- `polaris.authentication.token-broker.*`
The `polaris.oidc` configuration section is new and is used to further configure the OIDC tenants. See below for examples.
### Authenticator Changes
The `Authenticator` interface remains the same, but `BasePolarisAuthenticator` has been modified and became `DefaultAuthenticator`.
This is because Quarkus Security distinguishes two phases of authentication:
1. Extract the credentials (i.e., decode the token)
2. Authenticate the credentials
Therefore, **the `Authenticator` is not responsible anymore for decoding the token, but only for authenticating it**.
A new `PolarisCredential` interface has been introduced to hold the decoded JWT and pass it to the authenticator. The existing `DecodedToken` interface, which is used for internal authentication, is now a sub-type of `PolarisCredential`.
**The default logic for authenticating the principal, i.e. by a metastore lookup by ID or by name, remains the same.**
### Token Broker Changes
Token brokers now need to be injected directly, instead of injecting their factories. They are now request-scoped beans.
As a bonus, token brokers can now be configured per realm, i.e., one realm could use RSA key pairs, and another one shared secrets.
### Authentication Workflows in Detail
#### Internal
- `InternalAuthenticationMechanism` handles the authentication header
- Calls `JWTBroker` to decode the token
- `InternalIdentityProvider` creates a first SecurityIdentity with the token as a `PolarisCredential`
- `AuthenticatingAugmentor` authenticates the `PolarisCredential`
- Calls `Authenticator.authenticate()`
- Sets `AuthenticatedPolarisPrincipal` as the SecurityIdentity's principal
- `ActiveRolesAugmentor` sets the active roles in the SecurityIdentity
- Calls `ActiveRolesProvider`
#### External
- [Quarkus OIDC] `OidcAuthenticationMechanism` handles the authentication header
- [Quarkus OIDC] `OidcIdentityProvider` creates a first SecurityIdentity with the token
- `ClaimsMappingAugmentor` maps the JWT claims to a `PolarisCredential` and adds it to the SecurityIdentity
- `AuthenticatingAugmentor` authenticates the `PolarisCredential`
- Calls `Authenticator.authenticate()`
- Sets `AuthenticatedPolarisPrincipal` as the SecurityIdentity's principal
- `ActiveRolesAugmentor` sets the active roles in the SecurityIdentity
- Calls `ActiveRolesProvider`
#### Mixed
- `InternalAuthenticationMechanism` handles the authentication header
- Calls `JWTBroker` to decode the token
- If the token is issued by Polaris:
- `InternalIdentityProvider` creates a first SecurityIdentity with the token as a `PolarisCredential`
- Otherwise:
`InternalAuthenticationMechanism` yields to the OIDC mechanism
- [Quarkus OIDC] `OidcAuthenticationMechanism` handles the authentication header
- [Quarkus OIDC] `OidcIdentityProvider` creates a first SecurityIdentity with the token
- `ClaimsMappingAugmentor` maps the JWT claims to a `PolarisCredential` and adds it to the SecurityIdentity
- For both cases: `AuthenticatingAugmentor` authenticates the `PolarisCredential`
- Calls `Authenticator.authenticate()`
- Sets `AuthenticatedPolarisPrincipal` as the SecurityIdentity's principal
- `ActiveRolesAugmentor` sets the active roles in the SecurityIdentity
- Calls `ActiveRolesProvider`
### OIDC Claims Mapping
Currently, the following `polaris.oidc` options are available:
* `polaris.oidc.claims.principal-id-claim-path`: the claim path to the principal id in the JWT token, e.g. `polaris/principal_id`
* `polaris.oidc.claims.principal-name-claim-path`: the claim path to the principal name in the JWT token, e.g. `polaris/principal_name`
They can be overridden _per OIDC tenant_ (this is different from the realm!). The OIDC tenants will be looked up in the `quarkus.oidc` configuration section, e.g.:
```properties
quarkus.oidc.oidc-tenant1.auth-server-url=http://localhost:8080/realms/polaris
quarkus.oidc.oidc-tenant1.client-id=client1
quarkus.oidc.oidc-tenant1.application-type=service
polaris.oidc.oidc-tenant1.claims.principal-id-claim-path=polaris/principal_id
polaris.oidc.oidc-tenant1.claims.principal-name-claim-path=polaris/principal_name
```
If a tenant is not found in the `quarkus.oidc` or in the `polaris.oidc` section, the default OIDC tenant will be used.
### OIDC Roles Mapping
Roles will be mapped from the JWT by Quarkus OIDC. This is done by setting the `quarkus.oidc.roles.role-claim-path` property:
```properties
quarkus.oidc.roles.role-claim-path=polaris/roles
```
See https://quarkus.io/guides/security-oidc-bearer-token-authentication#token-claims-and-security-identity-roles for more information.
### Mapping Examples
Example: for a JWT token like this (some claims omitted for brevity):
```json
{
"sub": "a663c299-1118-4b35-ac85-f9a4f38d0699",
"typ": "Bearer",
"azp": "client1",
"scope": "profile email",
"polaris": {
"roles": [ "PRINCIPAL_ROLE:ALL" ],
"principal_name": "root",
"principal_id": 0
},
"preferred_username": "service-account-client1",
"clientAddress": "172.18.0.1",
"client_id": "client1"
}
```
You can configure Polaris like this:
```properties
quarkus.oidc.roles.role-claim-path=polaris/roles
polaris.oidc.claims.principal-id-claim-path=polaris/principal_id
polaris.oidc.claims.principal-name-claim-path=polaris/principal_name
```
Another example; if the JWT token is like this:
```json
{
"sub": "0", // principal id
"typ": "Bearer",
"azp": "client1",
"scope": "PRINCIPAL_ROLE:service_admin PRINCIPAL_ROLE:catalog_admin profile email",
"preferred_username": "root",
"clientAddress": "172.18.0.1",
"client_id": "client1"
}
```
You can configure Polaris like this:
```properties
quarkus.oidc.roles.role-claim-path=scope
polaris.oidc.claims.principal-id-claim-path=sub
polaris.oidc.claims.principal-name-claim-path=preferred_username
```
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2050607126
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthenticatedPolarisPrincipal.java:
##########
@@ -48,14 +48,23 @@ public PolarisEntity getPrincipalEntity() {
return principalEntity;
}
+ /**
+ * Returns the set of activated principal role names. Activated role names are the roles that were
+ * explicitly requested by the client when authenticating, through JWT claims or other means.
+ *
+ * <p>By convention, this method returns an empty set when the principal is requesting all
+ * available principal roles.
+ */
Review Comment:
I'm trying to document things as I go. This is the best knowledge I can get for this principal attribute.
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2050620639
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/ratelimiter/RateLimiterFilterTest.java:
##########
@@ -77,7 +77,11 @@ public Map<String, String> getConfigOverrides() {
"polaris.metrics.tags.environment",
"prod",
"polaris.realm-context.type",
- "test");
+ "test",
Review Comment:
This test would fail without this change because each realm would have a different pair of RSA keys. So switching to shared secret.
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2050608447
##########
quarkus/defaults/src/main/resources/application.properties:
##########
@@ -74,6 +75,20 @@ quarkus.log.category."io.smallrye.config".level=INFO
quarkus.management.port=8182
quarkus.management.test-port=0
+# OIDC settings. These settings are required only when using external authentication providers.
+# See https://quarkus.io/guides/security-oidc-configuration-properties-reference
+quarkus.oidc.application-type=service
+quarkus.oidc.resolve-tenants-with-issuer=true
+# Default tenant (disabled by default, set this to true if you use external authentication)
+quarkus.oidc.tenant-enabled=false
Review Comment:
We could enable this conditionally with a `ConfigSourceInterceptor` that would check if any realm is using external or mixed auth, in which case it makes sense to enable the default OIDC tenant, especially if not other tenant is configured. But I'm leaving this as a follow-up improvement.
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060400365
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -88,17 +92,19 @@ protected Optional<AuthenticatedPolarisPrincipal> getPrincipal(DecodedToken toke
.log("Unable to authenticate user with token");
throw new ServiceFailureException("Unable to fetch principal entity");
}
- if (principal == null) {
- LOGGER.warn(
- "Failed to resolve principal from tokenInfo client_id={}", tokenInfo.getClientId());
+ if (principal == null || principal.getType() != PolarisEntityType.PRINCIPAL) {
+ LOGGER.warn("Failed to resolve principal from credentials={}", credentials);
throw new NotAuthorizedException("Unable to authenticate");
}
+ LOGGER.debug("Resolved principal: {}", principal);
+
+ boolean allRoles = credentials.getPrincipalRoles().contains(PRINCIPAL_ROLE_ALL);
+
Set<String> activatedPrincipalRoles = new HashSet<>();
- // TODO: Consolidate the divergent "scopes" logic between test-bearer-token and token-exchange.
- if (tokenInfo.getScope() != null && !tokenInfo.getScope().equals(PRINCIPAL_ROLE_ALL)) {
+ if (!allRoles) {
activatedPrincipalRoles.addAll(
- Arrays.stream(tokenInfo.getScope().split(" "))
+ credentials.getPrincipalRoles().stream()
.map(
s -> // strip the principal_role prefix, if present
s.startsWith(PRINCIPAL_ROLE_PREFIX)
Review Comment:
BTW I also think that the responsibility of stripping off the prefix should be transferred from this class to `DefaultRolesMapper`. But we can do this 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 identity providers [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2069879396
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/external/mapping/DefaultPrincipalMapper.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.service.quarkus.auth.external.mapping;
+
+import static org.apache.polaris.service.quarkus.auth.external.OidcTenantResolvingAugmentor.getOidcTenantConfig;
+
+import io.quarkus.security.identity.SecurityIdentity;
+import io.smallrye.common.annotation.Identifier;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.inject.Inject;
+import java.util.Optional;
+import java.util.OptionalLong;
+import org.eclipse.microprofile.jwt.JsonWebToken;
+
+/**
+ * A default implementation of {@link PrincipalMapper}. It maps the {@link SecurityIdentity} to a
+ * Polaris principal by extracting the ID and name from the JWT claims, based on the configuration
+ * provided in the OIDC tenant.
+ */
+@ApplicationScoped
+@Identifier("default")
+public class DefaultPrincipalMapper implements PrincipalMapper {
+
+ @Inject ClaimsLocator claimsLocator;
Review Comment:
constructor inject?
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/external/OidcTenantResolvingAugmentor.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.service.quarkus.auth.external;
+
+import io.quarkus.security.identity.AuthenticationRequestContext;
+import io.quarkus.security.identity.SecurityIdentity;
+import io.quarkus.security.identity.SecurityIdentityAugmentor;
+import io.quarkus.security.runtime.QuarkusSecurityIdentity;
+import io.smallrye.mutiny.Uni;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.inject.Inject;
+import org.apache.polaris.service.quarkus.auth.external.tenant.OidcTenantResolver;
+import org.eclipse.microprofile.jwt.JsonWebToken;
+
+/**
+ * A {@link SecurityIdentityAugmentor} that resolves the Polaris OIDC tenant configuration for the
+ * given identity and adds it as an attribute to the {@link SecurityIdentity}.
+ */
+@ApplicationScoped
+public class OidcTenantResolvingAugmentor implements SecurityIdentityAugmentor {
Review Comment:
I think a lot of these classes can be package private. Any reason to expose them outside of this package?
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/external/mapping/ClaimsLocator.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * 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.service.quarkus.auth.external.mapping;
+
+import jakarta.annotation.Nullable;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.json.JsonNumber;
+import jakarta.json.JsonString;
+import jakarta.json.JsonValue;
+import java.util.AbstractMap;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.eclipse.microprofile.jwt.JsonWebToken;
+
+/** A utility class for locating claims in a JWT token by path. */
+@ApplicationScoped
+public class ClaimsLocator {
Review Comment:
package private?
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/ActiveRolesAugmentor.java:
##########
@@ -32,33 +32,42 @@
/**
* A custom {@link SecurityIdentityAugmentor} that adds active roles to the {@link
- * SecurityIdentity}. This is used to augment the identity with active roles after authentication.
+ * SecurityIdentity}. This is used to augment the identity with valid active roles after
+ * authentication.
*/
@ApplicationScoped
public class ActiveRolesAugmentor implements SecurityIdentityAugmentor {
+ // must run after AuthenticatingAugmentor
+ public static final int PRIORITY = AuthenticatingAugmentor.PRIORITY - 1;
+
@Inject ActiveRolesProvider activeRolesProvider;
+ @Override
+ public int priority() {
+ return PRIORITY;
+ }
+
@Override
public Uni<SecurityIdentity> augment(
SecurityIdentity identity, AuthenticationRequestContext context) {
if (identity.isAnonymous()) {
return Uni.createFrom().item(identity);
}
- return context.runBlocking(() -> augmentWithActiveRoles(identity));
+ return context.runBlocking(() -> validateActiveRoles(identity));
}
- private SecurityIdentity augmentWithActiveRoles(SecurityIdentity identity) {
- AuthenticatedPolarisPrincipal polarisPrincipal =
- identity.getPrincipal(AuthenticatedPolarisPrincipal.class);
- if (polarisPrincipal == null) {
+ private SecurityIdentity validateActiveRoles(SecurityIdentity identity) {
+ if (!(identity.getPrincipal() instanceof AuthenticatedPolarisPrincipal)) {
throw new AuthenticationFailedException("No Polaris principal found");
}
+ AuthenticatedPolarisPrincipal polarisPrincipal =
+ identity.getPrincipal(AuthenticatedPolarisPrincipal.class);
Set<String> validRoleNames = activeRolesProvider.getActiveRoles(polarisPrincipal);
Review Comment:
I wonder if the `ActiveRolesProvider` doesn't need more context about the roles the principal should have. E.g., in the external auth case, the token indicates the roles, whereas in the internal case, we have to look up grant records. How does the role provider know whether to accept the tokens contents or to validate the grants?
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/external/OidcTenantResolvingAugmentor.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.service.quarkus.auth.external;
+
+import io.quarkus.security.identity.AuthenticationRequestContext;
+import io.quarkus.security.identity.SecurityIdentity;
+import io.quarkus.security.identity.SecurityIdentityAugmentor;
+import io.quarkus.security.runtime.QuarkusSecurityIdentity;
+import io.smallrye.mutiny.Uni;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.inject.Inject;
+import org.apache.polaris.service.quarkus.auth.external.tenant.OidcTenantResolver;
+import org.eclipse.microprofile.jwt.JsonWebToken;
+
+/**
+ * A {@link SecurityIdentityAugmentor} that resolves the Polaris OIDC tenant configuration for the
+ * given identity and adds it as an attribute to the {@link SecurityIdentity}.
+ */
+@ApplicationScoped
+public class OidcTenantResolvingAugmentor implements SecurityIdentityAugmentor {
+
+ public static final String TENANT_CONFIG_ATTRIBUTE = "polaris-tenant-config";
+
+ // must run before PrincipalAuthInfoAugmentor
+ public static final int PRIORITY = PrincipalAuthInfoAugmentor.PRIORITY + 100;
+
+ public static OidcTenantConfiguration getOidcTenantConfig(SecurityIdentity identity) {
+ return identity.getAttribute(TENANT_CONFIG_ATTRIBUTE);
+ }
+
+ @Inject OidcTenantResolver resolver;
Review Comment:
Why no constructor injection?
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2050647833
##########
service/common/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -62,29 +65,42 @@ public abstract class JWTBroker implements TokenBroker {
@Override
public DecodedToken verify(String token) {
+
+ DecodedJWT decodedJWT;
+ try {
+ decodedJWT = JWT.decode(token);
+ } catch (JWTDecodeException e) {
+ LOGGER.error("Failed to decode the token", e);
+ throw new NotAuthorizedException("Failed to decode the token");
+ }
+
+ if (!decodedJWT.getIssuer().equals(ISSUER_KEY)) {
+ throw new UnprocessableEntityException("Unexpected issuer: %s", decodedJWT.getIssuer());
Review Comment:
Here, we need a way to signal the caller that the token issuer is wrong. This must be a different error so that the caller can distinguish this situation from a validation error situation.
I picked `UnprocessableEntityException` out of better choices.
--
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 identity providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2070321516
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/ActiveRolesAugmentor.java:
##########
@@ -32,33 +32,42 @@
/**
* A custom {@link SecurityIdentityAugmentor} that adds active roles to the {@link
- * SecurityIdentity}. This is used to augment the identity with active roles after authentication.
+ * SecurityIdentity}. This is used to augment the identity with valid active roles after
+ * authentication.
*/
@ApplicationScoped
public class ActiveRolesAugmentor implements SecurityIdentityAugmentor {
+ // must run after AuthenticatingAugmentor
+ public static final int PRIORITY = AuthenticatingAugmentor.PRIORITY - 1;
+
@Inject ActiveRolesProvider activeRolesProvider;
+ @Override
+ public int priority() {
+ return PRIORITY;
+ }
+
@Override
public Uni<SecurityIdentity> augment(
SecurityIdentity identity, AuthenticationRequestContext context) {
if (identity.isAnonymous()) {
return Uni.createFrom().item(identity);
}
- return context.runBlocking(() -> augmentWithActiveRoles(identity));
+ return context.runBlocking(() -> validateActiveRoles(identity));
}
- private SecurityIdentity augmentWithActiveRoles(SecurityIdentity identity) {
- AuthenticatedPolarisPrincipal polarisPrincipal =
- identity.getPrincipal(AuthenticatedPolarisPrincipal.class);
- if (polarisPrincipal == null) {
+ private SecurityIdentity validateActiveRoles(SecurityIdentity identity) {
+ if (!(identity.getPrincipal() instanceof AuthenticatedPolarisPrincipal)) {
throw new AuthenticationFailedException("No Polaris principal found");
}
+ AuthenticatedPolarisPrincipal polarisPrincipal =
+ identity.getPrincipal(AuthenticatedPolarisPrincipal.class);
Set<String> validRoleNames = activeRolesProvider.getActiveRoles(polarisPrincipal);
Review Comment:
Deferring this improvement makes sense to me. Also +1 to option 2 above.
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2051594977
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthenticatedPolarisPrincipal.java:
##########
@@ -56,10 +72,20 @@ public List<PrincipalRoleEntity> getActivatedPrincipalRoles() {
return activatedPrincipalRoles;
}
+ /** FIXME this method makes this class mutable, which seems risky */
public void setActivatedPrincipalRoles(List<PrincipalRoleEntity> activatedPrincipalRoles) {
this.activatedPrincipalRoles = activatedPrincipalRoles;
}
+ /**
+ * Whether all roles should be activated. This is true when the principal is requesting all
+ * available principal roles. When this is true, {@link #getActivatedPrincipalRoleNames()} returns
+ * an empty set.
+ */
+ public boolean allRoles() {
Review Comment:
I think we need this method to distinguish the case when the principal is requesting all roles from the case where no roles were found in the token – since both cases translate into an empty set of role names.
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060274893
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/ProductionReadinessChecks.java:
##########
@@ -109,43 +93,80 @@ public ProductionReadinessCheck checkTokenService(IcebergRestOAuth2ApiService se
}
@Produces
- public ProductionReadinessCheck checkTokenBroker(
- AuthenticationConfiguration configuration, TokenBrokerFactory factory) {
- if (factory instanceof JWTRSAKeyPairFactory) {
- if (configuration
- .tokenBroker()
- .rsaKeyPair()
- .map(RSAKeyPairConfiguration::publicKeyFile)
- .isEmpty()) {
- return ProductionReadinessCheck.of(
- Error.of(
- "A public key file wasn't provided and will be generated.",
- "polaris.authentication.token-broker.rsa-key-pair.public-key-file"));
- }
- if (configuration
- .tokenBroker()
- .rsaKeyPair()
- .map(RSAKeyPairConfiguration::privateKeyFile)
- .isEmpty()) {
- return ProductionReadinessCheck.of(
- Error.of(
- "A private key file wasn't provided and will be generated.",
- "polaris.authentication.token-broker.rsa-key-pair.private-key-file"));
- }
- }
- if (factory instanceof JWTSymmetricKeyFactory) {
- if (configuration
- .tokenBroker()
- .symmetricKey()
- .map(SymmetricKeyConfiguration::secret)
- .isPresent()) {
- return ProductionReadinessCheck.of(
- Error.of(
- "A symmetric key secret was provided through configuration rather than through a secret file.",
- "polaris.authentication.token-broker.symmetric-key.secret"));
- }
- }
- return ProductionReadinessCheck.OK;
+ public ProductionReadinessCheck checkAuthenticationType(
+ AuthenticationConfiguration configuration) {
+ List<ProductionReadinessCheck.Error> errors = new ArrayList<>();
+ configuration
+ .realms()
+ .forEach(
+ (realm, config) -> {
+ AuthenticationType authenticationType = config.type();
+ if (authenticationType == AuthenticationType.INTERNAL
+ || authenticationType == AuthenticationType.MIXED) {
+ errors.add(
+ Error.of(
+ "Internal authentication is deprecated since Iceberg 1.6.0.",
Review Comment:
I don't _need_ Iceberg 1.9 and auth manager, client credentials should still work, although without token refreshes. But OK to remove 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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060656678
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/internal/InternalAuthenticationMechanism.java:
##########
@@ -56,9 +84,34 @@ public Uni<SecurityIdentity> authenticate(
}
String credential = authHeader.substring(spaceIdx + 1);
+
+ DecodedToken token;
+ try {
+ token = decodeToken(credential);
+ } catch (Exception e) {
+ return configuration.type() == AuthenticationType.MIXED
Review Comment:
Hmm actually my idea won't even work. We need to verify the signature as well in `InternalAuthenticationMechanism`. That's the _only_ way to be sure that the token is or isn't internal to Polaris.
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2050647833
##########
service/common/src/main/java/org/apache/polaris/service/auth/JWTBroker.java:
##########
@@ -62,29 +65,42 @@ public abstract class JWTBroker implements TokenBroker {
@Override
public DecodedToken verify(String token) {
+
+ DecodedJWT decodedJWT;
+ try {
+ decodedJWT = JWT.decode(token);
+ } catch (JWTDecodeException e) {
+ LOGGER.error("Failed to decode the token", e);
+ throw new NotAuthorizedException("Failed to decode the token");
+ }
+
+ if (!decodedJWT.getIssuer().equals(ISSUER_KEY)) {
+ throw new UnprocessableEntityException("Unexpected issuer: %s", decodedJWT.getIssuer());
Review Comment:
Here, we need a way to signal the caller that the token issuer is wrong. This must be a different error so that the caller can distinguish this situation from a validation error situation.
I picked `UnprocessableEntityException` out of better choices.
--
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 identity providers [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2070616568
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/ActiveRolesAugmentor.java:
##########
@@ -32,33 +32,42 @@
/**
* A custom {@link SecurityIdentityAugmentor} that adds active roles to the {@link
- * SecurityIdentity}. This is used to augment the identity with active roles after authentication.
+ * SecurityIdentity}. This is used to augment the identity with valid active roles after
+ * authentication.
*/
@ApplicationScoped
public class ActiveRolesAugmentor implements SecurityIdentityAugmentor {
+ // must run after AuthenticatingAugmentor
+ public static final int PRIORITY = AuthenticatingAugmentor.PRIORITY - 1;
+
@Inject ActiveRolesProvider activeRolesProvider;
+ @Override
+ public int priority() {
+ return PRIORITY;
+ }
+
@Override
public Uni<SecurityIdentity> augment(
SecurityIdentity identity, AuthenticationRequestContext context) {
if (identity.isAnonymous()) {
return Uni.createFrom().item(identity);
}
- return context.runBlocking(() -> augmentWithActiveRoles(identity));
+ return context.runBlocking(() -> validateActiveRoles(identity));
}
- private SecurityIdentity augmentWithActiveRoles(SecurityIdentity identity) {
- AuthenticatedPolarisPrincipal polarisPrincipal =
- identity.getPrincipal(AuthenticatedPolarisPrincipal.class);
- if (polarisPrincipal == null) {
+ private SecurityIdentity validateActiveRoles(SecurityIdentity identity) {
+ if (!(identity.getPrincipal() instanceof AuthenticatedPolarisPrincipal)) {
throw new AuthenticationFailedException("No Polaris principal found");
}
+ AuthenticatedPolarisPrincipal polarisPrincipal =
+ identity.getPrincipal(AuthenticatedPolarisPrincipal.class);
Set<String> validRoleNames = activeRolesProvider.getActiveRoles(polarisPrincipal);
Review Comment:
This plan sounds ok to me. Using the different `ActiveRolesProvider` for different authn methods sounds like the right way to go.
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060211123
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -88,17 +92,19 @@ protected Optional<AuthenticatedPolarisPrincipal> getPrincipal(DecodedToken toke
.log("Unable to authenticate user with token");
throw new ServiceFailureException("Unable to fetch principal entity");
}
- if (principal == null) {
- LOGGER.warn(
- "Failed to resolve principal from tokenInfo client_id={}", tokenInfo.getClientId());
+ if (principal == null || principal.getType() != PolarisEntityType.PRINCIPAL) {
+ LOGGER.warn("Failed to resolve principal from credentials={}", credentials);
throw new NotAuthorizedException("Unable to authenticate");
}
+ LOGGER.debug("Resolved principal: {}", principal);
+
+ boolean allRoles = credentials.getPrincipalRoles().contains(PRINCIPAL_ROLE_ALL);
+
Set<String> activatedPrincipalRoles = new HashSet<>();
- // TODO: Consolidate the divergent "scopes" logic between test-bearer-token and token-exchange.
- if (tokenInfo.getScope() != null && !tokenInfo.getScope().equals(PRINCIPAL_ROLE_ALL)) {
+ if (!allRoles) {
activatedPrincipalRoles.addAll(
- Arrays.stream(tokenInfo.getScope().split(" "))
+ credentials.getPrincipalRoles().stream()
.map(
s -> // strip the principal_role prefix, if present
s.startsWith(PRINCIPAL_ROLE_PREFIX)
Review Comment:
Okay, let's change 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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra merged PR #1397:
URL: https://github.com/apache/polaris/pull/1397
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2050620138
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/it/QuarkusApplicationIntegrationTest.java:
##########
@@ -61,10 +58,6 @@ public Map<String, String> getConfigOverrides() {
}
}
- @Inject
- @Identifier("rsa-key-pair")
- TokenBrokerFactory tokenBrokerFactory;
Review Comment:
Unused.
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #1397:
URL: https://github.com/apache/polaris/pull/1397#issuecomment-3148369363
FYi I'm adding a getting-started example: #2244
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2050612375
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/PolarisIdentityProvider.java:
##########
@@ -1,80 +0,0 @@
-/*
- * 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.service.quarkus.auth;
-
-import io.quarkus.security.AuthenticationFailedException;
-import io.quarkus.security.identity.AuthenticationRequestContext;
-import io.quarkus.security.identity.IdentityProvider;
-import io.quarkus.security.identity.SecurityIdentity;
-import io.quarkus.security.identity.request.TokenAuthenticationRequest;
-import io.quarkus.security.runtime.QuarkusSecurityIdentity;
-import io.quarkus.vertx.http.runtime.security.HttpSecurityUtils;
-import io.smallrye.mutiny.Uni;
-import io.vertx.ext.web.RoutingContext;
-import jakarta.enterprise.context.ApplicationScoped;
-import jakarta.inject.Inject;
-import jakarta.ws.rs.NotAuthorizedException;
-import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal;
-import org.apache.polaris.service.auth.Authenticator;
-import org.apache.polaris.service.quarkus.auth.PolarisAuthenticationMechanism.PolarisTokenCredential;
-
-/** A custom {@link IdentityProvider} that handles Polaris token authentication requests. */
-@ApplicationScoped
-public class PolarisIdentityProvider implements IdentityProvider<TokenAuthenticationRequest> {
Review Comment:
FYI this was just moved to `org.apache.polaris.service.quarkus.auth.internal.InternalIdentityProvider` but git lost track of 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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060304846
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/internal/InternalAuthenticationMechanism.java:
##########
@@ -56,9 +84,34 @@ public Uni<SecurityIdentity> authenticate(
}
String credential = authHeader.substring(spaceIdx + 1);
+
+ DecodedToken token;
+ try {
+ token = decodeToken(credential);
+ } catch (Exception e) {
+ return configuration.type() == AuthenticationType.MIXED
Review Comment:
Well, the issue is that "parse" and "validate" are basically tied together.
Auth0 has two methods: `JWT.decode` and `JWT.require`: the former decodes without validating the signature, the latter decodes and validates.
Currently, `JWTBroker.verify()` uses `JWT.require`. We _could_ introduce a new method in `TokenBroker`, e.g. `parse()` or `decode()`, and call `JWT.decode` there. But I don't know if it's worth the hassle: I bet that decoding the token is just slightly faster than decoding and verifying.
BTW that's why I introduced the `MIXED` authentication type: so that the extra penalty of decoding the token twice is only paid by realms that opt for that authentication type.
--
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 identity providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060335924
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultActiveRolesProvider.java:
##########
@@ -82,7 +82,9 @@ protected List<PrincipalRoleEntity> loadActivePrincipalRoles(
principal.getId());
throw new NotAuthorizedException("Unable to authenticate");
}
- boolean allRoles = tokenRoles.contains(BasePolarisAuthenticator.PRINCIPAL_ROLE_ALL);
+
+ // FIXME how to distinguish allRoles from no roles at all?
Review Comment:
I always thought assuming all roles by default was normal in Polaris. I do not see any enforced restrictions for roles in Polaris-owned tokens. So roles in Polaris tokens always looked like a self-imposed restriction to me.
External tokens would have roles / scopes injected by the IdP, I assume.
@collado-mike : 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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2070230336
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/ActiveRolesAugmentor.java:
##########
@@ -32,33 +32,42 @@
/**
* A custom {@link SecurityIdentityAugmentor} that adds active roles to the {@link
- * SecurityIdentity}. This is used to augment the identity with active roles after authentication.
+ * SecurityIdentity}. This is used to augment the identity with valid active roles after
+ * authentication.
*/
@ApplicationScoped
public class ActiveRolesAugmentor implements SecurityIdentityAugmentor {
+ // must run after AuthenticatingAugmentor
+ public static final int PRIORITY = AuthenticatingAugmentor.PRIORITY - 1;
+
@Inject ActiveRolesProvider activeRolesProvider;
+ @Override
+ public int priority() {
+ return PRIORITY;
+ }
+
@Override
public Uni<SecurityIdentity> augment(
SecurityIdentity identity, AuthenticationRequestContext context) {
if (identity.isAnonymous()) {
return Uni.createFrom().item(identity);
}
- return context.runBlocking(() -> augmentWithActiveRoles(identity));
+ return context.runBlocking(() -> validateActiveRoles(identity));
}
- private SecurityIdentity augmentWithActiveRoles(SecurityIdentity identity) {
- AuthenticatedPolarisPrincipal polarisPrincipal =
- identity.getPrincipal(AuthenticatedPolarisPrincipal.class);
- if (polarisPrincipal == null) {
+ private SecurityIdentity validateActiveRoles(SecurityIdentity identity) {
+ if (!(identity.getPrincipal() instanceof AuthenticatedPolarisPrincipal)) {
throw new AuthenticationFailedException("No Polaris principal found");
}
+ AuthenticatedPolarisPrincipal polarisPrincipal =
+ identity.getPrincipal(AuthenticatedPolarisPrincipal.class);
Set<String> validRoleNames = activeRolesProvider.getActiveRoles(polarisPrincipal);
Review Comment:
That's a very good question. You'll notice that for now only one impl of `ActiveRolesProvider` exists, which will invariably lookup the grants in the database.
When we have federated principals, my suggestion would be to change this Augmentor by either:
1. Creating one different Augmentor for each authentication type (internal/external)
2. Make the Augmentor use a different impl of `ActiveRolesProvider` depending on the authentication type.
My preference would be for 2, but in any case, my intention was to tackle that once we have federated principals. Wdyt? Does this plan work for you?
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060400365
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -88,17 +92,19 @@ protected Optional<AuthenticatedPolarisPrincipal> getPrincipal(DecodedToken toke
.log("Unable to authenticate user with token");
throw new ServiceFailureException("Unable to fetch principal entity");
}
- if (principal == null) {
- LOGGER.warn(
- "Failed to resolve principal from tokenInfo client_id={}", tokenInfo.getClientId());
+ if (principal == null || principal.getType() != PolarisEntityType.PRINCIPAL) {
+ LOGGER.warn("Failed to resolve principal from credentials={}", credentials);
throw new NotAuthorizedException("Unable to authenticate");
}
+ LOGGER.debug("Resolved principal: {}", principal);
+
+ boolean allRoles = credentials.getPrincipalRoles().contains(PRINCIPAL_ROLE_ALL);
+
Set<String> activatedPrincipalRoles = new HashSet<>();
- // TODO: Consolidate the divergent "scopes" logic between test-bearer-token and token-exchange.
- if (tokenInfo.getScope() != null && !tokenInfo.getScope().equals(PRINCIPAL_ROLE_ALL)) {
+ if (!allRoles) {
activatedPrincipalRoles.addAll(
- Arrays.stream(tokenInfo.getScope().split(" "))
+ credentials.getPrincipalRoles().stream()
.map(
s -> // strip the principal_role prefix, if present
s.startsWith(PRINCIPAL_ROLE_PREFIX)
Review Comment:
BTW I also think that the responsibility of stripping off the prefix should be transferred from this class to `DefaultPrincipalRolesMapper`. But we can do this 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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2050627200
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultActiveRolesProvider.java:
##########
@@ -82,7 +82,9 @@ protected List<PrincipalRoleEntity> loadActivePrincipalRoles(
principal.getId());
throw new NotAuthorizedException("Unable to authenticate");
}
- boolean allRoles = tokenRoles.contains(BasePolarisAuthenticator.PRINCIPAL_ROLE_ALL);
+
+ // By convention, an empty set means all roles are activated
+ boolean allRoles = tokenRoles.isEmpty();
Review Comment:
As far as I understand this logic, I believe that the old code: `tokenRoles.contains(BasePolarisAuthenticator.PRINCIPAL_ROLE_ALL)` wasn't correct, because it is impossible that `tokenRoles` would contain `"PRINCIPAL_ROLE:ALL"`, since when this pseudo-role is present in the token, this translates into an empty 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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #1397:
URL: https://github.com/apache/polaris/pull/1397#issuecomment-2815380643
For those willing to test out integration with Keycloak:
1. Start a Keycloak server on port 8080 e.g.: `http://localhost:8080/realms/master`
2. With keycloak admin UI, configure a client to return the following claims (e.g. using hard-coded claim mappers):
a. `polaris/principal_id`
b. `polaris/principal_name`
c. `polaris/roles`
3. Run Polaris with:
```shell
./gradlew :polaris-quarkus-service:quarkusDev \
-Dpolaris.realm-context.realms=realm1,realm2,realm3 \
-Dpolaris.bootstrap.credentials="realm1,root,secret;realm2,root,secret;realm3,root,secret" \
-Dpolaris.authentication.realm2.type=external \
-Dpolaris.authentication.realm3.type=mixed \
-Dquarkus.oidc.tenant-enabled=true \
-Dquarkus.oidc.auth-server-url=http://localhost:8080/realms/master \
-Dquarkus.oidc.roles.role-claim-path=polaris/roles \
-Dpolaris.oidc.claims.principal-id-claim-path=polaris/principal_id \
-Dpolaris.oidc.claims.principal-name-claim-path=polaris/principal_name
```
4. 3 realms will be bootstrapped:
a. `realm1` internal auth
b. `realm2` external auth
c; `realm3` mixed auth
5. Obtain a token from Keycloak:
```shell
curl -v http://localhost:8080/realms/master/protocol/openid-connect/token \
-d client_id=client1 \
-d client_secret=s3cr3t \
-d grant_type=client_credentials
```
7. Obtain a token from Polaris (realm1 or realm3):
```shell
curl -v http://localhost:8181/api/catalog/v1/oauth/tokens \
-H "Polaris-Realm: realm1" \
-d client_id=root \
-d client_secret=secret \
-d grant_type=client_credentials \
-d scope=PRINCIPAL_ROLE:ALL
```
8. Using the token:
```shell
curl -v http://localhost:8181/api/catalog/v1/config\?warehouse\=default \
-H "Polaris-Realm: realm1" \
-H "Authorization: Bearer $token"
```
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2050627200
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultActiveRolesProvider.java:
##########
@@ -82,7 +82,9 @@ protected List<PrincipalRoleEntity> loadActivePrincipalRoles(
principal.getId());
throw new NotAuthorizedException("Unable to authenticate");
}
- boolean allRoles = tokenRoles.contains(BasePolarisAuthenticator.PRINCIPAL_ROLE_ALL);
+
+ // By convention, an empty set means all roles are activated
+ boolean allRoles = tokenRoles.isEmpty();
Review Comment:
As far as I understand this logic, I believe that the old code: `tokenRoles.contains(BasePolarisAuthenticator.PRINCIPAL_ROLE_ALL)` wasn't correct, because it is impossible that `tokenRoles` would contain `"PRINCIPAL_ROLE:ALL"`, since when this pseudo-role is present in the token, this translates into an empty 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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2051595737
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -88,17 +92,19 @@ protected Optional<AuthenticatedPolarisPrincipal> getPrincipal(DecodedToken toke
.log("Unable to authenticate user with token");
throw new ServiceFailureException("Unable to fetch principal entity");
}
- if (principal == null) {
- LOGGER.warn(
- "Failed to resolve principal from tokenInfo client_id={}", tokenInfo.getClientId());
+ if (principal == null || principal.getType() != PolarisEntityType.PRINCIPAL) {
+ LOGGER.warn("Failed to resolve principal from credentials={}", credentials);
throw new NotAuthorizedException("Unable to authenticate");
}
+ LOGGER.debug("Resolved principal: {}", principal);
+
+ boolean allRoles = credentials.getPrincipalRoles().contains(PRINCIPAL_ROLE_ALL);
+
Set<String> activatedPrincipalRoles = new HashSet<>();
- // TODO: Consolidate the divergent "scopes" logic between test-bearer-token and token-exchange.
- if (tokenInfo.getScope() != null && !tokenInfo.getScope().equals(PRINCIPAL_ROLE_ALL)) {
+ if (!allRoles) {
activatedPrincipalRoles.addAll(
- Arrays.stream(tokenInfo.getScope().split(" "))
+ credentials.getPrincipalRoles().stream()
.map(
s -> // strip the principal_role prefix, if present
s.startsWith(PRINCIPAL_ROLE_PREFIX)
Review Comment:
I believe this is dangerous and should be changed. For example, if the roles were initially:
`["PRINCIPAL_ROLE:catalog-admin', "service-admin"]`, this would be transformed into: `["catalog-admin', "service-admin"]`, potentially requesting the `service-admin` role, but that likely wasn't intentional.
I think it would be safer to discard all roles that don't have the `PRINCIPAL_ROLE:` prefix.
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2051740383
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthenticatedPolarisPrincipal.java:
##########
@@ -56,10 +72,20 @@ public List<PrincipalRoleEntity> getActivatedPrincipalRoles() {
return activatedPrincipalRoles;
}
+ /** FIXME this method makes this class mutable, which seems risky */
public void setActivatedPrincipalRoles(List<PrincipalRoleEntity> activatedPrincipalRoles) {
this.activatedPrincipalRoles = activatedPrincipalRoles;
}
+ /**
+ * Whether all roles should be activated. This is true when the principal is requesting all
+ * available principal roles. When this is true, {@link #getActivatedPrincipalRoleNames()} returns
+ * an empty set.
+ */
+ public boolean allRoles() {
Review Comment:
But a better solution imho would be to remove roles from this class completely, and let `SecurityIdentity.getRoles()` expose the roles.
I checked the call sites for `getActivatedPrincipalRoles()` and it's in fact never used. So it seems that `setActivatedPrincipalRoles()` could be removed 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 identity providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060373660
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/internal/InternalAuthenticationMechanism.java:
##########
@@ -56,9 +84,34 @@ public Uni<SecurityIdentity> authenticate(
}
String credential = authHeader.substring(spaceIdx + 1);
+
+ DecodedToken token;
+ try {
+ token = decodeToken(credential);
+ } catch (Exception e) {
+ return configuration.type() == AuthenticationType.MIXED
Review Comment:
Performance-wise current approach is probably easiest and fastest. However, `InternalIdentityProvider` looks like a no-op in the current state of the code, which looks a bit odd to me. Also, the `if` in exception handling seems to be coming exactly from the uncertainty of the token's origin.
If we parsed the token first, we could delegate to the right validation code and then, if an validation fails it will always be a certain failure.
Regarding parsing penalty, if the token is external, the first attempt will parse _and_ cause an exception on all call, which would be bigger overhead for the lower priority authenticators, I guess.
--
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 identity providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060705560
##########
service/common/src/main/java/org/apache/polaris/service/auth/PrincipalCredential.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.service.auth;
+
+import jakarta.annotation.Nullable;
+import java.util.Set;
+
+/**
+ * Principal information extracted from authentication data (typically, an access token) by the
+ * configured authentication mechanism. Used to determine the principal id, name, and roles while
+ * authenticating a request.
+ *
+ * @see DefaultAuthenticator
+ */
+public interface PrincipalCredential {
Review Comment:
Hmmm, but this interface does not have anything that requires it to be backed by JWT and the methods are only informative, they do not provide any "proof" of identity :thinking:
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2063830022
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/internal/InternalAuthenticationMechanism.java:
##########
@@ -56,9 +84,34 @@ public Uni<SecurityIdentity> authenticate(
}
String credential = authHeader.substring(spaceIdx + 1);
+
+ DecodedToken token;
+ try {
+ token = decodeToken(credential);
+ } catch (Exception e) {
+ return configuration.type() == AuthenticationType.MIXED
Review Comment:
I was able to achieve something that I think will improve the efficiency of token decoding:
https://github.com/adutra/polaris/pull/4
Let me know if I should merge those changes into this PR or wait until this one is merged.
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2070227247
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/external/OidcTenantResolvingAugmentor.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.service.quarkus.auth.external;
+
+import io.quarkus.security.identity.AuthenticationRequestContext;
+import io.quarkus.security.identity.SecurityIdentity;
+import io.quarkus.security.identity.SecurityIdentityAugmentor;
+import io.quarkus.security.runtime.QuarkusSecurityIdentity;
+import io.smallrye.mutiny.Uni;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.inject.Inject;
+import org.apache.polaris.service.quarkus.auth.external.tenant.OidcTenantResolver;
+import org.eclipse.microprofile.jwt.JsonWebToken;
+
+/**
+ * A {@link SecurityIdentityAugmentor} that resolves the Polaris OIDC tenant configuration for the
+ * given identity and adds it as an attribute to the {@link SecurityIdentity}.
+ */
+@ApplicationScoped
+public class OidcTenantResolvingAugmentor implements SecurityIdentityAugmentor {
Review Comment:
This one specifically is referenced outside the package, but OK for the others 👍
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2050623306
##########
service/common/src/main/java/org/apache/polaris/service/auth/Authenticator.java:
##########
@@ -20,8 +20,29 @@
import java.security.Principal;
import java.util.Optional;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+/**
+ * An interface for authenticating principals based on provided credentials.
+ *
+ * @param <C> the type of credentials used for authentication
+ * @param <P> the type of principal that is returned upon successful authentication
+ */
public interface Authenticator<C, P extends Principal> {
- Optional<P> authenticate(C credentials);
+ /**
+ * Authenticates the given credentials and returns an optional principal.
+ *
+ * <p>If the credentials are not valid or if the authentication fails, implementations may choose
Review Comment:
Again trying to document things a posteriori.
I don't really see the value of returning an `Optional` here, as the semantics of returning "empty" conflict with that of throwing an error. But I didn't want to change the method signature without knowing what others think.
--
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 identity providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2069474369
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/internal/InternalAuthenticationMechanism.java:
##########
@@ -56,9 +84,34 @@ public Uni<SecurityIdentity> authenticate(
}
String credential = authHeader.substring(spaceIdx + 1);
+
+ DecodedToken token;
+ try {
+ token = decodeToken(credential);
+ } catch (Exception e) {
+ return configuration.type() == AuthenticationType.MIXED
Review Comment:
Let's do that separately.. This PR is already big and reviewed :)
--
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 identity providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #1397:
URL: https://github.com/apache/polaris/pull/1397#issuecomment-2843341258
@collado-mike : any more comments from your side? :)
--
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 identity providers [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2056663577
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -88,17 +92,19 @@ protected Optional<AuthenticatedPolarisPrincipal> getPrincipal(DecodedToken toke
.log("Unable to authenticate user with token");
throw new ServiceFailureException("Unable to fetch principal entity");
}
- if (principal == null) {
- LOGGER.warn(
- "Failed to resolve principal from tokenInfo client_id={}", tokenInfo.getClientId());
+ if (principal == null || principal.getType() != PolarisEntityType.PRINCIPAL) {
+ LOGGER.warn("Failed to resolve principal from credentials={}", credentials);
throw new NotAuthorizedException("Unable to authenticate");
}
+ LOGGER.debug("Resolved principal: {}", principal);
+
+ boolean allRoles = credentials.getPrincipalRoles().contains(PRINCIPAL_ROLE_ALL);
+
Set<String> activatedPrincipalRoles = new HashSet<>();
- // TODO: Consolidate the divergent "scopes" logic between test-bearer-token and token-exchange.
- if (tokenInfo.getScope() != null && !tokenInfo.getScope().equals(PRINCIPAL_ROLE_ALL)) {
+ if (!allRoles) {
activatedPrincipalRoles.addAll(
- Arrays.stream(tokenInfo.getScope().split(" "))
+ credentials.getPrincipalRoles().stream()
.map(
s -> // strip the principal_role prefix, if present
s.startsWith(PRINCIPAL_ROLE_PREFIX)
Review Comment:
Agree, this should be `stream().filter(s -> s.startsWith(PRINCIPAL_ROLE_PREFIX)).map(...)`
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2051770521
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthenticatedPolarisPrincipal.java:
##########
@@ -56,10 +72,20 @@ public List<PrincipalRoleEntity> getActivatedPrincipalRoles() {
return activatedPrincipalRoles;
}
+ /** FIXME this method makes this class mutable, which seems risky */
public void setActivatedPrincipalRoles(List<PrincipalRoleEntity> activatedPrincipalRoles) {
this.activatedPrincipalRoles = activatedPrincipalRoles;
}
+ /**
+ * Whether all roles should be activated. This is true when the principal is requesting all
+ * available principal roles. When this is true, {@link #getActivatedPrincipalRoleNames()} returns
+ * an empty set.
+ */
+ public boolean allRoles() {
Review Comment:
I removed the method because it doesn't work well with the `Resolver`. TLDR we can't distinguish "no roles" from "all roles" for now.
--
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 identity providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060698739
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/internal/InternalAuthenticationMechanism.java:
##########
@@ -56,9 +84,34 @@ public Uni<SecurityIdentity> authenticate(
}
String credential = authHeader.substring(spaceIdx + 1);
+
+ DecodedToken token;
+ try {
+ token = decodeToken(credential);
+ } catch (Exception e) {
+ return configuration.type() == AuthenticationType.MIXED
Review Comment:
I was thinking that we could parse without validation in `InternalAuthenticationMechanism`, check a Polaris-specific property and trust it to choose the internal validation logic. Then, `InternalIdentityProvider` would finish the validation by checking the signature.
In any case, I think it makes sense to defer 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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060288770
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultActiveRolesProvider.java:
##########
@@ -82,7 +82,9 @@ protected List<PrincipalRoleEntity> loadActivePrincipalRoles(
principal.getId());
throw new NotAuthorizedException("Unable to authenticate");
}
- boolean allRoles = tokenRoles.contains(BasePolarisAuthenticator.PRINCIPAL_ROLE_ALL);
+
+ // FIXME how to distinguish allRoles from no roles at all?
Review Comment:
But that's the issue, if no roles are found in the token, Polaris will assume that the token has ALL of the principal roles available at the time of the request. That's imho concerning.
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2050608447
##########
quarkus/defaults/src/main/resources/application.properties:
##########
@@ -74,6 +75,20 @@ quarkus.log.category."io.smallrye.config".level=INFO
quarkus.management.port=8182
quarkus.management.test-port=0
+# OIDC settings. These settings are required only when using external authentication providers.
+# See https://quarkus.io/guides/security-oidc-configuration-properties-reference
+quarkus.oidc.application-type=service
+quarkus.oidc.resolve-tenants-with-issuer=true
+# Default tenant (disabled by default, set this to true if you use external authentication)
+quarkus.oidc.tenant-enabled=false
Review Comment:
We could enable this conditionally with a `ConfigSourceInterceptor` that would check if any realm is using external or mixed auth, in which case it makes sense to enable the default OIDC tenant, especially if no other tenant is configured. But I'm leaving this as a follow-up improvement.
--
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 identity providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2056964644
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/ProductionReadinessChecks.java:
##########
@@ -109,43 +93,80 @@ public ProductionReadinessCheck checkTokenService(IcebergRestOAuth2ApiService se
}
@Produces
- public ProductionReadinessCheck checkTokenBroker(
- AuthenticationConfiguration configuration, TokenBrokerFactory factory) {
- if (factory instanceof JWTRSAKeyPairFactory) {
- if (configuration
- .tokenBroker()
- .rsaKeyPair()
- .map(RSAKeyPairConfiguration::publicKeyFile)
- .isEmpty()) {
- return ProductionReadinessCheck.of(
- Error.of(
- "A public key file wasn't provided and will be generated.",
- "polaris.authentication.token-broker.rsa-key-pair.public-key-file"));
- }
- if (configuration
- .tokenBroker()
- .rsaKeyPair()
- .map(RSAKeyPairConfiguration::privateKeyFile)
- .isEmpty()) {
- return ProductionReadinessCheck.of(
- Error.of(
- "A private key file wasn't provided and will be generated.",
- "polaris.authentication.token-broker.rsa-key-pair.private-key-file"));
- }
- }
- if (factory instanceof JWTSymmetricKeyFactory) {
- if (configuration
- .tokenBroker()
- .symmetricKey()
- .map(SymmetricKeyConfiguration::secret)
- .isPresent()) {
- return ProductionReadinessCheck.of(
- Error.of(
- "A symmetric key secret was provided through configuration rather than through a secret file.",
- "polaris.authentication.token-broker.symmetric-key.secret"));
- }
- }
- return ProductionReadinessCheck.OK;
+ public ProductionReadinessCheck checkAuthenticationType(
+ AuthenticationConfiguration configuration) {
+ List<ProductionReadinessCheck.Error> errors = new ArrayList<>();
+ configuration
+ .realms()
+ .forEach(
+ (realm, config) -> {
+ AuthenticationType authenticationType = config.type();
+ if (authenticationType == AuthenticationType.INTERNAL
+ || authenticationType == AuthenticationType.MIXED) {
+ errors.add(
+ Error.of(
+ "Internal authentication is deprecated since Iceberg 1.6.0.",
Review Comment:
While the statement is true, Polaris still supports the `/token` endpoint. Should we really flag this as a problem for production deployments? Resolving this requires using Iceberg 1.9 clients plus an Auth Manager implementation, which may be too much of a burden to roll out ATM.
##########
service/common/src/main/java/org/apache/polaris/service/auth/AuthenticationConfiguration.java:
##########
@@ -18,60 +18,22 @@
*/
package org.apache.polaris.service.auth;
-import java.nio.file.Path;
-import java.time.Duration;
-import java.util.Optional;
+import java.util.Map;
+import org.apache.polaris.core.context.RealmContext;
public interface AuthenticationConfiguration {
- /** The configuration for the authenticator. */
- AuthenticatorConfiguration authenticator();
+ String DEFAULT_REALM_KEY = "<default>";
- interface AuthenticatorConfiguration {}
+ Map<String, ? extends AuthenticationRealmConfiguration> realms();
Review Comment:
maybe make `?` a declared type parameter to avoid type casts in sub-types?
##########
service/common/src/main/java/org/apache/polaris/service/auth/Authenticator.java:
##########
@@ -20,8 +20,29 @@
import java.security.Principal;
import java.util.Optional;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
+/**
+ * An interface for authenticating principals based on provided credentials.
+ *
+ * @param <C> the type of credentials used for authentication
+ * @param <P> the type of principal that is returned upon successful authentication
+ */
public interface Authenticator<C, P extends Principal> {
- Optional<P> authenticate(C credentials);
+ /**
+ * Authenticates the given credentials and returns an optional principal.
+ *
+ * <p>If the credentials are not valid or if the authentication fails, implementations may choose
Review Comment:
I tend to prefer exceptions here (for follow-up)
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -88,17 +92,19 @@ protected Optional<AuthenticatedPolarisPrincipal> getPrincipal(DecodedToken toke
.log("Unable to authenticate user with token");
throw new ServiceFailureException("Unable to fetch principal entity");
}
- if (principal == null) {
- LOGGER.warn(
- "Failed to resolve principal from tokenInfo client_id={}", tokenInfo.getClientId());
+ if (principal == null || principal.getType() != PolarisEntityType.PRINCIPAL) {
+ LOGGER.warn("Failed to resolve principal from credentials={}", credentials);
throw new NotAuthorizedException("Unable to authenticate");
}
+ LOGGER.debug("Resolved principal: {}", principal);
+
+ boolean allRoles = credentials.getPrincipalRoles().contains(PRINCIPAL_ROLE_ALL);
+
Set<String> activatedPrincipalRoles = new HashSet<>();
- // TODO: Consolidate the divergent "scopes" logic between test-bearer-token and token-exchange.
- if (tokenInfo.getScope() != null && !tokenInfo.getScope().equals(PRINCIPAL_ROLE_ALL)) {
+ if (!allRoles) {
activatedPrincipalRoles.addAll(
- Arrays.stream(tokenInfo.getScope().split(" "))
+ credentials.getPrincipalRoles().stream()
.map(
s -> // strip the principal_role prefix, if present
s.startsWith(PRINCIPAL_ROLE_PREFIX)
Review Comment:
+1 to filter
##########
service/common/src/main/java/org/apache/polaris/service/auth/DefaultActiveRolesProvider.java:
##########
@@ -82,7 +82,9 @@ protected List<PrincipalRoleEntity> loadActivePrincipalRoles(
principal.getId());
throw new NotAuthorizedException("Unable to authenticate");
}
- boolean allRoles = tokenRoles.contains(BasePolarisAuthenticator.PRINCIPAL_ROLE_ALL);
+
+ // FIXME how to distinguish allRoles from no roles at all?
Review Comment:
I believe this is a non-issue. My understanding is that tokens need to support restricting to specific roles. Restricting to no roles at all is hardly useful in Polaris since Principals cannot have access rights granted directly to them.
##########
service/common/src/main/java/org/apache/polaris/service/auth/PrincipalCredential.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.service.auth;
+
+import jakarta.annotation.Nullable;
+import java.util.Set;
+
+/**
+ * Principal information extracted from authentication data (typically, an access token) by the
+ * configured authentication mechanism. Used to determine the principal id, name, and roles while
+ * authenticating a request.
+ *
+ * @see DefaultAuthenticator
+ */
+public interface PrincipalCredential {
Review Comment:
What makes this class a "Credential"? How about `PrincipalAuthInfo`?
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/internal/InternalAuthenticationMechanism.java:
##########
@@ -56,9 +84,34 @@ public Uni<SecurityIdentity> authenticate(
}
String credential = authHeader.substring(spaceIdx + 1);
+
+ DecodedToken token;
+ try {
+ token = decodeToken(credential);
+ } catch (Exception e) {
+ return configuration.type() == AuthenticationType.MIXED
Review Comment:
Would it be more correct to only parse the token here and delegate validation to `InternalIdentityProvider`?
With that approach, I guess we will not have condition failure logic... All nuance should be handled by Quarkus, WDYT?
We could have a Polaris-specific JWT claim to identify Polaris tokens, I guess.
--
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 identity providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060698739
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/internal/InternalAuthenticationMechanism.java:
##########
@@ -56,9 +84,34 @@ public Uni<SecurityIdentity> authenticate(
}
String credential = authHeader.substring(spaceIdx + 1);
+
+ DecodedToken token;
+ try {
+ token = decodeToken(credential);
+ } catch (Exception e) {
+ return configuration.type() == AuthenticationType.MIXED
Review Comment:
I was thinking that we could parse without validation in `InternalAuthenticationMechanism`, check a Polaris-specific property and trust it to choose the internal validation logic. Then, `InternalIdentityProvider` would finish the validation by checking the signature. If the internal property is not present, validation will be done by OIDC.
In any case, I think it makes sense to defer 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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060412035
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/internal/InternalAuthenticationMechanism.java:
##########
@@ -56,9 +84,34 @@ public Uni<SecurityIdentity> authenticate(
}
String credential = authHeader.substring(spaceIdx + 1);
+
+ DecodedToken token;
+ try {
+ token = decodeToken(credential);
+ } catch (Exception e) {
+ return configuration.type() == AuthenticationType.MIXED
Review Comment:
> However, InternalIdentityProvider looks like a no-op in the current state of the code, which looks a bit odd to me
That is very true, but is intentional: we _could_ short-circuit in `InternalAuthenticationMechanism` and return a `SecurityIdentity` straight away, rather than invoking ` identityProviderManager.authenticate()`. In that case, an `IdentityProvider` is not necessary.
BUT: when the identity provider manager is not invoked, no augmentors are invoked either. I discovered this while testing.
So the TLDR is: we need `InternalIdentityProvider`, even if it's a passthrough impl for now, because there are augmentors that must be invoked.
> Regarding parsing penalty, if the token is external, the first attempt will parse and cause an exception on all call, which would be bigger overhead for the lower priority authenticators, I guess.
True. Let me try to introduce the new `TokenBroker` method and see if that looks better.
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060635867
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/internal/InternalAuthenticationMechanism.java:
##########
@@ -56,9 +84,34 @@ public Uni<SecurityIdentity> authenticate(
}
String credential = authHeader.substring(spaceIdx + 1);
+
+ DecodedToken token;
+ try {
+ token = decodeToken(credential);
+ } catch (Exception e) {
+ return configuration.type() == AuthenticationType.MIXED
Review Comment:
After playing with this a bit, I'd be in favor of leaving this for a follow-up PR. The changes to the `TokenBroker` interface become a bit invasive imho.
I was trying something like this:
```java
interface TokenBroker {
DecodedToken decode(String token); // done in InternalAuthenticationMechanism
void verify(DecodedToken token); // done in InternalIdentityProvider
...
}
```
But:
1. `decode` would still throw an exception on every request with an external token;
2. We'd need to leak some auth0 types in the `DecodedToken` interface in order to avoid re-parsing the token. e.g. `DecodedJWT getAuth0Token();`
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060326755
##########
service/common/src/main/java/org/apache/polaris/service/auth/PrincipalCredential.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.service.auth;
+
+import jakarta.annotation.Nullable;
+import java.util.Set;
+
+/**
+ * Principal information extracted from authentication data (typically, an access token) by the
+ * configured authentication mechanism. Used to determine the principal id, name, and roles while
+ * authenticating a request.
+ *
+ * @see DefaultAuthenticator
+ */
+public interface PrincipalCredential {
Review Comment:
From what I inferred from Quarkus Security parlance, a "principal" defines an identity, but a "credential" is any concrete form of principal identity proof, and could be: a string, a JWT token, a TLS certificate, etc. So "credential" seemed appropriate here since this is basically a view of a JWT token.
But OK to change to `PrincipalAuthInfo` 👍
--
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 identity providers [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2070234629
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/ActiveRolesAugmentor.java:
##########
@@ -32,33 +32,42 @@
/**
* A custom {@link SecurityIdentityAugmentor} that adds active roles to the {@link
- * SecurityIdentity}. This is used to augment the identity with active roles after authentication.
+ * SecurityIdentity}. This is used to augment the identity with valid active roles after
+ * authentication.
*/
@ApplicationScoped
public class ActiveRolesAugmentor implements SecurityIdentityAugmentor {
+ // must run after AuthenticatingAugmentor
+ public static final int PRIORITY = AuthenticatingAugmentor.PRIORITY - 1;
+
@Inject ActiveRolesProvider activeRolesProvider;
+ @Override
+ public int priority() {
+ return PRIORITY;
+ }
+
@Override
public Uni<SecurityIdentity> augment(
SecurityIdentity identity, AuthenticationRequestContext context) {
if (identity.isAnonymous()) {
return Uni.createFrom().item(identity);
}
- return context.runBlocking(() -> augmentWithActiveRoles(identity));
+ return context.runBlocking(() -> validateActiveRoles(identity));
}
- private SecurityIdentity augmentWithActiveRoles(SecurityIdentity identity) {
- AuthenticatedPolarisPrincipal polarisPrincipal =
- identity.getPrincipal(AuthenticatedPolarisPrincipal.class);
- if (polarisPrincipal == null) {
+ private SecurityIdentity validateActiveRoles(SecurityIdentity identity) {
+ if (!(identity.getPrincipal() instanceof AuthenticatedPolarisPrincipal)) {
throw new AuthenticationFailedException("No Polaris principal found");
}
+ AuthenticatedPolarisPrincipal polarisPrincipal =
+ identity.getPrincipal(AuthenticatedPolarisPrincipal.class);
Set<String> validRoleNames = activeRolesProvider.getActiveRoles(polarisPrincipal);
Review Comment:
We also need a more robust story around roles mapping. For now, most of the work is done by `PrincipalRolesMapper`, but some further processing is being done in `DefaultAuthenticator`, for historical reasons. And `AuthenticatedPolarisPrincipal` wouldn't need to expose roles anymore. But again I'm leaving that for later, as this PR is already big.
--
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