fix(auth): bind internal JWTs to principal secret generation#5053
fix(auth): bind internal JWTs to principal secret generation#5053vigneshio wants to merge 2 commits into
Conversation
5925d16 to
1a98a23
Compare
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for your diligence, @vigneshio ! The general idea is very reasonable. Some implementation comments below.
| * secrets are rotated or reset, {@code mainSecretHash} changes and tokens carrying a prior value | ||
| * are rejected on verify. | ||
| */ | ||
| @VisibleForTesting static final String CLAIM_KEY_CREDENTIALS_VERSION = "cv"; |
There was a problem hiding this comment.
Per RFC 7519 this has to be a "collision-resistant" name.
How about polaris-cv?
There was a problem hiding this comment.
Done.. renamed to polaris-cv.
| principal.entity().getId(), | ||
| clientId, | ||
| scope, | ||
| principal.mainSecretHash()); |
There was a problem hiding this comment.
I'm not sure exposing secret hashes in JWTs is a good idea. These JWTs are not encrypted.
I believe a more robust approach would be to generate a separate number for this. Perhaps it can be handled inside PolarisPrincipalSecrets by adjusting the processing of existing fields in a backward-compatible way.
| throw new NotAuthorizedException("Failed to verify the token"); | ||
| } | ||
| if (credentialsVersion == null || credentialsVersion.isBlank()) { | ||
| throw new NotAuthorizedException("Failed to verify the token"); |
There was a problem hiding this comment.
This may be too strong. This will break all clients on Polaris upgrade.
I believe a more robust option would be to enforce "cv" after the first credential reset/rotation that happens in the new Polaris version.
There was a problem hiding this comment.
Agreed, 👀 that was too aggressive 😅 fixed - tokens without the claim are just accepted now (treated as pre-binding legacy), so nothing breaks on upgrade. everything minted after the upgrade carries the claim and dies on the next rotate/reset.
on the "enforce after first rotation" idea - that needs per-principal state to tell "rotated after upgrade" from "never rotated", and jdbc keeps secrets in plain columns so that's a schema change. trade-off i took instead: old claim-less tokens stay valid till expiry (max 1h by default), all new tokens are fully bound. can do the strict per-principal version as a follow-up if you think the window matters. WDYT ? @dimas-b
There was a problem hiding this comment.
I think it's fine to re-mint without a forced rotation. The old token does not have a long lifespan and the new token will have "cv" data.
Embed a credentials-generation fingerprint (polaris-cv claim) in minted access tokens. The version is derived from the principal's current main secret hash (hash of the hash), so the secret-verification artifact itself is never exposed in the unencrypted token and no schema change is needed. On verify (and thus token exchange), tokens whose version no longer matches are rejected, so credential rotate/reset invalidates outstanding sessions without waiting for token expiry. Tokens minted before this change carry no claim and remain valid until expiry for upgrade compatibility; they become bound on the next rotate/reset.
Access tokens are bound to per-realm principal secret hashes. Reusing a POLARIS admin token against POLARIS2 correctly returns 401. Bootstrap both realms with known credentials and obtain a POLARIS2 token for the cross-realm rate-limit isolation assertion.
1a98a23 to
083f99e
Compare
| */ | ||
| @Nullable | ||
| public String getCredentialsVersion() { | ||
| return mainSecretHash == null ? null : DigestUtils.sha256Hex(mainSecretHash); |
There was a problem hiding this comment.
I think we should salt this hash for good measure.
Also, "version" processing should respect the "secondary" hash too.
2 participants
Internal JWTs are bound to the principal's current main secret hash (
cvclaim). After credential rotate or reset, previously issued access tokens are rejected on verify so sessions cannot outlive a secret compromise response.Tokens minted before this change (missing
cv) are also rejected and must be reissued via client credentials.No schema migration: the fingerprint is the existing
mainSecretHash.