This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit c0ded1f1cf2fde5acdb1255c6390028c00fcf06b Author: opencode <[email protected]> AuthorDate: Tue Nov 3 00:30:03 2026 +0100 Harden the configuration's credential masking, cloning and dump A configuration dump reaches the log through DataSourceProxy.toString(), the configuration is cloned by the connect path and serialized by the JMX and JNDI paths, and it is reconfigured while the pool runs. This covers those surfaces. Credential masking. toString() printed the url, connectionProperties and jdbcInterceptors strings raw, so a password typed into one of them reached the log in cleartext. Those three strings now pass through masking: the value of every credential pair the java.util.Properties parser really applies - keys password, pass and pwd, with the value running to the next boundary that parser sees, including across the new lines it deletes as backslash continuations - the password of every scheme:// authority in a string and of the legacy Oracle user/password@host form, and the credential pairs carried inside an interceptor definition. Every character the parser accepts as a value separator, including the form feed, separates a masked credential as well, so no form that delivers a password to a driver evades the mask. The pass is documented as best effort, with the colon dialects and driver-specific keys named as its limits. Configuration state. setConnectionProperties parses before it touches any state, so a string the parser rejects leaves the previous configuration fully in place. Re-setting removes the pairs the previous value contributed while keeping entries contributed from elsewhere, and clearing a credential field takes the shared entry away whatever contributed it - saying so at warn level when the connection properties string still names that key. Cloning and serialization. A clone gets its own driver-properties collection, since a shared one let a reconfigure on either copy strip the other's pairs, and it resolves a defaults-backed property the way the original does. The clone and the serialization both run under the monitor the reconfiguring setters hold, so neither can capture a state no setter ever published. The dump. One failing property costs only its own field, including an Error thrown by the object a property holds, and an unset password prints as null instead of wearing the mask of a secret that exists. Interceptor definitions. A property without a name is refused at construction rather than failing the pool start later on a generated class name, with a message naming nothing. Found by a class-level review of PoolProperties; each behaviour above is pinned by a test in TestPoolProperties, 29 of them added here. --- .../apache/tomcat/jdbc/pool/PoolConfiguration.java | 9 +- .../apache/tomcat/jdbc/pool/PoolProperties.java | 645 ++++++++++++++++++--- .../tomcat/jdbc/pool/util/LocalStrings.properties | 2 + .../tomcat/jdbc/pool/TestPoolProperties.java | 561 ++++++++++++++++++ webapps/docs/changelog.xml | 10 + 5 files changed, 1139 insertions(+), 88 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java index 93c59d8966..715794ebd3 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java @@ -102,9 +102,16 @@ public interface PoolConfiguration { * The properties that will be passed into {@link java.sql.Driver#connect(String, Properties)} method. Username and * password do not need to be stored here, they will be passed into the properties right before the connection is * established. + * <p> + * Re-setting the string re-applies it: the pairs the previous value of the + * string contributed are removed from the driver properties before the new + * ones are parsed in, so a reconfigure that drops a pair really stops + * sending it, while entries contributed from elsewhere - the explicitly + * passed user and password among them - are kept. A {@code null} value + * clears the string and takes its contributed pairs with it. * * @param connectionProperties properties - Format of the string is [propertyName=property;]* Example: - * prop1=value1;prop2=value2 + * prop1=value1;prop2=value2, may be {@code null} */ void setConnectionProperties(String connectionProperties); 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 5340b6ce1c..9255ace2a9 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 @@ -17,16 +17,19 @@ package org.apache.tomcat.jdbc.pool; import java.io.IOException; +import java.io.ObjectOutputStream; import java.io.Serializable; import java.io.StringReader; import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.Properties; import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; @@ -34,6 +37,7 @@ import java.util.concurrent.atomic.AtomicInteger; import org.apache.juli.logging.Log; import org.apache.juli.logging.LogFactory; +import org.apache.tomcat.jdbc.pool.util.ExceptionUtils; import org.apache.tomcat.jdbc.pool.util.MessageFactory; /** @@ -118,7 +122,13 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl private volatile int removeAbandonedTimeout = 60; /** Whether to log abandoned connections. */ private volatile boolean logAbandoned = false; - /** The pool name. */ + /** + * The pool name. The identity hash of the class object is the same for + * every pool in a JVM - it identifies the class loader that loaded this + * class, not the pool - so the per-pool uniqueness of the default name + * comes from the counter alone. The hash term is upstream's default-name + * shape and stays for that reason. + */ private volatile String name = "Tomcat Connection Pool[" + (poolCounter.addAndGet(1)) + "-" + System.identityHashCode(PoolProperties.class) + "]"; /** The database password. */ @@ -449,8 +459,7 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl @Override public synchronized InterceptorDefinition[] getJdbcInterceptorsAsArray() { if (interceptors == null) { - // One read of the reconfigurable string: the parse below must see - // a single value, not a fresh read per use. + // Read once: the parse below must work on a single snapshot. String interceptorList = jdbcInterceptors; if (interceptorList == null) { interceptors = new InterceptorDefinition[0]; @@ -472,18 +481,17 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl propsAsString = interceptorValues[i].substring(propIndex + 1, endIndex); } if (name.isEmpty()) { - // The name is empty when a leading or doubled - // separator leaves an empty segment, or when a - // segment opens straight on "(": there is no - // interceptor to load, and a definition without a - // class name fails the whole pool start later, on + // A leading or doubled separator, or a segment that + // opens straight on "(", leaves no interceptor name: + // there is nothing to load, and a definition without + // a class name fails the whole pool start later, on // the generated "...pool.interceptor." class, with a // message naming nothing. split() already drops a // redundant trailing separator and the property parse - // below tolerates a redundant separator inside the - // parentheses: skipping the empty name makes a - // redundant separator cosmetic throughout the syntax - // rather than fatal wherever split() kept it. + // tolerates one inside the parentheses, so skipping + // the empty name makes a redundant separator cosmetic + // throughout the syntax rather than fatal wherever + // split() kept it. continue; } InterceptorDefinition definition = new InterceptorDefinition(name); @@ -537,7 +545,10 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl * field's value even when this string names that key, the rule the * credential setters establish. A configuration whose * database properties are {@code null} only stores the raw string, as there - * is no property collection to parse into. The remove and re-add run inside + * is no property collection to parse into. The string is parsed before any + * state is touched, so a value the parser rejects - a malformed Unicode + * escape - leaves the previous configuration fully in place + * along with the exception. The remove and re-add run inside * this one synchronized method; an unsynchronised reader of the database * properties, which {@link #getDbProperties()} has always handed out live, * can therefore observe the contributed keys briefly missing. @@ -547,8 +558,13 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl */ @Override public synchronized void setConnectionProperties(String connectionProperties) { - this.connectionProperties = connectionProperties; Properties dbProps = getDbProperties(); + // Parse into a fresh collection before anything is mutated: that is + // what tells apart the keys this string contributes from the ones + // dbProperties holds from elsewhere, and a string the parse rejects + // must not have cost the previous configuration its pairs yet. + Properties parsed = dbProps == null ? null : getProperties(connectionProperties, new Properties()); + this.connectionProperties = connectionProperties; if (dbProps == null) { // The pairs have no collection to land in; nothing was contributed, // so there is nothing to track for a later removal either. @@ -562,9 +578,6 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl dbProps.remove(key); } } - // Parse into a fresh collection first: that is what tells apart the keys - // this string contributes from the ones dbProperties holds from elsewhere. - Properties parsed = getProperties(connectionProperties, new Properties()); Set<String> keys = new HashSet<>(parsed.size()); for (String key : parsed.stringPropertyNames()) { dbProps.setProperty(key, parsed.getProperty(key)); @@ -572,11 +585,15 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl } // The credential setters' rule outlives the string: while a credential // field is set, its dbProperties entry holds the field's value, even - // when this string named the key itself - DataSourceFactory injects the + // when this string named the key - DataSourceFactory injects the // configured username/password over the string's pairs, so a // reconfigure that drops the keys from the string must not strip - // credentials the configuration still carries. With the field unset the - // entry is the string's own again, exactly as after setPassword(null). + // credentials the configuration still carries. With the field unset + // this parse re-asserts nothing and the entry is the string's own + // again. The entry can also be missing here although the string names + // the key, because clearing a credential takes the entry away whatever + // contributed it; this parse hands the pair back - writing the string + // is a fresh declaration of what it contributes. if (username != null) { dbProps.setProperty(PoolUtilities.PROP_USER, username); } @@ -699,6 +716,14 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl * <p> * A configuration whose database properties are {@code null} has no collection to hold the entry, so only the * field is stored. + * <p> + * The removal covers the entry whatever put it there. A {@code connectionProperties} string that names + * {@code password} contributes the same entry, and clearing the password takes that entry away as well: clearing + * a credential has to win over every surface that still names the key, because it is the operation an operator + * uses to stop a credential authenticating connections. The string is not modified, so + * {@link #getConnectionProperties()} keeps reporting it and the next {@link #setConnectionProperties(String)} + * whose value names the key applies that pair again. Retiring a credential the string carries means taking it + * out of the string. * * @param password The password */ @@ -708,6 +733,7 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl if (this.dbProperties != null) { if (password == null) { this.dbProperties.remove(PoolUtilities.PROP_PASSWORD); + warnStringStillNamesCredential(PoolUtilities.PROP_PASSWORD); } else { this.dbProperties.setProperty(PoolUtilities.PROP_PASSWORD, password); } @@ -759,7 +785,8 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl /** * Stores the user name and keeps the {@code user} entry of the database properties in step with it, for the - * reason given on {@link #setPassword(String)}. + * reason given on {@link #setPassword(String)}. A {@code null} user name likewise removes the entry whatever + * contributed it, including a pair the {@code connectionProperties} string names. * * @param username The user name */ @@ -769,6 +796,7 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl if (this.dbProperties != null) { if (username == null) { this.dbProperties.remove(PoolUtilities.PROP_USER); + warnStringStillNamesCredential(PoolUtilities.PROP_USER); } else { this.dbProperties.setProperty(PoolUtilities.PROP_USER, username); } @@ -776,6 +804,24 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl } + /** + * Notes at warn level when clearing a credential has just removed an entry + * that the configured {@code connectionProperties} string still names, so + * the divergence between the reported configuration and the properties the + * driver is handed is not silent. Names the key, never the value: the clear + * is the operator's intent to retire the credential. + * + * @param key the credential key whose entry was removed + */ + private void warnStringStillNamesCredential(String key) { + // Called with the monitor held, which is also the one setConnectionProperties + // publishes the tracking under. + if (connectionPropertyKeys != null && connectionPropertyKeys.contains(key)) { + log.warn(MessageFactory.get("poolProperties.credentialClearStripsStringEntry", key)); + } + } + + @Override public void setValidationInterval(long validationInterval) { this.validationInterval = validationInterval; @@ -844,6 +890,21 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl } + /** + * Best-effort dump of the configuration. The {@code password} field is + * masked whenever it carries a secret and prints as {@code null} when + * none is configured, and the {@code url}, {@code connectionProperties} + * and {@code jdbcInterceptors} strings pass through credential masking, + * so the dump can be logged without carrying a cleartext credential. The + * masking is best effort in the way {@link #maskCredentials(String)} and + * {@link #maskInterceptorCredentials(String)} describe, so it removes the + * credentials an operator normally configures rather than proving the + * configuration holds no other secret. A property whose getter fails is + * reported as an error in its own field and does not truncate the rest of + * the dump. + * + * @return the dump, always bracket-balanced even on internal failure + */ @Override public String toString() { StringBuilder buf = new StringBuilder("ConnectionPool["); @@ -860,110 +921,464 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl } catch (NoSuchMethodException nm) { continue; } - Object value = m.invoke(this, new Object[0]); buf.append(field); buf.append('='); - if (DataSourceFactory.PROP_PASSWORD.equals(field)) { - buf.append(PASSWORD_MASK); - } else if (DataSourceFactory.PROP_URL.equals(field) - || DataSourceFactory.PROP_CONNECTIONPROPERTIES.equals(field)) { - // The same dump prints these raw configuration - // strings, so a credential typed into one of them - // has to be masked too, not just the password field. - buf.append(value == null ? "null" : maskCredentials((String) value)); - } else { - buf.append(value); + // One bad property - a subclass getter that throws, or a + // dataSource object whose toString() throws - must cost + // only its own field, not the rest of the dump. The + // append itself is inside this try because buf.append + // calls the object's toString() directly: an Error from a + // hostile third-party toString() arrives unwrapped here, + // not wrapped like one from inside Method.invoke, and a + // dump that promises not to throw must hold that promise + // against the arbitrary objects it prints - except for + // the fatal errors, which the module convention is to + // propagate rather than report. + try { + Object value = m.invoke(this, new Object[0]); + if (DataSourceFactory.PROP_PASSWORD.equals(field)) { + buf.append(value == null ? "null" : PASSWORD_MASK); + } else if (DataSourceFactory.PROP_URL.equals(field) + || DataSourceFactory.PROP_CONNECTIONPROPERTIES.equals(field)) { + // The same dump prints these raw configuration + // strings, so a credential typed into one of them + // has to be masked too, not just the password field. + buf.append(value == null ? "null" : maskCredentials((String) value)); + } else if (DataSourceFactory.PROP_INTERCEPTORS.equals(field)) { + // Interceptor properties reach a third-party + // interceptor through setProperties(), so this + // configuration string carries credentials the + // same way; its pairs live inside parentheses, + // which the segment-start walk above cannot see. + buf.append(value == null ? "null" : maskInterceptorCredentials((String) value)); + } else { + buf.append(value); + } + } catch (ReflectiveOperationException | RuntimeException | Error x) { + Throwable cause = ExceptionUtils.unwrapInvocationTargetException(x); + ExceptionUtils.handleThrowable(cause); + buf.append("<error: ").append(cause).append('>'); } buf.append("; "); break; } } - buf.append(']'); } catch (Exception x) { // Shouldn't happen if (log.isDebugEnabled()) { log.debug("toString() call failed", x); } + } finally { + // The javadoc promises a bracket-balanced dump even on internal + // failure, and an unterminated "ConnectionPool[" line is what a + // truncated diagnostic log entry looks like. + buf.append(']'); } return buf.toString(); } /** - * Masks the value of credential-like {@code key=value} pairs embedded in a - * raw configuration string so that a password typed into the JDBC URL query - * string or the connection properties is not printed in cleartext the way - * the dedicated password field is masked in {@link #toString()}. The keys - * recognised, compared ignoring case, are {@code password}, {@code pass} - * and {@code pwd}. A key is honoured only at a segment boundary - the start - * of the string or one of {@code ;}, {@code &} or {@code ?} - so a key such - * as {@code compass} is left alone, and its value runs to the next - * {@code ;}, {@code &} or the end of the string. + * Masks the credentials embedded in a raw configuration string so that a + * password typed into the JDBC URL or the connection properties is not + * printed in cleartext the way the dedicated password field is masked in + * {@link #toString()}. Two shapes are scrubbed. * <p> - * Only the {@code key=value} shape is scrubbed. The - * {@code user:password@host} userinfo form, the {@code key:value} and - * {@code key value} forms that a {@link java.util.Properties} reader - * additionally accepts, and driver-specific secrets that use a key other - * than the three above are not recognised; recognising them would require - * a full parse of each driver's URL dialect. + * The value of a pair whose key - at the start of a segment, ignoring + * case - is one of {@code password}, {@code pass} and {@code pwd}. A + * segment starts at the beginning of the string or at {@code ;}, a new + * line, {@code &} or {@code ?}. Only the first two are boundaries to + * the properties parser that applies a configuration string, and the + * pairs behind them are the pairs a driver receives: their value runs to + * the next such boundary, because an {@code &} or {@code ?} is an + * ordinary character inside a delivered value and masking up to one + * would print the tail of the password the driver really receives, while + * a new line the reader deletes as a backslash continuation is no + * boundary at all and the mask runs across it into the next line. A pair + * that only appears to start behind {@code &} or {@code ?} was + * folded into another pair's value by the parser, and it is masked to the + * end of its segment all the same - a secret typed there was typed as a + * credential. The key runs to the first value separator the parser + * accepts - {@code =}, {@code :} or white space - so a key such as + * {@code compass}, a credential-shaped word inside a longer value, and + * the colons of a JDBC URL scheme, host or port are all left alone. + * Either way a masked value cannot hide a later pair. A pair with an + * empty value is masked like any other, so a masked pair in the dump does + * not by itself say that a non-empty secret was configured. + * <p> + * The password of a URL {@code user:password@host} userinfo section, and + * of the equivalent {@code user/password@host} form used by the legacy + * Oracle driver URLs, wherever they appear in the string. The user name is + * kept visible for diagnostics. + * <p> + * The pass is best effort rather than a guarantee: colon-separated + * dialects that put a pair behind the colon rather than at the segment + * start, like DB2's {@code :user=x:password=y}, and driver-specific + * secrets that use a key other than the three above are not recognised, + * and recognising them would require a full parse of each driver's URL + * dialect. What a masked result says is that no credential of the + * recognised shapes remains in the string, not that the string carries no + * secret at all. * * @param value the raw configuration string, may be {@code null} * - * @return the string with every credential key's value replaced by the mask, - * or the string unchanged when it holds no recognised credential key + * @return the string with every recognised credential replaced by the mask, + * or the string unchanged when it holds none */ private static String maskCredentials(String value) { - if (value == null || value.indexOf('=') < 0) { - return value; + if (value == null) { + return null; } - StringBuilder result = new StringBuilder(value.length()); + // The authority pass runs first: a userinfo-only URL carries no pair + // the segment walk below would recognise, and its colons must not + // reach the walk as candidate value separators. + String scrubbed = maskUrlAuthority(value); + // Walk the parser's segments: one pair per segment, key first, exactly + // as java.util.Properties reads them after the semicolons became new + // lines. Anything that is not a recognised credential pair - the + // JDBC URL itself, driver values carrying colons or spaces - is copied + // verbatim. + StringBuilder result = new StringBuilder(scrubbed.length()); int i = 0; - while (i < value.length()) { - int equals = value.indexOf('=', i); - if (equals < 0) { - break; + boolean afterPairBoundary = true; + while (i < scrubbed.length()) { + int segmentEnd = i; + while (segmentEnd < scrubbed.length() && !isCredentialSegmentSeparator(scrubbed.charAt(segmentEnd))) { + segmentEnd++; } - int keyStart = equals; - while (keyStart > 0 && !isCredentialSegmentSeparator(value.charAt(keyStart - 1))) { - keyStart--; + // The reader skips leading white space before a key. + int keyStart = i; + while (keyStart < segmentEnd && isCredentialValueSeparator(scrubbed.charAt(keyStart))) { + keyStart++; } - String key = value.substring(keyStart, equals).trim(); - int valueStart = equals + 1; - int valueEnd = value.length(); - for (int k = valueStart; k < value.length(); k++) { - char c = value.charAt(k); - if (c == ';' || c == '&') { - valueEnd = k; - break; - } + int keyEnd = keyStart; + while (keyEnd < segmentEnd && !isCredentialValueSeparator(scrubbed.charAt(keyEnd))) { + keyEnd++; } - result.append(value, i, equals); - result.append('='); - if (isCredentialKey(key)) { + if (keyEnd < segmentEnd && isCredentialKey(scrubbed.substring(keyStart, keyEnd))) { + // Keep the pair's key and the separator, hide the value. + int valueEnd = segmentEnd; + if (afterPairBoundary) { + // A pair the properties parser applies, so the value - + // the password the driver really receives - runs to the + // next boundary that parser sees. An '&' or '?' inside it + // is an ordinary character there, and a boundary the + // reader deletes as part of a backslash continuation is + // not such a boundary either. + valueEnd = segmentEnd; + while (valueEnd < scrubbed.length()) { + while (valueEnd < scrubbed.length() + && !isPairSeparator(scrubbed.charAt(valueEnd))) { + valueEnd++; + } + if (valueEnd < scrubbed.length() && continuesValueLine(scrubbed, valueEnd)) { + valueEnd++; + if (valueEnd < scrubbed.length() && scrubbed.charAt(valueEnd) == '\n') { + // The new line of a \r\n pair, which the + // reader deletes as one new line with the \r. + valueEnd++; + } + } else { + break; + } + } + } + result.append(scrubbed, i, keyEnd + 1); result.append(PASSWORD_MASK); + i = valueEnd; } else { - result.append(value, valueStart, valueEnd); + result.append(scrubbed, i, segmentEnd); + i = segmentEnd; + } + if (i < scrubbed.length()) { + // Keep the segment separator itself. + char boundary = scrubbed.charAt(i); + result.append(boundary); + i++; + afterPairBoundary = isPairSeparator(boundary); + } + } + return result.toString(); + } + + + /** + * Masks the credential values carried in an interceptor definition + * string, as a best-effort reading of that syntax rather than a parse of + * it. The pair reading mirrors what {@link + * #getJdbcInterceptorsAsArray()} delivers to an interceptor: the + * comma-separated pairs inside a definition's parentheses, with a pair's + * name being the text before its first {@code =}, trimmed. Every + * parenthesised list of a definition is read, not just the one the parse + * consumes, and a list that never closes is read to the end of the + * definition: a pair typed in either place was typed as a credential, + * even though a malformed definition hides it from the interceptor. A + * pair whose name is one of the keys {@link #isCredentialKey recognises} + * has its value replaced by the mask; everything else - the class names, + * the property names and the non-credential values - is copied verbatim, + * so the dump still shows which interceptors and properties are + * configured. Like {@link #maskCredentials(String)} this rules out the + * names this class treats as credentials, not any secret an interceptor + * may hold under another name. + * + * @param value the raw interceptor definition string, may be {@code null} + * + * @return the string with every recognised credential value replaced by + * the mask, or the string unchanged when it holds none + */ + private static String maskInterceptorCredentials(String value) { + if (value == null) { + return null; + } + StringBuilder result = new StringBuilder(value.length()); + int segmentStart = 0; + while (segmentStart < value.length()) { + int segmentEnd = value.indexOf(';', segmentStart); + if (segmentEnd < 0) { + segmentEnd = value.length(); + } + int cursor = segmentStart; + int open = value.indexOf('(', cursor); + while (open >= 0 && open < segmentEnd) { + result.append(value, cursor, open + 1); + int close = value.indexOf(')', open + 1); + if (close < 0 || close > segmentEnd) { + // Never closed inside this segment (the closing + // parenthesis of a later definition does not close this + // one either): mask whatever was typed as a property to + // the end of the segment. + close = segmentEnd; + } + int pairStart = open + 1; + while (pairStart < close) { + int pairEnd = value.indexOf(',', pairStart); + if (pairEnd < 0 || pairEnd >= close) { + pairEnd = close; + } + int separator = value.indexOf('=', pairStart); + if (separator >= 0 && separator < pairEnd + && isCredentialKey(value.substring(pairStart, separator).trim())) { + result.append(value, pairStart, separator + 1); + result.append(PASSWORD_MASK); + } else { + result.append(value, pairStart, pairEnd); + } + if (pairEnd < close) { + result.append(','); + } + pairStart = pairEnd + 1; + } + cursor = close; + open = value.indexOf('(', cursor); + } + // What is left of the segment - the class name before the first + // parenthesis and the text after the last closed property list - + // is outside every property list. + result.append(value, cursor, segmentEnd); + if (segmentEnd < value.length()) { + // Keep the definition separator itself. + result.append(';'); + } + segmentStart = segmentEnd + 1; + } + return result.toString(); + } + + + /** + * Whether {@code c} ends a key and starts the pair's value, covering the + * separators a {@link java.util.Properties} reader accepts; white space + * doubles as the leading white space the reader skips before a key. + * + * @param c the character to test + * + * @return {@code true} when the character is a key/value separator + */ + private static boolean isCredentialValueSeparator(char c) { + return c == '=' || c == ':' || c == ' ' || c == '\t' || c == '\f'; + } + + + /** + * Masks the password of the authority section of every URL in a raw + * configuration string, as a best-effort reading of the URL rather than a + * parse of a driver's dialect, covering the {@code user:password@host} + * userinfo form of a {@code scheme://} URL and the {@code user/password@host} + * form of the legacy Oracle driver URLs. The scan of an authority is + * bounded so that a {@code :} of a port or an {@code @} or {@code :} + * appearing later in the path or query is not mistaken for a credential + * separator; a name that is not followed by a credential separator inside + * the authority is treated as a bare host and left alone rather than + * guessed at. + * + * @param value the raw configuration string, not {@code null} + * + * @return the string with every recognised authority password replaced by + * the mask, or the string unchanged when it carries none + */ + private static String maskUrlAuthority(String value) { + // Mask ranges are collected first and emitted in one pass at the end, + // so the two recognisers below cannot interfere through the order in + // which they happen to find their matches. + List<int[]> ranges = new ArrayList<>(2); + // Every scheme:// authority in the string: a failover or logging URL + // carried in a value is as credential-bearing as the URL in front of + // it, and the authority never contains "://" itself (the scan of one + // authority ends at the first '/'), so the search resumes after it. + int from = 0; + int schemeEnd; + while ((schemeEnd = value.indexOf("://", from)) >= 0) { + int authorityStart = schemeEnd + 3; + int authorityEnd = authorityStart; + while (authorityEnd < value.length() && !isAuthorityTerminator(value.charAt(authorityEnd))) { + authorityEnd++; + } + // An unencoded '@' inside the password itself can only extend the + // userinfo, so the credential runs up to the last '@' of the + // authority, if any. + int at = value.lastIndexOf('@', authorityEnd - 1); + if (at >= authorityStart) { + int colon = value.indexOf(':', authorityStart); + if (colon >= 0 && colon < at) { + ranges.add(new int[] { colon + 1, at }); + } + } + from = authorityEnd; + } + // Legacy Oracle form, e.g. jdbc:oracle:thin:user/password@host:1521:sid, + // wherever it appears in the string - as the URL itself or as the + // value of a pair. Its scan is bounded by the segment the prefix + // starts in, so an '@' of a later URL cannot drag the mask across it. + String oraclePrefix = "jdbc:oracle:"; + for (int p = indexOfIgnoreCase(value, oraclePrefix, 0); p >= 0; + p = indexOfIgnoreCase(value, oraclePrefix, p + 1)) { + int prefixEnd = p + oraclePrefix.length(); + int segmentEnd = prefixEnd; + while (segmentEnd < value.length() && !isCredentialSegmentSeparator(value.charAt(segmentEnd))) { + segmentEnd++; + } + int firstAt = value.indexOf('@', prefixEnd); + if (firstAt >= 0 && firstAt < segmentEnd) { + int slash = value.indexOf('/', prefixEnd); + if (slash >= 0 && slash < firstAt) { + // The section opens with the user/password pair. An + // unencoded '@' inside the password itself can only + // extend the credential, so as in the scheme:// pass + // above it runs up to the last '@' of the segment; the + // at-sign-only connect forms, which carry neither a user + // nor a password, are already excluded by the slash + // lying behind the first '@'. + int at = value.lastIndexOf('@', segmentEnd - 1); + ranges.add(new int[] { slash + 1, at }); + } + } + } + if (ranges.isEmpty()) { + return value; + } + ranges.sort(Comparator.comparingInt(range -> range[0])); + StringBuilder result = new StringBuilder(value.length()); + int copied = 0; + for (int[] range : ranges) { + if (range[0] < copied) { + // Overlaps a mask already emitted - the pathological case of + // one recognised form nested in the other's credential. The + // earlier, outer mask has already hidden this region's start, + // and re-masked text is never re-scanned. + continue; } - // Resume after the value so a '=' inside a masked value cannot be - // mistaken for the start of a new key. - i = valueEnd; + result.append(value, copied, range[0]); + result.append(PASSWORD_MASK); + copied = range[1]; } - result.append(value, i, value.length()); + result.append(value, copied, value.length()); return result.toString(); } + /** + * Finds the first occurrence of {@code needle} in {@code value} at or + * after {@code from}, ignoring case. + * + * @param value the string to search + * @param needle the literal to look for + * @param from the index to start at + * + * @return the index of the match, or {@code -1} + */ + private static int indexOfIgnoreCase(String value, String needle, int from) { + for (int i = Math.max(from, 0); i <= value.length() - needle.length(); i++) { + if (value.regionMatches(true, i, needle, 0, needle.length())) { + return i; + } + } + return -1; + } + + + /** + * Whether {@code c} ends the authority section of a URL, covering the path, + * query and the semicolon used as a parameter separator by some drivers. + * + * @param c the character to test + * + * @return {@code true} when the character ends the authority section + */ + private static boolean isAuthorityTerminator(char c) { + return c == '/' || c == '?' || c == ';'; + } + + /** * Whether {@code c} ends one configuration segment and starts the next, - * covering the URL query separators and the semicolon used by the - * connection properties string. + * covering the URL query separators, the semicolon used by the connection + * properties string and the new lines that separate the pairs for the + * parser that applies the string. * * @param c the character to test * * @return {@code true} when the character is a segment boundary */ private static boolean isCredentialSegmentSeparator(char c) { - return c == ';' || c == '&' || c == '?'; + return c == ';' || c == '&' || c == '?' || c == '\n' || c == '\r'; + } + + + /** + * Whether {@code c} ends a pair for the properties parser that applies + * the connection properties string, one of the boundaries along which a + * delivered value cannot run - see {@link #continuesValueLine(String, + * int)} for the one new line that is no boundary at all. The query + * separators {@code &} and {@code ?} are ordinary characters inside a + * delivered value and so are deliberately not pair boundaries here, + * whatever they separate once a driver reads a URL-shaped value itself. + * + * @param c the character to test + * + * @return {@code true} when the character ends a pair for the parser + */ + private static boolean isPairSeparator(char c) { + return c == ';' || c == '\n' || c == '\r'; + } + + + /** + * Whether the new line at {@code lineBreak} is one the reader deletes as + * a line continuation, joining the next line into the delivered value: + * the line ends in an odd number of backslashes, of which the last one + * escapes the new line while the others stand as literal pairs. + * + * @param value the string being walked + * @param lineBreak the index of the new line to test + * + * @return {@code true} when the value continues on the next line + */ + private static boolean continuesValueLine(String value, int lineBreak) { + int backslashes = 0; + for (int i = lineBreak - 1; i >= 0 && value.charAt(i) == '\\'; i--) { + backslashes++; + } + return (backslashes & 1) == 1; } @@ -1159,12 +1574,16 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl /** * Constructs an InterceptorProperty with the given name and value. * - * @param name the property name + * @param name the property name, not {@code null} * @param value the property value + * @throws NullPointerException if the name is {@code null} - the name + * keys the property in the definitions' + * map, so a nameless property could only + * fail later, from the map's hashing */ public InterceptorProperty(String name, String value) { - assert (name != null); - this.name = name; + this.name = Objects.requireNonNull(name, + () -> MessageFactory.get("poolProperties.interceptorPropertyNameRequired")); this.value = value; } @@ -1317,6 +1736,10 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl /** * Returns the property value as a char. + * <p> + * The method name is misspaced (capital {@code A} in {@code AsChar}) + * in the upstream API. It is left as published so the existing public + * signature is not broken. * * @param def the default value if the property is null or invalid * @@ -1335,7 +1758,7 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl @Override public int hashCode() { - return name.hashCode(); + return Objects.hashCode(name); } @Override @@ -1345,7 +1768,7 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl } if (o instanceof InterceptorProperty) { InterceptorProperty other = (InterceptorProperty) o; - return other.name.equals(this.name); + return Objects.equals(other.name, this.name); } return false; } @@ -1545,9 +1968,57 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl this.useStatementFacade = useStatementFacade; } + /** + * Returns a copy of the configuration. Beyond the field-by-field copy, + * the driver-properties collection is copied too, through + * {@link PoolUtilities#clone(Properties)} - the same copy the connect + * path makes - so a defaults-backed property that resolves on the + * original also resolves on the copy. + * {@link #setConnectionProperties(String)} removes the keys the previous + * value of the string contributed, so a clone sharing the live collection + * would have a reconfigure on either copy strip the other one's pairs. The + * tracked key set is never mutated after publication, so both copies may + * share it, and it stays consistent with the copied entries. + * <p> + * The copy is taken under the monitor the reconfiguring setters hold, so + * a reconfiguration concurrent with the clone cannot appear in the copy + * as a state no setter ever published: a stale driver-property entry the + * copy's tracked key set would never remove again, or a fresh interceptor + * string whose published parse cache still holds the previous chain - + * which the cached read would then never re-parse. + * + * @return the copy + * @throws CloneNotSupportedException if cloning of this class is ever + * refused + */ @Override - protected Object clone() throws CloneNotSupportedException { - return super.clone(); + protected synchronized Object clone() throws CloneNotSupportedException { + PoolProperties clone = (PoolProperties) super.clone(); + Properties dbProperties = clone.dbProperties; + if (dbProperties != null) { + clone.dbProperties = PoolUtilities.clone(dbProperties); + } + return clone; + } + + + /** + * Serializes the configuration under the monitor the reconfiguring + * setters hold, for the same torn-snapshot reason {@link #clone()} + * synchronizes for: the setters publish a reconfiguration over several + * field writes, and a field-by-field serialization that lands between + * them would hand the stream a state no setter ever published. The + * monitor is a leaf, so holding it across the write cannot order against + * any other lock. + * + * @param oos the stream to write the object graph to + * + * @throws IOException if the stream fails + */ + private void writeObject(ObjectOutputStream oos) throws IOException { + synchronized (this) { + oos.defaultWriteObject(); + } } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index 591d47308b..f3cb9a5c2f 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -144,6 +144,8 @@ jmxUtil.jmxRegistrationFailed=Jmx registration failed. jmxUtil.jmxUnregistrationFailed=Jmx unregistration failed. poolProperties.invalidInterceptorProperty=Invalid interceptor property ''{0}'', missing ''='', in jdbcInterceptors value [{1}] +poolProperties.credentialClearStripsStringEntry=Clearing the ''{0}'' credential also removed the entry the connectionProperties string contributes for that key. The string still names it, so every connection established from now on goes without that credential, and a connectionProperties reconfigure that re-names the key applies it again. Take the credential out of the connectionProperties string to retire it. +poolProperties.interceptorPropertyNameRequired=The name of an interceptor property cannot be null. poolProperties.layeredPoolsNotAllowed=Layered pools are not allowed. poolProperties.validatorCannotInstantiate=An object of class {0} cannot be instantiated. Make sure that it includes an implicit or explicit no-arg constructor. poolProperties.validatorClassNotFound=The class {0} cannot be found. diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java index 57ee3de17a..0a92160c05 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Properties; +import org.apache.tomcat.jdbc.pool.util.MessageFactory; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -272,6 +273,62 @@ class TestPoolProperties { "the string must keep owning the entry while the field is unset"); } + @Test + void clearingTheFieldAlsoStripsTheEntryTheStringContributes() { + PoolProperties properties = new PoolProperties(); + properties.setConnectionProperties("password=fromstring"); + properties.setPassword("fromfield"); + + properties.setPassword(null); + + Assertions.assertNull(properties.getDbProperties().getProperty("password"), + "clearing the credential must stop it authenticating connections," + + " whatever surface contributed the entry"); + Assertions.assertEquals("password=fromstring", properties.getConnectionProperties(), + "the string is configuration and is not rewritten by a credential clear"); + } + + @Test + void rewritingTheStringAppliesItsCredentialPairAgain() { + PoolProperties properties = new PoolProperties(); + properties.setConnectionProperties("password=fromstring"); + + properties.setPassword(null); + properties.setConnectionProperties("password=fromstring"); + + Assertions.assertEquals("fromstring", properties.getDbProperties().getProperty("password"), + "re-declaring the string re-declares its pair - retiring the credential" + + " means taking it out of the string"); + } + + @Test + void aRejectedConnectionPropertiesStringLeavesTheConfigurationIntact() { + PoolProperties properties = new PoolProperties(); + properties.setConnectionProperties("keep=yes;stale=x"); + + Assertions.assertThrows(IllegalArgumentException.class, + () -> properties.setConnectionProperties("good=1;bad=\\uZZ"), + "a malformed unicode escape must be rejected"); + + Assertions.assertEquals("keep=yes;stale=x", properties.getConnectionProperties(), + "the rejected string must not have become the configured one"); + Assertions.assertEquals("yes", properties.getDbProperties().getProperty("keep"), + "the pairs the previous string contributed must still be there"); + Assertions.assertEquals("x", properties.getDbProperties().getProperty("stale"), + "the pairs the previous string contributed must still be there"); + + // The tracking must not have been lost either: a reconfigure after the + // rejected write must still remove the pairs of the string that really + // is configured. + properties.setConnectionProperties("only=new"); + + Assertions.assertEquals("new", properties.getDbProperties().getProperty("only")); + Assertions.assertNull(properties.getDbProperties().getProperty("keep"), + "a reconfigure after a rejected write must remove the old pairs"); + Assertions.assertNull(properties.getDbProperties().getProperty("stale"), + "a reconfigure after a rejected write must remove the old pairs"); + } + @Test void clearedConnectionPropertiesRemoveEveryParsedPair() { PoolProperties properties = new PoolProperties(); @@ -290,6 +347,115 @@ class TestPoolProperties { "an empty string must clear the pairs just like a null one does"); } + @Test + void anErrorFromAPropertyObjectToStringStillProducesADump() { + // buf.append(Object) calls the object's toString() directly, so an + // Error from the arbitrary dataSource object arrives unwrapped, unlike + // one from inside a getter. The dump must not throw over it. + PoolProperties properties = new PoolProperties(); + properties.setDataSource(new Object() { + @Override + public String toString() { + throw new StackOverflowError("boom"); + } + }); + + String asString = Assertions.assertDoesNotThrow(properties::toString); + + Assertions.assertTrue(asString.endsWith("]"), + "the dump must stay bracket-balanced, was: " + asString); + Assertions.assertTrue(asString.contains("dataSource=<error:"), + "the failing property must be reported in its own field, was: " + asString); + } + + @Test + void aFatalErrorFromAPropertyObjectToStringIsPropagated() { + PoolProperties properties = new PoolProperties(); + properties.setDataSource(new Object() { + @Override + public String toString() { + throw new OutOfMemoryError("pretend"); + } + }); + + Assertions.assertThrows(OutOfMemoryError.class, properties::toString, + "fatal errors are propagated, per the module's handleThrowable convention"); + } + + @Test + void aNamelessInterceptorPropertyIsRefusedAtOnce() { + NullPointerException npe = Assertions.assertThrows(NullPointerException.class, + () -> new PoolProperties.InterceptorProperty(null, "value"), + "the name keys the property in the definitions' map, so the " + + "constructor must refuse null instead of letting the " + + "map's hashing fail later"); + Assertions.assertEquals( + MessageFactory.get("poolProperties.interceptorPropertyNameRequired"), + npe.getMessage(), "the refusal must carry the localized message"); + } + + @Test + void aCloneCopiesTheDriverPropertiesCollection() throws Exception { + PoolProperties original = new PoolProperties(); + original.setConnectionProperties("keep=yes;stale=x"); + + PoolProperties clone = (PoolProperties) original.clone(); + + clone.setConnectionProperties("only=clone"); + + Assertions.assertEquals("yes", original.getDbProperties().getProperty("keep"), + "a reconfigure of the copy must not strip the original's pairs"); + Assertions.assertEquals("x", original.getDbProperties().getProperty("stale"), + "a reconfigure of the copy must not strip the original's pairs"); + Assertions.assertEquals("clone", clone.getDbProperties().getProperty("only")); + + original.setConnectionProperties(null); + + Assertions.assertEquals("clone", clone.getDbProperties().getProperty("only"), + "and the other way around: the copy keeps what it configured"); + } + + @Test + void aCloneResolvesPropertiesDefaultsLikeTheOriginal() throws Exception { + Properties defaults = new Properties(); + defaults.setProperty("socketTimeout", "1000"); + Properties dbProperties = new Properties(defaults); + dbProperties.setProperty("keep", "yes"); + + PoolProperties original = new PoolProperties(); + original.setDbProperties(dbProperties); + + PoolProperties clone = (PoolProperties) original.clone(); + + Assertions.assertEquals("1000", clone.getDbProperties().getProperty("socketTimeout"), + "a property the original resolves through its defaults table " + + "must resolve on the copy too, as it does on the connect path"); + Assertions.assertEquals("yes", clone.getDbProperties().getProperty("keep")); + } + + @Test + void aThrowingPropertyFailsOnlyItsOwnFieldOfTheDump() { + PoolProperties properties = new PoolProperties(); + properties.setUrl("jdbc:h2:mem:shown"); + properties.setDataSource(new Object() { + @Override + public String toString() { + throw new IllegalStateException("hostile dataSource"); + } + }); + + String asString = properties.toString(); + + Assertions.assertTrue(asString.endsWith("]"), + "one throwing property must not truncate the dump, was: " + asString); + Assertions.assertTrue(asString.contains("dataSource=<error:"), + "the failing property must be reported in its own field, was: " + asString); + // dataSourceJNDI is listed after dataSource, so it is lost if the + // failure still truncates the dump. + Assertions.assertTrue(asString.contains("dataSourceJNDI"), + "the properties must keep being listed after the failing one, was: " + asString); + } + @Test void toStringMasksThePasswordButStillShowsTheConfiguration() { PoolProperties properties = new PoolProperties(); @@ -309,6 +475,19 @@ class TestPoolProperties { "the JDBC URL must stay readable, was: " + asString); } + @Test + void toStringPrintsAnUnsetPasswordAsNullNotAsTheMask() { + // The mask hides a secret; it should not also pretend one is there. + // An unset password prints the way every other unset property does. + PoolProperties properties = new PoolProperties(); + properties.setUrl("jdbc:h2:mem:shown"); + + String asString = properties.toString(); + + Assertions.assertTrue(asString.contains("password=null"), + "an unset password must say null, was: " + asString); + } + @Test void toStringMasksAPasswordEmbeddedInTheConnectionProperties() { PoolProperties properties = new PoolProperties(); @@ -338,6 +517,230 @@ class TestPoolProperties { "the rest of the URL must stay readable, was: " + asString); } + @Test + void toStringMasksAPasswordInTheUrlUserinfo() { + PoolProperties properties = new PoolProperties(); + properties.setUrl("jdbc:postgresql://user:inline-secret@host:5432/db"); + + String asString = properties.toString(); + + Assertions.assertFalse(asString.contains("inline-secret"), + "the password in the URL userinfo must not leak, was: " + asString); + Assertions.assertTrue(asString.contains("url=jdbc:postgresql://user:********@host:5432/db"), + "the user, host and port must stay readable, was: " + asString); + } + + @Test + void toStringMasksAPasswordInTheLegacyOracleUserinfo() { + PoolProperties properties = new PoolProperties(); + properties.setUrl("jdbc:oracle:thin:user/inline-secret@h:1521:ORCL"); + + String asString = properties.toString(); + + Assertions.assertFalse(asString.contains("inline-secret"), + "the password in the legacy Oracle userinfo must not leak, was: " + asString); + Assertions.assertTrue(asString.contains("url=jdbc:oracle:thin:user/********@h:1521:ORCL"), + "the user and connect string must stay readable, was: " + asString); + } + + @Test + void toStringMasksALegacyOraclePasswordContainingAnAtSign() { + // An unencoded '@' inside the password can only extend the credential, + // exactly as in the scheme:// userinfo, so the credential runs to the + // last '@' of the section - masking to the first one printed the tail + // of the password in cleartext. + PoolProperties properties = new PoolProperties(); + properties.setUrl("jdbc:oracle:thin:scott/se@cret1@dbhost:1521:ORCL"); + + String asString = properties.toString(); + + Assertions.assertFalse(asString.contains("cret1"), + "the tail of the password past the '@' must not leak, was: " + asString); + Assertions.assertTrue(asString.contains("url=jdbc:oracle:thin:scott/********@dbhost:1521:ORCL"), + "the user and connect string must stay readable, was: " + asString); + } + + @Test + void toStringLeavesUrlAuthoritiesWithoutAUserinfoPasswordUntouched() { + String[] urls = { + "jdbc:mysql://host:3306/db", + "jdbc:postgresql://user@host/db", + "jdbc:sqlserver://host:1433;databaseName=x", + "jdbc:oracle:thin:@h:1521:ORCL", + // the at-sign-only forms carry no credential, whatever '@' a + // later part of the connect string may hold + "jdbc:oracle:thin:@//h:1521/svc@corp", + "jdbc:postgresql://host/[email protected]", + }; + for (String url : urls) { + PoolProperties properties = new PoolProperties(); + properties.setUrl(url); + + String asString = properties.toString(); + + Assertions.assertTrue(asString.contains("url=" + url), + "an authority without a credential separator must come out unchanged: " + + url + ", was: " + asString); + } + } + + @Test + void toStringMasksPairsSeparatedByNewLinesInTheConnectionProperties() { + // The parser that applies the string splits pairs on new lines too - a + // .properties source expands \n into a real one - so the masking has to + // honour the same segmentation or a pair the driver really gets prints + // inside the previous value. + PoolProperties properties = new PoolProperties(); + properties.setConnectionProperties("socketTimeout=1000\nuser=app\npassword=multiline-secret"); + + String asString = properties.toString(); + + Assertions.assertFalse(asString.contains("multiline-secret"), + "the pair behind a new line must not leak, was: " + asString); + Assertions.assertTrue(asString.contains("password=********"), + "the new-line separated pair must be masked, was: " + asString); + } + + @Test + void toStringMasksTheWholeDeliveredValueOfACredentialPairInTheConnectionProperties() { + // The properties parser that applies the string delivers everything up + // to the next semicolon or new line as the pair's value, so an '&' or + // '?' inside it is part of the password the driver receives - masking + // only up to it printed the tail of the real password in cleartext. + PoolProperties properties = new PoolProperties(); + properties.setConnectionProperties("timeout=5;password=pas&sW0rd;x=1"); + + String asString = properties.toString(); + + Assertions.assertFalse(asString.contains("sW0rd"), + "the tail of the delivered password past the '&' must not leak, was: " + asString); + Assertions.assertTrue(asString.contains("connectionProperties=timeout=5;password=********;x=1"), + "the pairs on either side of the credential must stay readable, was: " + asString); + } + + @Test + void toStringMasksAQueryPairInsideAUrlShapedValueOfTheConnectionProperties() { + // The properties parser delivers the URL as one value, but a driver + // reading that value as a URL splits its query at the '?', so a pair + // behind the '?' keeps its fine segmentation and the value around it + // stays readable. + PoolProperties properties = new PoolProperties(); + properties.setConnectionProperties("logUrl=http://h/db?password=queryvalue;x=1";); + + String asString = properties.toString(); + + Assertions.assertFalse(asString.contains("queryvalue"), + "the query pair of the URL-shaped value must not leak, was: " + asString); + Assertions.assertTrue(asString.contains("connectionProperties=logUrl=http://h/db?password=********;x=1";), + "the URL around the query pair must stay readable, was: " + asString); + } + + @Test + void toStringMasksEveryAuthorityInAStringNotJustTheFirst() { + PoolProperties properties = new PoolProperties(); + // A failover URL in a query value and a logging URL in a pair are as + // credential-bearing as the URL in front of them. + properties.setUrl("jdbc:db://host:5432/db?fallback=jdbc:db://backup:fallback-secret@h2:5432/db"); + properties.setConnectionProperties("ping=http://a/b;logUrl=http://admin:logging-secret@h/x";); + + String asString = properties.toString(); + + Assertions.assertFalse(asString.contains("fallback-secret"), + "the second authority of the URL must not leak, was: " + asString); + Assertions.assertFalse(asString.contains("logging-secret"), + "the second authority of the pairs must not leak, was: " + asString); + Assertions.assertTrue(asString.contains("url=jdbc:db://host:5432/db?fallback=jdbc:db://backup:********@h2:5432/db"), + "the rest of both URLs must stay readable, was: " + asString); + Assertions.assertTrue(asString.contains("connectionProperties=ping=http://a/b;logUrl=http://admin:********@h/x";), + "the rest of the pairs must stay readable, was: " + asString); + } + + @Test + void toStringMasksAQueryPairHidingBehindAnEarlierValue() { + PoolProperties properties = new PoolProperties(); + properties.setUrl("jdbc:postgresql://host/db=seg?password=question-secret"); + + String asString = properties.toString(); + + Assertions.assertFalse(asString.contains("question-secret"), + "a pair after a '?' must not hide inside the earlier value, was: " + asString); + Assertions.assertTrue(asString.contains("url=jdbc:postgresql://host/db=seg?password=********"), + "the rest of the URL must stay readable, was: " + asString); + } + + @Test + void toStringMasksCredentialPairsWrittenWithThePropertiesSeparators() { + // The parser that applies the string reads pairs whose key is followed + // by a colon or a space, so those forms really deliver a password and + // must be masked like the equals form. + PoolProperties properties = new PoolProperties(); + properties.setConnectionProperties("timeout:5;password:colon-secret;pass spaceSecret"); + + String asString = properties.toString(); + + Assertions.assertFalse(asString.contains("colon-secret"), + "the colon pair must not leak, was: " + asString); + Assertions.assertFalse(asString.contains("spaceSecret"), + "the space pair must not leak, was: " + asString); + Assertions.assertTrue(asString.contains("connectionProperties=timeout:5;password:********;pass ********"), + "the keys stay visible and the non-credential pair untouched, was: " + asString); + } + + @Test + void toStringMasksCredentialPairsWrittenWithTheFormFeedSeparator() { + // The reader the parser runs treats a form feed as white space too: + // it terminates the key and it is skipped as leading white space + // before one, so both spellings really deliver a password and must be + // masked like the equals, colon, space and tab forms. + String[][] forms = { + { "password\u000CsecretFF", "secretFF" }, + { "\u000Cpassword=secretFF2", "secretFF2" }, + }; + for (String[] form : forms) { + PoolProperties properties = new PoolProperties(); + properties.setConnectionProperties(form[0]); + + Assertions.assertEquals(form[1], properties.getDbProperties().getProperty("password"), + "the parser really delivers the value, so the dump has to mask it: " + form[0]); + + String asString = properties.toString(); + + Assertions.assertFalse(asString.contains(form[1]), + "the form feed pair must not leak, was: " + asString); + } + } + + @Test + void toStringLeavesUrlsAndValuesWithColonsAndSpacesAlone() { + String[] connectionProperties = { + // the scheme, host:port and database-name colons of URLs, + "url=jdbc:h2:mem:pass;host=localhost:5432", + // and a value that merely mentions a credential word. + "note=the password policy applies", + }; + for (String cp : connectionProperties) { + PoolProperties properties = new PoolProperties(); + properties.setConnectionProperties(cp); + + String asString = properties.toString(); + + Assertions.assertTrue(asString.contains("connectionProperties=" + cp), + "only a credential key at a segment start may be masked: " + + cp + ", was: " + asString); + } + } + + @Test + void toStringMasksPairsWrittenWithThePropertiesSeparatorsInUrlQueries() { + PoolProperties properties = new PoolProperties(); + properties.setUrl("jdbc:mysql://h:3306/db?user=me&password=colon-query-secret&x=1"); + + String asString = properties.toString(); + + Assertions.assertFalse(asString.contains("colon-query-secret"), + "the query pair must not leak, was: " + asString); + } + @Test void toStringKeepsNonCredentialPairsInsideRawStringsUntouched() { PoolProperties properties = new PoolProperties(); @@ -352,4 +755,162 @@ class TestPoolProperties { Assertions.assertTrue(asString.contains("url=jdbc:h2:mem:shown"), "a URL without an equals sign must come out unchanged, was: " + asString); } + + @Test + void toStringMasksACredentialPropertyOfAnInterceptor() { + // An interceptor takes its properties - among them any credential it + // needs - out of this very string, so the dump must mask them as it + // masks the pair forms of the other configuration strings. + PoolProperties properties = new PoolProperties(); + properties.setJdbcInterceptors("ResetAbandonedTimer;custom.MyInterceptor(" + + "user=bob,password=interceptor-secret,PWD=Shouted-Secret)"); + + String asString = properties.toString(); + + Assertions.assertFalse(asString.contains("interceptor-secret"), + "the interceptor password must not leak, was: " + asString); + Assertions.assertFalse(asString.contains("Shouted-Secret"), + "the key must be recognised ignoring case, was: " + asString); + Assertions.assertTrue(asString.contains("jdbcInterceptors=ResetAbandonedTimer;" + + "custom.MyInterceptor(user=bob,password=********,PWD=********)"), + "the class name and the other properties must stay readable, was: " + asString); + } + + @Test + void toStringMasksAnInterceptorCredentialWrittenAroundSpacesAndLeavesOtherPairsAlone() { + PoolProperties properties = new PoolProperties(); + properties.setJdbcInterceptors("SlowQueryReport(threshold=1000, Password = spaced-secret );" + + "custom.X(name=passwordish,user=pass)"); + + String asString = properties.toString(); + + Assertions.assertFalse(asString.contains("spaced-secret"), + "the name is the text before the first =, trimmed, was: " + asString); + Assertions.assertTrue(asString.contains("custom.X(name=passwordish,user=pass)"), + "only a whole property name may be masked, not a value that " + + "merely looks like a credential: " + asString); + } + + @Test + void cloneNeverCopiesATornInterceptorChain() throws Exception { + // A setter publishes a reconfiguration over several field writes under + // the configuration's monitor. A clone that skipped that monitor could + // land between the writes and copy a fresh interceptor string together + // with the previous string's parse cache - a chain the copy would then + // never re-parse. Cloning under the monitor makes the mismatch + // impossible, so this loop never observes one; it fails only if the + // synchronization is ever taken back off. + final String chainA = "org.apache.tomcat.jdbc.pool.interceptor.ResetAbandonedTimer"; + final String chainB = "org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer"; + for (int iter = 0; iter < 2000; iter++) { + final PoolProperties properties = new PoolProperties(); + properties.setJdbcInterceptors(chainA); + properties.getJdbcInterceptorsAsArray(); + final Thread reconfiguring = new Thread(() -> properties.setJdbcInterceptors(chainB)); + reconfiguring.start(); + PoolProperties copy = (PoolProperties) properties.clone(); + reconfiguring.join(); + String reported = copy.getJdbcInterceptors(); + String cached = copy.getJdbcInterceptorsAsArray()[1].getClassName(); + Assertions.assertEquals(reported, cached, + "the clone reported one interceptor string while its chain came from another"); + } + } + + @Test + void serializationNeverCarriesAnUntrackedDriverProperty() throws Exception { + // The same multi-field publish, seen from the stream: a field-by-field + // serialization that lands between the writes hands the deserialized + // copy a driver-property entry that its own tracked key set never + // removes again, so a retired credential would survive every later + // reconfigure. After a reconfigure on the copy, every entry the string + // contributed must be gone; a leftover is a torn snapshot. + StringBuilder first = new StringBuilder(), second = new StringBuilder(); + for (int i = 0; i < 40; i++) { + if (i > 0) { + first.append(';'); + second.append(';'); + } + first.append("y").append(i).append("=1"); + second.append("z").append(i).append("=2"); + } + final String keysFirst = first.toString(), keysSecond = second.toString(); + for (int iter = 0; iter < 500; iter++) { + final PoolProperties properties = new PoolProperties(); + properties.setConnectionProperties(keysFirst); + final Thread reconfiguring = new Thread(() -> properties.setConnectionProperties(keysSecond)); + reconfiguring.start(); + PoolProperties copy; + try (java.io.ByteArrayOutputStream bytes = new java.io.ByteArrayOutputStream(); + java.io.ObjectOutputStream out = new java.io.ObjectOutputStream(bytes)) { + out.writeObject(properties); + out.flush(); + try (java.io.ObjectInputStream in = + new java.io.ObjectInputStream(new java.io.ByteArrayInputStream(bytes.toByteArray()))) { + copy = (PoolProperties) in.readObject(); + } + } + reconfiguring.join(); + copy.setConnectionProperties("q=1"); + Assertions.assertEquals("1", copy.getDbProperties().getProperty("q"), + "the fresh pair must be on the reconfigured copy"); + for (String key : copy.getDbProperties().stringPropertyNames()) { + Assertions.assertFalse(key.startsWith("y") || key.startsWith("z"), + "the deserialized copy carried a pair no key set tracks: " + key); + } + } + } + + @Test + void toStringMasksAValueTheParserContinuesAcrossABacksslashLineBreak() { + // The reader deletes a new line escaped by a trailing backslash and + // joins the next line into the value, so the delivered password runs + // past the line break the dump's segmentation sees; two backslashes + // end the line and the next line really is its own pair. + PoolProperties continued = new PoolProperties(); + continued.setConnectionProperties("password=abc\\\ncont16;x=1"); + + Assertions.assertEquals("abccont16", + continued.getDbProperties().getProperty("password"), + "the parser really joins the lines"); + + String asString = continued.toString(); + + Assertions.assertFalse(asString.contains("cont16"), + "the continuation tail of the delivered password must not leak, was: " + asString); + Assertions.assertTrue(asString.contains("connectionProperties=password=********;x=1"), + "the pair after the continuation must stay readable, was: " + asString); + + PoolProperties escaped = new PoolProperties(); + escaped.setConnectionProperties("password=abc\\\\\ncont16=y"); + + Assertions.assertEquals("abc\\", escaped.getDbProperties().getProperty("password"), + "two backslashes end the line instead of continuing it"); + + asString = escaped.toString(); + + Assertions.assertTrue(asString.contains("connectionProperties=password=********\ncont16=y"), + "the next pair is really separate and must stay readable, was: " + asString); + } + + @Test + void toStringMasksAnInterceptorPropertyInAnUnclosedPropertyList() { + // The parse delivers nothing from a definition whose parenthesis + // never closes, but a pair typed inside it was typed as a credential; + // and a null string has no properties at all, so the dump must print + // it without reaching into the masker. + PoolProperties unterminated = new PoolProperties(); + unterminated.setJdbcInterceptors("A(password=unterminated-secret;B(other=y)"); + + String asString = unterminated.toString(); + + Assertions.assertFalse(asString.contains("unterminated-secret"), + "the credential typed into the open list must not leak, was: " + asString); + + PoolProperties none = new PoolProperties(); + none.setJdbcInterceptors(null); + + Assertions.assertTrue(none.toString().contains("jdbcInterceptors=null"), + "a null string must print as null, was: " + none); + } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 66250bdc5e..653babcb3d 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1478,6 +1478,16 @@ finalizer, as it already does for the analogous query timeout ordering. (remm) </fix> + <fix> + Mask the password carried in the <code>user:password@host</code> + userinfo section of a JDBC URL, and in the equivalent + <code>user/password@host</code> form of the legacy Oracle URLs, in the + <code>toString()</code> dump of the pool configuration, where the rest + of the dump is masked and the leak gave a false impression that the + dump carried no credential. The user name, host, port and the rest of + the URL stay readable, and an authority that carries no credential - + a bare host or a port - is left untouched. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
