Increase internal algorithm security using HS512 and 128 byte hmac keys #26957
Conversation
There was a problem hiding this comment.
-
Added one nitpick comment.
-
Besides that, it seems that there are some conflicts in the meantime.
-
Did some search for the
Algorithm.HS256and found few more places using that as a default. Wonder if we should try to update those as well when we are at this? Some of the places I found (might missed some):Attributes.HS_ALGORITHM_PROPERTY(This probably causes that HS256 is still pre-selected as the default algorithm in the UI for the configuration properties of some providers)AbstractOAuth2IdentityProvider.getSignatureContext()- looks this one is also documented here https://www.keycloak.org/docs/latest/server_admin/index.html#_identity_broker_oidc (see "client assertion signature algorithm" section), so might change the docs tooCIBAAuthenticationRequest- but here I am not 100% sure if CIBA specification explicitly mentions HS256 should be the default, so might be good to doublecheck...
-
Do we also want to mention this a bit in
changes-24_0_0.adoc? It is internal in theory, but still can be nice to mention this, so people are aware
Finally, it may be probably good to add some minor update to
services/src/main/java/org/keycloak/jose/jws/DefaultTokenManager.java
Outdated
Show resolved
Hide resolved
c4d61ea to
433ec08
Compare
I have changed to set HS512 as the default one now.
I have not done this point. Changing this can break current configurations and this is really not related to internal keycloak signature. This is just using the client secret to create a JWT for the broker authentication, any other HS algorithm can be configured and it's an admin decision. If you think it's important I will do it but take in mind that configurations with no alg specified are changing from HS256 to HS512 and can break.
Done! I have checked that this direct signature is used to create the
Yep, for sure. I'll do when the other comments are OK. |
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.model.session.OfflineSessionPersistenceTest#testPersistenceMultipleNodesClientSessionsAtRandomNodeKeycloak CI - Store Model Tests |
|
@rmartinc Thanks for the updates! So perhaps only migration docs and then we're good to go? |
Closes keycloak#13080 Signed-off-by: rmartinc <rmartinc@redhat.com>
433ec08 to
34900aa
Compare
|
Done! I added a little upgrading section explaining we have changed from HS256 to HS512 for internal tokens. If tests are OK tomorrow will move it out of draft. |
|
@andymunro Do you please have a chance to review the documentation part of this PR? |
|
@kami619 - this change might (will?) affect Keycloak's performance. Once this is merged and available in a nightly release, please keep an eye on the nightly KCB runs if we need to update our sizing guide. Please add a new issue to the Keycloak project so we can track this, and update that issues's milestone for KC24 or KC25 depending on when this is being merged. Thanks! |
|
@ahus1 thanks, I will track this. |
Closes #13080
PR that increase security in default internal algorithm (used by actions, refresh tokens,...). As discussed in the issue the default hmac size has been updated from 64 to 128 bytes and the internal algorithm has changed from HS256 to HS512. Things to consider:
hmac-generated-hs512instead of previoushmac-generated. I decided to do that to differentiate the provider names in migrations. But maybe we prefer to maintain the same name or rename old HS256 for compatibility.It's just a draft for now. We also need to decide if we want this in keycloak 24 or we wait for 25. For the moment the upgrade task is for 24 but it can be moved to 25. An upgrading note commenting the final changes is also needed before moving out from draft.