apache / apache/polaris

[BUG] rotatePrincipalSecrets in JdbcBasePersistenceImpl silently drops credentials on concurrent rotation

Open
#5,503 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.