From 94f7a037eda52f3cd3f571b9b7b8ff54d15fcd03 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 14:52:24 -0800 Subject: [PATCH 01/13] initial commit --- ...olarisEclipseLinkMetaStoreSessionImpl.java | 4 +- .../core/entity/PolarisPrincipalSecrets.java | 81 +++++++++++++++++-- .../PolarisMetaStoreManagerImpl.java | 2 +- .../PolarisTreeMapMetaStoreSessionImpl.java | 4 +- .../models/ModelPrincipalSecrets.java | 51 ++++++++++-- .../polaris/service/auth/TokenBroker.java | 3 +- 6 files changed, 128 insertions(+), 17 deletions(-) diff --git a/extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java b/extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java index 071d7fbc39..ffe2d9e717 100644 --- a/extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java +++ b/extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java @@ -701,9 +701,9 @@ public int lookupEntityGrantRecordsVersion( principalSecrets.getPrincipalId()); // rotate the secrets - principalSecrets.rotateSecrets(mainSecretToRotate); + principalSecrets.rotateSecrets(); if (reset) { - principalSecrets.rotateSecrets(principalSecrets.getMainSecret()); + principalSecrets.rotateSecrets(); } // write back new secrets diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java index 195c668f6e..13a1535c48 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java @@ -20,6 +20,8 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.commons.codec.digest.DigestUtils; + import java.security.SecureRandom; /** @@ -37,12 +39,20 @@ public class PolarisPrincipalSecrets { // the client id for that principal private final String principalClientId; - // the main secret for that principal + // the main secret hash for that principal private String mainSecret; // the secondary secret for that principal private String secondarySecret; + // Hash of mainSecret + private String mainSecretHash; + + // Hash of secondarySecret + private String secondarySecretHash; + + private String secretSalt; + /** * Generate a secure random string * @@ -64,16 +74,51 @@ private String generateRandomHexString(int stringLength) { return sb.toString(); } + private String hashSecret(String secret) { + return DigestUtils.sha256Hex(secret + ":" + secretSalt); + } + @JsonCreator public PolarisPrincipalSecrets( @JsonProperty("principalId") long principalId, @JsonProperty("principalClientId") String principalClientId, @JsonProperty("mainSecret") String mainSecret, - @JsonProperty("secondarySecret") String secondarySecret) { + @JsonProperty("secondarySecret") String secondarySecret, + @JsonProperty("secretSalt") String secretSalt, + @JsonProperty("mainSecretHash") String mainSecretHash, + @JsonProperty("secondarySecretHash") String secondarySecretHash) { + this.principalId = principalId; + this.principalClientId = principalClientId; + this.mainSecret = mainSecret; + this.secondarySecret = secondarySecret; + + this.secretSalt = secretSalt; + if (this.secretSalt == null) { + this.secretSalt = generateRandomHexString(16); + } + this.mainSecretHash = mainSecretHash; + if (this.mainSecretHash == null) { + this.mainSecretHash = hashSecret(mainSecret); + } + this.secondarySecretHash = secondarySecretHash; + if (this.secondarySecretHash == null) { + this.secondarySecretHash = hashSecret(secondarySecret); + } + } + + public PolarisPrincipalSecrets( + long principalId, + String principalClientId, + String mainSecret, + String secondarySecret) { this.principalId = principalId; this.principalClientId = principalClientId; this.mainSecret = mainSecret; this.secondarySecret = secondarySecret; + + this.secretSalt = generateRandomHexString(16); + this.mainSecretHash = hashSecret(mainSecret); + this.secondarySecretHash = hashSecret(secondarySecret); } public PolarisPrincipalSecrets(PolarisPrincipalSecrets principalSecrets) { @@ -81,6 +126,9 @@ public PolarisPrincipalSecrets(PolarisPrincipalSecrets principalSecrets) { this.principalClientId = principalSecrets.getPrincipalClientId(); this.mainSecret = principalSecrets.getMainSecret(); this.secondarySecret = principalSecrets.getSecondarySecret(); + this.secretSalt = principalSecrets.getSecondarySecret(); + this.mainSecretHash = principalSecrets.getMainSecretHash(); + this.secondarySecretHash = principalSecrets.getSecondarySecretHash(); } public PolarisPrincipalSecrets(long principalId) { @@ -88,16 +136,22 @@ public PolarisPrincipalSecrets(long principalId) { this.principalClientId = this.generateRandomHexString(16); this.mainSecret = this.generateRandomHexString(32); this.secondarySecret = this.generateRandomHexString(32); + + this.secretSalt = this.generateRandomHexString(16); + this.mainSecretHash = hashSecret(mainSecret); + this.secondarySecretHash = hashSecret(secondarySecret); } /** * Rotate the main secrets * - * @param mainSecretToRotate the main secrets to rotate */ - public void rotateSecrets(String mainSecretToRotate) { - this.secondarySecret = mainSecretToRotate; + public void rotateSecrets() { + this.secondarySecret = this.mainSecret; + this.secondarySecretHash = this.mainSecretHash; + this.mainSecret = this.generateRandomHexString(32); + this.mainSecretHash = hashSecret(mainSecret); } public long getPrincipalId() { @@ -108,6 +162,11 @@ public String getPrincipalClientId() { return principalClientId; } + public boolean matchesSecret(String potentialSecret) { + String potentialSecretHash = hashSecret(potentialSecret); + return potentialSecretHash.equals(this.mainSecretHash) || potentialSecretHash.equals(this.secondarySecretHash); + } + public String getMainSecret() { return mainSecret; } @@ -115,4 +174,16 @@ public String getMainSecret() { public String getSecondarySecret() { return secondarySecret; } + + public String getMainSecretHash() { + return mainSecretHash; + } + + public String getSecondarySecretHash() { + return secondarySecretHash; + } + + public String getSecretSalt() { + return secretSalt; + } } 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 b05129fa3d..f094ba412d 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 @@ -954,7 +954,7 @@ public Map deserializeProperties(PolarisCallContext callCtx, Str return new CreatePrincipalResult(ReturnStatus.ENTITY_ALREADY_EXISTS, null); } - // generate new secretes for this principal + // generate new secrets for this principal PolarisPrincipalSecrets principalSecrets = ms.generateNewPrincipalSecrets(callCtx, principal.getName(), principal.getId()); diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisTreeMapMetaStoreSessionImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisTreeMapMetaStoreSessionImpl.java index 096bce49eb..7859f13039 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisTreeMapMetaStoreSessionImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisTreeMapMetaStoreSessionImpl.java @@ -501,9 +501,9 @@ public int lookupEntityGrantRecordsVersion( principalSecrets.getPrincipalId()); // rotate the secrets - principalSecrets.rotateSecrets(mainSecretToRotate); + principalSecrets.rotateSecrets(); if (reset) { - principalSecrets.rotateSecrets(principalSecrets.getMainSecret()); + principalSecrets.rotateSecrets(); } // write back new secrets diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/models/ModelPrincipalSecrets.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/models/ModelPrincipalSecrets.java index 349d54395b..c4ebad6a60 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/models/ModelPrincipalSecrets.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/models/ModelPrincipalSecrets.java @@ -43,6 +43,14 @@ public class ModelPrincipalSecrets { // the secondary secret for that principal private String secondarySecret; + // Hash of mainSecret + private String mainSecretHash; + + // Hash of secondarySecret + private String secondarySecretHash; + + private String secretSalt; + // Used for Optimistic Locking to handle concurrent reads and updates @Version private long version; @@ -62,6 +70,18 @@ public String getSecondarySecret() { return secondarySecret; } + public String getSecretSalt() { + return secretSalt; + } + + public String getMainSecretHash() { + return mainSecretHash; + } + + public String getSecondarySecretHash() { + return secondarySecretHash; + } + public static Builder builder() { return new Builder(); } @@ -93,6 +113,21 @@ public Builder secondarySecret(String secondarySecret) { return this; } + public Builder secretSalt(String secretSalt) { + principalSecrets.secretSalt = secretSalt; + return this; + } + + public Builder mainSecretHash(String mainSecretHash) { + principalSecrets.mainSecretHash = mainSecretHash; + return this; + } + + public Builder secondarySecretHash(String secondarySecretHash) { + principalSecrets.secondarySecretHash = secondarySecretHash; + return this; + } + public ModelPrincipalSecrets build() { return principalSecrets; } @@ -104,8 +139,9 @@ public static ModelPrincipalSecrets fromPrincipalSecrets(PolarisPrincipalSecrets return ModelPrincipalSecrets.builder() .principalId(record.getPrincipalId()) .principalClientId(record.getPrincipalClientId()) - .mainSecret(record.getMainSecret()) - .secondarySecret(record.getSecondarySecret()) + .secretSalt(record.getSecretSalt()) + .mainSecretHash(record.getMainSecretHash()) + .secondarySecretHash(record.getSecondarySecretHash()) .build(); } @@ -116,7 +152,11 @@ public static PolarisPrincipalSecrets toPrincipalSecrets(ModelPrincipalSecrets m model.getPrincipalId(), model.getPrincipalClientId(), model.getMainSecret(), - model.getSecondarySecret()); + model.getSecondarySecret(), + model.getSecretSalt(), + model.getMainSecretHash(), + model.getSecondarySecretHash() + ); } public void update(PolarisPrincipalSecrets principalSecrets) { @@ -124,7 +164,8 @@ public void update(PolarisPrincipalSecrets principalSecrets) { this.principalId = principalSecrets.getPrincipalId(); this.principalClientId = principalSecrets.getPrincipalClientId(); - this.mainSecret = principalSecrets.getMainSecret(); - this.secondarySecret = principalSecrets.getSecondarySecret(); + this.secretSalt = principalSecrets.getSecretSalt(); + this.mainSecretHash = principalSecrets.getMainSecretHash(); + this.secondarySecretHash = principalSecrets.getSecondarySecretHash(); } } diff --git a/polaris-service/src/main/java/org/apache/polaris/service/auth/TokenBroker.java b/polaris-service/src/main/java/org/apache/polaris/service/auth/TokenBroker.java index 86bd2748eb..095508a7f2 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/auth/TokenBroker.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/auth/TokenBroker.java @@ -53,8 +53,7 @@ TokenResponse generateFromToken( if (!principalSecrets.isSuccess()) { return Optional.empty(); } - if (!principalSecrets.getPrincipalSecrets().getMainSecret().equals(clientSecret) - && !principalSecrets.getPrincipalSecrets().getSecondarySecret().equals(clientSecret)) { + if (!principalSecrets.getPrincipalSecrets().matchesSecret(clientSecret)) { return Optional.empty(); } PolarisMetaStoreManager.EntityResult result = From 5bc253088a86145dbe2aaf0749dcd3ee5257865f Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 14:56:34 -0800 Subject: [PATCH 02/13] testing with postgres --- .../src/main/resources/META-INF/persistence.xml | 8 +++----- .../core/entity/PolarisPrincipalSecrets.java | 16 +++++----------- .../models/ModelPrincipalSecrets.java | 3 +-- polaris-server.yml | 8 ++++---- 4 files changed, 13 insertions(+), 22 deletions(-) diff --git a/extension/persistence/eclipselink/src/main/resources/META-INF/persistence.xml b/extension/persistence/eclipselink/src/main/resources/META-INF/persistence.xml index 913713796b..c316085597 100644 --- a/extension/persistence/eclipselink/src/main/resources/META-INF/persistence.xml +++ b/extension/persistence/eclipselink/src/main/resources/META-INF/persistence.xml @@ -35,12 +35,10 @@ NONE - - + value="jdbc:postgresql://localhost:5432/db"/> + + - - diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java index 13a1535c48..9fa677adcf 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java @@ -20,9 +20,8 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; -import org.apache.commons.codec.digest.DigestUtils; - import java.security.SecureRandom; +import org.apache.commons.codec.digest.DigestUtils; /** * Simple class to represent the secrets used to authenticate a catalog principal, These secrets are @@ -107,10 +106,7 @@ public PolarisPrincipalSecrets( } public PolarisPrincipalSecrets( - long principalId, - String principalClientId, - String mainSecret, - String secondarySecret) { + long principalId, String principalClientId, String mainSecret, String secondarySecret) { this.principalId = principalId; this.principalClientId = principalClientId; this.mainSecret = mainSecret; @@ -142,10 +138,7 @@ public PolarisPrincipalSecrets(long principalId) { this.secondarySecretHash = hashSecret(secondarySecret); } - /** - * Rotate the main secrets - * - */ + /** Rotate the main secrets */ public void rotateSecrets() { this.secondarySecret = this.mainSecret; this.secondarySecretHash = this.mainSecretHash; @@ -164,7 +157,8 @@ public String getPrincipalClientId() { public boolean matchesSecret(String potentialSecret) { String potentialSecretHash = hashSecret(potentialSecret); - return potentialSecretHash.equals(this.mainSecretHash) || potentialSecretHash.equals(this.secondarySecretHash); + return potentialSecretHash.equals(this.mainSecretHash) + || potentialSecretHash.equals(this.secondarySecretHash); } public String getMainSecret() { diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/models/ModelPrincipalSecrets.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/models/ModelPrincipalSecrets.java index c4ebad6a60..6dab687a13 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/models/ModelPrincipalSecrets.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/models/ModelPrincipalSecrets.java @@ -155,8 +155,7 @@ public static PolarisPrincipalSecrets toPrincipalSecrets(ModelPrincipalSecrets m model.getSecondarySecret(), model.getSecretSalt(), model.getMainSecretHash(), - model.getSecondarySecretHash() - ); + model.getSecondarySecretHash()); } public void update(PolarisPrincipalSecrets principalSecrets) { diff --git a/polaris-server.yml b/polaris-server.yml index e147d4d48e..9a031afb75 100644 --- a/polaris-server.yml +++ b/polaris-server.yml @@ -83,12 +83,12 @@ defaultRealms: - default-realm metaStoreManager: - type: in-memory - # type: eclipse-link # uncomment to use eclipse-link as metastore - # persistence-unit: polaris +# type: in-memory + type: eclipse-link # uncomment to use eclipse-link as metastore + persistence-unit: polaris io: - factoryType: wasb + factoryType: default # TODO - avoid duplicating token broker config oauth2: From 8cde7a5939919a469aa16a069789bc0429917350 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 15:01:49 -0800 Subject: [PATCH 03/13] reverts --- .../src/main/resources/META-INF/persistence.xml | 8 +++++--- polaris-server.yml | 8 ++++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/extension/persistence/eclipselink/src/main/resources/META-INF/persistence.xml b/extension/persistence/eclipselink/src/main/resources/META-INF/persistence.xml index c316085597..913713796b 100644 --- a/extension/persistence/eclipselink/src/main/resources/META-INF/persistence.xml +++ b/extension/persistence/eclipselink/src/main/resources/META-INF/persistence.xml @@ -35,10 +35,12 @@ NONE - - + value="jdbc:h2:file:./build/test_data/polaris/{realm}/db"/> + + + + diff --git a/polaris-server.yml b/polaris-server.yml index 9a031afb75..e147d4d48e 100644 --- a/polaris-server.yml +++ b/polaris-server.yml @@ -83,12 +83,12 @@ defaultRealms: - default-realm metaStoreManager: -# type: in-memory - type: eclipse-link # uncomment to use eclipse-link as metastore - persistence-unit: polaris + type: in-memory + # type: eclipse-link # uncomment to use eclipse-link as metastore + # persistence-unit: polaris io: - factoryType: default + factoryType: wasb # TODO - avoid duplicating token broker config oauth2: From 54f01ce9ab6888b5c1052a86511a957a3b023eae Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 16:11:43 -0800 Subject: [PATCH 04/13] test fixes --- .../PolarisTestMetaStoreManager.java | 30 ++++++++----------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java index 893d22927b..efcb865cdb 100644 --- a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java +++ b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java @@ -402,9 +402,9 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(reloadSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(reloadSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(reloadSecrets.getMainSecret()).isEqualTo(secrets.getMainSecret()); - Assertions.assertThat(reloadSecrets.getSecondarySecret()) - .isEqualTo(secrets.getSecondarySecret()); + Assertions.assertThat(reloadSecrets.getMainSecretHash()).isEqualTo(secrets.getMainSecretHash()); + Assertions.assertThat(reloadSecrets.getSecondarySecretHash()) + .isEqualTo(secrets.getSecondarySecretHash()); Map internalProperties = PolarisObjectMapperUtil.deserializeProperties( @@ -428,8 +428,7 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(newSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(newSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(newSecrets.getMainSecret()).isEqualTo(secrets.getMainSecret()); - Assertions.assertThat(newSecrets.getMainSecret()).isEqualTo(secrets.getMainSecret()); + Assertions.assertThat(newSecrets.getMainSecretHash()).isEqualTo(secrets.getMainSecretHash()); } secrets = @@ -455,13 +454,10 @@ PolarisBaseEntity createPrincipal(String name) { PolarisEntityConstants.PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATE)) .isNull(); - // rotate the secrets, twice! - polarisMetaStoreManager.rotatePrincipalSecrets( - this.polarisCallContext, clientId, principalEntity.getId(), secrets.getMainSecret(), false); + // rotate the secret polarisMetaStoreManager.rotatePrincipalSecrets( this.polarisCallContext, clientId, principalEntity.getId(), secrets.getMainSecret(), false); - // reload and check that now the main should be secondary reloadSecrets = polarisMetaStoreManager .loadPrincipalSecrets(this.polarisCallContext, clientId) @@ -470,8 +466,8 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(reloadSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(reloadSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(reloadSecrets.getSecondarySecret()).isEqualTo(secrets.getMainSecret()); - String newMainSecret = reloadSecrets.getMainSecret(); + Assertions.assertThat(reloadSecrets.getSecondarySecretHash()).isEqualTo(secrets.getMainSecretHash()); + String newMainSecretHash = reloadSecrets.getMainSecretHash(); // reset - the previous main secret is no longer one of the secrets polarisMetaStoreManager.rotatePrincipalSecrets( @@ -488,8 +484,8 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(reloadSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(reloadSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(reloadSecrets.getMainSecret()).isNotEqualTo(newMainSecret); - Assertions.assertThat(reloadSecrets.getSecondarySecret()).isNotEqualTo(newMainSecret); + Assertions.assertThat(reloadSecrets.getMainSecretHash()).isNotEqualTo(newMainSecretHash); + Assertions.assertThat(reloadSecrets.getSecondarySecretHash()).isNotEqualTo(newMainSecretHash); PolarisBaseEntity newPrincipal = polarisMetaStoreManager @@ -520,10 +516,10 @@ PolarisBaseEntity createPrincipal(String name) { .isEqualTo(reloadSecrets.getPrincipalId()); Assertions.assertThat(postResetCredentials.getPrincipalClientId()) .isEqualTo(reloadSecrets.getPrincipalClientId()); - Assertions.assertThat(postResetCredentials.getMainSecret()) - .isNotEqualTo(reloadSecrets.getMainSecret()); - Assertions.assertThat(postResetCredentials.getSecondarySecret()) - .isNotEqualTo(reloadSecrets.getSecondarySecret()); + Assertions.assertThat(postResetCredentials.getMainSecretHash()) + .isNotEqualTo(reloadSecrets.getMainSecretHash()); + Assertions.assertThat(postResetCredentials.getSecondarySecretHash()) + .isNotEqualTo(reloadSecrets.getSecondarySecretHash()); PolarisBaseEntity finalPrincipal = polarisMetaStoreManager From cfae37210b3ef1324b17eeeb3e0e4431bcbd0e29 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 16:11:46 -0800 Subject: [PATCH 05/13] autolint --- .../polaris/core/persistence/PolarisTestMetaStoreManager.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java index efcb865cdb..1a292840bb 100644 --- a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java +++ b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java @@ -466,7 +466,8 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(reloadSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(reloadSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(reloadSecrets.getSecondarySecretHash()).isEqualTo(secrets.getMainSecretHash()); + Assertions.assertThat(reloadSecrets.getSecondarySecretHash()) + .isEqualTo(secrets.getMainSecretHash()); String newMainSecretHash = reloadSecrets.getMainSecretHash(); // reset - the previous main secret is no longer one of the secrets From 0d085b3464651c79bf18765d273348cc357a147f Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 17:04:38 -0800 Subject: [PATCH 06/13] add a test --- ...olarisEclipseLinkMetaStoreManagerTest.java | 81 +++++++++++++++++++ .../models/ModelPrincipalSecrets.java | 10 +++ 2 files changed, 91 insertions(+) diff --git a/extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java b/extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java index 000a909e91..cb46b23b4e 100644 --- a/extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java +++ b/extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java @@ -18,19 +18,31 @@ */ package org.apache.polaris.extension.persistence.impl.eclipselink; +import static jakarta.persistence.Persistence.createEntityManagerFactory; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.time.ZoneId; +import java.util.UUID; import java.util.stream.Stream; + +import jakarta.persistence.EntityManager; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisConfigurationStore; import org.apache.polaris.core.PolarisDefaultDiagServiceImpl; import org.apache.polaris.core.PolarisDiagnostics; +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.PolarisPrincipalSecrets; import org.apache.polaris.core.persistence.BasePolarisMetaStoreManagerTest; import org.apache.polaris.core.persistence.PolarisMetaStoreManagerImpl; import org.apache.polaris.core.persistence.PolarisTestMetaStoreManager; +import org.apache.polaris.core.persistence.models.ModelEntity; +import org.apache.polaris.core.persistence.models.ModelPrincipalSecrets; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtensionContext; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -80,6 +92,75 @@ void testCreateStoreSession(String confFile, boolean success) { } } + @Test + void testRotateLegacyPrincipalSecret() { + + PolarisEclipseLinkMetaStoreSessionImpl.clearEntityManagerFactories(); + PolarisDiagnostics diagServices = new PolarisDefaultDiagServiceImpl(); + + var key = "client-id-" + UUID.randomUUID(); + ModelPrincipalSecrets model = ModelPrincipalSecrets.builder() + .principalId(Math.abs(key.hashCode())) + .principalClientId(key) + .mainSecret("secret!") + .build(); + Assertions.assertNotNull(model.getMainSecret()); + Assertions.assertNull(model.getMainSecretHash()); + + try (var emf = createEntityManagerFactory("polaris")) { + var entityManager = emf.createEntityManager(); + + // Persist the original model: + entityManager.getTransaction().begin(); + entityManager.persist(model); + entityManager.getTransaction().commit(); + + // Retrieve the model + entityManager.clear(); + ModelPrincipalSecrets retrievedModel = entityManager.find(ModelPrincipalSecrets.class, key); + + // Verify the retrieved entity still has no hash + Assertions.assertNotNull(retrievedModel); + Assertions.assertNotNull(retrievedModel.getMainSecret()); + Assertions.assertNull(retrievedModel.getMainSecretHash()); + + // Now read using PolarisEclipseLinkStore + PolarisEclipseLinkStore store = new PolarisEclipseLinkStore(diagServices); + store.initialize(entityManager); + PolarisPrincipalSecrets principalSecrets = ModelPrincipalSecrets + .toPrincipalSecrets(store.lookupPrincipalSecrets(entityManager, key)); + + // The principalSecrets should have both a main secret and a hashed secret + Assertions.assertNotNull(principalSecrets); + Assertions.assertNotNull(principalSecrets.getMainSecret()); + Assertions.assertNotNull(principalSecrets.getMainSecretHash()); + Assertions.assertNull(principalSecrets.getSecondarySecret()); + + // Rotate: + String originalSecret = principalSecrets.getMainSecret(); + String originalHash = principalSecrets.getMainSecretHash(); + principalSecrets.rotateSecrets(); + Assertions.assertNotEquals(originalHash, principalSecrets.getMainSecretHash()); + Assertions.assertEquals(originalHash, principalSecrets.getSecondarySecretHash()); + Assertions.assertEquals(originalSecret, principalSecrets.getSecondarySecret()); + + // Persist the rotated credential: + store.deletePrincipalSecrets(entityManager, key); + store.writePrincipalSecrets(entityManager, principalSecrets); + + // Reload the model: + var reloadedModel = store.lookupPrincipalSecrets(entityManager, key); + + // The old plaintext secret is gone: + Assertions.assertNull(reloadedModel.getMainSecret()); + Assertions.assertNull(reloadedModel.getSecondarySecret()); + + // Confirm the old secret still works via hash: + var reloadedSecrets = ModelPrincipalSecrets.toPrincipalSecrets(reloadedModel); + Assertions.assertTrue(reloadedSecrets.matchesSecret(originalSecret)); + } + } + private static class CreateStoreSessionArgs implements ArgumentsProvider { @Override public Stream provideArguments(ExtensionContext extensionContext) { diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/models/ModelPrincipalSecrets.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/models/ModelPrincipalSecrets.java index 6dab687a13..2a4cf7a55a 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/models/ModelPrincipalSecrets.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/models/ModelPrincipalSecrets.java @@ -164,7 +164,17 @@ public void update(PolarisPrincipalSecrets principalSecrets) { this.principalId = principalSecrets.getPrincipalId(); this.principalClientId = principalSecrets.getPrincipalClientId(); this.secretSalt = principalSecrets.getSecretSalt(); + this.mainSecret = principalSecrets.getMainSecret(); + this.secondarySecret = principalSecrets.getSecondarySecret(); this.mainSecretHash = principalSecrets.getMainSecretHash(); this.secondarySecretHash = principalSecrets.getSecondarySecretHash(); + + // Once a hash is stored, do not keep the original secret + if (this.mainSecretHash != null) { + this.mainSecret = null; + } + if (this.secondarySecretHash != null) { + this.secondarySecret = null; + } } } From 3cc38a8b679a9c99447018b5ceeb7d4ab7a0a261 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 17:04:42 -0800 Subject: [PATCH 07/13] autolint --- ...olarisEclipseLinkMetaStoreManagerTest.java | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java b/extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java index cb46b23b4e..64cdb07994 100644 --- a/extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java +++ b/extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java @@ -26,20 +26,14 @@ import java.time.ZoneId; import java.util.UUID; import java.util.stream.Stream; - -import jakarta.persistence.EntityManager; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisConfigurationStore; import org.apache.polaris.core.PolarisDefaultDiagServiceImpl; import org.apache.polaris.core.PolarisDiagnostics; -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.PolarisPrincipalSecrets; import org.apache.polaris.core.persistence.BasePolarisMetaStoreManagerTest; import org.apache.polaris.core.persistence.PolarisMetaStoreManagerImpl; import org.apache.polaris.core.persistence.PolarisTestMetaStoreManager; -import org.apache.polaris.core.persistence.models.ModelEntity; import org.apache.polaris.core.persistence.models.ModelPrincipalSecrets; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -99,11 +93,12 @@ void testRotateLegacyPrincipalSecret() { PolarisDiagnostics diagServices = new PolarisDefaultDiagServiceImpl(); var key = "client-id-" + UUID.randomUUID(); - ModelPrincipalSecrets model = ModelPrincipalSecrets.builder() - .principalId(Math.abs(key.hashCode())) - .principalClientId(key) - .mainSecret("secret!") - .build(); + ModelPrincipalSecrets model = + ModelPrincipalSecrets.builder() + .principalId(Math.abs(key.hashCode())) + .principalClientId(key) + .mainSecret("secret!") + .build(); Assertions.assertNotNull(model.getMainSecret()); Assertions.assertNull(model.getMainSecretHash()); @@ -127,8 +122,9 @@ void testRotateLegacyPrincipalSecret() { // Now read using PolarisEclipseLinkStore PolarisEclipseLinkStore store = new PolarisEclipseLinkStore(diagServices); store.initialize(entityManager); - PolarisPrincipalSecrets principalSecrets = ModelPrincipalSecrets - .toPrincipalSecrets(store.lookupPrincipalSecrets(entityManager, key)); + PolarisPrincipalSecrets principalSecrets = + ModelPrincipalSecrets.toPrincipalSecrets( + store.lookupPrincipalSecrets(entityManager, key)); // The principalSecrets should have both a main secret and a hashed secret Assertions.assertNotNull(principalSecrets); From 18d5377922759babb9b3afda5edf4f7b429e2b3e Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Thu, 7 Nov 2024 17:33:13 -0800 Subject: [PATCH 08/13] typofix --- .../org/apache/polaris/core/entity/PolarisPrincipalSecrets.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java index 9fa677adcf..99e9dcd171 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java @@ -122,7 +122,7 @@ public PolarisPrincipalSecrets(PolarisPrincipalSecrets principalSecrets) { this.principalClientId = principalSecrets.getPrincipalClientId(); this.mainSecret = principalSecrets.getMainSecret(); this.secondarySecret = principalSecrets.getSecondarySecret(); - this.secretSalt = principalSecrets.getSecondarySecret(); + this.secretSalt = principalSecrets.getSecretSalt(); this.mainSecretHash = principalSecrets.getMainSecretHash(); this.secondarySecretHash = principalSecrets.getSecondarySecretHash(); } From 7f490b0aa5f4fc8b778ae234107affc061a56a3b Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Tue, 12 Nov 2024 11:16:07 -0800 Subject: [PATCH 09/13] idempotent again --- ...olarisEclipseLinkMetaStoreSessionImpl.java | 8 ++--- .../core/entity/PolarisPrincipalSecrets.java | 6 ++-- .../LocalPolarisMetaStoreManagerFactory.java | 4 +-- .../persistence/PolarisMetaStoreManager.java | 6 ++-- .../PolarisMetaStoreManagerImpl.java | 12 +++---- .../persistence/PolarisMetaStoreSession.java | 6 ++-- .../PolarisTreeMapMetaStoreSessionImpl.java | 8 ++--- .../TransactionWorkspaceMetaStoreManager.java | 4 +-- .../PolarisTestMetaStoreManager.java | 31 ++++++++++--------- 9 files changed, 44 insertions(+), 41 deletions(-) diff --git a/extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java b/extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java index ffe2d9e717..534d32a0ba 100644 --- a/extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java +++ b/extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java @@ -672,8 +672,8 @@ public int lookupEntityGrantRecordsVersion( @NotNull PolarisCallContext callCtx, @NotNull String clientId, long principalId, - @NotNull String mainSecretToRotate, - boolean reset) { + boolean reset, + @NotNull String oldSecretHash) { // load the existing secrets PolarisPrincipalSecrets principalSecrets = @@ -701,9 +701,9 @@ public int lookupEntityGrantRecordsVersion( principalSecrets.getPrincipalId()); // rotate the secrets - principalSecrets.rotateSecrets(); + principalSecrets.rotateSecrets(oldSecretHash); if (reset) { - principalSecrets.rotateSecrets(); + principalSecrets.rotateSecrets(principalSecrets.getMainSecretHash()); } // write back new secrets diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java index 99e9dcd171..dc98d46f6a 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java @@ -139,9 +139,9 @@ public PolarisPrincipalSecrets(long principalId) { } /** Rotate the main secrets */ - public void rotateSecrets() { - this.secondarySecret = this.mainSecret; - this.secondarySecretHash = this.mainSecretHash; + public void rotateSecrets(String newSecondaryHash) { + this.secondarySecret = null; + this.secondarySecretHash = newSecondaryHash; this.mainSecret = this.generateRandomHexString(32); this.mainSecretHash = hashSecret(mainSecret); diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java index 4453e4f14d..095853cff1 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java @@ -204,8 +204,8 @@ public void setStorageIntegrationProvider(PolarisStorageIntegrationProvider stor polarisContext, secrets.getPrincipalClientId(), secrets.getPrincipalId(), - secrets.getMainSecret(), - false); + false, + secrets.getMainSecretHash()); return rotatedSecrets; } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java index 9033795d8c..697cb69e5e 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java @@ -547,10 +547,10 @@ PrincipalSecretsResult loadPrincipalSecrets( * @param callCtx call context * @param clientId principal client id * @param principalId id of the principal - * @param mainSecret main secret for the principal * @param reset true if the principal's secrets should be disabled and replaced with a one-time * password. if the principal's secret is already a one-time password, this flag is * automatically true + * @param oldSecretHash main secret hash for the principal * @return the secrets associated to that principal amd the id of the principal */ @NotNull @@ -558,8 +558,8 @@ PrincipalSecretsResult rotatePrincipalSecrets( @NotNull PolarisCallContext callCtx, @NotNull String clientId, long principalId, - @NotNull String mainSecret, - boolean reset); + boolean reset, + @NotNull String oldSecretHash); /** the return the result of a create-catalog method */ class CreateCatalogResult extends BaseResult { 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 f094ba412d..616dc4581d 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 @@ -1012,8 +1012,8 @@ public Map deserializeProperties(PolarisCallContext callCtx, Str @NotNull PolarisMetaStoreSession ms, @NotNull String clientId, long principalId, - @NotNull String masterSecret, - boolean reset) { + boolean reset, + @NotNull String oldSecretHash) { // if not found, the principal must have been dropped EntityResult loadEntityResult = loadEntity(callCtx, ms, PolarisEntityConstants.getNullId(), principalId); @@ -1033,7 +1033,7 @@ public Map deserializeProperties(PolarisCallContext callCtx, Str PolarisEntityConstants.PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATE) != null; PolarisPrincipalSecrets secrets = - ms.rotatePrincipalSecrets(callCtx, clientId, principalId, masterSecret, doReset); + ms.rotatePrincipalSecrets(callCtx, clientId, principalId, doReset, oldSecretHash); if (reset && !internalProps.containsKey( @@ -1061,8 +1061,8 @@ public Map deserializeProperties(PolarisCallContext callCtx, Str @NotNull PolarisCallContext callCtx, @NotNull String clientId, long principalId, - @NotNull String mainSecret, - boolean reset) { + boolean reset, + @NotNull String oldSecretHash) { // get metastore we should be using PolarisMetaStoreSession ms = callCtx.getMetaStore(); @@ -1071,7 +1071,7 @@ public Map deserializeProperties(PolarisCallContext callCtx, Str ms.runInTransaction( callCtx, () -> - this.rotatePrincipalSecrets(callCtx, ms, clientId, principalId, mainSecret, reset)); + this.rotatePrincipalSecrets(callCtx, ms, clientId, principalId, reset, oldSecretHash)); return (secrets == null) ? new PrincipalSecretsResult(ReturnStatus.ENTITY_NOT_FOUND, null) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreSession.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreSession.java index ff5bcbf3e7..448cb23533 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreSession.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreSession.java @@ -444,17 +444,17 @@ PolarisPrincipalSecrets generateNewPrincipalSecrets( * @param callCtx call context * @param clientId principal client id * @param principalId principal id - * @param mainSecretToRotate main secret for comparison with the current entity version * @param reset true if the principal secrets should be disabled and replaced with a one-time * password + * @param oldSecretHash the principal secret's old main secret hash */ @Nullable PolarisPrincipalSecrets rotatePrincipalSecrets( @NotNull PolarisCallContext callCtx, @NotNull String clientId, long principalId, - @NotNull String mainSecretToRotate, - boolean reset); + boolean reset, + @NotNull String oldSecretHash); /** * When dropping a principal, we also need to drop the secrets of that principal diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisTreeMapMetaStoreSessionImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisTreeMapMetaStoreSessionImpl.java index 7859f13039..bcf1a97f56 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisTreeMapMetaStoreSessionImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisTreeMapMetaStoreSessionImpl.java @@ -474,8 +474,8 @@ public int lookupEntityGrantRecordsVersion( @NotNull PolarisCallContext callCtx, @NotNull String clientId, long principalId, - @NotNull String mainSecretToRotate, - boolean reset) { + boolean reset, + @NotNull String oldSecretHash) { // load the existing secrets PolarisPrincipalSecrets principalSecrets = this.store.getSlicePrincipalSecrets().read(clientId); @@ -501,9 +501,9 @@ public int lookupEntityGrantRecordsVersion( principalSecrets.getPrincipalId()); // rotate the secrets - principalSecrets.rotateSecrets(); + principalSecrets.rotateSecrets(oldSecretHash); if (reset) { - principalSecrets.rotateSecrets(); + principalSecrets.rotateSecrets(principalSecrets.getMainSecretHash()); } // write back new secrets 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 4d7d656ad9..e2ae841468 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 @@ -134,8 +134,8 @@ public PrincipalSecretsResult rotatePrincipalSecrets( @NotNull PolarisCallContext callCtx, @NotNull String clientId, long principalId, - @NotNull String mainSecret, - boolean reset) { + boolean reset, + @NotNull String oldSecretHash) { callCtx .getDiagServices() .fail("illegal_method_in_transaction_workspace", "rotatePrincipalSecrets"); diff --git a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java index 1a292840bb..893d22927b 100644 --- a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java +++ b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java @@ -402,9 +402,9 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(reloadSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(reloadSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(reloadSecrets.getMainSecretHash()).isEqualTo(secrets.getMainSecretHash()); - Assertions.assertThat(reloadSecrets.getSecondarySecretHash()) - .isEqualTo(secrets.getSecondarySecretHash()); + Assertions.assertThat(reloadSecrets.getMainSecret()).isEqualTo(secrets.getMainSecret()); + Assertions.assertThat(reloadSecrets.getSecondarySecret()) + .isEqualTo(secrets.getSecondarySecret()); Map internalProperties = PolarisObjectMapperUtil.deserializeProperties( @@ -428,7 +428,8 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(newSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(newSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(newSecrets.getMainSecretHash()).isEqualTo(secrets.getMainSecretHash()); + Assertions.assertThat(newSecrets.getMainSecret()).isEqualTo(secrets.getMainSecret()); + Assertions.assertThat(newSecrets.getMainSecret()).isEqualTo(secrets.getMainSecret()); } secrets = @@ -454,10 +455,13 @@ PolarisBaseEntity createPrincipal(String name) { PolarisEntityConstants.PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATE)) .isNull(); - // rotate the secret + // rotate the secrets, twice! + polarisMetaStoreManager.rotatePrincipalSecrets( + this.polarisCallContext, clientId, principalEntity.getId(), secrets.getMainSecret(), false); polarisMetaStoreManager.rotatePrincipalSecrets( this.polarisCallContext, clientId, principalEntity.getId(), secrets.getMainSecret(), false); + // reload and check that now the main should be secondary reloadSecrets = polarisMetaStoreManager .loadPrincipalSecrets(this.polarisCallContext, clientId) @@ -466,9 +470,8 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(reloadSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(reloadSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(reloadSecrets.getSecondarySecretHash()) - .isEqualTo(secrets.getMainSecretHash()); - String newMainSecretHash = reloadSecrets.getMainSecretHash(); + Assertions.assertThat(reloadSecrets.getSecondarySecret()).isEqualTo(secrets.getMainSecret()); + String newMainSecret = reloadSecrets.getMainSecret(); // reset - the previous main secret is no longer one of the secrets polarisMetaStoreManager.rotatePrincipalSecrets( @@ -485,8 +488,8 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(reloadSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(reloadSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(reloadSecrets.getMainSecretHash()).isNotEqualTo(newMainSecretHash); - Assertions.assertThat(reloadSecrets.getSecondarySecretHash()).isNotEqualTo(newMainSecretHash); + Assertions.assertThat(reloadSecrets.getMainSecret()).isNotEqualTo(newMainSecret); + Assertions.assertThat(reloadSecrets.getSecondarySecret()).isNotEqualTo(newMainSecret); PolarisBaseEntity newPrincipal = polarisMetaStoreManager @@ -517,10 +520,10 @@ PolarisBaseEntity createPrincipal(String name) { .isEqualTo(reloadSecrets.getPrincipalId()); Assertions.assertThat(postResetCredentials.getPrincipalClientId()) .isEqualTo(reloadSecrets.getPrincipalClientId()); - Assertions.assertThat(postResetCredentials.getMainSecretHash()) - .isNotEqualTo(reloadSecrets.getMainSecretHash()); - Assertions.assertThat(postResetCredentials.getSecondarySecretHash()) - .isNotEqualTo(reloadSecrets.getSecondarySecretHash()); + Assertions.assertThat(postResetCredentials.getMainSecret()) + .isNotEqualTo(reloadSecrets.getMainSecret()); + Assertions.assertThat(postResetCredentials.getSecondarySecret()) + .isNotEqualTo(reloadSecrets.getSecondarySecret()); PolarisBaseEntity finalPrincipal = polarisMetaStoreManager From 0c054f5b6b47b8e830d4280a291fe18861ee0c74 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Tue, 12 Nov 2024 11:21:20 -0800 Subject: [PATCH 10/13] stable --- ...olarisEclipseLinkMetaStoreManagerTest.java | 4 +- .../PolarisTestMetaStoreManager.java | 42 +++++++++---------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java b/extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java index 64cdb07994..be2ff5bca4 100644 --- a/extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java +++ b/extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java @@ -135,10 +135,10 @@ void testRotateLegacyPrincipalSecret() { // Rotate: String originalSecret = principalSecrets.getMainSecret(); String originalHash = principalSecrets.getMainSecretHash(); - principalSecrets.rotateSecrets(); + principalSecrets.rotateSecrets(principalSecrets.getMainSecretHash()); Assertions.assertNotEquals(originalHash, principalSecrets.getMainSecretHash()); Assertions.assertEquals(originalHash, principalSecrets.getSecondarySecretHash()); - Assertions.assertEquals(originalSecret, principalSecrets.getSecondarySecret()); + Assertions.assertEquals(null, principalSecrets.getSecondarySecret()); // Persist the rotated credential: store.deletePrincipalSecrets(entityManager, key); diff --git a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java index 893d22927b..ac824689ab 100644 --- a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java +++ b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java @@ -402,9 +402,9 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(reloadSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(reloadSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(reloadSecrets.getMainSecret()).isEqualTo(secrets.getMainSecret()); - Assertions.assertThat(reloadSecrets.getSecondarySecret()) - .isEqualTo(secrets.getSecondarySecret()); + Assertions.assertThat(reloadSecrets.getMainSecretHash()).isEqualTo(secrets.getMainSecretHash()); + Assertions.assertThat(reloadSecrets.getSecondarySecretHash()) + .isEqualTo(secrets.getSecondarySecretHash()); Map internalProperties = PolarisObjectMapperUtil.deserializeProperties( @@ -428,8 +428,8 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(newSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(newSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(newSecrets.getMainSecret()).isEqualTo(secrets.getMainSecret()); - Assertions.assertThat(newSecrets.getMainSecret()).isEqualTo(secrets.getMainSecret()); + Assertions.assertThat(newSecrets.getMainSecretHash()).isEqualTo(secrets.getMainSecretHash()); + Assertions.assertThat(newSecrets.getMainSecretHash()).isEqualTo(secrets.getMainSecretHash()); } secrets = @@ -438,8 +438,8 @@ PolarisBaseEntity createPrincipal(String name) { this.polarisCallContext, clientId, principalEntity.getId(), - secrets.getMainSecret(), - false) + false, + secrets.getMainSecretHash()) .getPrincipalSecrets(); Assertions.assertThat(secrets.getMainSecret()).isNotEqualTo(reloadSecrets.getMainSecret()); @@ -457,9 +457,9 @@ PolarisBaseEntity createPrincipal(String name) { // rotate the secrets, twice! polarisMetaStoreManager.rotatePrincipalSecrets( - this.polarisCallContext, clientId, principalEntity.getId(), secrets.getMainSecret(), false); + this.polarisCallContext, clientId, principalEntity.getId(), false, secrets.getMainSecretHash()); polarisMetaStoreManager.rotatePrincipalSecrets( - this.polarisCallContext, clientId, principalEntity.getId(), secrets.getMainSecret(), false); + this.polarisCallContext, clientId, principalEntity.getId(), false, secrets.getMainSecretHash()); // reload and check that now the main should be secondary reloadSecrets = @@ -470,16 +470,16 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(reloadSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(reloadSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(reloadSecrets.getSecondarySecret()).isEqualTo(secrets.getMainSecret()); - String newMainSecret = reloadSecrets.getMainSecret(); + Assertions.assertThat(reloadSecrets.getSecondarySecretHash()).isEqualTo(secrets.getMainSecretHash()); + String newMainSecretHash = reloadSecrets.getMainSecretHash(); // reset - the previous main secret is no longer one of the secrets polarisMetaStoreManager.rotatePrincipalSecrets( this.polarisCallContext, clientId, principalEntity.getId(), - reloadSecrets.getMainSecret(), - true); + true, + reloadSecrets.getMainSecretHash()); reloadSecrets = polarisMetaStoreManager .loadPrincipalSecrets(this.polarisCallContext, clientId) @@ -488,8 +488,8 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(reloadSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(reloadSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(reloadSecrets.getMainSecret()).isNotEqualTo(newMainSecret); - Assertions.assertThat(reloadSecrets.getSecondarySecret()).isNotEqualTo(newMainSecret); + Assertions.assertThat(reloadSecrets.getMainSecretHash()).isNotEqualTo(newMainSecretHash); + Assertions.assertThat(reloadSecrets.getSecondarySecretHash()).isNotEqualTo(newMainSecretHash); PolarisBaseEntity newPrincipal = polarisMetaStoreManager @@ -509,8 +509,8 @@ PolarisBaseEntity createPrincipal(String name) { this.polarisCallContext, clientId, principalEntity.getId(), - reloadSecrets.getMainSecret(), - true); + true, + reloadSecrets.getMainSecretHash()); PolarisPrincipalSecrets postResetCredentials = polarisMetaStoreManager .loadPrincipalSecrets(this.polarisCallContext, clientId) @@ -520,10 +520,10 @@ PolarisBaseEntity createPrincipal(String name) { .isEqualTo(reloadSecrets.getPrincipalId()); Assertions.assertThat(postResetCredentials.getPrincipalClientId()) .isEqualTo(reloadSecrets.getPrincipalClientId()); - Assertions.assertThat(postResetCredentials.getMainSecret()) - .isNotEqualTo(reloadSecrets.getMainSecret()); - Assertions.assertThat(postResetCredentials.getSecondarySecret()) - .isNotEqualTo(reloadSecrets.getSecondarySecret()); + Assertions.assertThat(postResetCredentials.getMainSecretHash()) + .isNotEqualTo(reloadSecrets.getMainSecretHash()); + Assertions.assertThat(postResetCredentials.getSecondarySecretHash()) + .isNotEqualTo(reloadSecrets.getSecondarySecretHash()); PolarisBaseEntity finalPrincipal = polarisMetaStoreManager From 8e0a752bc25b17d8ca80db890ab42ef6530c0d5f Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Tue, 12 Nov 2024 11:21:24 -0800 Subject: [PATCH 11/13] autolint --- .../persistence/PolarisMetaStoreManagerImpl.java | 3 ++- .../persistence/PolarisTestMetaStoreManager.java | 15 ++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) 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 616dc4581d..c3fc3a81ee 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 @@ -1071,7 +1071,8 @@ public Map deserializeProperties(PolarisCallContext callCtx, Str ms.runInTransaction( callCtx, () -> - this.rotatePrincipalSecrets(callCtx, ms, clientId, principalId, reset, oldSecretHash)); + this.rotatePrincipalSecrets( + callCtx, ms, clientId, principalId, reset, oldSecretHash)); return (secrets == null) ? new PrincipalSecretsResult(ReturnStatus.ENTITY_NOT_FOUND, null) diff --git a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java index ac824689ab..fe44f38e81 100644 --- a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java +++ b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java @@ -457,9 +457,17 @@ PolarisBaseEntity createPrincipal(String name) { // rotate the secrets, twice! polarisMetaStoreManager.rotatePrincipalSecrets( - this.polarisCallContext, clientId, principalEntity.getId(), false, secrets.getMainSecretHash()); + this.polarisCallContext, + clientId, + principalEntity.getId(), + false, + secrets.getMainSecretHash()); polarisMetaStoreManager.rotatePrincipalSecrets( - this.polarisCallContext, clientId, principalEntity.getId(), false, secrets.getMainSecretHash()); + this.polarisCallContext, + clientId, + principalEntity.getId(), + false, + secrets.getMainSecretHash()); // reload and check that now the main should be secondary reloadSecrets = @@ -470,7 +478,8 @@ PolarisBaseEntity createPrincipal(String name) { Assertions.assertThat(reloadSecrets.getPrincipalId()).isEqualTo(secrets.getPrincipalId()); Assertions.assertThat(reloadSecrets.getPrincipalClientId()) .isEqualTo(secrets.getPrincipalClientId()); - Assertions.assertThat(reloadSecrets.getSecondarySecretHash()).isEqualTo(secrets.getMainSecretHash()); + Assertions.assertThat(reloadSecrets.getSecondarySecretHash()) + .isEqualTo(secrets.getMainSecretHash()); String newMainSecretHash = reloadSecrets.getMainSecretHash(); // reset - the previous main secret is no longer one of the secrets From b856e9b4d32e48a3731eb01df6413db686d7deff Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Tue, 12 Nov 2024 11:53:11 -0800 Subject: [PATCH 12/13] fix another place --- .../org/apache/polaris/service/admin/PolarisAdminService.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polaris-service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java b/polaris-service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java index 9a2dae2b9c..236279d4f8 100644 --- a/polaris-service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java +++ b/polaris-service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java @@ -854,8 +854,8 @@ public void deletePrincipal(String name) { getCurrentPolarisContext(), currentPrincipalEntity.getClientId(), currentPrincipalEntity.getId(), - currentSecrets.getMainSecret(), - shouldReset) + shouldReset, + currentSecrets.getMainSecretHash()) .getPrincipalSecrets(); if (newSecrets == null) { throw new IllegalStateException( From 323617251e9f761085fa6156ccdaeec3c887c5b4 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Tue, 12 Nov 2024 13:37:52 -0800 Subject: [PATCH 13/13] passing a test --- .../apache/polaris/service/admin/PolarisAuthzTestBase.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisAuthzTestBase.java b/polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisAuthzTestBase.java index f0a6d50dfb..6fc24a2b2a 100644 --- a/polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisAuthzTestBase.java +++ b/polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisAuthzTestBase.java @@ -325,8 +325,8 @@ public void after() { callContext.getPolarisCallContext(), credentials.getClientId(), lookupEntity.getEntity().getId(), - credentials.getClientSecret(), - false); + false, + credentials.getClientSecret()); // This should actually be the secret's hash return new PrincipalEntity( PolarisEntity.of(