From d662a4853e0b70c6be1985407e53e80382e4aa03 Mon Sep 17 00:00:00 2001 From: Ankit Tiwari Date: Tue, 6 Feb 2024 11:00:22 +0530 Subject: [PATCH] fix: remove db password from logs (#89) * fix: remove db password from logs * fix: mask db password * fix: Add tests * fix: Add more tests * fix: PR changes --- CHANGELOG.md | 4 + build.gradle | 2 +- .../storage/mysql/config/MySQLConfig.java | 14 +- .../storage/mysql/output/CustomLayout.java | 4 +- .../storage/mysql/output/Logging.java | 11 +- .../storage/mysql/utils/Utils.java | 17 ++ .../storage/mysql/test/LoggingTest.java | 275 ++++++++++++++++++ 7 files changed, 316 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5153bd7..11f48e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +## [5.0.6] - 2024-01-25 + +- Fixes the issue where passwords were inadvertently logged in the logs. + ## [5.0.5] - 2023-12-06 - Validates db config types in `canBeUsed` function diff --git a/build.gradle b/build.gradle index 85e59dd..ab2184a 100644 --- a/build.gradle +++ b/build.gradle @@ -2,7 +2,7 @@ plugins { id 'java-library' } -version = "5.0.5" +version = "5.0.6" repositories { mavenCentral() diff --git a/src/main/java/io/supertokens/storage/mysql/config/MySQLConfig.java b/src/main/java/io/supertokens/storage/mysql/config/MySQLConfig.java index 1fc14f1..ad5d92a 100644 --- a/src/main/java/io/supertokens/storage/mysql/config/MySQLConfig.java +++ b/src/main/java/io/supertokens/storage/mysql/config/MySQLConfig.java @@ -509,10 +509,18 @@ public String getConnectionPoolId() { StringBuilder connectionPoolId = new StringBuilder(); for (Field field : MySQLConfig.class.getDeclaredFields()) { if (field.isAnnotationPresent(ConnectionPoolProperty.class)) { - connectionPoolId.append("|"); try { - if (field.get(this) != null) { - connectionPoolId.append(field.get(this).toString()); + String fieldName = field.getName(); + String fieldValue = field.get(this) != null ? field.get(this).toString() : null; + if(fieldValue == null) { + continue; + } + // To ensure a unique connectionPoolId we include the database password and use the "|db_pass|" identifier. + // This facilitates easy removal of the password from logs when necessary. + if (fieldName.equals("mysql_password")) { + connectionPoolId.append("|db_pass|" + fieldValue + "|db_pass"); + } else { + connectionPoolId.append("|" + fieldValue); } } catch (IllegalAccessException e) { throw new RuntimeException(e); diff --git a/src/main/java/io/supertokens/storage/mysql/output/CustomLayout.java b/src/main/java/io/supertokens/storage/mysql/output/CustomLayout.java index 27f1e4c..74dca68 100644 --- a/src/main/java/io/supertokens/storage/mysql/output/CustomLayout.java +++ b/src/main/java/io/supertokens/storage/mysql/output/CustomLayout.java @@ -20,7 +20,7 @@ import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.core.CoreConstants; import ch.qos.logback.core.LayoutBase; -import io.supertokens.storage.mysql.Start; +import io.supertokens.storage.mysql.utils.Utils; import java.text.DateFormat; import java.text.SimpleDateFormat; @@ -58,7 +58,7 @@ public String doLayout(ILoggingEvent event) { sbuf.append(event.getCallerData()[1]); sbuf.append(" | "); - sbuf.append(event.getFormattedMessage()); + sbuf.append(Utils.maskDBPassword(event.getFormattedMessage())); sbuf.append(CoreConstants.LINE_SEPARATOR); sbuf.append(CoreConstants.LINE_SEPARATOR); diff --git a/src/main/java/io/supertokens/storage/mysql/output/Logging.java b/src/main/java/io/supertokens/storage/mysql/output/Logging.java index 531d76e..084f1a6 100644 --- a/src/main/java/io/supertokens/storage/mysql/output/Logging.java +++ b/src/main/java/io/supertokens/storage/mysql/output/Logging.java @@ -37,10 +37,10 @@ public class Logging extends ResourceDistributor.SingletonResource { private Logging(Start start, String infoLogPath, String errorLogPath) { this.infoLogger = infoLogPath.equals("null") - ? createLoggerForConsole(start, "io.supertokens.storage.mysql.Info") + ? createLoggerForConsole(start, "io.supertokens.storage.mysql.Info", LOG_LEVEL.INFO) : createLoggerForFile(start, infoLogPath, "io.supertokens.storage.mysql.Info"); this.errorLogger = errorLogPath.equals("null") - ? createLoggerForConsole(start, "io.supertokens.storage.mysql.Error") + ? createLoggerForConsole(start, "io.supertokens.storage.mysql.Error", LOG_LEVEL.ERROR) : createLoggerForFile(start, errorLogPath, "io.supertokens.storage.mysql.Error"); } @@ -154,12 +154,12 @@ public static void error(Start start, String message, boolean toConsoleAsWell, E private static void systemOut(String msg) { if (!Start.silent) { - System.out.println(msg); + System.out.println(Utils.maskDBPassword(msg)); } } private static void systemErr(String err) { - System.err.println(err); + System.err.println(Utils.maskDBPassword(err)); } public static void stopLogging(Start start) { @@ -198,7 +198,7 @@ private Logger createLoggerForFile(Start start, String file, String name) { return logger; } - private Logger createLoggerForConsole(Start start, String name) { + private Logger createLoggerForConsole(Start start, String name, LOG_LEVEL logLevel) { Logger logger = (Logger) LoggerFactory.getLogger(name); // We don't need to add appender if it is already added @@ -211,6 +211,7 @@ private Logger createLoggerForConsole(Start start, String name) { ple.setContext(lc); ple.start(); ConsoleAppender logConsoleAppender = new ConsoleAppender<>(); + logConsoleAppender.setTarget(logLevel == LOG_LEVEL.ERROR ? "System.err" : "System.out"); logConsoleAppender.setEncoder(ple); logConsoleAppender.setContext(lc); logConsoleAppender.start(); diff --git a/src/main/java/io/supertokens/storage/mysql/utils/Utils.java b/src/main/java/io/supertokens/storage/mysql/utils/Utils.java index 9dcb1f9..e68d6dd 100644 --- a/src/main/java/io/supertokens/storage/mysql/utils/Utils.java +++ b/src/main/java/io/supertokens/storage/mysql/utils/Utils.java @@ -19,6 +19,8 @@ import java.io.ByteArrayOutputStream; import java.io.PrintStream; +import java.util.regex.Matcher; +import java.util.regex.Pattern; public class Utils { public static String exceptionStacktraceToString(Exception e) { @@ -39,4 +41,19 @@ public static String generateCommaSeperatedQuestionMarks(int size) { } return builder.toString(); } + + public static String maskDBPassword(String log) { + String regex = "(\\|db_pass\\|)(.*?)(\\|db_pass\\|)"; + + Matcher matcher = Pattern.compile(regex).matcher(log); + StringBuffer maskedLog = new StringBuffer(); + + while (matcher.find()) { + String maskedPassword = "*".repeat(8); + matcher.appendReplacement(maskedLog, "|" + maskedPassword + "|"); + } + + matcher.appendTail(maskedLog); + return maskedLog.toString(); + } } diff --git a/src/test/java/io/supertokens/storage/mysql/test/LoggingTest.java b/src/test/java/io/supertokens/storage/mysql/test/LoggingTest.java index 59a7372..52ab2c0 100644 --- a/src/test/java/io/supertokens/storage/mysql/test/LoggingTest.java +++ b/src/test/java/io/supertokens/storage/mysql/test/LoggingTest.java @@ -21,6 +21,8 @@ import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.core.Appender; import com.google.gson.JsonObject; + +import io.supertokens.Main; import io.supertokens.ProcessState; import io.supertokens.config.Config; import io.supertokens.featureflag.EE_FEATURES; @@ -28,6 +30,7 @@ import io.supertokens.multitenancy.Multitenancy; import io.supertokens.pluginInterface.multitenancy.*; import io.supertokens.storage.mysql.Start; +import io.supertokens.storage.mysql.config.MySQLConfig; import io.supertokens.storage.mysql.output.Logging; import io.supertokens.storageLayer.StorageLayer; import org.apache.tomcat.util.http.fileupload.FileUtils; @@ -308,6 +311,278 @@ public void confirmHikariLoggerClosedOnlyWhenProcessEnds() throws Exception { assertFalse(hikariLogger.iteratorForAppenders().hasNext()); } + @Test + public void testDBPasswordMaskingOnDBConnectionFailUsingConnectionUri() throws Exception { + String[] args = { "../" }; + + String dbUser = "db_user"; + String dbPassword = "db_password"; + String dbName = "db_does_not_exist"; + String dbConnectionUri = "mysql://" + dbUser + ":" + dbPassword + "@localhost:3306/" + dbName; + + Utils.setValueInConfig("mysql_connection_uri", dbConnectionUri); + Utils.commentConfigValue("mysql_user"); + Utils.commentConfigValue("mysql_password"); + Utils.setValueInConfig("error_log_path", "null"); + + ByteArrayOutputStream errorOutput = new ByteArrayOutputStream(); + System.setErr(new PrintStream(errorOutput)); + + TestingProcessManager.TestingProcess process = TestingProcessManager.start(args, false); + try { + process.startProcess(); + process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.INIT_FAILURE); + + assertTrue(fileContainsString(errorOutput, dbUser)); + assertTrue(fileContainsString(errorOutput, dbName)); + assertTrue(fileContainsString(errorOutput, "********")); + assertFalse(fileContainsString(errorOutput, dbPassword)); + + } finally { + process.kill(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED)); + System.setErr(new PrintStream(new FileOutputStream(FileDescriptor.err))); + } + } + + @Test + public void testDBPasswordMaskingOnDBConnectionFailUsingCredentials() throws Exception { + String[] args = { "../" }; + + String dbUser = "db_user"; + String dbPassword = "db_password"; + String dbName = "db_does_not_exist"; + + Utils.commentConfigValue("mysql_connection_uri"); + Utils.setValueInConfig("mysql_user", dbUser); + Utils.setValueInConfig("mysql_password", dbPassword); + Utils.setValueInConfig("mysql_database_name", dbName); + Utils.setValueInConfig("error_log_path", "null"); + + ByteArrayOutputStream errorOutput = new ByteArrayOutputStream(); + System.setErr(new PrintStream(errorOutput)); + + TestingProcessManager.TestingProcess process = TestingProcessManager.start(args, false); + try { + process.startProcess(); + process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.INIT_FAILURE); + + assertTrue(fileContainsString(errorOutput, dbUser)); + assertTrue(fileContainsString(errorOutput, dbName)); + assertTrue(fileContainsString(errorOutput, "********")); + assertFalse(fileContainsString(errorOutput, dbPassword)); + + } finally { + process.kill(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED)); + System.setErr(new PrintStream(new FileOutputStream(FileDescriptor.err))); + } + } + + @Test + public void testDBPasswordMasking() throws Exception { + String[] args = { "../" }; + + ByteArrayOutputStream stdOutput = new ByteArrayOutputStream(); + ByteArrayOutputStream errorOutput = new ByteArrayOutputStream(); + + Utils.setValueInConfig("info_log_path", "null"); + Utils.setValueInConfig("error_log_path", "null"); + + System.setOut(new PrintStream(stdOutput)); + System.setErr(new PrintStream(errorOutput)); + + TestingProcessManager.TestingProcess process = TestingProcessManager.start(args, false); + + try { + process.startProcess(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STARTED)); + + Logging.info((Start) StorageLayer.getStorage(process.getProcess()), "INFO LOG: |db_pass|password|db_pass|", + false); + Logging.error((Start) StorageLayer.getStorage(process.getProcess()), + "ERROR LOG: |db_pass|password|db_pass|", false); + + assertTrue(fileContainsString(stdOutput, "INFO LOG: |********|")); + assertTrue(fileContainsString(errorOutput, "ERROR LOG: |********|")); + + } finally { + process.kill(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED)); + System.setOut(new PrintStream(new FileOutputStream(FileDescriptor.out))); + System.setErr(new PrintStream(new FileOutputStream(FileDescriptor.err))); + } + } + + @Test + public void testDBPasswordIsNotLoggedWhenProcessStartsEnds() throws Exception { + String[] args = { "../" }; + + Utils.setValueInConfig("error_log_path", "null"); + Utils.setValueInConfig("info_log_path", "null"); + + ByteArrayOutputStream stdOutput = new ByteArrayOutputStream(); + ByteArrayOutputStream errorOutput = new ByteArrayOutputStream(); + + System.setOut(new PrintStream(stdOutput)); + System.setErr(new PrintStream(errorOutput)); + + try { + // Case 1: DB Password shouldn't be logged after starting/stopping the process with correct credentials + { + TestingProcessManager.TestingProcess process = TestingProcessManager.start(args); + + process.startProcess(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STARTED)); + + Start start = (Start) StorageLayer.getStorage(process.getProcess()); + MySQLConfig userConfig = io.supertokens.storage.mysql.config.Config.getConfig(start); + String dbPasswordFromConfig = userConfig.getPassword(); + + process.kill(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED)); + + assertFalse(fileContainsString(stdOutput, dbPasswordFromConfig)); + assertFalse(fileContainsString(errorOutput, dbPasswordFromConfig)); + } + + // Case 2: DB Password shouldn't be logged after starting/stopping the process with incorrect credentials + { + String dbUser = "db_user"; + String dbPassword = "db_password"; + String dbName = "db_does_not_exist"; + String dbConnectionUri = "mysql://" + dbUser + ":" + dbPassword + "@localhost:3306/" + dbName; + + Utils.setValueInConfig("mysql_connection_uri", dbConnectionUri); + + TestingProcessManager.TestingProcess process = TestingProcessManager.start(args); + + process.startProcess(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.INIT_FAILURE)); + + Start start = (Start) StorageLayer.getStorage(process.getProcess()); + MySQLConfig userConfig = io.supertokens.storage.mysql.config.Config.getConfig(start); + String dbPasswordFromConfig = userConfig.getPassword(); + + process.kill(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED)); + + assertFalse(fileContainsString(stdOutput, dbPasswordFromConfig)); + assertFalse(fileContainsString(errorOutput, dbPasswordFromConfig)); + } + + } finally { + System.setOut(new PrintStream(new FileOutputStream(FileDescriptor.out))); + System.setErr(new PrintStream(new FileOutputStream(FileDescriptor.err))); + } + } + + @Test + public void testDBPasswordIsNotLoggedWhenTenantIsCreated() throws Exception { + String[] args = { "../" }; + + Utils.setValueInConfig("error_log_path", "null"); + Utils.setValueInConfig("info_log_path", "null"); + + ByteArrayOutputStream stdOutput = new ByteArrayOutputStream(); + ByteArrayOutputStream errorOutput = new ByteArrayOutputStream(); + + System.setOut(new PrintStream(stdOutput)); + System.setErr(new PrintStream(errorOutput)); + + try { + // Case 1: DB Password shouldn't be logged when tenant is created with valid credentials + { + TestingProcessManager.TestingProcess process = TestingProcessManager.start(args); + + process.startProcess(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STARTED)); + + Start start = (Start) StorageLayer.getStorage(process.getProcess()); + MySQLConfig userConfig = io.supertokens.storage.mysql.config.Config.getConfig(start);; + String dbPasswordFromConfig = userConfig.getPassword(); + + Main main = process.getProcess(); + + FeatureFlagTestContent.getInstance(main) + .setKeyValue(FeatureFlagTestContent.ENABLED_FEATURES, new EE_FEATURES[] { + EE_FEATURES.ACCOUNT_LINKING, EE_FEATURES.MULTI_TENANCY }); + + JsonObject config = new JsonObject(); + TenantIdentifier tenantIdentifier = new TenantIdentifier(null, "a1", null); + StorageLayer.getBaseStorage(process.getProcess()).modifyConfigToAddANewUserPoolForTesting(config, 1); + Multitenancy.addNewOrUpdateAppOrTenant( + main, + new TenantIdentifier(null, null, null), + new TenantConfig( + tenantIdentifier, + new EmailPasswordConfig(true), + new ThirdPartyConfig(true, null), + new PasswordlessConfig(true), + config + )); + + process.kill(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED)); + + assertFalse(fileContainsString(stdOutput, dbPasswordFromConfig)); + assertFalse(fileContainsString(errorOutput, dbPasswordFromConfig)); + } + + // Case 2: DB Password shouldn't be logged when tenant is created with invalid credentials + { + String dbUser = "db_user"; + String dbPassword = "db_password"; + String dbName = "db_does_not_exist"; + String dbConnectionUri = "mysql://" + dbUser + ":" + dbPassword + "@localhost:3306/" + dbName; + + TestingProcessManager.TestingProcess process = TestingProcessManager.start(args); + + process.startProcess(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STARTED)); + + Start start = (Start) StorageLayer.getStorage(process.getProcess()); + MySQLConfig userConfig = io.supertokens.storage.mysql.config.Config.getConfig(start); + String dbPasswordFromConfig = userConfig.getPassword(); + + Main main = process.getProcess(); + + FeatureFlagTestContent.getInstance(main) + .setKeyValue(FeatureFlagTestContent.ENABLED_FEATURES, new EE_FEATURES[] { + EE_FEATURES.ACCOUNT_LINKING, EE_FEATURES.MULTI_TENANCY }); + + TenantIdentifier tenantIdentifier = new TenantIdentifier(null, "a1", null); + JsonObject config = new JsonObject(); + config.addProperty("mysql_connection_uri", dbConnectionUri); + StorageLayer.getBaseStorage(process.getProcess()).modifyConfigToAddANewUserPoolForTesting(config, 1); + try { + Multitenancy.addNewOrUpdateAppOrTenant( + main, + new TenantIdentifier(null, null, null), + new TenantConfig( + tenantIdentifier, + new EmailPasswordConfig(true), + new ThirdPartyConfig(true, null), + new PasswordlessConfig(true), + new JsonObject())); + + } catch (Exception e) { + + } + + process.kill(); + assertNotNull(process.checkOrWaitForEvent(ProcessState.PROCESS_STATE.STOPPED)); + + assertFalse(fileContainsString(stdOutput, dbPasswordFromConfig)); + assertFalse(fileContainsString(errorOutput, dbPasswordFromConfig)); + } + + } finally { + System.setOut(new PrintStream(new FileOutputStream(FileDescriptor.out))); + System.setErr(new PrintStream(new FileOutputStream(FileDescriptor.err))); + } + } + private static int countAppenders(ch.qos.logback.classic.Logger logger) { int count = 0; Iterator> appenderIter = logger.iteratorForAppenders();