From f39eff42290bfda27edb46d2cef0181f12ee52c3 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-nl <143542970+sfc-gh-ext-simba-nl@users.noreply.github.com> Date: Tue, 2 Jul 2024 00:00:12 -0700 Subject: [PATCH] SNOW-957747: Easy logging improvements (#1730) --- .../client/config/SFClientConfigParser.java | 40 ++++++- .../jdbc/DefaultSFConnectionHandler.java | 110 +++++++++++++++--- .../net/snowflake/client/RunningOnWin.java | 9 ++ .../client/config/SFPermissionsTest.java | 86 ++++++++++++++ .../log/JDK14LoggerWithClientLatestIT.java | 49 ++++++++ 5 files changed, 276 insertions(+), 18 deletions(-) create mode 100644 src/test/java/net/snowflake/client/RunningOnWin.java create mode 100644 src/test/java/net/snowflake/client/config/SFPermissionsTest.java diff --git a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java index 3c960bc98..a0ca0fa11 100644 --- a/src/main/java/net/snowflake/client/config/SFClientConfigParser.java +++ b/src/main/java/net/snowflake/client/config/SFClientConfigParser.java @@ -8,9 +8,11 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Paths; +import java.nio.file.attribute.PosixFilePermission; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; +import net.snowflake.client.core.Constants; import net.snowflake.client.jdbc.SnowflakeDriver; import net.snowflake.client.log.SFLogger; import net.snowflake.client.log.SFLoggerFactory; @@ -26,14 +28,16 @@ public class SFClientConfigParser { * connection property. 2. Environment variable: SF_CLIENT_CONFIG_FILE containing full path to * sf_client_config file. 3. Searches for default config file name(sf_client_config.json under the * driver directory from where the driver gets loaded. 4. Searches for default config file - * name(sf_client_config.json) under user home directory 5. Searches for default config file - * name(sf_client_config.json) under tmp directory. + * name(sf_client_config.json) under user home directory. * * @param configFilePath SF_CLIENT_CONFIG_FILE parameter read from connection URL or connection * properties * @return SFClientConfig */ public static SFClientConfig loadSFClientConfig(String configFilePath) throws IOException { + if (configFilePath != null) { + logger.info("Attempting to enable easy logging with file path {}", configFilePath); + } String derivedConfigFilePath = null; if (configFilePath != null && !configFilePath.isEmpty()) { // 1. Try to read the file at configFilePath. @@ -63,9 +67,16 @@ public static SFClientConfig loadSFClientConfig(String configFilePath) throws IO } if (derivedConfigFilePath != null) { try { + checkConfigFilePermissions(derivedConfigFilePath); + File configFile = new File(derivedConfigFilePath); ObjectMapper objectMapper = new ObjectMapper(); SFClientConfig clientConfig = objectMapper.readValue(configFile, SFClientConfig.class); + logger.info( + "Reading values logLevel {} and logPath {} from client configuration", + clientConfig.getCommonProps().getLogLevel(), + clientConfig.getCommonProps().getLogPath()); + Set unknownParams = clientConfig.getUnknownParamKeys(); if (!unknownParams.isEmpty()) { for (String unknownParam : unknownParams) { @@ -111,6 +122,31 @@ && systemGetProperty("os.name").toLowerCase().startsWith("windows")) { } } + private static void checkConfigFilePermissions(String derivedConfigFilePath) throws IOException { + try { + if (Constants.getOS() != Constants.OS.WINDOWS) { + // Check permissions of config file + if (checkGroupOthersWritePermissions(derivedConfigFilePath)) { + String error = + String.format( + "Error due to other users having permission to modify the config file: %s", + derivedConfigFilePath); + // TODO: SNOW-1503722 to change warning log to throw an error instead + logger.warn(error); + } + } + } catch (IOException e) { + throw e; + } + } + + static Boolean checkGroupOthersWritePermissions(String configFilePath) throws IOException { + Set folderPermissions = + Files.getPosixFilePermissions(Paths.get(configFilePath)); + return folderPermissions.contains(PosixFilePermission.GROUP_WRITE) + || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE); + } + static String convertToWindowsPath(String filePath) { // Find the Windows file path pattern: ex) C:\ or D:\ Pattern windowsFilePattern = Pattern.compile("[C-Z]:[\\\\/]"); diff --git a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java index 7ada3a803..6bb62c82f 100644 --- a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java +++ b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java @@ -8,15 +8,19 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.nio.file.attribute.PosixFilePermission; +import java.nio.file.attribute.PosixFilePermissions; import java.sql.ResultSet; import java.sql.SQLException; import java.sql.SQLNonTransientConnectionException; import java.sql.Statement; import java.util.Map; import java.util.Properties; +import java.util.Set; import java.util.logging.Level; import net.snowflake.client.config.SFClientConfig; import net.snowflake.client.config.SFClientConfigParser; +import net.snowflake.client.core.Constants; import net.snowflake.client.core.SFBaseResultSet; import net.snowflake.client.core.SFBaseSession; import net.snowflake.client.core.SFBaseStatement; @@ -136,13 +140,16 @@ private void setClientConfig() throws SnowflakeSQLLoggedException { String clientConfigFilePath = (String) connectionPropertiesMap.getOrDefault(SFSessionProperty.CLIENT_CONFIG_FILE, null); - SFClientConfig sfClientConfig; - try { - sfClientConfig = SFClientConfigParser.loadSFClientConfig(clientConfigFilePath); - } catch (IOException e) { - throw new SnowflakeSQLLoggedException(sfSession, ErrorCode.INTERNAL_ERROR, e.getMessage()); + SFClientConfig sfClientConfig = sfSession.getSfClientConfig(); + if (sfClientConfig == null) { + try { + sfClientConfig = SFClientConfigParser.loadSFClientConfig(clientConfigFilePath); + } catch (IOException e) { + throw new SnowflakeSQLLoggedException( + sfSession, ErrorCode.INTERNAL_ERROR, e.getMessage(), e.getCause()); + } + sfSession.setSfClientConfig(sfClientConfig); } - sfSession.setSfClientConfig(sfClientConfig); } /** @@ -181,6 +188,7 @@ && systemGetProperty("java.util.logging.config.file") == null) { if (logLevel != null && logPattern != null) { try { + logger.info("Setting logger with log level {} and log pattern {}", logLevel, logPattern); JDK14Logger.instantiateLogger(logLevel, logPattern); } catch (IOException ex) { throw new SnowflakeSQLLoggedException( @@ -188,13 +196,10 @@ && systemGetProperty("java.util.logging.config.file") == null) { } if (sfClientConfig != null) { logger.debug( - String.format( - "SF Client config found at location: %s.", sfClientConfig.getConfigFilePath())); + "SF Client config found at location: {}.", sfClientConfig.getConfigFilePath()); } logger.debug( - String.format( - "Instantiating JDK14Logger with level: %s , output path: %s", - logLevel, logPattern)); + "Instantiating JDK14Logger with level: {}, output path: {}", logLevel, logPattern); } } } @@ -206,25 +211,98 @@ private String constructLogPattern(String logPathFromConfig) throws SnowflakeSQL String logPattern = "%t/snowflake_jdbc%u.log"; // java.tmpdir + Path logPath; if (logPathFromConfig != null && !logPathFromConfig.isEmpty()) { - Path path = Paths.get(logPathFromConfig, "jdbc"); - if (!Files.exists(path)) { + // Get log path from configuration + logPath = Paths.get(logPathFromConfig); + if (!Files.exists(logPath)) { try { - Files.createDirectories(path); + Files.createDirectories(logPath); } catch (IOException ex) { throw new SnowflakeSQLLoggedException( sfSession, ErrorCode.INTERNAL_ERROR, String.format( - "Un-able to create log path mentioned in configfile %s ,%s", + "Unable to create log path mentioned in configfile %s ,%s", logPathFromConfig, ex.getMessage())); } } - logPattern = Paths.get(path.toString(), "snowflake_jdbc%u.log").toString(); + } else { + // Get log path from home directory + String homePath = systemGetProperty("user.home"); + if (homePath == null || homePath.isEmpty()) { + throw new SnowflakeSQLLoggedException( + sfSession, + ErrorCode.INTERNAL_ERROR, + String.format( + "Log path not set in configfile %s and home directory not set.", + logPathFromConfig)); + } + logPath = Paths.get(homePath); } + + Path path = createLogPathSubDirectory(logPath); + + logPattern = Paths.get(path.toString(), "snowflake_jdbc%u.log").toString(); return logPattern; } + private Path createLogPathSubDirectory(Path logPath) throws SnowflakeSQLLoggedException { + Path path = Paths.get(logPath.toString(), "jdbc"); + if (!Files.exists(path)) { + createLogFolder(path); + } else { + checkLogFolderPermissions(path); + } + return path; + } + + private void createLogFolder(Path path) throws SnowflakeSQLLoggedException { + try { + if (Constants.getOS() == Constants.OS.WINDOWS) { + Files.createDirectories(path); + } else { + Files.createDirectories( + path, + PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"))); + } + } catch (IOException ex) { + throw new SnowflakeSQLLoggedException( + sfSession, + ErrorCode.INTERNAL_ERROR, + String.format( + "Unable to create jdbc subfolder in configfile %s ,%s", + path.toString(), ex.getMessage(), ex.getCause())); + } + } + + private void checkLogFolderPermissions(Path path) throws SnowflakeSQLLoggedException { + if (Constants.getOS() != Constants.OS.WINDOWS) { + try { + Set folderPermissions = Files.getPosixFilePermissions(path); + if (folderPermissions.contains(PosixFilePermission.GROUP_WRITE) + || folderPermissions.contains(PosixFilePermission.GROUP_READ) + || folderPermissions.contains(PosixFilePermission.GROUP_EXECUTE) + || folderPermissions.contains(PosixFilePermission.OTHERS_WRITE) + || folderPermissions.contains(PosixFilePermission.OTHERS_READ) + || folderPermissions.contains(PosixFilePermission.OTHERS_EXECUTE)) { + logger.warn( + "Access permission for the logs directory '{}' is currently {} and is potentially " + + "accessible to users other than the owner of the logs directory.", + path.toString(), + folderPermissions.toString()); + } + } catch (IOException ex) { + throw new SnowflakeSQLLoggedException( + sfSession, + ErrorCode.INTERNAL_ERROR, + String.format( + "Unable to get permissions of log directory %s ,%s", + path.toString(), ex.getMessage(), ex.getCause())); + } + } + } + private void initSessionProperties(SnowflakeConnectString conStr, String appID, String appVersion) throws SFException { Map properties = mergeProperties(conStr); diff --git a/src/test/java/net/snowflake/client/RunningOnWin.java b/src/test/java/net/snowflake/client/RunningOnWin.java new file mode 100644 index 000000000..025ab1e04 --- /dev/null +++ b/src/test/java/net/snowflake/client/RunningOnWin.java @@ -0,0 +1,9 @@ +package net.snowflake.client; + +import net.snowflake.client.core.Constants; + +public class RunningOnWin implements ConditionalIgnoreRule.IgnoreCondition { + public boolean isSatisfied() { + return Constants.getOS() == Constants.OS.WINDOWS; + } +} diff --git a/src/test/java/net/snowflake/client/config/SFPermissionsTest.java b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java new file mode 100644 index 000000000..92ec8a624 --- /dev/null +++ b/src/test/java/net/snowflake/client/config/SFPermissionsTest.java @@ -0,0 +1,86 @@ +package net.snowflake.client.config; + +import static org.junit.Assert.fail; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.attribute.PosixFilePermissions; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; +import net.snowflake.client.ConditionalIgnoreRule; +import net.snowflake.client.RunningOnWin; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +@RunWith(Parameterized.class) +public class SFPermissionsTest { + @Rule public ConditionalIgnoreRule rule = new ConditionalIgnoreRule(); + + @Parameterized.Parameters(name = "permission={0}") + public static Set> data() { + Map testConfigFilePermissions = + new HashMap() { + { + put("rwx------", false); + put("rw-------", false); + put("r-x------", false); + put("r--------", false); + put("rwxrwx---", true); + put("rwxrw----", true); + put("rwxr-x---", false); + put("rwxr-----", false); + put("rwx-wx---", true); + put("rwx-w----", true); + put("rwx--x---", false); + put("rwx---rwx", true); + put("rwx---rw-", true); + put("rwx---r-x", false); + put("rwx---r--", false); + put("rwx----wx", true); + put("rwx----w-", true); + put("rwx-----x", false); + } + }; + return testConfigFilePermissions.entrySet(); + } + + Path configFilePath = Paths.get("config.json"); + String configJson = "{\"common\":{\"log_level\":\"debug\",\"log_path\":\"logs\"}}"; + String permission; + Boolean isSucceed; + + public SFPermissionsTest(Map.Entry permission) { + this.permission = permission.getKey(); + this.isSucceed = permission.getValue(); + } + + @Before + public void createConfigFile() throws IOException { + Files.write(configFilePath, configJson.getBytes()); + } + + @After + public void cleanupConfigFile() throws IOException { + Files.deleteIfExists(configFilePath); + } + + @Test + @ConditionalIgnoreRule.ConditionalIgnore(condition = RunningOnWin.class) + public void testLogDirectoryPermissions() throws IOException { + // TODO: SNOW-1503722 Change to check for thrown exceptions + // Don't run on Windows + Files.setPosixFilePermissions(configFilePath, PosixFilePermissions.fromString(permission)); + Boolean result = + SFClientConfigParser.checkGroupOthersWritePermissions(configFilePath.toString()); + if (isSucceed != result) { + fail("testLogDirectoryPermissions failed. Expected " + isSucceed); + } + } +} diff --git a/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java b/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java index 84b876147..232da8451 100644 --- a/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java +++ b/src/test/java/net/snowflake/client/log/JDK14LoggerWithClientLatestIT.java @@ -1,5 +1,6 @@ package net.snowflake.client.log; +import static net.snowflake.client.jdbc.SnowflakeUtil.systemGetProperty; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -17,11 +18,14 @@ import java.util.Properties; import java.util.logging.Level; import net.snowflake.client.AbstractDriverIT; +import net.snowflake.client.jdbc.SnowflakeSQLLoggedException; import org.apache.commons.io.FileUtils; import org.junit.Test; public class JDK14LoggerWithClientLatestIT extends AbstractDriverIT { + String homePath = systemGetProperty("user.home"); + @Test public void testJDK14LoggingWithClientConfig() { Path configFilePath = Paths.get("config.json"); @@ -93,4 +97,49 @@ public void testJDK14LoggerWithQuotesInMessage() { logger.debug("Returning column: 12: a: Group b) Hi {Hello 'World' War} cant wait"); JDK14Logger.setLevel(Level.OFF); } + + @Test + public void testJDK14LoggingWithMissingLogPathClientConfig() throws Exception { + Path configFilePath = Paths.get("config.json"); + String configJson = "{\"common\":{\"log_level\":\"debug\"}}"; + + Path homeLogPath = Paths.get(homePath, "jdbc"); + Files.write(configFilePath, configJson.getBytes()); + Properties properties = new Properties(); + properties.put("client_config_file", configFilePath.toString()); + try (Connection connection = getConnection(properties); + Statement statement = connection.createStatement()) { + try { + statement.executeQuery("select 1"); + + File file = new File(homeLogPath.toString()); + assertTrue(file.exists()); + + } finally { + Files.deleteIfExists(configFilePath); + FileUtils.deleteDirectory(new File(homeLogPath.toString())); + } + } + } + + @Test + public void testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig() throws Exception { + System.clearProperty("user.home"); + + Path configFilePath = Paths.get("config.json"); + String configJson = "{\"common\":{\"log_level\":\"debug\"}}"; + Files.write(configFilePath, configJson.getBytes()); + Properties properties = new Properties(); + properties.put("client_config_file", configFilePath.toString()); + try (Connection connection = getConnection(properties); + Statement statement = connection.createStatement()) { + + fail("testJDK14LoggingWithMissingLogPathNoHomeDirClientConfig failed"); + } catch (SnowflakeSQLLoggedException e) { + // Succeed + } finally { + System.setProperty("user.home", homePath); + Files.deleteIfExists(configFilePath); + } + } }