Skip to content

Commit c72f84b

Browse files
committed
Fix dir perm verification
1 parent e3b1c37 commit c72f84b

4 files changed

+45
-58
lines changed

os_specific_posix.go

-8
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,6 @@ import (
88
"syscall"
99
)
1010

11-
func providePathOwner(filepath string) (uint32, error) {
12-
info, err := os.Stat(filepath)
13-
if err != nil {
14-
return 0, err
15-
}
16-
return provideOwnerFromStat(info, filepath)
17-
}
18-
1911
func provideFileOwner(file *os.File) (uint32, error) {
2012
info, err := file.Stat()
2113
if err != nil {

os_specific_windows.go

-4
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,6 @@ import (
77
"os"
88
)
99

10-
func providePathOwner(filepath string) (uint32, error) {
11-
return 0, errors.New("providePathOwner is unsupported on windows")
12-
}
13-
1410
func provideFileOwner(file *os.File) (uint32, error) {
1511
return 0, errors.New("provideFileOwner is unsupported on windows")
1612
}

secure_storage_manager.go

+22-45
Original file line numberDiff line numberDiff line change
@@ -180,14 +180,29 @@ func (ssm *fileBasedSecureStorageManager) withLock(action func(cacheFile *os.Fil
180180
func (ssm *fileBasedSecureStorageManager) withCacheFile(action func(*os.File)) {
181181
cacheFile, err := os.OpenFile(ssm.credFilePath(), os.O_CREATE|os.O_RDWR, 0600)
182182
if err != nil {
183-
logger.Warn("cannot access %v. %v", ssm.credFilePath(), err)
183+
logger.Warnf("cannot access %v. %v", ssm.credFilePath(), err)
184184
return
185185
}
186186
defer func(file *os.File) {
187187
if err := file.Close(); err != nil {
188188
logger.Warnf("cannot release file descriptor for %v. %v", ssm.credFilePath(), err)
189189
}
190190
}(cacheFile)
191+
192+
cacheDir, err := os.Open(ssm.credDirPath)
193+
if err != nil {
194+
logger.Warnf("cannot access %v. %v", ssm.credDirPath, err)
195+
}
196+
197+
if err := ssm.ensurePermissionsAndOwner(cacheFile, 0600); err != nil {
198+
logger.Warnf("failed to ensure permission for temporary cache file. %v", err)
199+
return
200+
}
201+
if err := ssm.ensurePermissionsAndOwner(cacheDir, 0700|os.ModeDir); err != nil {
202+
logger.Warnf("failed to ensure permission for temporary cache dir. %v", err)
203+
return
204+
}
205+
191206
action(cacheFile)
192207
}
193208

@@ -298,68 +313,34 @@ func (ssm *fileBasedSecureStorageManager) credFilePath() string {
298313
return filepath.Join(ssm.credDirPath, credCacheFileName)
299314
}
300315

301-
func (ssm *fileBasedSecureStorageManager) ensurePermissions(cacheFile *os.File) error {
302-
dirInfo, err := os.Stat(ssm.credDirPath)
303-
if err != nil {
304-
return err
305-
}
306-
307-
if dirInfo.Mode().Perm() != 0700&os.ModePerm {
308-
return fmt.Errorf("incorrect permissions(%o, expected 700) for %s", dirInfo.Mode().Perm(), ssm.credDirPath)
309-
}
310-
311-
fileInfo, err := cacheFile.Stat()
316+
func (ssm *fileBasedSecureStorageManager) ensurePermissionsAndOwner(f *os.File, expectedMode os.FileMode) error {
317+
fileInfo, err := f.Stat()
312318
if err != nil {
313319
return err
314320
}
315321

316-
if fileInfo.Mode().Perm() != 0600&os.ModePerm {
317-
return fmt.Errorf("incorrect permissions(%v, expected 600) for credential file", fileInfo.Mode().Perm())
322+
if fileInfo.Mode() != expectedMode {
323+
return fmt.Errorf("incorrect permissions(%v, expected %v) for credential file", fileInfo.Mode(), expectedMode)
318324
}
319325

320-
return nil
321-
}
322-
323-
func (ssm *fileBasedSecureStorageManager) ensureOwnerForDir(filePath string) error {
324-
ownerUID, err := providePathOwner(filePath)
325-
if err != nil && !errors.Is(err, os.ErrNotExist) {
326-
return err
327-
}
328-
return ssm.ensureOwner(ownerUID)
329-
}
330-
331-
func (ssm *fileBasedSecureStorageManager) ensureOwnerForFile(file *os.File) error {
332-
ownerUID, err := provideFileOwner(file)
326+
ownerUID, err := provideFileOwner(f)
333327
if err != nil && !errors.Is(err, os.ErrNotExist) {
334328
return err
335329
}
336-
return ssm.ensureOwner(ownerUID)
337-
}
338-
339-
func (ssm *fileBasedSecureStorageManager) ensureOwner(ownerID uint32) error {
340330
currentUser, err := user.Current()
341331
if err != nil {
342332
return err
343333
}
344334
if errors.Is(err, os.ErrNotExist) {
345335
return nil
346336
}
347-
if strconv.Itoa(int(ownerID)) != currentUser.Uid {
337+
if strconv.Itoa(int(ownerUID)) != currentUser.Uid {
348338
return errors.New("incorrect owner of " + ssm.credDirPath)
349339
}
350340
return nil
351341
}
352342

353343
func (ssm *fileBasedSecureStorageManager) readTemporaryCacheFile(cacheFile *os.File) (map[string]any, error) {
354-
if err := ssm.ensurePermissions(cacheFile); err != nil {
355-
return map[string]any{}, fmt.Errorf("failed to ensure permission for temporary cache file. %v", err)
356-
}
357-
if err := ssm.ensureOwnerForDir(ssm.credDirPath); err != nil {
358-
return map[string]any{}, fmt.Errorf("failed to ensure owner for %v. %v", ssm.credDirPath, err)
359-
}
360-
if err := ssm.ensureOwnerForFile(cacheFile); err != nil {
361-
return map[string]any{}, fmt.Errorf("failed to ensure owner for %v. %v", ssm.credFilePath(), err)
362-
}
363344

364345
jsonData, err := io.ReadAll(cacheFile)
365346
if err != nil {
@@ -407,10 +388,6 @@ func (ssm *fileBasedSecureStorageManager) deleteCredential(tokenSpec *secureToke
407388
}
408389

409390
func (ssm *fileBasedSecureStorageManager) writeTemporaryCacheFile(cache map[string]any, cacheFile *os.File) error {
410-
if err := ssm.ensureOwnerForDir(ssm.credDirPath); err != nil {
411-
return err
412-
}
413-
414391
bytes, err := json.Marshal(cache)
415392
if err != nil {
416393
return fmt.Errorf("failed to marshal credential cache map. %w", err)

secure_storage_manager_test.go

+23-1
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) {
6969
defer credCacheDirEnvOverride.rollback()
7070
ssm, err := newFileBasedSecureStorageManager()
7171
assertNilF(t, err)
72-
72+
7373
t.Run("store single token", func(t *testing.T) {
7474
tokenSpec := newMfaTokenSpec("host.com", "johndoe")
7575
cred := "token123"
@@ -173,6 +173,28 @@ func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) {
173173
tokens := m["tokens"].(map[string]any)
174174
assertEqualE(t, tokens[cacheKey], "initialValue")
175175
})
176+
177+
t.Run("should not modify file if its dir has wrong permission", func(t *testing.T) {
178+
tokenSpec := newMfaTokenSpec("somehost.com", "someUser")
179+
ssm.setCredential(tokenSpec, "initialValue")
180+
assertEqualE(t, ssm.getCredential(tokenSpec), "initialValue")
181+
err = os.Chmod(ssm.credDirPath, 0777)
182+
assertNilF(t, err)
183+
defer func() {
184+
assertNilE(t, os.Chmod(ssm.credDirPath, 0700))
185+
}()
186+
ssm.setCredential(tokenSpec, "newValue")
187+
assertEqualE(t, ssm.getCredential(tokenSpec), "")
188+
fileContent, err := os.ReadFile(ssm.credFilePath())
189+
assertNilF(t, err)
190+
var m map[string]any
191+
err = json.Unmarshal(fileContent, &m)
192+
assertNilF(t, err)
193+
cacheKey, err := tokenSpec.buildKey()
194+
assertNilF(t, err)
195+
tokens := m["tokens"].(map[string]any)
196+
assertEqualE(t, tokens[cacheKey], "initialValue")
197+
})
176198
}
177199

178200
func TestSetAndGetCredentialMfa(t *testing.T) {

0 commit comments

Comments
 (0)