From 51a9712e59880c5c5c19cb9923b44402e6962561 Mon Sep 17 00:00:00 2001 From: Marek Posolda Date: Thu, 1 Jun 2023 14:18:13 +0200 Subject: [PATCH] Improper Client Certificate Validation for OAuth/OpenID clients (#20) Co-authored-by: Stian Thorgersen --- .../client/X509ClientAuthenticator.java | 26 +++++++--------- .../testsuite/util/MutualTLSUtils.java | 3 ++ .../keycloak/testsuite/client/FAPI1Test.java | 2 +- .../testsuite/client/FAPICIBATest.java | 6 ++-- .../testsuite/client/MutualTLSClientTest.java | 31 ++++++++++++++++--- 5 files changed, 45 insertions(+), 23 deletions(-) diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/client/X509ClientAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/client/X509ClientAuthenticator.java index f05f10f926..10382ad103 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/client/X509ClientAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/client/X509ClientAuthenticator.java @@ -130,42 +130,38 @@ public class X509ClientAuthenticator extends AbstractClientAuthenticator { return; } - Optional matchedCertificate; + // Testing only 1st certificate in the chain to match with configured subject + X509Certificate certificate = certs[0]; + boolean matchedCertificate = false; if (clientCfg.getAllowRegexPatternComparison()) { Pattern subjectDNPattern = Pattern.compile(subjectDNRegexp); - matchedCertificate = Arrays.stream(certs) - .map(certificate -> certificate.getSubjectDN().getName()) - .filter(subjectdn -> subjectDNPattern.matcher(subjectdn).matches()) - .findFirst(); + String subjectdn = certificate.getSubjectDN().getName(); + matchedCertificate = subjectDNPattern.matcher(subjectdn).matches(); } else { // OIDC/OAuth2 does not use regex comparison as it expects exact DN given in the format according to RFC4514. See RFC8705 for the details. // We allow custom OIDs attributes to be "expanded" or not expanded in the given Subject DN X500Principal expectedDNPrincipal = new X500Principal(subjectDNRegexp, CUSTOM_OIDS_REVERSED); - matchedCertificate = Arrays.stream(certs) - .filter(certificate -> expectedDNPrincipal.getName(X500Principal.RFC2253, CUSTOM_OIDS).equals(certificate.getSubjectX500Principal().getName(X500Principal.RFC2253, CUSTOM_OIDS))) - .map(certificate -> certificate.getSubjectDN().getName()) - .findFirst(); + matchedCertificate = (expectedDNPrincipal.getName(X500Principal.RFC2253, CUSTOM_OIDS).equals(certificate.getSubjectX500Principal().getName(X500Principal.RFC2253, CUSTOM_OIDS))); } - if (!matchedCertificate.isPresent()) { + if (!matchedCertificate) { // We do quite expensive operation here, so better check the logging level beforehand. if (logger.isDebugEnabled()) { logger.debug("[X509ClientCertificateAuthenticator:authenticate] Couldn't match any certificate for expected Subject DN '" + subjectDNRegexp + "' with allow regex pattern '" + clientCfg.getAllowRegexPatternComparison() + "'."); - logger.debug("[X509ClientCertificateAuthenticator:authenticate] Available SubjectDNs: " + + logger.debug("[X509ClientCertificateAuthenticator:authenticate] Checked Subject DN: " + certificate.getSubjectDN().getName()); + logger.debug("[X509ClientCertificateAuthenticator:authenticate] All SubjectDNs from the certificate chain: " + Arrays.stream(certs) .map(cert -> cert.getSubjectDN().getName()) .collect(Collectors.toList())); } context.attempted(); - return; } else { - logger.debug("[X509ClientCertificateAuthenticator:authenticate] Matched " + matchedCertificate.get() + " certificate."); + logger.debug("[X509ClientCertificateAuthenticator:authenticate] Matched " + certificate.getSubjectDN().getName() + " certificate."); + context.success(); } - - context.success(); } public String getDisplayType() { diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/MutualTLSUtils.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/MutualTLSUtils.java index 9d97bb65f0..a2a2bc4612 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/MutualTLSUtils.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/MutualTLSUtils.java @@ -28,6 +28,9 @@ public class MutualTLSUtils { public static final String DEFAULT_KEYSTOREPATH = System.getProperty("client.certificate.keystore"); public static final String DEFAULT_KEYSTOREPASSWORD = System.getProperty("client.certificate.keystore.passphrase"); + + // Subject DN of the 1st certificate corresponding to private-key in DEFAULT_KEYSTOREPATH keystore + public static final String DEFAULT_KEYSTORE_SUBJECT_DN = "EMAILADDRESS=test-user@localhost, CN=test-user@localhost, OU=Keycloak, O=Red Hat, L=Westford, ST=MA, C=US"; public static final String DEFAULT_TRUSTSTOREPATH = System.getProperty("client.truststore"); public static final String DEFAULT_TRUSTSTOREPASSWORD = System.getProperty("client.truststore.passphrase"); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPI1Test.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPI1Test.java index c0106a225b..e7382869ff 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPI1Test.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPI1Test.java @@ -616,7 +616,7 @@ public class FAPI1Test extends AbstractClientPoliciesTest { clientRep.setImplicitFlowEnabled(true); OIDCAdvancedConfigWrapper clientConfig = OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep); clientConfig.setRequestUris(Collections.singletonList(TestApplicationResourceUrls.clientRequestUri())); - clientConfig.setTlsClientAuthSubjectDn("EMAILADDRESS=contact@keycloak.org, CN=Keycloak Intermediate CA, OU=Keycloak, O=Red Hat, ST=MA, C=US"); + clientConfig.setTlsClientAuthSubjectDn(MutualTLSUtils.DEFAULT_KEYSTORE_SUBJECT_DN); clientConfig.setAllowRegexPatternComparison(false); }); ClientResource clientResource = adminClient.realm(REALM_NAME).clients().get(clientUUID); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPICIBATest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPICIBATest.java index 4718ea381e..2678575bb0 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPICIBATest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPICIBATest.java @@ -370,7 +370,7 @@ public class FAPICIBATest extends AbstractClientPoliciesTest { clientRep.setClientAuthenticatorType(X509ClientAuthenticator.PROVIDER_ID); OIDCAdvancedConfigWrapper clientConfig = OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep); clientConfig.setRequestUris(Collections.singletonList(TestApplicationResourceUrls.clientRequestUri())); - clientConfig.setTlsClientAuthSubjectDn("EMAILADDRESS=contact@keycloak.org, CN=Keycloak Intermediate CA, OU=Keycloak, O=Red Hat, ST=MA, C=US"); + clientConfig.setTlsClientAuthSubjectDn(MutualTLSUtils.DEFAULT_KEYSTORE_SUBJECT_DN); clientConfig.setAllowRegexPatternComparison(false); setClientAuthMethodNeutralSettings(clientRep); }); @@ -414,7 +414,7 @@ public class FAPICIBATest extends AbstractClientPoliciesTest { clientRep.setClientAuthenticatorType(X509ClientAuthenticator.PROVIDER_ID); OIDCAdvancedConfigWrapper clientConfig = OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep); clientConfig.setRequestUris(Collections.singletonList(TestApplicationResourceUrls.clientRequestUri())); - clientConfig.setTlsClientAuthSubjectDn("EMAILADDRESS=contact@keycloak.org, CN=Keycloak Intermediate CA, OU=Keycloak, O=Red Hat, ST=MA, C=US"); + clientConfig.setTlsClientAuthSubjectDn(MutualTLSUtils.DEFAULT_KEYSTORE_SUBJECT_DN); clientConfig.setAllowRegexPatternComparison(false); setClientAuthMethodNeutralSettings(clientRep); }); @@ -444,7 +444,7 @@ public class FAPICIBATest extends AbstractClientPoliciesTest { clientRep.setClientAuthenticatorType(X509ClientAuthenticator.PROVIDER_ID); OIDCAdvancedConfigWrapper clientConfig = OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep); clientConfig.setRequestUris(Collections.singletonList(TestApplicationResourceUrls.clientRequestUri())); - clientConfig.setTlsClientAuthSubjectDn("EMAILADDRESS=contact@keycloak.org, CN=Keycloak Intermediate CA, OU=Keycloak, O=Red Hat, ST=MA, C=US"); + clientConfig.setTlsClientAuthSubjectDn(MutualTLSUtils.DEFAULT_KEYSTORE_SUBJECT_DN); clientConfig.setAllowRegexPatternComparison(false); setClientAuthMethodNeutralSettings(clientRep); }); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/MutualTLSClientTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/MutualTLSClientTest.java index 180ad75a7e..c080ed0a93 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/MutualTLSClientTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/MutualTLSClientTest.java @@ -9,7 +9,6 @@ import java.util.List; import java.util.Map; import java.util.function.Supplier; -import org.apache.commons.collections.map.UnmodifiableMap; import org.apache.http.NameValuePair; import org.apache.http.client.entity.UrlEncodedFormEntity; import org.apache.http.client.methods.HttpPost; @@ -43,11 +42,15 @@ public class MutualTLSClientTest extends AbstractTestRealmKeycloakTest { private static final String CLIENT_ID = "confidential-x509"; private static final String DISABLED_CLIENT_ID = "confidential-disabled-x509"; private static final String EXACT_SUBJECT_DN_CLIENT_ID = "confidential-subjectdn-x509"; + + private static final String ISSUER_SUBJECT_DN_CLIENT_ID = "confidential-issuer-subjectdn-x509"; private static final String OBB_SUBJECT_DN_CLIENT_ID = "obb-subjectdn-x509"; private static final String USER = "keycloak-user@localhost"; private static final String PASSWORD = "password"; private static final String REALM = "test"; - private static final String EXACT_CERTIFICATE_SUBJECT_DN = "EMAILADDRESS=contact@keycloak.org, CN=Keycloak Intermediate CA, OU=Keycloak, O=Red Hat, ST=MA, C=US"; + + // This is DN of the issuer certificate, which signed certificate corresponding to EXACT_CERTIFICATE_SUBJECT_DN. This issuer certificate is present in the client.jks keystore on the 2nd position + private static final String ISSUER_CERTIFICATE_SUBJECT_DN = "EMAILADDRESS=contact@keycloak.org, CN=Keycloak Intermediate CA, OU=Keycloak, O=Red Hat, ST=MA, C=US"; @Override public void configureTestRealm(RealmRepresentation testRealm) { @@ -68,10 +71,19 @@ public class MutualTLSClientTest extends AbstractTestRealmKeycloakTest { exactSubjectDNConfiguration.setRedirectUris(Arrays.asList("https://localhost:8543/auth/realms/master/app/auth")); exactSubjectDNConfiguration.setClientAuthenticatorType(X509ClientAuthenticator.PROVIDER_ID); Map attrs = new HashMap<>(); - attrs.put(X509ClientAuthenticator.ATTR_SUBJECT_DN, EXACT_CERTIFICATE_SUBJECT_DN); + attrs.put(X509ClientAuthenticator.ATTR_SUBJECT_DN, MutualTLSUtils.DEFAULT_KEYSTORE_SUBJECT_DN); attrs.put(X509ClientAuthenticator.ATTR_ALLOW_REGEX_PATTERN_COMPARISON, "false"); exactSubjectDNConfiguration.setAttributes(attrs); + ClientRepresentation issuerSubjectDNConfiguration = KeycloakModelUtils.createClient(testRealm, ISSUER_SUBJECT_DN_CLIENT_ID); + issuerSubjectDNConfiguration.setServiceAccountsEnabled(Boolean.TRUE); + issuerSubjectDNConfiguration.setRedirectUris(Arrays.asList("https://localhost:8543/auth/realms/master/app/auth")); + issuerSubjectDNConfiguration.setClientAuthenticatorType(X509ClientAuthenticator.PROVIDER_ID); + attrs = new HashMap<>(); + attrs.put(X509ClientAuthenticator.ATTR_SUBJECT_DN, ISSUER_CERTIFICATE_SUBJECT_DN); + attrs.put(X509ClientAuthenticator.ATTR_ALLOW_REGEX_PATTERN_COMPARISON, "false"); + issuerSubjectDNConfiguration.setAttributes(attrs); + ClientRepresentation obbSubjectDNConfiguration = KeycloakModelUtils.createClient(testRealm, OBB_SUBJECT_DN_CLIENT_ID); obbSubjectDNConfiguration.setServiceAccountsEnabled(Boolean.TRUE); obbSubjectDNConfiguration.setRedirectUris(Arrays.asList("https://localhost:8543/auth/realms/master/app/auth")); @@ -109,6 +121,18 @@ public class MutualTLSClientTest extends AbstractTestRealmKeycloakTest { assertTokenObtained(token); } + @Test + public void testFailedClientInvocationWithIssuerCertificateAndSubjectDN() throws Exception { + //given + Supplier clientWithProperCertificate = MutualTLSUtils::newCloseableHttpClientWithDefaultKeyStoreAndTrustStore; + + //when (Certificate with the client's expected subjectDN is available in the certificate chain, but not on the 1st position. Hence authentication should not be successful) + OAuthClient.AccessTokenResponse token = loginAndGetAccessTokenResponse(ISSUER_SUBJECT_DN_CLIENT_ID, clientWithProperCertificate); + + //then + assertTokenNotObtained(token); + } + @Test public void testSuccessfulClientInvocationWithClientIdInQueryParams() throws Exception { //given//when @@ -249,7 +273,6 @@ public class MutualTLSClientTest extends AbstractTestRealmKeycloakTest { * It test a scenario, where we do not follow the spec and specify client_id in Query Params (for in a form). */ private OAuthClient.AccessTokenResponse getAccessTokenResponseWithQueryParams(String clientId, CloseableHttpClient client) throws Exception { - OAuthClient.AccessTokenResponse token;// This is a very simplified version of HttpPost post = new HttpPost(oauth.getAccessTokenUrl() + "?client_id=" + clientId); List parameters = new LinkedList<>(); parameters.add(new BasicNameValuePair(OAuth2Constants.GRANT_TYPE, OAuth2Constants.AUTHORIZATION_CODE));