diff --git a/docs/documentation/server_admin/topics/identity-broker/saml.adoc b/docs/documentation/server_admin/topics/identity-broker/saml.adoc index de74f907794..5b3d372ab86 100644 --- a/docs/documentation/server_admin/topics/identity-broker/saml.adoc +++ b/docs/documentation/server_admin/topics/identity-broker/saml.adoc @@ -167,6 +167,15 @@ Adding a query parameter named `login_hint` to this URL adds the parameter's val Enable the "Pass subject" option to send the subject in SAML requests. +[[_identity_broker_saml_subject_confirmation]] +==== Support for subject confirmation methods + +When brokering a SAML v2.0 Identity Provider, {project_name} implements the SAML V2.0 Web Browser SSO Profile, which requires the assertion to carry at least one `` using the `bearer` method. +{project_name} accepts the assertion only if a bearer confirmation is present and contains a `` element that validates, including the recipient, the `InResponseTo` value, and the validity window. + +{project_name} does not support a subject confirmation method other than `bearer`. +Any other unsupported method is rejected and the login fails. + [[_identity_broker_saml_proxy_restriction]] ==== Support for the ProxyRestriction condition diff --git a/docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc b/docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc index d8664f8dae3..a07d07ed602 100644 --- a/docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc +++ b/docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc @@ -129,6 +129,7 @@ The following conditions are now enforced: * The assertion's `Subject` must contain a `SubjectConfirmation` with `Method` set to Bearer, and it must carry a `SubjectConfirmationData` element. A missing `SubjectConfirmation`, or one without `SubjectConfirmationData`, is now rejected; previously it was silently ignored. If the assertion carries multiple Bearer confirmations, they are treated as alternatives and only one of them needs to validate. * `SubjectConfirmationData` must carry a `NotOnOrAfter` attribute. +* Other subject confirmation methods, including holder-of-key, are not supported; an assertion without a valid bearer confirmation is rejected. * `SubjectConfirmationData` must carry a `Recipient` attribute matching the response's `Destination`, whenever the response specifies one. * For broker logins only, `SubjectConfirmationData` must carry an `InResponseTo` attribute matching the expected request ID, for SP-initiated logins. 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 787fabc0583..31f24f48ddc 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java @@ -1215,7 +1215,7 @@ private boolean validateSubjectConfirmationData(ResponseType responseType, Strin .collect(Collectors.toList()); if (bearerConfirmations.isEmpty()) { - return isSubjectConfirmationDataValid(assertion, null, expectedRequestId, expectedRecipient); + return false; } return bearerConfirmations.stream().anyMatch(subjectConfirmationElement -> diff --git a/tests/base/src/test/java/org/keycloak/tests/broker/saml/KcSamlBrokerTest.java b/tests/base/src/test/java/org/keycloak/tests/broker/saml/KcSamlBrokerTest.java index c82abbd5339..d5fa252f8f3 100644 --- a/tests/base/src/test/java/org/keycloak/tests/broker/saml/KcSamlBrokerTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/broker/saml/KcSamlBrokerTest.java @@ -19,6 +19,7 @@ import org.keycloak.tests.saml.SamlBrowserlessBrokerClient; import org.keycloak.tests.saml.SamlClient; import org.keycloak.tests.saml.SamlClientBuilder; +import org.keycloak.tests.saml.SamlResponseDocument; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -78,6 +79,55 @@ public void testRejectsBearerSubjectConfirmationWithoutRecipient() throws Except assertInvalidSamlResponse(this::removeRecipient); } + @Test + public void testHolderOfKeyOnlySubjectConfirmationIsRejected() throws Exception { + try (SamlBrowserlessBrokerClient broker = new SamlBrowserlessBrokerClient(getConsumerRealm(), SAML_CLIENT_ID_SALES_POST, IDP_SAML_ALIAS)) { + assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), + broker.login(getUserLogin(), getUserPassword()) + .onResponse(response -> response.removeSubjectConfirmations() + .addSubjectConfirmation(SamlResponseDocument.HOLDER_OF_KEY_METHOD)) + .submit()); + } + + assertTrue(getConsumerRealm().admin().users().search(getUserLogin(), true).isEmpty()); + } + + @Test + public void testUnknownSubjectConfirmationMethodIsRejected() throws Exception { + try (SamlBrowserlessBrokerClient broker = new SamlBrowserlessBrokerClient(getConsumerRealm(), SAML_CLIENT_ID_SALES_POST, IDP_SAML_ALIAS)) { + assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), + broker.login(getUserLogin(), getUserPassword()) + .onResponse(response -> response.removeSubjectConfirmations() + .addSubjectConfirmation("urn:example:unsupported")) + .submit()); + } + + assertTrue(getConsumerRealm().admin().users().search(getUserLogin(), true).isEmpty()); + } + + @Test + public void testBearerAndHolderOfKeySubjectConfirmationIsAccepted() throws Exception { + try (SamlBrowserlessBrokerClient broker = new SamlBrowserlessBrokerClient(getConsumerRealm(), SAML_CLIENT_ID_SALES_POST, IDP_SAML_ALIAS)) { + assertEquals(Response.Status.OK.getStatusCode(), + broker.login(getUserLogin(), getUserPassword()) + .onResponse(response -> response.addSubjectConfirmation(SamlResponseDocument.HOLDER_OF_KEY_METHOD)) + .submit()); + } + } + + @Test + public void testInvalidBearerAndHolderOfKeySubjectConfirmationIsRejected() throws Exception { + try (SamlBrowserlessBrokerClient broker = new SamlBrowserlessBrokerClient(getConsumerRealm(), SAML_CLIENT_ID_SALES_POST, IDP_SAML_ALIAS)) { + assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), + broker.login(getUserLogin(), getUserPassword()) + .onResponse(response -> response.expireSubjectConfirmationData(SamlResponseDocument.BEARER_METHOD) + .addSubjectConfirmation(SamlResponseDocument.HOLDER_OF_KEY_METHOD)) + .submit()); + } + + assertTrue(getConsumerRealm().admin().users().search(getUserLogin(), true).isEmpty()); + } + @Test public void testProxyRestrictionRejectedByDefault() throws Exception { try (SamlBrowserlessBrokerClient broker = new SamlBrowserlessBrokerClient(getConsumerRealm(), SAML_CLIENT_ID_SALES_POST, IDP_SAML_ALIAS)) { diff --git a/tests/base/src/test/java/org/keycloak/tests/saml/SamlResponseDocument.java b/tests/base/src/test/java/org/keycloak/tests/saml/SamlResponseDocument.java index 7f1dcb1d8b4..ba54f71c9ef 100644 --- a/tests/base/src/test/java/org/keycloak/tests/saml/SamlResponseDocument.java +++ b/tests/base/src/test/java/org/keycloak/tests/saml/SamlResponseDocument.java @@ -19,7 +19,10 @@ import java.nio.charset.StandardCharsets; import java.security.KeyPair; import java.security.cert.X509Certificate; +import java.util.ArrayList; import java.util.Base64; +import java.util.List; +import java.util.Objects; import org.keycloak.saml.BaseSAML2BindingBuilder; import org.keycloak.saml.SignatureAlgorithm; @@ -42,6 +45,10 @@ public class SamlResponseDocument { private static final String ASSERTION_NS = JBossSAMLURIConstants.ASSERTION_NSURI.get(); + public static final String BEARER_METHOD = JBossSAMLURIConstants.SUBJECT_CONFIRMATION_BEARER.get(); + + public static final String HOLDER_OF_KEY_METHOD = JBossSAMLURIConstants.HOLDER_OF_KEY.get(); + private final org.w3c.dom.Document document; private SamlResponseDocument(org.w3c.dom.Document document) { @@ -138,6 +145,50 @@ private Element getFirstAssertionSubject() { return subject; } + private List getSubjectConfirmations() { + NodeList nodes = getFirstAssertionSubject().getElementsByTagNameNS(ASSERTION_NS, "SubjectConfirmation"); + List confirmations = new ArrayList<>(nodes.getLength()); + for (int i = 0; i < nodes.getLength(); i++) { + confirmations.add((Element) nodes.item(i)); + } + return confirmations; + } + + /** + * Removes every {@code } from the assertion subject. + */ + public SamlResponseDocument removeSubjectConfirmations() { + for (Element confirmation : getSubjectConfirmations()) { + confirmation.getParentNode().removeChild(confirmation); + } + return this; + } + + /** + * Appends a {@code } with the given {@code Method} and no + * {@code }. + */ + public SamlResponseDocument addSubjectConfirmation(String method) { + Element confirmation = createAssertionElement("SubjectConfirmation"); + confirmation.setAttribute("Method", method); + getFirstAssertionSubject().appendChild(confirmation); + return this; + } + + /** + * Expires the {@code } for the specified confirmation method. + */ + public SamlResponseDocument expireSubjectConfirmationData(String method) { + Element data = getSubjectConfirmations().stream() + .filter(confirmation -> method.equals(confirmation.getAttribute("Method"))) + .map(confirmation -> (Element) confirmation.getElementsByTagNameNS(ASSERTION_NS, "SubjectConfirmationData").item(0)) + .filter(Objects::nonNull) + .findFirst() + .orElseThrow(() -> new IllegalStateException("No SubjectConfirmationData found for method " + method)); + data.setAttribute("NotOnOrAfter", "2000-01-01T00:00:00Z"); + return this; + } + /** * Appends a {@code } condition to the first assertion. A {@code Count} * of {@code 0} forbids a proxying party (e.g. an identity broker) from issuing a further assertion.