Skip to content

Commit

Permalink
fix(FIR-35270): Avoid printing secrets in logs
Browse files Browse the repository at this point in the history
  • Loading branch information
@ptiurin
ptiurin committed Sep 19, 2024
1 parent b97c8bb commit bc6d224
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 22 deletions.
12 changes: 9 additions & 3 deletions src/main/java/com/firebolt/jdbc/statement/FireboltStatement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,8 @@ private Optional<ResultSet> execute(StatementInfoWrapper statementInfoWrapper) t
}
InputStream inputStream = null;
try {

log.info("Executing the statement with label {} : {}", statementInfoWrapper.getLabel(),
statementInfoWrapper.getSql());
log.debug("Executing the statement with label {} : {}", statementInfoWrapper.getLabel(),
sanitizeSql(statementInfoWrapper.getSql()));
if (statementInfoWrapper.getType() == StatementType.PARAM_SETTING) {
connection.addProperty(statementInfoWrapper.getParam());
log.debug("The property from the query {} was stored", runningStatementLabel);
Expand Down Expand Up @@ -519,4 +518,11 @@ public void setPoolable(boolean poolable) throws SQLException {
public boolean hasMoreResults() {
return currentStatementResult.getNext() != null;
}

private String sanitizeSql(String sql) {
// Replace any occurrence of secrets with ***
return sql.replaceAll("AWS_KEY_ID\\s*=\\s*[\\S]*", "AWS_KEY_ID=***")
.replaceAll("AWS_SECRET_KEY\\s*=\\s*[\\S]*",
"AWS_SECRET_KEY=***");
}
}
65 changes: 46 additions & 19 deletions src/test/java/com/firebolt/jdbc/statement/FireboltStatementTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,13 @@
import java.util.Map;
import java.util.Optional;
import java.util.logging.Handler;
import java.util.logging.Level;
import java.util.logging.LogRecord;
import java.util.logging.Logger;
import java.util.stream.Stream;
import org.slf4j.LoggerFactory;
import ch.qos.logback.classic.Level;
import ch.qos.logback.classic.Logger;
import ch.qos.logback.classic.spi.ILoggingEvent;
import ch.qos.logback.core.read.ListAppender;

import static java.lang.String.format;
import static java.sql.Statement.CLOSE_CURRENT_RESULT;
Expand Down Expand Up @@ -241,23 +244,13 @@ void test() throws SQLException {
FireboltConnection connection = mock(FireboltConnection.class);
FireboltStatement fireboltStatement = new FireboltStatement(fireboltStatementService, fireboltProperties, connection);

List<String> logMessages = new ArrayList<>();
Logger log = Logger.getLogger(FireboltStatement.class.getName());
log.setLevel(Level.ALL);
log.addHandler(new Handler() {
@Override
public void publish(LogRecord record) {
logMessages.add(new MessageFormat(record.getMessage()).format(record.getParameters()));
}
Logger fooLogger = (Logger) LoggerFactory.getLogger(FireboltStatement.class);

@Override
public void flush() {
}
fooLogger.setLevel(Level.ALL);
ListAppender<ILoggingEvent> listAppender = new ListAppender<>();
listAppender.start();

@Override
public void close() throws SecurityException {
}
});
fooLogger.addAppender(listAppender);

String query = "SELECT 1";
// This trick simulates cancelled statement. getLabel() called first time returns the initial label generated
Expand All @@ -281,8 +274,9 @@ public String getLabel() {
assertNull(fireboltStatement.getResultSet());
fireboltStatement.getMoreResults(CLOSE_CURRENT_RESULT);
verify(fireboltStatementService, times(0)).execute(any(), any(), any());
// TODO: fix logging here, since we've reverted logging to lombok it doesn't catch log messages properly anymore
// assertTrue(logMessages.contains("Aborted query with id other label"), "Expected log message is not found");

List<ILoggingEvent> logsList = listAppender.list;
assertTrue(logsList.stream().anyMatch(event -> event.getFormattedMessage().contains("Aborted query with id other label")));
}


Expand Down Expand Up @@ -579,4 +573,37 @@ void shouldClearBatch() throws SQLException {

assertArrayEquals(new int[] {SUCCESS_NO_INFO, SUCCESS_NO_INFO}, fireboltStatement.executeBatch());
}

@ParameterizedTest
@CsvSource(value = {
"CREATE EXTERNAL TABLE x WITH CREDENTIALS = (AWS_KEY_ID='123', AWS_SECRET_KEY='4%5-6');",
"CREATE EXTERNAL TABLE x WITH CREDENTIALS = (AWS_KEY_ID ='123', AWS_SECRET_KEY= '4%5-6');",
"CREATE EXTERNAL TABLE x WITH CREDENTIALS = (AWS_KEY_ID = '123', AWS_SECRET_KEY = '4%5-6');",
"COPY INTO x FROM s3 WITH CREDENTIALS = (AWS_KEY_ID = '123' AWS_SECRET_KEY = '4%5-6');",
"COPY INTO x FROM s3 WITH CREDENTIALS = (AWS_KEY_ID = '123', AWS_SECRET_KEY = '4%5-6') ERROR_FILE_CREDENTIALS = AWS_KEY_ID = '123' AWS_SECRET_KEY = '4%5-6';",
"COPY INTO x FROM s3 WITH ERROR_FILE_CREDENTIALS = AWS_KEY_ID = '123' AWS_SECRET_KEY = '4%5-6';"
}, delimiter = ';')
void shouldNotOutputAWSKeyIdInDebugLogs(String query) throws SQLException {

Logger fooLogger = (Logger) LoggerFactory.getLogger(FireboltStatement.class);

fooLogger.setLevel(Level.ALL);
ListAppender<ILoggingEvent> listAppender = new ListAppender<>();
listAppender.start();

fooLogger.addAppender(listAppender);

FireboltConnection connection = mock(FireboltConnection.class);
FireboltStatement fireboltStatement = new FireboltStatement(fireboltStatementService, fireboltProperties,
connection);
fireboltStatement.execute(query);

List<ILoggingEvent> logsList = listAppender.list;
assertTrue(logsList.stream().noneMatch(event -> event.getFormattedMessage().contains("123")));
assertTrue(logsList.stream().noneMatch(event -> event.getFormattedMessage().contains("4%5-6")));
// Make sure the query is not lost
assertTrue(logsList.stream().anyMatch(event -> event.getFormattedMessage().contains("WITH")));
assertTrue(logsList.stream().anyMatch(event -> event.getFormattedMessage().contains("AWS_KEY_ID")));
assertTrue(logsList.stream().anyMatch(event -> event.getFormattedMessage().contains("AWS_SECRET_KEY")));
}
}

0 comments on commit bc6d224

Please sign in to comment.