SNOW-957747: Easy logging improvements (#1730)

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<String> 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<PosixFilePermission> 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]:[\\\\/]");

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,20 +188,18 @@ && 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(
               sfSession, ErrorCode.INTERNAL_ERROR, ex.getMessage());
         }
         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<PosixFilePermission> 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<String, Object> properties = mergeProperties(conStr);

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;
+  }
+}

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<Map.Entry<String, Boolean>> data() {
+    Map<String, Boolean> testConfigFilePermissions =
+        new HashMap<String, Boolean>() {
+          {
+            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<String, Boolean> 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);
+    }
+  }
+}