You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2022/08/22 14:56:05 UTC

[tomcat] branch 10.0.x updated: Fix BZ 66120

This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch 10.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/10.0.x by this push:
     new c2493ab782 Fix BZ 66120
c2493ab782 is described below

commit c2493ab782f1e4d4190b2ea2a6931373a1f502be
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Aug 22 15:12:46 2022 +0100

    Fix BZ 66120
    
    Replicate the session note required by FORM authentication. This enables
    session failover / persistence+restoration to work correctly if it
    occurs during the FORM authentication process.
    
    The feature is disabled by default for backwards compatibility of the
    serialized session data.
---
 .../apache/catalina/ha/session/DeltaRequest.java   | 24 +++++++
 .../apache/catalina/ha/session/DeltaSession.java   | 76 +++++++++++++++++++++-
 java/org/apache/catalina/session/ManagerBase.java  | 39 +++++++++++
 .../apache/catalina/session/StandardSession.java   | 50 ++++++++++++--
 webapps/docs/changelog.xml                         |  9 +++
 webapps/docs/config/cluster-manager.xml            | 30 +++++++++
 webapps/docs/config/manager.xml                    | 30 +++++++++
 7 files changed, 250 insertions(+), 8 deletions(-)

diff --git a/java/org/apache/catalina/ha/session/DeltaRequest.java b/java/org/apache/catalina/ha/session/DeltaRequest.java
index 4a5c65792f..1d1dd23e44 100644
--- a/java/org/apache/catalina/ha/session/DeltaRequest.java
+++ b/java/org/apache/catalina/ha/session/DeltaRequest.java
@@ -53,6 +53,7 @@ public class DeltaRequest implements Externalizable {
     public static final int TYPE_MAXINTERVAL = 3;
     public static final int TYPE_AUTHTYPE = 4;
     public static final int TYPE_LISTENER = 5;
+    public static final int TYPE_NOTE = 6;
 
     public static final int ACTION_SET = 0;
     public static final int ACTION_REMOVE = 1;
@@ -90,6 +91,15 @@ public class DeltaRequest implements Externalizable {
         addAction(TYPE_ATTRIBUTE, ACTION_REMOVE, name, null);
     }
 
+    public void setNote(String name, Object value) {
+        int action = (value == null) ? ACTION_REMOVE : ACTION_SET;
+        addAction(TYPE_NOTE, action, name, value);
+    }
+
+    public void removeNote(String name) {
+        addAction(TYPE_NOTE, ACTION_REMOVE, name, null);
+    }
+
     public void setMaxInactiveInterval(int interval) {
         addAction(TYPE_MAXINTERVAL, ACTION_SET, NAME_MAXINTERVAL, Integer.valueOf(interval));
     }
@@ -216,6 +226,20 @@ public class DeltaRequest implements Externalizable {
                     } else {
                         session.removeSessionListener(listener, false);
                     }
+                    break;
+                case TYPE_NOTE:
+                    if (info.getAction() == ACTION_SET) {
+                        if (log.isTraceEnabled()) {
+                            log.trace("Session.setNote('" + info.getName() + "', '" + info.getValue() + "')");
+                        }
+                        session.setNote(info.getName(), info.getValue(), false);
+                    } else {
+                        if (log.isTraceEnabled()) {
+                            log.trace("Session.removeNote('" + info.getName() + "')");
+                        }
+                        session.removeNote(info.getName(), false);
+                    }
+
                     break;
                 default:
                     log.warn(sm.getString("deltaRequest.invalidAttributeInfoType", info));
diff --git a/java/org/apache/catalina/ha/session/DeltaSession.java b/java/org/apache/catalina/ha/session/DeltaSession.java
index 8e4e803f79..21d411936d 100644
--- a/java/org/apache/catalina/ha/session/DeltaSession.java
+++ b/java/org/apache/catalina/ha/session/DeltaSession.java
@@ -808,6 +808,52 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
     }
 
 
+    @Override
+    public void removeNote(String name) {
+        removeNote(name, true);
+    }
+
+    @SuppressWarnings("deprecation")
+    public void removeNote(String name, boolean addDeltaRequest) {
+        lockInternal();
+        try {
+            super.removeNote(name);
+            if (addDeltaRequest && manager instanceof ManagerBase &&
+                    ((ManagerBase) manager).getPersistAuthenticationNotes()) {
+                deltaRequest.removeNote(name);
+            }
+        } finally {
+            unlockInternal();
+        }
+    }
+
+
+    @Override
+    public void setNote(String name, Object value) {
+        setNote(name, value, true);
+    }
+
+    @SuppressWarnings("deprecation")
+    public void setNote(String name, Object value, boolean addDeltaRequest) {
+
+        if (value == null) {
+            removeNote(name, addDeltaRequest);
+            return;
+        }
+
+        lockInternal();
+        try {
+            super.setNote(name, value);
+            if (addDeltaRequest && manager instanceof ManagerBase &&
+                    ((ManagerBase) manager).getPersistAuthenticationNotes()) {
+                deltaRequest.setNote(name, value);
+            }
+        } finally {
+            unlockInternal();
+        }
+    }
+
+
     // -------------------------------------------- HttpSession Private Methods
 
     /**
@@ -852,11 +898,30 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
             log.debug(sm.getString("deltaSession.readSession", id));
         }
 
+        Object nextObject = stream.readObject();
+
+        // Compatibility with versions that do not persist the authentication
+        // notes
+        if (!(nextObject instanceof Integer)) {
+            // Not an Integer so the next two objects will be
+            // 'expected session ID' and 'saved request'
+            if (nextObject != null) {
+                notes.put(org.apache.catalina.authenticator.Constants.SESSION_ID_NOTE, nextObject);
+            }
+            nextObject = stream.readObject();
+            if (nextObject != null) {
+                notes.put(org.apache.catalina.authenticator.Constants.FORM_REQUEST_NOTE, nextObject);
+            }
+
+            // Next object will be the number of attributes
+            nextObject = stream.readObject();
+        }
+
         // Deserialize the attribute count and attribute values
         if (attributes == null) {
             attributes = new ConcurrentHashMap<>();
         }
-        int n = ( (Integer) stream.readObject()).intValue();
+        int n = ((Integer) nextObject).intValue();
         boolean isValidSave = isValid;
         isValid = true;
         for (int i = 0; i < n; i++) {
@@ -936,6 +1001,7 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
         doWriteObject((ObjectOutput)stream);
     }
 
+    @SuppressWarnings("deprecation")
     private void doWriteObject(ObjectOutput stream) throws IOException {
         // Write the scalar instance variables (except Manager)
         stream.writeObject(Long.valueOf(creationTime));
@@ -955,6 +1021,14 @@ public class DeltaSession extends StandardSession implements Externalizable,Clus
             log.debug(sm.getString("deltaSession.writeSession", id));
         }
 
+        if (manager instanceof ManagerBase && ((ManagerBase) manager).getPersistAuthenticationNotes()) {
+            // Write the notes associated with authentication. Without these,
+            // authentication can fail without sticky sessions or if there is a
+            // fail-over during authentication.
+            stream.writeObject(notes.get(org.apache.catalina.authenticator.Constants.SESSION_ID_NOTE));
+            stream.writeObject(notes.get(org.apache.catalina.authenticator.Constants.FORM_REQUEST_NOTE));
+        }
+
         // Accumulate the names of serializable and non-serializable attributes
         String keys[] = keys();
         List<String> saveNames = new ArrayList<>();
diff --git a/java/org/apache/catalina/session/ManagerBase.java b/java/org/apache/catalina/session/ManagerBase.java
index d142701d43..a6070a6c34 100644
--- a/java/org/apache/catalina/session/ManagerBase.java
+++ b/java/org/apache/catalina/session/ManagerBase.java
@@ -204,6 +204,12 @@ public abstract class ManagerBase extends LifecycleMBeanBase implements Manager
      */
     private boolean persistAuthentication = false;
 
+    /**
+     * Determines if the session notes required by the FORM authentication
+     * process are persisted (or replicated for clusters) by this Manager.
+     */
+    private boolean persistAuthenticationNotes = false;
+
     private boolean sessionActivityCheck = Globals.STRICT_SERVLET_COMPLIANCE;
 
     private boolean sessionLastAccessAtStart = Globals.STRICT_SERVLET_COMPLIANCE;
@@ -605,6 +611,39 @@ public abstract class ManagerBase extends LifecycleMBeanBase implements Manager
     }
 
 
+    /**
+     * Return whether sessions managed by this manager shall persist
+     * authentication notes used by FORM authentication or not.
+     *
+     * @return {@code true}, sessions managed by this manager shall persist
+     *         authentication notes used by FORM authentication; {@code false}
+     *         otherwise
+     *
+     * @deprecated Will be removed in Tomcat 10.1.x where it is effectively
+     *             hard-coded to <code>true</code>
+     */
+    @Deprecated
+    public boolean getPersistAuthenticationNotes() {
+        return this.persistAuthenticationNotes;
+    }
+
+    /**
+     * Set whether sessions managed by this manager shall persist authentication
+     * notes used by FORM authentication or not.
+     *
+     * @param persistAuthentication if {@code true}, sessions managed by this
+     *                              manager shall persist authentication notes
+     *                              used by FORM authentication
+     *
+     * @deprecated Will be removed in Tomcat 10.1.x where it is effectively
+     *             hard-coded to <code>true</code>
+     */
+    @Deprecated
+    public void setPersistAuthenticationNotes(boolean persistAuthenticationNotes) {
+        this.persistAuthenticationNotes = persistAuthenticationNotes;
+    }
+
+
     // --------------------------------------------------------- Public Methods
 
     /**
diff --git a/java/org/apache/catalina/session/StandardSession.java b/java/org/apache/catalina/session/StandardSession.java
index db32b3231a..439f71bb41 100644
--- a/java/org/apache/catalina/session/StandardSession.java
+++ b/java/org/apache/catalina/session/StandardSession.java
@@ -57,6 +57,7 @@ import org.apache.catalina.Session;
 import org.apache.catalina.SessionEvent;
 import org.apache.catalina.SessionListener;
 import org.apache.catalina.TomcatPrincipal;
+import org.apache.catalina.authenticator.SavedRequest;
 import org.apache.catalina.security.SecurityUtil;
 import org.apache.tomcat.util.ExceptionUtils;
 import org.apache.tomcat.util.res.StringManager;
@@ -1565,10 +1566,23 @@ public class StandardSession implements HttpSession, Session, Serializable {
                 ("readObject() loading session " + id);
         }
 
-        // The next object read could either be the number of attributes (Integer) or the session's
-        // authType followed by a Principal object (not an Integer)
+        if (notes == null) {
+            notes = new Hashtable<>();
+        }
+        /*
+         * The next object read could either be the number of attributes
+         * (Integer) or if authentication information is persisted then:
+         * - authType (String)   - always present
+         * - Principal object    - always present
+         * - expected session ID - present if persistAuthenticationNotes == true
+         * - saved request       - present if persistAuthenticationNotes == true
+         *
+         * Note: Some, all or none of the above objects may be null
+         */
         Object nextObject = stream.readObject();
         if (!(nextObject instanceof Integer)) {
+            // Not an Integer so the next two objects will be authType and
+            // Principal
             setAuthType((String) nextObject);
             try {
                 setPrincipal((Principal) stream.readObject());
@@ -1581,8 +1595,22 @@ public class StandardSession implements HttpSession, Session, Serializable {
                 }
                 throw e;
             }
-            // After that, the next object read should be the number of attributes (Integer)
+
             nextObject = stream.readObject();
+            if (!(nextObject instanceof Integer)) {
+                // Not an Integer so the next two objects will be
+                // 'expected session ID' and 'saved request'
+                if (nextObject != null) {
+                    notes.put(org.apache.catalina.authenticator.Constants.SESSION_ID_NOTE, nextObject);
+                }
+                nextObject = stream.readObject();
+                if (nextObject != null) {
+                    notes.put(org.apache.catalina.authenticator.Constants.FORM_REQUEST_NOTE, nextObject);
+                }
+
+                // Next object will be the number of attributes
+                nextObject = stream.readObject();
+            }
         }
 
         // Deserialize the attribute count and attribute values
@@ -1629,10 +1657,6 @@ public class StandardSession implements HttpSession, Session, Serializable {
         if (listeners == null) {
             listeners = new ArrayList<>();
         }
-
-        if (notes == null) {
-            notes = new Hashtable<>();
-        }
     }
 
 
@@ -1655,6 +1679,7 @@ public class StandardSession implements HttpSession, Session, Serializable {
      *
      * @exception IOException if an input/output error occurs
      */
+    @SuppressWarnings("deprecation")
     protected void doWriteObject(ObjectOutputStream stream) throws IOException {
 
         // Write the scalar instance variables (except Manager)
@@ -1673,6 +1698,8 @@ public class StandardSession implements HttpSession, Session, Serializable {
         // Gather authentication information (if configured)
         String sessionAuthType = null;
         Principal sessionPrincipal = null;
+        String expectedSessionId = null;
+        SavedRequest savedRequest = null;
         if (getPersistAuthentication()) {
             sessionAuthType = getAuthType();
             sessionPrincipal = getPrincipal();
@@ -1681,6 +1708,8 @@ public class StandardSession implements HttpSession, Session, Serializable {
                 manager.getContext().getLogger().warn(
                         sm.getString("standardSession.principalNotSerializable", id));
             }
+            expectedSessionId = (String) notes.get(org.apache.catalina.authenticator.Constants.SESSION_ID_NOTE);
+            savedRequest = (SavedRequest) notes.get(org.apache.catalina.authenticator.Constants.FORM_REQUEST_NOTE);
         }
 
         // Write authentication information (may be null values)
@@ -1691,6 +1720,13 @@ public class StandardSession implements HttpSession, Session, Serializable {
             manager.getContext().getLogger().warn(
                     sm.getString("standardSession.principalNotSerializable", id), e);
         }
+        if (manager instanceof ManagerBase && ((ManagerBase) manager).getPersistAuthenticationNotes()) {
+            // Write the notes associated with authentication. Without these,
+            // authentication can fail if there is a persist/restore during
+            // FORM authentication
+            stream.writeObject(expectedSessionId);
+            stream.writeObject(savedRequest);
+        }
 
         // Accumulate the names of serializable and non-serializable attributes
         String keys[] = keys();
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 183597493d..db270bcba7 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -137,6 +137,11 @@
         Improve handling of stack overflow errors when parsing SSI expressions.
         (markt)
       </fix>
+      <fix>
+        <bug>66120</bug>: Enable FORM authentication to work correctly if
+        session persistence and restoration occurs during the authentication
+        process. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Coyote">
@@ -195,6 +200,10 @@
         passed an unrecognised action type, a warning message will now be
         logged. (markt)
       </fix>
+      <fix>
+        <bug>66120</bug>: Enable FORM authentication to work correctly if
+        session failover occurs during the authentication process. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Other">
diff --git a/webapps/docs/config/cluster-manager.xml b/webapps/docs/config/cluster-manager.xml
index 7d742cbe5f..85740da5a0 100644
--- a/webapps/docs/config/cluster-manager.xml
+++ b/webapps/docs/config/cluster-manager.xml
@@ -141,6 +141,21 @@
         Set to <code>true</code> if you wish to have container listeners notified
         across Tomcat nodes in the cluster.
       </attribute>
+      <attribute name="persistAuthenticationNotes" required="false">
+        <p>Should authentication notes (used during FORM authentication) be
+        included when sessions are replicated? If <code>true</code>, the
+        session's authentication notes (used during FORM authentication) are
+        replicated so that an in progress FORM authentication can continue if
+        failover occurs during FORM authentication. If not specified, the
+        default value of <code>false</code> will be used.</p>
+
+        <p>Please note that all nodes must be upgraded to at least 10.0.24 (the
+        version where this feature was introduced) before this feature is
+        enabled else session replication may fail.</p>
+
+        <p>This attribute has been removed for Tomcat 10.1.x onwards which
+        always replicates FORM authenticaton notes.</p>
+      </attribute>
       <attribute name="stateTransferTimeout" required="false">
         The time in seconds to wait for a session state transfer to complete
         from another node when a node is starting up.
@@ -222,6 +237,21 @@
         sessions where the current node is the primary node for the session are
         considered active sessions.
       </attribute>
+      <attribute name="persistAuthenticationNotes" required="false">
+        <p>Should authentication notes (used during FORM authentication) be
+        included when sessions are replicated? If <code>true</code>, the
+        session's authentication notes (used during FORM authentication) are
+        replicated so that an in progress FORM authentication can continue if
+        failover occurs during FORM authentication. If not specified, the
+        default value of <code>false</code> will be used.</p>
+
+        <p>Please note that all nodes must be upgraded to at least 10.0.24 (the
+        version where this feature was introduced) before this feature is
+        enabled else session replication may fail.</p>
+
+        <p>This attribute has been removed for Tomcat 10.1.x onwards which
+        always replicates FORM authenticaton notes.</p>
+      </attribute>
       <attribute name="rpcTimeout" required="false">
         Timeout for RPC message used for broadcast and transfer state from
         another map.
diff --git a/webapps/docs/config/manager.xml b/webapps/docs/config/manager.xml
index 93489f8f9c..5747447e14 100644
--- a/webapps/docs/config/manager.xml
+++ b/webapps/docs/config/manager.xml
@@ -159,6 +159,17 @@
         filter pattern in order to be restored.</p>
       </attribute>
 
+      <attribute name="persistAuthenticationNotes" required="false">
+        <p>Should authentication notes (used during FORM authentication) be
+        included when session state is preserved across application restarts?
+        If <code>true</code>, the session's authentication notes are preserved
+        so that an in progress FORM authentication can continue after the
+        application has been restarted. If not specified, the default value of
+        <code>false</code> will be used.<br />See
+        <a href="#Persistence_Across_Restarts">Persistence Across Restarts</a>
+        for more information.</p>
+      </attribute>
+
       <attribute name="processExpiresFrequency" required="false">
         <p>Frequency of the session expiration, and related manager operations.
         Manager operations will be done once for the specified amount of
@@ -301,6 +312,25 @@
         filter pattern in order to be restored.</p>
       </attribute>
 
+      <attribute name="persistAuthenticationNotes" required="false">
+        <p>Should authentication notes (used during FORM authentication) be
+        included when sessions are swapped out to persistent storage? If
+        <code>true</code>, the session's authentication notes (used during
+        FORM authentication) are preserved so that an in progress FORM
+        authentication can continue after being reloaded (swapped in) from
+        persistent storage. If not specified, the default value of
+        <code>false</code> will be used.</p>
+
+        <p>Please note that if it is possible that the session will be swapped
+        in to a different node to the node from which it was swapped out, all
+        nodes must be upgraded to at least 10.0.24 (the version where this
+        feature was introduced) before this feature is enabled else the swapping
+        in may fail.</p>
+
+        <p>This attribute has been removed for Tomcat 10.1.x onwards which
+        always persists FORM authenticaton notes.</p>
+      </attribute>
+
       <attribute name="processExpiresFrequency" required="false">
         <p>It is the same as described above for the
         <code>org.apache.catalina.session.StandardManager</code> class.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org