Repository navigation
Increased AES default from 128 to 256 - #50250
Conversation
Signed-off-by: Marie Daly <marie.daly1@ibm.com>
There was a problem hiding this comment.
Pull request overview
This PR updates Keycloak’s generated AES key provider default from AES-128 (16 bytes) to AES-256 (32 bytes) and adds an integration test intended to ensure KC_RESTART cookies encrypted with the old AES-128 key remain decryptable after introducing an AES-256 key.
Changes:
- Increase
GeneratedAesKeyProviderFactorydefault secret size from 16 to 32 bytes (AES-256). - Add a new
RestartCookieTestscenario to exercise AES-128 → AES-256 migration/backwards compatibility for KC_RESTART cookies.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RestartCookieTest.java | Adds a migration/backwards-compatibility test for KC_RESTART cookies across AES key-size change. |
| services/src/main/java/org/keycloak/keys/GeneratedAesKeyProviderFactory.java | Switches the generated AES key default size to 32 bytes (AES-256). |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.cluster.RealmInvalidationClusterTest#crudWithFailoverKeycloak CI - Store IT (mariadb) |
Signed-off-by: Marie Daly <marie.daly1@ibm.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
services/src/main/java/org/keycloak/keys/GeneratedAesKeyProviderFactory.java:51
- Now that the default is AES-256 (32 bytes), the config help text should be updated to clarify the 128/192/256 mapping and to fix the grammar in the warning ("than", not "then"). The current warning is also easy to misread as saying AES-256 is unsupported in general, even though it is the new default.
private static final int DEFAULT_AES_KEY_SIZE = 32;
static {
AES_KEY_SIZE_PROPERTY = new ProviderConfigProperty(Attributes.SECRET_SIZE_KEY, "AES Key size",
"Size in bytes for the generated AES Key. Size 16 is for AES-128, Size 24 for AES-192 and Size 32 for AES-256. WARN: Bigger keys then 128 bits are not allowed on some JDK implementations",
514d53c to
a20dc69
Compare
Signed-off-by: Marie Daly <marie.daly1@ibm.com>
There was a problem hiding this comment.
@msdaly200 Changes LGTM! There should not be any breaking changes either based on my lookup. There should not be any performance issue as well, as the AES key is used mainly for internal cookies and tokens with small payloads (like CIBA authn request).
We should also include a migration guide stating that existing realms are not affected + maybe migration path even for them?
Something like:
"""
=== Default AES key size increased to 256 bits
The default key size for newly generated AES key providers (aes-generated) has been increased from 128 bits to 256 bits for improved security. Existing realms are not affected - only newly created key providers use the new default.
To migrate an existing realm, add a new aes-generated key provider with key size 32 and a higher priority. The old provider can be removed once all sessions using the old key have expired.
"""
There was a problem hiding this comment.
| "Size in bytes for the generated AES Key. Size 16 is for AES-128, Size 24 for AES-192 and Size 32 for AES-256.", |
Nitpick: It'd be good to update this help message. Based on my lookup, it should not be relevant anymore since Java 9+ (should be an issue with the old JCE Unlimited Strength Jurisdiction Policy requirement).
Signed-off-by: Marie Daly <marie.daly1@ibm.com>
There was a problem hiding this comment.
@msdaly200 LGTM, thanks!
Btw. There might be some KC conventions for the commit names - the best would be git commit -m "Increased AES default from 128 to 256" -m "Closes XXX" -s (note the 'Closes' as sub text). But it's just recommendation
10BC8
for the future.
@rmartinc Could you please do the doubl-check? Thanks!
There was a problem hiding this comment.
Thanks @msdaly200! I have been thinking about this and if there could be any issue and I didn't find anything. The new realms uses AES-256 and the old ones maintains the AES-128. So I think we are OK with the change.
Just a little suggestion for the note to mention PQC and the reasons for the change.
Closes 49858 Signed-off-by: Marie Daly <marie.daly1@ibm.com>
thanks @mabartos for calling that out. I wasn't aware, will do that going forward. |
| // Verify cookie uses AES128 encoding | ||
| String aes128Header = aes128Cookie.split("\\.")[0]; | ||
| String aes128HeaderJson = new String(java.util.Base64.getUrlDecoder().decode(aes128Header), StandardCharsets.UTF_8); | ||
| Assertions.assertTrue(aes128HeaderJson.contains("A128GCM"), "Expected pre-migration cookie to use A128GCM (AES-128)"); | ||
|
|
| // Verify new cookie uses A256GCM encryption | ||
| String jweHeader = aes256Cookie.split("\\.")[0]; | ||
| String headerJson = new String( | ||
| java.util.Base64.getUrlDecoder().decode(jweHeader), | ||
| StandardCharsets.UTF_8 | ||
| ); | ||
| Assertions.assertTrue(headerJson.contains("A256GCM"), | ||
| "New cookie should use A256GCM encryption after migration"); | ||
|
|
| To migrate an existing realm, add a new aes-generated key provider with key size 32 and a higher priority. | ||
| The old provider can be removed once all sessions using the old key have expired. |
* Closes keycloak#49858, updated to use AES256 Signed-off-by: Marie Daly <marie.daly1@ibm.com> * Closes keycloak#49858, updated following copilot comments Signed-off-by: Marie Daly <marie.daly1@ibm.com> * Closes keycloak#49858, updated priority values Signed-off-by: Marie Daly <marie.daly1@ibm.com> * Closes keycloak#49858, updated following comments Signed-off-by: Marie Daly <marie.daly1@ibm.com> * Increased AES default from 128 to 256 Closes 49858 Signed-off-by: Marie Daly <marie.daly1@ibm.com> --------- Signed-off-by: Marie Daly <marie.daly1@ibm.com>
Closes #49858
Increased DEFAULT_AES_KEY_SIZE from 16 (AES-128) to 32 (AES-256) for post-quantum security. Added test to validate AES-128 cookies decrypt correctly after AES-256 upgrade.