Merge pull request #4083 from knutz3n/KEYCLOAK-4780

KEYCLOAK-4780 Ensure Base64 encoded HMAC secret key is decoded before use

common/src/main/java/org/keycloak/common/util/KeyUtils.java

@@ -40,8 +40,8 @@ public class KeyUtils {
     private KeyUtils() {
     }
 
-    public static SecretKey loadSecretKey(String secret) {
+    public static SecretKey loadSecretKey(byte[] secret) {
-        return new SecretKeySpec(secret.getBytes(), "HmacSHA256");
+        return new SecretKeySpec(secret, "HmacSHA256");
     }
 
     public static KeyPair generateRsaKeyPair(int keysize) {

common/src/test/java/org/keycloak/common/util/KeyUtilsTest.java

@@ -0,0 +1,24 @@
+package org.keycloak.common.util;
+
+import org.junit.Test;
+
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+import java.util.concurrent.ThreadLocalRandom;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+
+public class KeyUtilsTest {
+
+    @Test
+    public void loadSecretKey() throws Exception {
+        byte[] secretBytes = new byte[32];
+        ThreadLocalRandom.current().nextBytes(secretBytes);
+        SecretKeySpec expected = new SecretKeySpec(secretBytes, "HmacSHA256");
+        SecretKey actual = KeyUtils.loadSecretKey(secretBytes);
+        assertEquals(expected.getAlgorithm(), actual.getAlgorithm());
+        assertArrayEquals(expected.getEncoded(), actual.getEncoded());
+    }
+
+}

server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java

@@ -19,7 +19,6 @@ package org.keycloak.models.utils;
 
 import org.keycloak.broker.social.SocialIdentityProvider;
 import org.keycloak.broker.social.SocialIdentityProviderFactory;
-import org.keycloak.common.util.Base64Url;
 import org.keycloak.common.util.CertificateUtils;
 import org.keycloak.common.util.KeyUtils;
 import org.keycloak.common.util.PemUtils;
@@ -75,14 +74,14 @@ public final class KeycloakModelUtils {
         return UUID.randomUUID().toString();
     }
 
-    public static String generateSecret() {
+    public static byte[] generateSecret() {
         return generateSecret(32);
     }
 
-    public static String generateSecret(int bytes) {
+    public static byte[] generateSecret(int bytes) {
         byte[] buf = new byte[bytes];
         new SecureRandom().nextBytes(buf);
-        return Base64Url.encode(buf);
+        return buf;
     }
 
     public static PublicKey getPublicKey(String publicKeyPem) {

server-spi-private/src/main/java/org/keycloak/services/managers/ClientSessionCode.java

@@ -18,6 +18,7 @@
 package org.keycloak.services.managers;
 
 import org.jboss.logging.Logger;
+import org.keycloak.common.util.Base64Url;
 import org.keycloak.common.util.Time;
 import org.keycloak.models.ClientModel;
 import org.keycloak.models.ClientSessionModel;
@@ -227,7 +228,7 @@ public class ClientSessionCode {
 
     private static String generateCode(ClientSessionModel clientSession) {
         try {
-            String actionId = KeycloakModelUtils.generateSecret();
+            String actionId = Base64Url.encode(KeycloakModelUtils.generateSecret());
 
             StringBuilder sb = new StringBuilder();
             sb.append(actionId);

services/src/main/java/org/keycloak/keys/GeneratedHmacKeyProvider.java

@@ -17,6 +17,7 @@
 
 package org.keycloak.keys;
 
+import org.keycloak.common.util.Base64Url;
 import org.keycloak.common.util.KeyUtils;
 import org.keycloak.component.ComponentModel;
 import org.keycloak.jose.jws.AlgorithmType;
@@ -47,7 +48,7 @@ public class GeneratedHmacKeyProvider implements HmacKeyProvider {
         if (model.hasNote(SecretKey.class.getName())) {
             secretKey = model.getNote(SecretKey.class.getName());
         } else {
-            secretKey = KeyUtils.loadSecretKey(model.get(Attributes.SECRET_KEY));
+            secretKey = KeyUtils.loadSecretKey(Base64Url.decode(model.get(Attributes.SECRET_KEY)));
             model.setNote(SecretKey.class.getName(), secretKey);
         }
     }

services/src/main/java/org/keycloak/keys/GeneratedHmacKeyProviderFactory.java

@@ -81,8 +81,8 @@ public class GeneratedHmacKeyProviderFactory extends AbstractHmacKeyProviderFact
 
     private void generateSecret(ComponentModel model, int size) {
         try {
-            String secret = KeycloakModelUtils.generateSecret(size);
+            byte[] secret = KeycloakModelUtils.generateSecret(size);
-            model.put(Attributes.SECRET_KEY, secret);
+            model.put(Attributes.SECRET_KEY, Base64Url.encode(secret));
 
             String kid = KeycloakModelUtils.generateId();
             model.put(Attributes.KID_KEY, kid);

services/src/main/java/org/keycloak/services/resources/AbstractSecuredLocalService.java

@@ -22,6 +22,7 @@ import org.jboss.resteasy.spi.HttpRequest;
 import org.keycloak.AbstractOAuthClient;
 import org.keycloak.OAuth2Constants;
 import org.keycloak.common.ClientConnection;
+import org.keycloak.common.util.Base64Url;
 import org.keycloak.common.util.KeycloakUriBuilder;
 import org.keycloak.common.util.UriUtils;
 import org.keycloak.models.ClientModel;
@@ -133,7 +134,7 @@ public abstract class AbstractSecuredLocalService {
         if (cookie != null) {
             stateChecker = cookie.getValue();
         } else {
-            stateChecker = KeycloakModelUtils.generateSecret();
+            stateChecker = Base64Url.encode(KeycloakModelUtils.generateSecret());
             String cookiePath = AuthenticationManager.getRealmCookiePath(realm, uriInfo);
             boolean secureOnly = realm.getSslRequired().isRequired(clientConnection);
             CookieHelper.addCookie(KEYCLOAK_STATE_CHECKER, stateChecker, cookiePath, null, null, -1, secureOnly, true);

services/src/main/java/org/keycloak/services/resources/WelcomeResource.java

@@ -19,6 +19,7 @@ package org.keycloak.services.resources;
 import org.jboss.logging.Logger;
 import org.keycloak.Config;
 import org.keycloak.common.ClientConnection;
+import org.keycloak.common.util.Base64Url;
 import org.keycloak.common.util.MimeTypeUtil;
 import org.keycloak.models.BrowserSecurityHeaders;
 import org.keycloak.models.KeycloakSession;
@@ -246,7 +247,7 @@ public class WelcomeResource {
         if (stateChecker != null) {
             return stateChecker;
         } else {
-            stateChecker = KeycloakModelUtils.generateSecret();
+            stateChecker = Base64Url.encode(KeycloakModelUtils.generateSecret());
             String cookiePath = uriInfo.getPath();
             boolean secureOnly = uriInfo.getRequestUri().getScheme().equalsIgnoreCase("https");
             CookieHelper.addCookie(KEYCLOAK_STATE_CHECKER, stateChecker, cookiePath, null, null, -1, secureOnly, true);