Skip to content

Commit 63f91e1

Browse files
jgebaljgebal
authored
Merge pull request #232 from utPLSQL/feature/prevent_logging_credentials
 Don't reveal credentials in log and error output
2 parents a3f4d5e + 0e9158e commit 63f91e1

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)