Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not persist plaintext secrets in the metastore #438

Merged
merged 16 commits into from
Nov 14, 2024
Merged

Do not persist plaintext secrets in the metastore #438

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
94f7a03
initial commit
eric-maynard Nov 7, 2024
5bc2530
testing with postgres
eric-maynard Nov 7, 2024
8cde7a5
reverts
eric-maynard Nov 7, 2024
54f01ce
test fixes
eric-maynard Nov 8, 2024
cfae372
autolint
eric-maynard Nov 8, 2024
0d085b3
add a test
eric-maynard Nov 8, 2024
3cc38a8
autolint
eric-maynard Nov 8, 2024
18d5377
typofix
eric-maynard Nov 8, 2024
3e2b0f8
Merge branch 'main' into hash-secrets
eric-maynard Nov 12, 2024
7f490b0
idempotent again
eric-maynard Nov 12, 2024
0c054f5
stable
eric-maynard Nov 12, 2024
8e0a752
autolint
eric-maynard Nov 12, 2024
203d534
Merge branch 'hash-secrets' of github.com-oss:eric-maynard/polaris in…
eric-maynard Nov 12, 2024
b856e9b
fix another place
eric-maynard Nov 12, 2024
3236172
passing a test
eric-maynard Nov 12, 2024
bf648ec
Merge branch 'main' into hash-secrets
eric-maynard Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions ...olaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -701,9 +701,9 @@ public int lookupEntityGrantRecordsVersion(
principalSecrets.getPrincipalId());

// rotate the secrets
principalSecrets.rotateSecrets(mainSecretToRotate);
principalSecrets.rotateSecrets(oldSecretHash);
if (reset) {
principalSecrets.rotateSecrets(principalSecrets.getMainSecret());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the reason we passed in the original secret was for idempotence. It's imperative to make sure that if a rotate request is sent twice accidentally (e.g., client retry), the keys aren't rotated twice

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good idea, we should probably document that on the method if it isn't already

principalSecrets.rotateSecrets(principalSecrets.getMainSecretHash());
}

// write back new secrets
Expand Down
77 changes: 77 additions & 0 deletions ...olaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,25 @@
*/
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 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.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.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;
Expand Down Expand Up @@ -80,6 +86,77 @@ 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(principalSecrets.getMainSecretHash());
Assertions.assertNotEquals(originalHash, principalSecrets.getMainSecretHash());
Assertions.assertEquals(originalHash, principalSecrets.getSecondarySecretHash());
Assertions.assertEquals(null, 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<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
Expand Down
83 changes: 74 additions & 9 deletions polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrincipalSecrets.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
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
Expand All @@ -37,12 +38,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
*
Expand All @@ -64,40 +73,78 @@ private String generateRandomHexString(int stringLength) {
return sb.toString();
}

private String hashSecret(String secret) {
return DigestUtils.sha256Hex(secret + ":" + secretSalt);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is probably advisable to use different salt values for each secret (for the same reason why different salt values are used for different users).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am actually not clear on why/if we need two secrets per user.

If we turn out to only want to use one, then just having the one salt is fine. If we are really using both secrets, then probably having two salts for the two secrets is better.

}

@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) {
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();
}

public PolarisPrincipalSecrets(long principalId) {
this.principalId = 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;
/** Rotate the main secrets */
public void rotateSecrets(String newSecondaryHash) {
this.secondarySecret = null;
this.secondarySecretHash = newSecondaryHash;

this.mainSecret = this.generateRandomHexString(32);
this.mainSecretHash = hashSecret(mainSecret);
}

public long getPrincipalId() {
Expand All @@ -108,11 +155,29 @@ public String getPrincipalClientId() {
return principalClientId;
}

public boolean matchesSecret(String potentialSecret) {
String potentialSecretHash = hashSecret(potentialSecret);
return potentialSecretHash.equals(this.mainSecretHash)
|| potentialSecretHash.equals(this.secondarySecretHash);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to support rolling upgrades (when requests may submit the old plain text secrets for validation)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, I am not clear on this. I put a note in the description, but I basically copied what I saw as the existing behavior. I am open to keeping/changing this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like an easy enough thing to add for now to prevent a backwards incompatibility, but maybe we should just only do so if the "hash" is not set? Then the moment hash is set only accept a hashed comparison?

Copy link
Contributor

@dimas-b dimas-b Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, clients will take time to update their configuration. How about we allow plain test password even when a hash is set, but until the next password rotation? Meaning: remove plain text passwords during rotation if hash is set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is just an idea from my side. I do not mean to say that supporting plain text passwords and rolling upgrades are required from my POV :) )

Copy link
Contributor Author

@eric-maynard eric-maynard Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way it works right now is this:

  1. As soon as you rotate-credentials, the plaintext secret is wiped from the metastore. Atomically with this, the hash of that secret is added as the secondary hash
  2. You can still authenticate with the old credential (it checks against that hash) until you rotate-credentials again, at which point the secondary secret is gone. The client still sends the full secret, but it's not persisted.

I think this is the correct behavior iff we want to have secrets be valid after one rotate-credentials but not after two.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eric-maynard : Apologies, I misinterpreted the code. You're right in how it works. I think the PR is good to merge.

}

public String getMainSecret() {
return mainSecret;
}

public String getSecondarySecret() {
return secondarySecret;
}

public String getMainSecretHash() {
return mainSecretHash;
}

public String getSecondarySecretHash() {
return secondarySecretHash;
}

public String getSecretSalt() {
return secretSalt;
}
}
4 changes: 2 additions & 2 deletions ...rc/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,8 @@ public void setStorageIntegrationProvider(PolarisStorageIntegrationProvider stor
polarisContext,
secrets.getPrincipalClientId(),
secrets.getPrincipalId(),
secrets.getMainSecret(),
false);
false,
secrets.getMainSecretHash());
return rotatedSecrets;
}

Expand Down
6 changes: 3 additions & 3 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,19 +547,19 @@ 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
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 {
Expand Down
15 changes: 8 additions & 7 deletions ...s-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -954,7 +954,7 @@ public Map<String, String> 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());

Expand Down Expand Up @@ -1012,8 +1012,8 @@ public Map<String, String> 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);
Expand All @@ -1033,7 +1033,7 @@ public Map<String, String> 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(
Expand Down Expand Up @@ -1061,8 +1061,8 @@ public Map<String, String> 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();

Expand All @@ -1071,7 +1071,8 @@ public Map<String, String> 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)
Expand Down
6 changes: 3 additions & 3 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreSession.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading