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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
