Repository navigation
Store the size of aes-generated key providers created before the default changed - #54035
choksi2212 wants to merge 1 commit into
Conversation
…ult changed Providers created before 26.7.0 have no secretSize configuration, because the value they were generated with was the default at the time. The default grew from 16 to 32 in keycloak#50250, so the admin console shows the new default for them, and saving such a provider without any change rotates the realm AES key: AbstractGeneratedSecretKeyProviderFactory sees the sent size differ from the current secret length and generates a new key with a new kid, which also invalidates existing KC_RESTART cookies. A change set in 26.8.0 now records the actual size of every aes-generated provider that has a secret but no secretSize, so the console shows the real size and saving is a no-op. Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The focused migration preserves key material and configured sizes, includes regression coverage, and has no identified blocking issues.
0 open findings
What changed in this PR
Fixes #54016 by storing actual AES key sizes for existing providers, preventing unintended key rotation when administrators save unchanged settings.
Changes:
- Backfills missing
secretSizevalues without changing existing keys. - Registers the migration in the 26.8.0 changelog.
- Adds regression coverage for legacy providers and excluded configurations.
| File | Description |
|---|---|
| model/jpa/src/test/java/org/keycloak/connections/jpa/updater/liquibase/custom/JpaUpdate26_8_0_AesGeneratedSecretSizeTest.java | Tests size backfilling and preservation of existing configuration. |
| model/jpa/src/main/resources/META-INF/jpa-changelog-26.8.0.xml | Registers the migration. |
| model/jpa/src/main/java/org/keycloak/connections/jpa/updater/liquibase/custom/JpaUpdate26_8_0_AesGeneratedSecretSize.java | Derives missing sizes from stored AES secrets. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Gentle nudge on this one. The full CI matrix has been green since Oct 16 and there are no open findings on the review. Anything you'd like changed before it can be considered for merge? Happy to adjust the scope or rebase if that helps. |
|
@choksi2212 Thanks for the fix, it seems good. Could we also test saving the provider after migration and assert that its secret and kid stay unchanged? The current test verifies the backfilled size, but that extra step would cover the silent rotation reported in the issue. @mposolda Can you please review it as well? It's a DB update. |
|
@jimmychakkalakal @pskopek @graziang if you have more time then Marek, you can review it instead, thanks for help 👍 |
There was a problem hiding this comment.
@choksi2212 Thanks for the PR.
In case when updating of some component configuration is needed when migrating to newer version, we try to use Java migrators instead of the generic liquibase migrators.
Java migrators are easier to use and less fragile (due not containing low level DB SQL stuff). The DB migrators are used typically just in cases when using java migrators is impossible or very uneffective.
For the inspiration, you can look for example at the MigrateTo26_7_0 or MigrateTo26_8_2.addPolicyIfMissing . Also might be good to do integration test for the migration rather than low-level "unit test" . For the inspiration, please see for example 512331e9
|
will be fixed in #54910 |
Fixes #54016
The problem
Since #49858 / #50250 (26.7.0) the default
secretSizeofaes-generatedis 32 instead of 16. Providers created before 26.7.0 store nosecretSize, only the 16-byte secret they were generated with. For such a provider the admin console shows the new default, and clicking Save without any change sendssecretSize=32:AbstractGeneratedSecretKeyProviderFactory.validateConfigurationthen seessize(32) differ from the current secret length (16), generates a new 256-bit key with a newkid, and26.7.0-45292-realm-display-name-remove-attribute-style silent rotation follows — KC_RESTART cookies issued before it can no longer be decrypted, as described in the issue.The server is already defensive when no size is sent (
size = model.get(SECRET_SIZE_KEY, currentSize)pins the current length, and the third branch even persists it when it differs from the default) — the console defeats it by always sending the default it displayed.The fix
Implements the first option from the issue: a
26.8.0change set stores the actual key size of everyaes-generatedprovider that has a secret but nosecretSizeconfiguration. The size is computed by decoding the stored secret (unpadded base64url, exactly asAbstractGeneratedSecretKeyProviderFactorywrote it), so the value stored is the real number of key bytes rather than an assumption about the old default. After the migration the console shows the real size and saving without changes is a no-op.Details:
JpaUpdate26_8_0_AesGeneratedSecretSizefollows the house pattern of the other custom tasks (SELECTviaPreparedStatement, writes viaRawParameterizedSqlStatementwith the values bound as JDBC parameters). It is not realm-scoped: key providers of every realm are covered.secretSize(created or saved after 26.7.0) are excluded by aNOT EXISTSsubquery; providers whose secret was never generated (never used fallbacks) are skipped; components of other provider types (hmac-generated, whose default did not change) are never touched; the storedsecret/kid/priorityvalues are never modified. A singlesecretSizerow per component is guaranteed.26.8.0-54016-aes-generated-secret-sizeinjpa-changelog-26.8.0.xml. Because the id is new, it runs for everyone upgrading past 26.8.0, including realms that already went through 26.7.x while carrying the un-rotated legacy state.secretSize, pinned only in memory by the third branch ofvalidateConfiguration) is backfilled to 32 — its real size — which is exactly what the pin would store on the next save.Verification
JpaUpdate26_8_0_AesGeneratedSecretSizeTestdrives the task through liquibase's own executor on H2 with the productionNON_KEYWORDS=VALUEURL parameter, mirroring how the change set runs at startup, and covers: a pre-26.7 provider (16 ->secretSize=16, the 22-character base64url encoding pinned as in the issue), a second realm's pre-26.7 provider, a post-26.7 provider left untouched with exactly onesecretSizerow, anaes-generatedprovider with a 24-byte secret backfilled to 24, a fallback without a secret left alone, anhmac-generatedprovider left alone,secret/kidrows never modified, onesecretSizerow per component, and the confirmation message count. Fullmvn -pl model/jpa testis 9/9 andspotless:checkis clean; the changelog parses as well-formed XML.The rotation itself is not re-proven here — the issue's database-checked repro covers it end to end, and
validateConfiguration's regeneration path is visible atAbstractGeneratedSecretKeyProviderFactory(generateSecretoncurrentSize != size). What this PR proves is that after the change set, the stored state makes that comparison an equality (16 == 16), which the tests pin.The console-side alternative from the issue (not prefilling a default the component has no value for) would also stop the silent rotation and could complement this change, but the migration additionally makes the stored state self-describing for every client, and it is the reporter's first suggested option.