forked from keycloak/keycloak
-
Notifications
You must be signed in to change notification settings - Fork 1
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Encrypted KC_RESTART cookie and removed sensitive notes (#167)
Closes #keycloak/keycloak-private#162 Signed-off-by: Giuseppe Graziano <[email protected]>
- Loading branch information
Showing
8 changed files
with
145 additions
and
6 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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:[email protected]">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 { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters