Skip to content

Commit 0e9158e

Browse files
jgebaljgebal
committed
Resolved issue with credentials leaking into debug log.
Added tests to confirm the solution. Changed how credentials are parsed to avoid leaking passwords on unquoted passwords with multiple @ signs. Cleanup, unification and refactoring of credentials masking. Update of readme to include information about new behavior.
1 parent a3f4d5e commit 0e9158e

6 files changed

Lines changed: 153 additions & 46 deletions

File tree

‎README.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,8 @@ The directory holding `tnsnames.ora` (and `ojdbc.properties`, if used) is taken
110110

111111
Options 1-3 are handled by the Oracle JDBC driver. Option 4 is a fallback provided by utPLSQL-cli, used only when none of the others is set.
112112

113-
In case you use a username containing `/` or a password containing `@` you should encapsulate it with double quotes `"`:
113+
A password may contain `@`: everything up to the last `@` is taken as the password, e.g. `utplsql run myUser/myP@ssword@connectstring`.
114+
A username containing `/` must be enclosed in double quotes `"`, and so may the password:
114115
```
115116
utplsql run "my/Username"/"myP@ssword"@connectstring
116117
```

‎src/main/java/org/utplsql/cli/Cli.java‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@
44
import org.slf4j.LoggerFactory;
55
import picocli.CommandLine;
66

7+
import java.util.Arrays;
78
import java.util.List;
9+
import java.util.stream.Collectors;
810

911
public class Cli {
1012

@@ -19,10 +21,19 @@ public static void main(String[] args) {
1921
System.exit(exitCode);
2022
}
2123

24+
/**
25+
* @return the arguments separated by ", ", with the credentials of the connect string masked
26+
*/
27+
static String maskedArgs(String... args) {
28+
return Arrays.stream(args)
29+
.map(ConnectionConfig::maskCredentials)
30+
.collect(Collectors.joining(", "));
31+
}
32+
2233
static int runPicocliWithExitCode(String[] args) {
2334

2435

25-
logger.debug("Args: "+String.join(", ", args));
36+
logger.debug("Args: {}", maskedArgs(args));
2637

2738
CommandLine commandLine = new CommandLine(UtplsqlPicocliCommand.class);
2839
commandLine.setTrimQuotes(true);

‎src/main/java/org/utplsql/cli/ConnectionConfig.java‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,10 @@ public class ConnectionConfig {
77

88
/**
99
* Either {@code <user>/<password>@<connect>} or {@code /@<connect>}.
10+
* An unquoted password extends to the last {@code @}, so it may contain {@code @} itself.
1011
*/
1112
private static final Pattern CONNECT_STRING_PATTERN =
12-
Pattern.compile("^(?:(\".+\"|[^/]+)/(\".+\"|[^@]+)|/)@(.*)$");
13+
Pattern.compile("^(?:(\".+\"|[^/]+)/(\".+\"|.+)|/)@(.*)$");
1314

1415
private final String user;
1516
private final String password;
@@ -26,6 +27,19 @@ public ConnectionConfig(String connectString) {
2627
}
2728
}
2829

30+
/**
31+
* Masks the credentials of a connect string, e.g. for logging.
32+
*
33+
* @param value any string, e.g. a command line argument
34+
* @return the value as returned by {@link #getMaskedConnectString()} for a connect string, otherwise the unchanged value
35+
*/
36+
public static String maskCredentials(String value) {
37+
if (value == null || !CONNECT_STRING_PATTERN.matcher(value).matches()) {
38+
return value;
39+
}
40+
return new ConnectionConfig(value).getMaskedConnectString();
41+
}
42+
2943
private String stripEnclosingQuotes(String value) {
3044
if (value.length() > 1
3145
&& value.startsWith("\"")
@@ -63,6 +77,17 @@ public String getConnectString() {
6377
return user + "/" + password + "@" + connect;
6478
}
6579

80+
/**
81+
* @return the connect string with user and password replaced by asterisks,
82+
* or {@code /@<connect>} for external authentication
83+
*/
84+
public String getMaskedConnectString() {
85+
if (isExternalAuthentication()) {
86+
return "/@" + connect;
87+
}
88+
return "****/****@" + connect;
89+
}
90+
6691
public boolean isSysDba() {
6792
return user != null &&
6893
(user.toLowerCase().endsWith(" as sysdba")

‎src/main/java/org/utplsql/cli/datasource/TestedDataSourceProvider.java‎

Lines changed: 11 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -16,23 +16,18 @@
1616

1717
public class TestedDataSourceProvider {
1818

19-
interface ConnectStringPossibility {
20-
String getConnectString(ConnectionConfig config);
21-
22-
String getMaskedConnectString(ConnectionConfig config);
23-
}
24-
2519
private static final Logger logger = LoggerFactory.getLogger(TestedDataSourceProvider.class);
20+
/**
21+
* JDBC URL prefixes tried in this order: thick (OCI) driver first, then thin driver
22+
*/
23+
private static final List<String> JDBC_URL_PREFIXES = List.of("jdbc:oracle:oci8:", "jdbc:oracle:thin:");
24+
2625
private final ConnectionConfig config;
27-
private final List<ConnectStringPossibility> possibilities = new ArrayList<>();
2826
private final int maxConnections;
2927

3028
public TestedDataSourceProvider(ConnectionConfig config, int maxConnections) {
3129
this.config = config;
3230
this.maxConnections = maxConnections;
33-
34-
possibilities.add(new ThickConnectStringPossibility());
35-
possibilities.add(new ThinConnectStringPossibility());
3631
}
3732

3833
public DataSource getDataSource() throws SQLException {
@@ -55,14 +50,15 @@ private void setThickOrThinJdbcUrl(InitializableOracleDataSource ds) throws SQLE
5550
ds.setPassword(config.getPassword());
5651
}
5752

58-
for (ConnectStringPossibility possibility : possibilities) {
59-
logger.debug("Try connecting {}", possibility.getMaskedConnectString(config));
60-
ds.setURL(possibility.getConnectString(config));
53+
for (String jdbcUrlPrefix : JDBC_URL_PREFIXES) {
54+
String maskedUrl = jdbcUrlPrefix + config.getMaskedConnectString();
55+
logger.debug("Try connecting {}", maskedUrl);
56+
ds.setURL(jdbcUrlPrefix + "@" + config.getConnect());
6157
try (Connection ignored = ds.getConnection()) {
62-
logger.info("Use connection string {}", possibility.getMaskedConnectString(config));
58+
logger.info("Use connection string {}", maskedUrl);
6359
return;
6460
} catch (Error | Exception e) {
65-
errors.add(possibility.getMaskedConnectString(config) + ": " + e.getMessage());
61+
errors.add(maskedUrl + ": " + e.getMessage());
6662
lastException = e;
6763
}
6864
}
@@ -101,32 +97,4 @@ private void setInitSqlFrom_NLS_LANG(InitializableOracleDataSource ds) {
10197
}
10298
}
10399
}
104-
105-
private static class ThickConnectStringPossibility implements ConnectStringPossibility {
106-
@Override
107-
public String getConnectString(ConnectionConfig config) {
108-
return "jdbc:oracle:oci8:@" + config.getConnect();
109-
}
110-
111-
@Override
112-
public String getMaskedConnectString(ConnectionConfig config) {
113-
return "jdbc:oracle:oci8:" + maskedCredentials(config) + "@" + config.getConnect();
114-
}
115-
}
116-
117-
private static class ThinConnectStringPossibility implements ConnectStringPossibility {
118-
@Override
119-
public String getConnectString(ConnectionConfig config) {
120-
return "jdbc:oracle:thin:@" + config.getConnect();
121-
}
122-
123-
@Override
124-
public String getMaskedConnectString(ConnectionConfig config) {
125-
return "jdbc:oracle:thin:" + maskedCredentials(config) + "@" + config.getConnect();
126-
}
127-
}
128-
129-
private static String maskedCredentials(ConnectionConfig config) {
130-
return config.isExternalAuthentication() ? "/" : "****/****";
131-
}
132100
}

‎src/test/java/org/utplsql/cli/CliArgsLoggingTest.java‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
package org.utplsql.cli;
2+
3+
import ch.qos.logback.classic.Level;
4+
import ch.qos.logback.classic.Logger;
5+
import ch.qos.logback.classic.spi.ILoggingEvent;
6+
import ch.qos.logback.core.read.ListAppender;
7+
import org.junit.jupiter.api.AfterEach;
8+
import org.junit.jupiter.api.BeforeEach;
9+
import org.junit.jupiter.api.Test;
10+
import org.slf4j.LoggerFactory;
11+
12+
import java.util.List;
13+
import java.util.stream.Collectors;
14+
15+
import static org.junit.jupiter.api.Assertions.assertEquals;
16+
import static org.junit.jupiter.api.Assertions.assertFalse;
17+
import static org.junit.jupiter.api.Assertions.assertTrue;
18+
19+
/**
20+
* The command line arguments are logged at debug level; the credentials of the connect string must not be (issue #172).
21+
*/
22+
class CliArgsLoggingTest {
23+
24+
private final Logger cliLogger = (Logger) LoggerFactory.getLogger(Cli.class);
25+
private final ListAppender<ILoggingEvent> appender = new ListAppender<>();
26+
private Level originalLevel;
27+
28+
@BeforeEach
29+
void captureCliLog() {
30+
originalLevel = cliLogger.getLevel();
31+
cliLogger.setLevel(Level.DEBUG);
32+
appender.start();
33+
cliLogger.addAppender(appender);
34+
}
35+
36+
@AfterEach
37+
void restoreCliLog() {
38+
cliLogger.detachAppender(appender);
39+
cliLogger.setLevel(originalLevel);
40+
}
41+
42+
@Test
43+
void maskedArgs() {
44+
assertEquals("run, ****/****@//localhost:1521/FREEPDB1, --debug",
45+
Cli.maskedArgs("run", "app/Sup3rSecretPw@//localhost:1521/FREEPDB1", "--debug"));
46+
}
47+
48+
@Test
49+
void passwordIsNotLogged() {
50+
// "run -h" only prints the usage, so no database is needed
51+
Cli.runPicocliWithExitCode(new String[]{"run", "app/Sup3rSecretPw@//localhost:1521/FREEPDB1", "-h"});
52+
53+
List<String> messages = appender.list.stream()
54+
.map(ILoggingEvent::getFormattedMessage)
55+
.toList();
56+
57+
assertTrue(messages.contains("Args: run, ****/****@//localhost:1521/FREEPDB1, -h"), () -> "Logged: " + messages);
58+
assertFalse(messages.stream().anyMatch(message -> message.contains("Sup3rSecretPw")), () -> "Logged: " + messages);
59+
}
60+
}

‎src/test/java/org/utplsql/cli/ConnectionConfigTest.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import org.junit.jupiter.api.Test;
44
import org.junit.jupiter.params.ParameterizedTest;
5+
import org.junit.jupiter.params.provider.CsvSource;
56
import org.junit.jupiter.params.provider.ValueSource;
67

78
import static org.junit.jupiter.api.Assertions.*;
@@ -106,4 +107,45 @@ void parseExternalAuthenticationWithEzConnect() {
106107
void rejectInvalidConnectString(String connectString) {
107108
assertThrows(IllegalArgumentException.class, () -> new ConnectionConfig(connectString));
108109
}
110+
111+
@Test
112+
void parseUnquotedPasswordWithAt() {
113+
ConnectionConfig info = new ConnectionConfig("test/p@ss@w0rd@MY_TNS_ALIAS");
114+
115+
assertEquals("test", info.getUser());
116+
assertEquals("p@ss@w0rd", info.getPassword());
117+
assertEquals("MY_TNS_ALIAS", info.getConnect());
118+
}
119+
120+
@ParameterizedTest
121+
@CsvSource(delimiter = '|', value = {
122+
"test/pw@my.local.host/service | ****/****@my.local.host/service",
123+
"test/pw@//my.local.host:1521/service | ****/****@//my.local.host:1521/service",
124+
"sys as sysdba/pw@MY_TNS_ALIAS | ****/****@MY_TNS_ALIAS",
125+
"test/\"p@ssw0rd=\"@MY_TNS_ALIAS | ****/****@MY_TNS_ALIAS",
126+
"\"User/Mine@=\"/pw@MY_TNS_ALIAS | ****/****@MY_TNS_ALIAS",
127+
"test/p@ss@MY_TNS_ALIAS | ****/****@MY_TNS_ALIAS",
128+
"/@MY_TNS_ALIAS | /@MY_TNS_ALIAS"
129+
})
130+
void maskCredentials(String connectString, String expected) {
131+
assertEquals(expected, ConnectionConfig.maskCredentials(connectString));
132+
assertEquals(expected, new ConnectionConfig(connectString).getMaskedConnectString());
133+
}
134+
135+
@ParameterizedTest
136+
@ValueSource(strings = {
137+
"run",
138+
"--debug",
139+
"-p=app.test_pkg",
140+
"-f=ut_documentation_reporter",
141+
"MY_TNS_ALIAS"
142+
})
143+
void maskCredentialsLeavesOtherValuesUnchanged(String value) {
144+
assertEquals(value, ConnectionConfig.maskCredentials(value));
145+
}
146+
147+
@Test
148+
void maskCredentialsOfNull() {
149+
assertNull(ConnectionConfig.maskCredentials(null));
150+
}
109151
}

0 commit comments

Comments
 (0)