From 83073aeae8546f5cff378e8691bdaecef89b2c9b Mon Sep 17 00:00:00 2001 From: Thomas Nymand Date: Thu, 13 Aug 2026 14:02:22 +0200 Subject: [PATCH] REF-28: Verify the signature on incoming LogoutRequests LogoutRequestService.validateLogoutRequest was an empty stub with no callers, so LogoutRequestHandler acted on LogoutRequests without authenticating them. The signature was logged, never verified, and the endpoint is reachable without a session. validateLogoutRequest now requires the request to be issued by the configured IdP and signed with its signing key from metadata, and handleGet and handleSOAP call it before any session state is read or changed. Both signature forms are accepted: on the message itself, as used for POST and SOAP, and on the query string, as used by the HTTP-Redirect binding and verified through OpenSAMLs SAML2HTTPRedirectDeflateSignatureSecurityHandler with a trust engine over the IdP metadata credential. A request carrying neither is rejected. Tests cover both signature forms, and the rejection of an unsigned request, one signed with an unknown key and one from another issuer, each asserting that no session is touched. Test support: the test IdP issues LogoutRequests as the IdP rather than as the SP and can sign them. Two SOAP tests serialized the signed message with the pretty printing StringUtil.elementToString, which broke its signature. --- .../saml/service/LogoutRequestService.java | 109 +++++++++++++++++- .../saml/servlet/LogoutRequestHandler.java | 6 + .../servlet/LogoutRequestHandlerTest.java | 103 ++++++++++++++++- .../java/dk/gov/oio/saml/util/IdpUtil.java | 83 ++++++++++++- 4 files changed, 294 insertions(+), 7 deletions(-) diff --git a/oiosaml/src/main/java/dk/gov/oio/saml/service/LogoutRequestService.java b/oiosaml/src/main/java/dk/gov/oio/saml/service/LogoutRequestService.java index 4634ab9..66d4d7f 100644 --- a/oiosaml/src/main/java/dk/gov/oio/saml/service/LogoutRequestService.java +++ b/oiosaml/src/main/java/dk/gov/oio/saml/service/LogoutRequestService.java @@ -1,27 +1,130 @@ package dk.gov.oio.saml.service; +import javax.servlet.http.HttpServletRequest; + import org.joda.time.DateTime; import org.opensaml.core.config.InitializationException; import org.opensaml.messaging.context.MessageContext; +import org.opensaml.messaging.handler.MessageHandlerException; import org.opensaml.saml.common.SAMLObject; import org.opensaml.saml.common.messaging.context.SAMLEndpointContext; import org.opensaml.saml.common.messaging.context.SAMLPeerEntityContext; +import org.opensaml.saml.common.messaging.context.SAMLProtocolContext; import org.opensaml.saml.common.xml.SAMLConstants; +import org.opensaml.saml.saml2.binding.security.impl.SAML2HTTPRedirectDeflateSignatureSecurityHandler; import org.opensaml.saml.saml2.core.Issuer; import org.opensaml.saml.saml2.core.LogoutRequest; import org.opensaml.saml.saml2.core.NameID; import org.opensaml.saml.saml2.core.SessionIndex; +import org.opensaml.saml.saml2.metadata.IDPSSODescriptor; import org.opensaml.saml.saml2.metadata.SingleSignOnService; +import org.opensaml.saml.security.impl.SAMLSignatureProfileValidator; +import org.opensaml.security.credential.UsageType; +import org.opensaml.security.credential.impl.StaticCredentialResolver; +import org.opensaml.security.x509.BasicX509Credential; import org.opensaml.xmlsec.SignatureSigningParameters; +import org.opensaml.xmlsec.SignatureValidationParameters; import org.opensaml.xmlsec.context.SecurityParametersContext; - +import org.opensaml.xmlsec.keyinfo.impl.StaticKeyInfoCredentialResolver; +import org.opensaml.xmlsec.signature.Signature; +import org.opensaml.xmlsec.signature.support.SignatureException; +import org.opensaml.xmlsec.signature.support.SignatureValidator; +import org.opensaml.xmlsec.signature.support.impl.ExplicitKeySignatureTrustEngine; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import dk.gov.oio.saml.util.ExternalException; import dk.gov.oio.saml.util.InternalException; import dk.gov.oio.saml.util.SamlHelper; +import dk.gov.oio.saml.util.StringUtil; +import net.shibboleth.utilities.java.support.component.ComponentInitializationException; import net.shibboleth.utilities.java.support.security.RandomIdentifierGenerationStrategy; public class LogoutRequestService { - public void validateLogoutRequest() { - return; + private static final Logger log = LoggerFactory.getLogger(LogoutRequestService.class); + + /** + * Verify that an incoming LogoutRequest was issued by the configured IdP and signed with its signing key. + * + *

Accepts a signature on the message itself (POST and SOAP) or on the query string (HTTP-Redirect + * binding), and rejects a request carrying neither. Must be called before any session is terminated.

+ */ + public static void validateLogoutRequest(HttpServletRequest httpServletRequest, MessageContext messageContext, LogoutRequest logoutRequest) throws ExternalException, InternalException { + validateIssuer(logoutRequest); + + if (logoutRequest.isSigned()) { + validateMessageSignature(logoutRequest); + } + else if (StringUtil.isNotEmpty(httpServletRequest.getParameter("Signature"))) { + validateQueryStringSignature(httpServletRequest, messageContext); + } + else { + throw new ExternalException("LogoutRequest was not signed"); + } + } + + private static void validateIssuer(LogoutRequest logoutRequest) throws ExternalException { + Issuer issuer = logoutRequest.getIssuer(); + String idpEntityID = OIOSAML3Service.getConfig().getIdpEntityID(); + + if (issuer == null || !idpEntityID.equals(issuer.getValue())) { + log.warn("LogoutRequest issuer '{}' does not match the configured IdP '{}'", (issuer != null) ? issuer.getValue() : null, idpEntityID); + throw new ExternalException("LogoutRequest was not issued by the configured IdP"); + } + } + + private static void validateMessageSignature(LogoutRequest logoutRequest) throws ExternalException, InternalException { + Signature signature = logoutRequest.getSignature(); + try { + // Establishes that the signature is bound to this message, see SAMLSignatureProfileValidator + new SAMLSignatureProfileValidator().validate(signature); + SignatureValidator.validate(signature, getIdPSigningCredential()); + } + catch (SignatureException e) { + throw new ExternalException("LogoutRequest signature could not be validated", e); + } + } + + private static void validateQueryStringSignature(HttpServletRequest httpServletRequest, MessageContext messageContext) throws ExternalException, InternalException { + SAMLPeerEntityContext peerEntityContext = messageContext.getSubcontext(SAMLPeerEntityContext.class, true); + peerEntityContext.setEntityId(OIOSAML3Service.getConfig().getIdpEntityID()); + peerEntityContext.setRole(IDPSSODescriptor.DEFAULT_ELEMENT_NAME); + + messageContext.getSubcontext(SAMLProtocolContext.class, true).setProtocol(SAMLConstants.SAML20P_NS); + + SignatureValidationParameters validationParameters = new SignatureValidationParameters(); + validationParameters.setSignatureTrustEngine(new ExplicitKeySignatureTrustEngine( + new StaticCredentialResolver(getIdPSigningCredential()), + new StaticKeyInfoCredentialResolver(getIdPSigningCredential()))); + messageContext.getSubcontext(SecurityParametersContext.class, true).setSignatureValidationParameters(validationParameters); + + SAML2HTTPRedirectDeflateSignatureSecurityHandler signatureHandler = new SAML2HTTPRedirectDeflateSignatureSecurityHandler(); + try { + signatureHandler.setHttpServletRequest(httpServletRequest); + signatureHandler.initialize(); + signatureHandler.invoke(messageContext); + } + catch (ComponentInitializationException e) { + throw new InternalException("Could not initialize SAML2HTTPRedirectDeflateSignatureSecurityHandler", e); + } + catch (MessageHandlerException e) { + throw new ExternalException("LogoutRequest signature could not be validated", e); + } + finally { + if (signatureHandler.isInitialized() && !signatureHandler.isDestroyed()) { + signatureHandler.destroy(); + } + } + + // The handler leaves the peer unauthenticated if it did not handle the message, for instance when the + // binding does not match, so a completed invoke is not on its own proof that the signature was checked + if (!peerEntityContext.isAuthenticated()) { + throw new ExternalException("LogoutRequest signature was not verified"); + } + } + + private static BasicX509Credential getIdPSigningCredential() throws ExternalException, InternalException { + return new BasicX509Credential(IdPMetadataService.getInstance().getIdPMetadata().getValidX509Certificate(UsageType.SIGNING)); } public static MessageContext createMessageWithLogoutRequest(String nameID, String nameIDFormat, String destination, String index) throws InitializationException, InternalException { diff --git a/oiosaml/src/main/java/dk/gov/oio/saml/servlet/LogoutRequestHandler.java b/oiosaml/src/main/java/dk/gov/oio/saml/servlet/LogoutRequestHandler.java index 1bc2bbb..a7ca2bb 100644 --- a/oiosaml/src/main/java/dk/gov/oio/saml/servlet/LogoutRequestHandler.java +++ b/oiosaml/src/main/java/dk/gov/oio/saml/servlet/LogoutRequestHandler.java @@ -41,6 +41,9 @@ public void handleGet(HttpServletRequest httpServletRequest, HttpServletResponse MessageContext context = decodeGet(httpServletRequest); LogoutRequest logoutRequest = getSamlObject(context, LogoutRequest.class); + // Nothing on the session may be touched before the request is known to come from the IdP + LogoutRequestService.validateLogoutRequest(httpServletRequest, context, logoutRequest); + MessageContext outgoingMessage = handleRequest(httpServletRequest, new LogoutRequestWrapper(logoutRequest)); try { sendPost(httpServletResponse, outgoingMessage); @@ -61,6 +64,9 @@ public void handleSOAP(HttpServletRequest httpServletRequest, HttpServletRespons MessageContext context = decodeSOAP(httpServletRequest); LogoutRequest logoutRequest = getSamlObject(context, LogoutRequest.class); + // Nothing on the session may be touched before the request is known to come from the IdP + LogoutRequestService.validateLogoutRequest(httpServletRequest, context, logoutRequest); + MessageContext outgoingMessage = handleRequest(httpServletRequest, new LogoutRequestWrapper(logoutRequest)); try { sendSOAP(httpServletResponse, outgoingMessage); diff --git a/oiosaml/src/test/java/dk/gov/oio/saml/servlet/LogoutRequestHandlerTest.java b/oiosaml/src/test/java/dk/gov/oio/saml/servlet/LogoutRequestHandlerTest.java index e52be4e..3399499 100644 --- a/oiosaml/src/test/java/dk/gov/oio/saml/servlet/LogoutRequestHandlerTest.java +++ b/oiosaml/src/test/java/dk/gov/oio/saml/servlet/LogoutRequestHandlerTest.java @@ -171,8 +171,9 @@ public void testIdPSOAPLogoutRequestWhenLoggedIn() throws Exception { // Marshall and serialize Element marshalledMessage = XMLObjectSupport.marshall(messageContext.getMessage()); + // Serialized as is: indenting or otherwise reformatting the message would break its signature final String soapXml = "" + - StringUtil.elementToString(marshalledMessage) + ""; + SerializeSupport.nodeToString(marshalledMessage).replaceFirst("^<\\?xml[^>]*\\?>", "") + ""; InputStream inputStream = new ByteArrayInputStream(soapXml.getBytes("UTF-8")); @@ -229,6 +230,103 @@ public int read() throws IOException { Assertions.assertEquals(TestConstants.IDP_LOGOUT_RESPONSE_URL, logoutResponse.getDestination()); } + @DisplayName("Test that an IdP can request a logout with a signature on the query string") + @Test + public void testIdPLogoutRequestSignedOnQueryString() throws Exception { + HttpSession session = Mockito.mock(HttpSession.class); + AssertionWrapper assertionWrapper = Mockito.mock(AssertionWrapper.class); + SessionHandler sessionHandler = OIOSAML3Service.getSessionHandlerFactory().getHandler(); + + // Create LogoutRequest without a signature on the message itself, the HTTP-Redirect binding signs + // the query string instead + String nameID = "https://data.gov.dk/model/core/eid/person/uuid/37a5a1aa-67ce-4f70-b7c0-b8e678d585f7"; + MessageContext messageContext = IdpUtil.createMessageWithLogoutRequest(nameID, NameID.PERSISTENT, TestConstants.SP_LOGOUT_REQUEST_URL, false, true); + String sessionIndex = ((LogoutRequest) messageContext.getMessage()).getSessionIndexes().get(0).getSessionIndex(); + String redirectUrl = IdpUtil.encodeAsRedirectUrl(messageContext); + + Mockito.when(sessionHandler.getAssertion(sessionIndex)).thenReturn(assertionWrapper); + Mockito.when(sessionHandler.isAuthenticated(session)).thenReturn(true); + Mockito.when(sessionHandler.getAuthnRequest(session)).thenReturn(null); + + // Mock HttpServletRequest + HttpServletRequest request = Mockito.mock(HttpServletRequest.class); + Mockito.when(request.getRequestURL()).thenReturn(new StringBuffer(TestConstants.SP_ASSERTION_CONSUMER_URL)); + Mockito.when(request.getSession()).thenReturn(session); + Mockito.when(request.getMethod()).thenReturn("GET"); + IdpUtil.stubRedirectRequest(request, redirectUrl); + + // Mock HttpServletResponse + ServletOutputStream outputStreamMock = Mockito.mock(ServletOutputStream.class); + HttpServletResponse response = Mockito.mock(HttpServletResponse.class); + Mockito.when(response.getOutputStream()).thenReturn(outputStreamMock); + + new LogoutRequestHandler().handleGet(request, response); + + Mockito.verify(sessionHandler).logout(session, assertionWrapper); + Mockito.verify(session).invalidate(); + } + + @DisplayName("Test that a LogoutRequest without a signature is rejected") + @Test + public void testRejectUnsignedLogoutRequest() throws Exception { + assertLogoutRequestRejected(IdpUtil.createMessageWithLogoutRequest( + "https://data.gov.dk/model/core/eid/person/uuid/37a5a1aa-67ce-4f70-b7c0-b8e678d585f7", + NameID.PERSISTENT, TestConstants.SP_LOGOUT_REQUEST_URL, false, true)); + } + + @DisplayName("Test that a LogoutRequest signed with an unknown key is rejected") + @Test + public void testRejectLogoutRequestSignedWithUnknownKey() throws Exception { + assertLogoutRequestRejected(IdpUtil.createMessageWithLogoutRequest( + "https://data.gov.dk/model/core/eid/person/uuid/37a5a1aa-67ce-4f70-b7c0-b8e678d585f7", + NameID.PERSISTENT, TestConstants.SP_LOGOUT_REQUEST_URL, true, false)); + } + + @DisplayName("Test that a LogoutRequest from another issuer is rejected") + @Test + public void testRejectLogoutRequestFromUnknownIssuer() throws Exception { + assertLogoutRequestRejected(IdpUtil.createMessageWithLogoutRequest( + "https://data.gov.dk/model/core/eid/person/uuid/37a5a1aa-67ce-4f70-b7c0-b8e678d585f7", + NameID.PERSISTENT, TestConstants.SP_LOGOUT_REQUEST_URL, true, true, "https://not-the-configured-idp")); + } + + /** + * Send the LogoutRequest to the handler and require that it is refused without any session being touched. + */ + private void assertLogoutRequestRejected(MessageContext messageContext) throws Exception { + HttpSession session = Mockito.mock(HttpSession.class); + AssertionWrapper assertionWrapper = Mockito.mock(AssertionWrapper.class); + SessionHandler sessionHandler = OIOSAML3Service.getSessionHandlerFactory().getHandler(); + + String sessionIndex = ((LogoutRequest) messageContext.getMessage()).getSessionIndexes().get(0).getSessionIndex(); + Mockito.when(sessionHandler.getAssertion(sessionIndex)).thenReturn(assertionWrapper); + Mockito.when(sessionHandler.isAuthenticated(session)).thenReturn(true); + Mockito.when(sessionHandler.getAuthnRequest(session)).thenReturn(null); + + // Marshall, deflate and base64 encode as the HTTP-Redirect binding does + Element marshalledMessage = XMLObjectSupport.marshall(messageContext.getMessage()); + ByteArrayOutputStream bytesOut = new ByteArrayOutputStream(); + DeflaterOutputStream deflaterStream = new DeflaterOutputStream(bytesOut, new Deflater(8, true)); + deflaterStream.write(SerializeSupport.nodeToString(marshalledMessage).getBytes("UTF-8")); + deflaterStream.finish(); + + // Mock HttpServletRequest + HttpServletRequest request = Mockito.mock(HttpServletRequest.class); + Mockito.when(request.getRequestURL()).thenReturn(new StringBuffer(TestConstants.SP_ASSERTION_CONSUMER_URL)); + Mockito.when(request.getSession()).thenReturn(session); + Mockito.when(request.getMethod()).thenReturn("GET"); + Mockito.when(request.getParameter("RelayState")).thenReturn(null); + Mockito.when(request.getParameter("SAMLRequest")).thenReturn(Base64Support.encode(bytesOut.toByteArray(), Base64Support.UNCHUNKED)); + + HttpServletResponse response = Mockito.mock(HttpServletResponse.class); + + Assertions.assertThrows(ExternalException.class, () -> new LogoutRequestHandler().handleGet(request, response)); + + // The session handler mock is shared between tests, so verify against this tests own session + Mockito.verify(sessionHandler, Mockito.never()).logout(Mockito.eq(session), Mockito.any(AssertionWrapper.class)); + Mockito.verify(session, Mockito.never()).invalidate(); + } + @DisplayName("Test that a user that is not logged in can safely attempt a logout") @Test public void testLogoutRequestWhenNotLoggedIn() throws InternalException, IOException, ExternalException { @@ -268,8 +366,9 @@ public void testSOAPLogoutRequestWhenNotLoggedIn() throws Exception { // Marshall and serialize Element marshalledMessage = XMLObjectSupport.marshall(messageContext.getMessage()); + // Serialized as is: indenting or otherwise reformatting the message would break its signature final String soapXml = "" + - StringUtil.elementToString(marshalledMessage) + ""; + SerializeSupport.nodeToString(marshalledMessage).replaceFirst("^<\\?xml[^>]*\\?>", "") + ""; InputStream inputStream = new ByteArrayInputStream(soapXml.getBytes("UTF-8")); diff --git a/oiosaml/src/test/java/dk/gov/oio/saml/util/IdpUtil.java b/oiosaml/src/test/java/dk/gov/oio/saml/util/IdpUtil.java index 427dcb7..c11454d 100644 --- a/oiosaml/src/test/java/dk/gov/oio/saml/util/IdpUtil.java +++ b/oiosaml/src/test/java/dk/gov/oio/saml/util/IdpUtil.java @@ -1,6 +1,10 @@ package dk.gov.oio.saml.util; import java.io.FileInputStream; +import java.net.URL; +import java.net.URLDecoder; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import java.security.KeyStore; import java.security.cert.CertificateFactory; import java.security.cert.X509Certificate; @@ -18,6 +22,10 @@ import org.opensaml.core.criterion.EntityIdCriterion; import org.opensaml.core.xml.XMLObjectBuilderFactory; import org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport; +import org.opensaml.core.xml.util.XMLObjectSupport; +import org.opensaml.saml.saml2.binding.encoding.impl.HTTPRedirectDeflateEncoder; +import org.mockito.ArgumentCaptor; +import org.mockito.Mockito; import org.opensaml.core.xml.schema.XSAny; import org.opensaml.core.xml.schema.impl.XSAnyBuilder; import org.opensaml.messaging.context.MessageContext; @@ -177,11 +185,28 @@ private static LogoutResponse createLogoutResponse(String destination, LogoutReq } public static MessageContext createMessageWithLogoutRequest(String nameID, String nameIDFormat, String destination) throws Exception { + return createMessageWithLogoutRequest(nameID, nameIDFormat, destination, true, true); + } + + /** + * Create a LogoutRequest as the IdP would send it. + * + * @param signMessage sign the LogoutRequest itself, as done for the POST and SOAP bindings + * @param validSignature sign with the IdP key from metadata, or with an unrelated key + */ + public static MessageContext createMessageWithLogoutRequest(String nameID, String nameIDFormat, String destination, boolean signMessage, boolean validSignature) throws Exception { + return createMessageWithLogoutRequest(nameID, nameIDFormat, destination, signMessage, validSignature, TestConstants.IDP_ENTITY_ID); + } + + public static MessageContext createMessageWithLogoutRequest(String nameID, String nameIDFormat, String destination, boolean signMessage, boolean validSignature, String issuerEntityID) throws Exception { // Create message context MessageContext messageContext = new MessageContext<>(); // Create AuthnRequest - LogoutRequest outgoingLogoutRequest = createLogoutRequest(nameID, nameIDFormat, destination); + LogoutRequest outgoingLogoutRequest = createLogoutRequest(nameID, nameIDFormat, destination, issuerEntityID); + if (signMessage) { + signLogoutRequest(outgoingLogoutRequest, validSignature); + } messageContext.setMessage(outgoingLogoutRequest); // Destination @@ -203,7 +228,45 @@ public static MessageContext createMessageWithLogoutRequest(String n return messageContext; } + /** + * Encode a message for the HTTP-Redirect binding, signing the query string when the message context + * carries signing parameters, and return the redirect URL the IdP would send the user agent to. + */ + public static String encodeAsRedirectUrl(MessageContext messageContext) throws Exception { + HttpServletResponse httpServletResponse = Mockito.mock(HttpServletResponse.class); + + HTTPRedirectDeflateEncoder encoder = new HTTPRedirectDeflateEncoder(); + encoder.setMessageContext(messageContext); + encoder.setHttpServletResponse(httpServletResponse); + encoder.initialize(); + encoder.encode(); + + ArgumentCaptor redirectUrl = ArgumentCaptor.forClass(String.class); + Mockito.verify(httpServletResponse).sendRedirect(redirectUrl.capture()); + + return redirectUrl.getValue(); + } + + /** + * Stub query string and parameters on a mocked request, as a container would present the redirect URL. + */ + public static void stubRedirectRequest(HttpServletRequest httpServletRequest, String redirectUrl) throws Exception { + String queryString = new URL(redirectUrl).getQuery(); + Mockito.when(httpServletRequest.getQueryString()).thenReturn(queryString); + + for (String parameter : queryString.split("&")) { + int separator = parameter.indexOf('='); + String name = parameter.substring(0, separator); + String value = URLDecoder.decode(parameter.substring(separator + 1), "UTF-8"); + Mockito.when(httpServletRequest.getParameter(name)).thenReturn(value); + } + } + public static LogoutRequest createLogoutRequest(String nameID, String nameIDFormat, String destination) throws InitializationException { + return createLogoutRequest(nameID, nameIDFormat, destination, OIOSAML3Service.getConfig().getSpEntityID()); + } + + public static LogoutRequest createLogoutRequest(String nameID, String nameIDFormat, String destination, String issuerEntityID) throws InitializationException { LogoutRequest outgoingLR = SamlHelper.build(LogoutRequest.class); // Set ID @@ -218,7 +281,7 @@ public static LogoutRequest createLogoutRequest(String nameID, String nameIDForm Issuer issuer = SamlHelper.build(Issuer.class); outgoingLR.setIssuer(issuer); - issuer.setValue(OIOSAML3Service.getConfig().getSpEntityID()); + issuer.setValue(issuerEntityID); // NameID NameID nameIDObj = SamlHelper.build(NameID.class); @@ -263,6 +326,22 @@ private static X509Certificate getSPCertificate(boolean validCert) throws Except return (X509Certificate) instance.generateCertificate(fis); } + public static void signLogoutRequest(LogoutRequest logoutRequest, boolean validSignature) throws Exception { + Signature signature = buildSAMLObject(Signature.class); + + BasicX509Credential x509Credential = getX509Credential(validSignature); + + signature.setSigningCredential(x509Credential); + signature.setCanonicalizationAlgorithm(CanonicalizationMethod.EXCLUSIVE); + signature.setSignatureAlgorithm(new SignatureRSASHA256().getURI()); + signature.setKeyInfo(getPublicKeyInfo(x509Credential)); + + logoutRequest.setSignature(signature); + + XMLObjectSupport.marshall(logoutRequest); + Signer.signObject(signature); + } + private static void SignAssertion(Assertion assertion, boolean validSignature) throws Exception { Signature signature = buildSAMLObject(Signature.class);