From d5e82356f90893ca3b308a7e10020103e402369a Mon Sep 17 00:00:00 2001 From: Giuseppe Graziano Date: Tue, 21 May 2024 08:29:17 +0200 Subject: [PATCH] Encrypted KC_RESTART cookie and removed sensitive notes Closes #keycloak/keycloak-private#162 Signed-off-by: Giuseppe Graziano --- docs/documentation/release_notes/index.adoc | 3 + .../release_notes/topics/24_0_5.adoc | 5 ++ .../jose/jws/DefaultTokenManager.java | 4 + .../keycloak/protocol/RestartLoginCookie.java | 38 +++++++- .../request/AuthzEndpointRequestParser.java | 5 ++ .../keycloak/testsuite/util/OAuthClient.java | 6 +- .../testsuite/forms/RestartCookieTest.java | 88 +++++++++++++++++++ .../keys/FallbackKeyProviderTest.java | 2 +- 8 files changed, 145 insertions(+), 6 deletions(-) create mode 100644 docs/documentation/release_notes/topics/24_0_5.adoc diff --git a/docs/documentation/release_notes/index.adoc b/docs/documentation/release_notes/index.adoc index 03d9b2eb6d..754fde73f7 100644 --- a/docs/documentation/release_notes/index.adoc +++ b/docs/documentation/release_notes/index.adoc @@ -16,6 +16,9 @@ include::topics/templates/release-header.adoc[] == {project_name_full} 25.0.0 include::topics/25_0_0.adoc[leveloffset=2] +== {project_name_full} 24.0.5 +include::topics/24_0_5.adoc[leveloffset=2] + == {project_name_full} 24.0.4 include::topics/24_0_4.adoc[leveloffset=2] diff --git a/docs/documentation/release_notes/topics/24_0_5.adoc b/docs/documentation/release_notes/topics/24_0_5.adoc new file mode 100644 index 0000000000..9af7da2adc --- /dev/null +++ b/docs/documentation/release_notes/topics/24_0_5.adoc @@ -0,0 +1,5 @@ += Security issue with PAR clients using client_secret_post based authentication + +This release contains the fix of the important security issue affecting some OIDC confidential clients using PAR (Pushed authorization request). In case you use OIDC confidential clients together +with PAR and you use client authentication based on `client_id` and `client_secret` sent as parameters in the HTTP request body (method `client_secret_post` specified in the OIDC specification), it is +highly encouraged to rotate the client secrets of your clients after upgrading to this version. diff --git a/services/src/main/java/org/keycloak/jose/jws/DefaultTokenManager.java b/services/src/main/java/org/keycloak/jose/jws/DefaultTokenManager.java index 3775084824..99eb998ce8 100644 --- a/services/src/main/java/org/keycloak/jose/jws/DefaultTokenManager.java +++ b/services/src/main/java/org/keycloak/jose/jws/DefaultTokenManager.java @@ -283,6 +283,8 @@ public class DefaultTokenManager implements TokenManager { public String cekManagementAlgorithm(TokenCategory category) { if (category == null) return null; switch (category) { + case INTERNAL: + return Algorithm.AES; case ID: case LOGOUT: return getCekManagementAlgorithm(OIDCConfigAttributes.ID_TOKEN_ENCRYPTED_RESPONSE_ALG); @@ -310,6 +312,8 @@ public class DefaultTokenManager implements TokenManager { switch (category) { case ID: return getEncryptAlgorithm(OIDCConfigAttributes.ID_TOKEN_ENCRYPTED_RESPONSE_ENC, JWEConstants.A128CBC_HS256); + case INTERNAL: + return JWEConstants.A128CBC_HS256; case LOGOUT: return getEncryptAlgorithm(OIDCConfigAttributes.ID_TOKEN_ENCRYPTED_RESPONSE_ENC); case AUTHORIZATION_RESPONSE: diff --git a/services/src/main/java/org/keycloak/protocol/RestartLoginCookie.java b/services/src/main/java/org/keycloak/protocol/RestartLoginCookie.java index d676a5121f..9432bfee92 100644 --- a/services/src/main/java/org/keycloak/protocol/RestartLoginCookie.java +++ b/services/src/main/java/org/keycloak/protocol/RestartLoginCookie.java @@ -23,13 +23,17 @@ import org.keycloak.Token; import org.keycloak.TokenCategory; import org.keycloak.cookie.CookieProvider; import org.keycloak.cookie.CookieType; +import org.keycloak.crypto.KeyUse; import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.services.managers.AuthenticationSessionManager; import org.keycloak.sessions.AuthenticationSessionModel; import org.keycloak.sessions.RootAuthenticationSessionModel; +import org.keycloak.util.TokenUtil; +import javax.crypto.SecretKey; +import java.nio.charset.StandardCharsets; import java.util.HashMap; import java.util.Map; @@ -118,7 +122,7 @@ public class RestartLoginCookie implements Token { public static void setRestartCookie(KeycloakSession session, AuthenticationSessionModel authSession) { RestartLoginCookie restart = new RestartLoginCookie(authSession); - String encoded = session.tokens().encode(restart); + String encoded = encodeAndEncrypt(session, restart); session.getProvider(CookieProvider.class).set(CookieType.AUTH_RESTART, encoded); } @@ -138,7 +142,7 @@ public class RestartLoginCookie implements Token { public static AuthenticationSessionModel restartSession(KeycloakSession session, RealmModel realm, RootAuthenticationSessionModel rootSession, String expectedClientId, String encodedCookie) throws Exception { - RestartLoginCookie cookie = session.tokens().decode(encodedCookie, RestartLoginCookie.class); + RestartLoginCookie cookie = decryptAndDecode(session, encodedCookie); if (cookie == null) { logger.debug("Failed to verify encoded RestartLoginCookie"); return null; @@ -169,6 +173,36 @@ public class RestartLoginCookie implements Token { return authSession; } + private static RestartLoginCookie decryptAndDecode(KeycloakSession session, String encodedToken) { + try { + String sigAlgorithm = session.tokens().signatureAlgorithm(TokenCategory.INTERNAL); + String algAlgorithm = session.tokens().cekManagementAlgorithm(TokenCategory.INTERNAL); + SecretKey encKey = session.keys().getActiveKey(session.getContext().getRealm(), KeyUse.ENC, algAlgorithm).getSecretKey(); + SecretKey signKey = session.keys().getActiveKey(session.getContext().getRealm(), KeyUse.SIG, sigAlgorithm).getSecretKey(); + + byte[] contentBytes = TokenUtil.jweDirectVerifyAndDecode(encKey, signKey, encodedToken); + String jwt = new String(contentBytes, StandardCharsets.UTF_8); + return session.tokens().decode(jwt, RestartLoginCookie.class); + } catch (Exception e) { + // Might be the cookie from the older version + return session.tokens().decode(encodedToken, RestartLoginCookie.class); + } + } + + private static String encodeAndEncrypt(KeycloakSession session, RestartLoginCookie cookie) { + try { + String sigAlgorithm = session.tokens().signatureAlgorithm(cookie.getCategory()); + String algAlgorithm = session.tokens().cekManagementAlgorithm(cookie.getCategory()); + SecretKey encKey = session.keys().getActiveKey(session.getContext().getRealm(), KeyUse.ENC, algAlgorithm).getSecretKey(); + SecretKey signKey = session.keys().getActiveKey(session.getContext().getRealm(), KeyUse.SIG, sigAlgorithm).getSecretKey(); + + String encodedJwt = session.tokens().encode(cookie); + return TokenUtil.jweDirectEncode(encKey, signKey, encodedJwt.getBytes(StandardCharsets.UTF_8)); + } catch (Exception e) { + throw new RuntimeException("Error encoding cookie.", e); + } + } + @Override public TokenCategory getCategory() { return TokenCategory.INTERNAL; diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestParser.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestParser.java index 8f696ea0ba..246d409783 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestParser.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestParser.java @@ -73,6 +73,11 @@ public abstract class AuthzEndpointRequestParser { // https://tools.ietf.org/html/rfc7636#section-6.1 KNOWN_REQ_PARAMS.add(OIDCLoginProtocol.CODE_CHALLENGE_PARAM); KNOWN_REQ_PARAMS.add(OIDCLoginProtocol.CODE_CHALLENGE_METHOD_PARAM); + + // Those are not OAuth/OIDC parameters, but they should never be added to the additionalRequestParameters + KNOWN_REQ_PARAMS.add(OAuth2Constants.CLIENT_ASSERTION_TYPE); + KNOWN_REQ_PARAMS.add(OAuth2Constants.CLIENT_ASSERTION); + KNOWN_REQ_PARAMS.add(OAuth2Constants.CLIENT_SECRET); } public void parseRequest(AuthorizationEndpointRequest request) { diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java index cb5901161f..7e3c9cec60 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java @@ -1202,10 +1202,10 @@ public class OAuthClient { parameters.add(new BasicNameValuePair(OIDCLoginProtocol.RESPONSE_MODE_PARAM, responseMode)); } if (clientId != null && clientSecret != null) { - String authorization = BasicAuthHelper.createHeader(clientId, clientSecret); - post.setHeader("Authorization", authorization); + parameters.add(new BasicNameValuePair(OAuth2Constants.CLIENT_ID, clientId)); + parameters.add(new BasicNameValuePair(OAuth2Constants.CLIENT_SECRET, clientSecret)); } - if (clientId != null) { + else if (clientId != null) { parameters.add(new BasicNameValuePair(OAuth2Constants.CLIENT_ID, clientId)); } if (redirectUri != null) { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RestartCookieTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RestartCookieTest.java index b9d783fc1a..f80ee8bf7a 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RestartCookieTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RestartCookieTest.java @@ -18,12 +18,20 @@ package org.keycloak.testsuite.forms; import jakarta.ws.rs.core.Response; + import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.HashSet; +import java.util.Set; + import org.jboss.arquillian.graphene.page.Page; import org.junit.Rule; import org.junit.Test; +import org.keycloak.OAuth2Constants; +import org.keycloak.TokenCategory; import org.keycloak.common.util.MultivaluedHashMap; import org.keycloak.crypto.Algorithm; +import org.keycloak.crypto.KeyUse; import org.keycloak.events.Details; import org.keycloak.events.Errors; import org.keycloak.jose.jws.JWSBuilder; @@ -32,17 +40,26 @@ import org.keycloak.keys.GeneratedHmacKeyProviderFactory; import org.keycloak.keys.KeyProvider; import org.keycloak.models.KeyManager; import org.keycloak.models.KeycloakSession; +import org.keycloak.models.ParConfig; import org.keycloak.models.RealmModel; import org.keycloak.models.utils.DefaultKeyProviders; import org.keycloak.protocol.RestartLoginCookie; +import org.keycloak.protocol.oidc.endpoints.AuthorizationEndpoint; import org.keycloak.representations.idm.ComponentRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.testsuite.AbstractTestRealmKeycloakTest; import org.keycloak.testsuite.Assert; import org.keycloak.testsuite.AssertEvents; import org.keycloak.testsuite.pages.LoginPage; +import org.keycloak.testsuite.util.ClientBuilder; +import org.keycloak.testsuite.util.OAuthClient; +import org.keycloak.util.TokenUtil; import org.openqa.selenium.Cookie; +import javax.crypto.SecretKey; + +import static org.junit.Assert.assertEquals; + /** * @author Marek Posolda */ @@ -76,6 +93,16 @@ public class RestartCookieTest extends AbstractTestRealmKeycloakTest { " }\n" + "}"; + public static final Set sensitiveNotes = new HashSet<>(); + static { + sensitiveNotes.add(OAuth2Constants.CLIENT_ASSERTION_TYPE); + sensitiveNotes.add(OAuth2Constants.CLIENT_ASSERTION); + sensitiveNotes.add(OAuth2Constants.CLIENT_SECRET); + sensitiveNotes.add(AuthorizationEndpoint.LOGIN_SESSION_NOTE_ADDITIONAL_REQ_PARAMS_PREFIX + OAuth2Constants.CLIENT_ASSERTION_TYPE); + sensitiveNotes.add(AuthorizationEndpoint.LOGIN_SESSION_NOTE_ADDITIONAL_REQ_PARAMS_PREFIX + OAuth2Constants.CLIENT_ASSERTION); + sensitiveNotes.add(AuthorizationEndpoint.LOGIN_SESSION_NOTE_ADDITIONAL_REQ_PARAMS_PREFIX + OAuth2Constants.CLIENT_SECRET); + } + @Override public void configureTestRealm(RealmRepresentation testRealm) { } @@ -99,6 +126,67 @@ public class RestartCookieTest extends AbstractTestRealmKeycloakTest { } } + @Test + public void testRestartCookie() { + loginPage.open(); + String restartCookie = loginPage.getDriver().manage().getCookieNamed(RestartLoginCookie.KC_RESTART).getValue(); + assertRestartCookie(restartCookie); + } + + @Test + public void testRestartCookieWithPar() { + String clientId = "par-confidential-client"; + adminClient.realm("test").clients().create(ClientBuilder.create() + .clientId("par-confidential-client") + .secret("secret") + .redirectUris(oauth.getRedirectUri() + "/*") + .attribute(ParConfig.REQUIRE_PUSHED_AUTHORIZATION_REQUESTS, "true") + .build()); + + oauth.clientId(clientId); + String requestUri = null; + try { + OAuthClient.ParResponse pResp = oauth.doPushedAuthorizationRequest(clientId, "secret"); + assertEquals(201, pResp.getStatusCode()); + requestUri = pResp.getRequestUri(); + } + catch (Exception e) { + Assert.fail(); + } + + oauth.redirectUri(null); + oauth.scope(null); + oauth.responseType(null); + oauth.requestUri(requestUri); + String state = oauth.stateParamRandom().getState(); + oauth.stateParamHardcoded(state); + + oauth.openLoginForm(); + String restartCookie = loginPage.getDriver().manage().getCookieNamed(RestartLoginCookie.KC_RESTART).getValue(); + assertRestartCookie(restartCookie); + } + + private void assertRestartCookie(String restartCookie) { + getTestingClient() + .server(TEST_REALM_NAME) + .run(session -> + { + try { + String sigAlgorithm = session.tokens().signatureAlgorithm(TokenCategory.INTERNAL); + String encAlgorithm = session.tokens().cekManagementAlgorithm(TokenCategory.INTERNAL); + SecretKey encKey = session.keys().getActiveKey(session.getContext().getRealm(), KeyUse.ENC, encAlgorithm).getSecretKey(); + SecretKey signKey = session.keys().getActiveKey(session.getContext().getRealm(), KeyUse.SIG, sigAlgorithm).getSecretKey(); + + byte[] contentBytes = TokenUtil.jweDirectVerifyAndDecode(encKey, signKey, restartCookie); + String jwt = new String(contentBytes, StandardCharsets.UTF_8); + RestartLoginCookie restartLoginCookie = session.tokens().decode(jwt, RestartLoginCookie.class); + Assert.assertFalse(restartLoginCookie.getNotes().keySet().stream().anyMatch(sensitiveNotes::contains)); + } catch (Exception e) { + Assert.fail(); + } + }); + } + // KEYCLOAK-5440 -- migration from Keycloak 3.1.0 @Test public void testRestartCookieBackwardsCompatible_Keycloak25() throws IOException { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/keys/FallbackKeyProviderTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/keys/FallbackKeyProviderTest.java index 5212318c45..ba11ae46af 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/keys/FallbackKeyProviderTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/keys/FallbackKeyProviderTest.java @@ -82,7 +82,7 @@ public class FallbackKeyProviderTest extends AbstractKeycloakTest { Assert.assertEquals(AppPage.RequestType.AUTH_RESPONSE, appPage.getRequestType()); providers = realmsResouce().realm("test").components().query(realmId, "org.keycloak.keys.KeyProvider"); - assertProviders(providers, "fallback-RS256", "fallback-" + Constants.INTERNAL_SIGNATURE_ALGORITHM); + assertProviders(providers, "fallback-RS256", "fallback-AES", "fallback-" + Constants.INTERNAL_SIGNATURE_ALGORITHM); } @Test