From 46c764798be92286ce18b6f0260db0dbe236e7af Mon Sep 17 00:00:00 2001 From: Robert Stupp Date: Sun, 8 Dec 2024 10:18:36 +0100 Subject: [PATCH] Move secret validation logic and authN-loadPrincipal into `PolarisSecretsManager` The logic _how_ a principal and/or principal secret's are persisted should be transparent to the calling code. Relying on the persistence internals for principals and secrets management makes it impossible to factor out secrets management / make principal management possible. This change moves the secret validation and retrieval of a principal by client-ID behind an implementation of `PolarisSecretsManager`. --- .../PolarisServiceImplIntegrationTest.java | 5 +- .../service/auth/JWTRSAKeyPairTest.java | 11 +--- .../auth/JWTSymmetricKeyGeneratorTest.java | 11 +--- .../core/auth/PolarisSecretsManager.java | 43 ++++++++++++++ .../polaris/core/persistence/BaseResult.java | 2 + .../persistence/PolarisEntityManager.java | 2 +- .../PolarisMetaStoreManagerImpl.java | 58 +++++++++++++++++++ .../TransactionWorkspaceMetaStoreManager.java | 18 ++++++ .../auth/BasePolarisAuthenticator.java | 19 ++---- .../polaris/service/auth/TokenBroker.java | 20 ++----- 10 files changed, 143 insertions(+), 46 deletions(-) diff --git a/dropwizard/service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java b/dropwizard/service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java index 5f4830a55..74c34adcd 100644 --- a/dropwizard/service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java +++ b/dropwizard/service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java @@ -2355,7 +2355,10 @@ public void testTokenInvalidSignature() { @Test public void testTokenInvalidPrincipalId() { String newToken = - defaultJwt().withClaim(CLAIM_KEY_PRINCIPAL_ID, 0).sign(Algorithm.HMAC256("polaris")); + defaultJwt() + .withClaim(CLAIM_KEY_PRINCIPAL_ID, 0) + .withClaim(CLAIM_KEY_CLIENT_ID, "foo") + .sign(Algorithm.HMAC256("polaris")); try (Response response = newRequest("http://localhost:%d/api/management/v1/principals", newToken).get()) { assertThat(response) diff --git a/dropwizard/service/src/test/java/org/apache/polaris/service/auth/JWTRSAKeyPairTest.java b/dropwizard/service/src/test/java/org/apache/polaris/service/auth/JWTRSAKeyPairTest.java index 3cd8b925d..ddbef57d2 100644 --- a/dropwizard/service/src/test/java/org/apache/polaris/service/auth/JWTRSAKeyPairTest.java +++ b/dropwizard/service/src/test/java/org/apache/polaris/service/auth/JWTRSAKeyPairTest.java @@ -36,13 +36,12 @@ import java.util.HashMap; import java.util.Map; import org.apache.polaris.core.PolarisCallContext; -import org.apache.polaris.core.auth.PolarisSecretsManager.PrincipalSecretsResult; +import org.apache.polaris.core.auth.PolarisSecretsManager; import org.apache.polaris.core.context.CallContext; import org.apache.polaris.core.context.RealmContext; import org.apache.polaris.core.entity.PolarisBaseEntity; import org.apache.polaris.core.entity.PolarisEntitySubType; import org.apache.polaris.core.entity.PolarisEntityType; -import org.apache.polaris.core.entity.PolarisPrincipalSecrets; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.service.config.DefaultConfigurationStore; import org.junit.jupiter.api.Test; @@ -116,10 +115,6 @@ public void testSuccessfulTokenGeneration() throws Exception { CallContext.setCurrentContext(getTestCallContext(polarisCallContext)); PolarisMetaStoreManager metastoreManager = Mockito.mock(PolarisMetaStoreManager.class); String mainSecret = "client-secret"; - PolarisPrincipalSecrets principalSecrets = - new PolarisPrincipalSecrets(1L, clientId, mainSecret, "otherSecret"); - Mockito.when(metastoreManager.loadPrincipalSecrets(polarisCallContext, clientId)) - .thenReturn(new PrincipalSecretsResult(principalSecrets)); PolarisBaseEntity principal = new PolarisBaseEntity( 0L, @@ -128,8 +123,8 @@ public void testSuccessfulTokenGeneration() throws Exception { PolarisEntitySubType.NULL_SUBTYPE, 0L, "principal"); - Mockito.when(metastoreManager.loadEntity(polarisCallContext, 0L, 1L)) - .thenReturn(new PolarisMetaStoreManager.EntityResult(principal)); + Mockito.when(metastoreManager.validateSecret(polarisCallContext, clientId, mainSecret)) + .thenReturn(new PolarisSecretsManager.SecretValidationResult(principal)); TokenBroker tokenBroker = new JWTRSAKeyPair(metastoreManager, 420); TokenResponse token = tokenBroker.generateFromClientSecrets( diff --git a/dropwizard/service/src/test/java/org/apache/polaris/service/auth/JWTSymmetricKeyGeneratorTest.java b/dropwizard/service/src/test/java/org/apache/polaris/service/auth/JWTSymmetricKeyGeneratorTest.java index db7a00fff..c64138363 100644 --- a/dropwizard/service/src/test/java/org/apache/polaris/service/auth/JWTSymmetricKeyGeneratorTest.java +++ b/dropwizard/service/src/test/java/org/apache/polaris/service/auth/JWTSymmetricKeyGeneratorTest.java @@ -26,13 +26,12 @@ import com.auth0.jwt.interfaces.DecodedJWT; import java.util.Map; import org.apache.polaris.core.PolarisCallContext; -import org.apache.polaris.core.auth.PolarisSecretsManager.PrincipalSecretsResult; +import org.apache.polaris.core.auth.PolarisSecretsManager; import org.apache.polaris.core.context.CallContext; import org.apache.polaris.core.context.RealmContext; import org.apache.polaris.core.entity.PolarisBaseEntity; import org.apache.polaris.core.entity.PolarisEntitySubType; import org.apache.polaris.core.entity.PolarisEntityType; -import org.apache.polaris.core.entity.PolarisPrincipalSecrets; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -63,10 +62,6 @@ public Map contextVariables() { PolarisMetaStoreManager metastoreManager = Mockito.mock(PolarisMetaStoreManager.class); String mainSecret = "test_secret"; String clientId = "test_client_id"; - PolarisPrincipalSecrets principalSecrets = - new PolarisPrincipalSecrets(1L, clientId, mainSecret, "otherSecret"); - Mockito.when(metastoreManager.loadPrincipalSecrets(polarisCallContext, clientId)) - .thenReturn(new PrincipalSecretsResult(principalSecrets)); PolarisBaseEntity principal = new PolarisBaseEntity( 0L, @@ -75,8 +70,8 @@ public Map contextVariables() { PolarisEntitySubType.NULL_SUBTYPE, 0L, "principal"); - Mockito.when(metastoreManager.loadEntity(polarisCallContext, 0L, 1L)) - .thenReturn(new PolarisMetaStoreManager.EntityResult(principal)); + Mockito.when(metastoreManager.validateSecret(polarisCallContext, clientId, mainSecret)) + .thenReturn(new PolarisSecretsManager.SecretValidationResult(principal)); TokenBroker generator = new JWTSymmetricKeyBroker(metastoreManager, 666, () -> "polaris"); TokenResponse token = generator.generateFromClientSecrets( diff --git a/polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecretsManager.java b/polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecretsManager.java index d63368fd9..2704dae24 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecretsManager.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecretsManager.java @@ -23,8 +23,10 @@ import jakarta.annotation.Nonnull; import jakarta.annotation.Nullable; import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.entity.PolarisBaseEntity; import org.apache.polaris.core.entity.PolarisPrincipalSecrets; import org.apache.polaris.core.persistence.BaseResult; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager.EntityResult; /** Manages secrets for Polaris principals. */ public interface PolarisSecretsManager { @@ -39,6 +41,17 @@ public interface PolarisSecretsManager { PrincipalSecretsResult loadPrincipalSecrets( @Nonnull PolarisCallContext callCtx, @Nonnull String clientId); + @Nonnull + SecretValidationResult validateSecret( + @Nonnull PolarisCallContext callCtx, @Nonnull String clientId, @Nonnull String clientSecret); + + @Nonnull + EntityResult loadPrincipal( + @Nonnull PolarisCallContext callCtx, + @Nullable String roleName, + @Nullable String clientId, + @Nullable Long principalId); + /** * Rotate secrets * @@ -100,4 +113,34 @@ public PolarisPrincipalSecrets getPrincipalSecrets() { return principalSecrets; } } + + /** the result of load/rotate principal secrets */ + class SecretValidationResult extends BaseResult { + + private final PolarisBaseEntity principal; + + public SecretValidationResult( + @Nonnull BaseResult.ReturnStatus errorCode, @Nullable String extraInformation) { + super(errorCode, extraInformation); + this.principal = null; + } + + public SecretValidationResult(@Nonnull PolarisBaseEntity principal) { + super(BaseResult.ReturnStatus.SUCCESS); + this.principal = principal; + } + + @JsonCreator + private SecretValidationResult( + @JsonProperty("returnStatus") @Nonnull BaseResult.ReturnStatus returnStatus, + @JsonProperty("extraInformation") @Nullable String extraInformation, + @JsonProperty("principalSecrets") @Nonnull PolarisBaseEntity principal) { + super(returnStatus, extraInformation); + this.principal = principal; + } + + public PolarisBaseEntity getPrincipal() { + return principal; + } + } } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/BaseResult.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/BaseResult.java index cea33071e..8fd3f4bc1 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/BaseResult.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/BaseResult.java @@ -111,6 +111,8 @@ public enum ReturnStatus { // error caught while sub-scoping credentials. Error message will be returned SUBSCOPE_CREDS_ERROR(13), + + SECRET_VALIDATION_FAILED(14), ; // code for the enum diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisEntityManager.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisEntityManager.java index 3d0d2457a..8b4255b0b 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisEntityManager.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisEntityManager.java @@ -70,7 +70,7 @@ public Resolver prepareResolver( callContext.getPolarisCallContext(), metaStoreManager, authenticatedPrincipal.getPrincipalEntity().getId(), - null, /* callerPrincipalName */ + authenticatedPrincipal.getPrincipalEntity().getName(), authenticatedPrincipal.getActivatedPrincipalRoleNames().isEmpty() ? null : authenticatedPrincipal.getActivatedPrincipalRoleNames(), diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java index b90c7e51c..b226d829a 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java @@ -18,6 +18,8 @@ */ package org.apache.polaris.core.persistence; +import static com.google.common.base.Preconditions.checkArgument; + import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.JsonMappingException; @@ -55,6 +57,7 @@ import org.apache.polaris.core.storage.PolarisStorageActions; import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; import org.apache.polaris.core.storage.PolarisStorageIntegration; +import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -1006,6 +1009,61 @@ public Map deserializeProperties(PolarisCallContext callCtx, Str : new PrincipalSecretsResult(secrets); } + @Override + public @NotNull SecretValidationResult validateSecret( + @NotNull PolarisCallContext callCtx, @NotNull String clientId, @NotNull String clientSecret) { + PrincipalSecretsResult principalSecrets = loadPrincipalSecrets(callCtx, clientId); + if (!principalSecrets.isSuccess() + || !principalSecrets.getPrincipalSecrets().matchesSecret(clientSecret)) { + return new SecretValidationResult(BaseResult.ReturnStatus.SECRET_VALIDATION_FAILED, ""); + } + PolarisMetaStoreManager.EntityResult result = + loadEntity(callCtx, 0L, principalSecrets.getPrincipalSecrets().getPrincipalId()); + if (!result.isSuccess() || result.getEntity().getType() != PolarisEntityType.PRINCIPAL) { + return new SecretValidationResult(BaseResult.ReturnStatus.SECRET_VALIDATION_FAILED, ""); + } + return new SecretValidationResult(result.getEntity()); + } + + @Override + public @NotNull EntityResult loadPrincipal( + @NotNull PolarisCallContext callCtx, + @Nullable String roleName, + @Nullable String clientId, + @Nullable Long principalId) { + checkArgument(principalId != null || clientId != null || roleName != null); + if (principalId != null && principalId > 0) { + EntityResult result = loadEntity(callCtx, 0L, principalId); + if (result.isSuccess() && result.getEntity().getType() == PolarisEntityType.PRINCIPAL) { + return result; + } + } + if (roleName != null) { + EntityResult result = + readEntityByName( + callCtx, + null, + PolarisEntityType.PRINCIPAL, + PolarisEntitySubType.NULL_SUBTYPE, + roleName); + if (result.isSuccess()) { + return result; + } + } + if (clientId != null) { + PrincipalSecretsResult principalSecrets = loadPrincipalSecrets(callCtx, clientId); + if (principalSecrets.isSuccess() + && principalSecrets.getPrincipalSecrets().getPrincipalId() > 0) { + EntityResult result = + loadEntity(callCtx, 0L, principalSecrets.getPrincipalSecrets().getPrincipalId()); + if (result.isSuccess() && result.getEntity().getType() == PolarisEntityType.PRINCIPAL) { + return result; + } + } + } + return new EntityResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, ""); + } + /** See {@link #} */ private @Nullable PolarisPrincipalSecrets rotatePrincipalSecrets( @Nonnull PolarisCallContext callCtx, diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java index 44134751b..bcdffbfe4 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java @@ -34,6 +34,7 @@ import org.apache.polaris.core.entity.PolarisEntityType; import org.apache.polaris.core.entity.PolarisPrivilege; import org.apache.polaris.core.storage.PolarisStorageActions; +import org.jetbrains.annotations.NotNull; /** * Wraps an existing impl of PolarisMetaStoreManager and delegates expected "read" operations @@ -129,6 +130,23 @@ public PrincipalSecretsResult loadPrincipalSecrets( return null; } + @Override + public @NotNull SecretValidationResult validateSecret( + @NotNull PolarisCallContext callCtx, @NotNull String clientId, @NotNull String clientSecret) { + callCtx.getDiagServices().fail("illegal_method_in_transaction_workspace", "validateSecret"); + return null; + } + + @Override + public @NotNull EntityResult loadPrincipal( + @NotNull PolarisCallContext callCtx, + @Nullable String roleName, + @Nullable String clientId, + @Nullable Long principalId) { + callCtx.getDiagServices().fail("illegal_method_in_transaction_workspace", "validateSecret"); + return null; + } + @Override public PrincipalSecretsResult rotatePrincipalSecrets( @Nonnull PolarisCallContext callCtx, diff --git a/service/common/src/main/java/org/apache/polaris/service/auth/BasePolarisAuthenticator.java b/service/common/src/main/java/org/apache/polaris/service/auth/BasePolarisAuthenticator.java index 28dd9e0ef..0be645843 100644 --- a/service/common/src/main/java/org/apache/polaris/service/auth/BasePolarisAuthenticator.java +++ b/service/common/src/main/java/org/apache/polaris/service/auth/BasePolarisAuthenticator.java @@ -30,8 +30,6 @@ import org.apache.polaris.core.context.CallContext; import org.apache.polaris.core.context.RealmContext; import org.apache.polaris.core.entity.PolarisEntity; -import org.apache.polaris.core.entity.PolarisEntitySubType; -import org.apache.polaris.core.entity.PolarisEntityType; import org.apache.polaris.core.entity.PrincipalEntity; import org.apache.polaris.core.persistence.MetaStoreManagerFactory; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; @@ -65,17 +63,12 @@ protected Optional getPrincipal(DecodedToken toke PolarisEntity principal; try { principal = - tokenInfo.getPrincipalId() > 0 - ? PolarisEntity.of( - metaStoreManager.loadEntity( - getCurrentPolarisContext(), 0L, tokenInfo.getPrincipalId())) - : PolarisEntity.of( - metaStoreManager.readEntityByName( - getCurrentPolarisContext(), - null, - PolarisEntityType.PRINCIPAL, - PolarisEntitySubType.NULL_SUBTYPE, - tokenInfo.getSub())); + PolarisEntity.of( + metaStoreManager.loadPrincipal( + getCurrentPolarisContext(), + tokenInfo.getSub(), + tokenInfo.getClientId(), + tokenInfo.getPrincipalId())); } catch (Exception e) { LOGGER .atError() diff --git a/service/common/src/main/java/org/apache/polaris/service/auth/TokenBroker.java b/service/common/src/main/java/org/apache/polaris/service/auth/TokenBroker.java index 190a21e54..8fa43f352 100644 --- a/service/common/src/main/java/org/apache/polaris/service/auth/TokenBroker.java +++ b/service/common/src/main/java/org/apache/polaris/service/auth/TokenBroker.java @@ -21,9 +21,8 @@ import jakarta.annotation.Nonnull; import java.util.Optional; import org.apache.polaris.core.PolarisCallContext; -import org.apache.polaris.core.auth.PolarisSecretsManager.PrincipalSecretsResult; +import org.apache.polaris.core.auth.PolarisSecretsManager; import org.apache.polaris.core.context.CallContext; -import org.apache.polaris.core.entity.PolarisEntityType; import org.apache.polaris.core.entity.PrincipalEntity; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.service.types.TokenType; @@ -47,20 +46,11 @@ TokenResponse generateFromToken( PolarisMetaStoreManager metaStoreManager, String clientId, String clientSecret) { // Validate the principal is present and secrets match PolarisCallContext polarisCallContext = CallContext.getCurrentContext().getPolarisCallContext(); - PrincipalSecretsResult principalSecrets = - metaStoreManager.loadPrincipalSecrets(polarisCallContext, clientId); - if (!principalSecrets.isSuccess()) { + PolarisSecretsManager.SecretValidationResult result = + metaStoreManager.validateSecret(polarisCallContext, clientId, clientSecret); + if (!result.isSuccess()) { return Optional.empty(); } - if (!principalSecrets.getPrincipalSecrets().matchesSecret(clientSecret)) { - return Optional.empty(); - } - PolarisMetaStoreManager.EntityResult result = - metaStoreManager.loadEntity( - polarisCallContext, 0L, principalSecrets.getPrincipalSecrets().getPrincipalId()); - if (!result.isSuccess() || result.getEntity().getType() != PolarisEntityType.PRINCIPAL) { - return Optional.empty(); - } - return Optional.of(PrincipalEntity.of(result.getEntity())); + return Optional.of(PrincipalEntity.of(result.getPrincipal())); } }