KEYCLOAK-10786 Check signature presence in SAML broker
(cherry picked from commit ba9f73aaff22eb34c7dec16f4b76d36d855d569b)
This commit is contained in:
Hynek Mlnarik
Bruno Oliveira da Silva
parent
0ce10a3249
commit
97811fdd51
2 changed files with 188 additions and 14 deletions
|
|
@ -94,6 +94,7 @@ import java.security.cert.CertificateException;
|
|||
import org.w3c.dom.Element;
|
||||
|
||||
import java.util.*;
|
||||
import javax.ws.rs.core.MultivaluedMap;
|
||||
import javax.xml.crypto.dsig.XMLSignature;
|
||||
import org.w3c.dom.NodeList;
|
||||
|
||||
|
|
@ -216,6 +217,7 @@ public class SAMLEndpoint {
|
|||
}
|
||||
|
||||
protected abstract String getBindingType();
|
||||
protected abstract boolean containsUnencryptedSignature(SAMLDocumentHolder documentHolder);
|
||||
protected abstract void verifySignature(String key, SAMLDocumentHolder documentHolder) throws VerificationException;
|
||||
protected abstract SAMLDocumentHolder extractRequestDocument(String samlRequest);
|
||||
protected abstract SAMLDocumentHolder extractResponseDocument(String response);
|
||||
|
|
@ -384,8 +386,11 @@ public class SAMLEndpoint {
|
|||
}
|
||||
|
||||
boolean signed = AssertionUtil.isSignedElement(assertionElement);
|
||||
if ((config.isWantAssertionsSigned() && !signed)
|
||||
|| (signed && config.isValidateSignature() && !AssertionUtil.isSignatureValid(assertionElement, getIDPKeyLocator()))) {
|
||||
final boolean assertionSignatureNotExistsWhenRequired = config.isWantAssertionsSigned() && !signed;
|
||||
final boolean signatureNotValid = signed && config.isValidateSignature() && !AssertionUtil.isSignatureValid(assertionElement, getIDPKeyLocator());
|
||||
final boolean hasNoSignatureWhenRequired = ! signed && config.isValidateSignature() && ! containsUnencryptedSignature(holder);
|
||||
|
||||
if (assertionSignatureNotExistsWhenRequired || signatureNotValid || hasNoSignatureWhenRequired) {
|
||||
logger.error("validation failed");
|
||||
event.event(EventType.IDENTITY_PROVIDER_RESPONSE);
|
||||
event.error(Errors.INVALID_SIGNATURE);
|
||||
|
|
@ -545,10 +550,14 @@ public class SAMLEndpoint {
|
|||
|
||||
protected class PostBinding extends Binding {
|
||||
@Override
|
||||
protected void verifySignature(String key, SAMLDocumentHolder documentHolder) throws VerificationException {
|
||||
protected boolean containsUnencryptedSignature(SAMLDocumentHolder documentHolder) {
|
||||
NodeList nl = documentHolder.getSamlDocument().getElementsByTagNameNS(XMLSignature.XMLNS, "Signature");
|
||||
boolean anyElementSigned = (nl != null && nl.getLength() > 0);
|
||||
if ((! anyElementSigned) && (documentHolder.getSamlObject() instanceof ResponseType)) {
|
||||
return (nl != null && nl.getLength() > 0);
|
||||
}
|
||||
|
||||
@Override
|
||||
protected void verifySignature(String key, SAMLDocumentHolder documentHolder) throws VerificationException {
|
||||
if ((! containsUnencryptedSignature(documentHolder)) && (documentHolder.getSamlObject() instanceof ResponseType)) {
|
||||
ResponseType responseType = (ResponseType) documentHolder.getSamlObject();
|
||||
List<ResponseType.RTChoiceType> assertions = responseType.getAssertions();
|
||||
if (! assertions.isEmpty() ) {
|
||||
|
|
@ -577,6 +586,14 @@ public class SAMLEndpoint {
|
|||
}
|
||||
|
||||
protected class RedirectBinding extends Binding {
|
||||
@Override
|
||||
protected boolean containsUnencryptedSignature(SAMLDocumentHolder documentHolder) {
|
||||
MultivaluedMap<String, String> encodedParams = session.getContext().getUri().getQueryParameters(false);
|
||||
String algorithm = encodedParams.getFirst(GeneralConstants.SAML_SIG_ALG_REQUEST_KEY);
|
||||
String signature = encodedParams.getFirst(GeneralConstants.SAML_SIGNATURE_REQUEST_KEY);
|
||||
return algorithm != null && signature != null;
|
||||
}
|
||||
|
||||
@Override
|
||||
protected void verifySignature(String key, SAMLDocumentHolder documentHolder) throws VerificationException {
|
||||
KeyLocator locator = getIDPKeyLocator();
|
||||
|
|
|
|||
|
|
@ -10,6 +10,8 @@ import org.keycloak.representations.idm.RealmRepresentation;
|
|||
import org.keycloak.saml.common.constants.JBossSAMLURIConstants;
|
||||
import org.keycloak.saml.common.util.DocumentUtil;
|
||||
import org.keycloak.saml.processing.api.saml.v2.request.SAML2Request;
|
||||
import org.keycloak.saml.processing.core.parsers.saml.assertion.SAMLAssertionQNames;
|
||||
import org.keycloak.saml.processing.core.parsers.saml.protocol.SAMLProtocolQNames;
|
||||
import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder;
|
||||
import org.keycloak.testsuite.arquillian.SuiteContext;
|
||||
|
||||
|
|
@ -21,28 +23,44 @@ import org.keycloak.testsuite.util.KeyUtils;
|
|||
import org.keycloak.testsuite.util.SamlClient;
|
||||
import org.keycloak.testsuite.util.SamlClient.Binding;
|
||||
import org.keycloak.testsuite.util.SamlClientBuilder;
|
||||
import org.keycloak.testsuite.util.saml.SamlDocumentStepBuilder.Saml2DocumentTransformer;
|
||||
import java.io.Closeable;
|
||||
import java.util.Arrays;
|
||||
import java.util.Collections;
|
||||
import java.util.HashMap;
|
||||
import java.util.HashSet;
|
||||
import java.util.LinkedList;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
|
||||
import java.util.Map.Entry;
|
||||
import java.util.Set;
|
||||
import javax.ws.rs.core.Response.Status;
|
||||
import javax.xml.crypto.dsig.XMLSignature;
|
||||
import javax.xml.namespace.QName;
|
||||
import org.apache.http.HttpResponse;
|
||||
import org.hamcrest.Matcher;
|
||||
import org.hamcrest.Matchers;
|
||||
import org.junit.Assert;
|
||||
import org.junit.Test;
|
||||
import org.w3c.dom.DOMException;
|
||||
import org.w3c.dom.Document;
|
||||
import org.w3c.dom.Element;
|
||||
import org.w3c.dom.NamedNodeMap;
|
||||
import org.w3c.dom.Node;
|
||||
import org.w3c.dom.NodeList;
|
||||
import static org.hamcrest.Matchers.containsString;
|
||||
import static org.hamcrest.Matchers.not;
|
||||
import static org.junit.Assert.assertThat;
|
||||
import static org.keycloak.testsuite.broker.BrokerTestConstants.*;
|
||||
import static org.keycloak.testsuite.util.Matchers.bodyHC;
|
||||
import static org.keycloak.testsuite.util.Matchers.isSamlResponse;
|
||||
|
||||
public class KcSamlSignedBrokerTest extends KcSamlBrokerTest {
|
||||
|
||||
private static final String PRIVATE_KEY = "MIIBVQIBADANBgkqhkiG9w0BAQEFAASCAT8wggE7AgEAAkEAs46ICYPRIkmr8diECmyT59cChTWIEiXYBY3T6OLlZrF8ofVCzbEeoUOmhrtHijxxuKSoqLWP4nNOt3rINtQNBQIDAQABAkBL2nyxuFQTLhhLdPJjDPd2y6gu6ixvrjkSL5ZEHgZXWRHzhTzBT0eRxg/5rJA2NDRMBzTTegaEGkWUt7lF5wDJAiEA5pC+h9NEgqDJSw42I52BOml3II35Z6NlNwl6OMfnD1sCIQDHXUiOIJy4ZcSgv5WGue1KbdNVOT2gop1XzfuyWgtjHwIhAOCjLb9QC3PqC7Tgx8azcnDiyHojWVesTrTsuvQPcAP5AiAkX5OeQrr1NbQTNAEe7IsrmjAFi4T/6stUOsOiPaV4NwIhAJIeyh4foIXIVQ+M4To2koaDFRssxKI9/O72vnZSJ+uA";
|
||||
private static final String PUBLIC_KEY = "MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBALOOiAmD0SJJq/HYhApsk+fXAoU1iBIl2AWN0+ji5WaxfKH1Qs2xHqFDpoa7R4o8cbikqKi1j+JzTrd6yDbUDQUCAwEAAQ==";
|
||||
|
||||
public class KcSamlSignedBrokerConfiguration extends KcSamlBrokerConfiguration {
|
||||
|
||||
@Override
|
||||
|
|
@ -115,7 +133,7 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest {
|
|||
return new KcSamlSignedBrokerConfiguration();
|
||||
}
|
||||
|
||||
public void withSignedEncryptedAssertions(Runnable testBody, boolean signedAssertion, boolean encryptedAssertion) throws Exception {
|
||||
public void withSignedEncryptedAssertions(Runnable testBody, boolean signedDocument, boolean signedAssertion, boolean encryptedAssertion) throws Exception {
|
||||
String providerCert = KeyUtils.getActiveKey(adminClient.realm(bc.providerRealmName()).keys().getKeyMetadata(), Algorithm.RS256).getCertificate();
|
||||
Assert.assertThat(providerCert, Matchers.notNullValue());
|
||||
|
||||
|
|
@ -123,18 +141,20 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest {
|
|||
Assert.assertThat(consumerCert, Matchers.notNullValue());
|
||||
|
||||
try (Closeable idpUpdater = new IdentityProviderAttributeUpdater(identityProviderResource)
|
||||
.setAttribute(SAMLIdentityProviderConfig.VALIDATE_SIGNATURE, Boolean.toString(signedAssertion))
|
||||
.setAttribute(SAMLIdentityProviderConfig.VALIDATE_SIGNATURE, Boolean.toString(signedAssertion || signedDocument))
|
||||
.setAttribute(SAMLIdentityProviderConfig.WANT_ASSERTIONS_SIGNED, Boolean.toString(signedAssertion))
|
||||
.setAttribute(SAMLIdentityProviderConfig.WANT_ASSERTIONS_ENCRYPTED, Boolean.toString(encryptedAssertion))
|
||||
.setAttribute(SAMLIdentityProviderConfig.WANT_AUTHN_REQUESTS_SIGNED, "false")
|
||||
.setAttribute(SAMLIdentityProviderConfig.ENCRYPTION_PUBLIC_KEY, PUBLIC_KEY)
|
||||
.setAttribute(SAMLIdentityProviderConfig.SIGNING_CERTIFICATE_KEY, providerCert)
|
||||
.update();
|
||||
Closeable clientUpdater = ClientAttributeUpdater.forClient(adminClient, bc.providerRealmName(), bc.getIDPClientIdInProviderRealm(suiteContext))
|
||||
.setAttribute(SamlConfigAttributes.SAML_ENCRYPT, Boolean.toString(encryptedAssertion))
|
||||
.setAttribute(SamlConfigAttributes.SAML_ENCRYPTION_CERTIFICATE_ATTRIBUTE, consumerCert)
|
||||
.setAttribute(SamlConfigAttributes.SAML_SERVER_SIGNATURE, "false") // only sign assertions
|
||||
.setAttribute(SamlConfigAttributes.SAML_SERVER_SIGNATURE, Boolean.toString(signedDocument))
|
||||
.setAttribute(SamlConfigAttributes.SAML_ASSERTION_SIGNATURE, Boolean.toString(signedAssertion))
|
||||
.setAttribute(SamlConfigAttributes.SAML_CLIENT_SIGNATURE_ATTRIBUTE, "false")
|
||||
.setAttribute(SamlConfigAttributes.SAML_ENCRYPTION_PRIVATE_KEY_ATTRIBUTE, PRIVATE_KEY)
|
||||
.setAttribute(SamlConfigAttributes.SAML_CLIENT_SIGNATURE_ATTRIBUTE, "false") // Do not require client signature
|
||||
.update())
|
||||
{
|
||||
testBody.run();
|
||||
|
|
@ -143,12 +163,12 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest {
|
|||
|
||||
@Test
|
||||
public void testSignedEncryptedAssertions() throws Exception {
|
||||
withSignedEncryptedAssertions(this::testAssertionSignatureRespected, true, true);
|
||||
withSignedEncryptedAssertions(this::testAssertionSignatureRespected, false, true, true);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSignedAssertion() throws Exception {
|
||||
withSignedEncryptedAssertions(this::testAssertionSignatureRespected, true, false);
|
||||
withSignedEncryptedAssertions(this::testAssertionSignatureRespected, false, true, false);
|
||||
}
|
||||
|
||||
private void testAssertionSignatureRespected() {
|
||||
|
|
@ -245,17 +265,17 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest {
|
|||
|
||||
@Test
|
||||
public void loginUserAllNamespacesInTopElementSignedEncryptedAssertion() throws Exception {
|
||||
withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, true, true);
|
||||
withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, false, true, true);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void loginUserAllNamespacesInTopElementSignedAssertion() throws Exception {
|
||||
withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, true, false);
|
||||
withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, false, true, false);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void loginUserAllNamespacesInTopElementEncryptedAssertion() throws Exception {
|
||||
withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, false, true);
|
||||
withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, false, false, true);
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
@ -289,4 +309,141 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest {
|
|||
}
|
||||
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSignatureTampering_NOsignDoc_NOsignAssert_NOencAssert() throws Exception {
|
||||
loginAttackChangeSignature(false, false, false);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSignatureTampering_NOsignDoc_NOsignAssert_encAssert() throws Exception {
|
||||
loginAttackChangeSignature(false, false, true);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSignatureTampering_NOsignDoc_signAssert_NOencAssert() throws Exception {
|
||||
loginAttackChangeSignature(false, true, false);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSignatureTampering_NOsignDoc_signAssert_encAssert() throws Exception {
|
||||
loginAttackChangeSignature(false, true, true);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSignatureTampering_signDoc_NOsignAssert_NOencAssert() throws Exception {
|
||||
loginAttackChangeSignature(true, false, false);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSignatureTampering_signDoc_NOsignAssert_encAssert() throws Exception {
|
||||
loginAttackChangeSignature(true, false, true);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSignatureTampering_signDoc_signAssert_NOencAssert() throws Exception {
|
||||
loginAttackChangeSignature(true, true, false);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSignatureTampering_signDoc_signAssert_encAssert() throws Exception {
|
||||
loginAttackChangeSignature(true, true, true);
|
||||
}
|
||||
|
||||
private Document removeDocumentSignature(Document orig) {
|
||||
return removeSignatureTag(orig, Collections.singleton(SAMLProtocolQNames.RESPONSE.getQName()));
|
||||
}
|
||||
|
||||
private Document removeAssertionSignature(Document orig) {
|
||||
return removeSignatureTag(orig, Collections.singleton(SAMLAssertionQNames.ASSERTION.getQName()));
|
||||
}
|
||||
|
||||
private Document removeDocumentAndAssertionSignature(Document orig) {
|
||||
return removeSignatureTag(orig,
|
||||
new HashSet<>(Arrays.asList(SAMLProtocolQNames.RESPONSE.getQName(), SAMLAssertionQNames.ASSERTION.getQName()))
|
||||
);
|
||||
}
|
||||
|
||||
private Document removeSignatureTag(Document orig, final Set<QName> qNames) throws DOMException {
|
||||
NodeList sigElements = orig.getElementsByTagNameNS(XMLSignature.XMLNS, "Signature");
|
||||
LinkedList<Node> nodesToRemove = new LinkedList<>();
|
||||
for (int i = 0; i < sigElements.getLength(); i ++) {
|
||||
Node n = sigElements.item(i);
|
||||
final Node p = n.getParentNode();
|
||||
QName q = new QName(p.getNamespaceURI(), p.getLocalName());
|
||||
if (qNames.contains(q)) {
|
||||
nodesToRemove.add(n);
|
||||
}
|
||||
}
|
||||
nodesToRemove.forEach(n -> n.getParentNode().removeChild(n));
|
||||
return orig;
|
||||
}
|
||||
|
||||
private void loginAttackChangeSignature(boolean producerSignDocument, boolean producerSignAssertions, boolean producerEncryptAssertions) throws Exception {
|
||||
log.debug("");
|
||||
|
||||
loginAttackChangeSignature("No changes to SAML document", producerSignDocument, producerSignAssertions, producerEncryptAssertions,
|
||||
t -> t, true);
|
||||
|
||||
// TODO: producerSignAssertions should be removed once there would be option to force check SAML document signature
|
||||
boolean validAfterTamperingWithDocumentSignature = ! producerSignDocument || producerSignAssertions;
|
||||
|
||||
loginAttackChangeSignature("Remove document signature", producerSignDocument, producerSignAssertions, producerEncryptAssertions,
|
||||
this::removeDocumentSignature, validAfterTamperingWithDocumentSignature);
|
||||
|
||||
|
||||
// Tests for assertion signature manipulation follow. Tampering with assertion signature is
|
||||
// relevant only if assertion is not encrypted since signature is part of encrypted data,
|
||||
// hence skipped in the opposite case.
|
||||
if (producerEncryptAssertions) {
|
||||
return;
|
||||
}
|
||||
|
||||
// When assertion signature is removed, the expected document validation passes only
|
||||
// if neither the document was not signed
|
||||
// (otherwise document signature is invalidated by removing signature from the assertion)
|
||||
// nor the assertion is
|
||||
boolean validAfterTamperingWithAssertionSignature = ! producerSignAssertions;
|
||||
|
||||
// When both assertion and document signatures are removed, the document validation passes only
|
||||
// if neiter the document nor assertion were signed
|
||||
boolean validAfterTamperingWithBothDocumentAndAssertionSignature = ! producerSignDocument && ! producerSignAssertions;
|
||||
|
||||
loginAttackChangeSignature("Remove assertion signature", producerSignDocument, producerSignAssertions, producerEncryptAssertions,
|
||||
this::removeAssertionSignature, validAfterTamperingWithAssertionSignature);
|
||||
loginAttackChangeSignature("Remove both document and assertion signature", producerSignDocument, producerSignAssertions, producerEncryptAssertions,
|
||||
this::removeDocumentAndAssertionSignature, validAfterTamperingWithBothDocumentAndAssertionSignature);
|
||||
}
|
||||
|
||||
private void loginAttackChangeSignature(String description,
|
||||
boolean producerSignDocument, boolean producerSignAssertions, boolean producerEncryptAssertions,
|
||||
Saml2DocumentTransformer tr, boolean shouldSucceed) throws Exception {
|
||||
log.infof("producerSignDocument: %s, producerSignAssertions: %s, producerEncryptAssertions: %s", producerSignDocument, producerSignAssertions, producerEncryptAssertions);
|
||||
|
||||
Matcher<HttpResponse> responseFromConsumerMatcher = shouldSucceed
|
||||
? bodyHC(containsString("Update Account Information"))
|
||||
: not(bodyHC(containsString("Update Account Information")));
|
||||
|
||||
AuthnRequestType loginRep = SamlClient.createLoginRequestDocument(AbstractSamlTest.SAML_CLIENT_ID_SALES_POST, AUTH_SERVER_SCHEME + "://localhost:" + AUTH_SERVER_PORT + "/sales-post/saml", null);
|
||||
Document doc = SAML2Request.convert(loginRep);
|
||||
|
||||
withSignedEncryptedAssertions(() -> {
|
||||
new SamlClientBuilder()
|
||||
.authnRequest(getAuthServerSamlEndpoint(bc.consumerRealmName()), doc, Binding.POST).build() // Request to consumer IdP
|
||||
|
||||
.login().idp(bc.getIDPAlias()).build()
|
||||
|
||||
.processSamlResponse(Binding.POST).build() // AuthnRequest to producer IdP
|
||||
|
||||
.login().user(bc.getUserLogin(), bc.getUserPassword()).build()
|
||||
|
||||
.processSamlResponse(Binding.POST) // Response from producer IdP
|
||||
.transformDocument(tr)
|
||||
.build()
|
||||
|
||||
// first-broker flow: if valid request, it displays an update profile page on consumer realm
|
||||
.execute(currentResponse -> assertThat(description, currentResponse, responseFromConsumerMatcher));
|
||||
}, producerSignDocument, producerSignAssertions, producerEncryptAssertions);
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue