From 89c6cda2acaaf7662e103de7f1959908dbd3b357 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20B=C3=A5tstrand?= Date: Thu, 23 Feb 2017 21:28:01 +0100 Subject: [PATCH] Two new configuration options for the Saml broker: * wantAssertionsSigned: This will toggle the flag in the SP Metadata Descriptor, and validate the signature if and only if "Validate signature" is selected. * wantAssertionsEncrypted: This will simply require that the assertion is encrypted. Default behavior is unchanged. The signature validation uses the original XML, and supports therefore an IdP that adds whitespace and line breaks between tags (for example OpenAM). --- .../keycloak/saml/SPMetadataDescriptor.java | 4 +- .../core/saml/v2/util/AssertionUtil.java | 40 +++++++++++++++++-- .../keycloak/broker/saml/SAMLEndpoint.java | 39 +++++++++++++++++- .../broker/saml/SAMLIdentityProvider.java | 3 +- .../saml/SAMLIdentityProviderConfig.java | 16 ++++++++ .../SamlSPDescriptorClientInstallation.java | 3 +- .../testsuite/saml/ValidationTest.java | 2 +- .../messages/admin-messages_en.properties | 6 ++- .../realm-identity-provider-saml.html | 14 +++++++ 9 files changed, 116 insertions(+), 11 deletions(-) diff --git a/saml-core/src/main/java/org/keycloak/saml/SPMetadataDescriptor.java b/saml-core/src/main/java/org/keycloak/saml/SPMetadataDescriptor.java index 9a28137694..5feda2b568 100755 --- a/saml-core/src/main/java/org/keycloak/saml/SPMetadataDescriptor.java +++ b/saml-core/src/main/java/org/keycloak/saml/SPMetadataDescriptor.java @@ -23,10 +23,10 @@ package org.keycloak.saml; */ public class SPMetadataDescriptor { - public static String getSPDescriptor(String binding, String assertionEndpoint, String logoutEndpoint, boolean wantAuthnRequestsSigned, String entityId, String nameIDPolicyFormat, String signingCerts) { + public static String getSPDescriptor(String binding, String assertionEndpoint, String logoutEndpoint, boolean wantAuthnRequestsSigned, boolean wantAssertionsSigned, String entityId, String nameIDPolicyFormat, String signingCerts) { String descriptor = "\n" + - " \n"; if (wantAuthnRequestsSigned && signingCerts != null) { descriptor += signingCerts; diff --git a/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/util/AssertionUtil.java b/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/util/AssertionUtil.java index 89d668052e..bb15d233bf 100755 --- a/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/util/AssertionUtil.java +++ b/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/util/AssertionUtil.java @@ -32,6 +32,7 @@ import org.keycloak.dom.saml.v2.assertion.StatementAbstractType; import org.keycloak.dom.saml.v2.assertion.SubjectType; import org.keycloak.dom.saml.v2.assertion.SubjectType.STSubType; import org.keycloak.dom.saml.v2.protocol.ResponseType; +import org.keycloak.rotation.KeyLocator; import org.keycloak.saml.common.ErrorCodes; import org.keycloak.saml.common.PicketLinkLogger; import org.keycloak.saml.common.PicketLinkLoggerFactory; @@ -41,7 +42,6 @@ import org.keycloak.saml.common.exceptions.ParsingException; import org.keycloak.saml.common.exceptions.ProcessingException; import org.keycloak.saml.common.exceptions.fed.IssueInstantMissingException; import org.keycloak.saml.common.util.DocumentUtil; -import org.keycloak.saml.common.util.StaxParserUtil; import org.keycloak.saml.common.util.StaxUtil; import org.keycloak.saml.processing.api.saml.v2.response.SAML2Response; import org.keycloak.saml.processing.api.saml.v2.sig.SAML2Signature; @@ -287,6 +287,22 @@ public class AssertionUtil { return false; } + /** + * Given an assertion element, validate the signature. + */ + public static boolean isSignatureValid(Element assertionElement, KeyLocator keyLocator) { + try { + Document doc = DocumentUtil.createDocument(); + Node n = doc.importNode(assertionElement, true); + doc.appendChild(n); + + return new SAML2Signature().validate(doc, keyLocator); + } catch (Exception e) { + logger.signatureAssertionValidationError(e); + } + return false; + } + /** * Check whether the assertion has expired * @@ -541,7 +557,23 @@ public class AssertionUtil { return responseType.getAssertions().get(0).getAssertion(); } - public static ResponseType decryptAssertion(ResponseType responseType, PrivateKey privateKey) throws ParsingException, ProcessingException, ConfigurationException { + public static boolean isAssertionEncrypted(ResponseType responseType) throws ProcessingException { + List assertions = responseType.getAssertions(); + + if (assertions.isEmpty()) { + throw new ProcessingException("No assertion from response."); + } + + ResponseType.RTChoiceType rtChoiceType = assertions.get(0); + return rtChoiceType.getEncryptedAssertion() != null; + } + + /** + * This method modifies the given responseType, and replaces the encrypted assertion with a decrypted version. + * + * It returns the assertion element as it was decrypted. This can be used in sginature verification. + */ + public static Element decryptAssertion(ResponseType responseType, PrivateKey privateKey) throws ParsingException, ProcessingException, ConfigurationException { SAML2Response saml2Response = new SAML2Response(); Document doc = saml2Response.convert(responseType); @@ -560,11 +592,11 @@ public class AssertionUtil { SAMLParser parser = new SAMLParser(); JAXPValidationUtil.checkSchemaValidation(decryptedDocumentElement); - AssertionType assertion = (AssertionType) parser.parse(StaxParserUtil.getXMLEventReader(DocumentUtil + AssertionType assertion = (AssertionType) parser.parse(parser.createEventReader(DocumentUtil .getNodeAsStream(decryptedDocumentElement))); responseType.replaceAssertion(oldID, new ResponseType.RTChoiceType(assertion)); - return responseType; + return decryptedDocumentElement; } } \ No newline at end of file diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java index 38e57cb256..60893ba535 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java @@ -49,9 +49,12 @@ import org.keycloak.protocol.saml.SamlProtocolUtils; import org.keycloak.saml.SAML2LogoutResponseBuilder; import org.keycloak.saml.SAMLRequestParser; import org.keycloak.saml.common.constants.GeneralConstants; +import org.keycloak.saml.common.constants.JBossSAMLConstants; import org.keycloak.saml.common.constants.JBossSAMLURIConstants; import org.keycloak.saml.common.exceptions.ConfigurationException; import org.keycloak.saml.common.exceptions.ProcessingException; +import org.keycloak.saml.common.util.DocumentUtil; +import org.keycloak.saml.processing.api.saml.v2.sig.SAML2Signature; import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder; import org.keycloak.saml.processing.core.saml.v2.constants.X500SAMLProfileConstants; import org.keycloak.saml.processing.core.saml.v2.util.AssertionUtil; @@ -74,6 +77,7 @@ import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import javax.ws.rs.core.UriBuilder; import javax.ws.rs.core.UriInfo; +import javax.xml.namespace.QName; import java.io.IOException; import java.security.Key; import java.security.cert.X509Certificate; @@ -83,6 +87,8 @@ import java.util.List; import org.keycloak.rotation.HardcodedKeyLocator; import org.keycloak.rotation.KeyLocator; import org.keycloak.saml.processing.core.util.KeycloakKeySamlExtensionGenerator; +import org.w3c.dom.Document; +import org.w3c.dom.Element; import java.util.*; @@ -344,7 +350,38 @@ public class SAMLEndpoint { if (responseType.getAssertions() == null || responseType.getAssertions().isEmpty()) { return callback.error(relayState, Messages.IDENTITY_PROVIDER_UNEXPECTED_ERROR); } - AssertionType assertion = AssertionUtil.getAssertion(responseType, keys.getPrivateKey()); + + boolean assertionIsEncrypted = AssertionUtil.isAssertionEncrypted(responseType); + + if (config.isWantAssertionsEncrypted() && !assertionIsEncrypted) { + logger.error("The assertion is not encrypted, which is required."); + event.event(EventType.IDENTITY_PROVIDER_RESPONSE); + event.error(Errors.INVALID_SAML_RESPONSE); + return ErrorPage.error(session, Messages.INVALID_REQUESTER); + } + + Element assertionElement; + + if (assertionIsEncrypted) { + // This methods writes the parsed and decrypted assertion back on the responseType parameter: + assertionElement = AssertionUtil.decryptAssertion(responseType, keys.getPrivateKey()); + } else { + /* We verify the assertion using original document to handle cases where the IdP + includes whitespace and/or newlines inside tags. */ + assertionElement = DocumentUtil.getElement(holder.getSamlDocument(), new QName(JBossSAMLConstants.ASSERTION.get())); + } + + if (config.isWantAssertionsSigned() && config.isValidateSignature()) { + if (!AssertionUtil.isSignatureValid(assertionElement, getIDPKeyLocator())) { + logger.error("validation failed"); + event.event(EventType.IDENTITY_PROVIDER_RESPONSE); + event.error(Errors.INVALID_SIGNATURE); + return ErrorPage.error(session, Messages.INVALID_REQUESTER); + } + } + + AssertionType assertion = responseType.getAssertions().get(0).getAssertion(); + SubjectType subject = assertion.getSubject(); SubjectType.STSubType subType = subject.getSubType(); NameIDType subjectNameID = (NameIDType) subType.getBaseID(); diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProvider.java b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProvider.java index 1f8f7933d8..bdfea6a5ef 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProvider.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProvider.java @@ -233,6 +233,7 @@ public class SAMLIdentityProvider extends AbstractIdentityProvider {{:: 'want-authn-requests-signed.tooltip' | translate}} +
+ +
+ +
+ {{:: 'want-assertions-signed.tooltip' | translate}} +
+
+ +
+ +
+ {{:: 'want-assertions-encrypted.tooltip' | translate}} +