REF-30: Change the session id when a login is accepted

README.md

@@ -2,7 +2,7 @@
 
 OIOSAML.Java is a SAML 2.0 framework for Java, intended for use with NemLog-in3, but it can also be used as a generic SAML framework for Java.
 
-The framework supports the OIO SAML 3.0 profile and can generate `AuthnRequest`s and perform validation on SAML Assertions according to this profile. It works with **Java 11+**.
+The framework supports the OIO SAML 3.0 profile and can generate `AuthnRequest`s and perform validation on SAML Assertions according to this profile. It works with **Java 11+** and requires a **Servlet 3.1** or later container.
 
 This document covers how to use and configure the OIOSAML.Java 3.x framework.
 
@@ -323,10 +323,11 @@ Finally, there are three folders, each containing identical JSP files. These fil
 Compiling and running the demo application is performed with Maven like this:
 
 ```bash
-$ mvn clean install tomcat7:run-war
+$ mvn clean install
+$ mvn -pl demo jetty:run-war
 ```
 
-> Note that Tomcat 7 performs some validation on class files during startup, which has some issues with JAXB. This results in warnings that can safely be ignored. Tomcat 8 does not have these issues.
+> The demo runs on Jetty 9.4, which implements Servlet 3.1 as the library requires. It listens on HTTPS only, using the demo keystore in `misc/ssl-demo.pfx`; the connector is configured in `demo/src/main/jetty/jetty-https.xml`.
 
 Once the application is running, it can be accessed at <https://localhost:8443/oiosaml3-demo.java/>.
 

demo/pom.xml

@@ -13,16 +13,27 @@
 
     <build>
         <plugins>
+            <!-- Jetty 9.4 is a Servlet 3.1 container, which the library requires. Tomcat 7, used here
+                 before, only implements Servlet 3.0 -->
             <plugin>
-                <groupId>org.apache.tomcat.maven</groupId>
-                <artifactId>tomcat7-maven-plugin</artifactId>
-                <version>2.2</version>
+                <groupId>org.eclipse.jetty</groupId>
+                <artifactId>jetty-maven-plugin</artifactId>
+                <version>9.4.53.v20231009</version>
                 <configuration>
-                    <port>8080</port>
-                    <httpsPort>8443</httpsPort>
-                    <keystoreFile>${project.basedir}/../misc/ssl-demo.pfx</keystoreFile>
-                    <keystorePass>Test1234</keystorePass>
-                    <path>/oiosaml3-demo.java</path>
+                    <webApp>
+                        <contextPath>/oiosaml3-demo.java</contextPath>
+                    </webApp>
+                    <jettyXml>${project.basedir}/src/main/jetty/jetty-https.xml</jettyXml>
+                    <systemProperties>
+                        <systemProperty>
+                            <name>demo.keystore.path</name>
+                            <value>${project.basedir}/../misc/ssl-demo.pfx</value>
+                        </systemProperty>
+                        <systemProperty>
+                            <name>demo.keystore.password</name>
+                            <value>Test1234</value>
+                        </systemProperty>
+                    </systemProperties>
                 </configuration>
             </plugin>
         </plugins>
@@ -46,7 +57,7 @@
         <dependency>
             <groupId>javax.servlet</groupId>
             <artifactId>javax.servlet-api</artifactId>
-            <version>3.0.1</version>
+            <version>3.1.0</version>
             <scope>provided</scope>
         </dependency>
 

demo/src/main/jetty/jetty-https.xml

@@ -0,0 +1,52 @@
+<?xml version="1.0"?>
+<!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "https://www.eclipse.org/jetty/configure_9_3.dtd">
+
+<!--
+    HTTPS connector for the demo, run with 'mvn -pl demo jetty:run-war'. The demo SP is configured with
+    an https base URL, so the IdP redirects the user agent back to this connector. Keystore location and
+    password come from system properties set by the jetty-maven-plugin in demo/pom.xml.
+-->
+<Configure id="Server" class="org.eclipse.jetty.server.Server">
+
+    <New id="httpsConfiguration" class="org.eclipse.jetty.server.HttpConfiguration">
+        <Set name="secureScheme">https</Set>
+        <Set name="securePort">8443</Set>
+        <Call name="addCustomizer">
+            <Arg>
+                <New class="org.eclipse.jetty.server.SecureRequestCustomizer"/>
+            </Arg>
+        </Call>
+    </New>
+
+    <New id="sslContextFactory" class="org.eclipse.jetty.util.ssl.SslContextFactory$Server">
+        <Set name="keyStorePath"><SystemProperty name="demo.keystore.path"/></Set>
+        <Set name="keyStoreType">PKCS12</Set>
+        <Set name="keyStorePassword"><SystemProperty name="demo.keystore.password"/></Set>
+        <Set name="keyManagerPassword"><SystemProperty name="demo.keystore.password"/></Set>
+    </New>
+
+    <Call name="addConnector">
+        <Arg>
+            <New class="org.eclipse.jetty.server.ServerConnector">
+                <Arg name="server"><Ref refid="Server"/></Arg>
+                <Arg name="factories">
+                    <Array type="org.eclipse.jetty.server.ConnectionFactory">
+                        <Item>
+                            <New class="org.eclipse.jetty.server.SslConnectionFactory">
+                                <Arg name="next">http/1.1</Arg>
+                                <Arg name="sslContextFactory"><Ref refid="sslContextFactory"/></Arg>
+                            </New>
+                        </Item>
+                        <Item>
+                            <New class="org.eclipse.jetty.server.HttpConnectionFactory">
+                                <Arg name="config"><Ref refid="httpsConfiguration"/></Arg>
+                            </New>
+                        </Item>
+                    </Array>
+                </Arg>
+                <Set name="port">8443</Set>
+            </New>
+        </Arg>
+    </Call>
+
+</Configure>

oiosaml/pom.xml

@@ -150,7 +150,7 @@
         <dependency>
             <groupId>javax.servlet</groupId>
             <artifactId>javax.servlet-api</artifactId>
-            <version>3.0.1</version>
+            <version>3.1.0</version>
             <scope>provided</scope>
         </dependency>
 

oiosaml/src/main/java/dk/gov/oio/saml/servlet/AssertionHandler.java

@@ -142,6 +142,9 @@ public void handlePost(HttpServletRequest httpServletRequest, HttpServletRespons
 
         AssertionWrapper assertionWrapper = new AssertionWrapper(assertion);
 
+        // The session is authenticated from here, so it must not keep the id it had before login
+        rotateSessionId(httpServletRequest, sessionHandler, authnRequest);
+
         sessionHandler.storeAssertion(session, assertionWrapper);
 
         OIOSAML3Service.getAuditService().auditLog(AuditRequestUtil
@@ -159,4 +162,17 @@ public void handlePost(HttpServletRequest httpServletRequest, HttpServletRespons
 
         httpServletResponse.sendRedirect(url);
     }
+
+    /**
+     * Give the session a new id now that it carries an authenticated user, so an id planted in the browser
+     * before login cannot be used afterwards.
+     *
+     * <p>Session state is keyed on the container session id, so the in-flight AuthnRequest is stored again
+     * under the new id.</p>
+     */
+    private void rotateSessionId(HttpServletRequest httpServletRequest, SessionHandler sessionHandler, AuthnRequestWrapper authnRequest) throws InternalException {
+        httpServletRequest.changeSessionId();
+
+        sessionHandler.storeAuthnRequest(httpServletRequest.getSession(), authnRequest);
+    }
 }

oiosaml/src/test/java/dk/gov/oio/saml/servlet/AssertionHandlerTest.java

@@ -20,6 +20,7 @@
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.mockito.ArgumentCaptor;
+import org.mockito.InOrder;
 import org.mockito.Mockito;
 import org.opensaml.core.xml.util.XMLObjectSupport;
 import org.opensaml.messaging.context.MessageContext;
@@ -85,6 +86,47 @@ public void testPostWithValidAssertion() throws Exception {
         Assertions.assertEquals(assertionWrapperArgumentCaptor.getValue().getNsisLevel(), NSISLevel.SUBSTANTIAL);
     }
 
+    @DisplayName("Test that handler changes the session id when the assertion is accepted")
+    @Test
+    public void testPostWithValidAssertionChangesSessionId() throws Exception {
+        // Create MessageContext, Response and Assertion
+        String nameID = "https://data.gov.dk/model/core/eid/person/uuid/37a5a1aa-67ce-4f70-b7c0-b8e678d585f7";
+        String inResponseToId = UUID.randomUUID().toString();
+        MessageContext<SAMLObject> messageContext = IdpUtil.createMessageWithAssertion(true, true, true, nameID, TestConstants.SP_ENTITY_ID, TestConstants.SP_ASSERTION_CONSUMER_URL, inResponseToId);
+
+        Element marshalledMessage = XMLObjectSupport.marshall(messageContext.getMessage());
+        String base64EncodedMessage = Base64Support.encode(SerializeSupport.nodeToString(marshalledMessage).getBytes("UTF-8"), Base64Support.UNCHUNKED);
+
+        // Create AuthnRequest
+        AuthnRequestService authnRequestService = new AuthnRequestService();
+        MessageContext<SAMLObject> authnRequestMessageContext = authnRequestService.createMessageWithAuthnRequest(false, false, NSISLevel.SUBSTANTIAL, null, null);
+        AuthnRequest authnRequest = (AuthnRequest) authnRequestMessageContext.getMessage();
+        authnRequest.setID(inResponseToId);
+        AuthnRequestWrapper wrapper = new AuthnRequestWrapper(authnRequest, NSISLevel.SUBSTANTIAL, "");
+
+        SessionHandler sessionHandler = OIOSAML3Service.getSessionHandlerFactory().getHandler();
+        Mockito.when(sessionHandler.getAuthnRequest(session)).thenReturn(wrapper);
+
+        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("POST");
+        Mockito.when(request.getParameter("RelayState")).thenReturn(null);
+        Mockito.when(request.getParameter("SAMLResponse")).thenReturn(base64EncodedMessage);
+
+        HttpServletResponse response = Mockito.mock(HttpServletResponse.class);
+
+        new AssertionHandler().handlePost(request, response);
+
+        Mockito.verify(request).changeSessionId();
+
+        // Session state is keyed on the container session id, so the AuthnRequest follows the session
+        InOrder inOrder = Mockito.inOrder(request, sessionHandler);
+        inOrder.verify(request).changeSessionId();
+        inOrder.verify(sessionHandler).storeAuthnRequest(session, wrapper);
+        inOrder.verify(sessionHandler).storeAssertion(Mockito.eq(session), Mockito.any(AssertionWrapper.class));
+    }
+
     @DisplayName("Test that handler will reject am invalid assertion")
     @Test
     public void testPostWithInvalidAssertion() throws Exception {

oiosaml/src/test/java/dk/gov/oio/saml/servlet/LogoutRequestHandlerTest.java

@@ -7,6 +7,7 @@
 import java.util.zip.Deflater;
 import java.util.zip.DeflaterOutputStream;
 
+import javax.servlet.ReadListener;
 import javax.servlet.ServletInputStream;
 import javax.servlet.ServletOutputStream;
 import javax.servlet.http.HttpServletRequest;
@@ -196,6 +197,21 @@ public void testIdPSOAPLogoutRequestWhenLoggedIn() throws Exception {
             public int read() throws IOException {
                 return inputStream.read();
             }
+
+            @Override
+            public boolean isFinished() {
+                return false;
+            }
+
+            @Override
+            public boolean isReady() {
+                return true;
+            }
+
+            @Override
+            public void setReadListener(ReadListener readListener) {
+                //Do nothing
+            }
         });
 
         // Mock DummyOutputStream
@@ -390,6 +406,21 @@ public void testSOAPLogoutRequestWhenNotLoggedIn() throws Exception {
             public int read() throws IOException {
                 return inputStream.read();
             }
+
+            @Override
+            public boolean isFinished() {
+                return false;
+            }
+
+            @Override
+            public boolean isReady() {
+                return true;
+            }
+
+            @Override
+            public void setReadListener(ReadListener readListener) {
+                //Do nothing
+            }
         });
 
         // Mock DummyOutputStream

oiosaml/src/test/java/dk/gov/oio/saml/servlet/LogoutResponseHandlerTest.java

@@ -7,6 +7,7 @@
 import java.util.zip.Deflater;
 import java.util.zip.DeflaterOutputStream;
 import javax.servlet.ServletOutputStream;
+import javax.servlet.WriteListener;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpSession;
@@ -110,5 +111,15 @@ private class DummyOutputStream extends ServletOutputStream {
         public void write(int i) throws IOException {
             //Do nothing
         }
+
+        @Override
+        public boolean isReady() {
+            return true;
+        }
+
+        @Override
+        public void setWriteListener(WriteListener writeListener) {
+            //Do nothing
+        }
     }
 }