SAML SLO does not clear session cookies on logout (#14017)

Author: DaanHoogland

plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/SAML2LogoutAPIAuthenticatorCmd.java

@@ -16,6 +16,7 @@
 // under the License.
 package org.apache.cloudstack.api.command;
 
+import com.cloud.api.ApiServlet;
 import com.cloud.api.response.ApiResponseSerializer;
 import com.cloud.user.Account;
 import org.apache.cloudstack.api.APICommand;
@@ -88,6 +89,8 @@ public String authenticate(String command, Map<String, Object[]> params, HttpSes
         response.setResponseName(getCommandName());
         String responseString = ApiResponseSerializer.toSerializedString(response, responseType);
 
+        ApiServlet.clearRequestCookies(req, resp);
+
         if (session == null) {
             try {
                 resp.sendRedirect(SAML2AuthManager.SAMLCloudStackRedirectionUrl.value());

plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/SAML2LogoutAPIAuthenticatorCmdTest.java

@@ -23,6 +23,7 @@
 import java.net.InetAddress;
 import java.security.cert.X509Certificate;
 
+import javax.servlet.http.Cookie;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpSession;
@@ -35,6 +36,7 @@
 import org.junit.Assert;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
@@ -81,6 +83,30 @@ public void testAuthenticate() throws Exception {
         Mockito.verify(session, Mockito.atLeastOnce()).getAttribute(Mockito.anyString());
     }
 
+    @Test
+    public void testAuthenticateClearsSessionCookiesBeforeRedirect() throws Exception {
+        SAML2LogoutAPIAuthenticatorCmd cmd = new SAML2LogoutAPIAuthenticatorCmd();
+
+        Field apiServerField = SAML2LogoutAPIAuthenticatorCmd.class.getDeclaredField("_apiServer");
+        apiServerField.setAccessible(true);
+        apiServerField.set(cmd, apiServer);
+
+        Field managerField = SAML2LogoutAPIAuthenticatorCmd.class.getDeclaredField("_samlAuthManager");
+        managerField.setAccessible(true);
+        managerField.set(cmd, samlAuthManager);
+
+        Mockito.when(session.getAttribute(Mockito.anyString())).thenReturn(null);
+        Cookie sessionKeyCookie = new Cookie("sessionkey", "someKey");
+        Mockito.when(req.getCookies()).thenReturn(new Cookie[]{sessionKeyCookie});
+
+        cmd.authenticate("command", null, session, InetAddress.getByName("127.0.0.1"), HttpUtils.RESPONSE_TYPE_JSON, new StringBuilder(), req, resp);
+
+        ArgumentCaptor<Cookie> cookieCaptor = ArgumentCaptor.forClass(Cookie.class);
+        Mockito.verify(resp, Mockito.times(1)).addCookie(cookieCaptor.capture());
+        Assert.assertEquals(0, cookieCaptor.getValue().getMaxAge());
+        Assert.assertEquals("", cookieCaptor.getValue().getValue());
+    }
+
     @Test
     public void testGetAPIType() throws Exception {
         Assert.assertTrue(new SAML2LogoutAPIAuthenticatorCmd().getAPIType() == APIAuthenticationType.LOGOUT_API);

server/src/main/java/com/cloud/api/ApiServlet.java

@@ -332,15 +332,7 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
                             apiServer.logoutUser(userId);
                         }
                         invalidateHttpSession(session, "invalidating session after logout call");
-
-                        final Cookie[] cookies = req.getCookies();
-                        if (cookies != null) {
-                            for (final Cookie cookie : cookies) {
-                                cookie.setValue("");
-                                cookie.setMaxAge(0);
-                                resp.addCookie(cookie);
-                            }
-                        }
+                        clearRequestCookies(req, resp);
                     }
                     HttpUtils.writeHttpResponse(resp, responseString, httpResponseCode, responseType, ApiServer.JSONContentType.value());
                     return;
@@ -632,6 +624,22 @@ private boolean invalidateHttpSessionIfNeeded(HttpServletRequest req, HttpServle
         return false;
     }
 
+    /**
+     * Echoes back every cookie on the request with Max-Age=0 so the browser drops them. Must be
+     * called before the response is committed (e.g. before HttpServletResponse#sendRedirect),
+     * otherwise the Set-Cookie headers are silently dropped by the servlet container.
+     */
+    public static void clearRequestCookies(HttpServletRequest req, HttpServletResponse resp) {
+        final Cookie[] cookies = req.getCookies();
+        if (cookies != null) {
+            for (final Cookie cookie : cookies) {
+                cookie.setValue("");
+                cookie.setMaxAge(0);
+                resp.addCookie(cookie);
+            }
+        }
+    }
+
     public static void invalidateHttpSession(HttpSession session, String msg) {
         try {
             if (LOGGER.isTraceEnabled()) {