Merge pull request #3579 from mposolda/master

KEYCLOAK-3824 Ensure sending notBefore invalidates JWKPublicKeyLocator

adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/AdapterDeploymentContext.java

@@ -342,6 +342,12 @@ public class AdapterDeploymentContext {
             return delegate.getNotBefore();
         }
 
+        @Override
+        public void updateNotBefore(int notBefore) {
+            delegate.setNotBefore(notBefore);
+            getPublicKeyLocator().reset(this);
+        }
+
         @Override
         public void setExposeToken(boolean exposeToken) {
             delegate.setExposeToken(exposeToken);

adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/KeycloakDeployment.java

@@ -329,6 +329,11 @@ public class KeycloakDeployment {
         this.notBefore = notBefore;
     }
 
+    public void updateNotBefore(int notBefore) {
+        this.notBefore = notBefore;
+        getPublicKeyLocator().reset(this);
+    }
+
     public boolean isAlwaysRefreshToken() {
         return alwaysRefreshToken;
     }

adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/OAuthRequestAuthenticator.java

@@ -357,7 +357,7 @@ public class OAuthRequestAuthenticator {
             return challenge(403, OIDCAuthenticationError.Reason.INVALID_TOKEN, null);
         }
         if (tokenResponse.getNotBeforePolicy() > deployment.getNotBefore()) {
-            deployment.setNotBefore(tokenResponse.getNotBeforePolicy());
+            deployment.updateNotBefore(tokenResponse.getNotBeforePolicy());
         }
         if (token.getIssuedAt() < deployment.getNotBefore()) {
             log.error("Stale token");

adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/PreAuthActionsHandler.java

@@ -155,7 +155,7 @@ public class PreAuthActionsHandler {
             } else {
                 log.debugf("logout of all sessions for application '%s'", action.getResource());
                 if (action.getNotBefore() > deployment.getNotBefore()) {
-                    deployment.setNotBefore(action.getNotBefore());
+                    deployment.updateNotBefore(action.getNotBefore());
                 }
                 userSessionManagement.logoutAll();
             }
@@ -177,7 +177,7 @@ public class PreAuthActionsHandler {
             }
             PushNotBeforeAction action = JsonSerialization.readValue(token.getContent(), PushNotBeforeAction.class);
             if (!validateAction(action)) return;
-            deployment.setNotBefore(action.getNotBefore());
+            deployment.updateNotBefore(action.getNotBefore());
         } catch (Exception e) {
             throw new RuntimeException(e);
         }

adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/RefreshableKeycloakSecurityContext.java

@@ -144,7 +144,7 @@ public class RefreshableKeycloakSecurityContext extends KeycloakSecurityContext
         }
 
         if (response.getNotBeforePolicy() > deployment.getNotBefore()) {
-            deployment.setNotBefore(response.getNotBeforePolicy());
+            deployment.updateNotBefore(response.getNotBeforePolicy());
         }
 
         this.token = token;

adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/rotation/HardcodedPublicKeyLocator.java

@@ -37,4 +37,9 @@ public class HardcodedPublicKeyLocator implements PublicKeyLocator {
     public PublicKey getPublicKey(String kid, KeycloakDeployment deployment) {
         return publicKey;
     }
+
+    @Override
+    public void reset(KeycloakDeployment deployment) {
+
+    }
 }

adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/rotation/JWKPublicKeyLocator.java

@@ -57,20 +57,24 @@ public class JWKPublicKeyLocator implements PublicKeyLocator {
         }
 
         // Check if we are allowed to send request
-        if (currentTime > lastRequestTime + minTimeBetweenRequests) {
+        synchronized (this) {
-            synchronized (this) {
+            currentTime = Time.currentTime();
-                currentTime = Time.currentTime();
+            if (currentTime > lastRequestTime + minTimeBetweenRequests) {
-                if (currentTime > lastRequestTime + minTimeBetweenRequests) {
+                sendRequest(deployment);
-                    sendRequest(deployment);
+                lastRequestTime = currentTime;
-                    lastRequestTime = currentTime;
+            } else {
-                } else {
+                log.debugf("Won't send request to realm jwks url. Last request time was %d", lastRequestTime);
-                    log.debugf("Won't send request to realm jwks url. Last request time was %d", lastRequestTime);
-                }
             }
+
+            return lookupCachedKey(publicKeyCacheTtl, currentTime, kid);
         }
+    }
 
-        return lookupCachedKey(publicKeyCacheTtl, currentTime, kid);
 
+    @Override
+    public void reset(KeycloakDeployment deployment) {
+        sendRequest(deployment);
+        lastRequestTime = Time.currentTime();
     }
 
 

adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/rotation/PublicKeyLocator.java

@@ -34,4 +34,11 @@ public interface PublicKeyLocator {
      */
     PublicKey getPublicKey(String kid, KeycloakDeployment deployment);
 
+    /**
+     * Reset the state of locator (eg. clear the cached keys)
+     *
+     * @param deployment
+     */
+    void reset(KeycloakDeployment deployment);
+
 }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/AbstractOIDCPublicKeyRotationAdapterTest.java

@@ -187,8 +187,6 @@ public class AbstractOIDCPublicKeyRotationAdapterTest extends AbstractServletsAd
     // KEYCLOAK-3824: Test for public-key-cache-ttl
     @Test
     public void testPublicKeyCacheTtl() {
-        driver.manage().timeouts().pageLoadTimeout(1000, TimeUnit.SECONDS);
-
         // increase accessTokenLifespan to 1200
         RealmRepresentation demoRealm = adminClient.realm(DEMO).toRepresentation();
         demoRealm.setAccessTokenLifespan(1200);
@@ -202,8 +200,10 @@ public class AbstractOIDCPublicKeyRotationAdapterTest extends AbstractServletsAd
         int status = invokeRESTEndpoint(accessTokenString);
         Assert.assertEquals(200, status);
 
-        // Invalidate realm public key
+        // Re-generate realm public key and remove the old key
+        String oldKeyId = getActiveKeyId();
         generateNewRealmKey();
+        adminClient.realm(DEMO).components().component(oldKeyId).remove();
 
         // Send REST request to the customer-db app. Should be still succcessfully authenticated as the JWKPublicKeyLocator cache is still valid
         status = invokeRESTEndpoint(accessTokenString);
@@ -225,6 +225,8 @@ public class AbstractOIDCPublicKeyRotationAdapterTest extends AbstractServletsAd
     // KEYCLOAK-3823: Test that sending notBefore policy invalidates JWKPublicKeyLocator cache
     @Test
     public void testPublicKeyCacheInvalidatedWhenPushedNotBefore() {
+        driver.manage().timeouts().pageLoadTimeout(1000, TimeUnit.SECONDS);
+
         // increase accessTokenLifespan to 1200
         RealmRepresentation demoRealm = adminClient.realm(DEMO).toRepresentation();
         demoRealm.setAccessTokenLifespan(1200);
@@ -234,20 +236,20 @@ public class AbstractOIDCPublicKeyRotationAdapterTest extends AbstractServletsAd
         loginToTokenMinTtlApp();
         String accessTokenString = tokenMinTTLPage.getAccessTokenString();
 
-        // Send REST request to customer-db app. I should be successfully authenticated
+        // Generate new realm public key
+        String oldKeyId = getActiveKeyId();
+        generateNewRealmKey();
+
+        // Send REST request to customer-db app. It should be successfully authenticated even that token is signed by the old key
         int status = invokeRESTEndpoint(accessTokenString);
         Assert.assertEquals(200, status);
 
-        // Invalidate realm public key
+        // Remove the old realm key now
-        generateNewRealmKey();
+        adminClient.realm(DEMO).components().component(oldKeyId).remove();
 
         // Set some offset to ensure pushing notBefore will pass
         setAdapterAndServerTimeOffset(130, customerDb.toString() + "/unsecured/foo", tokenMinTTLPage.toString() + "/unsecured/foo");
 
-        // Send REST request to the REST app. Should be still succcessfully authenticated as the JWKPublicKeyLocator cache is still valid
-        status = invokeRESTEndpoint(accessTokenString);
-        Assert.assertEquals(200, status);
-
         // Send notBefore policy from the realm
         demoRealm.setNotBefore(Time.currentTime() - 1);
         adminClient.realm(DEMO).update(demoRealm);
@@ -281,9 +283,6 @@ public class AbstractOIDCPublicKeyRotationAdapterTest extends AbstractServletsAd
     private void generateNewRealmKey() {
         String realmId = adminClient.realm(DEMO).toRepresentation().getId();
 
-        String oldKeyId = adminClient.realm(DEMO).components().query(realmId, KeyProvider.class.getName())
-                .get(0).getId();
-
         ComponentRepresentation keys = new ComponentRepresentation();
         keys.setName("generated");
         keys.setProviderType(KeyProvider.class.getName());
@@ -294,9 +293,12 @@ public class AbstractOIDCPublicKeyRotationAdapterTest extends AbstractServletsAd
         Response response = adminClient.realm(DEMO).components().add(keys);
         assertEquals(201, response.getStatus());
         response.close();
+    }
 
-        // Remove original key
+    private String getActiveKeyId() {
-        adminClient.realm(DEMO).components().component(oldKeyId).remove();
+        String realmId = adminClient.realm(DEMO).toRepresentation().getId();
+        return adminClient.realm(DEMO).components().query(realmId, KeyProvider.class.getName())
+                .get(0).getId();
     }