Do not persist plaintext secrets in the metastore #438
Conversation
eric-maynard
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau,
jackye1995,
flyrain,
collado-mike,
ebyhr and
adutra
as code owners
| @@ -64,40 +73,78 @@ private String generateRandomHexString(int stringLength) { | |||
| return sb.toString(); | |||
| } | |||
|
|
|||
| private String hashSecret(String secret) { | |||
| return DigestUtils.sha256Hex(secret + ":" + secretSalt); | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| public boolean matchesSecret(String potentialSecret) { | ||
| String potentialSecretHash = hashSecret(potentialSecret); | ||
| return potentialSecretHash.equals(this.mainSecretHash) | ||
| || potentialSecretHash.equals(this.secondarySecretHash); |
There was a problem hiding this comment.
Do we want to support rolling upgrades (when requests may submit the old plain text secrets for validation)?
There was a problem hiding this comment.
+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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(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 :) )
There was a problem hiding this comment.
The way it works right now is this:
- 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 - You can still authenticate with the old credential (it checks against that hash) until you
rotate-credentialsagain, 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.
There was a problem hiding this comment.
@eric-maynard : Apologies, I misinterpreted the code. You're right in how it works. I think the PR is good to merge.
|
The reason for two secrets is to support period rotation. If only one secret is valid, then an engineer can't rotate the secrets and push the new secret out to production in time. Instead, they can rotate, get the new secret and push to production while the old secrets still functions. |
| if (reset) { | ||
| principalSecrets.rotateSecrets(principalSecrets.getMainSecret()); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
That is a good idea, we should probably document that on the method if it isn't already
|
Hm - @eric-maynard looks like this change broke |
|
Example failure: |
|
Hi @snazy, do you see consistent failures? I was not able to repro: |
|
I wonder if your test is the result of what happens when you have an existing deployment already? Since it looks like the new column is totally missing. However I manually tested this scenario, and I saw the column get added. Let me know if you find out any more details about the error(s) you're seeing. If you want to revert while we debug, let me know and we can do that. |
|
It's definitely an issue. Not saying, the root-cause is this change, but there's something odd. I've opened #450 for this. |
|
@eric-maynard what's the plan for schema changes on any tables in Polaris? Right now I don't see any DDL scripts to handle this additional |
|
Hi @johnnysohn, thanks for raising this. I think we probably should figure out how to do metastore versioning at some point, but in the immediate term I would like to unblock anyone impacted by this change. I think the issue is ultimately metastore-dependent and maybe even database-dependent. Of course, you could manually alter the schema of your database. But I have done some reading and I believe this EclipseLink parameter could resolve the issue: If you are affected and able to confirm this works for you, we can consider just adding it to the default |
|
@eric-maynard I was using the principal_secrets table to find out the client ID and secret of the root principal, to be able to use the OAuth2 API. Where can I find it now? |
|
Hi @shantanu-dahiya -- please see this comment or #450 for further discussion |
Description
Currently, the entire
PolarisPrincipalSecretsgets passed into the metastore for persistence. For EclipseLink, this means it gets translated into aModelPrincipalSecretswhich then gets written to the DB. Unfortunately, this means the plaintext client secrets are being persisted. These are then later used to check if provided secrets are valid.This PR proposes that we persist a salted hash of these secrets, and that we no longer persist the plaintext secrets.
Important: Is it intended that after rotation secrets are still valid?
Based on what I observed, it seems like we check secrets against both the main and secondary secret. If that's not intended it's easy enough to fix but this PR is a good time to do it since we are changing the semantics of the secret check.
Fixes #219
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added a unit test in
PolarisEclipseLinkMetaStoreManagerTestcovering rotation for old secrets.Manual Testing:
I am able to create new principals and use them without persisted plaintext secrets.
Coming back to the new (this PR) version, I am able to authenticate principals using both the new + old formats.
rotate-credentialsthe principal created with an old version of Polaris, which still has plaintext credentials, the hash gets added:Note that the persisted secret doesn't go away, but it shouldn't be used anymore.
Checklist:
Please delete options that are not relevant.