Make sure that signature validation possible to configure for OIDC id… (#44516)

closes #44473


Signed-off-by: mposolda <mposolda@gmail.com>
Signed-off-by: Marek Posolda <mposolda@gmail.com>
Co-authored-by: Ricardo Martin <rmartinc@redhat.com>

js/apps/admin-ui/maven-resources/theme/keycloak.v2/admin/messages/messages_en.properties

@@ -530,7 +530,7 @@ secretExpiresOn=Secret expires on {{time}}
 searchClientByName=Search client by name
 loginTimeout=Login timeout
 attributeName=Attribute [Name]
-updateError=Could not update the provider {{error}}
+updateError=Could not update the provider. {{error}}
 importUsersHelp=If true, LDAP users will be imported into the local database and synced by the configured sync policies. If import is enabled, the username and email attributes will be stored in the local database using case-insensitivity values, in lower-case. If disabled, those attributes will be treated as case-sensitive and the values will have the same format from their corresponding LDAP entries.
 emptyClientProfilesInstructions=There are no profiles, select 'Create client profile' to create a new client profile
 policyProvider.js=Define conditions for your permissions using JavaScript. It is one of the rule-based policy types supported by Keycloak, and provides flexibility to write any policy based on the Evaluation API.
@@ -2027,7 +2027,7 @@ scopePermissions.clients.view-description=Policies that decide if an administrat
 allowEcpFlow=Allow ECP flow
 rsa=rsa
 setPasswordConfirmText=Are you sure you want to set the password for the user {{username}}?
-updateErrorIdentityProvider=Could not update the provider {{error}}
+updateErrorIdentityProvider=Could not update the provider. {{error}}
 emptyProfiles=No client profiles configured
 createClientProfileError=Could not create client profile\: '{{error}}'
 usermodel.clientRoleMapping.clientId.tooltip=Client ID for role mappings. Just client roles of this client will be added to the token. If this is unset, client roles of all clients will be added to the token.
@@ -2479,7 +2479,7 @@ targetContextAttributes=Target Context Attributes
 targetContextAttributesHelp=Defines the evaluation of context attributes (claims) instead of identity attributes
 filteredByClaim=Verify essential claim
 rowCancelBtnAriaLabel=Cancel edits for {{messageBundle}}
-validateSignatureHelp=Enable/disable signature validation of external IDP signatures. For Federated Client Authentication and JWT Authorization Grant the signature validation must be enabled.
+validateSignatureHelp=Enable/disable signature validation of external IDP tokens. When enabled, Keycloak will validate JWT tokens retrieved from the Identity provider during scenarios related to user authentication, for example when Keycloak obtains IDToken or accessToken after completion of the OIDC/OAuth2 flow with Identity provider. For Federated Client Authentication and JWT Authorization Grant, the signature validation is always used regardless of the value of this switch.
 searchForFlow=Search for flow
 verifyEmail=Verify email
 addressClaim.locality.label=User Attribute Name for Locality

js/apps/admin-ui/src/identity-providers/add/DiscoverySettings.tsx

@@ -35,6 +35,14 @@ const Fields = ({ readOnly, isOIDC }: DiscoverySettingsProps) => {
     control,
     name: "config.pkceEnabled",
   });
+  const jwtAuthorizationGrantEnabled = useWatch({
+    control,
+    name: "config.jwtAuthorizationGrantEnabled",
+  });
+  const supportsClientAssertions = useWatch({
+    control,
+    name: "config.supportsClientAssertions",
+  });
 
   return (
     <div className="pf-v5-c-form pf-m-horizontal">
@@ -93,7 +101,9 @@ const Fields = ({ readOnly, isOIDC }: DiscoverySettingsProps) => {
             isDisabled={readOnly}
             stringify
           />
-          {validateSignature === "true" && (
+          {(validateSignature === "true" ||
+            jwtAuthorizationGrantEnabled === "true" ||
+            supportsClientAssertions == "true") && (
             <>
               <DefaultSwitchControl
                 name="config.useJwksUrl"
@@ -103,9 +113,11 @@ const Fields = ({ readOnly, isOIDC }: DiscoverySettingsProps) => {
                 stringify
               />
               {useJwks === "true" ? (
-                <TextAreaControl
+                <TextControl
                   name="config.jwksUrl"
                   label={t("jwksUrl")}
+                  labelIcon={t("jwksUrlHelp")}
+                  type="url"
                   readOnly={readOnly}
                 />
               ) : (
@@ -113,10 +125,12 @@ const Fields = ({ readOnly, isOIDC }: DiscoverySettingsProps) => {
                   <TextAreaControl
                     name="config.publicKeySignatureVerifier"
                     label={t("validatingPublicKey")}
+                    labelIcon={t("validatingPublicKeyHelp")}
                   />
                   <TextControl
                     name="config.publicKeySignatureVerifierKeyId"
                     label={t("validatingPublicKeyId")}
+                    labelIcon={t("validatingPublicKeyIdHelp")}
                     readOnly={readOnly}
                   />
                 </>

js/apps/admin-ui/test/identity-providers/main.ts

@@ -87,7 +87,7 @@ export async function assertInvalidUrlNotification(
   urlType: UrlType,
 ) {
   await expect(page.getByTestId("last-alert")).toHaveText(
-    `Could not update the provider The url [${urlType}${urlType.startsWith("single") ? "U" : "_u"}rl] is malformed`,
+    `Could not update the provider. The url [${urlType}${urlType.startsWith("single") ? "U" : "_u"}rl] is malformed`,
   );
 }
 

js/apps/admin-ui/test/identity-providers/oidc.spec.ts

@@ -67,6 +67,15 @@ test.describe.serial("OIDC identity provider test", () => {
     await assertPkceMethodExists(page);
 
     await clickSaveButton(page);
+    await assertNotificationMessage(
+      page,
+      "Could not update the provider. The 'Validating public key' is required when 'Validate signatures' enabled and 'Use JWKS URL' disabled",
+    );
+
+    await switchOn(page, "#config\\.useJwksUrl");
+    await assertJwksUrlExists(page, true);
+    await clickSaveButton(page);
+
     await assertNotificationMessage(page, "Provider successfully updated");
   });
 });

js/apps/keycloak-server/scripts/start-server.js

@@ -60,7 +60,7 @@ async function startServer() {
     path.join(SERVER_DIR, `bin/kc${SCRIPT_EXTENSION}`),
     [
       "start-dev",
-      `--features="login:v2,account:v3,admin-fine-grained-authz:v2,transient-users,oid4vc-vci,organization,declarative-ui,quick-theme,spiffe,kubernetes-service-accounts,workflows"`,
+      `--features="transient-users,oid4vc-vci,declarative-ui,quick-theme,spiffe,kubernetes-service-accounts,workflows,client-auth-federated,jwt-authorization-grant"`,
       ...keycloakArgs,
     ],
     {

services/src/main/java/org/keycloak/broker/jwtauthorizationgrant/JWTAuthorizationGrantConfig.java

@@ -8,23 +8,27 @@ import static org.keycloak.protocol.oidc.OIDCLoginProtocol.ISSUER;
 
 public interface JWTAuthorizationGrantConfig {
 
-    public static final String JWT_AUTHORIZATION_GRANT_ENABLED = "jwtAuthorizationGrantEnabled";
+    String JWT_AUTHORIZATION_GRANT_ENABLED = "jwtAuthorizationGrantEnabled";
 
-    public static final String JWT_AUTHORIZATION_GRANT_ASSERTION_REUSE_ALLOWED = "jwtAuthorizationGrantAssertionReuseAllowed";
+    String JWT_AUTHORIZATION_GRANT_ASSERTION_REUSE_ALLOWED = "jwtAuthorizationGrantAssertionReuseAllowed";
 
-    public static final String JWT_AUTHORIZATION_GRANT_MAX_ALLOWED_ASSERTION_EXPIRATION = "jwtAuthorizationGrantMaxAllowedAssertionExpiration";
+    String JWT_AUTHORIZATION_GRANT_MAX_ALLOWED_ASSERTION_EXPIRATION = "jwtAuthorizationGrantMaxAllowedAssertionExpiration";
 
-    public static final String JWT_AUTHORIZATION_GRANT_ASSERTION_SIGNATURE_ALG = "jwtAuthorizationGrantAssertionSignatureAlg";
+    String JWT_AUTHORIZATION_GRANT_ASSERTION_SIGNATURE_ALG = "jwtAuthorizationGrantAssertionSignatureAlg";
 
-    public static final String JWT_AUTHORIZATION_GRANT_ALLOWED_CLOCK_SKEW = "jwtAuthorizationGrantAllowedClockSkew";
+    String JWT_AUTHORIZATION_GRANT_ALLOWED_CLOCK_SKEW = "jwtAuthorizationGrantAllowedClockSkew";
 
     Map<String, String> getConfig();
 
-    default boolean getJWTAuthorizationGrantEnabled() {
+    default boolean isJWTAuthorizationGrantEnabled() {
         return Boolean.parseBoolean(getConfig().getOrDefault(JWT_AUTHORIZATION_GRANT_ENABLED, "false"));
     }
 
-    default boolean getJWTAuthorizationGrantAssertionReuseAllowed() {
+    default void setJWTAuthorizationGrantEnabled(boolean jwtAuthorizationGrantEnableds) {
+        getConfig().put(JWT_AUTHORIZATION_GRANT_ENABLED, String.valueOf(jwtAuthorizationGrantEnableds));
+    }
+
+    default boolean isJWTAuthorizationGrantAssertionReuseAllowed() {
         return Boolean.parseBoolean(getConfig().getOrDefault(JWT_AUTHORIZATION_GRANT_ASSERTION_REUSE_ALLOWED, "false"));
     }
 

services/src/main/java/org/keycloak/broker/jwtauthorizationgrant/JWTAuthorizationGrantIdentityProvider.java

@@ -51,7 +51,7 @@ public class JWTAuthorizationGrantIdentityProvider implements JWTAuthorizationGr
 
     @Override
     public boolean isAssertionReuseAllowed() {
-        return config.getJWTAuthorizationGrantAssertionReuseAllowed();
+        return config.isJWTAuthorizationGrantAssertionReuseAllowed();
     }
 
     @Override

services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java

@@ -630,7 +630,15 @@ public class OIDCIdentityProvider extends AbstractOAuth2IdentityProvider<OIDCIde
 
     protected boolean verify(JWSInput jws) {
         if (!getConfig().isValidateSignature()) return true;
+        return verifySignature(jws);
+    }
 
+    /**
+     * Verify signature on given JWS
+     *
+     * @return true if signature was successfully verified with the keys available to identity provider
+     */
+    protected boolean verifySignature(JWSInput jws) {
         try {
             KeyWrapper key = getIdentityProviderKeyWrapper(jws);
             if (key == null) {
@@ -1054,7 +1062,7 @@ public class OIDCIdentityProvider extends AbstractOAuth2IdentityProvider<OIDCIde
     public boolean verifyClientAssertion(ClientAuthenticationFlowContext context) throws Exception {
         OIDCIdentityProviderConfig config = getConfig();
 
-        FederatedJWTClientValidator validator = new FederatedJWTClientValidator(context, v -> verify(v.getJws()),
+        FederatedJWTClientValidator validator = new FederatedJWTClientValidator(context, v -> verifySignature(v.getJws()),
                 config.getIssuer(), config.getAllowedClockSkew(), config.isSupportsClientAssertionReuse());
 
         if (!Profile.isFeatureEnabled(Profile.Feature.CLIENT_AUTH_FEDERATED)) {
@@ -1065,24 +1073,16 @@ public class OIDCIdentityProvider extends AbstractOAuth2IdentityProvider<OIDCIde
             throw new RuntimeException("Issuer does not support client assertions");
         }
 
-        if (!config.isValidateSignature()) {
-            throw new RuntimeException("Signature validation not enabled for issuer");
-        }
-
         return validator.validate();
     }
 
     public BrokeredIdentityContext validateAuthorizationGrantAssertion(JWTAuthorizationGrantValidationContext context) throws IdentityBrokerException {
-        if (!getConfig().getJWTAuthorizationGrantEnabled()) {
+        if (!getConfig().isJWTAuthorizationGrantEnabled()) {
             throw new IdentityBrokerException("JWT Authorization Granted is not enabled for the identity provider");
         }
 
-        if (!getConfig().isValidateSignature()) {
-            throw new IdentityBrokerException("Signature validation not enabled for issuer");
-        }
-
         // verify signature
-        if (!verify(context.getJws())) {
+        if (!verifySignature(context.getJws())) {
             throw new IdentityBrokerException("Invalid signature");
         }
 
@@ -1099,7 +1099,7 @@ public class OIDCIdentityProvider extends AbstractOAuth2IdentityProvider<OIDCIde
 
     @Override
     public boolean isAssertionReuseAllowed() {
-        return getConfig().getJWTAuthorizationGrantAssertionReuseAllowed();
+        return getConfig().isJWTAuthorizationGrantAssertionReuseAllowed();
     }
 
     @Override

services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProviderConfig.java

@@ -186,6 +186,10 @@ public class OIDCIdentityProviderConfig extends OAuth2IdentityProviderConfig imp
         return Boolean.parseBoolean(getConfig().get(SUPPORTS_CLIENT_ASSERTIONS));
     }
 
+    public void setSupportsClientAssertions(boolean supportsClientAssertions) {
+        getConfig().put(SUPPORTS_CLIENT_ASSERTIONS, String.valueOf(supportsClientAssertions));
+    }
+
     public boolean isSupportsClientAssertionReuse() {
         return Boolean.parseBoolean(getConfig().get(SUPPORTS_CLIENT_ASSERTION_REUSE));
     }
@@ -196,5 +200,18 @@ public class OIDCIdentityProviderConfig extends OAuth2IdentityProviderConfig imp
         SslRequired sslRequired = realm.getSslRequired();
         checkUrl(sslRequired, getJwksUrl(), "jwks_url");
         checkUrl(sslRequired, getLogoutUrl(), "logout_url");
+
+        if (isValidateSignature() || isJWTAuthorizationGrantEnabled() || isSupportsClientAssertions()) {
+            String optionText = isValidateSignature() ? "Validate signatures" :
+                    (isJWTAuthorizationGrantEnabled() ? "JWT Authorization Grant" : "Supports client assertions");
+
+            if (isUseJwksUrl()) {
+                if (getJwksUrl() == null) {
+                    throw new IllegalArgumentException(String.format("JWKS URL is required when '%s' enabled and 'Use JWKS URL' enabled", optionText));
+                }
+            } else if (getPublicKeySignatureVerifier() == null) {
+                throw new IllegalArgumentException(String.format("The 'Validating public key' is required when '%s' enabled and 'Use JWKS URL' disabled", optionText));
+            }
+        }
     }
 }

tests/base/src/test/java/org/keycloak/tests/admin/identityprovider/IdentityProviderOidcTest.java

@@ -365,11 +365,7 @@ public class IdentityProviderOidcTest extends AbstractIdentityProviderTest {
             resource.update(representation);
             fail("Invalid URL");
         } catch (Exception e) {
-            assertTrue(e instanceof  ClientErrorException);
+            assertError(e, "The url [authorization_url] is malformed");
-            Response response = ClientErrorException.class.cast(e).getResponse();
-            assertEquals( Response.Status.BAD_REQUEST.getStatusCode(), response.getStatus());
-            ErrorRepresentation error = ((ClientErrorException) e).getResponse().readEntity(ErrorRepresentation.class);
-            assertEquals("The url [authorization_url] is malformed", error.getErrorMessage());
         }
 
         oidcConfig.setAuthorizationUrl(null);
@@ -379,11 +375,7 @@ public class IdentityProviderOidcTest extends AbstractIdentityProviderTest {
             resource.update(representation);
             fail("Invalid URL");
         } catch (Exception e) {
-            assertTrue(e instanceof  ClientErrorException);
+            assertError(e, "The url [token_url] requires secure connections");
-            Response response = ClientErrorException.class.cast(e).getResponse();
-            assertEquals( Response.Status.BAD_REQUEST.getStatusCode(), response.getStatus());
-            ErrorRepresentation error = ((ClientErrorException) e).getResponse().readEntity(ErrorRepresentation.class);
-            assertEquals("The url [token_url] requires secure connections", error.getErrorMessage());
         }
 
         oidcConfig.setAuthorizationUrl(null);
@@ -393,11 +385,7 @@ public class IdentityProviderOidcTest extends AbstractIdentityProviderTest {
             resource.update(representation);
             fail("Invalid URL");
         } catch (Exception e) {
-            assertTrue(e instanceof  ClientErrorException);
+            assertError(e, "The url [jwks_url] requires secure connections");
-            Response response = ClientErrorException.class.cast(e).getResponse();
-            assertEquals( Response.Status.BAD_REQUEST.getStatusCode(), response.getStatus());
-            ErrorRepresentation error = ((ClientErrorException) e).getResponse().readEntity(ErrorRepresentation.class);
-            assertEquals("The url [jwks_url] requires secure connections", error.getErrorMessage());
         }
 
         oidcConfig.setAuthorizationUrl(null);
@@ -408,11 +396,7 @@ public class IdentityProviderOidcTest extends AbstractIdentityProviderTest {
             resource.update(representation);
             fail("Invalid URL");
         } catch (Exception e) {
-            assertTrue(e instanceof  ClientErrorException);
+            assertError(e, "The url [logout_url] requires secure connections");
-            Response response = ClientErrorException.class.cast(e).getResponse();
-            assertEquals( Response.Status.BAD_REQUEST.getStatusCode(), response.getStatus());
-            ErrorRepresentation error = ((ClientErrorException) e).getResponse().readEntity(ErrorRepresentation.class);
-            assertEquals("The url [logout_url] requires secure connections", error.getErrorMessage());
         }
 
         oidcConfig.setAuthorizationUrl(null);
@@ -425,11 +409,7 @@ public class IdentityProviderOidcTest extends AbstractIdentityProviderTest {
             resource.update(representation);
             fail("Invalid URL");
         } catch (Exception e) {
-            assertTrue(e instanceof  ClientErrorException);
+            assertError(e, "The url [userinfo_url] requires secure connections");
-            Response response = ClientErrorException.class.cast(e).getResponse();
-            assertEquals( Response.Status.BAD_REQUEST.getStatusCode(), response.getStatus());
-            ErrorRepresentation error = ((ClientErrorException) e).getResponse().readEntity(ErrorRepresentation.class);
-            assertEquals("The url [userinfo_url] requires secure connections", error.getErrorMessage());
         }
 
         managedRealm.updateWithCleanup(r -> r.sslRequired(SslRequired.EXTERNAL.name()));
@@ -438,6 +418,67 @@ public class IdentityProviderOidcTest extends AbstractIdentityProviderTest {
         managedRealm.cleanup().add(r -> r.identityProviders().get(id).remove());
     }
 
+    @Test
+    public void testOIDCKeysRequiredForVariousConfigs() {
+        String id = create(createRep("keycloak-oidc", "keycloak-oidc"));
+
+        IdentityProviderResource resource = this.managedRealm.admin().identityProviders().get("keycloak-oidc");
+        IdentityProviderRepresentation representation = resource.toRepresentation();
+        OIDCIdentityProviderConfigRep oidcConfig = new OIDCIdentityProviderConfigRep(representation);
+
+        // OIDC Keys required when "validate signature" is ON
+        oidcConfig.setValidateSignature(true);
+        try {
+            resource.update(representation);
+            fail("Not expected to update identity provider");
+        } catch (Exception e) {
+            assertError(e, "The 'Validating public key' is required when 'Validate signatures' enabled and 'Use JWKS URL' disabled");
+        }
+
+        // OIDC Keys (set by JWKS URL) required when "validate signature" is ON
+        oidcConfig.setUseJwksUrl(true);
+        try {
+            resource.update(representation);
+            fail("JWKS URL is required when 'Validate signatures' enabled and 'Use JWKS URL' enabled");
+        } catch (Exception e) {
+            assertError(e, "JWKS URL is required when 'Validate signatures' enabled and 'Use JWKS URL' enabled");
+        }
+
+        // OIDC Keys (set by JWKS URL) required when "authorization grant" is ON
+        oidcConfig.setValidateSignature(false);
+        oidcConfig.setJWTAuthorizationGrantEnabled(true);
+        try {
+            resource.update(representation);
+            fail("JWKS URL is required when 'Validate signatures' enabled and 'Use JWKS URL' enabled");
+        } catch (Exception e) {
+            assertError(e, "JWKS URL is required when 'JWT Authorization Grant' enabled and 'Use JWKS URL' enabled");
+        }
+
+        // OIDC Keys (set by JWKS URL) required when "federated client authentication" is ON
+        oidcConfig.setJWTAuthorizationGrantEnabled(false);
+        oidcConfig.setSupportsClientAssertions(true);
+        try {
+            resource.update(representation);
+            fail("JWKS URL is required when 'Validate signatures' enabled and 'Use JWKS URL' enabled");
+        } catch (Exception e) {
+            assertError(e, "JWKS URL is required when 'Supports client assertions' enabled and 'Use JWKS URL' enabled");
+        }
+
+        // Successful update when JWKS URL set
+        oidcConfig.setJwksUrl("https://foo");
+        resource.update(representation);
+
+        managedRealm.cleanup().add(r -> r.identityProviders().get(id).remove());
+    }
+
+    private void assertError(Exception e, String expectedError) {
+        assertTrue(e instanceof  ClientErrorException);
+        Response response = ClientErrorException.class.cast(e).getResponse();
+        assertEquals( Response.Status.BAD_REQUEST.getStatusCode(), response.getStatus());
+        ErrorRepresentation error = ((ClientErrorException) e).getResponse().readEntity(ErrorRepresentation.class);
+        assertEquals(expectedError, error.getErrorMessage());
+    }
+
     @Test
     public void testNoExport() {
         String id = create(createRep("keycloak-oidc", "keycloak-oidc"));

tests/base/src/test/java/org/keycloak/tests/oauth/OIDCIdentityProviderJWTAuthorizationGrantTest.java

@@ -43,9 +43,13 @@ public class OIDCIdentityProviderJWTAuthorizationGrantTest extends AbstractJWTAu
             rep.getConfig().put(OIDCIdentityProviderConfig.VALIDATE_SIGNATURE, "false");
         });
 
+        // Test with JWT signed by invalid key. It tests that signature validation is triggered even if "validate signature" switch on OIDC provider is false
+        testInvalidSignature();
+
+        // Test with correct signature
         String jwt = getIdentityProvider().encodeToken(createAuthorizationGrantToken("basic-user-id", oAuthClient.getEndpoints().getIssuer(), IDP_ISSUER));
         AccessTokenResponse response = oAuthClient.jwtAuthorizationGrantRequest(jwt).send();
-        assertFailure("Signature validation not enabled for issuer", response, events.poll());
+        assertSuccess("test-app", response);
     }
 
     public static class JWTAuthorizationGrantRealmConfig extends AbstractJWTAuthorizationGrantTest.JWTAuthorizationGrantRealmConfig {