This is an automated email from the ASF dual-hosted git repository. markt-asf pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 6fe265aded6705fb63478082e706ee46fac79f65 Author: Mark Thomas <[email protected]> AuthorDate: Wed Sep 16 08:08:41 2026 +0100 Fix various concurrency issues around adding and removing Realms This makes it much safer to add and/or remove nested realms at runtime. This is a further follow-on to "Add removeRealm, matching addRealm" --- java/org/apache/catalina/realm/CombinedRealm.java | 198 +++++++++++++++------ .../apache/catalina/realm/LocalStrings.properties | 3 + webapps/docs/changelog.xml | 9 + 3 files changed, 156 insertions(+), 54 deletions(-) diff --git a/java/org/apache/catalina/realm/CombinedRealm.java b/java/org/apache/catalina/realm/CombinedRealm.java index 78b7793018..19c1516fce 100644 --- a/java/org/apache/catalina/realm/CombinedRealm.java +++ b/java/org/apache/catalina/realm/CombinedRealm.java @@ -18,9 +18,10 @@ package org.apache.catalina.realm; import java.security.Principal; import java.security.cert.X509Certificate; -import java.util.ArrayList; -import java.util.Iterator; +import java.util.HashSet; import java.util.List; +import java.util.Set; +import java.util.concurrent.CopyOnWriteArrayList; import javax.management.ObjectName; @@ -40,71 +41,126 @@ import org.ietf.jgss.GSSName; /** * Realm implementation that contains one or more realms. Authentication is attempted for each realm in the order they - * were configured. If any realm authenticates the user then the authentication succeeds. When combining realms - * usernames should be unique across all combined realms. + * were configured. If any realm authenticates the user then the authentication succeeds. When combining realms user + * names should be unique across all combined realms. + * <p> + * For the typical usage (create realms, add realms to container, start container), each realm will be registered with + * JMX and {@link #getRealmPath()} will match the JMX object name. For simple changes (remove a realm, add a new realm) + * {@link #getRealmPath()} and the JMX object name will remain synchronized. For more unusual changes such as moving the + * realm to a new container, the JMX object names and {@link #getRealmPath()} are very likely to end up out of sync. If + * there are naming conflicts it is possible for realms that are still in use to not be registered in JMX. */ public class CombinedRealm extends RealmBase { + private static final Log log = LogFactory.getLog(CombinedRealm.class); + /** - * Default constructor for CombinedRealm. + * The list of Realms contained by this Realm. */ - public CombinedRealm() { - } + protected final List<Realm> realms = new CopyOnWriteArrayList<>(); + + private final Set<Realm> realmsToDestroy = new HashSet<>(); + + private int nextRealmIndex = 0; - private static final Log log = LogFactory.getLog(CombinedRealm.class); /** - * The list of Realms contained by this Realm. + * Default constructor for CombinedRealm. */ - protected final List<Realm> realms = new ArrayList<>(); + public CombinedRealm() { + } + /** * Add a realm to the list of realms that will be used to authenticate users. * - * @param theRealm realm which should be wrapped by the combined realm + * @param realm Realm which should be added to the combined realm */ - public void addRealm(Realm theRealm) { - realms.add(theRealm); + public synchronized void addRealm(Realm realm) { + + if (realms.contains(realm)) { + if (log.isDebugEnabled()) { + log.debug(sm.getString("combinedRealm.addRealmDuplicate", realm)); + } + return; + } + + boolean addRealm = true; - if (log.isDebugEnabled()) { - log.debug(sm.getString("combinedRealm.addRealm", theRealm.getClass().getName(), - Integer.toString(realms.size()))); + setSubRealmPath(realm); + + if (getState().isAvailable()) { + addRealm = startRealm(realm); + } + if (addRealm) { + nextRealmIndex++; + realms.add(realm); + // In case the Realm has been removed and then re-added + realmsToDestroy.remove(realm); + if (log.isDebugEnabled()) { + log.debug(sm.getString("combinedRealm.addRealm", realm.getClass().getName(), + Integer.toString(realms.size()))); + } + } else { + destroyRealm(realm); + } + } + + + private boolean startRealm(Realm realm) { + realm.setContainer(getContainer()); + if (realm instanceof Lifecycle) { + try { + ((Lifecycle) realm).start(); + } catch (LifecycleException e) { + log.error(sm.getString("combinedRealm.realmStartFail", realm.getClass().getName()), e); + return false; + } } + return true; } /** - * Remove a realm from the list of realms that are used to authenticate - * users. + * Remove a realm from the list of realms that are used to authenticate users. * - * @param theRealm realm which should no longer be wrapped by the combined - * realm + * @param realm Realm which should be removed from the combined realm * - * @return {@code true} if the realm was present and removed, - * {@code false} otherwise + * @return {@code true} if the realm was present and removed, {@code false} otherwise */ - public boolean removeRealm(Realm theRealm) { - boolean removed = realms.remove(theRealm); - - if (removed && log.isDebugEnabled()) { - log.debug(sm.getString("combinedRealm.removeRealm", theRealm.getClass().getName(), - Integer.toString(realms.size()))); + public synchronized boolean removeRealm(Realm realm) { + boolean removed = realms.remove(realm); + + if (removed) { + if (getState().isAvailable()) { + /* + * This realm could be being used in an authenticate() call. Delay destroying it until after the + * combined realm has stopped. + */ + realmsToDestroy.add(realm); + } else { + destroyRealm(realm); + } + if (log.isDebugEnabled()) { + log.debug(sm.getString("combinedRealm.removeRealm", realm.getClass().getName(), + Integer.toString(realms.size()))); + } } - return removed; } /** - * Returns the JMX ObjectNames of the realms that this realm is wrapping. - * Entries for realms that do not implement LifecycleMBeanBase will be null. + * Returns the JMX ObjectNames of the realms that this realm is wrapping. Entries for realms that do not implement + * LifecycleMBeanBase will be null. * * @return the array of realm ObjectNames, which may contain null entries */ public ObjectName[] getRealms() { - ObjectName[] result = new ObjectName[realms.size()]; + Realm[] realmsSnapshot = getNestedRealms(); + ObjectName[] result = new ObjectName[realmsSnapshot.length]; int i = 0; - for (Realm realm : realms) { + for (Realm realm : realmsSnapshot) { if (realm instanceof LifecycleMBeanBase) { result[i] = ((LifecycleMBeanBase) realm).getObjectName(); } @@ -206,35 +262,45 @@ public class CombinedRealm extends RealmBase { @Override public void setContainer(Container container) { - int i = 0; for (Realm realm : realms) { - // Set the realmPath for JMX naming - if (realm instanceof RealmBase) { - ((RealmBase) realm).setRealmPath(getRealmPath() + "/realm" + Integer.toString(i)); - } // Set the container for sub-realms. Mainly so logging works. realm.setContainer(container); - i++; } super.setContainer(container); } + /** + * {@inheritDoc} + * <p> + * Calling this method will also (re)set the paths for all of the nested realms. If a nested realm has been removed + * this will result in the remaining nested realms being re-numbered which may create an inconsistency between the + * nested realm's path and its JMX registration (if any). + */ + @Override + public synchronized void setRealmPath(String theRealmPath) { + super.setRealmPath(theRealmPath); + nextRealmIndex = 0; + for (Realm realm : realms) { + setSubRealmPath(realm); + nextRealmIndex++; + } + } + + + private void setSubRealmPath(Realm realm) { + if (realm instanceof RealmBase) { + ((RealmBase) realm).setRealmPath(getRealmPath() + "/realm" + Integer.toString(nextRealmIndex)); + } + } + + @Override protected void startInternal() throws LifecycleException { // Start 'sub-realms' then this one - Iterator<Realm> iter = realms.iterator(); - - while (iter.hasNext()) { - Realm realm = iter.next(); - if (realm instanceof Lifecycle) { - try { - ((Lifecycle) realm).start(); - } catch (LifecycleException e) { - // If realm doesn't start can't authenticate against it - iter.remove(); - log.error(sm.getString("combinedRealm.realmStartFail", realm.getClass().getName()), e); - } + for (Realm realm : realms) { + if (!startRealm(realm)) { + removeRealm(realm); } } @@ -253,7 +319,11 @@ public class CombinedRealm extends RealmBase { super.stopInternal(); for (Realm realm : realms) { if (realm instanceof Lifecycle) { - ((Lifecycle) realm).stop(); + try { + ((Lifecycle) realm).stop(); + } catch (LifecycleException e) { + log.error(sm.getString("combinedRealm.realmStopFail", realm.getClass().getName()), e); + } } } } @@ -265,11 +335,31 @@ public class CombinedRealm extends RealmBase { @Override protected void destroyInternal() throws LifecycleException { for (Realm realm : realms) { - if (realm instanceof Lifecycle) { + destroyRealm(realm); + } + super.destroyInternal(); + + for (Realm realm : realmsToDestroy) { + destroyRealm(realm); + } + } + + + private void destroyRealm(Realm realm) { + if (realm instanceof Lifecycle) { + if (((Lifecycle) realm).getState().isAvailable()) { + try { + ((Lifecycle) realm).stop(); + } catch (LifecycleException e) { + log.error(sm.getString("combinedRealm.realmStopFail", realm.getClass().getName()), e); + } + } + try { ((Lifecycle) realm).destroy(); + } catch (LifecycleException e) { + log.error(sm.getString("combinedRealm.realmDestroyFail", realm.getClass().getName()), e); } } - super.destroyInternal(); } diff --git a/java/org/apache/catalina/realm/LocalStrings.properties b/java/org/apache/catalina/realm/LocalStrings.properties index 6800265e3a..f84e69b447 100644 --- a/java/org/apache/catalina/realm/LocalStrings.properties +++ b/java/org/apache/catalina/realm/LocalStrings.properties @@ -14,12 +14,15 @@ # limitations under the License. combinedRealm.addRealm=Add [{0}] realm, making a total of [{1}] realms +combinedRealm.addRealmDuplicate=The [{0}] realm has already been added to this combined realm combinedRealm.authFail=Failed to authenticate user [{0}] with realm [{1}] combinedRealm.authStart=Attempting to authenticate user [{0}] with realm [{1}] combinedRealm.authSuccess=Authenticated user [{0}] with realm [{1}] combinedRealm.getPassword=The getPassword() method should never be called combinedRealm.getPrincipal=The getPrincipal() method should never be called +combinedRealm.realmDestroyFail=Failed to destroy [{0}] realm combinedRealm.realmStartFail=Failed to start [{0}] realm +combinedRealm.realmStopFail=Failed to stop [{0}] realm combinedRealm.removeRealm=Remove [{0}] realm, leaving a total of [{1}] realms combinedRealm.setCredentialHandler=A CredentialHandler was set on an instance of the CombinedRealm (or a sub-class of CombinedRealm). CombinedRealm doesn't use a configured CredentialHandler. Is this a configuration error? combinedRealm.unexpectedMethod=An unexpected call was made to a method on the combined realm diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index d567477601..a13ce985fa 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -246,6 +246,15 @@ class loader until after the web application class loader has started. (markt) </fix> + <fix> + Fix various concurrency issues around adding and removing + <code>Realm</code>s from a <code>CombinedRealm</code>, in particular + doing so while the <code>CombinedRealm</code> is in use. Note that once + a <code>Realm</code> instance has been removed from a + <code>CombinedRealm</code> instance at run-time, the removed + <code>Realm</code> instance may not be reused apart from re-adding it to + the same <code>CombinedRealm</code>. (markt) + </fix> </changelog> </subsection> <subsection name="Coyote"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
