SNOW-2161718-JDBC-fix-permission-check-for-toml-config

CHANGELOG.md

@@ -6,4 +6,5 @@
     - Added HTTP 307 & 308 retries in case of internal IP redirects
     - Make PAT creation return `ResultSet` when using `execute` method
     - Renamed CRL_REVOCATION_CHECK_MODE to CERT_REVOCATION_CHECK_MODE in CLIENT_ENVIRONMENT metrics
-	- Test coverage for multistatement jdbc.
+    - Test coverage for multistatement jdbc.
+    - Fixed permission check for .toml config file.

src/main/java/net/snowflake/client/config/SFConnectionConfigParser.java

@@ -16,9 +16,11 @@
 import java.nio.file.attribute.PosixFilePermission;
 import java.util.Arrays;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Properties;
+import java.util.Set;
 import net.snowflake.client.core.SnowflakeJdbcInternalApi;
 import net.snowflake.client.jdbc.SnowflakeSQLException;
 import net.snowflake.client.log.SFLogger;
@@ -37,6 +39,11 @@ public class SFConnectionConfigParser {
   public static final String SNOWFLAKE_TOKEN_FILE_PATH = "/snowflake/session/token";
   public static final String SKIP_TOKEN_FILE_PERMISSIONS_VERIFICATION =
       "SKIP_TOKEN_FILE_PERMISSIONS_VERIFICATION";
+  public static final String SF_SKIP_WARNING_FOR_READ_PERMISSIONS_ON_CONFIG_FILE =
+      "SF_SKIP_WARNING_FOR_READ_PERMISSIONS_ON_CONFIG_FILE";
+
+  private static final List<PosixFilePermission> REQUIRED_PERMISSIONS =
+      Arrays.asList(PosixFilePermission.OWNER_WRITE, PosixFilePermission.OWNER_READ);
 
   public static ConnectionParameters buildConnectionParameters() throws SnowflakeSQLException {
     String defaultConnectionName =
@@ -115,23 +122,68 @@ private static Map<String, Map> readParametersMap(Path configFilePath)
     }
   }
 
-  private static void verifyFilePermissionSecure(Path configFilePath)
+  static void verifyFilePermissionSecure(Path configFilePath)
       throws IOException, SnowflakeSQLException {
+    final String fileName = "connections.toml";
     if (!isWindows()) {
-      PosixFileAttributeView posixFileAttributeView =
-          Files.getFileAttributeView(configFilePath, PosixFileAttributeView.class);
-      if (!posixFileAttributeView.readAttributes().permissions().stream()
-          .allMatch(
-              o ->
-                  Arrays.asList(PosixFilePermission.OWNER_WRITE, PosixFilePermission.OWNER_READ)
-                      .contains(o))) {
-        logger.error(
-            "Reading from file %s is not safe because file permissions are different than read/write for user",
-            configFilePath);
-        throw new SnowflakeSQLException(
-            String.format(
-                "Reading from file %s is not safe because file permissions are different than read/write for user",
-                configFilePath));
+      if (configFilePath.getFileName().toString().equals(fileName)) {
+        boolean shouldSkipWarningForReadPermissions =
+            convertSystemGetEnvToBooleanValue(
+                SF_SKIP_WARNING_FOR_READ_PERMISSIONS_ON_CONFIG_FILE, false);
+        PosixFileAttributeView posixFileAttributeView =
+            Files.getFileAttributeView(configFilePath, PosixFileAttributeView.class);
+        Set<PosixFilePermission> permissions =
+            posixFileAttributeView.readAttributes().permissions();
+
+        if (!shouldSkipWarningForReadPermissions) {
+          boolean groupRead = permissions.contains(PosixFilePermission.GROUP_READ);
+          boolean othersRead = permissions.contains(PosixFilePermission.OTHERS_READ);
+          // Warning if readable by group/others (must be 600 or stricter)
+          if (groupRead || othersRead) {
+            logger.warn(
+                "File %s is readable by group or others. Permissions should be 600 or stricter for maximum security.",
+                configFilePath);
+          }
+        }
+
+        boolean groupWrite = permissions.contains(PosixFilePermission.GROUP_WRITE);
+        boolean othersWrite = permissions.contains(PosixFilePermission.OTHERS_WRITE);
+        // Error if writable by group/others (must be 644 or stricter)
+        if (groupWrite || othersWrite) {
+          logger.error(
+              "File %s is writable by group or others. Permissions must be 644 or stricter.",
+              configFilePath);
+          throw new SnowflakeSQLException(
+              String.format(
+                  "File %s is writable by group or others. Permissions must be 644 or stricter.",
+                  configFilePath));
+        }
+
+        // Error if executable by anyone
+        boolean ownerExec = permissions.contains(PosixFilePermission.OWNER_EXECUTE);
+        boolean groupExec = permissions.contains(PosixFilePermission.GROUP_EXECUTE);
+        boolean othersExec = permissions.contains(PosixFilePermission.OTHERS_EXECUTE);
+        // Executable permission is not allowed
+        if (ownerExec || groupExec || othersExec) {
+          logger.error(
+              "File %s is executable. Executable permission is not allowed.", configFilePath);
+          throw new SnowflakeSQLException(
+              String.format(
+                  "File %s is executable. Executable permission is not allowed.", configFilePath));
+        }
+      } else {
+        PosixFileAttributeView posixFileAttributeView =
+            Files.getFileAttributeView(configFilePath, PosixFileAttributeView.class);
+        if (!posixFileAttributeView.readAttributes().permissions().stream()
+            .allMatch(o -> REQUIRED_PERMISSIONS.contains(o))) {
+          logger.error(
+              "Reading from file %s is not safe because file permissions are different than read/write for user",
+              configFilePath);
+          throw new SnowflakeSQLException(
+              String.format(
+                  "Reading from file %s is not safe because file permissions are different than read/write for user",
+                  configFilePath));
+        }
       }
     }
   }

src/test/java/net/snowflake/client/config/SFConnectionConfigParserPermissionTest.java

@@ -0,0 +1,145 @@
+package net.snowflake.client.config;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import net.snowflake.client.annotations.DontRunOnWindows;
+import net.snowflake.client.jdbc.SnowflakeSQLException;
+import net.snowflake.client.jdbc.SnowflakeUtil;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+class SFConnectionConfigParserPermissionTest {
+
+  private Path createTempFileWithPermissions(Set<PosixFilePermission> perms) throws Exception {
+    // Create a unique temporary directory
+    Path tempDir = Files.createTempDirectory("snowflake");
+
+    // Inside it, create a file named "connections.toml"
+    Path tomlFile = tempDir.resolve("connections.toml");
+    Files.createFile(tomlFile);
+
+    // Apply the given POSIX permissions
+    Files.setPosixFilePermissions(tomlFile, perms);
+
+    // Mark both the file and the directory for deletion on JVM exit
+    tomlFile.toFile().deleteOnExit();
+    tempDir.toFile().deleteOnExit();
+
+    return tomlFile;
+  }
+
+  static List<Object[]> permissionTestCases() {
+    return Arrays.asList(
+        new Object[][] {
+          { // Group write
+            new HashSet<>(
+                Arrays.asList(
+                    PosixFilePermission.OWNER_READ,
+                    PosixFilePermission.OWNER_WRITE,
+                    PosixFilePermission.GROUP_WRITE)),
+            true,
+            "writable by group or others"
+          },
+          { // Others write
+            new HashSet<>(
+                Arrays.asList(
+                    PosixFilePermission.OWNER_READ,
+                    PosixFilePermission.OWNER_WRITE,
+                    PosixFilePermission.OTHERS_WRITE)),
+            true,
+            "writable by group or others"
+          },
+          { // Owner execute
+            new HashSet<>(
+                Arrays.asList(
+                    PosixFilePermission.OWNER_READ,
+                    PosixFilePermission.OWNER_WRITE,
+                    PosixFilePermission.OWNER_EXECUTE)),
+            true,
+            "executable"
+          },
+          { // Group execute
+            new HashSet<>(
+                Arrays.asList(
+                    PosixFilePermission.OWNER_READ,
+                    PosixFilePermission.OWNER_WRITE,
+                    PosixFilePermission.GROUP_EXECUTE)),
+            true,
+            "executable"
+          },
+          { // Others execute
+            new HashSet<>(
+                Arrays.asList(
+                    PosixFilePermission.OWNER_READ,
+                    PosixFilePermission.OWNER_WRITE,
+                    PosixFilePermission.OTHERS_EXECUTE)),
+            true,
+            "executable"
+          },
+          { // Owner read/write only
+            new HashSet<>(
+                Arrays.asList(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE)),
+            false,
+            null
+          }
+        });
+  }
+
+  @ParameterizedTest
+  @MethodSource("permissionTestCases")
+  @DontRunOnWindows
+  void testFilePermissionScenarios(
+      Set<PosixFilePermission> perms, boolean shouldThrow, String expectedMsg) throws Exception {
+    Path tempFile = createTempFileWithPermissions(perms);
+    try {
+      if (shouldThrow) {
+        Exception ex =
+            assertThrows(
+                SnowflakeSQLException.class,
+                () -> SFConnectionConfigParser.verifyFilePermissionSecure(tempFile));
+        assertTrue(ex.getMessage().contains(expectedMsg));
+      } else {
+        assertDoesNotThrow(() -> SFConnectionConfigParser.verifyFilePermissionSecure(tempFile));
+      }
+    } finally {
+      Files.deleteIfExists(tempFile);
+    }
+  }
+
+  static List<Object[]> skipReadWarningTestCases() {
+    return Arrays.asList(
+        new Object[][] {
+          {
+            new HashSet<>(
+                Arrays.asList(
+                    PosixFilePermission.OWNER_READ,
+                    PosixFilePermission.OWNER_WRITE,
+                    PosixFilePermission.GROUP_READ,
+                    PosixFilePermission.OTHERS_READ))
+          }
+        });
+  }
+
+  @ParameterizedTest
+  @MethodSource("skipReadWarningTestCases")
+  @DontRunOnWindows
+  void testSkipWarningForReadPermissionsEnvVar(Set<PosixFilePermission> perms) throws Exception {
+    Path tempFile = createTempFileWithPermissions(perms);
+    SnowflakeUtil.systemSetEnv("SF_SKIP_WARNING_FOR_READ_PERMISSIONS_ON_CONFIG_FILE", "true");
+    try {
+      assertDoesNotThrow(() -> SFConnectionConfigParser.verifyFilePermissionSecure(tempFile));
+    } finally {
+      Files.deleteIfExists(tempFile);
+      SnowflakeUtil.systemSetEnv("SF_SKIP_WARNING_FOR_READ_PERMISSIONS_ON_CONFIG_FILE", null);
+    }
+  }
+}