From 43a3c676f752a41ea93852187767c6ee505f2329 Mon Sep 17 00:00:00 2001 From: Luca Leonardo Scorcia Date: Wed, 25 Nov 2020 05:28:33 -0500 Subject: [PATCH] KEYCLOAK-16456 X509 Auth: add option for OCSP fail-open behavior --- ...actX509ClientCertificateAuthenticator.java | 2 + ...ClientCertificateAuthenticatorFactory.java | 8 ++ .../x509/CertificateValidator.java | 60 +++++++--- .../x509/X509AuthenticatorConfigModel.java | 9 ++ .../x509/X509OCSPResponderFailOpenTest.java | 111 ++++++++++++++++++ 5 files changed, 172 insertions(+), 18 deletions(-) create mode 100644 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/x509/X509OCSPResponderFailOpenTest.java diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/x509/AbstractX509ClientCertificateAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/x509/AbstractX509ClientCertificateAuthenticator.java index 078bb2d299..09c0fca901 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/x509/AbstractX509ClientCertificateAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/x509/AbstractX509ClientCertificateAuthenticator.java @@ -59,6 +59,7 @@ public abstract class AbstractX509ClientCertificateAuthenticator implements Auth public static final String REGULAR_EXPRESSION = "x509-cert-auth.regular-expression"; public static final String ENABLE_CRL = "x509-cert-auth.crl-checking-enabled"; public static final String ENABLE_OCSP = "x509-cert-auth.ocsp-checking-enabled"; + public static final String OCSP_FAIL_OPEN = "x509-cert-auth.ocsp-fail-open"; public static final String ENABLE_CRLDP = "x509-cert-auth.crldp-checking-enabled"; public static final String CANONICAL_DN = "x509-cert-auth.canonical-dn-enabled"; public static final String TIMESTAMP_VALIDATION = "x509-cert-auth.timestamp-validation-enabled"; @@ -116,6 +117,7 @@ public abstract class AbstractX509ClientCertificateAuthenticator implements Auth .cRLDPEnabled(config.getCRLDistributionPointEnabled()) .cRLrelativePath(config.getCRLRelativePath()) .oCSPEnabled(config.getOCSPEnabled()) + .oCSPFailOpen(config.getOCSPFailOpen()) .oCSPResponseCertificate(config.getOCSPResponderCertificate()) .oCSPResponderURI(config.getOCSPResponder()) .trustValidation() diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/x509/AbstractX509ClientCertificateAuthenticatorFactory.java b/services/src/main/java/org/keycloak/authentication/authenticators/x509/AbstractX509ClientCertificateAuthenticatorFactory.java index 2dd9c99167..25500bc398 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/x509/AbstractX509ClientCertificateAuthenticatorFactory.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/x509/AbstractX509ClientCertificateAuthenticatorFactory.java @@ -182,6 +182,13 @@ public abstract class AbstractX509ClientCertificateAuthenticatorFactory implemen oCspCheckingEnabled.setHelpText("Enable Certificate Revocation Checking using OCSP"); oCspCheckingEnabled.setLabel("OCSP Checking Enabled"); + ProviderConfigProperty ocspFailOpen = new ProviderConfigProperty(); + ocspFailOpen.setType(BOOLEAN_TYPE); + ocspFailOpen.setName(OCSP_FAIL_OPEN); + ocspFailOpen.setDefaultValue(Boolean.toString(false)); + ocspFailOpen.setHelpText("Whether to allow or deny authentication for client certificates that have missing/invalid/inconclusive OCSP endpoints. By default a successful OCSP response is required."); + ocspFailOpen.setLabel("OCSP Fail-Open Behavior"); + ProviderConfigProperty ocspResponderUri = new ProviderConfigProperty(); ocspResponderUri.setType(STRING_TYPE); ocspResponderUri.setName(OCSPRESPONDER_URI); @@ -245,6 +252,7 @@ public abstract class AbstractX509ClientCertificateAuthenticatorFactory implemen crlDPEnabled, cRLRelativePath, oCspCheckingEnabled, + ocspFailOpen, ocspResponderUri, ocspResponderCert, keyUsage, diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/x509/CertificateValidator.java b/services/src/main/java/org/keycloak/authentication/authenticators/x509/CertificateValidator.java index c35681af31..01e8f67371 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/x509/CertificateValidator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/x509/CertificateValidator.java @@ -403,6 +403,7 @@ public class CertificateValidator { boolean _crldpEnabled; CRLLoaderImpl _crlLoader; boolean _ocspEnabled; + boolean _ocspFailOpen; OCSPChecker ocspChecker; boolean _timestampValidationEnabled; boolean _trustValidationEnabled; @@ -417,6 +418,7 @@ public class CertificateValidator { boolean cRLDPCheckingEnabled, CRLLoaderImpl crlLoader, boolean oCSPCheckingEnabled, + boolean ocspFailOpen, OCSPChecker ocspChecker, KeycloakSession session, boolean timestampValidationEnabled, @@ -430,6 +432,7 @@ public class CertificateValidator { _crldpEnabled = cRLDPCheckingEnabled; _crlLoader = crlLoader; _ocspEnabled = oCSPCheckingEnabled; + _ocspFailOpen = ocspFailOpen; this.ocspChecker = ocspChecker; this.session = session; _timestampValidationEnabled = timestampValidationEnabled; @@ -705,25 +708,38 @@ public class CertificateValidator { } } - OCSPUtils.OCSPRevocationStatus rs = ocspChecker.check(cert, issuer); + try { + OCSPUtils.OCSPRevocationStatus rs = ocspChecker.check(cert, issuer); - if (rs == null) { - throw new GeneralSecurityException("Unable to check client revocation status using OCSP"); - } + if (rs == null) { + if (_ocspFailOpen) + logger.warnf("Unable to check client revocation status using OCSP - continuing certificate authentication because of fail-open OCSP configuration setting"); + else + throw new GeneralSecurityException("Unable to check client revocation status using OCSP"); + } - if (rs.getRevocationStatus() == OCSPUtils.RevocationStatus.UNKNOWN) { - throw new GeneralSecurityException("Unable to determine certificate's revocation status."); - } - else if (rs.getRevocationStatus() == OCSPUtils.RevocationStatus.REVOKED) { + if (rs.getRevocationStatus() == OCSPUtils.RevocationStatus.UNKNOWN) { + if (_ocspFailOpen) + logger.warnf("Unable to determine certificate's revocation status - continuing certificate authentication because of fail-open OCSP configuration setting"); + else + throw new GeneralSecurityException("Unable to determine certificate's revocation status."); + } + else if (rs.getRevocationStatus() == OCSPUtils.RevocationStatus.REVOKED) { - StringBuilder sb = new StringBuilder(); - sb.append("Certificate's been revoked."); - sb.append("\n"); - sb.append(rs.getRevocationReason().toString()); - sb.append("\n"); - sb.append(String.format("Revoked on: %s",rs.getRevocationTime().toString())); + StringBuilder sb = new StringBuilder(); + sb.append("Certificate's been revoked."); + sb.append("\n"); + sb.append(rs.getRevocationReason().toString()); + sb.append("\n"); + sb.append(String.format("Revoked on: %s",rs.getRevocationTime().toString())); - throw new GeneralSecurityException(sb.toString()); + throw new GeneralSecurityException(sb.toString()); + } + } catch (CertPathValidatorException e) { + if (_ocspFailOpen) + logger.warnf("Unable to check client revocation status using OCSP - continuing certificate authentication because of fail-open OCSP configuration setting"); + else + throw e; } } @@ -792,6 +808,7 @@ public class CertificateValidator { boolean _crldpEnabled; CRLLoaderImpl _crlLoader; boolean _ocspEnabled; + boolean _ocspFailOpen; String _responderUri; X509Certificate _responderCert; boolean _timestampValidationEnabled; @@ -967,7 +984,14 @@ public class CertificateValidator { } public class GotOCSP { - public GotOCSP oCSPResponseCertificate(String responderCert) { + public GotOCSPFailOpen oCSPFailOpen(boolean ocspFailOpen) { + _ocspFailOpen = ocspFailOpen; + return new GotOCSPFailOpen(); + } + } + + public class GotOCSPFailOpen { + public GotOCSPFailOpen oCSPResponseCertificate(String responderCert) { if (responderCert != null && !responderCert.isEmpty()) { try { _responderCert = XMLSignatureUtil.getX509CertificateFromKeyInfoString(responderCert); @@ -979,7 +1003,7 @@ public class CertificateValidator { throw new RuntimeException(e); } } - return new GotOCSP(); + return new GotOCSPFailOpen(); } public CertificateValidatorBuilder oCSPResponderURI(String responderURI) { @@ -1050,7 +1074,7 @@ public class CertificateValidator { } return new CertificateValidator(certs, _keyUsageBits, _extendedKeyUsage, _certificatePolicy, _certificatePolicyMode, - _crlCheckingEnabled, _crldpEnabled, _crlLoader, _ocspEnabled, + _crlCheckingEnabled, _crldpEnabled, _crlLoader, _ocspEnabled, _ocspFailOpen, new BouncyCastleOCSPChecker(session, _responderUri, _responderCert), session, _timestampValidationEnabled, _trustValidationEnabled); } } diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/x509/X509AuthenticatorConfigModel.java b/services/src/main/java/org/keycloak/authentication/authenticators/x509/X509AuthenticatorConfigModel.java index 0cefb20a23..0a087e5c59 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/x509/X509AuthenticatorConfigModel.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/x509/X509AuthenticatorConfigModel.java @@ -130,6 +130,15 @@ public class X509AuthenticatorConfigModel extends AuthenticatorConfigModel { return this; } + public boolean getOCSPFailOpen() { + return Boolean.parseBoolean(getConfig().getOrDefault(OCSP_FAIL_OPEN, Boolean.toString(false))); + } + + public X509AuthenticatorConfigModel setOCSPFailOpen(boolean value) { + getConfig().put(OCSP_FAIL_OPEN, Boolean.toString(value)); + return this; + } + public boolean getCRLDistributionPointEnabled() { return Boolean.parseBoolean(getConfig().get(ENABLE_CRLDP)); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/x509/X509OCSPResponderFailOpenTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/x509/X509OCSPResponderFailOpenTest.java new file mode 100644 index 0000000000..6ba93711db --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/x509/X509OCSPResponderFailOpenTest.java @@ -0,0 +1,111 @@ +package org.keycloak.testsuite.x509; + +import com.google.common.base.Charsets; + +import io.undertow.Undertow; +import io.undertow.server.handlers.BlockingHandler; + +import java.nio.file.Paths; +import java.util.function.Supplier; +import javax.ws.rs.core.Response; +import javax.ws.rs.core.Response; + +import org.apache.commons.io.IOUtils; +import org.apache.http.impl.client.CloseableHttpClient; +import org.jboss.arquillian.drone.api.annotation.Drone; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.keycloak.testsuite.util.PhantomJSBrowser; +import org.keycloak.authentication.authenticators.x509.X509AuthenticatorConfigModel; +import org.keycloak.representations.idm.AuthenticatorConfigRepresentation; +import org.keycloak.testsuite.util.OAuthClient; +import org.openqa.selenium.WebDriver; + +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.assertEquals; +import static org.keycloak.authentication.authenticators.x509.X509AuthenticatorConfigModel.IdentityMapperType.USERNAME_EMAIL; +import static org.keycloak.authentication.authenticators.x509.X509AuthenticatorConfigModel.MappingSourceType.SUBJECTDN_EMAIL; + +public class X509OCSPResponderFailOpenTest extends AbstractX509AuthenticationTest { + + private static final String OCSP_RESPONDER_HOST = "localhost"; + + private static final int OCSP_RESPONDER_PORT = 8888; + + private Undertow ocspResponder; + + @Drone + @PhantomJSBrowser + private WebDriver phantomJS; + + @Before + public void replaceTheDefaultDriver() { + replaceDefaultWebDriver(phantomJS); + } + + @Test + public void ocspFailCloseLoginFailed() throws Exception { + // Test of OCSP failure (invalid OCSP responder host) when OCSP Fail-Open is set to OFF + // If test is successful, it should return an auth error + + X509AuthenticatorConfigModel config = new X509AuthenticatorConfigModel() + .setOCSPEnabled(true) + .setOCSPResponder("http://" + OCSP_RESPONDER_HOST + ".invalid.host:" + OCSP_RESPONDER_PORT + "/oscp") + .setOCSPFailOpen(false) + .setMappingSourceType(SUBJECTDN_EMAIL) + .setUserIdentityMapperType(USERNAME_EMAIL); + AuthenticatorConfigRepresentation cfg = newConfig("x509-directgrant-config", config.getConfig()); + String cfgId = createConfig(directGrantExecution.getId(), cfg); + Assert.assertNotNull(cfgId); + + oauth.clientId("resource-owner"); + OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest("secret", "", "", null); + + assertEquals(Response.Status.UNAUTHORIZED.getStatusCode(), response.getStatusCode()); + assertEquals("invalid_request", response.getError()); + + // Make sure we got the right error + Assert.assertThat(response.getErrorDescription(), containsString("OCSP check failed")); + } + + @Test + public void ocspFailOpenLoginSuccess() throws Exception { + // Test of OCSP failure (invalid OCSP responder host) when OCSP Fail-Open is set to ON + // If test is successful, it should continue the login + + X509AuthenticatorConfigModel config = + new X509AuthenticatorConfigModel() + .setOCSPEnabled(true) + .setOCSPFailOpen(true) + .setMappingSourceType(SUBJECTDN_EMAIL) + .setOCSPResponder("http://" + OCSP_RESPONDER_HOST + ".invalid.host:" + OCSP_RESPONDER_PORT + "/oscp") + .setOCSPResponderCertificate( + IOUtils.toString(this.getClass().getResourceAsStream(OcspHandler.OCSP_RESPONDER_CERT_PATH), Charsets.UTF_8) + .replace("-----BEGIN CERTIFICATE-----", "") + .replace("-----END CERTIFICATE-----", "")) + .setUserIdentityMapperType(USERNAME_EMAIL); + AuthenticatorConfigRepresentation cfg = newConfig("x509-directgrant-config", config.getConfig()); + String cfgId = createConfig(directGrantExecution.getId(), cfg); + Assert.assertNotNull(cfgId); + + String keyStorePath = Paths.get(System.getProperty("client.certificate.keystore")) + .getParent().resolve("client-ca.jks").toString(); + String keyStorePassword = System.getProperty("client.certificate.keystore.passphrase"); + String trustStorePath = System.getProperty("client.truststore"); + String trustStorePassword = System.getProperty("client.truststore.passphrase"); + Supplier previous = oauth.getHttpClient(); + try { + oauth.clientId("resource-owner"); + oauth.httpClient(() -> OAuthClient.newCloseableHttpClientSSL(keyStorePath, keyStorePassword, trustStorePath, trustStorePassword)); + OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest("secret", "", "", null); + + // Make sure authentication is allowed + assertEquals(Response.Status.OK.getStatusCode(), response.getStatusCode()); + } finally { + oauth.httpClient(previous); + } + } + +} \ No newline at end of file