Skip to content

Commit

Permalink
Applying PR suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
sfc-gh-jmartinezramirez committed Nov 27, 2024
1 parent 9ae4120 commit 2f1c811
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 43 deletions.
6 changes: 3 additions & 3 deletions Snowflake.Data.Tests/UnitTests/Tools/FileOperationsTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -96,7 +96,7 @@ public void TestShouldThrowExceptionIfOtherPermissionsIsSetWhenReadConfiguration
Syscall.chmod(filePath, (FilePermissions)filePermissions);

// act and assert
Assert.Throws<SecurityException>(() => s_fileOperations.ReadAllText(filePath, UnixOperations.ValidateFileWhenReadIsAccessedOnlyByItsOwner),
Assert.Throws<SecurityException>(() => s_fileOperations.ReadAllText(filePath, TomlConnectionBuilder.ValidateFilePermissions),
"Attempting to read a file with too broad permissions assigned");
}

Expand Down
11 changes: 6 additions & 5 deletions Snowflake.Data.Tests/UnitTests/Tools/UnixOperationsTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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))
Expand All @@ -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]
Expand All @@ -139,7 +140,7 @@ public void TestFailIfGroupOrOthersHavePermissionsToFileWhileReadingWithUnixVali
Syscall.chmod(filePath, filePermissions);

// act and assert
Assert.Throws<SecurityException>(() => s_unixOperations.ReadAllText(filePath, UnixOperations.ValidateFileWhenReadIsAccessedOnlyByItsOwner), "Attempting to read a file with too broad permissions assigned");
Assert.Throws<SecurityException>(() => s_unixOperations.ReadAllText(filePath, TomlConnectionBuilder.ValidateFilePermissions), "Attempting to read a file with too broad permissions assigned");
}

[Test]
Expand All @@ -163,7 +164,7 @@ public void TestFailIfGroupOrOthersHavePermissionsToFileWhileWritingWithUnixVali
Syscall.chmod(filePath, filePermissions);

// act and assert
Assert.Throws<SecurityException>(() => s_unixOperations.WriteAllText(filePath, "test", UnixOperations.ValidateFileWhenReadIsAccessedOnlyByItsOwner), "Attempting to write a file with too broad permissions assigned");
Assert.Throws<SecurityException>(() => s_unixOperations.WriteAllText(filePath, "test", SFCredentialManagerFileImpl.ValidateFilePermissions), "Attempting to read or write a file with too broad permissions assigned");
}

public static IEnumerable<FilePermissions> UserPermissions()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, string>;

Expand Down Expand Up @@ -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);
Expand All @@ -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<KeyTokenDict>(contentFile);
}

Expand Down Expand Up @@ -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");

Check warning on line 183 in Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs

View check run for this annotation

Codecov / codecov/patch

Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs#L183

Added line #L183 was not covered by tests
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");

Check warning on line 185 in Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs

View check run for this annotation

Codecov / codecov/patch

Snowflake.Data/Core/CredentialManager/Infrastructure/SFCredentialManagerFileImpl.cs#L185

Added line #L185 was not covered by tests
if (!(allowedPermissions.Any(a => stream.FileAccessPermissions == a)))
throw new SecurityException("Attempting to read or write a file with too broad permissions assigned");
}
}
}
20 changes: 18 additions & 2 deletions Snowflake.Data/Core/TomlConnectionBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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))
{
Expand All @@ -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");
}
}
}
36 changes: 5 additions & 31 deletions Snowflake.Data/Core/Tools/UnixOperations.cs
Original file line number Diff line number Diff line change
Expand Up @@ -63,40 +63,14 @@ public void WriteAllText(string path, string content, Action<UnixStream> 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");
}
}
}

0 comments on commit 2f1c811

Please sign in to comment.