[BUG] rotatePrincipalSecrets in JdbcBasePersistenceImpl silently drops credentials on concurrent rotation
- Dominant language
- Java
- Stars
- 2.1k
- Forks
- 522
- Avg merge
- 1d 17h
- Merged PRs (30d)
- 137
Description
### Describe the bug
In `JdbcBasePersistenceImpl.rotatePrincipalSecrets`, the SQL `UPDATE` statement executed against the `PRINCIPAL_AUTHENTICATION_DATA` table only includes `principal_client_id` and `realm_id` in its `WHERE` clause:
```java
Map params = Map.of("principal_client_id", clientId, "realm_id", realmId);
datasourceOperations.executeUpdate(
QueryGenerator.generateUpdateQuery(
ModelPrincipalAuthenticationData.ALL_COLUMNS,
ModelPrincipalAuthenticationData.TABLE_NAME,
modelPrincipalAuthenticationData
.toMap(datasourceOperations.getDatabaseType())
.values()
.stream()
.toList(),
params));
```
This omits the caller's expected `oldSecretHash` from the `WHERE` clause and completely ignores the integer return value of `datasourceOperations.executeUpdate(...)`.
When two concurrent processes (e.g. automated secret rotation jobs, multi-replica Kubernetes external-secret operators, or an automated rotation racing an admin reset) rotate the principal's secrets at the same time:
1. Both read the same initial secret hash.
2. Both generate new credentials and issue unversioned `UPDATE` queries.
3. The second update silently overwrites the first update in the database.
4. The first worker receives HTTP 200 OK containing a credential that was never persisted or was immediately erased from the database.
5. The first worker fails all subsequent authentication with `401 Unauthorized`.
### To Reproduce
1. Initialize a principal with secrets (`storePrincipalSecrets`).
2. Simulate two concurrent rotation calls (Worker A and Worker B) starting from the same `initialHash`.
3. Worker A calls `rotatePrincipalSecrets(..., initialHash)` and receives `secretA`.
4. Worker B calls `rotatePrincipalSecrets(..., initialHash)` and receives `secretB`.
5. Check `loadPrincipalSecrets(...)`: `secretB` is present, but `secretA` is completely absent from both main and secondary secrets.
6. Worker A cannot authenticate with `secretA`.
### Actual Behavior
Worker A is returned a ghost credential with HTTP 200 OK. The credential is not present in the database, resulting in immediate authentication failures and breaking the zero-downtime rolling secret rotation guarantee.
### Expected Behavior
`rotatePrincipalSecrets` should execute an atomic Compare-And-Swap (CAS) update by including `main_secret_hash = oldSecretHash` in the `WHERE` clause and checking `rowsUpdated == 1`. If `rowsUpdated == 0`, it should throw `RetryOnConcurrencyException` so the conflicting caller can retry against the updated state rather than returning invalid credentials.
### Additional context
- This behavior was reproduced with an in-tree unit test on H2 in `JdbcBasePersistenceImplTest`.
- The fix aligns with `writeEntity` in `JdbcBasePersistenceImpl.java` which also checks `if (rowsUpdated == 0) throw new RetryOnConcurrencyException(...)`.
- Fix and tests are ready; PR to follow.
### System information
OS: Linux / macOS / Windows
Polaris Catalog Version: main (commit 4812619037+)
Persistence: Relational JDBC (polaris-relational-jdbc)
Contributor guide
Research direction
Start in JdbcBasePersistenceImpl.java at rotatePrincipalSecrets, then compare its update handling with writeEntity and inspect the H2 coverage in JdbcBasePersistenceImplTest. Run the relevant unit test first; done means concurrent rotations use the expected old hash, handle the update count, and cover the retry-on-conflict behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- database
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100