This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit a1f099f7950e6c7574ce1ca3dc60122101db744e Author: opencode <[email protected]> AuthorDate: Wed Nov 4 16:00:34 2026 +0100 Put the pool MBean's masking, counters and promises in line A review of the pool's JMX surface (the ConnectionPoolMBean interface and its implementation) closed two credential findings and corrected every promise the interface makes that the code does not keep. JdbcInterceptorsAsArray, the parsed form of the interceptor string, undid the masking the sibling attributes had just been given: it returned the configuration's live cached InterceptorDefinition array, whose per-property values carry the credential in cleartext, Serializable so a connector carries it to a remote reader, and the array itself writable, so an in-JVM reader held the definitions the pools build their interceptor chains from. The wrapper's getter now returns a masked, defensive copy produced by a new overload of PoolProperties' maskInterceptorCredentials - beside the string form so both share one key rule and one mask constant - carrying class names rather than cached Class objects, because reading a configuration for a display attribute must not load classes. PoolProperties' own accessor is untouched: chain construction legitimately needs the live parsed configuration. DbProperties publishes a read that withholds the password and a write that replaces the whole set, so the ordinary read-modify-write hands the pool a set with the credential taken out - silently for a pool whose password travels only among the database properties, which then establishes its next connections with nothing to authenticate. That asymmetry is the project's deliberate masking policy on a trusted administration surface and stands, but it is now impossible to miss: both halves say what they do, name the Password and Username attributes as the credential's owning controls, and two tests pin exactly which shape breaks and which survives. The interface javadoc was then checked promise by promise against the delegated code. The seven counters no longer claim "since pool creation" across a resetStats; Created/Released count pooled objects including attempts that never established a session; ReleasedIdle counts any idle pass, not just the cleaner; WaitCount is scoped to the synchronous wait loop; the size/idle/active trio is documented as slot-counting, per-read samples that need not add up, with the Num aliases named. testIdle and checkAbandoned no longer promise what testWhileIdle and removeAbandoned can refuse. NOTIFY_CONNECT is documented as the driver-path promise it is, POOL_EMPTY as best effort over a bounded queue, the query constants by the interceptor flags that gate them. The class javadoc records the inherited boxed- Boolean trap: the isDefaultAutoCommit/isDefaultReadOnly pair publishes as operations, and primitive-ising either would give the attributes two getters and make the MBean unregisterable. On the implementation setName finally documents that renaming a running pool moves only the display name, every registration staying keyed by the name recorded at start; the wrapped-pool field and SlowQueryReport's PoolStatistics constructor got the comments the javadoc build asks for, taking mvn clean javadoc:javadoc to zero warnings project-wide. Tests: TestJmxCredentialAttributes gained the parsed-attribute case, TestJmxCredentialWrites the two round-trip cases. No behavior changed beyond the masked attribute; the suite is green. --- .../apache/tomcat/jdbc/pool/PoolProperties.java | 52 +++++++ .../jdbc/pool/interceptor/SlowQueryReport.java | 9 ++ .../tomcat/jdbc/pool/jmx/ConnectionPool.java | 62 ++++++++- .../tomcat/jdbc/pool/jmx/ConnectionPoolMBean.java | 133 ++++++++++++++---- .../jdbc/pool/jmx/TestJmxCredentialAttributes.java | 115 ++++++++++++++- .../jdbc/pool/jmx/TestJmxCredentialWrites.java | 154 ++++++++++++++++++++- 6 files changed, 491 insertions(+), 34 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java index 5c961fdd9d..9258ab5f79 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java @@ -1263,6 +1263,58 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl } + /** + * Masks the credential values carried by an array of parsed interceptor + * definitions, the display counterpart of + * {@link #maskInterceptorCredentials(String)} for what + * {@link #getJdbcInterceptorsAsArray()} has already parsed out of that + * string. Every definition of the array is replaced by a new definition of + * the same class name, and every property whose name is one of the keys + * {@link #isCredentialKey recognises} by a property holding the mask; the + * other property names and values, and the order of the array, are copied as + * they stand, so the copy still shows which interceptors are configured and + * which properties they carry. The copy is also defensive: it shares no + * definition, no property and no array slot with the array it was made from, + * so a reader handed the copy cannot reach through it to the definitions the + * pool builds its interceptor chains from. Like the masking of the string + * this rules out the names this class treats as credentials, not any secret + * an interceptor may hold under another name. + * <p> + * The copy carries the interceptor's class <em>name</em> rather than the + * class object a definition may have cached: a display copy is never meant + * to instantiate an interceptor, and loading a class is not something a + * read of the configuration should do. + * </p> + * + * @param definitions the parsed definitions, may be {@code null} + * + * @return a masked copy of the array, or {@code null} when the array is + * {@code null} + */ + public static InterceptorDefinition[] maskInterceptorCredentials(InterceptorDefinition[] definitions) { + if (definitions == null) { + return null; + } + InterceptorDefinition[] result = new InterceptorDefinition[definitions.length]; + for (int i = 0; i < definitions.length; i++) { + InterceptorDefinition definition = definitions[i]; + if (definition == null) { + // An array this class did not build may hold a hole; a hole is + // copied as a hole rather than turned into a failure on the + // read of a configuration. + continue; + } + InterceptorDefinition copy = new InterceptorDefinition(definition.getClassName()); + for (InterceptorProperty property : definition.getProperties().values()) { + copy.addProperty(new InterceptorProperty(property.getName(), + isCredentialKey(property.getName()) ? PASSWORD_MASK : property.getValue())); + } + result[i] = copy; + } + return result; + } + + /** * Whether {@code c} ends a key and starts the pair's value, covering the * separators a {@link java.util.Properties} reader accepts; white space diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java index ec95878305..351e798c5a 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java @@ -740,6 +740,15 @@ public class SlowQueryReport extends AbstractQueryReport { private final Set<ConnectionPool> livePools = Collections .synchronizedSet(Collections.newSetFromMap(new WeakHashMap<ConnectionPool, Boolean>())); + /** + * Creates the registration of one name: an empty statistics map and no live pools until the first pool + * {@link #addLivePool registers itself} under it. + */ + protected PoolStatistics() { + // The two collections are initialised at their declarations; a + // registration of a name starts with nothing else. + } + /** * Registers a pool that started under this name. * diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java index 6084782d5f..3e11f13a5d 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java @@ -48,6 +48,11 @@ public class ConnectionPool extends NotificationBroadcasterSupport implements Co private static final Log log = LogFactory.getLog(ConnectionPool.class); + /** + * The pool this MBean wraps, assigned once by the constructor and never swapped: attribute reads consult the + * live configuration the pool shares with its DataSource, the operations run against this pool, and it is the + * pool the reported conditions describe. {@link #getPool()} hands out the same reference in-process. + */ protected org.apache.tomcat.jdbc.pool.ConnectionPool pool = null; /** * sequence for JMX notifications @@ -471,6 +476,18 @@ public class ConnectionPool extends NotificationBroadcasterSupport implements Co * plaintext password while {@link #getPassword()} is explicitly masked. A configuration whose database properties * are {@code null} - a state {@link PoolConfiguration#setDbProperties(Properties)} accepts - has nothing to mask, * so this returns {@code null}. {@inheritDoc} + * <p> + * The copy is a display value, and it looks complete: nothing in it says that the pool's own set carries a pair + * this one withholds. That matters because this attribute is also writable and its write is the whole set - + * {@link #setDbProperties(Properties)} replaces the live collection - so the ordinary read-modify-write of the + * attribute hands the pool back a set with the credential taken out and takes that credential away from the live + * configuration along with it. A configuration whose password travels only among the database properties - the + * shape the credential has when a {@link PoolConfiguration#getConnectionProperties() connectionProperties} line + * carries it - then establishes its next connections with nothing to authenticate them, and nothing on the read, + * the write or in between warns. The credential's owning controls are the {@link #setPassword(String) Password} + * and {@link #setUsername(String) Username} attributes: each mirrors its value into the database properties and + * takes the entry away when cleared, and while one of them holds the password it, not the entry, is what new + * connections are established with however this attribute is written. */ @Override public Properties getDbProperties() { @@ -687,9 +704,21 @@ public class ConnectionPool extends NotificationBroadcasterSupport implements Co return getPoolProperties().getDefaultReadOnly(); } + /** + * Returns the parsed interceptor configuration with any credential its + * definitions carry masked out, so that a read of this attribute does not + * hand back in cleartext the password {@link #getPassword()} exists to + * withhold - the pairs of a definition reach a third-party interceptor as + * configured, which is why {@link #getJdbcInterceptors()} masks them in the + * string this attribute is the parsed form of. There is no write to mask: + * {@link #setJdbcInterceptors(String)} refuses the change on a running pool + * at all. The array is a copy made for this read, so the configuration the + * interceptor chains are built from keeps the raw pairs and this attribute is + * no way to reach those definitions either. {@inheritDoc} + */ @Override public InterceptorDefinition[] getJdbcInterceptorsAsArray() { - return getPoolProperties().getJdbcInterceptorsAsArray(); + return PoolProperties.maskInterceptorCredentials(getPoolProperties().getJdbcInterceptorsAsArray()); } @Override @@ -722,6 +751,21 @@ public class ConnectionPool extends NotificationBroadcasterSupport implements Co getPoolProperties().setAccessToUnderlyingConnectionAllowed(accessToUnderlyingConnectionAllowed); } + /** + * Replaces the whole set of database properties the pool connects with: the write is a new set rather than a + * change applied over the set the pool holds, and the caller owns whatever the written collection holds, as + * {@link PoolProperties#setDbProperties(Properties)} records. It re-mirrors nothing - the {@code user} and + * {@code password} entries are whatever the set written here carries. The read of this same attribute, + * {@link #getDbProperties()}, withholds the password, so writing back what that read returned - the ordinary way + * of adding or removing one entry - drops the credential from the live configuration: a pool whose password + * travels only among the database properties establishes its next connections with nothing to authenticate them, + * while a pool whose credential the {@link #setPassword(String) Password} attribute holds keeps authenticating + * with the attribute, the superseded entry going with the replaced set. The Password attribute and its + * {@link #setUsername(String) Username} pair are therefore the credential's owning controls: they mirror their + * value into the database properties and take the entry away when cleared, whereas this attribute can only be + * written with a set the writer itself already holds the credential for - what the read hands back is never that + * set. + */ @Override public void setDbProperties(Properties dbProperties) { getPoolProperties().setDbProperties(dbProperties); @@ -738,6 +782,22 @@ public class ConnectionPool extends NotificationBroadcasterSupport implements Co poolConfigurationUpdated(); } + /** + * Renames the running pool. The write is allowed at any time and changes only what the pool calls itself: the + * {@code Name} and {@code PoolName} attributes of this MBean, the name + * {@link org.apache.tomcat.jdbc.pool.ConnectionPool#getName()} reports for the pool, and - while this MBean is not + * registered, so it has no {@link ObjectName} to name itself by - the string its notifications carry as their + * source (see {@link #getNotificationSource()}). + * <p> + * Nothing that was registered under a name moves with the rename. Every registration made for a pool is keyed by + * the name the pool started with, which {@link org.apache.tomcat.jdbc.pool.ConnectionPool#getRegistrationName()} + * returns, and not by this property: the statistics the {@code SlowQueryReport} interceptor keeps for this pool + * stay reachable under the old name and are released under it when the pool closes, as does the MBean its + * {@code SlowQueryReportJmx} subclass publishes for the pool. The ObjectName this MBean is registered under is, in + * the same way, the one its registration took, and the per-connection MBeans stay published below that name. + * + * @param name the name the pool is to call itself by from now on + */ @Override public void setName(String name) { getPoolProperties().setName(name); diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPoolMBean.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPoolMBean.java index a9db1923bd..1a192f3fa7 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPoolMBean.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPoolMBean.java @@ -19,130 +19,194 @@ import org.apache.tomcat.jdbc.pool.PoolConfiguration; /** * JMX MBean interface for monitoring and managing a JDBC connection pool. + * <p> + * Inheriting {@link PoolConfiguration} brings this surface the boxed-{@link Boolean} accessors + * {@code isDefaultAutoCommit()} and {@code isDefaultReadOnly()}, and the JDK's MBean analyzer treats an {@code is...} + * method as a getter only when it returns the primitive {@code boolean}: those two are therefore published as + * operations alongside the {@code DefaultAutoCommit} and {@code DefaultReadOnly} attributes their {@code get...} pair + * provides. Making either one a primitive {@code boolean} getter would hand that attribute two getters, and the MBean + * could no longer be registered. */ public interface ConnectionPoolMBean extends PoolConfiguration { /** - * Returns the total number of connections in the pool. + * Returns the number of connection slots the pool holds right now - the idle ones and the checked out ones + * together. + * <p> + * This counts slots, not database sessions, and each of these counts reads the live state for itself. While + * the pool is establishing connections a slot is claimed before its session is signed on, and releasing a + * connection to a waiting borrower leaves a wake-up placeholder in the idle queue, so this count - and the idle + * count - can include an entry with no session behind it. Two of these attributes read at the same moment can + * still disagree: a {@code Size} against {@code Idle} plus {@code Active}, or an {@code Idle} against a + * {@code NumIdle}, is a comparison of samples taken one after the other, not of one state read once. * - * @return total connection count + * @return the number of connections the pool holds a slot for */ int getSize(); /** - * Returns the number of idle connections in the pool. + * Returns the number of idle connections in the pool, meaning the entries that sit in the idle queue - among which + * there can be a wake-up placeholder with no session behind it. Like the other counts of this interface this is one + * sample of live state, so {@link #getSize()} and {@link #getActive()} are read separately and need not add up to + * it. * * @return idle connection count */ int getIdle(); /** - * Returns the number of active (in-use) connections. + * Returns the number of active (in-use) connections: the entries of the pool's busy set, which is what the pool + * counts as checked out rather than idle. Like the other counts of this interface this is one sample of live state, + * so it need not add up against {@link #getSize()} and {@link #getIdle()}, which are read separately. * * @return active connection count */ int getActive(); /** - * Returns the number of idle connections in the pool. + * Returns the number of idle connections in the pool. This is another name for {@link #getIdle()} - the + * commons-dbcp naming - and reads the same count. * * @return idle connection count */ int getNumIdle(); /** - * Returns the number of active (in-use) connections. + * Returns the number of active (in-use) connections. This is another name for {@link #getActive()} - the + * commons-dbcp naming - and reads the same count. * * @return active connection count */ int getNumActive(); /** - * Returns the number of threads waiting for a connection. + * Returns the number of threads waiting for a connection. This is the count of borrowers parked in the wait loop of + * the synchronous {@code getConnection}: a caller of {@code getConnectionAsync} is counted only while that call is + * still running, and once it has been handed a future and waits on the queue behind it, that caller is no longer + * counted here - the pool's own wake-up logic has to ask the queue about those waiters separately, because this + * counter does not see them. * * @return wait count */ int getWaitCount(); /** - * Returns the total number of connections borrowed since pool creation. + * Returns the total number of connections borrowed since the pool was created or since the last call to + * {@link #resetStats()}, whichever is more recent. * * @return borrowed count */ long getBorrowedCount(); /** - * Returns the total number of connections returned since pool creation. + * Returns the total number of connections returned since the pool was created or since the last call to + * {@link #resetStats()}, whichever is more recent. * * @return returned count */ long getReturnedCount(); /** - * Returns the total number of connections created since pool creation. + * Returns the total number of connections created since the pool was created or since the last call to + * {@link #resetStats()}, whichever is more recent. This counts pooled objects the pool took into service, not + * sessions that came up: an attempt to establish a connection that the database refused is counted here, and its + * teardown is counted by {@link #getReleasedCount()}. * * @return created count */ long getCreatedCount(); /** - * Returns the total number of connections released since pool creation. + * Returns the total number of connections released since the pool was created or since the last call to + * {@link #resetStats()}, whichever is more recent. This is the counterpart of {@link #getCreatedCount()}: it counts + * each pooled object once as it goes out of service, whether or not it ever established a session. * * @return released count */ long getReleasedCount(); /** - * Returns the total number of reconnections since pool creation. + * Returns the total number of reconnections since the pool was created or since the last call to + * {@link #resetStats()}, whichever is more recent. A reconnection is one session the pool replaced under a pooled + * object it already had: the borrower came with different credentials, the session had passed its + * {@code maxAge}, or it could not be handed out as it stood. * * @return reconnected count */ long getReconnectedCount(); /** - * Returns the total number of connections removed due to abandonment. + * Returns the total number of connections removed due to abandonment since the pool was created or since the last + * call to {@link #resetStats()}, whichever is more recent. Only the connections the pool actually took away are + * counted here; a connection that was merely marked as suspect is not counted. * * @return abandoned count */ long getRemoveAbandonedCount(); /** - * Returns the total number of idle connections released by the pool cleaner. + * Returns the total number of idle connections released since the pool was created or since the last call to + * {@link #resetStats()}, whichever is more recent. Any pass that takes an idle connection out of the pool counts + * here - evicting one the configuration had kept idle too long, taking back one a purge had made stale, or + * dropping one that did not survive the idle validation or reconnection - and that pass may be the pool cleaner + * working on its own timer or an explicit {@link #checkIdle()}, {@link #testIdle()} or {@link #purge()} call. * * @return released idle count */ long getReleasedIdleCount(); /** - * Checks and cleans up idle connections in the pool. + * Runs the idle-eviction pass over the connections that are currently idle. An idle connection the configuration + * says should be evicted - one that has been idle past {@code minEvictableIdleTimeMillis}, or one a + * {@link #purgeOnReturn()} or {@link #purge()} call made stale - is released, and the pass stops at the + * {@code minIdle} floor. An idle connection a borrower claims while this runs is left to that borrower. This is + * the pass the pool cleaner runs on its own timer, run on demand, and what it releases is counted by + * {@link #getReleasedIdleCount()}. */ void checkIdle(); /** - * Checks for and removes abandoned connections from the pool. + * Looks through the connections that are currently checked out for ones that have been held too long. A + * connection past the remove-abandoned timeout is taken from its borrower and removed only while + * {@code removeAbandoned} is enabled and {@code abandonWhenPercentageFull} lets the pool's occupancy allow an + * abandon; each such removal is counted by {@link #getRemoveAbandonedCount()} and reported through + * {@link #NOTIFY_ABANDON}. A checked-out connection that is not removed - because removal is not allowed, or + * because it has not passed that timeout - keeps its borrower: one held longer than {@code suspectTimeout} is only + * marked as suspect and reported through {@link #SUSPECT_ABANDONED_NOTIFICATION}. With {@code suspectTimeout} unset + * and removal off, this operation reports nothing at all. */ void checkAbandoned(); /** - * Validates all idle connections in the pool. + * Runs the pass over the idle connections that validates them and takes out of the pool the ones that do not + * survive it. The validation only runs when {@code testWhileIdle} is set; with that flag unset nothing is put to + * the database. Whatever the flag, a connection whose {@code maxAge} has expired is reconnected first, and a + * connection is released when that reconnection fails, when the validation runs and fails it, or when it reaches + * the validation with no session behind it at all. Those releases are counted by + * {@link #getReleasedIdleCount()}, and an idle connection a borrower claims while this runs is left to that + * borrower. */ void testIdle(); /** * Purges all connections in the pool. For connections currently in use, these connections will be purged when - * returned on the pool. This call also purges connections that are idle and in the pool To only purge used/active - * connections see {@link #purgeOnReturn()} + * returned on the pool. This call also purges connections that are idle and in the pool. To only purge used/active + * connections see {@link #purgeOnReturn()}. */ void purge(); /** * Purges connections when they are returned from the pool. This call does not purge idle connections until they are - * used. To purge idle connections see {@link #purge()} + * used. To purge idle connections see {@link #purge()}. */ void purgeOnReturn(); /** - * reset the statistics of this pool. + * Resets the statistics of this pool: the seven counters of this interface - {@link #getBorrowedCount()}, + * {@link #getReturnedCount()}, {@link #getCreatedCount()}, {@link #getReleasedCount()}, + * {@link #getReconnectedCount()}, {@link #getRemoveAbandonedCount()} and {@link #getReleasedIdleCount()} - are + * brought back to zero. Everything else about the pool - its connections, its configuration, its notifications - + * is left as it is. */ void resetStats(); @@ -154,8 +218,11 @@ public interface ConnectionPoolMBean extends PoolConfiguration { String NOTIFY_INIT = "INIT FAILED"; /** - * Notification type sent when the pool cannot establish a physical connection to the database. The notification - * message carries the stack trace of the connection failure. + * Notification type sent when the pool cannot establish a physical connection to the database through the driver + * it instantiated itself, that being the only route on which the pool meets the failure. A pool that takes its + * sessions from a {@code DataSource} - one set through {@code dataSource} or resolved from {@code dataSourceJNDI} + * - asks that data source for a session instead, and never sends this notification. The notification message + * carries the stack trace of the connection failure. */ String NOTIFY_CONNECT = "CONNECTION FAILED"; @@ -168,15 +235,20 @@ public interface ConnectionPoolMBean extends PoolConfiguration { /** * Notification type sent when the {@code SlowQueryReportJmx} interceptor reports a query that took longer than its - * configured threshold. This pool MBean sends it only when the interceptor is configured with - * {@code notifyPool=true}; otherwise the interceptor's own MBean sends the notification. + * configured threshold. Nothing reaches a listener that the interceptor does not report, so its + * {@code logSlow} flag - on by default - switches this notification off as well, and a negative + * {@code threshold} keeps every execution out of the slow report. This pool MBean sends it only when the + * interceptor is configured with {@code notifyPool=true}; otherwise the interceptor's own MBean sends the + * notification. */ String SLOW_QUERY_NOTIFICATION = "SLOW QUERY"; /** - * Notification type sent when the {@code SlowQueryReportJmx} interceptor reports a query that failed. This pool - * MBean sends it only when the interceptor is configured with {@code notifyPool=true}; otherwise the interceptor's - * own MBean sends the notification. + * Notification type sent when the {@code SlowQueryReportJmx} interceptor reports a query that failed - a failed + * execution is reported whatever the threshold is. Nothing reaches a listener that the interceptor does not + * report, so its {@code logFailed} flag, which is off by default, switches this notification off with it. This + * pool MBean sends it only when the interceptor is configured with {@code notifyPool=true}; otherwise the + * interceptor's own MBean sends the notification. */ String FAILED_QUERY_NOTIFICATION = "FAILED QUERY"; @@ -191,7 +263,10 @@ public interface ConnectionPoolMBean extends PoolConfiguration { /** * Notification type sent when a borrow from the pool fails because the pool is empty: the borrower was not willing * to wait for a connection or its wait timed out. It is sent once per failed borrow, so a pool that stays exhausted - * sends one per failed borrow attempt. + * reports one per failed borrow attempt. Delivery is best effort: the notification is accepted for delivery or + * dropped whole, and the queue it is handed to has a fixed capacity, so a pool that reports these faster than its + * listeners drain them loses notifications - logging each loss - rather than holding them up or growing without + * limit. */ String POOL_EMPTY = "POOL EMPTY"; diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxCredentialAttributes.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxCredentialAttributes.java index dc7e1295a1..d75b177c5d 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxCredentialAttributes.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxCredentialAttributes.java @@ -20,6 +20,7 @@ import java.lang.management.ManagementFactory; import java.sql.Connection; import java.sql.ResultSet; import java.sql.Statement; +import java.util.Arrays; import java.util.HashSet; import java.util.Set; @@ -35,6 +36,7 @@ import org.junit.jupiter.api.Test; import org.apache.tomcat.jdbc.JdbcPoolBaseTest; import org.apache.tomcat.jdbc.pool.DataSource; import org.apache.tomcat.jdbc.pool.PoolProperties.InterceptorDefinition; +import org.apache.tomcat.jdbc.pool.PoolProperties.InterceptorProperty; /** * The three raw configuration strings of the pool - the JDBC URL, the @@ -64,7 +66,11 @@ import org.apache.tomcat.jdbc.pool.PoolProperties.InterceptorDefinition; * a configuration written scrubbed is a pool that cannot connect, while the read * gives back the pool's own best-effort masking, so the JMX read, the * configuration dump and the log all say the same thing about a configuration - * that carries its credential inline. + * that carries its credential inline. The parsed form of those strings is + * published beside them - the interceptor definitions as an array of objects + * rather than as the one line they were parsed from - and is pinned to the same + * rule, since an attribute that names the same credential in its structured form + * is as short a path back to it as the string was. * </p> */ class TestJmxCredentialAttributes extends JdbcPoolBaseTest { @@ -278,6 +284,74 @@ class TestJmxCredentialAttributes extends JdbcPoolBaseTest { } } + @Test + void theJdbcInterceptorsAsArrayAttributeOfThePoolMBeanMasksTheCredentialItsDefinitionsCarry() + throws Exception { + DataSource ds = newDataSource(); + // The same live definition the string attribute above is pinned on, with + // a second pair added: the credential and the ordinary property are + // neighbours of one parameter list, so the pair is what tells the two + // apart below - a masking that hid the whole definition would pass a test + // that only looked for the secret. + ds.setJdbcInterceptors("ResetAbandonedTimer(password=" + INTERCEPTOR_SECRET + ",fast=yes)"); + ds.getPoolProperties().setJmxEnabled(true); + ds.getPoolProperties().setInitialSize(0); + ds.createPool(); + InterceptorDefinition[] live = ds.getPoolProperties().getJdbcInterceptorsAsArray(); + Assertions.assertEquals(INTERCEPTOR_SECRET, cleartextIn(live, INTERCEPTOR_SECRET), + "precondition: the pool's own parsed definitions must still carry the credential - the attribute" + + " under test has one to hide precisely because the parse keeps what the operator typed"); + + Registration r = publish(ds, "interceptors-array-attribute"); + try { + InterceptorDefinition[] published = + (InterceptorDefinition[]) server().getAttribute(r.poolOname, "JdbcInterceptorsAsArray"); + + Assertions.assertNull(cleartextIn(published, INTERCEPTOR_SECRET), + "a password typed into an interceptor definition must not come back out of the pool's MBean in" + + " cleartext through the parsed form of that very definition either: the array is the" + + " same credential in a shape a monitor can walk programmatically rather than one it" + + " has to read a line of text for, and it is published to the same read-only operator" + + " the masked password attribute exists to serve"); + + Assertions.assertArrayEquals(classNamesOf(live), classNamesOf(published), + "the masking is a display copy and not a shorter answer: the same interceptors, in the order the" + + " chain is built in, so the attribute still says what this pool intercepts"); + InterceptorDefinition publishedDefinition = definitionNamed(published, "ResetAbandonedTimer"); + Assertions.assertNotNull(publishedDefinition, + "precondition: the configured definition must be the one the array published"); + Assertions.assertEquals("yes", publishedDefinition.getProperties().get("fast").getValue(), + "a property that is not a credential is published as configured: the attribute is read to see" + + " how a pool is set up, and a mask that swallowed the harmless pairs would leave an" + + " operator reading an empty configuration"); + Assertions.assertEquals(MASK, publishedDefinition.getProperties().get("password").getValue(), + "and the credential is published under the mask the string attribute and the configuration dump" + + " already use, the pair still named as a password, so the monitor can tell that a" + + " credential is configured without learning which one it is"); + + InterceptorDefinition[] cached = ds.getPoolProperties().getJdbcInterceptorsAsArray(); + Assertions.assertNotSame(cached, published, + "the attribute must answer with a copy of its own: as published it handed out the pool's live" + + " array of definitions, the very objects every new session's chain is built from, so" + + " a reader in the pool's own JVM held the pool's configuration in writable form"); + publishedDefinition.addProperty(new InterceptorProperty("fast", "rewritten-by-a-reader")); + Assertions.assertEquals("yes", definitionNamed(cached, "ResetAbandonedTimer").getProperties().get("fast") + .getValue(), + "and writing to what the attribute handed back must stop at the copy: a configuration an" + + " unprivileged reader can rewrite is a configuration it can break, and here it would" + + " break the interceptors of every connection the pool opens from then on"); + + Assertions.assertEquals(INTERCEPTOR_SECRET, + definitionNamed(ds.getPoolProperties().getJdbcInterceptorsAsArray(), "ResetAbandonedTimer") + .getProperties().get("password").getValue(), + "the configuration the interceptor chain is built from must keep the pair as it was written: the" + + " mask belongs to the display copy, and an interceptor that was handed the mask would" + + " authenticate with the mask"); + } finally { + closeAndUnpublish(r); + } + } + @Test void aCredentialWrittenThroughThePoolMBeanIsStoredRawAndReadBackMasked() throws Exception { Registration r = publish(poolWithInlineCredentials(), "write-path"); @@ -354,4 +428,43 @@ class TestJmxCredentialAttributes extends JdbcPoolBaseTest { private static String urlForDatabase(String database) { return getUrl().substring(0, getUrl().indexOf("/mem:")) + "/mem:" + database; } + + /* + * The first thing a definition array still carries in the clear - every class + * name and every property value of every definition is walked, because the + * attribute publishes the whole definition rather than only its properties - + * or {@code null} when the array holds no cleartext. + */ + private static String cleartextIn(InterceptorDefinition[] definitions, String secret) { + for (InterceptorDefinition definition : definitions) { + if (definition == null) { + continue; + } + if (definition.getClassName() != null && definition.getClassName().contains(secret)) { + return definition.getClassName(); + } + for (InterceptorProperty property : definition.getProperties().values()) { + String value = property.getValue(); + if (value != null && value.contains(secret)) { + return value; + } + } + } + return null; + } + + /* The class names of a definition array, in the order the array holds them. */ + private static String[] classNamesOf(InterceptorDefinition[] definitions) { + return Arrays.stream(definitions).map(InterceptorDefinition::getClassName).toArray(String[]::new); + } + + /* The definition of the named interceptor of an array, or {@code null} when it holds none. */ + private static InterceptorDefinition definitionNamed(InterceptorDefinition[] definitions, String className) { + for (InterceptorDefinition definition : definitions) { + if (definition != null && className.equals(definition.getClassName())) { + return definition; + } + } + return null; + } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxCredentialWrites.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxCredentialWrites.java index 5ab0c671a0..567322b45f 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxCredentialWrites.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxCredentialWrites.java @@ -16,6 +16,7 @@ */ package org.apache.tomcat.jdbc.pool.jmx; +import java.lang.management.ManagementFactory; import java.sql.Connection; import java.sql.Driver; import java.sql.DriverPropertyInfo; @@ -29,6 +30,10 @@ import java.util.Properties; import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Logger; +import javax.management.Attribute; +import javax.management.MBeanServer; +import javax.management.ObjectName; + import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -49,9 +54,18 @@ import org.apache.tomcat.jdbc.pool.PoolUtilities; * The pool here is configured the way the complaint is configured in practice - the credential sits among the * database properties, with no top-level property overriding it. The driver records the properties it is handed and * then connects with the test database's own credentials, so that the assertions are about what the pool offers the - * driver rather than about what the database would have accepted. The last test leaves that fixture for one where the - * credential is a top level property and the database refuses a superseded password, because what it pins is how a - * write reaches the connections that were already in service when it was written. + * driver rather than about what the database would have accepted. The rotation test leaves that fixture for one where + * the credential is a top level property and the database refuses a superseded password, because what it pins is how + * a write reaches the connections that were already in service when it was written. + * <p> + * The last two tests pin the database properties attribute itself, the one configuration surface whose read and + * write are the same attribute: the read is masked and the write replaces the whole set, so the ordinary idiom of + * reading the attribute, changing one entry and writing it back is a write of the set without the password. That + * consequence is deliberate and documented on {@link ConnectionPool#getDbProperties()} and + * {@link ConnectionPool#setDbProperties(Properties)}, and what these tests pin is that it costs exactly what the + * documentation says: over a pool whose credential lives only among the database properties the round trip takes it + * away and new connections go out with nothing to authenticate them, while over a pool whose credential the password + * property also holds, the mirrored entry still drops but the property is what signs the connections afterwards. */ class TestJmxCredentialWrites extends JdbcPoolBaseTest { @@ -247,6 +261,140 @@ class TestJmxCredentialWrites extends JdbcPoolBaseTest { } } + /* A published-MBean pool whose credential is a top level property, so the setters mirror it into the entries. */ + private DataSource poolWithTopLevelCredential() throws Exception { + RecordingDriver.lastConnectInfo.set(null); + DataSource ds = newDataSource(); + ds.setDriverClassName(RecordingDriver.class.getName()); + PoolConfiguration properties = ds.getPoolProperties(); + properties.setJmxEnabled(true); + properties.setInitialSize(0); + properties.setUsername("alice"); + properties.setPassword("first-secret"); + properties.getDbProperties().setProperty("applicationName", "pool-jmx-test"); + ds.createPool(); + Assertions.assertNotNull(ds.getPool().getJmxPool(), + "precondition: the fixture must have published the pool's own MBean"); + return ds; + } + + /* + * The platform server the pool's own MBean is published to, and the registration a container would have made: + * the data source itself is not a JMX compliant MBean, so preRegister() is what publishes the pool MBean. The + * round-trip tests read and write the database properties attribute over the server rather than calling the + * wrapper, because it is the attribute - as any console sees it - whose masked value makes the write destructive. + */ + private static MBeanServer server() { + return ManagementFactory.getPlatformMBeanServer(); + } + + private static ObjectName publish(DataSource ds) throws Exception { + ObjectName dsName = new ObjectName("tomcat:type=JmxCredentialWritesTest,uid=" + System.nanoTime()); + ds.preRegister(server(), dsName); + ObjectName poolOname = ds.createObjectName(dsName); + Assertions.assertTrue(server().isRegistered(poolOname), + "precondition: the container's registration must have published the pool's own MBean"); + return poolOname; + } + + /* Takes a registration down the way a closing container does, and leaves nothing of it published. */ + private static void unpublish(DataSource ds, ObjectName poolOname) throws Exception { + MBeanServer server = server(); + ds.close(); + ds.postDeregister(); + String uid = poolOname.getKeyProperty("uid"); + for (ObjectName stale : server.queryNames(new ObjectName("tomcat.jdbc:*"), null)) { + if (uid.equals(stale.getKeyProperty("uid"))) { + server.unregisterMBean(stale); + } + } + } + + /** + * The database properties attribute is published read-write, and unlike the masked strings beside it the masked + * value it reads is a whole configuration: an operator who reads it, changes one entry and writes it back hands + * the pool a set with the password taken out, and because the write replaces the live collection, that is the + * credential's last appearance. Nothing on the read, the write or the MBeanInfo says so - every attribute of a + * standard registration carries the same boilerplate description - so the hazard is documented on the attribute + * instead, and what is pinned here is that it costs exactly what that documentation says: the live configuration + * loses the entry, a connection the pool establishes afterwards is handed no password, and the Password + * attribute - the credential's owning control, which mirrors into the database properties - is what puts it back. + */ + @Test + void aDatabasePropertiesRoundTripThroughThePoolMBeanTakesTheCredentialAway() throws Exception { + DataSource ds = poolWithMBean(); + Assertions.assertEquals("first-secret", establishFreshConnection(ds).getProperty(PoolUtilities.PROP_PASSWORD), + "precondition: the credential among the database properties is what connects"); + + ObjectName poolOname = publish(ds); + try { + Properties read = (Properties) server().getAttribute(poolOname, "DbProperties"); + Assertions.assertEquals("alice", read.getProperty(PoolUtilities.PROP_USER), + "precondition: the published read still carries the user name - it looks complete"); + Assertions.assertEquals("pool-jmx-test", read.getProperty("applicationName"), + "precondition: and the ordinary properties unchanged"); + Assertions.assertFalse(read.containsKey(PoolUtilities.PROP_PASSWORD), + "precondition: the read is the masked one - and nothing in the set it hands the caller says " + + "which pair it left out"); + + server().setAttribute(poolOname, new Attribute("DbProperties", read)); + + Assertions.assertFalse(ds.getPoolProperties().getDbProperties().containsKey(PoolUtilities.PROP_PASSWORD), + "the write is the whole set the read handed it, so the live configuration now holds no password " + + "entry: the write is a replacement, not a change applied over the set the pool holds"); + Properties handed = establishFreshConnection(ds); + Assertions.assertFalse(handed.containsKey(PoolUtilities.PROP_PASSWORD), + "and the pool that lost the entry establishes its connections with nothing to authenticate them - " + + "the documented, deliberate consequence of a masked read beside a whole-set write, " + + "pinned so that the documentation cannot silently drift from the behaviour"); + Assertions.assertEquals("alice", handed.getProperty(PoolUtilities.PROP_USER), + "the round trip changed the credential and nothing else"); + Assertions.assertEquals("pool-jmx-test", handed.getProperty("applicationName"), + "the ordinary properties came back exactly as written"); + + ds.getPool().getJmxPool().setPassword("first-secret"); + Assertions.assertEquals("first-secret", + establishFreshConnection(ds).getProperty(PoolUtilities.PROP_PASSWORD), + "the Password attribute is the credential's owning control: writing it mirrors the entry back " + + "among the database properties and the pool authenticates again"); + } finally { + unpublish(ds, poolOname); + } + } + + /** + * The same round trip over a pool whose password the property also holds, which is where the two configurations + * part: the replaced set carries no password entry whatever mirrored it, because the write re-mirrors nothing - + * but the connect path falls back on the property, so this configuration loses the mirrored entry, not its + * credential. + */ + @Test + void aDatabasePropertiesRoundTripThroughThePoolMBeanLeavesATopLevelPasswordAuthenticating() throws Exception { + DataSource ds = poolWithTopLevelCredential(); + Assertions.assertEquals("first-secret", establishFreshConnection(ds).getProperty(PoolUtilities.PROP_PASSWORD), + "precondition: the mirrored entry is present and what the driver is handed"); + Assertions.assertEquals("first-secret", ds.getPoolProperties().getDbProperties() + .getProperty(PoolUtilities.PROP_PASSWORD), "precondition: and it mirrors the property"); + + ObjectName poolOname = publish(ds); + try { + Properties read = (Properties) server().getAttribute(poolOname, "DbProperties"); + server().setAttribute(poolOname, new Attribute("DbProperties", read)); + + Assertions.assertFalse(ds.getPoolProperties().getDbProperties().containsKey(PoolUtilities.PROP_PASSWORD), + "even here the replaced set holds no password entry: the write mirrors nothing back from the " + + "properties beside it"); + Assertions.assertEquals("first-secret", ds.getPoolProperties().getPassword(), + "while the property the credential also lives in is untouched by the write"); + Assertions.assertEquals("first-secret", + establishFreshConnection(ds).getProperty(PoolUtilities.PROP_PASSWORD), + "and what authenticates afterwards is the property: this configuration loses an entry it had " + + "been mirroring, not the credential that signs its connections"); + } finally { + unpublish(ds, poolOname); + } + } + /** * A driver that, like a database that has just rotated a password, accepts only the credential it is currently * configured to accept and refuses every other, and that records the password each connect presented. The --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
