From fc514aa2567ca353b8c3dc380ff80b9af88c9295 Mon Sep 17 00:00:00 2001
From: Pedro Igor <pigor.craveiro@gmail.com>
Date: Wed, 29 Jan 2020 14:06:29 -0300
Subject: [PATCH] [KEYCLOAK-12792] - Invalid nonce handling in OIDC identity
 brokering

---
 .../oidc/AbstractOAuth2IdentityProvider.java  |  7 --
 .../broker/oidc/OIDCIdentityProvider.java     | 42 +++++++++
 .../keycloak/testsuite/util/OAuthClient.java  | 18 ++++
 .../KcOidcBrokerNonceParameterTest.java       | 88 +++++++++++++++++++
 4 files changed, 148 insertions(+), 7 deletions(-)
 create mode 100644 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerNonceParameterTest.java

diff --git a/services/src/main/java/org/keycloak/broker/oidc/AbstractOAuth2IdentityProvider.java b/services/src/main/java/org/keycloak/broker/oidc/AbstractOAuth2IdentityProvider.java
index 7f31603ae1..b927d2bbdb 100755
--- a/services/src/main/java/org/keycloak/broker/oidc/AbstractOAuth2IdentityProvider.java
+++ b/services/src/main/java/org/keycloak/broker/oidc/AbstractOAuth2IdentityProvider.java
@@ -335,13 +335,6 @@ public abstract class AbstractOAuth2IdentityProvider<C extends OAuth2IdentityPro
             uriBuilder.queryParam(OAuth2Constants.PROMPT, prompt);
         }
 
-        String nonce = request.getAuthenticationSession().getClientNote(OIDCLoginProtocol.NONCE_PARAM);
-        if (nonce == null || nonce.isEmpty()) {
-            nonce = UUID.randomUUID().toString();
-            request.getAuthenticationSession().setClientNote(OIDCLoginProtocol.NONCE_PARAM, nonce);
-        }
-        uriBuilder.queryParam(OIDCLoginProtocol.NONCE_PARAM, nonce);
-
         String acr = request.getAuthenticationSession().getClientNote(OAuth2Constants.ACR_VALUES);
         if (acr != null) {
             uriBuilder.queryParam(OAuth2Constants.ACR_VALUES, acr);
diff --git a/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java b/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java
index b42c5340d9..5db9bb55ce 100755
--- a/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java
+++ b/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java
@@ -21,10 +21,12 @@ import org.jboss.logging.Logger;
 import org.keycloak.OAuth2Constants;
 import org.keycloak.OAuthErrorException;
 import org.keycloak.broker.oidc.mappers.AbstractJsonUserAttributeMapper;
+import org.keycloak.broker.provider.AuthenticationRequest;
 import org.keycloak.broker.provider.BrokeredIdentityContext;
 import org.keycloak.broker.provider.ExchangeExternalToken;
 import org.keycloak.broker.provider.IdentityBrokerException;
 import org.keycloak.broker.provider.util.SimpleHttp;
+import org.keycloak.common.util.Base64Url;
 import org.keycloak.common.util.Time;
 import org.keycloak.events.Details;
 import org.keycloak.events.Errors;
@@ -40,6 +42,7 @@ import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.RealmModel;
 import org.keycloak.models.UserModel;
 import org.keycloak.models.UserSessionModel;
+import org.keycloak.models.utils.KeycloakModelUtils;
 import org.keycloak.protocol.oidc.OIDCLoginProtocol;
 import org.keycloak.representations.AccessTokenResponse;
 import org.keycloak.representations.IDToken;
@@ -65,6 +68,7 @@ import javax.ws.rs.core.UriBuilder;
 import javax.ws.rs.core.UriInfo;
 import java.io.IOException;
 import java.security.PublicKey;
+import java.util.UUID;
 
 /**
  * @author Pedro Igor
@@ -79,6 +83,7 @@ public class OIDCIdentityProvider extends AbstractOAuth2IdentityProvider<OIDCIde
     public static final String VALIDATED_ID_TOKEN = "VALIDATED_ID_TOKEN";
     public static final String ACCESS_TOKEN_EXPIRATION = "accessTokenExpiration";
     public static final String EXCHANGE_PROVIDER = "EXCHANGE_PROVIDER";
+    private static final String BROKER_NONCE_PARAM = "BROKER_NONCE";
 
     public OIDCIdentityProvider(KeycloakSession session, OIDCIdentityProviderConfig config) {
         super(session, config);
@@ -361,6 +366,8 @@ public class OIDCIdentityProvider extends AbstractOAuth2IdentityProvider<OIDCIde
         try {
             BrokeredIdentityContext identity = extractIdentity(tokenResponse, accessToken, idToken);
 
+            identity.getContextData().put(BROKER_NONCE_PARAM, idToken.getOtherClaims().get(OIDCLoginProtocol.NONCE_PARAM));
+            
             if (getConfig().isStoreToken()) {
                 if (tokenResponse.getExpiresIn() > 0) {
                     long accessTokenExpiration = Time.currentTime() + tokenResponse.getExpiresIn();
@@ -453,6 +460,7 @@ public class OIDCIdentityProvider extends AbstractOAuth2IdentityProvider<OIDCIde
         }
         if (tokenResponse != null) identity.getContextData().put(FEDERATED_ACCESS_TOKEN_RESPONSE, tokenResponse);
         if (tokenResponse != null) processAccessTokenResponse(identity, tokenResponse);
+        
         return identity;
     }
 
@@ -715,4 +723,38 @@ public class OIDCIdentityProvider extends AbstractOAuth2IdentityProvider<OIDCIde
             throw new ErrorResponseException(OAuthErrorException.INVALID_TOKEN, "invalid token type", Response.Status.BAD_REQUEST);
         }
     }
+
+    @Override
+    protected UriBuilder createAuthorizationUrl(AuthenticationRequest request) {
+        UriBuilder uriBuilder = super.createAuthorizationUrl(request);
+        String nonce = Base64Url.encode(KeycloakModelUtils.generateSecret(16));
+        AuthenticationSessionModel authenticationSession = request.getAuthenticationSession();
+
+        authenticationSession.setClientNote(BROKER_NONCE_PARAM, nonce);
+        uriBuilder.queryParam(OIDCLoginProtocol.NONCE_PARAM, nonce);
+        
+        return uriBuilder;
+    }
+
+    @Override
+    public void preprocessFederatedIdentity(KeycloakSession session, RealmModel realm, BrokeredIdentityContext context) {
+        AuthenticationSessionModel authenticationSession = session.getContext().getAuthenticationSession();
+        
+        if (authenticationSession == null) {
+            // no interacting with the brokered OP, likely doing token exchanges
+            return;
+        }
+
+        String nonce = (String) context.getContextData().get(BROKER_NONCE_PARAM);
+
+        if (nonce == null) {
+            throw new IdentityBrokerException("OpenID Provider [" + getConfig().getProviderId() + "] did not return a nonce");
+        }
+
+        String expectedNonce = authenticationSession.getClientNote(BROKER_NONCE_PARAM);
+
+        if (!nonce.equals(expectedNonce)) {
+            throw new ErrorResponseException(OAuthErrorException.INVALID_TOKEN, "invalid nonce", Response.Status.BAD_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 88b4e9eedf..f95c5f649d 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
@@ -70,6 +70,7 @@ import org.keycloak.util.JsonSerialization;
 import org.keycloak.util.TokenUtil;
 import org.openqa.selenium.By;
 import org.openqa.selenium.WebDriver;
+import org.openqa.selenium.WebElement;
 
 import javax.ws.rs.client.Entity;
 import javax.ws.rs.core.Form;
@@ -90,6 +91,7 @@ import java.util.Map;
 import java.util.function.Supplier;
 
 import static org.keycloak.testsuite.admin.Users.getPasswordOf;
+import static org.keycloak.testsuite.util.UIUtils.clickLink;
 import static org.keycloak.testsuite.util.URLUtils.removeDefaultPorts;
 
 /**
@@ -239,6 +241,17 @@ public class OAuthClient {
         return new AuthorizationEndpointResponse(this);
     }
 
+    public AuthorizationEndpointResponse doLoginSocial(String brokerId, String username, String password) {
+        openLoginForm();
+        WaitUtils.waitForPageToLoad();
+
+        WebElement socialButton = findSocialButton(brokerId);
+        clickLink(socialButton);
+        fillLoginForm(username, password);
+
+        return new AuthorizationEndpointResponse(this);
+    }
+
     public AuthorizationEndpointResponse doLogin(UserRepresentation user) {
 
         return doLogin(user.getUsername(), getPasswordOf(user));
@@ -1326,6 +1339,11 @@ public class OAuthClient {
         return driver;
     }
 
+    private WebElement findSocialButton(String providerId) {
+        String id = "zocial-" + providerId;
+        return DroneUtils.getCurrentDriver().findElement(By.id(id));
+    }
+    
     private interface StateParamProvider {
 
         String getState();
diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerNonceParameterTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerNonceParameterTest.java
new file mode 100644
index 0000000000..bae3ef8c36
--- /dev/null
+++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerNonceParameterTest.java
@@ -0,0 +1,88 @@
+package org.keycloak.testsuite.broker;
+
+import static org.keycloak.testsuite.broker.BrokerTestTools.waitForPage;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.junit.Assert;
+import org.junit.Ignore;
+import org.junit.Test;
+import org.keycloak.OAuth2Constants;
+import org.keycloak.admin.client.resource.UsersResource;
+import org.keycloak.jose.jws.JWSInput;
+import org.keycloak.jose.jws.JWSInputException;
+import org.keycloak.representations.AccessToken;
+import org.keycloak.representations.IDToken;
+import org.keycloak.representations.idm.ClientRepresentation;
+import org.keycloak.representations.idm.UserRepresentation;
+import org.keycloak.services.managers.AuthenticationManager;
+import org.keycloak.testsuite.arquillian.SuiteContext;
+import org.keycloak.testsuite.util.ClientBuilder;
+import org.keycloak.testsuite.util.OAuthClient;
+import org.openqa.selenium.Cookie;
+
+public class KcOidcBrokerNonceParameterTest extends AbstractBrokerTest {
+
+    @Override
+    protected BrokerConfiguration getBrokerConfiguration() {
+        return new KcOidcBrokerConfiguration() {
+            @Override
+            public List<ClientRepresentation> createConsumerClients(SuiteContext suiteContext) {
+                List<ClientRepresentation> clients = new ArrayList<>(super.createConsumerClients(suiteContext));
+                
+                clients.add(ClientBuilder.create().clientId("consumer-client")
+                        .publicClient()
+                        .redirectUris("http://localhost:8180/auth/realms/master/app/auth/*", "https://localhost:8543/auth/realms/master/app/auth/*")
+                        .publicClient().build());
+                
+                return clients;
+            }
+        };
+    }
+
+    @Override
+    protected void loginUser() {
+        updateExecutions(AbstractBrokerTest::disableUpdateProfileOnFirstLogin);
+
+        oauth.realm(bc.consumerRealmName());
+        oauth.clientId("consumer-client");
+        oauth.nonce("123456");
+
+        OAuthClient.AuthorizationEndpointResponse authzResponse = oauth
+                .doLoginSocial(bc.getIDPAlias(), bc.getUserLogin(), bc.getUserPassword());
+        String code = authzResponse.getCode();
+        OAuthClient.AccessTokenResponse response = oauth.doAccessTokenRequest(code, null);
+        IDToken idToken = toIdToken(response.getIdToken());
+        
+        Assert.assertEquals("123456", idToken.getNonce());
+    }
+    
+    @Test
+    public void testNonceNotSet() {
+        updateExecutions(AbstractBrokerTest::disableUpdateProfileOnFirstLogin);
+
+        oauth.realm(bc.consumerRealmName());
+        oauth.clientId("consumer-client");
+        oauth.nonce(null);
+
+        OAuthClient.AuthorizationEndpointResponse authzResponse = oauth
+                .doLoginSocial(bc.getIDPAlias(), bc.getUserLogin(), bc.getUserPassword());
+        String code = authzResponse.getCode();
+        OAuthClient.AccessTokenResponse response = oauth.doAccessTokenRequest(code, null);
+        IDToken idToken = toIdToken(response.getIdToken());
+
+        Assert.assertNull(idToken.getNonce());
+    }
+
+    protected IDToken toIdToken(String encoded) {
+        IDToken idToken;
+
+        try {
+            idToken = new JWSInput(encoded).readJsonContent(IDToken.class);
+        } catch (JWSInputException cause) {
+            throw new RuntimeException("Failed to deserialize RPT", cause);
+        }
+        return idToken;
+    }
+}