Skip to content

Commit

Permalink
Eliminate persistence of secrets entirely (#522)
Browse files Browse the repository at this point in the history 
Clear text secrets must never be persisted, hence the attributes in `ModelPrincipalSecrets` needs to be removed.
  • Loading branch information
@snazy
snazy authored Dec 10, 2024
1 parent 99e128a commit a2cdcf8
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 67 deletions.
106 changes: 75 additions & 31 deletions ...olaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@

import static jakarta.persistence.Persistence.createEntityManagerFactory;
import static org.apache.polaris.core.persistence.PrincipalSecretsGenerator.RANDOM_SECRETS;
import static org.assertj.core.api.Assertions.assertThat;
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 org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.PolarisConfigurationStore;
Expand Down Expand Up @@ -91,17 +91,41 @@ void testCreateStoreSession(String confFile, boolean success) {
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());
var newSecrets = new PolarisPrincipalSecrets(42L);
assertThat(newSecrets)
.extracting(
PolarisPrincipalSecrets::getMainSecret,
PolarisPrincipalSecrets::getSecondarySecret,
PolarisPrincipalSecrets::getMainSecretHash,
PolarisPrincipalSecrets::getSecondarySecretHash,
PolarisPrincipalSecrets::getSecretSalt)
.doesNotContainNull()
.allMatch(x -> !x.toString().isEmpty());

ModelPrincipalSecrets model = ModelPrincipalSecrets.fromPrincipalSecrets(newSecrets);
var key = model.getPrincipalClientId();

var fromModel = ModelPrincipalSecrets.toPrincipalSecrets(model);

assertThat(fromModel)
.extracting(
PolarisPrincipalSecrets::getMainSecret, PolarisPrincipalSecrets::getSecondarySecret)
.containsOnlyNulls();

assertThat(model)
.extracting(
ModelPrincipalSecrets::getPrincipalId,
ModelPrincipalSecrets::getPrincipalClientId,
ModelPrincipalSecrets::getSecretSalt,
ModelPrincipalSecrets::getMainSecretHash,
ModelPrincipalSecrets::getSecondarySecretHash)
.containsExactly(
newSecrets.getPrincipalId(),
newSecrets.getPrincipalClientId(),
newSecrets.getSecretSalt(),
newSecrets.getMainSecretHash(),
newSecrets.getSecondarySecretHash());

try (var emf = createEntityManagerFactory("polaris")) {
var entityManager = emf.createEntityManager();
Expand All @@ -115,31 +139,54 @@ void testRotateLegacyPrincipalSecret() {
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());
assertThat(retrievedModel)
.extracting(
ModelPrincipalSecrets::getPrincipalId,
ModelPrincipalSecrets::getPrincipalClientId,
ModelPrincipalSecrets::getSecretSalt,
ModelPrincipalSecrets::getMainSecretHash,
ModelPrincipalSecrets::getSecondarySecretHash)
.containsExactly(
model.getPrincipalId(),
model.getPrincipalClientId(),
model.getSecretSalt(),
model.getMainSecretHash(),
model.getSecondarySecretHash());

// Now read using PolarisEclipseLinkStore
PolarisEclipseLinkStore store = new PolarisEclipseLinkStore(diagServices);
var store = new PolarisEclipseLinkStore(new PolarisDefaultDiagServiceImpl());
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());
assertThat(principalSecrets)
.extracting(
PolarisPrincipalSecrets::getPrincipalId,
PolarisPrincipalSecrets::getPrincipalClientId,
PolarisPrincipalSecrets::getSecretSalt,
PolarisPrincipalSecrets::getMainSecret,
PolarisPrincipalSecrets::getMainSecretHash,
PolarisPrincipalSecrets::getSecondarySecret,
PolarisPrincipalSecrets::getSecondarySecretHash)
.containsExactly(
fromModel.getPrincipalId(),
fromModel.getPrincipalClientId(),
fromModel.getSecretSalt(),
null,
fromModel.getMainSecretHash(),
null,
fromModel.getSecondarySecretHash());

// Rotate:
String originalSecret = principalSecrets.getMainSecret();
String originalHash = principalSecrets.getMainSecretHash();
principalSecrets.rotateSecrets(principalSecrets.getMainSecretHash());
Assertions.assertNotEquals(originalHash, principalSecrets.getMainSecretHash());
Assertions.assertEquals(originalHash, principalSecrets.getSecondarySecretHash());
Assertions.assertEquals(null, principalSecrets.getSecondarySecret());
assertThat(principalSecrets.getMainSecret()).isNotEqualTo(newSecrets.getMainSecret());
assertThat(principalSecrets.getMainSecretHash()).isNotEqualTo(newSecrets.getMainSecretHash());
assertThat(principalSecrets)
.extracting(
PolarisPrincipalSecrets::getSecondarySecret,
PolarisPrincipalSecrets::getSecondarySecretHash)
.containsExactly(null, newSecrets.getMainSecretHash());

// Persist the rotated credential:
store.deletePrincipalSecrets(entityManager, key);
Expand All @@ -148,13 +195,10 @@ void testRotateLegacyPrincipalSecret() {
// 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));
Assertions.assertTrue(reloadedSecrets.matchesSecret(newSecrets.getMainSecret()));
Assertions.assertFalse(reloadedSecrets.matchesSecret(newSecrets.getSecondarySecret()));
}
}

Expand Down
38 changes: 2 additions & 36 deletions ...sistence/jpa-model/src/main/java/org/apache/polaris/jpa/models/ModelPrincipalSecrets.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,6 @@ public class ModelPrincipalSecrets {
// the client id for that principal
@Id private String principalClientId;

// the main secret for that principal
private String mainSecret;

// the secondary secret for that principal
private String secondarySecret;

// Hash of mainSecret
private String mainSecretHash;

Expand All @@ -62,14 +56,6 @@ public String getPrincipalClientId() {
return principalClientId;
}

public String getMainSecret() {
return mainSecret;
}

public String getSecondarySecret() {
return secondarySecret;
}

public String getSecretSalt() {
return secretSalt;
}
Expand Down Expand Up @@ -103,16 +89,6 @@ public Builder principalClientId(String principalClientId) {
return this;
}

public Builder mainSecret(String mainSecret) {
principalSecrets.mainSecret = mainSecret;
return this;
}

public Builder secondarySecret(String secondarySecret) {
principalSecrets.secondarySecret = secondarySecret;
return this;
}

public Builder secretSalt(String secretSalt) {
principalSecrets.secretSalt = secretSalt;
return this;
Expand Down Expand Up @@ -151,8 +127,8 @@ public static PolarisPrincipalSecrets toPrincipalSecrets(ModelPrincipalSecrets m
return new PolarisPrincipalSecrets(
model.getPrincipalId(),
model.getPrincipalClientId(),
model.getMainSecret(),
model.getSecondarySecret(),
null,
null,
model.getSecretSalt(),
model.getMainSecretHash(),
model.getSecondarySecretHash());
Expand All @@ -164,17 +140,7 @@ 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;
}
}
}

0 comments on commit a2cdcf8

Please sign in to comment.