From 9e1a5acf12406b16c4780ca013f4c4db48b74b59 Mon Sep 17 00:00:00 2001 From: Przemyslaw Motacki Date: Wed, 29 Jan 2025 12:32:57 +0100 Subject: [PATCH] SNOW-1653464: Creating .cache directory and cache files permissions (#2052) --- .../config/SFConnectionConfigParser.java | 4 +- .../client/core/FileCacheManager.java | 61 +++++-- .../net/snowflake/client/core/FileUtil.java | 78 ++++++++- .../snowflake/client/core/SFTrustManager.java | 1 + .../jdbc/DefaultSFConnectionHandler.java | 3 +- .../snowflake/client/jdbc/SnowflakeUtil.java | 10 ++ .../config/SFConnectionConfigParserTest.java | 4 +- .../client/core/FileCacheManagerTest.java | 163 ++++++++++++++++++ 8 files changed, 304 insertions(+), 20 deletions(-) create mode 100644 src/test/java/net/snowflake/client/core/FileCacheManagerTest.java diff --git a/src/main/java/net/snowflake/client/config/SFConnectionConfigParser.java b/src/main/java/net/snowflake/client/config/SFConnectionConfigParser.java index 1da9f766a..1a93d20f6 100644 --- a/src/main/java/net/snowflake/client/config/SFConnectionConfigParser.java +++ b/src/main/java/net/snowflake/client/config/SFConnectionConfigParser.java @@ -1,6 +1,7 @@ package net.snowflake.client.config; import static net.snowflake.client.jdbc.SnowflakeUtil.convertSystemGetEnvToBooleanValue; +import static net.snowflake.client.jdbc.SnowflakeUtil.isWindows; import static net.snowflake.client.jdbc.SnowflakeUtil.systemGetEnv; import com.fasterxml.jackson.dataformat.toml.TomlMapper; @@ -18,7 +19,6 @@ import java.util.Map; import java.util.Optional; import java.util.Properties; -import net.snowflake.client.core.Constants; import net.snowflake.client.core.SnowflakeJdbcInternalApi; import net.snowflake.client.jdbc.SnowflakeSQLException; import net.snowflake.client.log.SFLogger; @@ -117,7 +117,7 @@ private static Map readParametersMap(Path configFilePath) private static void verifyFilePermissionSecure(Path configFilePath) throws IOException, SnowflakeSQLException { - if (Constants.getOS() != Constants.OS.WINDOWS) { + if (!isWindows()) { PosixFileAttributeView posixFileAttributeView = Files.getFileAttributeView(configFilePath, PosixFileAttributeView.class); if (!posixFileAttributeView.readAttributes().permissions().stream() diff --git a/src/main/java/net/snowflake/client/core/FileCacheManager.java b/src/main/java/net/snowflake/client/core/FileCacheManager.java index 05972b620..2d00d6d0a 100644 --- a/src/main/java/net/snowflake/client/core/FileCacheManager.java +++ b/src/main/java/net/snowflake/client/core/FileCacheManager.java @@ -4,6 +4,7 @@ package net.snowflake.client.core; +import static net.snowflake.client.jdbc.SnowflakeUtil.isWindows; import static net.snowflake.client.jdbc.SnowflakeUtil.systemGetEnv; import static net.snowflake.client.jdbc.SnowflakeUtil.systemGetProperty; @@ -23,7 +24,11 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.attribute.BasicFileAttributes; +import java.nio.file.attribute.PosixFilePermission; +import java.nio.file.attribute.PosixFilePermissions; import java.util.Date; +import java.util.stream.Collectors; +import java.util.stream.Stream; import net.snowflake.client.log.SFLogger; import net.snowflake.client.log.SFLoggerFactory; @@ -46,6 +51,8 @@ class FileCacheManager { private File cacheDir; + private boolean onlyOwnerPermissions = true; + private FileCacheManager() {} static FileCacheManager builder() { @@ -78,16 +85,29 @@ FileCacheManager setCacheFileLockExpirationInSeconds(long cacheFileLockExpiratio return this; } + FileCacheManager setOnlyOwnerPermissions(boolean onlyOwnerPermissions) { + this.onlyOwnerPermissions = onlyOwnerPermissions; + return this; + } + /** * Override the cache file. * * @param newCacheFile a file object to override the default one. */ void overrideCacheFile(File newCacheFile) { + if (!newCacheFile.exists()) { + logger.debug("Cache file doesn't exists. File: {}", newCacheFile); + } + if (onlyOwnerPermissions) { + FileUtil.throwWhenPermiossionDifferentThanReadWriteForOwner( + newCacheFile, "Override cache file"); + } else { + FileUtil.logFileUsage(cacheFile, "Override cache file", false); + } this.cacheFile = newCacheFile; this.cacheDir = newCacheFile.getParentFile(); this.baseCacheFileName = newCacheFile.getName(); - FileUtil.logFileUsage(cacheFile, "Override cache file", true); } FileCacheManager build() { @@ -116,15 +136,12 @@ FileCacheManager build() { } else { // use user home directory to store the cache file String homeDir = systemGetProperty("user.home"); - if (homeDir == null) { - // use tmp dir if not exists. - homeDir = systemGetProperty("java.io.tmpdir"); - } else { + if (homeDir != null) { // Checking if home directory is writable. File homeFile = new File(homeDir); if (!homeFile.canWrite()) { - logger.debug("Home directory not writeable, using tmpdir", false); - homeDir = systemGetProperty("java.io.tmpdir"); + logger.debug("Home directory not writeable, skip using cache", false); + homeDir = null; } } if (homeDir == null) { @@ -155,7 +172,16 @@ FileCacheManager build() { // If exists. the method returns false. // In this particular case, it doesn't matter as long as the file is // writable. - if (cacheFileTmp.createNewFile()) { + if (!cacheFileTmp.exists()) { + if (!isWindows() && onlyOwnerPermissions) { + Files.createFile( + cacheFileTmp.toPath(), + PosixFilePermissions.asFileAttribute( + Stream.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE) + .collect(Collectors.toSet()))); + } else { + Files.createFile(cacheFileTmp.toPath()); + } logger.debug("Successfully created a cache file {}", cacheFileTmp); } else { logger.debug("Cache file already exists {}", cacheFileTmp); @@ -165,7 +191,10 @@ FileCacheManager build() { this.cacheLockFile = new File(this.cacheFile.getParentFile(), this.baseCacheFileName + ".lck"); } catch (IOException | SecurityException ex) { - logger.debug("Failed to touch the cache file. Ignored. {}", cacheFileTmp.getAbsoluteFile()); + logger.info( + "Failed to touch the cache file: {}. Ignored. {}", + ex.getMessage(), + cacheFileTmp.getAbsoluteFile()); } return this; } @@ -184,7 +213,13 @@ JsonNode readCacheFile() { try (Reader reader = new InputStreamReader(new FileInputStream(cacheFile), DEFAULT_FILE_ENCODING)) { - FileUtil.logFileUsage(cacheFile, "Read cache", false); + + if (onlyOwnerPermissions) { + FileUtil.throwWhenPermiossionDifferentThanReadWriteForOwner(cacheFile, "Read cache"); + FileUtil.throwWhenOwnerDifferentThanCurrentUser(cacheFile, "Read cache"); + } else { + FileUtil.logFileUsage(cacheFile, "Read cache", false); + } return OBJECT_MAPPER.readTree(reader); } } catch (IOException ex) { @@ -208,7 +243,11 @@ void writeCacheFile(JsonNode input) { } try (Writer writer = new OutputStreamWriter(new FileOutputStream(cacheFile), DEFAULT_FILE_ENCODING)) { - FileUtil.logFileUsage(cacheFile, "Write to cache", false); + if (onlyOwnerPermissions) { + FileUtil.throwWhenPermiossionDifferentThanReadWriteForOwner(cacheFile, "Write to cache"); + } else { + FileUtil.logFileUsage(cacheFile, "Write to cache", false); + } writer.write(input.toString()); } } catch (IOException ex) { diff --git a/src/main/java/net/snowflake/client/core/FileUtil.java b/src/main/java/net/snowflake/client/core/FileUtil.java index 3ae68909b..005b9ac18 100644 --- a/src/main/java/net/snowflake/client/core/FileUtil.java +++ b/src/main/java/net/snowflake/client/core/FileUtil.java @@ -1,11 +1,14 @@ package net.snowflake.client.core; +import static net.snowflake.client.jdbc.SnowflakeUtil.isWindows; + import com.google.common.base.Strings; import java.io.File; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.nio.file.attribute.FileOwnerAttributeView; import java.nio.file.attribute.PosixFilePermission; import java.util.Arrays; import java.util.Collection; @@ -19,9 +22,13 @@ public class FileUtil { Arrays.asList(PosixFilePermission.GROUP_WRITE, PosixFilePermission.OTHERS_WRITE); private static final Collection READ_BY_OTHERS = Arrays.asList(PosixFilePermission.GROUP_READ, PosixFilePermission.OTHERS_READ); + private static final Collection EXECUTABLE = + Arrays.asList( + PosixFilePermission.OWNER_EXECUTE, + PosixFilePermission.GROUP_EXECUTE, + PosixFilePermission.OTHERS_EXECUTE); public static void logFileUsage(Path filePath, String context, boolean logReadAccess) { - logger.info("{}Accessing file: {}", getContextStr(context), filePath); logWarnWhenAccessibleByOthers(filePath, context, logReadAccess); } @@ -34,10 +41,40 @@ public static void logFileUsage(String stringPath, String context, boolean logRe logFileUsage(path, context, logReadAccess); } + public static void throwWhenPermiossionDifferentThanReadWriteForOwner(File file, String context) { + throwWhenPermiossionDifferentThanReadWriteForOwner(file.toPath(), context); + } + + public static void throwWhenPermiossionDifferentThanReadWriteForOwner( + Path filePath, String context) { + // we do not check the permissions for Windows + if (isWindows()) { + return; + } + + try { + Collection filePermissions = Files.getPosixFilePermissions(filePath); + boolean isWritableByOthers = isPermPresent(filePermissions, WRITE_BY_OTHERS); + boolean isReadableByOthers = isPermPresent(filePermissions, READ_BY_OTHERS); + boolean isExecutable = isPermPresent(filePermissions, EXECUTABLE); + + if (isWritableByOthers || isReadableByOthers || isExecutable) { + logger.debug( + "{}File {} access rights: {}", getContextStr(context), filePath, filePermissions); + throw new SecurityException( + String.format("Access to file %s is wider than allowed only to the owner", filePath)); + } + } catch (IOException e) { + throw new SecurityException( + String.format( + "%s Unable to access the file to check the permissions. Error: %s", filePath, e)); + } + } + private static void logWarnWhenAccessibleByOthers( Path filePath, String context, boolean logReadAccess) { // we do not check the permissions for Windows - if (Constants.getOS() == Constants.OS.WINDOWS) { + if (isWindows()) { return; } @@ -48,14 +85,16 @@ private static void logWarnWhenAccessibleByOthers( boolean isWritableByOthers = isPermPresent(filePermissions, WRITE_BY_OTHERS); boolean isReadableByOthers = isPermPresent(filePermissions, READ_BY_OTHERS); + boolean isExecutable = isPermPresent(filePermissions, EXECUTABLE); - if (isWritableByOthers || (isReadableByOthers && logReadAccess)) { + if (isWritableByOthers || (isReadableByOthers || isExecutable)) { logger.warn( "{}File {} is accessible by others to:{}{}", getContextStr(context), filePath, isReadableByOthers && logReadAccess ? " read" : "", - isWritableByOthers ? " write" : ""); + isWritableByOthers ? " write" : "", + isExecutable ? " executable" : ""); } } catch (IOException e) { logger.warn( @@ -66,12 +105,43 @@ private static void logWarnWhenAccessibleByOthers( } } + public static void throwWhenOwnerDifferentThanCurrentUser(File file, String context) { + // we do not check the permissions for Windows + if (isWindows()) { + return; + } + + Path filePath = Paths.get(file.getPath()); + + try { + String fileOwnerName = getFileOwnerName(filePath); + String currentUser = System.getProperty("user.name"); + if (!currentUser.equalsIgnoreCase(fileOwnerName)) { + logger.debug( + "The file owner: {} is different than current user: {}", fileOwnerName, currentUser); + throw new SecurityException("The file owner is different than current user"); + } + } catch (IOException e) { + logger.warn( + "{}Unable to access the file to check the owner: {}. Error: {}", + getContextStr(context), + filePath, + e); + } + } + private static boolean isPermPresent( Collection filePerms, Collection permsToCheck) throws IOException { return filePerms.stream().anyMatch(permsToCheck::contains); } + static String getFileOwnerName(Path filePath) throws IOException { + FileOwnerAttributeView ownerAttributeView = + Files.getFileAttributeView(filePath, FileOwnerAttributeView.class); + return ownerAttributeView.getOwner().getName(); + } + private static String getContextStr(String context) { return Strings.isNullOrEmpty(context) ? "" : context + ": "; } diff --git a/src/main/java/net/snowflake/client/core/SFTrustManager.java b/src/main/java/net/snowflake/client/core/SFTrustManager.java index 275037eb0..6440ec6c3 100644 --- a/src/main/java/net/snowflake/client/core/SFTrustManager.java +++ b/src/main/java/net/snowflake/client/core/SFTrustManager.java @@ -227,6 +227,7 @@ public class SFTrustManager extends X509ExtendedTrustManager { .setBaseCacheFileName(CACHE_FILE_NAME) .setCacheExpirationInSeconds(CACHE_EXPIRATION_IN_SECONDS) .setCacheFileLockExpirationInSeconds(CACHE_FILE_LOCK_EXPIRATION_IN_SECONDS) + .setOnlyOwnerPermissions(false) .build(); } diff --git a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java index 48d5e0734..7c1c3d863 100644 --- a/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java +++ b/src/main/java/net/snowflake/client/jdbc/DefaultSFConnectionHandler.java @@ -2,6 +2,7 @@ import static net.snowflake.client.core.SessionUtil.CLIENT_SFSQL; import static net.snowflake.client.core.SessionUtil.JVM_PARAMS_TO_PARAMS; +import static net.snowflake.client.jdbc.SnowflakeUtil.isWindows; import static net.snowflake.client.jdbc.SnowflakeUtil.systemGetProperty; import java.io.IOException; @@ -278,7 +279,7 @@ private void createLogFolder(Path path) throws SnowflakeSQLLoggedException { } private void checkLogFolderPermissions(Path path) throws SnowflakeSQLLoggedException { - if (Constants.getOS() != Constants.OS.WINDOWS) { + if (!isWindows()) { try { Set folderPermissions = Files.getPosixFilePermissions(path); if (folderPermissions.contains(PosixFilePermission.GROUP_WRITE) diff --git a/src/main/java/net/snowflake/client/jdbc/SnowflakeUtil.java b/src/main/java/net/snowflake/client/jdbc/SnowflakeUtil.java index fbfa84676..0edee155c 100644 --- a/src/main/java/net/snowflake/client/jdbc/SnowflakeUtil.java +++ b/src/main/java/net/snowflake/client/jdbc/SnowflakeUtil.java @@ -901,4 +901,14 @@ public static Map createCaseInsensitiveMap(Header[] headers) { return new TreeMap<>(String.CASE_INSENSITIVE_ORDER); } } + + /** + * Check whether the OS is Windows + * + * @return boolean + */ + @SnowflakeJdbcInternalApi + public static boolean isWindows() { + return Constants.getOS() == Constants.OS.WINDOWS; + } } diff --git a/src/test/java/net/snowflake/client/config/SFConnectionConfigParserTest.java b/src/test/java/net/snowflake/client/config/SFConnectionConfigParserTest.java index 50dd75ff2..29e99361b 100644 --- a/src/test/java/net/snowflake/client/config/SFConnectionConfigParserTest.java +++ b/src/test/java/net/snowflake/client/config/SFConnectionConfigParserTest.java @@ -4,6 +4,7 @@ import static net.snowflake.client.config.SFConnectionConfigParser.SKIP_TOKEN_FILE_PERMISSIONS_VERIFICATION; import static net.snowflake.client.config.SFConnectionConfigParser.SNOWFLAKE_DEFAULT_CONNECTION_NAME_KEY; import static net.snowflake.client.config.SFConnectionConfigParser.SNOWFLAKE_HOME_KEY; +import static net.snowflake.client.jdbc.SnowflakeUtil.isWindows; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; @@ -25,7 +26,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import net.snowflake.client.core.Constants; import net.snowflake.client.jdbc.SnowflakeSQLException; import net.snowflake.client.jdbc.SnowflakeUtil; import org.junit.jupiter.api.AfterEach; @@ -237,7 +237,7 @@ private void prepareConnectionConfigurationTomlFile( private Path createFilePathWithPermission(Path path, boolean onlyUserPermission) throws IOException { - if (Constants.getOS() != Constants.OS.WINDOWS) { + if (!isWindows()) { FileAttribute> fileAttribute = onlyUserPermission ? PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rw-------")) diff --git a/src/test/java/net/snowflake/client/core/FileCacheManagerTest.java b/src/test/java/net/snowflake/client/core/FileCacheManagerTest.java new file mode 100644 index 000000000..023c20d40 --- /dev/null +++ b/src/test/java/net/snowflake/client/core/FileCacheManagerTest.java @@ -0,0 +1,163 @@ +/* + * Copyright (c) 2012-2022 Snowflake Computing Inc. All rights reserved. + */ + +package net.snowflake.client.core; + +import static net.snowflake.client.core.StmtUtil.mapper; +import static net.snowflake.client.jdbc.SnowflakeUtil.systemGetProperty; +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 static org.mockito.ArgumentMatchers.isA; + +import com.fasterxml.jackson.databind.node.ObjectNode; +import java.io.File; +import java.io.IOException; +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.util.stream.Collectors; +import java.util.stream.Stream; +import net.snowflake.client.annotations.RunOnLinuxOrMac; +import net.snowflake.client.category.TestTags; +import net.snowflake.client.jdbc.BaseJDBCTest; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.mockito.MockedStatic; +import org.mockito.Mockito; + +@Nested +@Tag(TestTags.CORE) +class FileCacheManagerTest extends BaseJDBCTest { + + private static final String CACHE_FILE_NAME = "temporary_credential.json"; + private static final String CACHE_DIR_PROP = "net.snowflake.jdbc.temporaryCredentialCacheDir"; + private static final String CACHE_DIR_ENV = "SF_TEMPORARY_CREDENTIAL_CACHE_DIR"; + private static final long CACHE_EXPIRATION_IN_SECONDS = 86400L; + private static final long CACHE_FILE_LOCK_EXPIRATION_IN_SECONDS = 60L; + + private FileCacheManager fileCacheManager; + private File cacheFile; + + @BeforeEach + public void setup() throws IOException { + fileCacheManager = + FileCacheManager.builder() + .setCacheDirectorySystemProperty(CACHE_DIR_PROP) + .setCacheDirectoryEnvironmentVariable(CACHE_DIR_ENV) + .setBaseCacheFileName(CACHE_FILE_NAME) + .setCacheExpirationInSeconds(CACHE_EXPIRATION_IN_SECONDS) + .setCacheFileLockExpirationInSeconds(CACHE_FILE_LOCK_EXPIRATION_IN_SECONDS) + .build(); + cacheFile = createCacheFile(); + } + + @AfterEach + public void clean() throws IOException { + if (Files.exists(cacheFile.toPath())) { + Files.delete(cacheFile.toPath()); + } + } + + @ParameterizedTest + @CsvSource({ + "rwx------,false", + "rw-------,true", + "r-x------,false", + "r--------,true", + "rwxrwx---,false", + "rwxrw----,false", + "rwxr-x---,false", + "rwxr-----,false", + "rwx-wx---,false", + "rwx-w----,false", + "rwx--x---,false", + "rwx---rwx,false", + "rwx---rw-,false", + "rwx---r-x,false", + "rwx---r--,false", + "rwx----wx,false", + "rwx----w-,false", + "rwx-----x,false" + }) + @RunOnLinuxOrMac + public void throwWhenReadCacheFileWithPermissionDifferentThanReadWriteForUserTest( + String permission, boolean isSucceed) throws IOException { + fileCacheManager.overrideCacheFile(cacheFile); + Files.setPosixFilePermissions(cacheFile.toPath(), PosixFilePermissions.fromString(permission)); + if (isSucceed) { + assertDoesNotThrow(() -> fileCacheManager.readCacheFile()); + } else { + SecurityException ex = + assertThrows(SecurityException.class, () -> fileCacheManager.readCacheFile()); + assertTrue(ex.getMessage().contains("is wider than allowed only to the owner")); + } + } + + @Test + @RunOnLinuxOrMac + public void throwWhenOverrideCacheFileHasDifferentOwnerThanCurrentUserTest() { + try (MockedStatic fileUtilMock = + Mockito.mockStatic(FileUtil.class, Mockito.CALLS_REAL_METHODS)) { + fileUtilMock.when(() -> FileUtil.getFileOwnerName(isA(Path.class))).thenReturn("anotherUser"); + SecurityException ex = + assertThrows(SecurityException.class, () -> fileCacheManager.readCacheFile()); + assertTrue(ex.getMessage().contains("The file owner is different than current user")); + } + } + + @Test + @RunOnLinuxOrMac + public void notThrowForToWidePermissionsWhenOnlyOwnerPermissionsSetFalseTest() + throws IOException { + fileCacheManager.setOnlyOwnerPermissions(false); + Files.setPosixFilePermissions(cacheFile.toPath(), PosixFilePermissions.fromString("rwxrwx---")); + assertDoesNotThrow(() -> fileCacheManager.readCacheFile()); + } + + @Test + @RunOnLinuxOrMac + public void throwWhenOverrideCacheFileNotFound() { + Path wrongPath = + Paths.get(systemGetProperty("user.home"), ".cache", "snowflake2", "wrongFileName"); + SecurityException ex = + assertThrows( + SecurityException.class, () -> fileCacheManager.overrideCacheFile(wrongPath.toFile())); + assertTrue( + ex.getMessage() + .contains( + "Unable to access the file to check the permissions. Error: java.nio.file.NoSuchFileException:")); + } + + private File createCacheFile() { + Path cacheFile = + Paths.get(systemGetProperty("user.home"), ".cache", "snowflake2", CACHE_FILE_NAME); + try { + if (Files.exists(cacheFile)) { + Files.delete(cacheFile); + } + Files.createDirectories(cacheFile.getParent()); + Files.createFile( + cacheFile, + PosixFilePermissions.asFileAttribute( + Stream.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE) + .collect(Collectors.toSet()))); + ObjectNode cacheContent = mapper.createObjectNode(); + cacheContent.put("token", "tokenValue"); + fileCacheManager.overrideCacheFile(cacheFile.toFile()); + fileCacheManager.writeCacheFile(cacheContent); + return cacheFile.toFile(); + + } catch (IOException e) { + throw new RuntimeException(e); + } + } +}