Fix dir perm verification

Author: sfc-gh-pfus

secure_storage_manager.go

@@ -180,14 +180,29 @@ func (ssm *fileBasedSecureStorageManager) withLock(action func(cacheFile *os.Fil
 func (ssm *fileBasedSecureStorageManager) withCacheFile(action func(*os.File)) {
 	cacheFile, err := os.OpenFile(ssm.credFilePath(), os.O_CREATE|os.O_RDWR, 0600)
 	if err != nil {
-		logger.Warn("cannot access %v. %v", ssm.credFilePath(), err)
+		logger.Warnf("cannot access %v. %v", ssm.credFilePath(), err)
 		return
 	}
 	defer func(file *os.File) {
 		if err := file.Close(); err != nil {
 			logger.Warnf("cannot release file descriptor for %v. %v", ssm.credFilePath(), err)
 		}
 	}(cacheFile)
+
+	cacheDir, err := os.Open(ssm.credDirPath)
+	if err != nil {
+		logger.Warnf("cannot access %v. %v", ssm.credDirPath, err)
+	}
+
+	if err := ssm.ensurePermissionsAndOwner(cacheFile, 0600); err != nil {
+		logger.Warnf("failed to ensure permission for temporary cache file. %v", err)
+		return
+	}
+	if err := ssm.ensurePermissionsAndOwner(cacheDir, 0700|os.ModeDir); err != nil {
+		logger.Warnf("failed to ensure permission for temporary cache dir. %v", err)
+		return
+	}
+
 	action(cacheFile)
 }
 
@@ -298,68 +313,34 @@ func (ssm *fileBasedSecureStorageManager) credFilePath() string {
 	return filepath.Join(ssm.credDirPath, credCacheFileName)
 }
 
-func (ssm *fileBasedSecureStorageManager) ensurePermissions(cacheFile *os.File) error {
-	dirInfo, err := os.Stat(ssm.credDirPath)
-	if err != nil {
-		return err
-	}
-
-	if dirInfo.Mode().Perm() != 0700&os.ModePerm {
-		return fmt.Errorf("incorrect permissions(%o, expected 700) for %s", dirInfo.Mode().Perm(), ssm.credDirPath)
-	}
-
-	fileInfo, err := cacheFile.Stat()
+func (ssm *fileBasedSecureStorageManager) ensurePermissionsAndOwner(f *os.File, expectedMode os.FileMode) error {
+	fileInfo, err := f.Stat()
 	if err != nil {
 		return err
 	}
 
-	if fileInfo.Mode().Perm() != 0600&os.ModePerm {
-		return fmt.Errorf("incorrect permissions(%v, expected 600) for credential file", fileInfo.Mode().Perm())
+	if fileInfo.Mode() != expectedMode {
+		return fmt.Errorf("incorrect permissions(%v, expected %v) for credential file", fileInfo.Mode(), expectedMode)
 	}
 
-	return nil
-}
-
-func (ssm *fileBasedSecureStorageManager) ensureOwnerForDir(filePath string) error {
-	ownerUID, err := providePathOwner(filePath)
-	if err != nil && !errors.Is(err, os.ErrNotExist) {
-		return err
-	}
-	return ssm.ensureOwner(ownerUID)
-}
-
-func (ssm *fileBasedSecureStorageManager) ensureOwnerForFile(file *os.File) error {
-	ownerUID, err := provideFileOwner(file)
+	ownerUID, err := provideFileOwner(f)
 	if err != nil && !errors.Is(err, os.ErrNotExist) {
 		return err
 	}
-	return ssm.ensureOwner(ownerUID)
-}
-
-func (ssm *fileBasedSecureStorageManager) ensureOwner(ownerID uint32) error {
 	currentUser, err := user.Current()
 	if err != nil {
 		return err
 	}
 	if errors.Is(err, os.ErrNotExist) {
 		return nil
 	}
-	if strconv.Itoa(int(ownerID)) != currentUser.Uid {
+	if strconv.Itoa(int(ownerUID)) != currentUser.Uid {
 		return errors.New("incorrect owner of " + ssm.credDirPath)
 	}
 	return nil
 }
 
 func (ssm *fileBasedSecureStorageManager) readTemporaryCacheFile(cacheFile *os.File) (map[string]any, error) {
-	if err := ssm.ensurePermissions(cacheFile); err != nil {
-		return map[string]any{}, fmt.Errorf("failed to ensure permission for temporary cache file. %v", err)
-	}
-	if err := ssm.ensureOwnerForDir(ssm.credDirPath); err != nil {
-		return map[string]any{}, fmt.Errorf("failed to ensure owner for %v. %v", ssm.credDirPath, err)
-	}
-	if err := ssm.ensureOwnerForFile(cacheFile); err != nil {
-		return map[string]any{}, fmt.Errorf("failed to ensure owner for %v. %v", ssm.credFilePath(), err)
-	}
 
 	jsonData, err := io.ReadAll(cacheFile)
 	if err != nil {
@@ -407,10 +388,6 @@ func (ssm *fileBasedSecureStorageManager) deleteCredential(tokenSpec *secureToke
 }
 
 func (ssm *fileBasedSecureStorageManager) writeTemporaryCacheFile(cache map[string]any, cacheFile *os.File) error {
-	if err := ssm.ensureOwnerForDir(ssm.credDirPath); err != nil {
-		return err
-	}
-
 	bytes, err := json.Marshal(cache)
 	if err != nil {
 		return fmt.Errorf("failed to marshal credential cache map. %w", err)

secure_storage_manager_test.go

@@ -70,6 +70,8 @@ func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) {
 	ssm, err := newFileBasedSecureStorageManager()
 	assertNilF(t, err)
 
+	logger.SetLogLevel("debug")
+
 	t.Run("store single token", func(t *testing.T) {
 		tokenSpec := newMfaTokenSpec("host.com", "johndoe")
 		cred := "token123"
@@ -173,6 +175,28 @@ func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) {
 		tokens := m["tokens"].(map[string]any)
 		assertEqualE(t, tokens[cacheKey], "initialValue")
 	})
+
+	t.Run("should not modify file if its dir has wrong permission", func(t *testing.T) {
+		tokenSpec := newMfaTokenSpec("somehost.com", "someUser")
+		ssm.setCredential(tokenSpec, "initialValue")
+		assertEqualE(t, ssm.getCredential(tokenSpec), "initialValue")
+		err = os.Chmod(ssm.credDirPath, 0777)
+		assertNilF(t, err)
+		defer func() {
+			assertNilE(t, os.Chmod(ssm.credDirPath, 0700))
+		}()
+		ssm.setCredential(tokenSpec, "newValue")
+		assertEqualE(t, ssm.getCredential(tokenSpec), "")
+		fileContent, err := os.ReadFile(ssm.credFilePath())
+		assertNilF(t, err)
+		var m map[string]any
+		err = json.Unmarshal(fileContent, &m)
+		assertNilF(t, err)
+		cacheKey, err := tokenSpec.buildKey()
+		assertNilF(t, err)
+		tokens := m["tokens"].(map[string]any)
+		assertEqualE(t, tokens[cacheKey], "initialValue")
+	})
 }
 
 func TestSetAndGetCredentialMfa(t *testing.T) {