From 32f13016fafcaadec9092a8b078d7a7717a18b29 Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Wed, 29 Apr 2020 05:35:48 +0200 Subject: [PATCH] KEYCLOAK-12874 Align Destination field existence check with spec --- .../AbstractSamlAuthenticationHandler.java | 215 ++++++++---------- .../saml/SAML2LogoutRequestBuilder.java | 4 +- .../keycloak/broker/saml/SAMLEndpoint.java | 12 +- .../protocol/saml/SamlProtocolUtils.java | 4 - .../keycloak/protocol/saml/SamlService.java | 105 +++++---- .../testsuite/util/SamlClientBuilder.java | 4 +- .../saml/CreateLogoutRequestStepBuilder.java | 14 +- .../util/saml/SamlDocumentStepBuilder.java | 28 +++ .../testsuite/AbstractKeycloakTest.java | 2 +- .../adapter/AbstractAdapterTest.java | 4 +- .../servlet/SAMLLogoutAdapterTest.java | 135 ++++++++++- .../keycloak/testsuite/saml/LogoutTest.java | 157 ++++++++++--- .../saml/SessionNotOnOrAfterTest.java | 10 +- 13 files changed, 484 insertions(+), 210 deletions(-) diff --git a/adapters/saml/core/src/main/java/org/keycloak/adapters/saml/profile/AbstractSamlAuthenticationHandler.java b/adapters/saml/core/src/main/java/org/keycloak/adapters/saml/profile/AbstractSamlAuthenticationHandler.java index 05cd076b5c..f469146b74 100644 --- a/adapters/saml/core/src/main/java/org/keycloak/adapters/saml/profile/AbstractSamlAuthenticationHandler.java +++ b/adapters/saml/core/src/main/java/org/keycloak/adapters/saml/profile/AbstractSamlAuthenticationHandler.java @@ -87,6 +87,8 @@ import org.keycloak.saml.processing.core.util.KeycloakKeySamlExtensionGenerator; import org.keycloak.saml.processing.core.util.XMLEncryptionUtil; import org.keycloak.saml.validators.ConditionsValidator; import org.keycloak.saml.validators.DestinationValidator; +import javax.xml.crypto.dsig.XMLSignature; +import org.w3c.dom.NodeList; /** * @@ -101,6 +103,34 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic protected final SamlDeployment deployment; protected AuthChallenge challenge; private final DestinationValidator destinationValidator = DestinationValidator.forProtocolMap(null); + private static final AuthChallenge CHALLENGE_EXTRACTION_FAILURE = new AuthChallenge() { + @Override + public boolean challenge(HttpFacade exchange) { + SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.EXTRACTION_FAILURE); + exchange.getRequest().setError(error); + exchange.getResponse().sendError(403); + return true; + } + + @Override + public int getResponseCode() { + return 403; + } + }; + private static final AuthChallenge CHALLENGE_INVALID_SIGNATURE = new AuthChallenge() { + @Override + public boolean challenge(HttpFacade exchange) { + SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.INVALID_SIGNATURE); + exchange.getRequest().setError(error); + exchange.getResponse().sendError(403); + return true; + } + + @Override + public int getResponseCode() { + return 403; + } + }; public AbstractSamlAuthenticationHandler(HttpFacade facade, SamlDeployment deployment, SamlSessionStore sessionStore) { this.facade = facade; @@ -117,7 +147,7 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic } else if (samlResponse != null) { return handleSamlResponse(samlResponse, relayState, onCreateSession); } else if (sessionStore.isLoggedIn()) { - if (verifySSL()) return AuthOutcome.FAILED; + if (verifySSL()) return failedTerminal(); log.debug("AUTHENTICATED: was cached"); return handleRequest(); } @@ -150,12 +180,16 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic } if (holder == null) { log.error("Error parsing SAML document"); - return AuthOutcome.FAILED; + return failedTerminal(); } RequestAbstractType requestAbstractType = (RequestAbstractType) holder.getSamlObject(); + if (requestAbstractType.getDestination() == null && containsUnencryptedSignature(holder, postBinding)) { + log.error("Destination field required."); + return failed(CHALLENGE_EXTRACTION_FAILURE); + } if (! destinationValidator.validate(requestUri, requestAbstractType.getDestination())) { - log.error("expected destination '" + requestUri + "' got '" + requestAbstractType.getDestination() + "'"); - return AuthOutcome.FAILED; + log.error("Expected destination '" + requestUri + "' got '" + requestAbstractType.getDestination() + "'"); + return failedTerminal(); } if (requestAbstractType instanceof LogoutRequestType) { @@ -164,7 +198,7 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic validateSamlSignature(holder, postBinding, GeneralConstants.SAML_REQUEST_KEY); } catch (VerificationException e) { log.error("Failed to verify saml request signature", e); - return AuthOutcome.FAILED; + return failedTerminal(); } } LogoutRequestType logout = (LogoutRequestType) requestAbstractType; @@ -172,7 +206,7 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic } else { log.error("unknown SAML request type"); - return AuthOutcome.FAILED; + return failedTerminal(); } } @@ -194,27 +228,17 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic } if (holder == null) { log.error("Error parsing SAML document"); - challenge = new AuthChallenge() { - @Override - public boolean challenge(HttpFacade exchange) { - SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.EXTRACTION_FAILURE); - exchange.getRequest().setError(error); - exchange.getResponse().sendError(403); - return true; - } - - @Override - public int getResponseCode() { - return 403; - } - }; - return AuthOutcome.FAILED; + return failed(CHALLENGE_EXTRACTION_FAILURE); } final StatusResponseType statusResponse = (StatusResponseType) holder.getSamlObject(); // validate destination + if (statusResponse.getDestination() == null && containsUnencryptedSignature(holder, postBinding)) { + log.error("Destination field required."); + return failed(CHALLENGE_EXTRACTION_FAILURE); + } if (! destinationValidator.validate(requestUri, statusResponse.getDestination())) { log.error("Request URI '" + requestUri + "' does not match SAML request destination '" + statusResponse.getDestination() + "'"); - return AuthOutcome.FAILED; + return failedTerminal(); } if (statusResponse instanceof ResponseType) { @@ -225,21 +249,7 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic } catch (VerificationException e) { log.error("Failed to verify saml response signature", e); - challenge = new AuthChallenge() { - @Override - public boolean challenge(HttpFacade exchange) { - SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.INVALID_SIGNATURE, statusResponse); - exchange.getRequest().setError(error); - exchange.getResponse().sendError(403); - return true; - } - - @Override - public int getResponseCode() { - return 403; - } - }; - return AuthOutcome.FAILED; + return failed(CHALLENGE_INVALID_SIGNATURE); } } return handleLoginResponse(holder, postBinding, onCreateSession); @@ -255,7 +265,7 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic validateSamlSignature(holder, postBinding, GeneralConstants.SAML_RESPONSE_KEY); } catch (VerificationException e) { log.error("Failed to verify saml response signature", e); - return AuthOutcome.FAILED; + return failedTerminal(); } } return handleLogoutResponse(holder, statusResponse, relayState); @@ -273,21 +283,7 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic return AuthOutcome.NOT_AUTHENTICATED; } - challenge = new AuthChallenge() { - @Override - public boolean challenge(HttpFacade exchange) { - SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.ERROR_STATUS, statusResponse); - exchange.getRequest().setError(error); - exchange.getResponse().sendError(403); - return true; - } - - @Override - public int getResponseCode() { - return 403; - } - }; - return AuthOutcome.FAILED; + return failed(createAuthChallenge403(statusResponse)); } finally { sessionStore.setCurrentAction(SamlSessionStore.CurrentAction.NONE); } @@ -297,6 +293,17 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic } + private boolean containsUnencryptedSignature(SAMLDocumentHolder documentHolder, boolean postBinding) { + if (postBinding) { + Document signedDoc = documentHolder.getSamlDocument(); + NodeList nl = signedDoc.getElementsByTagNameNS(XMLSignature.XMLNS, "Signature"); + return nl != null && nl.getLength() > 0; + } else { + String algorithm = facade.getRequest().getQueryParamValue(GeneralConstants.SAML_SIG_ALG_REQUEST_KEY); + return algorithm != null; + } + } + private void validateSamlSignature(SAMLDocumentHolder holder, boolean postBinding, String paramKey) throws VerificationException { KeyLocator signatureValidationKey = deployment.getIDP().getSignatureValidationKeyLocator(); if (postBinding) { @@ -348,21 +355,7 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic final ResponseType responseType = (ResponseType) responseHolder.getSamlObject(); AssertionType assertion = null; if (! isSuccessfulSamlResponse(responseType) || responseType.getAssertions() == null || responseType.getAssertions().isEmpty()) { - challenge = new AuthChallenge() { - @Override - public boolean challenge(HttpFacade exchange) { - SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.ERROR_STATUS, responseType); - exchange.getRequest().setError(error); - exchange.getResponse().sendError(403); - return true; - } - - @Override - public int getResponseCode() { - return 403; - } - }; - return AuthOutcome.FAILED; + return failed(createAuthChallenge403(responseType)); } try { assertion = AssertionUtil.getAssertion(responseHolder, responseType, deployment.getDecryptionKey()); @@ -382,21 +375,7 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic } } catch (Exception e) { log.error("Error extracting SAML assertion: " + e.getMessage()); - challenge = new AuthChallenge() { - @Override - public boolean challenge(HttpFacade exchange) { - SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.EXTRACTION_FAILURE); - exchange.getRequest().setError(error); - exchange.getResponse().sendError(403); - return true; - } - - @Override - public int getResponseCode() { - return 403; - } - }; - return AuthOutcome.FAILED; + return failed(CHALLENGE_EXTRACTION_FAILURE); } Element assertionElement = null; @@ -405,42 +384,11 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic assertionElement = getAssertionFromResponse(responseHolder); if (!AssertionUtil.isSignatureValid(assertionElement, deployment.getIDP().getSignatureValidationKeyLocator())) { log.error("Failed to verify saml assertion signature"); - - challenge = new AuthChallenge() { - - @Override - public boolean challenge(HttpFacade exchange) { - SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.INVALID_SIGNATURE, responseType); - exchange.getRequest().setError(error); - exchange.getResponse().sendError(403); - return true; - } - - @Override - public int getResponseCode() { - return 403; - } - }; - return AuthOutcome.FAILED; + return failed(CHALLENGE_INVALID_SIGNATURE); } } catch (Exception e) { log.error("Error processing validation of SAML assertion: " + e.getMessage()); - challenge = new AuthChallenge() { - - @Override - public boolean challenge(HttpFacade exchange) { - SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.EXTRACTION_FAILURE); - exchange.getRequest().setError(error); - exchange.getResponse().sendError(403); - return true; - } - - @Override - public int getResponseCode() { - return 403; - } - }; - return AuthOutcome.FAILED; + return failed(CHALLENGE_EXTRACTION_FAILURE); } } @@ -546,6 +494,20 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic return AuthOutcome.AUTHENTICATED; } + private AuthOutcome failed(AuthChallenge challenge) { + this.challenge = challenge; + return AuthOutcome.FAILED; + } + + /** + * Used to indicate failure without returning a challenge back to caller. + * @param challenge + * @return + */ + private AuthOutcome failedTerminal() { + return failed(null); + } + private boolean isSuccessfulSamlResponse(ResponseType responseType) { return responseType != null && responseType.getStatus() != null @@ -801,4 +763,25 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic return true; } + + private static AuthChallenge createAuthChallenge(final int httpError, final SamlAuthenticationError error) { + return new AuthChallenge() { + @Override + public boolean challenge(HttpFacade exchange) { + exchange.getRequest().setError(error); + exchange.getResponse().sendError(httpError); + return true; + } + + @Override + public int getResponseCode() { + return httpError; + } + }; + } + + private static AuthChallenge createAuthChallenge403(final StatusResponseType responseType) { + return createAuthChallenge(403, new SamlAuthenticationError(SamlAuthenticationError.Reason.ERROR_STATUS, responseType)); + } + } diff --git a/saml-core/src/main/java/org/keycloak/saml/SAML2LogoutRequestBuilder.java b/saml-core/src/main/java/org/keycloak/saml/SAML2LogoutRequestBuilder.java index 27063242c9..1a1b5dd875 100755 --- a/saml-core/src/main/java/org/keycloak/saml/SAML2LogoutRequestBuilder.java +++ b/saml-core/src/main/java/org/keycloak/saml/SAML2LogoutRequestBuilder.java @@ -118,7 +118,9 @@ public class SAML2LogoutRequestBuilder implements SamlProtocolExtensionsAwareBui if (assertionExpiration > 0) lort.setNotOnOrAfter(XMLTimeUtil.add(lort.getIssueInstant(), assertionExpiration * 1000)); - lort.setDestination(URI.create(destination)); + if (destination != null) { + lort.setDestination(URI.create(destination)); + } if (! this.extensions.isEmpty()) { ExtensionsType extensionsType = new ExtensionsType(); 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 e34476b809..8ce9126b08 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java @@ -257,6 +257,11 @@ public class SAMLEndpoint { SAMLDocumentHolder holder = extractRequestDocument(samlRequest); RequestAbstractType requestAbstractType = (RequestAbstractType) holder.getSamlObject(); // validate destination + if (requestAbstractType.getDestination() == null && containsUnencryptedSignature(holder)) { + event.detail(Details.REASON, "missing_required_destination"); + event.error(Errors.INVALID_REQUEST); + return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); + } if (! destinationValidator.validate(session.getContext().getUri().getAbsolutePath(), requestAbstractType.getDestination())) { event.event(EventType.IDENTITY_PROVIDER_RESPONSE); event.detail(Details.REASON, "invalid_destination"); @@ -515,11 +520,16 @@ public class SAMLEndpoint { } StatusResponseType statusResponse = (StatusResponseType)holder.getSamlObject(); // validate destination + if (statusResponse.getDestination() == null && containsUnencryptedSignature(holder)) { + event.detail(Details.REASON, "missing_required_destination"); + event.error(Errors.INVALID_SAML_LOGOUT_RESPONSE); + return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); + } if (! destinationValidator.validate(session.getContext().getUri().getAbsolutePath(), statusResponse.getDestination())) { event.event(EventType.IDENTITY_PROVIDER_RESPONSE); event.detail(Details.REASON, "invalid_destination"); event.error(Errors.INVALID_SAML_RESPONSE); - return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_FEDERATED_IDENTITY_ACTION); + return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); } if (config.isValidateSignature()) { try { diff --git a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java b/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java index dd5eb7aaf6..aa69efbd91 100755 --- a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java +++ b/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java @@ -60,10 +60,6 @@ public class SamlProtocolUtils { * @throws VerificationException */ public static void verifyDocumentSignature(ClientModel client, Document document) throws VerificationException { - SamlClient samlClient = new SamlClient(client); - if (!samlClient.requiresClientSignature()) { - return; - } PublicKey publicKey = getSignatureValidationKey(client); verifyDocumentSignature(document, new HardcodedKeyLocator(publicKey)); } diff --git a/services/src/main/java/org/keycloak/protocol/saml/SamlService.java b/services/src/main/java/org/keycloak/protocol/saml/SamlService.java index 06d949a0db..2c267143ce 100755 --- a/services/src/main/java/org/keycloak/protocol/saml/SamlService.java +++ b/services/src/main/java/org/keycloak/protocol/saml/SamlService.java @@ -94,6 +94,10 @@ import org.keycloak.saml.processing.core.util.KeycloakKeySamlExtensionGenerator; import org.keycloak.saml.validators.DestinationValidator; import org.keycloak.sessions.AuthenticationSessionModel; import java.nio.charset.StandardCharsets; +import javax.ws.rs.core.MultivaluedMap; +import javax.xml.crypto.dsig.XMLSignature; +import org.w3c.dom.Document; +import org.w3c.dom.NodeList; /** * Resource class for the saml connect token service @@ -152,6 +156,11 @@ public class SamlService extends AuthorizationEndpointBase { StatusResponseType statusResponse = (StatusResponseType) holder.getSamlObject(); // validate destination + if (statusResponse.getDestination() == null && containsUnencryptedSignature(holder)) { + event.detail(Details.REASON, "missing_required_destination"); + event.error(Errors.INVALID_SAML_LOGOUT_RESPONSE); + return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); + } if (! destinationValidator.validate(this.getExpectedDestinationUri(session), statusResponse.getDestination())) { event.detail(Details.REASON, "invalid_destination"); event.error(Errors.INVALID_SAML_LOGOUT_RESPONSE); @@ -206,9 +215,16 @@ public class SamlService extends AuthorizationEndpointBase { SAML2Object samlObject = documentHolder.getSamlObject(); - if (! (samlObject instanceof RequestAbstractType)) { + if (samlObject instanceof AuthnRequestType) { + logger.debug("** login request"); event.event(EventType.LOGIN); - event.error(Errors.INVALID_SAML_AUTHN_REQUEST); + } else if (samlObject instanceof LogoutRequestType) { + logger.debug("** logout request"); + event.event(EventType.LOGOUT); + } else { + event.event(EventType.LOGIN); + event.error(Errors.INVALID_TOKEN); + event.detail(Details.REASON, "Unhandled SAML document type: " + (samlObject == null ? "" : samlObject.getClass().getSimpleName())); return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); } @@ -218,82 +234,75 @@ public class SamlService extends AuthorizationEndpointBase { ClientModel client = realm.getClientByClientId(issuer); if (client == null) { - event.event(EventType.LOGIN); event.client(issuer); event.error(Errors.CLIENT_NOT_FOUND); return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.UNKNOWN_LOGIN_REQUESTER); } if (!client.isEnabled()) { - event.event(EventType.LOGIN); event.error(Errors.CLIENT_DISABLED); return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.LOGIN_REQUESTER_NOT_ENABLED); } if (client.isBearerOnly()) { - event.event(EventType.LOGIN); event.error(Errors.NOT_ALLOWED); return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.BEARER_ONLY); } if (!client.isStandardFlowEnabled()) { - event.event(EventType.LOGIN); event.error(Errors.NOT_ALLOWED); return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.STANDARD_FLOW_DISABLED); } if (!isClientProtocolCorrect(client)) { - event.event(EventType.LOGIN); event.error(Errors.INVALID_CLIENT); return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, "Wrong client protocol."); } session.getContext().setClient(client); + SamlClient samlClient = new SamlClient(client); try { - verifySignature(documentHolder, client); + if (samlClient.requiresClientSignature()) { + verifySignature(documentHolder, client); + } } catch (VerificationException e) { SamlService.logger.error("request validation failed", e); - event.event(EventType.LOGIN); event.error(Errors.INVALID_SIGNATURE); return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUESTER); } logger.debug("verified request"); + + if (requestAbstractType.getDestination() == null && containsUnencryptedSignature(documentHolder)) { + event.detail(Details.REASON, "missing_required_destination"); + event.error(Errors.INVALID_REQUEST); + return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); + } + if (samlObject instanceof AuthnRequestType) { - logger.debug("** login request"); - event.event(EventType.LOGIN); // Get the SAML Request Message AuthnRequestType authn = (AuthnRequestType) samlObject; return loginRequest(relayState, authn, client); } else if (samlObject instanceof LogoutRequestType) { - logger.debug("** logout request"); - event.event(EventType.LOGOUT); LogoutRequestType logout = (LogoutRequestType) samlObject; return logoutRequest(logout, client, relayState); - } else { - event.event(EventType.LOGIN); - event.error(Errors.INVALID_TOKEN); - return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); + throw new IllegalStateException("Invalid SAML object"); } } protected abstract void verifySignature(SAMLDocumentHolder documentHolder, ClientModel client) throws VerificationException; + protected abstract boolean containsUnencryptedSignature(SAMLDocumentHolder documentHolder); + protected abstract SAMLDocumentHolder extractRequestDocument(String samlRequest); protected abstract SAMLDocumentHolder extractResponseDocument(String response); protected Response loginRequest(String relayState, AuthnRequestType requestAbstractType, ClientModel client) { SamlClient samlClient = new SamlClient(client); - // validate destination - if (requestAbstractType.getDestination() == null && samlClient.requiresClientSignature()) { - event.detail(Details.REASON, "invalid_destination"); - event.error(Errors.INVALID_SAML_AUTHN_REQUEST); - return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); - } - if (! destinationValidator.validate(this.getExpectedDestinationUri(session), requestAbstractType.getDestination())) { - event.detail(Details.REASON, "invalid_destination"); - event.error(Errors.INVALID_SAML_AUTHN_REQUEST); + + if (! validateDestination(requestAbstractType, samlClient, Errors.INVALID_SAML_AUTHN_REQUEST)) { return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); } + String bindingType = getBindingType(requestAbstractType); if (samlClient.forcePostBinding()) bindingType = SamlProtocol.SAML_POST_BINDING; @@ -397,15 +406,7 @@ public class SamlService extends AuthorizationEndpointBase { protected Response logoutRequest(LogoutRequestType logoutRequest, ClientModel client, String relayState) { SamlClient samlClient = new SamlClient(client); - // validate destination - if (logoutRequest.getDestination() == null && samlClient.requiresClientSignature()) { - event.detail(Details.REASON, "invalid_destination"); - event.error(Errors.INVALID_SAML_LOGOUT_REQUEST); - return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); - } - if (! destinationValidator.validate(this.getExpectedDestinationUri(session), logoutRequest.getDestination())) { - event.detail(Details.REASON, "invalid_destination"); - event.error(Errors.INVALID_SAML_LOGOUT_REQUEST); + if (! validateDestination(logoutRequest, samlClient, Errors.INVALID_SAML_LOGOUT_REQUEST)) { return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); } @@ -501,6 +502,21 @@ public class SamlService extends AuthorizationEndpointBase { } } + private boolean validateDestination(RequestAbstractType req, SamlClient samlClient, String errorCode) { + // validate destination + if (req.getDestination() == null && samlClient.requiresClientSignature()) { + event.detail(Details.REASON, "missing_destination_required"); + event.error(errorCode); + return false; + } + if (! destinationValidator.validate(this.getExpectedDestinationUri(session), req.getDestination())) { + event.detail(Details.REASON, "invalid_destination"); + event.error(errorCode); + return false; + } + return true; + } + private boolean checkSsl() { if (session.getContext().getUri().getBaseUri().getScheme().equals("https")) { return true; @@ -539,6 +555,13 @@ public class SamlService extends AuthorizationEndpointBase { SamlProtocolUtils.verifyDocumentSignature(client, documentHolder.getSamlDocument()); } + @Override + protected boolean containsUnencryptedSignature(SAMLDocumentHolder documentHolder) { + Document signedDoc = documentHolder.getSamlDocument(); + NodeList nl = signedDoc.getElementsByTagNameNS(XMLSignature.XMLNS, "Signature"); + return nl != null && nl.getLength() > 0; + } + @Override protected SAMLDocumentHolder extractRequestDocument(String samlRequest) { return SAMLRequestParser.parseRequestPostBinding(samlRequest); @@ -560,15 +583,19 @@ public class SamlService extends AuthorizationEndpointBase { @Override protected void verifySignature(SAMLDocumentHolder documentHolder, ClientModel client) throws VerificationException { - SamlClient samlClient = new SamlClient(client); - if (!samlClient.requiresClientSignature()) { - return; - } PublicKey publicKey = SamlProtocolUtils.getSignatureValidationKey(client); KeyLocator clientKeyLocator = new HardcodedKeyLocator(publicKey); SamlProtocolUtils.verifyRedirectSignature(documentHolder, clientKeyLocator, session.getContext().getUri(), GeneralConstants.SAML_REQUEST_KEY); } + @Override + protected boolean containsUnencryptedSignature(SAMLDocumentHolder documentHolder) { + KeycloakUriInfo uriInformation = session.getContext().getUri(); + MultivaluedMap encodedParams = uriInformation.getQueryParameters(false); + String algorithm = encodedParams.getFirst(GeneralConstants.SAML_SIG_ALG_REQUEST_KEY); + return algorithm != null; + } + @Override protected SAMLDocumentHolder extractRequestDocument(String samlRequest) { return SAMLRequestParser.parseRequestRedirectBinding(samlRequest); diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SamlClientBuilder.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SamlClientBuilder.java index 77fa65d193..4cdcf2eb08 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SamlClientBuilder.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SamlClientBuilder.java @@ -114,7 +114,7 @@ public class SamlClientBuilder { return this; } - public SamlClientBuilder assertResponse(Matcher matcher) { + public SamlClientBuilder assertResponse(Matcher matcher) { steps.add((client, currentURI, currentResponse, context) -> { Assert.assertThat(currentResponse, matcher); return null; @@ -122,7 +122,7 @@ public class SamlClientBuilder { return this; } - public SamlClientBuilder assertResponse(Consumer consumer) { + public SamlClientBuilder assertResponse(Consumer consumer) { steps.add((client, currentURI, currentResponse, context) -> { consumer.accept(currentResponse); return null; diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/saml/CreateLogoutRequestStepBuilder.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/saml/CreateLogoutRequestStepBuilder.java index 5775a9da05..60dd9c14b2 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/saml/CreateLogoutRequestStepBuilder.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/saml/CreateLogoutRequestStepBuilder.java @@ -42,6 +42,8 @@ public class CreateLogoutRequestStepBuilder extends SamlDocumentStepBuilder sessionIndex = () -> null; private Supplier nameId = () -> null; private Supplier relayState = () -> null; + private String signingPublicKeyPem; // TODO: should not be needed + private String signingPrivateKeyPem; public CreateLogoutRequestStepBuilder(URI authServerSamlUrl, String issuer, Binding requestBinding, SamlClientBuilder clientBuilder) { super(clientBuilder); @@ -92,10 +94,16 @@ public class CreateLogoutRequestStepBuilder extends SamlDocumentStepBuilder tr) { + return transformObject(so -> { + tr.accept(so); + return so; + }); + } + @SuppressWarnings("unchecked") public This transformObject(Saml2ObjectTransformer tr) { final StringTransformer original = this.transformer; @@ -117,6 +125,13 @@ public abstract class SamlDocumentStepBuilder tr) { + return transformDocument(so -> { + tr.accept(so); + return so; + }); + } + @SuppressWarnings("unchecked") public This transformDocument(Saml2DocumentTransformer tr) { final StringTransformer original = this.transformer; @@ -133,6 +148,13 @@ public abstract class SamlDocumentStepBuilder tr) { + return transformString(s -> { + tr.accept(s); + return s; + }); + } + @SuppressWarnings("unchecked") public This transformString(StringTransformer tr) { final StringTransformer original = this.transformer; @@ -148,6 +170,12 @@ public abstract class SamlDocumentStepBuilder updaterOfThis) { + updaterOfThis.accept((This) this); + return (This) this; + } + public SamlClientBuilder build() { return this.clientBuilder; } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AbstractKeycloakTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AbstractKeycloakTest.java index 7be6667000..6708450955 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AbstractKeycloakTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AbstractKeycloakTest.java @@ -370,7 +370,7 @@ public abstract class AbstractKeycloakTest { private void modifySamlAttributes(ClientRepresentation cr) { if (cr.getProtocol() != null && cr.getProtocol().equals("saml")) { - log.info("Modifying attributes of SAML client: " + cr.getClientId()); + log.debug("Modifying attributes of SAML client: " + cr.getClientId()); for (Map.Entry entry : cr.getAttributes().entrySet()) { cr.getAttributes().put(entry.getKey(), replaceHttpValuesWithHttps(entry.getValue())); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/AbstractAdapterTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/AbstractAdapterTest.java index f7bcd36b6d..679f165e64 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/AbstractAdapterTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/AbstractAdapterTest.java @@ -198,7 +198,7 @@ public abstract class AbstractAdapterTest extends AbstractAuthTest { if (realm.getClients() != null) { for (ClientRepresentation client : realm.getClients()) { if (client.getProtocol() != null && client.getProtocol().equals("saml")) { - log.info("Modifying attributes of SAML client: " + client.getClientId()); + log.debug("Modifying attributes of SAML client: " + client.getClientId()); for (Map.Entry entry : client.getAttributes().entrySet()) { client.getAttributes().put(entry.getKey(), entry.getValue().replaceAll(regex, replacement)); } @@ -211,7 +211,7 @@ public abstract class AbstractAdapterTest extends AbstractAuthTest { if (realm.getClients() != null) { for (ClientRepresentation client : realm.getClients()) { if (client.getProtocol() != null && client.getProtocol().equals("saml")) { - log.info("Modifying master URL of SAML client: " + client.getClientId()); + log.debug("Modifying master URL of SAML client: " + client.getClientId()); String masterUrl = client.getAdminUrl(); if (masterUrl == null) { masterUrl = client.getBaseUrl(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/SAMLLogoutAdapterTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/SAMLLogoutAdapterTest.java index 2c24750e1a..ace2122fcb 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/SAMLLogoutAdapterTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/SAMLLogoutAdapterTest.java @@ -2,6 +2,7 @@ package org.keycloak.testsuite.adapter.servlet; import org.keycloak.dom.saml.v2.SAML2Object; import org.keycloak.dom.saml.v2.assertion.AssertionType; +import org.keycloak.dom.saml.v2.assertion.AuthnStatementType; import org.keycloak.dom.saml.v2.assertion.NameIDType; import org.keycloak.dom.saml.v2.protocol.LogoutRequestType; import org.keycloak.dom.saml.v2.protocol.ResponseType; @@ -10,23 +11,39 @@ import org.keycloak.saml.common.constants.JBossSAMLURIConstants; import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder; import org.keycloak.testsuite.adapter.AbstractServletsAdapterTest; import org.keycloak.testsuite.adapter.page.EmployeeServlet; +import org.keycloak.testsuite.adapter.page.SalesPostServlet; import org.keycloak.testsuite.arquillian.annotation.AppServerContainer; +import org.keycloak.testsuite.util.Matchers; import org.keycloak.testsuite.util.SamlClient.Binding; import org.keycloak.testsuite.util.SamlClientBuilder; +import org.keycloak.testsuite.util.saml.CreateLogoutRequestStepBuilder; import org.keycloak.testsuite.utils.arquillian.ContainerConstants; import org.keycloak.testsuite.utils.io.IOUtil; +import java.io.IOException; +import java.net.URI; import java.util.List; import java.util.concurrent.atomic.AtomicReference; -import org.hamcrest.Matchers; +import java.util.function.Consumer; +import javax.ws.rs.core.Response.Status; +import org.apache.http.HttpResponse; +import org.apache.http.client.methods.CloseableHttpResponse; import org.jboss.arquillian.container.test.api.Deployment; import org.jboss.arquillian.graphene.page.Page; import org.jboss.shrinkwrap.api.spec.WebArchive; import org.junit.Test; +import static org.hamcrest.Matchers.anyOf; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertThat; import static org.keycloak.testsuite.adapter.AbstractServletsAdapterTest.samlServletDeployment; +import static org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest.FORBIDDEN_TEXT; +import static org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest.WEBSPHERE_FORBIDDEN_TEXT; +import static org.keycloak.testsuite.saml.AbstractSamlTest.SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY; +import static org.keycloak.testsuite.saml.AbstractSamlTest.SAML_CLIENT_SALES_POST_SIG_PUBLIC_KEY; import static org.keycloak.testsuite.util.Matchers.isSamlResponse; +import static org.keycloak.testsuite.util.SamlClient.Binding.POST; +import static org.keycloak.testsuite.util.SamlClient.Binding.REDIRECT; /** * @@ -52,16 +69,30 @@ public class SAMLLogoutAdapterTest extends AbstractServletsAdapterTest { return samlServletDeployment(EmployeeServlet.DEPLOYMENT_NAME, SendUsernameServlet.class); } + @Deployment(name = SalesPostServlet.DEPLOYMENT_NAME) + protected static WebArchive sales() { + return samlServletDeployment(SalesPostServlet.DEPLOYMENT_NAME, SendUsernameServlet.class); + } + @Page private EmployeeServlet employeeServletPage; + @Page + private SalesPostServlet salesPostServlet; + private final AtomicReference nameIdRef = new AtomicReference<>(); + private final AtomicReference sessionIndexRef = new AtomicReference<>(); @Override public void addAdapterTestRealms(List testRealms) { testRealms.add(IOUtil.loadRealm("/adapter-test/keycloak-saml/testsaml.json")); } + @Override + protected boolean isImportAfterEachMethod() { + return false; + } + private SAML2Object extractNameId(SAML2Object so) { assertThat(so, isSamlResponse(JBossSAMLURIConstants.STATUS_SUCCESS)); ResponseType loginResp1 = (ResponseType) so; @@ -70,14 +101,16 @@ public class SAMLLogoutAdapterTest extends AbstractServletsAdapterTest { assertThat(firstAssertion.getSubject().getSubType().getBaseID(), instanceOf(NameIDType.class)); NameIDType nameId = (NameIDType) firstAssertion.getSubject().getSubType().getBaseID(); + AuthnStatementType firstAssertionStatement = (AuthnStatementType) firstAssertion.getStatements().iterator().next(); nameIdRef.set(nameId); + sessionIndexRef.set(firstAssertionStatement.getSessionIndex()); return so; } @Test - public void employeeTest() { + public void employeeGlobalLogoutTest() { SAMLDocumentHolder b = new SamlClientBuilder() .navigateTo(employeeServletPage) .processSamlResponse(Binding.POST) @@ -93,7 +126,6 @@ public class SAMLLogoutAdapterTest extends AbstractServletsAdapterTest { t.setNameQualifier(NAME_QUALIFIER); t.setSPNameQualifier(SP_NAME_QUALIFIER); t.setSPProvidedID(SP_PROVIDED_ID); - return o; }).build() .navigateTo(employeeServletPage.getUriBuilder().clone().queryParam("GLO", "true").build()) .getSamlResponse(Binding.POST); @@ -108,4 +140,101 @@ public class SAMLLogoutAdapterTest extends AbstractServletsAdapterTest { assertThat(logoutRequestNameID.getSPNameQualifier(), is(SP_NAME_QUALIFIER)); } + @Test + public void testLogoutDestinationOptionalIfUnsignedRedirect() throws IOException { + testLogoutDestination(REDIRECT, + builder -> builder.transformObject(logoutReq -> { logoutReq.setDestination(null); }), + SAMLLogoutAdapterTest::assertSamlLogoutResponse + ); + } + + @Test + public void testLogoutMandatoryDestinationUnsetRedirect() throws IOException { + testLogoutDestination(REDIRECT, + builder -> builder + .transformObject(logoutReq -> { logoutReq.setDestination(null); }) + .signWith(SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY, SAML_CLIENT_SALES_POST_SIG_PUBLIC_KEY), + SAMLLogoutAdapterTest::assertBadRequest + ); + } + + @Test + public void testLogoutMandatoryDestinationSetRedirect() throws IOException { + testLogoutDestination(REDIRECT, + builder -> builder.signWith(SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY, SAML_CLIENT_SALES_POST_SIG_PUBLIC_KEY), + SAMLLogoutAdapterTest::assertSamlLogoutResponse + ); + } + + @Test + public void testLogoutDestinationOptionalIfUnsignedPost() throws IOException { + testLogoutDestination(POST, + builder -> builder.transformObject(logoutReq -> { logoutReq.setDestination(null); }), + SAMLLogoutAdapterTest::assertSamlLogoutResponse + ); + } + + @Test + public void testLogoutMandatoryDestinationUnsetPost() throws IOException { + testLogoutDestination(POST, + builder -> builder + .transformObject(logoutReq -> { logoutReq.setDestination(null); }) + .signWith(SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY, SAML_CLIENT_SALES_POST_SIG_PUBLIC_KEY), + SAMLLogoutAdapterTest::assertBadRequest + ); + } + + @Test + public void testLogoutMandatoryDestinationSetPost() throws IOException { + testLogoutDestination(POST, + builder -> builder.signWith(SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY, SAML_CLIENT_SALES_POST_SIG_PUBLIC_KEY), + SAMLLogoutAdapterTest::assertSamlLogoutResponse + ); + } + + private void testLogoutDestination(Binding binding, final Consumer logoutReqUpdater, Consumer responseTester) throws IOException { + URI clientSamlEndpoint = salesPostServlet.getUriBuilder().clone().path("saml").build(); + + new SamlClientBuilder() + .navigateTo(salesPostServlet) + .processSamlResponse(Binding.POST) + .build() + .login().user(bburkeUser).build() + + .processSamlResponse(Binding.POST) + .targetAttributeSamlResponse() + .transformObject(this::extractNameId) + .build() + + .logoutRequest(clientSamlEndpoint, "http://no.one.cares/", binding) + .nameId(nameIdRef::get) + .sessionIndex(sessionIndexRef::get) + .apply(logoutReqUpdater) + .build() + + .doNotFollowRedirects() + .assertResponse(responseTester) + + .execute(); + } + + public static void assertSamlLogoutResponse(CloseableHttpResponse response) { + try { + assertThat(POST.extractResponse(response).getSamlObject(), Matchers.isSamlStatusResponse(JBossSAMLURIConstants.STATUS_SUCCESS)); + } catch (IOException ex) { + throw new RuntimeException(ex); + } + } + + public static void assertBadRequest(HttpResponse response) { + assertThat(response, anyOf( + Matchers.statusCodeIsHC(Status.BAD_REQUEST), + Matchers.statusCodeIsHC(Status.FORBIDDEN), + Matchers.bodyHC(anyOf( + containsString("Forbidden"), + containsString(FORBIDDEN_TEXT), + containsString(WEBSPHERE_FORBIDDEN_TEXT) + )) + )); + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/LogoutTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/LogoutTest.java index 8dbae0fd8b..8012950d99 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/LogoutTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/LogoutTest.java @@ -44,15 +44,23 @@ import org.keycloak.testsuite.updaters.ClientAttributeUpdater; import org.keycloak.testsuite.updaters.IdentityProviderCreator; import org.keycloak.testsuite.util.ClientBuilder; import org.keycloak.testsuite.util.IdentityProviderBuilder; +import org.keycloak.testsuite.util.Matchers; +import org.keycloak.testsuite.util.SamlClient.Binding; import org.keycloak.testsuite.util.SamlClientBuilder; +import org.keycloak.testsuite.util.saml.CreateLogoutRequestStepBuilder; import java.io.Closeable; import java.io.IOException; import java.util.Arrays; import java.util.List; import java.util.UUID; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; +import javax.ws.rs.core.Response.Status; +import javax.ws.rs.core.UriBuilderException; import javax.xml.transform.dom.DOMSource; +import org.apache.http.HttpResponse; +import org.apache.http.client.methods.CloseableHttpResponse; import org.junit.Before; import org.junit.Test; @@ -124,6 +132,26 @@ public class LogoutTest extends AbstractSamlTest { return null; } + private SamlClientBuilder logIntoUnsignedSalesAppViaIdp() throws IllegalArgumentException, UriBuilderException { + return new SamlClientBuilder() + .authnRequest(getAuthServerSamlEndpoint(REALM_NAME), SAML_CLIENT_ID_SALES_POST, SAML_ASSERTION_CONSUMER_URL_SALES_POST, POST).build() + + // Virtually perform login at IdP (return artificial SAML response) + .login().idp(SAML_BROKER_ALIAS).build() + .processSamlResponse(REDIRECT) + .transformObject(this::createAuthnResponse) + .targetAttributeSamlResponse() + .targetUri(getSamlBrokerUrl(REALM_NAME)) + .build() + .updateProfile().username("a").email("a@b.c").firstName("A").lastName("B").build() + .followOneRedirect() + + // Now returning back to the app + .processSamlResponse(POST) + .transformObject(this::extractNameIdAndSessionIndexAndTerminate) + .build(); + } + private SamlClientBuilder prepareLogIntoTwoApps() { return new SamlClientBuilder() .authnRequest(getAuthServerSamlEndpoint(REALM_NAME), SAML_CLIENT_ID_SALES_POST, SAML_ASSERTION_CONSUMER_URL_SALES_POST, POST).build() @@ -385,23 +413,7 @@ public class LogoutTest extends AbstractSamlTest { Closeable idp = new IdentityProviderCreator(realm, addIdentityProvider()) ) { - SAMLDocumentHolder samlResponse = new SamlClientBuilder() - .authnRequest(getAuthServerSamlEndpoint(REALM_NAME), SAML_CLIENT_ID_SALES_POST, SAML_ASSERTION_CONSUMER_URL_SALES_POST, POST).build() - - // Virtually perform login at IdP (return artificial SAML response) - .login().idp(SAML_BROKER_ALIAS).build() - .processSamlResponse(REDIRECT) - .transformObject(this::createAuthnResponse) - .targetAttributeSamlResponse() - .targetUri(getSamlBrokerUrl(REALM_NAME)) - .build() - .updateProfile().username("a").email("a@b.c").firstName("A").lastName("B").build() - .followOneRedirect() - - // Now returning back to the app - .processSamlResponse(POST) - .transformObject(this::extractNameIdAndSessionIndexAndTerminate) - .build() + SAMLDocumentHolder samlResponse = logIntoUnsignedSalesAppViaIdp() // ----- Logout phase ------ @@ -437,23 +449,7 @@ public class LogoutTest extends AbstractSamlTest { Closeable idp = new IdentityProviderCreator(realm, addIdentityProvider()) ) { - SAMLDocumentHolder samlResponse = new SamlClientBuilder() - .authnRequest(getAuthServerSamlEndpoint(REALM_NAME), SAML_CLIENT_ID_SALES_POST, SAML_ASSERTION_CONSUMER_URL_SALES_POST, POST).build() - - // Virtually perform login at IdP (return artificial SAML response) - .login().idp(SAML_BROKER_ALIAS).build() - .processSamlResponse(REDIRECT) - .transformObject(this::createAuthnResponse) - .targetAttributeSamlResponse() - .targetUri(getSamlBrokerUrl(REALM_NAME)) - .build() - .updateProfile().username("a").email("a@b.c").firstName("A").lastName("B").build() - .followOneRedirect() - - // Now returning back to the app - .processSamlResponse(POST) - .transformObject(this::extractNameIdAndSessionIndexAndTerminate) - .build() + SAMLDocumentHolder samlResponse = logIntoUnsignedSalesAppViaIdp() // ----- Logout phase ------ @@ -476,4 +472,97 @@ public class LogoutTest extends AbstractSamlTest { } } + @Test + public void testLogoutDestinationOptionalIfUnsignedRedirect() throws IOException { + testLogoutDestination(REDIRECT, + builder -> builder.transformObject(logoutReq -> { logoutReq.setDestination(null); }), + LogoutTest::assertSamlLogoutRequest + ); + } + + @Test + public void testLogoutMandatoryDestinationUnsetRedirect() throws IOException { + testLogoutDestination(REDIRECT, + builder -> builder + .transformObject(logoutReq -> { logoutReq.setDestination(null); }) + .signWith(SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY, SAML_CLIENT_SALES_POST_SIG_PUBLIC_KEY), + LogoutTest::assertBadRequest + ); + } + + @Test + public void testLogoutMandatoryDestinationSetRedirect() throws IOException { + testLogoutDestination(REDIRECT, + builder -> builder.signWith(SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY, SAML_CLIENT_SALES_POST_SIG_PUBLIC_KEY), + LogoutTest::assertSamlLogoutRequest + ); + } + + @Test + public void testLogoutDestinationOptionalIfUnsignedPost() throws IOException { + testLogoutDestination(POST, + builder -> builder.transformObject(logoutReq -> { logoutReq.setDestination(null); }), + LogoutTest::assertSamlLogoutRequest + ); + } + + @Test + public void testLogoutMandatoryDestinationUnsetPost() throws IOException { + testLogoutDestination(POST, + builder -> builder + .transformObject(logoutReq -> { logoutReq.setDestination(null); }) + .signWith(SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY, SAML_CLIENT_SALES_POST_SIG_PUBLIC_KEY), + LogoutTest::assertBadRequest + ); + } + + @Test + public void testLogoutMandatoryDestinationSetPost() throws IOException { + testLogoutDestination(POST, + builder -> builder.signWith(SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY, SAML_CLIENT_SALES_POST_SIG_PUBLIC_KEY), + LogoutTest::assertSamlLogoutRequest + ); + } + + private void testLogoutDestination(Binding binding, final Consumer logoutReqUpdater, Consumer responseTester) throws IOException { + final RealmResource realm = adminClient.realm(REALM_NAME); + + try ( + Closeable sales = ClientAttributeUpdater.forClient(adminClient, REALM_NAME, SAML_CLIENT_ID_SALES_POST) + .setFrontchannelLogout(true) + .removeAttribute(SamlProtocol.SAML_SINGLE_LOGOUT_SERVICE_URL_POST_ATTRIBUTE) + .setAttribute(SamlProtocol.SAML_SINGLE_LOGOUT_SERVICE_URL_REDIRECT_ATTRIBUTE, "http://url") + .update(); + + Closeable idp = new IdentityProviderCreator(realm, addIdentityProvider()) + ) { + logIntoUnsignedSalesAppViaIdp() + + // ----- Logout phase ------ + + // Logout initiated from the app + .logoutRequest(getAuthServerSamlEndpoint(REALM_NAME), SAML_CLIENT_ID_SALES_POST, binding) + .nameId(nameIdRef::get) + .sessionIndex(sessionIndexRef::get) + .apply(logoutReqUpdater) + .build() + + .doNotFollowRedirects() + .assertResponse(responseTester) + .execute(); + } + } + + public static void assertSamlLogoutRequest(CloseableHttpResponse response) { + try { + assertThat(REDIRECT.extractResponse(response).getSamlObject(), isSamlLogoutRequest(BROKER_LOGOUT_SERVICE_URL)); + } catch (IOException ex) { + throw new RuntimeException(ex); + } + } + + public static void assertBadRequest(HttpResponse response) { + assertThat(response, Matchers.statusCodeIsHC(Status.BAD_REQUEST)); + } + } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/SessionNotOnOrAfterTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/SessionNotOnOrAfterTest.java index b21f407755..b6b2da5491 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/SessionNotOnOrAfterTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/SessionNotOnOrAfterTest.java @@ -95,7 +95,7 @@ public class SessionNotOnOrAfterTest extends AbstractSamlTest { .idpInitiatedLogin(getAuthServerSamlEndpoint(REALM_NAME), "sales-post").build() .login().user(bburkeUser).build() .processSamlResponse(SamlClient.Binding.POST) - .transformObject(r -> checkSessionNotOnOrAfter(r, SSO_MAX_LIFESPAN, ACCESS_CODE_LIFESPAN, ACCESS_TOKEN_LIFESPAN)) + .transformObject(r -> { checkSessionNotOnOrAfter(r, SSO_MAX_LIFESPAN, ACCESS_CODE_LIFESPAN, ACCESS_TOKEN_LIFESPAN); }) .build() .execute(); } @@ -114,7 +114,7 @@ public class SessionNotOnOrAfterTest extends AbstractSamlTest { .idpInitiatedLogin(getAuthServerSamlEndpoint(REALM_NAME), "sales-post").build() .login().user(bburkeUser).build() .processSamlResponse(SamlClient.Binding.POST) - .transformObject(r -> checkSessionNotOnOrAfter(r, Integer.MAX_VALUE, Integer.MAX_VALUE, Integer.MAX_VALUE)) + .transformObject(r -> { checkSessionNotOnOrAfter(r, Integer.MAX_VALUE, Integer.MAX_VALUE, Integer.MAX_VALUE); }) .build() .execute(); } @@ -134,7 +134,7 @@ public class SessionNotOnOrAfterTest extends AbstractSamlTest { .build() .login().user(bburkeUser).build() .processSamlResponse(SamlClient.Binding.POST) - .transformObject(r -> checkSessionNotOnOrAfter(r, SSO_MAX_LIFESPAN, ACCESS_CODE_LIFESPAN, ACCESS_TOKEN_LIFESPAN)) + .transformObject(r -> { checkSessionNotOnOrAfter(r, SSO_MAX_LIFESPAN, ACCESS_CODE_LIFESPAN, ACCESS_TOKEN_LIFESPAN); }) .build() .execute(); } @@ -150,7 +150,7 @@ public class SessionNotOnOrAfterTest extends AbstractSamlTest { .idpInitiatedLogin(getAuthServerSamlEndpoint(REALM_NAME), "sales-post").build() .login().user(bburkeUser).build() .processSamlResponse(SamlClient.Binding.POST) - .transformObject(r -> checkSessionNotOnOrAfter(r, ssoMaxLifespan, 2000, 2000)) + .transformObject(r -> { checkSessionNotOnOrAfter(r, ssoMaxLifespan, 2000, 2000); }) .build() .execute(); } @@ -167,7 +167,7 @@ public class SessionNotOnOrAfterTest extends AbstractSamlTest { .build() .login().user(bburkeUser).build() .processSamlResponse(SamlClient.Binding.POST) - .transformObject(r -> checkSessionNotOnOrAfter(r, ssoMaxLifespan, 1800, 1800)) + .transformObject(r -> { checkSessionNotOnOrAfter(r, ssoMaxLifespan, 1800, 1800); }) .build() .execute(); }