KEYCLOAK-12874 Align Destination field existence check with spec

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() + "'");
+            log.error("Expected destination '" + requestUri + "' got '" + requestAbstractType.getDestination() + "'");
-            return AuthOutcome.FAILED;
+            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() {
+            return failed(CHALLENGE_EXTRACTION_FAILURE);
-                @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;
         }
         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() {
+                        return failed(CHALLENGE_INVALID_SIGNATURE);
-                            @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 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() {
+                    return failed(createAuthChallenge403(statusResponse));
-                        @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;
                 } 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() {
+            return failed(createAuthChallenge403(responseType));
-                @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;
         }
         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() {
+            return failed(CHALLENGE_EXTRACTION_FAILURE);
-                @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;
         }
 
         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");
-
+                    return failed(CHALLENGE_INVALID_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;
                 }
             } catch (Exception e) {
                 log.error("Error processing validation of SAML assertion: " + e.getMessage());
-                challenge = new AuthChallenge() {
+                return failed(CHALLENGE_EXTRACTION_FAILURE);
-
-                    @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;
             }
         }
 
@@ -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));
+    }
+
 }

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));
+        if (destination != null) {
             lort.setDestination(URI.create(destination));
+        }
 
         if (! this.extensions.isEmpty()) {
             ExtensionsType extensionsType = new ExtensionsType();

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 {

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));
     }

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 ? "<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 {
+                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);
+                throw new IllegalStateException("Invalid SAML object");
-                event.error(Errors.INVALID_TOKEN);
-                return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST);
             }
         }
 
         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()) {
+            if (! validateDestination(requestAbstractType, samlClient, Errors.INVALID_SAML_AUTHN_REQUEST)) {
-                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);
                 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 (! validateDestination(logoutRequest, samlClient, Errors.INVALID_SAML_LOGOUT_REQUEST)) {
-            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);
                 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<String, String> 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);

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<HttpResponse> matcher) {
+    public SamlClientBuilder assertResponse(Matcher<? super CloseableHttpResponse> 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<HttpResponse> consumer) {
+    public SamlClientBuilder assertResponse(Consumer<? super CloseableHttpResponse> consumer) {
         steps.add((client, currentURI, currentResponse, context) -> {
             consumer.accept(currentResponse);
             return null;

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

@@ -42,6 +42,8 @@ public class CreateLogoutRequestStepBuilder extends SamlDocumentStepBuilder<Logo
     private Supplier<String> sessionIndex = () -> null;
     private Supplier<NameIDType> nameId = () -> null;
     private Supplier<String> 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<Logo
         return this;
     }
 
+    public CreateLogoutRequestStepBuilder 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 {
         SAML2LogoutRequestBuilder builder = new SAML2LogoutRequestBuilder()
-          .destination(authServerSamlUrl.toString())
+          .destination(authServerSamlUrl == null ? null : authServerSamlUrl.toString())
           .issuer(issuer)
           .sessionIndex(sessionIndex())
           .nameId(nameId());
@@ -107,7 +115,9 @@ public class CreateLogoutRequestStepBuilder extends SamlDocumentStepBuilder<Logo
             return null;
         }
 
-        return requestBinding.createSamlUnsignedRequest(authServerSamlUrl, relayState(), DocumentUtil.getDocument(transformed));
+        return this.signingPrivateKeyPem == null
+          ? requestBinding.createSamlUnsignedRequest(authServerSamlUrl, relayState(), DocumentUtil.getDocument(transformed))
+          : requestBinding.createSamlSignedRequest(authServerSamlUrl, relayState(), DocumentUtil.getDocument(transformed), signingPrivateKeyPem, signingPublicKeyPem);
     }
 
 }

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

@@ -35,6 +35,7 @@ import org.keycloak.saml.processing.core.saml.v2.writers.SAMLResponseWriter;
 import org.keycloak.testsuite.util.SamlClient.Step;
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
+import java.util.function.Consumer;
 import javax.xml.stream.XMLStreamWriter;
 import org.jboss.logging.Logger;
 import org.junit.Assert;
@@ -71,6 +72,13 @@ public abstract class SamlDocumentStepBuilder<T extends SAML2Object, This extend
         this.clientBuilder = clientBuilder;
     }
 
+    public This transformObject(Consumer<T> tr) {
+        return transformObject(so -> {
+            tr.accept(so);
+            return so;
+        });
+    }
+
     @SuppressWarnings("unchecked")
     public This transformObject(Saml2ObjectTransformer<T> tr) {
         final StringTransformer original = this.transformer;
@@ -117,6 +125,13 @@ public abstract class SamlDocumentStepBuilder<T extends SAML2Object, This extend
         return (This) this;
     }
 
+    public This transformDocument(Consumer<Document> 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<T extends SAML2Object, This extend
         return (This) this;
     }
 
+    public This transformString(Consumer<String> 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<T extends SAML2Object, This extend
         return (This) this;
     }
 
+    @SuppressWarnings("unchecked")
+    public This apply(Consumer<This> updaterOfThis) {
+        updaterOfThis.accept((This) this);
+        return (This) this;
+    }
+
     public SamlClientBuilder build() {
         return this.clientBuilder;
     }

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<String, String> entry : cr.getAttributes().entrySet()) {
                 cr.getAttributes().put(entry.getKey(), replaceHttpValuesWithHttps(entry.getValue()));
             }

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<String, String> 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();

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<NameIDType> nameIdRef = new AtomicReference<>();
+    private final AtomicReference<String> sessionIndexRef = new AtomicReference<>();
 
     @Override
     public void addAdapterTestRealms(List<RealmRepresentation> 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<CreateLogoutRequestStepBuilder> logoutReqUpdater, Consumer<? super CloseableHttpResponse> 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)
+          ))
+        ));
+    }
 }

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()
+            SAMLDocumentHolder samlResponse = logIntoUnsignedSalesAppViaIdp()
-              .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()
 
               // ----- Logout phase ------
 
@@ -437,23 +449,7 @@ public class LogoutTest extends AbstractSamlTest {
 
           Closeable idp = new IdentityProviderCreator(realm, addIdentityProvider())
           ) {
-            SAMLDocumentHolder samlResponse = new SamlClientBuilder()
+            SAMLDocumentHolder samlResponse = logIntoUnsignedSalesAppViaIdp()
-              .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()
 
               // ----- 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<CreateLogoutRequestStepBuilder> logoutReqUpdater, Consumer<? super CloseableHttpResponse> 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));
+    }
+
 }

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();
         }