FFFF Encrypted KC_RESTART cookie and removed sensitive notes by graziang · Pull Request #30114 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Encrypted KC_RESTART cookie and removed sensitive notes #30114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ahus1 merged 1 commit into from
Jun 5, 2024
Merged

Encrypted KC_RESTART cookie and removed sensitive notes #30114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/documentation/release_notes/index.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
5 changes: 5 additions & 0 deletions docs/documentation/release_notes/topics/24_0_5.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
4 changes: 4 additions & 0 deletions services/src/main/java/org/keycloak/jose/jws/DefaultTokenManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,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);
Expand Down Expand Up @@ -310,6 +312,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:
Expand Down
38 changes: 36 additions & 2 deletions services/src/main/java/org/keycloak/protocol/RestartLoginCookie.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}

Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
5 changes: 5 additions & 0 deletions ...rc/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
6 changes: 3 additions & 3 deletions ...egration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1202,10 +1202,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) {
Expand Down
88 changes: 88 additions & 0 deletions ...n-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RestartCookieTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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>
*/
Expand Down Expand Up @@ -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) {
}
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion ...uillian/tests/base/src/test/java/org/keycloak/testsuite/keys/FallbackKeyProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
0