KEYCLOAK-8163 Improve SAML validations

adapters/saml/core/src/main/java/org/keycloak/adapters/saml/config/parsers/DeploymentBuilder.java

@@ -42,6 +42,11 @@
 import java.util.Set;
 import org.keycloak.adapters.cloned.HttpClientBuilder;
 import java.net.URI;
+import java.security.cert.CertificateException;
+import java.security.cert.CertificateExpiredException;
+import java.security.cert.CertificateNotYetValidException;
+import java.security.cert.X509Certificate;
+import java.util.logging.Level;
 
 /**
  * @author <a href="mailto:bill@burkecentral.com">Bill Burke</a>
@@ -205,8 +210,13 @@ private void processSigningKey(DefaultSamlDeployment.DefaultIDP idp, Key key, Re
             Certificate cert = null;
             try {
                 cert = keyStore.getCertificate(key.getKeystore().getCertificateAlias());
+                if (cert instanceof X509Certificate) {
+                    ((X509Certificate) cert).checkValidity();
+                }
             } catch (KeyStoreException e) {
                 throw new RuntimeException(e);
+            } catch (CertificateException e) {
+                throw new RuntimeException(e);
             }
             publicKey = cert.getPublicKey();
         } else {
@@ -224,7 +234,12 @@ protected static PublicKey getPublicKeyFromPem(Key key) {
         if (key.getPublicKeyPem() != null) {
             publicKey = PemUtils.decodePublicKey(key.getPublicKeyPem().trim());
         } else {
-            Certificate cert = PemUtils.decodeCertificate(key.getCertificatePem().trim());
+            X509Certificate cert = PemUtils.decodeCertificate(key.getCertificatePem().trim());
+            try {
+                cert.checkValidity();
+            } catch (CertificateException ex) {
+                throw new RuntimeException(ex);
+            }
             publicKey = cert.getPublicKey();
         }
         return publicKey;

adapters/saml/core/src/main/java/org/keycloak/adapters/saml/rotation/SamlDescriptorPublicKeyLocator.java

@@ -36,6 +36,7 @@
 import org.keycloak.dom.saml.v2.metadata.KeyTypes;
 import org.keycloak.rotation.KeyLocator;
 import org.keycloak.saml.processing.api.util.KeyInfoTools;
+import java.security.cert.CertificateException;
 
 /**
  * This class defines a {@link KeyLocator} that looks up public keys and certificates in IdP's
@@ -147,6 +148,11 @@ private synchronized PublicKey refreshCertificateCacheAndGet(String kid) {
         for (KeyInfo ki : signingCerts) {
             KeyName keyName = KeyInfoTools.getKeyName(ki);
             X509Certificate x509certificate = KeyInfoTools.getX509Certificate(ki);
+            try {
+                x509certificate.checkValidity();
+            } catch (CertificateException ex) {
+                x509certificate = null;
+            }
             if (x509certificate != null && keyName != null) {
                 LOG.tracef("Registering signing certificate %s", keyName.getName());
                 this.publicKeyCache.put(keyName.getName(), x509certificate.getPublicKey());

services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java

@@ -88,9 +88,11 @@
 import org.keycloak.rotation.KeyLocator;
 import org.keycloak.saml.processing.core.util.KeycloakKeySamlExtensionGenerator;
 import org.keycloak.saml.validators.DestinationValidator;
+import java.security.cert.CertificateException;
 import org.w3c.dom.Element;
 
 import java.util.*;
+import javax.security.auth.x500.X500Principal;
 import javax.xml.crypto.dsig.XMLSignature;
 import org.w3c.dom.NodeList;
 
@@ -218,9 +220,13 @@ protected KeyLocator getIDPKeyLocator() {
             List<Key> keys = new LinkedList<>();
 
             for (String signingCertificate : config.getSigningCertificates()) {
+                X509Certificate cert = null;
                 try {
-                    X509Certificate cert = XMLSignatureUtil.getX509CertificateFromKeyInfoString(signingCertificate.replaceAll("\\s", ""));
+                    cert = XMLSignatureUtil.getX509CertificateFromKeyInfoString(signingCertificate.replaceAll("\\s", ""));
+                    cert.checkValidity();
                     keys.add(cert.getPublicKey());
+                } catch (CertificateException e) {
+                    logger.warnf("Ignoring invalid certificate: %s", cert);
                 } catch (ProcessingException e) {
                     throw new RuntimeException(e);
                 }

services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java

@@ -29,11 +29,9 @@
 import org.w3c.dom.Document;
 
 import javax.ws.rs.core.MultivaluedMap;
-import javax.ws.rs.core.UriBuilder;
 import javax.ws.rs.core.UriInfo;
 import java.security.PublicKey;
 import java.security.Signature;
-import java.security.cert.Certificate;
 import org.keycloak.dom.saml.v2.SAML2Object;
 import org.keycloak.dom.saml.v2.protocol.ExtensionsType;
 import org.keycloak.dom.saml.v2.protocol.RequestAbstractType;
@@ -42,6 +40,8 @@
 import org.keycloak.rotation.KeyLocator;
 import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder;
 import org.keycloak.saml.processing.core.util.KeycloakKeySamlExtensionGenerator;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
 import org.w3c.dom.Element;
 
 /**
@@ -114,9 +114,12 @@ public static PublicKey getPublicKey(ClientModel client, String attribute) throw
 
     private static PublicKey getPublicKey(String certPem) throws VerificationException {
         if (certPem == null) throw new VerificationException("Client does not have a public key.");
-        Certificate cert = null;
+        X509Certificate cert = null;
         try {
             cert = PemUtils.decodeCertificate(certPem);
+            cert.checkValidity();
+        } catch (CertificateException ex) {
+            throw new VerificationException("Certificate is not valid.");
         } catch (Exception e) {
             throw new VerificationException("Could not decode cert", e);
         }

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java

@@ -1,9 +1,14 @@
 package org.keycloak.testsuite.updaters;
 
+import org.keycloak.admin.client.Keycloak;
 import org.keycloak.admin.client.resource.ClientResource;
+import org.keycloak.admin.client.resource.ClientsResource;
 import org.keycloak.representations.idm.ClientRepresentation;
 import java.io.Closeable;
 import java.util.HashMap;
+import java.util.List;
+import static org.hamcrest.Matchers.hasSize;
+import static org.junit.Assert.assertThat;
 
 /**
  *
@@ -16,6 +21,15 @@ public class ClientAttributeUpdater {
     private final ClientRepresentation rep;
     private final ClientRepresentation origRep;
 
+    public static ClientAttributeUpdater forClient(Keycloak adminClient, String realm, String clientId) {
+        ClientsResource clients = adminClient.realm(realm).clients();
+        List<ClientRepresentation> foundClients = clients.findByClientId(clientId);
+        assertThat(foundClients, hasSize(1));
+        ClientResource clientRes = clients.get(foundClients.get(0).getId());
+
+        return new ClientAttributeUpdater(clientRes);
+    }
+
     public ClientAttributeUpdater(ClientResource clientResource) {
         this.clientResource = clientResource;
         this.origRep = clientResource.toRepresentation();

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/RealmAttributeUpdater.java

@@ -41,6 +41,16 @@ public RealmAttributeUpdater removeAttribute(String name) {
         return this;
     }
 
+    public RealmAttributeUpdater setPublicKey(String key) {
+        this.rep.setPublicKey(key);
+        return this;
+    }
+
+    public RealmAttributeUpdater setPrivateKey(String key) {
+        this.rep.setPrivateKey(key);
+        return this;
+    }
+
     public Closeable update() {
         realmResource.update(rep);
 

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SamlClient.java

@@ -248,7 +248,7 @@ public HttpUriRequest createSamlSignedResponse(URI samlEndpoint, String relaySta
 
             @Override
             public HttpUriRequest createSamlSignedRequest(URI samlEndpoint, String relayState, Document samlRequest, String realmPrivateKey, String realmPublicKey) {
-                return null;
+                throw new UnsupportedOperationException("Not implemented yet.");
             }
         };
 

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/saml/CreateAuthnRequestStepBuilder.java

@@ -39,6 +39,8 @@ public class CreateAuthnRequestStepBuilder extends SamlDocumentStepBuilder<Authn
     private final URI authServerSamlUrl;
     private final Binding requestBinding;
     private final String assertionConsumerURL;
+    private String signingPublicKeyPem;  // TODO: should not be needed
+    private String signingPrivateKeyPem;
 
     private final Document forceLoginRequestDocument;
 
@@ -77,6 +79,12 @@ public void relayState(String relayState) {
         this.relayState = relayState;
     }
 
+    public CreateAuthnRequestStepBuilder signWith(String signingPrivateKeyPem, String signingPublicKeyPem) {
+        this.signingPrivateKeyPem = signingPrivateKeyPem;
+        this.signingPublicKeyPem = signingPublicKeyPem;
+        return this;
+    }
+
     @Override
     public HttpUriRequest perform(CloseableHttpClient client, URI currentURI, CloseableHttpResponse currentResponse, HttpClientContext context) throws Exception {
         Document doc = createLoginRequestDocument();
@@ -88,7 +96,10 @@ public HttpUriRequest perform(CloseableHttpClient client, URI currentURI, Closea
             return null;
         }
 
-        return requestBinding.createSamlUnsignedRequest(authServerSamlUrl, relayState, DocumentUtil.getDocument(transformed));
+        Document samlDoc = DocumentUtil.getDocument(transformed);
+        return this.signingPrivateKeyPem == null
+          ? requestBinding.createSamlUnsignedRequest(authServerSamlUrl, relayState, samlDoc)
+          : requestBinding.createSamlSignedRequest(authServerSamlUrl, relayState, samlDoc, signingPrivateKeyPem, signingPublicKeyPem);
     }
 
     protected Document createLoginRequestDocument() {

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedBrokerTest.java

@@ -7,7 +7,6 @@
 import org.keycloak.protocol.saml.SamlConfigAttributes;
 import org.keycloak.representations.idm.ClientRepresentation;
 import org.keycloak.representations.idm.IdentityProviderRepresentation;
-import org.keycloak.representations.idm.KeysMetadataRepresentation;
 import org.keycloak.representations.idm.RealmRepresentation;
 import org.keycloak.saml.common.constants.JBossSAMLURIConstants;
 import org.keycloak.saml.common.util.DocumentUtil;
@@ -18,6 +17,7 @@
 import org.keycloak.testsuite.saml.AbstractSamlTest;
 import org.keycloak.testsuite.updaters.ClientAttributeUpdater;
 import org.keycloak.testsuite.updaters.IdentityProviderAttributeUpdater;
+import org.keycloak.testsuite.updaters.RealmAttributeUpdater;
 import org.keycloak.testsuite.util.KeyUtils;
 import org.keycloak.testsuite.util.SamlClient;
 import org.keycloak.testsuite.util.SamlClient.Binding;
@@ -30,6 +30,7 @@
 
 import java.util.Map.Entry;
 import java.util.Set;
+import javax.ws.rs.core.Response.Status;
 import org.hamcrest.Matchers;
 import org.junit.Assert;
 import org.junit.Test;
@@ -265,4 +266,44 @@ public void loginUserAllNamespacesInTopElementSignedAssertion() throws Exception
     public void loginUserAllNamespacesInTopElementEncryptedAssertion() throws Exception {
         withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, false, true);
     }
+
+    @Test
+    public void testWithExpiredBrokerCertificate() throws Exception {
+        ClientRepresentation client = adminClient.realm(bc.providerRealmName())
+          .clients()
+          .findByClientId(bc.getIDPClientIdInProviderRealm(suiteContext))
+          .get(0);
+
+        final ClientResource clientResource = realmsResouce().realm(bc.providerRealmName()).clients().get(client.getId());
+        Assert.assertThat(clientResource, Matchers.notNullValue());
+
+        try (Closeable idpUpdater = new IdentityProviderAttributeUpdater(identityProviderResource)
+            .setAttribute(SAMLIdentityProviderConfig.VALIDATE_SIGNATURE, Boolean.toString(true))
+            .setAttribute(SAMLIdentityProviderConfig.WANT_ASSERTIONS_SIGNED, Boolean.toString(true))
+            .setAttribute(SAMLIdentityProviderConfig.WANT_ASSERTIONS_ENCRYPTED, Boolean.toString(false))
+            .setAttribute(SAMLIdentityProviderConfig.WANT_AUTHN_REQUESTS_SIGNED, "true")
+            .setAttribute(SAMLIdentityProviderConfig.SIGNING_CERTIFICATE_KEY, AbstractSamlTest.SAML_CLIENT_SALES_POST_SIG_EXPIRED_CERTIFICATE)
+            .update();
+          Closeable clientUpdater = new ClientAttributeUpdater(clientResource)
+            .setAttribute(SamlConfigAttributes.SAML_ENCRYPT, Boolean.toString(false))
+            .setAttribute(SamlConfigAttributes.SAML_SERVER_SIGNATURE, "true")
+            .setAttribute(SamlConfigAttributes.SAML_ASSERTION_SIGNATURE, Boolean.toString(true))
+            .setAttribute(SamlConfigAttributes.SAML_CLIENT_SIGNATURE_ATTRIBUTE, "false")
+            .update();
+          Closeable realmUpdater = new RealmAttributeUpdater(adminClient.realm(bc.providerRealmName()))
+            .setPublicKey(AbstractSamlTest.SAML_CLIENT_SALES_POST_SIG_EXPIRED_PUBLIC_KEY)
+            .setPrivateKey(AbstractSamlTest.SAML_CLIENT_SALES_POST_SIG_EXPIRED_PRIVATE_KEY)
+            .update())
+        {
+            AuthnRequestType loginRep = SamlClient.createLoginRequestDocument(AbstractSamlTest.SAML_CLIENT_ID_SALES_POST + ".dot/ted", AbstractSamlTest.SAML_ASSERTION_CONSUMER_URL_SALES_POST, null);
+
+            Document doc = SAML2Request.convert(loginRep);
+            new SamlClientBuilder()
+              .authnRequest(getAuthServerSamlEndpoint(bc.consumerRealmName()), doc, Binding.POST).build()   // Request to consumer IdP
+              .login().idp(bc.getIDPAlias()).build()
+
+              .assertResponse(org.keycloak.testsuite.util.Matchers.statusCodeIsHC(Status.BAD_REQUEST));
+        }
+
+    }
 }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/AbstractSamlTest.java

@@ -35,6 +35,11 @@ public abstract class AbstractSamlTest extends AbstractAuthTest {
     public static final String SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY = "MIICdQIBADANBgkqhkiG9w0BAQEFAASCAl8wggJbAgEAAoGBANUbxrvEY3pkiQNt55zJLKBwN+zKmNQw08ThAmOKzwHfXoK+xlDSFxNMtTKJGkeUdnKzaTfESEcEfKYULUA41y/NnOlvjS0CEsc7Wq0Ce63TSSGMB2NHea4tV0aQz/MwLsbmz2IjAFWHA5CHL5WwacIf3UTOSNnhJUSvnkomjJAlAgMBAAECgYANpO2gb/5+g5lSIuNFYov86bJq8r2+ODIW1OE2Rljioc6HSHeiDRF1JuAjECwikRrUVTBTZbnK8jqY14neJsWAKBzGo+ToaQALsNZ9B91DxxL50K5oVOzw5shAS9TnRjN40+KIXFED4ydq4JRdoqb8+cN+N3i0+Cu7tdm+UaHTAQJBAOwFs3ZwqQEqmv9vmgmIFwFpJm1aIw25gEOf3Hy45GP4bL/j0FQgwcXYRbLE5bPqhw/liLKc1GQ97bVm6zs8SvUCQQDnJZA6TFRMiDjezinE1J4e0v4RupyDniVjbE5ArTK5/FRVkjw4Ny0AqZUEyIIqlTeZlCq45pCJy4a2hymDGVJxAj9gzfXNnmezEsZ//kYvoqHM8lPQhifaeTsigW7tuOf0GPCBw+6uksDnZM0xhZCxOoArBPoMSEbU1pGo1Y2lvhUCQF6E5sBgHAybm53Ich4Rz4LNRqWbSIstrR5F2I3sBRU2kInZXZSjQ1zE+7HUCB4/nFfJ1dp8NdiTCEg1Zw072pECQQDnxyQALmWhQbBTl0tq6CwYf9rZDwBzxuY+CXB8Ky1gOmXwan96KZvV4rK8MQQs6HIiYC/j+5lX3A3zlXTFldaz";
     public static final String SAML_CLIENT_SALES_POST_SIG_PUBLIC_KEY = "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDVG8a7xGN6ZIkDbeecySygcDfsypjUMNPE4QJjis8B316CvsZQ0hcTTLUyiRpHlHZys2k3xEhHBHymFC1AONcvzZzpb40tAhLHO1qtAnut00khjAdjR3muLVdGkM/zMC7G5s9iIwBVhwOQhy+VsGnCH91EzkjZ4SVEr55KJoyQJQIDAQAB";
 
+    // Set date to past; then: openssl req -x509 -newkey rsa:1024 -keyout key.pem -out cert.pem -days 1 -nodes -subj '/CN=http:\/\/localhost:8080\/sales-post-sig\/'
+    public static final String SAML_CLIENT_SALES_POST_SIG_EXPIRED_PRIVATE_KEY = "MIICdwIBADANBgkqhkiG9w0BAQEFAASCAmEwggJdAgEAAoGBAMrGzRp3HVf6Ti75rl5mPAPXua8APCCLANikzOd82VI0R8Ml0UAchkfRUBvBedobJIn9r8wwxMeXLmKsMynW52SYeC/Zx5b5K6ayMS3GWJIgqLpp/n1piUeI4sbJXlUj9UtW+QTpGhrHt9n7s7znwoNqGDUkjmyZiekEspjdfzzlAgMBAAECgYBJvPFo5lftXkCAJJucCGFapGAJm3RCAUpVfdhldakxk4FlHaNyRO0vwJX5AeplvekTpQUAo9trGTbs+uHAHT4XWOnwhHHyBRkWdiwXX9bzNdHnIwf/0SLIBBYUk0hoWEDvpklBPqllM215a0sEnB2ykYSsMDBSkFB7Ah+RK7zTAQJBAOw9v7SsfIhOXci9vnkQPuQpL8T4kwj7nWi+YtRGrXbF/bJGwjsgXN5i7otwBV/W+TNzI5H7s2opPUXdIxfP9C0CQQDbvIcxXjwjO1hjXXY4axiT1sxU8Oq1bds033atMoN9pib7IxkWh6ouOQZT8bxwQ2ElH0rswZ0/2CusrIUIekaZAkEAk9UUSQiDKXz4vSzXq8SZxodriDQRNtbVqv0wtSvBUwkU9+HFm+BlnRiFtCYWhuHsseCESs8ad/10hWqbkkQkxQJAZOvN2+rADB5xlhGS/o6RlzUMW+bapcFy8HHB/AI7SjZJqQaRuztL+jbOpTddqOIJeBdLPjoekvgh9wi1gRNH4QJBAMjfB1xYxmztfbUcUuOsATz3s7StprOAukd+hhBiMukxcKhi1IQp7tFhfFe/+xUY3fSh1a3KlyItFKxp68EdDRk=";
+    public static final String SAML_CLIENT_SALES_POST_SIG_EXPIRED_PUBLIC_KEY = "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDKxs0adx1X+k4u+a5eZjwD17mvADwgiwDYpMznfNlSNEfDJdFAHIZH0VAbwXnaGySJ/a/MMMTHly5irDMp1udkmHgv2ceW+SumsjEtxliSIKi6af59aYlHiOLGyV5VI/VLVvkE6Roax7fZ+7O858KDahg1JI5smYnpBLKY3X885QIDAQAB";
+    public static final String SAML_CLIENT_SALES_POST_SIG_EXPIRED_CERTIFICATE = "MIICMTCCAZqgAwIBAgIJAPlizW20Nhe6MA0GCSqGSIb3DQEBCwUAMDAxLjAsBgNVBAMMJWh0dHA6Ly9sb2NhbGhvc3Q6ODA4MC9zYWxlcy1wb3N0LXNpZy8wHhcNMTYwODI5MDg1MjMzWhcNMTYwODMwMDg1MjMzWjAwMS4wLAYDVQQDDCVodHRwOi8vbG9jYWxob3N0OjgwODAvc2FsZXMtcG9zdC1zaWcvMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDKxs0adx1X+k4u+a5eZjwD17mvADwgiwDYpMznfNlSNEfDJdFAHIZH0VAbwXnaGySJ/a/MMMTHly5irDMp1udkmHgv2ceW+SumsjEtxliSIKi6af59aYlHiOLGyV5VI/VLVvkE6Roax7fZ+7O858KDahg1JI5smYnpBLKY3X885QIDAQABo1MwUTAdBgNVHQ4EFgQUE9C6Ck0jsdY+sjN064ZYwYkZJr4wHwYDVR0jBBgwFoAUE9C6Ck0jsdY+sjN064ZYwYkZJr4wDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOBgQBuypHw5DMDBgfI6LcXBiCjpiQP3DLRLdwthh/RfCnZT7PrhXRJV8RMm8EqxqtEgfg2SKqMyA02uxMKH0p277U2iQveSDAaICTJRxtyFm6FERtgLNlsekusC2I14gZpLe84oHDf6L1w3dKFzzLEC9+bHg/XCg/KthWxW8iuVct5qg==";
+
     public static final String SAML_ASSERTION_CONSUMER_URL_SALES_POST_ENC = "http://localhost:8080/sales-post-enc/";
     public static final String SAML_CLIENT_ID_SALES_POST_ENC = "http://localhost:8081/sales-post-enc/";
     public static final String SAML_CLIENT_SALES_POST_ENC_PRIVATE_KEY = "MIICXQIBAAKBgQDb7kwJPkGdU34hicplwfp6/WmNcaLh94TSc7Jyr9Undp5pkyLgb0DE7EIE+6kSs4LsqCb8HDkB0nLD5DXbBJFd8n0WGoKstelvtg6FtVJMnwN7k7yZbfkPECWH9zF70VeOo9vbzrApNRnct8ZhH5fbflRB4JMA9L9R+LbURdoSKQIDAQABAoGBANtbZG9bruoSGp2s5zhzLzd4hczT6Jfk3o9hYjzNb5Z60ymN3Z1omXtQAdEiiNHkRdNxK+EM7TcKBfmoJqcaeTkW8cksVEAW23ip8W9/XsLqmbU2mRrJiKa+KQNDSHqJi1VGyimi4DDApcaqRZcaKDFXg2KDr/Qt5JFD/o9IIIPZAkEA+ZENdBIlpbUfkJh6Ln+bUTss/FZ1FsrcPZWu13rChRMrsmXsfzu9kZUWdUeQ2Dj5AoW2Q7L/cqdGXS7Mm5XhcwJBAOGZq9axJY5YhKrsksvYRLhQbStmGu5LG75suF+rc/44sFq+aQM7+oeRr4VY88Mvz7mk4esdfnk7ae+cCazqJvMCQQCx1L1cZw3yfRSn6S6u8XjQMjWE/WpjulujeoRiwPPY9WcesOgLZZtYIH8nRL6ehEJTnMnahbLmlPFbttxPRUanAkA11MtSIVcKzkhp2KV2ipZrPJWwI18NuVJXb+3WtjypTrGWFZVNNkSjkLnHIeCYlJIGhDd8OL9zAiBXEm6kmgLNAkBWAg0tK2hCjvzsaA505gWQb4X56uKWdb0IzN+fOLB3Qt7+fLqbVQNQoNGzqey6B4MoS1fUKAStqdGTFYPG/+9t";