From bc6d2243dd52bc055482ea6722f55a2530edc8ac Mon Sep 17 00:00:00 2001 From: ptiurin Date: Thu, 19 Sep 2024 16:47:49 +0100 Subject: [PATCH] fix(FIR-35270): Avoid printing secrets in logs --- .../jdbc/statement/FireboltStatement.java | 12 +++- .../jdbc/statement/FireboltStatementTest.java | 65 +++++++++++++------ 2 files changed, 55 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/firebolt/jdbc/statement/FireboltStatement.java b/src/main/java/com/firebolt/jdbc/statement/FireboltStatement.java index be2f4994..3e2a3554 100644 --- a/src/main/java/com/firebolt/jdbc/statement/FireboltStatement.java +++ b/src/main/java/com/firebolt/jdbc/statement/FireboltStatement.java @@ -108,9 +108,8 @@ private Optional 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); @@ -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=***"); + } } diff --git a/src/test/java/com/firebolt/jdbc/statement/FireboltStatementTest.java b/src/test/java/com/firebolt/jdbc/statement/FireboltStatementTest.java index b42a68b0..30068fa2 100644 --- a/src/test/java/com/firebolt/jdbc/statement/FireboltStatementTest.java +++ b/src/test/java/com/firebolt/jdbc/statement/FireboltStatementTest.java @@ -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; @@ -241,23 +244,13 @@ void test() throws SQLException { FireboltConnection connection = mock(FireboltConnection.class); FireboltStatement fireboltStatement = new FireboltStatement(fireboltStatementService, fireboltProperties, connection); - List 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 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 @@ -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 logsList = listAppender.list; + assertTrue(logsList.stream().anyMatch(event -> event.getFormattedMessage().contains("Aborted query with id other label"))); } @@ -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 listAppender = new ListAppender<>(); + listAppender.start(); + + fooLogger.addAppender(listAppender); + + FireboltConnection connection = mock(FireboltConnection.class); + FireboltStatement fireboltStatement = new FireboltStatement(fireboltStatementService, fireboltProperties, + connection); + fireboltStatement.execute(query); + + List 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"))); + } } \ No newline at end of file