Skip to content

Commit 393aad3

Browse files
sfc-gh-knozderkosfc-gh-ext-simba-lf
sfc-gh-knozderko
and
sfc-gh-ext-simba-lf
authored
SNOW-1156046: Fix race condition with easy logging config (#1156)
Co-authored-by: sfc-gh-ext-simba-lf <[email protected]>
1 parent 35c4e2f commit 393aad3

File tree

9 files changed

+150
-80
lines changed

9 files changed

+150
-80
lines changed

‎Snowflake.Data.Tests/Mock/MockUnixOperations.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ public override bool CheckDirectoryHasAnyOfPermissions(string path, FileAccessPe
1616
return DirectoryPermissionsReturn;
1717
}
1818

19-
public override bool CheckFileHasAnyOfPermissions(string path, FileAccessPermissions permissions)
19+
public override bool CheckFileHasAnyOfPermissions(FileAccessPermissions actualPermissions , FileAccessPermissions expectedPermissions)
2020
{
2121
return FilePermissionsReturn;
2222
}

‎Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigFinderTest.cs‎

Lines changed: 1 addition & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
using System;
22
using System.IO;
3-
using System.Runtime.InteropServices;
4-
using Mono.Unix;
53
using Moq;
64
using NUnit.Framework;
75
using Snowflake.Data.Configuration;
@@ -22,9 +20,6 @@ public class EasyLoggingConfigFinderTest
2220
[ThreadStatic]
2321
private static Mock<FileOperations> t_fileOperations;
2422

25-
[ThreadStatic]
26-
private static Mock<UnixOperations> t_unixOperations;
27-
2823
[ThreadStatic]
2924
private static Mock<EnvironmentOperations> t_environmentOperations;
3025

@@ -35,9 +30,8 @@ public class EasyLoggingConfigFinderTest
3530
public void Setup()
3631
{
3732
t_fileOperations = new Mock<FileOperations>();
38-
t_unixOperations = new Mock<UnixOperations>();
3933
t_environmentOperations = new Mock<EnvironmentOperations>();
40-
t_finder = new EasyLoggingConfigFinder(t_fileOperations.Object, t_unixOperations.Object, t_environmentOperations.Object);
34+
t_finder = new EasyLoggingConfigFinder(t_fileOperations.Object, t_environmentOperations.Object);
4135
MockHomeDirectory();
4236
MockExecutionDirectory();
4337
}
@@ -115,26 +109,6 @@ public void TestThatTakesFilePathFromHomeDirectoryWhenNoOtherWaysPossible()
115109
Assert.AreEqual(s_homeConfigFilePath, filePath);
116110
}
117111

118-
[Test]
119-
public void TestThatConfigFileIsNotUsedIfOthersCanModifyTheConfigFile()
120-
{
121-
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
122-
{
123-
Assert.Ignore("skip test on Windows");
124-
}
125-
126-
// arrange
127-
MockFileOnHomePath();
128-
MockHasFlagReturnsTrue();
129-
130-
// act
131-
var thrown = Assert.Throws<Exception>(() => t_finder.FindConfigFilePath(null));
132-
133-
// assert
134-
Assert.IsNotNull(thrown);
135-
Assert.AreEqual(thrown.Message, $"Error due to other users having permission to modify the config file: {s_homeConfigFilePath}");
136-
}
137-
138112
[Test]
139113
public void TestThatReturnsNullIfNoWayOfGettingTheFile()
140114
{
@@ -188,14 +162,6 @@ public void TestThatDoesNotFailWhenHomeDirectoryDoesNotExist()
188162
t_environmentOperations.Verify(e => e.GetFolderPath(Environment.SpecialFolder.UserProfile), Times.Once);
189163
}
190164

191-
private static void MockHasFlagReturnsTrue()
192-
{
193-
t_unixOperations
194-
.Setup(f => f.CheckFileHasAnyOfPermissions(s_homeConfigFilePath,
195-
It.Is<FileAccessPermissions>(p => p.Equals(FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite))))
196-
.Returns(true);
197-
}
198-
199165
private static void MockHomeDirectory()
200166
{
201167
t_environmentOperations

‎Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigParserTest.cs‎

Lines changed: 78 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
using System;
22
using System.Collections.Generic;
33
using System.IO;
4+
using Mono.Unix;
5+
using Moq;
46
using NUnit.Framework;
57
using Snowflake.Data.Configuration;
8+
using Snowflake.Data.Core.Tools;
69
using static Snowflake.Data.Tests.UnitTests.Configuration.EasyLoggingConfigGenerator;
710

811
namespace Snowflake.Data.Tests.UnitTests.Configuration
@@ -89,7 +92,7 @@ public void TestThatFailsWhenTheFileDoesNotExist()
8992

9093
// assert
9194
Assert.IsNotNull(thrown);
92-
Assert.AreEqual("Finding easy logging configuration failed", thrown.Message);
95+
Assert.IsTrue(thrown.Message.Contains("Finding easy logging configuration failed"));
9396
}
9497

9598
[Test, TestCaseSource(nameof(WrongConfigFiles))]
@@ -102,7 +105,80 @@ public void TestThatFailsIfMissingOrInvalidRequiredFields(string filePath)
102105
var thrown = Assert.Throws<Exception>(() => parser.Parse(filePath));
103106
// assert
104107
Assert.IsNotNull(thrown);
105-
Assert.IsTrue(thrown.Message == "Parsing easy logging configuration failed");
108+
Assert.AreEqual(thrown.Message, "Parsing easy logging configuration failed");
109+
}
110+
111+
[Test]
112+
[Platform(Exclude = "Win")]
113+
public void TestThatConfigFileIsNotUsedIfOthersCanModifyTheConfigFile()
114+
{
115+
// arrange
116+
var unixOperations = new Mock<UnixOperations>();
117+
var configFilePath = CreateConfigTempFile(s_workingDirectory, null);
118+
var stream = new UnixFileInfo(configFilePath).OpenRead();
119+
var parser = new EasyLoggingConfigParser(unixOperations.Object);
120+
unixOperations
121+
.Setup(u => u.CheckFileHasAnyOfPermissions(stream.FileAccessPermissions,
122+
It.Is<FileAccessPermissions>(p => p.Equals(FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite))))
123+
.Returns(true);
124+
unixOperations
125+
.Setup(u => u.GetCurrentUserId())
126+
.Returns(stream.OwnerUserId);
127+
unixOperations
128+
.Setup(u => u.GetCurrentGroupId())
129+
.Returns(stream.OwnerGroupId);
130+
131+
// act
132+
var thrown = Assert.Throws<Exception>(() => parser.Parse(configFilePath));
133+
134+
// assert
135+
Assert.IsNotNull(thrown);
136+
Assert.AreEqual(thrown.Message, "Finding easy logging configuration failed: Error due to other users having permission to modify the config file");
137+
}
138+
139+
[Test]
140+
[Platform(Exclude = "Win")]
141+
public void TestThatConfigFileIsNotUsedIfUserDoesNotOwnConfigFile()
142+
{
143+
// arrange
144+
var unixOperations = new Mock<UnixOperations>();
145+
var configFilePath = CreateConfigTempFile(s_workingDirectory, null);
146+
var stream = new UnixFileInfo(configFilePath).OpenRead();
147+
var parser = new EasyLoggingConfigParser(unixOperations.Object);
148+
unixOperations
149+
.Setup(u => u.GetCurrentUserId())
150+
.Returns(stream.OwnerUserId - 1);
151+
152+
// act
153+
var thrown = Assert.Throws<Exception>(() => parser.Parse(configFilePath));
154+
155+
// assert
156+
Assert.IsNotNull(thrown);
157+
Assert.AreEqual(thrown.Message, "Finding easy logging configuration failed: Error due to user not having ownership of the config file");
158+
}
159+
160+
[Test]
161+
[Platform(Exclude = "Win")]
162+
public void TestThatConfigFileIsNotUsedIfGroupDoesNotOwnConfigFile()
163+
{
164+
// arrange
165+
var unixOperations = new Mock<UnixOperations>();
166+
var configFilePath = CreateConfigTempFile(s_workingDirectory, null);
167+
var stream = new UnixFileInfo(configFilePath).OpenRead();
168+
var parser = new EasyLoggingConfigParser(unixOperations.Object);
169+
unixOperations
170+
.Setup(u => u.GetCurrentUserId())
171+
.Returns(stream.OwnerUserId);
172+
unixOperations
173+
.Setup(u => u.GetCurrentGroupId())
174+
.Returns(stream.OwnerGroupId - 1);
175+
176+
// act
177+
var thrown = Assert.Throws<Exception>(() => parser.Parse(configFilePath));
178+
179+
// assert
180+
Assert.IsNotNull(thrown);
181+
Assert.AreEqual(thrown.Message, "Finding easy logging configuration failed: Error due to group not having ownership of the config file");
106182
}
107183

108184
public static IEnumerable<string> ConfigFilesWithoutValues()

‎Snowflake.Data.Tests/UnitTests/Tools/FileOperationsTest.cs‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -155,11 +155,19 @@ public void TestFileIsNotSecureWhenNotOwnedByCurrentUser()
155155
{
156156
// arrange
157157
var absolutePath = Path.Combine(s_workingDirectory, s_fileName);
158-
var mockUnixOperations = new MockUnixOperations{ CurrentUserId = 1, FileOwnerId = 2 };
159-
var fileOps = new FileOperations(mockUnixOperations);
158+
File.Create(absolutePath);
159+
try
160+
{
161+
var mockUnixOperations = new MockUnixOperations { CurrentUserId = 1, FileOwnerId = 2 };
162+
var fileOps = new FileOperations(mockUnixOperations);
160163

161-
// act and assert
162-
Assert.IsFalse(fileOps.IsFileSafe(absolutePath));
164+
// act and assert
165+
Assert.IsFalse(fileOps.IsFileSafe(absolutePath));
166+
}
167+
finally
168+
{
169+
File.Delete(absolutePath);
170+
}
163171
}
164172

165173
[Test]

‎Snowflake.Data.Tests/UnitTests/Tools/UnixOperationsTest.cs‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,10 @@ public void TestDetectGroupOrOthersWritablePermissions(
5050
var readWriteUserPermissions = FileAccessPermissions.UserRead | FileAccessPermissions.UserWrite;
5151
var filePermissions = readWriteUserPermissions | groupOrOthersWritablePermissions | groupNotWritablePermissions | otherNotWritablePermissions;
5252
Syscall.chmod(filePath, (FilePermissions) filePermissions);
53+
var fileInfo = new UnixFileInfo(filePath);
5354

5455
// act
55-
var result = s_unixOperations.CheckFileHasAnyOfPermissions(filePath, FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite);
56+
var result = s_unixOperations.CheckFileHasAnyOfPermissions(fileInfo.FileAccessPermissions, FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite);
5657

5758
// assert
5859
Assert.IsTrue(result);
@@ -68,9 +69,10 @@ public void TestDetectGroupOrOthersNotWritablePermissions(
6869
var filePath = CreateConfigTempFile(s_workingDirectory, "random text");
6970
var filePermissions = userPermissions | groupNotWritablePermissions | otherNotWritablePermissions;
7071
Syscall.chmod(filePath, (FilePermissions) filePermissions);
72+
var fileInfo = new UnixFileInfo(filePath);
7173

7274
// act
73-
var result = s_unixOperations.CheckFileHasAnyOfPermissions(filePath, FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite);
75+
var result = s_unixOperations.CheckFileHasAnyOfPermissions(fileInfo.FileAccessPermissions, FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite);
7476

7577
// assert
7678
Assert.IsFalse(result);
@@ -158,7 +160,8 @@ public void TestCreateFileWithUserRwPermissions()
158160
s_unixOperations.CreateFileWithPermissions(filePath, FileAccessPermissions.UserRead | FileAccessPermissions.UserWrite);
159161

160162
// assert
161-
var result = s_unixOperations.CheckFileHasAnyOfPermissions(filePath, FileAccessPermissions.AllPermissions & ~(FileAccessPermissions.UserRead | FileAccessPermissions.UserWrite));
163+
var fileInfo = new UnixFileInfo(filePath);
164+
var result = s_unixOperations.CheckFileHasAnyOfPermissions(fileInfo.FileAccessPermissions, FileAccessPermissions.AllPermissions & ~(FileAccessPermissions.UserRead | FileAccessPermissions.UserWrite));
162165
Assert.IsFalse(result);
163166
}
164167

@@ -173,7 +176,8 @@ public void TestCreateDirectoryWithUserRwxPermissions()
173176
s_unixOperations.CreateDirectoryWithPermissions(dirPath, FileAccessPermissions.UserReadWriteExecute);
174177

175178
// assert
176-
var result = s_unixOperations.CheckFileHasAnyOfPermissions(dirPath, FileAccessPermissions.AllPermissions & ~FileAccessPermissions.UserReadWriteExecute);
179+
var dirInfo = new UnixDirectoryInfo(dirPath);
180+
var result = s_unixOperations.CheckFileHasAnyOfPermissions(dirInfo.FileAccessPermissions, FileAccessPermissions.AllPermissions & ~FileAccessPermissions.UserReadWriteExecute);
177181
Assert.IsFalse(result);
178182
}
179183

@@ -186,7 +190,8 @@ public void TestNestedDir()
186190
s_unixOperations.CreateDirectoryWithPermissions(dirPath, FileAccessPermissions.UserReadWriteExecute);
187191

188192
// act
189-
var result = s_unixOperations.CheckFileHasAnyOfPermissions(dirPath, FileAccessPermissions.AllPermissions & ~FileAccessPermissions.UserReadWriteExecute);
193+
var dirInfo = new UnixDirectoryInfo(dirPath);
194+
var result = s_unixOperations.CheckFileHasAnyOfPermissions(dirInfo.FileAccessPermissions, FileAccessPermissions.AllPermissions & ~FileAccessPermissions.UserReadWriteExecute);
190195

191196
// assert
192197
Assert.IsFalse(result);

‎Snowflake.Data/Configuration/EasyLoggingConfigFinder.cs‎

Lines changed: 2 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
using System;
22
using System.IO;
3-
using System.Runtime.InteropServices;
4-
using Mono.Unix;
53
using Snowflake.Data.Core.Tools;
64
using Snowflake.Data.Log;
75

@@ -15,15 +13,13 @@ internal class EasyLoggingConfigFinder
1513
internal const string ClientConfigEnvironmentName = "SF_CLIENT_CONFIG_FILE";
1614

1715
private readonly FileOperations _fileOperations;
18-
private readonly UnixOperations _unixOperations;
1916
private readonly EnvironmentOperations _environmentOperations;
2017

21-
public static readonly EasyLoggingConfigFinder Instance = new EasyLoggingConfigFinder(FileOperations.Instance, UnixOperations.Instance, EnvironmentOperations.Instance);
18+
public static readonly EasyLoggingConfigFinder Instance = new EasyLoggingConfigFinder(FileOperations.Instance, EnvironmentOperations.Instance);
2219

23-
internal EasyLoggingConfigFinder(FileOperations fileOperations, UnixOperations unixFileOperations, EnvironmentOperations environmentOperations)
20+
internal EasyLoggingConfigFinder(FileOperations fileOperations, EnvironmentOperations environmentOperations)
2421
{
2522
_fileOperations = fileOperations;
26-
_unixOperations = unixFileOperations;
2723
_environmentOperations = environmentOperations;
2824
}
2925

@@ -37,10 +33,6 @@ public virtual string FindConfigFilePath(string configFilePathFromConnectionStri
3733
?? GetFilePathEnvironmentVariable()
3834
?? GetFilePathFromDriverLocation()
3935
?? GetFilePathFromHomeDirectory();
40-
if (configFilePath != null)
41-
{
42-
CheckIfValidPermissions(configFilePath);
43-
}
4436
return configFilePath;
4537
}
4638

@@ -96,19 +88,5 @@ private string OnlyIfFileExists(string filePath, string directoryDescription)
9688
}
9789
return null;
9890
}
99-
100-
private void CheckIfValidPermissions(string filePath)
101-
{
102-
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
103-
return;
104-
105-
// Check if others have permissions to modify the file and fail if so
106-
if (_unixOperations.CheckFileHasAnyOfPermissions(filePath, FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite))
107-
{
108-
var errorMessage = $"Error due to other users having permission to modify the config file: {filePath}";
109-
s_logger.Error(errorMessage);
110-
throw new Exception(errorMessage);
111-
}
112-
}
11391
}
11492
}

‎Snowflake.Data/Configuration/EasyLoggingConfigParser.cs‎

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
using System;
22
using System.Collections.Generic;
3-
using System.IO;
43
using System.Linq;
54
using System.Reflection;
5+
using Mono.Unix;
6+
using System.Security;
67
using Newtonsoft.Json;
78
using Newtonsoft.Json.Linq;
9+
using Snowflake.Data.Core.Tools;
810
using Snowflake.Data.Log;
911

1012
namespace Snowflake.Data.Configuration
@@ -13,7 +15,16 @@ internal class EasyLoggingConfigParser
1315
{
1416
private static readonly SFLogger s_logger = SFLoggerFactory.GetLogger<EasyLoggingConfigParser>();
1517

16-
public static readonly EasyLoggingConfigParser Instance = new EasyLoggingConfigParser();
18+
private readonly UnixOperations _unixOperations;
19+
20+
public static readonly EasyLoggingConfigParser Instance = new EasyLoggingConfigParser(UnixOperations.Instance);
21+
22+
internal EasyLoggingConfigParser(UnixOperations unixOperations)
23+
{
24+
_unixOperations = unixOperations;
25+
}
26+
27+
internal EasyLoggingConfigParser() : this(UnixOperations.Instance) { }
1728

1829
public virtual ClientConfig Parse(string filePath)
1930
{
@@ -29,11 +40,11 @@ private string TryToReadFile(string filePath)
2940
}
3041
try
3142
{
32-
return File.ReadAllText(filePath);
43+
return FileOperations.Instance.ReadAllText(filePath, CheckIfValidPermissions);
3344
}
3445
catch (Exception e)
3546
{
36-
var errorMessage = "Finding easy logging configuration failed";
47+
var errorMessage = $"Finding easy logging configuration failed: {e.Message}";
3748
s_logger.Error(errorMessage, e);
3849
throw new Exception(errorMessage);
3950
}
@@ -76,5 +87,32 @@ private void CheckForUnknownFields(string fileContent)
7687
.ToList()
7788
.ForEach(unknownKey => s_logger.Warn($"Unknown field from config: {unknownKey.Name}"));
7889
}
90+
91+
private void CheckIfValidPermissions(UnixStream stream)
92+
{
93+
// Check user ownership of file
94+
if (stream.OwnerUserId != _unixOperations.GetCurrentUserId())
95+
{
96+
var errorMessage = $"Error due to user not having ownership of the config file";
97+
s_logger.Error(errorMessage);
98+
throw new SecurityException(errorMessage);
99+
}
100+
101+
// Check group ownership of file
102+
if (stream.OwnerGroupId != _unixOperations.GetCurrentGroupId())
103+
{
104+
var errorMessage = $"Error due to group not having ownership of the config file";
105+
s_logger.Error(errorMessage);
106+
throw new SecurityException(errorMessage);
107+
}
108+
109+
// Check if others have permissions to modify the file and fail if so
110+
if (_unixOperations.CheckFileHasAnyOfPermissions(stream.FileAccessPermissions, FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite))
111+
{
112+
var errorMessage = $"Error due to other users having permission to modify the config file";
113+
s_logger.Error(errorMessage);
114+
throw new SecurityException(errorMessage);
115+
}
116+
}
79117
}
80118
}

0 commit comments

Comments
 (0)