From 7675dcff698ec23d009ca858ae5965d1c4451914 Mon Sep 17 00:00:00 2001 From: Juan Martinez Ramirez Date: Tue, 26 Nov 2024 18:20:11 -0600 Subject: [PATCH] Applying PR suggestions --- .../UnitTests/Tools/FileOperationsTest.cs | 6 ++-- .../UnitTests/Tools/UnixOperationsTest.cs | 11 +++--- .../SFCredentialManagerFileImpl.cs | 20 +++++++++-- Snowflake.Data/Core/TomlConnectionBuilder.cs | 20 +++++++++-- Snowflake.Data/Core/Tools/UnixOperations.cs | 36 +++---------------- 5 files changed, 50 insertions(+), 43 deletions(-) diff --git a/Snowflake.Data.Tests/UnitTests/Tools/FileOperationsTest.cs b/Snowflake.Data.Tests/UnitTests/Tools/FileOperationsTest.cs index 37b7cc48d..b8b311357 100644 --- a/Snowflake.Data.Tests/UnitTests/Tools/FileOperationsTest.cs +++ b/Snowflake.Data.Tests/UnitTests/Tools/FileOperationsTest.cs @@ -50,7 +50,7 @@ public void TestReadAllTextOnWindows() var filePath = CreateConfigTempFile(s_workingDirectory, content); // act - var result = s_fileOperations.ReadAllText(filePath, UnixOperations.ValidateFileWhenReadIsAccessedOnlyByItsOwner); + var result = s_fileOperations.ReadAllText(filePath, TomlConnectionBuilder.ValidateFilePermissions); // assert Assert.AreEqual(content, result); @@ -73,7 +73,7 @@ public void TestReadAllTextCheckingPermissionsUsingTomlConfigurationFileValidati Syscall.chmod(filePath, (FilePermissions)filePermissions); // act - var result = s_fileOperations.ReadAllText(filePath, UnixOperations.ValidateFileWhenReadIsAccessedOnlyByItsOwner); + var result = s_fileOperations.ReadAllText(filePath, TomlConnectionBuilder.ValidateFilePermissions); // assert Assert.AreEqual(content, result); @@ -96,7 +96,7 @@ public void TestShouldThrowExceptionIfOtherPermissionsIsSetWhenReadConfiguration Syscall.chmod(filePath, (FilePermissions)filePermissions); // act and assert - Assert.Throws(() => s_fileOperations.ReadAllText(filePath, UnixOperations.ValidateFileWhenReadIsAccessedOnlyByItsOwner), + Assert.Throws(() => s_fileOperations.ReadAllText(filePath, TomlConnectionBuilder.ValidateFilePermissions), "Attempting to read a file with too broad permissions assigned"); } diff --git a/Snowflake.Data.Tests/UnitTests/Tools/UnixOperationsTest.cs b/Snowflake.Data.Tests/UnitTests/Tools/UnixOperationsTest.cs index 5a6db8ae7..57b656d92 100644 --- a/Snowflake.Data.Tests/UnitTests/Tools/UnixOperationsTest.cs +++ b/Snowflake.Data.Tests/UnitTests/Tools/UnixOperationsTest.cs @@ -6,6 +6,7 @@ using Mono.Unix.Native; using NUnit.Framework; using Snowflake.Data.Core; +using Snowflake.Data.Core.CredentialManager.Infrastructure; using Snowflake.Data.Core.Tools; using static Snowflake.Data.Tests.UnitTests.Configuration.EasyLoggingConfigGenerator; @@ -96,14 +97,14 @@ public void TestReadAllTextCheckingPermissionsUsingTomlConfigurationFileValidati Syscall.chmod(filePath, userAllowedPermissions); // act - var result = s_unixOperations.ReadAllText(filePath, UnixOperations.ValidateFileWhenReadIsAccessedOnlyByItsOwner); + var result = s_unixOperations.ReadAllText(filePath, TomlConnectionBuilder.ValidateFilePermissions); // assert Assert.AreEqual(content, result); } [Test] - public void TestWriteAllTextCheckingPermissionsUsingTomlConfigurationFileValidations( + public void TestWriteAllTextCheckingPermissionsUsingSFCredentialManagerFileValidations( [ValueSource(nameof(UserAllowedWritePermissions))] FilePermissions userAllowedPermissions) { if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) @@ -115,7 +116,7 @@ public void TestWriteAllTextCheckingPermissionsUsingTomlConfigurationFileValidat Syscall.chmod(filePath, userAllowedPermissions); // act and assert - Assert.DoesNotThrow(() => s_unixOperations.WriteAllText(filePath,"test", UnixOperations.ValidateFileWhenWriteIsAccessedOnlyByItsOwner)); + Assert.DoesNotThrow(() => s_unixOperations.WriteAllText(filePath,"test", SFCredentialManagerFileImpl.ValidateFilePermissions)); } [Test] @@ -139,7 +140,7 @@ public void TestFailIfGroupOrOthersHavePermissionsToFileWhileReadingWithUnixVali Syscall.chmod(filePath, filePermissions); // act and assert - Assert.Throws(() => s_unixOperations.ReadAllText(filePath, UnixOperations.ValidateFileWhenReadIsAccessedOnlyByItsOwner), "Attempting to read a file with too broad permissions assigned"); + Assert.Throws(() => s_unixOperations.ReadAllText(filePath, TomlConnectionBuilder.ValidateFilePermissions), "Attempting to read a file with too broad permissions assigned"); } [Test] @@ -163,7 +164,7 @@ public void TestFailIfGroupOrOthersHavePermissionsToFileWhileWritingWithUnixVali Syscall.chmod(filePath, filePermissions); // act and assert - Assert.Throws(() => s_unixOperations.WriteAllText(filePath, "test", UnixOperations.ValidateFileWhenReadIsAccessedOnlyByItsOwner), "Attempting to write a file with too broad permissions assigned"); + Assert.Throws(() => s_unixOperations.WriteAllText(filePath, "test", SFCredentialManagerFileImpl.ValidateFilePermissions), "Attempting to read or write a file with too broad permissions assigned"); } public static IEnumerable UserPermissions() diff --git a/Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs b/Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs index 3dd547611..59792b5a1 100644 --- a/Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs +++ b/Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs @@ -10,7 +10,9 @@ using Snowflake.Data.Log; using System; using System.IO; +using System.Linq; using System.Runtime.InteropServices; +using System.Security; using System.Threading; using KeyTokenDict = System.Collections.Generic.Dictionary; @@ -91,7 +93,7 @@ internal void WriteToJsonFile(string content) } else { - _fileOperations.Write(_jsonCacheFilePath, content, UnixOperations.ValidateFileWhenWriteIsAccessedOnlyByItsOwner); + _fileOperations.Write(_jsonCacheFilePath, content, ValidateFilePermissions); } var jsonPermissions = _unixOperations.GetFilePermissions(_jsonCacheFilePath); @@ -106,7 +108,7 @@ internal void WriteToJsonFile(string content) internal KeyTokenDict ReadJsonFile() { - var contentFile = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? File.ReadAllText(_jsonCacheFilePath) : _fileOperations.ReadAllText(_jsonCacheFilePath, UnixOperations.ValidateFileWhenReadIsAccessedOnlyByItsOwner); + var contentFile = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? File.ReadAllText(_jsonCacheFilePath) : _fileOperations.ReadAllText(_jsonCacheFilePath, ValidateFilePermissions); return JsonConvert.DeserializeObject(contentFile); } @@ -170,5 +172,19 @@ public void SaveCredentials(string key, string token) _lock.ExitWriteLock(); } } + + internal static void ValidateFilePermissions(UnixStream stream) + { + var allowedPermissions = new[] + { + FileAccessPermissions.UserRead | FileAccessPermissions.UserWrite + }; + if (stream.OwnerUser.UserId != Syscall.geteuid()) + throw new SecurityException("Attempting to read or write a file not owned by the effective user of the current process"); + if (stream.OwnerGroup.GroupId != Syscall.getegid()) + throw new SecurityException("Attempting to read or write a file not owned by the effective group of the current process"); + if (!(allowedPermissions.Any(a => stream.FileAccessPermissions == a))) + throw new SecurityException("Attempting to read or write a file with too broad permissions assigned"); + } } } diff --git a/Snowflake.Data/Core/TomlConnectionBuilder.cs b/Snowflake.Data/Core/TomlConnectionBuilder.cs index 6206c856b..481628802 100644 --- a/Snowflake.Data/Core/TomlConnectionBuilder.cs +++ b/Snowflake.Data/Core/TomlConnectionBuilder.cs @@ -116,7 +116,7 @@ private string LoadTokenFromFile(string tokenFilePathValue) tokenFile = tokenFilePathValue; } s_logger.Info($"Read token from file path: {tokenFile}"); - return _fileOperations.Exists(tokenFile) ? _fileOperations.ReadAllText(tokenFile, UnixOperations.ValidateFileWhenReadIsAccessedOnlyByItsOwner) : null; + return _fileOperations.Exists(tokenFile) ? _fileOperations.ReadAllText(tokenFile, ValidateFilePermissions) : null; } private TomlTable GetTomlTableFromConfig(string tomlPath, string connectionName) @@ -126,7 +126,7 @@ private TomlTable GetTomlTableFromConfig(string tomlPath, string connectionName) return null; } - var tomlContent = _fileOperations.ReadAllText(tomlPath, UnixOperations.ValidateFileWhenReadIsAccessedOnlyByItsOwner) ?? string.Empty; + var tomlContent = _fileOperations.ReadAllText(tomlPath, ValidateFilePermissions) ?? string.Empty; var toml = Toml.ToModel(tomlContent); if (string.IsNullOrEmpty(connectionName)) { @@ -152,5 +152,21 @@ private string ResolveConnectionTomlFile() var tomlPath = Path.Combine(tomlFolder, "connections.toml"); return tomlPath; } + + + internal static void ValidateFilePermissions(UnixStream stream) + { + var allowedPermissions = new[] + { + FileAccessPermissions.UserRead | FileAccessPermissions.UserWrite, + FileAccessPermissions.UserRead + }; + if (stream.OwnerUser.UserId != Syscall.geteuid()) + throw new SecurityException("Attempting to read a file not owned by the effective user of the current process"); + if (stream.OwnerGroup.GroupId != Syscall.getegid()) + throw new SecurityException("Attempting to read a file not owned by the effective group of the current process"); + if (!(allowedPermissions.Any(a => stream.FileAccessPermissions == a))) + throw new SecurityException("Attempting to read a file with too broad permissions assigned"); + } } } diff --git a/Snowflake.Data/Core/Tools/UnixOperations.cs b/Snowflake.Data/Core/Tools/UnixOperations.cs index b09d02973..b1a6a196e 100644 --- a/Snowflake.Data/Core/Tools/UnixOperations.cs +++ b/Snowflake.Data/Core/Tools/UnixOperations.cs @@ -63,40 +63,14 @@ public void WriteAllText(string path, string content, Action validat { var fileInfo = new UnixFileInfo(path: path); - using (var handle = fileInfo.OpenRead()) + using (var handle = fileInfo.Open(FileMode.OpenOrCreate, FileAccess.ReadWrite, FilePermissions.S_IWUSR | FilePermissions.S_IRUSR)) { validator?.Invoke(handle); + using (var streamWriter = new StreamWriter(handle, Encoding.UTF8)) + { + streamWriter.Write(content); + } } - File.WriteAllText(path, content); - } - - internal static void ValidateFileWhenReadIsAccessedOnlyByItsOwner(UnixStream stream) - { - var allowedPermissions = new[] - { - FileAccessPermissions.UserRead | FileAccessPermissions.UserWrite, - FileAccessPermissions.UserRead - }; - if (stream.OwnerUser.UserId != Syscall.geteuid()) - throw new SecurityException("Attempting to read a file not owned by the effective user of the current process"); - if (stream.OwnerGroup.GroupId != Syscall.getegid()) - throw new SecurityException("Attempting to read a file not owned by the effective group of the current process"); - if (!(allowedPermissions.Any(a => stream.FileAccessPermissions == a))) - throw new SecurityException("Attempting to read a file with too broad permissions assigned"); - } - - internal static void ValidateFileWhenWriteIsAccessedOnlyByItsOwner(UnixStream stream) - { - var allowedPermissions = new[] - { - FileAccessPermissions.UserRead | FileAccessPermissions.UserWrite - }; - if (stream.OwnerUser.UserId != Syscall.geteuid()) - throw new SecurityException("Attempting to write a file not owned by the effective user of the current process"); - if (stream.OwnerGroup.GroupId != Syscall.getegid()) - throw new SecurityException("Attempting to write a file not owned by the effective group of the current process"); - if (!(allowedPermissions.Any(a => stream.FileAccessPermissions == a))) - throw new SecurityException("Attempting to write a file with too broad permissions assigned"); } } }