Encrypted KC_RESTART cookie and removed sensitive notes (#167)

Closes #keycloak/keycloak-private#162

Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>

docs/documentation/release_notes/index.adoc

@@ -13,6 +13,9 @@ include::topics/templates/document-attributes.adoc[]
 :release_header_latest_link: {releasenotes_link_latest}
 include::topics/templates/release-header.adoc[]
 
+== {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]
 

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.

services/src/main/java/org/keycloak/jose/jws/DefaultTokenManager.java

@@ -273,6 +273,8 @@ private String getEncryptedToken(TokenCategory category, String encodedToken) {
     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);
@@ -300,6 +302,8 @@ public String encryptAlgorithm(TokenCategory category) {
         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:

services/src/main/java/org/keycloak/protocol/RestartLoginCookie.java

@@ -23,13 +23,17 @@
 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 RestartLoginCookie(AuthenticationSessionModel authSession) {
 
     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 static String getRestartCookie(KeycloakSession session){
     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 static AuthenticationSessionModel restartSession(KeycloakSession session,
         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;

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) {

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java

@@ -1198,10 +1198,10 @@ public ParResponse doPushedAuthorizationRequest(String clientId, String clientSe
                 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) {

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.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 <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
  */
@@ -76,6 +93,16 @@ public class RestartCookieTest extends AbstractTestRealmKeycloakTest {
             "  }\n" +
             "}";
 
+    public static final Set<String> 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 @@ protected void afterAbstractKeycloakTestRealmImport() {
         }
     }
 
+    @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 {

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/keys/FallbackKeyProviderTest.java

@@ -82,7 +82,7 @@ public void fallbackAfterDeletingAllKeysInRealm() {
         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