From b574a79f66f16971fce092aec7da6801b591c7eb Mon Sep 17 00:00:00 2001 From: Piotr Fus Date: Thu, 6 Mar 2025 11:46:00 +0100 Subject: [PATCH 01/10] SNOW-1825790 Implement safer file based token cache --- os_specific_posix.go | 21 ++ os_specific_windows.go | 9 + secure_storage_manager.go | 456 +++++++++++++++++++++++---------- secure_storage_manager_test.go | 183 ++++++++++++- util_test.go | 25 ++ 5 files changed, 562 insertions(+), 132 deletions(-) create mode 100644 os_specific_posix.go create mode 100644 os_specific_windows.go diff --git a/os_specific_posix.go b/os_specific_posix.go new file mode 100644 index 000000000..ef7ab7518 --- /dev/null +++ b/os_specific_posix.go @@ -0,0 +1,21 @@ +//go:build darwin || linux + +package gosnowflake + +import ( + "fmt" + "os" + "syscall" +) + +func provideFileOwner(filepath string) (uint32, error) { + info, err := os.Stat(filepath) + if err != nil { + return 0, err + } + nativeStat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return 0, fmt.Errorf("cannot cast file info for %v to *syscall.Stat_t", filepath) + } + return nativeStat.Uid, nil +} diff --git a/os_specific_windows.go b/os_specific_windows.go new file mode 100644 index 000000000..adfc98ee0 --- /dev/null +++ b/os_specific_windows.go @@ -0,0 +1,9 @@ +// go:build windows + +package gosnowflake + +import "errors" + +func provideFileOwner(filepath string) (uint32, error) { + return 0, errors.New("provideFileOwner is unsupported on windows") +} diff --git a/secure_storage_manager.go b/secure_storage_manager.go index 6be71a579..00bddfb1e 100644 --- a/secure_storage_manager.go +++ b/secure_storage_manager.go @@ -3,16 +3,20 @@ package gosnowflake import ( + "crypto/sha256" + "encoding/hex" "encoding/json" + "errors" "fmt" + "github.com/99designs/keyring" "os" + "os/user" "path/filepath" "runtime" + "strconv" "strings" "sync" "time" - - "github.com/99designs/keyring" ) type tokenType string @@ -23,17 +27,27 @@ const ( ) const ( - driverName = "SNOWFLAKE-GO-DRIVER" credCacheDirEnv = "SF_TEMPORARY_CREDENTIAL_CACHE_DIR" - credCacheFileName = "temporary_credential.json" + credCacheFileName = "credential_cache_v1.json" ) +type cacheDirConf struct { + envVar string + pathSegments []string +} + +var defaultLinuxCacheDirConf = []cacheDirConf{ + {envVar: credCacheDirEnv, pathSegments: []string{}}, + {envVar: "XDG_CACHE_DIR", pathSegments: []string{"snowflake"}}, + {envVar: "HOME", pathSegments: []string{".cache", "snowflake"}}, +} + type secureTokenSpec struct { host, user string tokenType tokenType } -func (t *secureTokenSpec) buildKey() string { +func (t *secureTokenSpec) buildKey() (string, error) { return buildCredentialsKey(t.host, t.user, t.tokenType) } @@ -69,181 +83,324 @@ func newSecureStorageManager() secureStorageManager { logger.Debugf("failed to create credentials cache dir. %v", err) return newNoopSecureStorageManager() } - return ssm + return &threadSafeSecureStorageManager{&sync.Mutex{}, ssm} case "darwin", "windows": - return newKeyringBasedSecureStorageManager() + return &threadSafeSecureStorageManager{&sync.Mutex{}, newKeyringBasedSecureStorageManager()} default: + logger.Infof("OS %v does not support credentials cache", runtime.GOOS) return newNoopSecureStorageManager() } } type fileBasedSecureStorageManager struct { - credCacheFilePath string - localCredCache map[string]string - credCacheLock sync.RWMutex + credDirPath string } func newFileBasedSecureStorageManager() (*fileBasedSecureStorageManager, error) { - ssm := &fileBasedSecureStorageManager{ - localCredCache: map[string]string{}, - credCacheLock: sync.RWMutex{}, - } - credCacheDir := ssm.buildCredCacheDirPath() - if err := ssm.createCacheDir(credCacheDir); err != nil { + credDirPath, err := buildCredCacheDirPath(defaultLinuxCacheDirConf) + if err != nil { return nil, err } - credCacheFilePath := filepath.Join(credCacheDir, credCacheFileName) - logger.Infof("Credentials cache path: %v", credCacheFilePath) - ssm.credCacheFilePath = credCacheFilePath + ssm := &fileBasedSecureStorageManager{ + credDirPath: credDirPath, + } return ssm, nil } -func (ssm *fileBasedSecureStorageManager) createCacheDir(credCacheDir string) error { - _, err := os.Stat(credCacheDir) - if os.IsNotExist(err) { - if err = os.MkdirAll(credCacheDir, os.ModePerm); err != nil { - return fmt.Errorf("failed to create cache directory. %v, err: %v", credCacheDir, err) +func lookupCacheDir(envVar string, pathSegments ...string) (string, error) { + envVal := os.Getenv(envVar) + if envVal == "" { + return "", fmt.Errorf("environment variable %s not set", envVar) + } + + fileInfo, err := os.Stat(envVal) + if err != nil { + return "", fmt.Errorf("failed to stat %s=%s, due to %v", envVar, envVal, err) + } + + if !fileInfo.IsDir() { + return "", fmt.Errorf("environment variable %s=%s is not a directory", envVar, envVal) + } + + cacheDir := filepath.Join(envVal, filepath.Join(pathSegments...)) + + if err = os.MkdirAll(cacheDir, os.FileMode(0o755)); err != nil { + return "", err + } + + if err = os.Chmod(cacheDir, os.FileMode(0700)); err != nil { + return "", err + } + + fileInfo, err = os.Stat(cacheDir) + if err != nil { + return "", fmt.Errorf("failed to stat %s=%s, due to %w", envVar, cacheDir, err) + } + + if fileInfo.Mode().Perm()|os.ModeDir != 0o700|os.ModeDir { + return "", fmt.Errorf("incorrect permission to %v", cacheDir) + } + + return cacheDir, nil +} + +func buildCredCacheDirPath(confs []cacheDirConf) (string, error) { + for _, conf := range confs { + path, err := lookupCacheDir(conf.envVar, conf.pathSegments...) + if err != nil { + logger.Debugf("Skipping %s in cache directory lookup due to %v", conf.envVar, err) + } else { + logger.Infof("Using %s as cache directory", path) + return path, nil } - return nil } - return err + + return "", errors.New("no credentials cache directory found") } -func (ssm *fileBasedSecureStorageManager) buildCredCacheDirPath() string { - credCacheDir := os.Getenv(credCacheDirEnv) - if credCacheDir != "" { - return credCacheDir +func (ssm *fileBasedSecureStorageManager) getTokens(data map[string]any) map[string]interface{} { + val, ok := data["tokens"] + emptyMap := map[string]interface{}{} + if !ok { + data["tokens"] = emptyMap + return emptyMap } - home := os.Getenv("HOME") - if home == "" { - logger.Info("HOME is blank") - return "" + + tokens, ok := val.(map[string]interface{}) + if !ok { + data["tokens"] = emptyMap + return emptyMap } - credCacheDir = filepath.Join(home, ".cache", "snowflake") - return credCacheDir + + return tokens } func (ssm *fileBasedSecureStorageManager) setCredential(tokenSpec *secureTokenSpec, value string) { - if value == "" { - logger.Debug("no token provided") - } else { - credentialsKey := tokenSpec.buildKey() - ssm.credCacheLock.Lock() - defer ssm.credCacheLock.Unlock() - ssm.localCredCache[credentialsKey] = value + credentialsKey, err := tokenSpec.buildKey() + if err != nil { + logger.Warn(err) + return + } + err = ssm.lockFile() + if err != nil { + logger.Warnf("Set credential failed. Unable to lock cache. %v", err) + return + } + defer ssm.unlockFile() - j, err := json.Marshal(ssm.localCredCache) + credCache := ssm.readTemporaryCacheFile() + ssm.getTokens(credCache)[credentialsKey] = value + + err = ssm.writeTemporaryCacheFile(credCache) + if err != nil { + logger.Warnf("Set credential failed. Unable to write cache. %v", err) + } +} + +func (ssm *fileBasedSecureStorageManager) lockPath() string { + return filepath.Join(ssm.credDirPath, credCacheFileName+".lck") +} + +func (ssm *fileBasedSecureStorageManager) lockFile() error { + const numRetries = 10 + const retryInterval = 100 * time.Millisecond + lockPath := ssm.lockPath() + locked := false + for i := 0; i < numRetries; i++ { + err := os.Mkdir(lockPath, 0o700) if err != nil { - logger.Warnf("failed to convert credential to JSON.") - return + if errors.Is(err, os.ErrExist) { + time.Sleep(retryInterval) + continue + } + return fmt.Errorf("failed to create cache lock: %v, err: %v", lockPath, err) } + locked = true + break + } - logger.Debugf("writing credential cache file. %v\n", ssm.credCacheFilePath) - credCacheLockFileName := ssm.credCacheFilePath + ".lck" - logger.Debugf("Creating lock file. %v", credCacheLockFileName) - err = os.Mkdir(credCacheLockFileName, 0600) + if !locked { + logger.Warnf("failed to lock cache lock. lockPath: %v.", lockPath) + fileInfo, err := os.Stat(lockPath) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("failed to stat %v and determine if lock is stale. err: %v", lockPath, err) + } - switch { - case os.IsExist(err): - statinfo, err := os.Stat(credCacheLockFileName) + if fileInfo.ModTime().Add(time.Second).UnixNano() < time.Now().UnixNano() { + logger.Debugf("removing credentials cache lock file, stale for %v", time.Now().UnixNano()-fileInfo.ModTime().UnixNano()) + err := os.Remove(lockPath) if err != nil { - logger.Debugf("failed to write credential cache file. file: %v, err: %v. ignored.\n", ssm.credCacheFilePath, err) - return - } - if time.Since(statinfo.ModTime()) < 15*time.Minute { - logger.Debugf("other process locks the cache file. %v. ignored.\n", ssm.credCacheFilePath) - return + return fmt.Errorf("failed to remove %v while trying to remove stale lock. err: %v", lockPath, err) } - if err = os.Remove(credCacheLockFileName); err != nil { - logger.Debugf("failed to delete lock file. file: %v, err: %v. ignored.\n", credCacheLockFileName, err) - return - } - if err = os.Mkdir(credCacheLockFileName, 0600); err != nil { - logger.Debugf("failed to delete lock file. file: %v, err: %v. ignored.\n", credCacheLockFileName, err) - return + err = os.Mkdir(lockPath, 0o700) + if err != nil { + return fmt.Errorf("failed to recreate cache lock after removing stale lock. %v, err: %v", lockPath, err) } } - defer os.RemoveAll(credCacheLockFileName) + } + return nil +} - if err = os.WriteFile(ssm.credCacheFilePath, j, 0644); err != nil { - logger.Debugf("Failed to write the cache file. File: %v err: %v.", ssm.credCacheFilePath, err) - } +func (ssm *fileBasedSecureStorageManager) unlockFile() { + lockPath := ssm.lockPath() + err := os.Remove(lockPath) + if err != nil { + logger.Warnf("Failed to unlock cache lock: %v. %v", lockPath, err) } } func (ssm *fileBasedSecureStorageManager) getCredential(tokenSpec *secureTokenSpec) string { - credentialsKey := tokenSpec.buildKey() - ssm.credCacheLock.Lock() - defer ssm.credCacheLock.Unlock() - localCredCache := ssm.readTemporaryCacheFile() - cred := localCredCache[credentialsKey] - if cred != "" { - logger.Debug("Successfully read token. Returning as string") - } else { - logger.Debug("Returned credential is empty") + credentialsKey, err := tokenSpec.buildKey() + if err != nil { + logger.Warn(err) + return "" } - return cred + err = ssm.lockFile() + if err != nil { + logger.Warn("Failed to lock credential cache file.") + return "" + } + + credCache := ssm.readTemporaryCacheFile() + ssm.unlockFile() + cred, ok := ssm.getTokens(credCache)[credentialsKey] + if !ok { + return "" + } + + credStr, ok := cred.(string) + if !ok { + return "" + } + + return credStr } -func (ssm *fileBasedSecureStorageManager) readTemporaryCacheFile() map[string]string { - jsonData, err := os.ReadFile(ssm.credCacheFilePath) +func (ssm *fileBasedSecureStorageManager) credFilePath() string { + return filepath.Join(ssm.credDirPath, credCacheFileName) +} + +func (ssm *fileBasedSecureStorageManager) ensurePermissions() error { + dirInfo, err := os.Stat(ssm.credDirPath) if err != nil { - logger.Debugf("Failed to read credential file: %v", err) - return nil + return err + } + + if dirInfo.Mode().Perm() != 0o700&os.ModePerm { + return fmt.Errorf("incorrect permissions(%o, expected 700) for %s", dirInfo.Mode().Perm(), ssm.credDirPath) } - err = json.Unmarshal([]byte(jsonData), &ssm.localCredCache) + + fileInfo, err := os.Stat(ssm.credFilePath()) + if err != nil { + return err + } + + if fileInfo.Mode().Perm() != 0o600&os.ModePerm { + return fmt.Errorf("Incorrect permissions(%v, expected 600) for credential file.", fileInfo.Mode().Perm()) + + } + + return nil +} + +func (ssm *fileBasedSecureStorageManager) ensureOwner(filePath string) error { + currentUser, err := user.Current() if err != nil { - logger.Debugf("failed to read JSON. Err: %v", err) + return err + } + dirOwnerUid, err := provideFileOwner(filePath) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return err + } + if errors.Is(err, os.ErrNotExist) { return nil } + if strconv.Itoa(int(dirOwnerUid)) != currentUser.Uid { + return errors.New("incorrect owner of " + ssm.credDirPath) + } + return nil +} + +func (ssm *fileBasedSecureStorageManager) readTemporaryCacheFile() map[string]any { + if err := ssm.ensurePermissions(); err != nil { + logger.Warnf("Failed to ensure permission for temporary cache file. %v.\n", err) + return map[string]any{} + } + if err := ssm.ensureOwner(ssm.credDirPath); err != nil { + logger.Warn("Failed to ensure owner for %v. %v", ssm.credDirPath, err) + return map[string]any{} + } + if err := ssm.ensureOwner(ssm.credFilePath()); err != nil { + logger.Warn("Failed to ensure owner for %v. %v", ssm.credFilePath(), err) + return map[string]any{} + } + + jsonData, err := os.ReadFile(ssm.credFilePath()) + if err != nil { + logger.Warnf("Failed to read credential cache file. %v.\n", err) + return map[string]any{} + } + + credentialsMap := map[string]any{} + err = json.Unmarshal(jsonData, &credentialsMap) + if err != nil { + logger.Warnf("Failed to unmarshal credential cache file. %v.\n", err) + } - return ssm.localCredCache + return credentialsMap } func (ssm *fileBasedSecureStorageManager) deleteCredential(tokenSpec *secureTokenSpec) { - ssm.credCacheLock.Lock() - defer ssm.credCacheLock.Unlock() - credentialsKey := tokenSpec.buildKey() - delete(ssm.localCredCache, credentialsKey) - j, err := json.Marshal(ssm.localCredCache) + credentialsKey, err := tokenSpec.buildKey() if err != nil { - logger.Warnf("failed to convert credential to JSON.") + logger.Warn(err) return } - ssm.writeTemporaryCacheFile(j) + err = ssm.lockFile() + if err != nil { + logger.Warnf("Set credential failed. Unable to lock cache. %v", err) + return + } + defer ssm.unlockFile() + + credCache := ssm.readTemporaryCacheFile() + delete(ssm.getTokens(credCache), credentialsKey) + + err = ssm.writeTemporaryCacheFile(credCache) + if err != nil { + logger.Warnf("Set credential failed. Unable to write cache. %v", err) + } } -func (ssm *fileBasedSecureStorageManager) writeTemporaryCacheFile(input []byte) { - logger.Debugf("writing credential cache file. %v\n", ssm.credCacheFilePath) - credCacheLockFileName := ssm.credCacheFilePath + ".lck" - err := os.Mkdir(credCacheLockFileName, 0600) - logger.Debugf("Creating lock file. %v", credCacheLockFileName) +func (ssm *fileBasedSecureStorageManager) writeTemporaryCacheFile(cache map[string]any) error { + if err := ssm.ensureOwner(ssm.credDirPath); err != nil { + return err + } + if err := ssm.ensureOwner(ssm.credFilePath()); err != nil { + logger.Warn("Failed to ensure owner for %v. %v", ssm.credFilePath(), err) + return err + } - switch { - case os.IsExist(err): - statinfo, err := os.Stat(credCacheLockFileName) - if err != nil { - logger.Debugf("failed to write credential cache file. file: %v, err: %v. ignored.\n", ssm.credCacheFilePath, err) - return - } - if time.Since(statinfo.ModTime()) < 15*time.Minute { - logger.Debugf("other process locks the cache file. %v. ignored.\n", ssm.credCacheFilePath) - return - } - if err = os.Remove(credCacheLockFileName); err != nil { - logger.Debugf("failed to delete lock file. file: %v, err: %v. ignored.\n", credCacheLockFileName, err) - return - } - if err = os.Mkdir(credCacheLockFileName, 0600); err != nil { - logger.Debugf("failed to delete lock file. file: %v, err: %v. ignored.\n", credCacheLockFileName, err) - return + bytes, err := json.Marshal(cache) + if err != nil { + return fmt.Errorf("failed to marshal credential cache map. %w", err) + } + + stat, err := os.Stat(ssm.credFilePath()) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return err + } + if err == nil { + if stat.Mode().Perm() != 0o600&os.ModePerm { + return fmt.Errorf("incorrect %v permission, expected 0600, got %v", ssm.credFilePath(), stat.Mode().Perm()) } } - defer os.RemoveAll(credCacheLockFileName) - if err = os.WriteFile(ssm.credCacheFilePath, input, 0644); err != nil { - logger.Debugf("Failed to write the cache file. File: %v err: %v.", ssm.credCacheFilePath, err) + err = os.WriteFile(ssm.credFilePath(), bytes, 0600) + if err != nil { + return fmt.Errorf("failed to write the credential cache file: %w", err) } + return nil } type keyringSecureStorageManager struct { @@ -257,7 +414,11 @@ func (ssm *keyringSecureStorageManager) setCredential(tokenSpec *secureTokenSpec if value == "" { logger.Debug("no token provided") } else { - credentialsKey := tokenSpec.buildKey() + credentialsKey, err := tokenSpec.buildKey() + if err != nil { + logger.Warn(err) + return + } if runtime.GOOS == "windows" { ring, _ := keyring.Open(keyring.Config{ WinCredPrefix: strings.ToUpper(tokenSpec.host), @@ -288,7 +449,11 @@ func (ssm *keyringSecureStorageManager) setCredential(tokenSpec *secureTokenSpec func (ssm *keyringSecureStorageManager) getCredential(tokenSpec *secureTokenSpec) string { cred := "" - credentialsKey := tokenSpec.buildKey() + credentialsKey, err := tokenSpec.buildKey() + if err != nil { + logger.Warn(err) + return "" + } if runtime.GOOS == "windows" { ring, _ := keyring.Open(keyring.Config{ WinCredPrefix: strings.ToUpper(tokenSpec.host), @@ -319,7 +484,11 @@ func (ssm *keyringSecureStorageManager) getCredential(tokenSpec *secureTokenSpec } func (ssm *keyringSecureStorageManager) deleteCredential(tokenSpec *secureTokenSpec) { - credentialsKey := tokenSpec.buildKey() + credentialsKey, err := tokenSpec.buildKey() + if err != nil { + logger.Warn(err) + return + } if runtime.GOOS == "windows" { ring, _ := keyring.Open(keyring.Config{ WinCredPrefix: strings.ToUpper(tokenSpec.host), @@ -341,11 +510,17 @@ func (ssm *keyringSecureStorageManager) deleteCredential(tokenSpec *secureTokenS } } -func buildCredentialsKey(host, user string, credType tokenType) string { - host = strings.ToUpper(host) - user = strings.ToUpper(user) - credTypeStr := strings.ToUpper(string(credType)) - return host + ":" + user + ":" + driverName + ":" + credTypeStr +func buildCredentialsKey(host, user string, credType tokenType) (string, error) { + if host == "" { + return "", errors.New("host is not provided to store in token cache, skipping") + } + if user == "" { + return "", errors.New("user is not provided to store in token cache, skipping") + } + plainCredKey := host + ":" + user + ":" + string(credType) + checksum := sha256.New() + checksum.Write([]byte(plainCredKey)) + return hex.EncodeToString(checksum.Sum(nil)), nil } type noopSecureStorageManager struct { @@ -362,5 +537,28 @@ func (ssm *noopSecureStorageManager) getCredential(_ *secureTokenSpec) string { return "" } -func (ssm *noopSecureStorageManager) deleteCredential(_ *secureTokenSpec) { //TODO implement me +func (ssm *noopSecureStorageManager) deleteCredential(_ *secureTokenSpec) { +} + +type threadSafeSecureStorageManager struct { + mu *sync.Mutex + delegate secureStorageManager +} + +func (ssm *threadSafeSecureStorageManager) setCredential(tokenSpec *secureTokenSpec, value string) { + ssm.mu.Lock() + defer ssm.mu.Unlock() + ssm.delegate.setCredential(tokenSpec, value) +} + +func (ssm *threadSafeSecureStorageManager) getCredential(tokenSpec *secureTokenSpec) string { + ssm.mu.Lock() + defer ssm.mu.Unlock() + return ssm.delegate.getCredential(tokenSpec) +} + +func (ssm *threadSafeSecureStorageManager) deleteCredential(tokenSpec *secureTokenSpec) { + ssm.mu.Lock() + defer ssm.mu.Unlock() + ssm.delegate.deleteCredential(tokenSpec) } diff --git a/secure_storage_manager_test.go b/secure_storage_manager_test.go index 22299708b..b30b426e0 100644 --- a/secure_storage_manager_test.go +++ b/secure_storage_manager_test.go @@ -3,9 +3,157 @@ package gosnowflake import ( + "encoding/json" + "os" + "path/filepath" "testing" + "time" ) +func TestBuildCredCacheDirPath(t *testing.T) { + skipOnWindows(t, "permission model is different") + testRoot1, err := os.MkdirTemp("", "") + assertNilF(t, err) + defer os.RemoveAll(testRoot1) + testRoot2, err := os.MkdirTemp("", "") + assertNilF(t, err) + defer os.RemoveAll(testRoot2) + + assertNilF(t, os.Setenv("CACHE_DIR_TEST_NOT_EXISTING", "/tmp/not_existing_dir")) + assertNilF(t, os.Setenv("CACHE_DIR_TEST_1", testRoot1)) + assertNilF(t, os.Setenv("CACHE_DIR_TEST_2", testRoot2)) + + t.Run("cannot find any dir", func(t *testing.T) { + _, err := buildCredCacheDirPath([]cacheDirConf{ + {envVar: "CACHE_DIR_TEST_NOT_EXISTING"}, + }) + assertEqualE(t, err.Error(), "no credentials cache directory found") + _, err = os.Stat("/tmp/not_existing_dir") + assertStringContainsE(t, err.Error(), "no such file or directory") + }) + + t.Run("should use first dir that exists", func(t *testing.T) { + path, err := buildCredCacheDirPath([]cacheDirConf{ + {envVar: "CACHE_DIR_TEST_NOT_EXISTING"}, + {envVar: "CACHE_DIR_TEST_1"}, + }) + assertNilF(t, err) + assertEqualE(t, path, testRoot1) + stat, err := os.Stat(testRoot1) + assertNilF(t, err) + assertEqualE(t, stat.Mode(), 0700|os.ModeDir) + }) + + t.Run("should use first dir that exists and append segments", func(t *testing.T) { + path, err := buildCredCacheDirPath([]cacheDirConf{ + {envVar: "CACHE_DIR_TEST_NOT_EXISTING"}, + {envVar: "CACHE_DIR_TEST_2", pathSegments: []string{"sub1", "sub2"}}, + }) + assertNilF(t, err) + assertEqualE(t, path, filepath.Join(testRoot2, "sub1", "sub2")) + stat, err := os.Stat(testRoot2) + assertNilF(t, err) + assertEqualE(t, stat.Mode(), 0700|os.ModeDir) + }) +} + +func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) { + skipOnWindows(t, "file system permission is different") + credCacheDir, err := os.MkdirTemp("", "") + assertNilF(t, err) + assertNilF(t, os.MkdirAll(credCacheDir, os.ModePerm)) + credCacheDirEnvOverride := overrideEnv(credCacheDirEnv, credCacheDir) + defer credCacheDirEnvOverride.rollback() + ssm, err := newFileBasedSecureStorageManager() + assertNilF(t, err) + + t.Run("store single token", func(t *testing.T) { + tokenSpec := newMfaTokenSpec("host.com", "johndoe") + cred := "token123" + ssm.setCredential(tokenSpec, cred) + assertEqualE(t, ssm.getCredential(tokenSpec), cred) + ssm.deleteCredential(tokenSpec) + assertEqualE(t, ssm.getCredential(tokenSpec), "") + }) + + t.Run("store tokens of different types, hosts and users", func(t *testing.T) { + mfaTokenSpec := newMfaTokenSpec("host.com", "johndoe") + mfaCred := "token12" + idTokenSpec := newIDTokenSpec("host.com", "johndoe") + idCred := "token34" + idTokenSpec2 := newIDTokenSpec("host.org", "johndoe") + idCred2 := "token56" + idTokenSpec3 := newIDTokenSpec("host.com", "someoneelse") + idCred3 := "token78" + ssm.setCredential(mfaTokenSpec, mfaCred) + ssm.setCredential(idTokenSpec, idCred) + ssm.setCredential(idTokenSpec2, idCred2) + ssm.setCredential(idTokenSpec3, idCred3) + assertEqualE(t, ssm.getCredential(mfaTokenSpec), mfaCred) + assertEqualE(t, ssm.getCredential(idTokenSpec), idCred) + assertEqualE(t, ssm.getCredential(idTokenSpec2), idCred2) + assertEqualE(t, ssm.getCredential(idTokenSpec3), idCred3) + ssm.deleteCredential(mfaTokenSpec) + assertEqualE(t, ssm.getCredential(mfaTokenSpec), "") + assertEqualE(t, ssm.getCredential(idTokenSpec), idCred) + assertEqualE(t, ssm.getCredential(idTokenSpec2), idCred2) + assertEqualE(t, ssm.getCredential(idTokenSpec3), idCred3) + }) + + t.Run("override single token", func(t *testing.T) { + mfaTokenSpec := newMfaTokenSpec("host.com", "johndoe") + mfaCred := "token123" + idTokenSpec := newIDTokenSpec("host.com", "johndoe") + idCred := "token456" + ssm.setCredential(mfaTokenSpec, mfaCred) + ssm.setCredential(idTokenSpec, idCred) + assertEqualE(t, ssm.getCredential(mfaTokenSpec), mfaCred) + mfaCredOverride := "token789" + ssm.setCredential(mfaTokenSpec, mfaCredOverride) + assertEqualE(t, ssm.getCredential(mfaTokenSpec), mfaCredOverride) + ssm.setCredential(idTokenSpec, idCred) + }) + + t.Run("unlock stale cache", func(t *testing.T) { + startTime := time.Now() + assertNilF(t, os.Mkdir(ssm.lockPath(), 0o700)) + ssm.getCredential(newMfaTokenSpec("ignored", "ignored")) + assertTrueE(t, time.Since(startTime).Milliseconds() > 1000) + }) + + t.Run("should not modify keys other than tokens", func(t *testing.T) { + content := []byte(`{ + "otherKey": "otherValue" + }`) + err = os.WriteFile(ssm.credFilePath(), content, 0o600) + assertNilF(t, err) + ssm.setCredential(newMfaTokenSpec("somehost.com", "someUser"), "someToken") + result, err := os.ReadFile(ssm.credFilePath()) + assertNilF(t, err) + assertStringContainsE(t, string(result), `"otherKey":"otherValue"`) + }) + + t.Run("should not modify file if it 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.credFilePath(), 0644) + assertNilF(t, err) + defer os.Chmod(ssm.credFilePath(), 0600) + 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) { for _, tokenSpec := range []*secureTokenSpec{ newMfaTokenSpec("testhost", "testuser"), @@ -25,6 +173,34 @@ func TestSetAndGetCredentialMfa(t *testing.T) { } } +func TestSkipStoringCredentialIfUserIsEmpty(t *testing.T) { + tokenSpecs := []*secureTokenSpec{ + newMfaTokenSpec("mfaHost.com", ""), + newIDTokenSpec("idHost.com", ""), + } + + for _, tokenSpec := range tokenSpecs { + t.Run(tokenSpec.host, func(t *testing.T) { + credentialsStorage.setCredential(tokenSpec, "non-empty-value") + assertEqualE(t, credentialsStorage.getCredential(tokenSpec), "") + }) + } +} + +func TestSkipStoringCredentialIfHostIsEmpty(t *testing.T) { + tokenSpecs := []*secureTokenSpec{ + newMfaTokenSpec("", "mfaUser"), + newIDTokenSpec("", "idUser"), + } + + for _, tokenSpec := range tokenSpecs { + t.Run(tokenSpec.user, func(t *testing.T) { + credentialsStorage.setCredential(tokenSpec, "non-empty-value") + assertEqualE(t, credentialsStorage.getCredential(tokenSpec), "") + }) + } +} + func TestStoreTemporaryCredental(t *testing.T) { if runningOnGithubAction() { t.Skip("cannot write to github file system") @@ -58,11 +234,12 @@ func TestBuildCredentialsKey(t *testing.T) { credType tokenType out string }{ - {"testaccount.snowflakecomputing.com", "testuser", "mfaToken", "TESTACCOUNT.SNOWFLAKECOMPUTING.COM:TESTUSER:SNOWFLAKE-GO-DRIVER:MFATOKEN"}, - {"testaccount.snowflakecomputing.com", "testuser", "IdToken", "TESTACCOUNT.SNOWFLAKECOMPUTING.COM:TESTUSER:SNOWFLAKE-GO-DRIVER:IDTOKEN"}, + {"testaccount.snowflakecomputing.com", "testuser", "mfaToken", "c4e781475e7a5e74aca87cd462afafa8cc48ebff6f6ccb5054b894dae5eb6345"}, // pragma: allowlist secret + {"testaccount.snowflakecomputing.com", "testuser", "IdToken", "5014e26489992b6ea56b50e936ba85764dc51338f60441bdd4a69eac7e15bada"}, // pragma: allowlist secret } for _, test := range testcases { - target := buildCredentialsKey(test.host, test.user, test.credType) + target, err := buildCredentialsKey(test.host, test.user, test.credType) + assertNilF(t, err) if target != test.out { t.Fatalf("failed to convert target. expected: %v, but got: %v", test.out, target) } diff --git a/util_test.go b/util_test.go index 62bfcbef8..6a09f40d6 100644 --- a/util_test.go +++ b/util_test.go @@ -404,6 +404,12 @@ func skipOnMac(t *testing.T, reason string) { } } +func skipOnWindows(t *testing.T, reason string) { + if runtime.GOOS == "windows" { + t.Skip("skipped on Windows: " + reason) + } +} + func randomString(n int) string { r := rand.New(rand.NewSource(time.Now().UnixNano())) alpha := []rune("abcdefghijklmnopqrstuvwxyz") @@ -440,3 +446,22 @@ func TestInternal(t *testing.T) { ctx = WithInternal(ctx) assertTrueE(t, isInternal(ctx)) } + +type envOverride struct { + envName string + oldValue string +} + +func (e *envOverride) rollback() { + if e.oldValue != "" { + os.Setenv(e.envName, e.oldValue) + } else { + os.Unsetenv(e.envName) + } +} + +func overrideEnv(env string, value string) envOverride { + oldValue := os.Getenv(env) + os.Setenv(env, value) + return envOverride{env, oldValue} +} From 577ce1c755364fbb2068e0d0b50973f2defd10e5 Mon Sep 17 00:00:00 2001 From: Piotr Fus Date: Thu, 6 Mar 2025 12:38:27 +0100 Subject: [PATCH 02/10] Fix linter --- secure_storage_manager.go | 6 +++--- secure_storage_manager_test.go | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/secure_storage_manager.go b/secure_storage_manager.go index 00bddfb1e..de4601a89 100644 --- a/secure_storage_manager.go +++ b/secure_storage_manager.go @@ -296,7 +296,7 @@ func (ssm *fileBasedSecureStorageManager) ensurePermissions() error { } if fileInfo.Mode().Perm() != 0o600&os.ModePerm { - return fmt.Errorf("Incorrect permissions(%v, expected 600) for credential file.", fileInfo.Mode().Perm()) + return fmt.Errorf("incorrect permissions(%v, expected 600) for credential file", fileInfo.Mode().Perm()) } @@ -308,14 +308,14 @@ func (ssm *fileBasedSecureStorageManager) ensureOwner(filePath string) error { if err != nil { return err } - dirOwnerUid, err := provideFileOwner(filePath) + dirOwnerUID, err := provideFileOwner(filePath) if err != nil && !errors.Is(err, os.ErrNotExist) { return err } if errors.Is(err, os.ErrNotExist) { return nil } - if strconv.Itoa(int(dirOwnerUid)) != currentUser.Uid { + if strconv.Itoa(int(dirOwnerUID)) != currentUser.Uid { return errors.New("incorrect owner of " + ssm.credDirPath) } return nil diff --git a/secure_storage_manager_test.go b/secure_storage_manager_test.go index b30b426e0..832250f85 100644 --- a/secure_storage_manager_test.go +++ b/secure_storage_manager_test.go @@ -139,7 +139,9 @@ func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) { assertEqualE(t, ssm.getCredential(tokenSpec), "initialValue") err = os.Chmod(ssm.credFilePath(), 0644) assertNilF(t, err) - defer os.Chmod(ssm.credFilePath(), 0600) + defer func() { + assertNilE(t, os.Chmod(ssm.credFilePath(), 0600)) + }() ssm.setCredential(tokenSpec, "newValue") assertEqualE(t, ssm.getCredential(tokenSpec), "") fileContent, err := os.ReadFile(ssm.credFilePath()) From 506100d003cd620a5c28ab3ec1dea85d073c7447 Mon Sep 17 00:00:00 2001 From: Piotr Fus Date: Fri, 7 Mar 2025 10:38:59 +0100 Subject: [PATCH 03/10] Fix empty tokens --- secure_storage_manager.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/secure_storage_manager.go b/secure_storage_manager.go index de4601a89..bde5e7ad7 100644 --- a/secure_storage_manager.go +++ b/secure_storage_manager.go @@ -160,16 +160,13 @@ func buildCredCacheDirPath(confs []cacheDirConf) (string, error) { func (ssm *fileBasedSecureStorageManager) getTokens(data map[string]any) map[string]interface{} { val, ok := data["tokens"] - emptyMap := map[string]interface{}{} if !ok { - data["tokens"] = emptyMap - return emptyMap + return map[string]interface{}{} } tokens, ok := val.(map[string]interface{}) if !ok { - data["tokens"] = emptyMap - return emptyMap + return map[string]interface{}{} } return tokens @@ -189,8 +186,9 @@ func (ssm *fileBasedSecureStorageManager) setCredential(tokenSpec *secureTokenSp defer ssm.unlockFile() credCache := ssm.readTemporaryCacheFile() - ssm.getTokens(credCache)[credentialsKey] = value - + tokens := ssm.getTokens(credCache) + tokens[credentialsKey] = value + credCache["tokens"] = tokens err = ssm.writeTemporaryCacheFile(credCache) if err != nil { logger.Warnf("Set credential failed. Unable to write cache. %v", err) @@ -297,7 +295,6 @@ func (ssm *fileBasedSecureStorageManager) ensurePermissions() error { if fileInfo.Mode().Perm() != 0o600&os.ModePerm { return fmt.Errorf("incorrect permissions(%v, expected 600) for credential file", fileInfo.Mode().Perm()) - } return nil From 8a47a6753735906587cc233ed4f67a4bd2a0ddfe Mon Sep 17 00:00:00 2001 From: Piotr Fus Date: Tue, 11 Mar 2025 14:45:15 +0100 Subject: [PATCH 04/10] Fixes after review --- os_specific_posix.go | 14 ++- os_specific_windows.go | 11 +- secure_storage_manager.go | 214 +++++++++++++++++++-------------- secure_storage_manager_test.go | 30 ++++- 4 files changed, 172 insertions(+), 97 deletions(-) diff --git a/os_specific_posix.go b/os_specific_posix.go index ef7ab7518..1de40f3be 100644 --- a/os_specific_posix.go +++ b/os_specific_posix.go @@ -8,11 +8,23 @@ import ( "syscall" ) -func provideFileOwner(filepath string) (uint32, error) { +func providePathOwner(filepath string) (uint32, error) { info, err := os.Stat(filepath) if err != nil { return 0, err } + return provideOwnerFromStat(info, filepath) +} + +func provideFileOwner(file *os.File) (uint32, error) { + info, err := file.Stat() + if err != nil { + return 0, err + } + return provideOwnerFromStat(info, file.Name()) +} + +func provideOwnerFromStat(info os.FileInfo, filepath string) (uint32, error) { nativeStat, ok := info.Sys().(*syscall.Stat_t) if !ok { return 0, fmt.Errorf("cannot cast file info for %v to *syscall.Stat_t", filepath) diff --git a/os_specific_windows.go b/os_specific_windows.go index adfc98ee0..d30a27200 100644 --- a/os_specific_windows.go +++ b/os_specific_windows.go @@ -2,8 +2,15 @@ package gosnowflake -import "errors" +import ( + "errors" + "os" +) -func provideFileOwner(filepath string) (uint32, error) { +func providePathOwner(filepath string) (uint32, error) { + return 0, errors.New("providePathOwner is unsupported on windows") +} + +func provideFileOwner(file *os.File) (uint32, error) { return 0, errors.New("provideFileOwner is unsupported on windows") } diff --git a/secure_storage_manager.go b/secure_storage_manager.go index bde5e7ad7..9b2d27c2f 100644 --- a/secure_storage_manager.go +++ b/secure_storage_manager.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "github.com/99designs/keyring" + "io" "os" "os/user" "path/filepath" @@ -87,7 +88,7 @@ func newSecureStorageManager() secureStorageManager { case "darwin", "windows": return &threadSafeSecureStorageManager{&sync.Mutex{}, newKeyringBasedSecureStorageManager()} default: - logger.Infof("OS %v does not support credentials cache", runtime.GOOS) + logger.Warnf("OS %v does not support credentials cache", runtime.GOOS) return newNoopSecureStorageManager() } } @@ -132,15 +133,6 @@ func lookupCacheDir(envVar string, pathSegments ...string) (string, error) { return "", err } - fileInfo, err = os.Stat(cacheDir) - if err != nil { - return "", fmt.Errorf("failed to stat %s=%s, due to %w", envVar, cacheDir, err) - } - - if fileInfo.Mode().Perm()|os.ModeDir != 0o700|os.ModeDir { - return "", fmt.Errorf("incorrect permission to %v", cacheDir) - } - return cacheDir, nil } @@ -150,7 +142,7 @@ func buildCredCacheDirPath(confs []cacheDirConf) (string, error) { if err != nil { logger.Debugf("Skipping %s in cache directory lookup due to %v", conf.envVar, err) } else { - logger.Infof("Using %s as cache directory", path) + logger.Debugf("Using %s as cache directory", path) return path, nil } } @@ -172,6 +164,20 @@ func (ssm *fileBasedSecureStorageManager) getTokens(data map[string]any) map[str return tokens } +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) + 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) + action(cacheFile) +} + func (ssm *fileBasedSecureStorageManager) setCredential(tokenSpec *secureTokenSpec, value string) { credentialsKey, err := tokenSpec.buildKey() if err != nil { @@ -185,14 +191,20 @@ func (ssm *fileBasedSecureStorageManager) setCredential(tokenSpec *secureTokenSp } defer ssm.unlockFile() - credCache := ssm.readTemporaryCacheFile() - tokens := ssm.getTokens(credCache) - tokens[credentialsKey] = value - credCache["tokens"] = tokens - err = ssm.writeTemporaryCacheFile(credCache) - if err != nil { - logger.Warnf("Set credential failed. Unable to write cache. %v", err) - } + ssm.withCacheFile(func(cacheFile *os.File) { + credCache, err := ssm.readTemporaryCacheFile(cacheFile) + if err != nil { + logger.Warnf("Error while reading cache file. %v", err) + return + } + tokens := ssm.getTokens(credCache) + tokens[credentialsKey] = value + credCache["tokens"] = tokens + err = ssm.writeTemporaryCacheFile(credCache, cacheFile) + if err != nil { + logger.Warnf("Set credential failed. Unable to write cache. %v", err) + } + }) } func (ssm *fileBasedSecureStorageManager) lockPath() string { @@ -203,6 +215,22 @@ func (ssm *fileBasedSecureStorageManager) lockFile() error { const numRetries = 10 const retryInterval = 100 * time.Millisecond lockPath := ssm.lockPath() + + fileInfo, err := os.Stat(lockPath) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("failed to stat %v and determine if lock is stale. err: %v", lockPath, err) + } + + // removing stale lock + now := time.Now() + if !errors.Is(err, os.ErrNotExist) && fileInfo.ModTime().Add(time.Second).UnixNano() < now.UnixNano() { + logger.Debugf("removing credentials cache lock file, stale for %vms", (now.UnixNano()-fileInfo.ModTime().UnixNano())/1000/1000) + err = os.Remove(lockPath) + if err != nil { + return fmt.Errorf("failed to remove %v while trying to remove stale lock. err: %v", lockPath, err) + } + } + locked := false for i := 0; i < numRetries; i++ { err := os.Mkdir(lockPath, 0o700) @@ -216,25 +244,8 @@ func (ssm *fileBasedSecureStorageManager) lockFile() error { locked = true break } - if !locked { - logger.Warnf("failed to lock cache lock. lockPath: %v.", lockPath) - fileInfo, err := os.Stat(lockPath) - if err != nil && !errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("failed to stat %v and determine if lock is stale. err: %v", lockPath, err) - } - - if fileInfo.ModTime().Add(time.Second).UnixNano() < time.Now().UnixNano() { - logger.Debugf("removing credentials cache lock file, stale for %v", time.Now().UnixNano()-fileInfo.ModTime().UnixNano()) - err := os.Remove(lockPath) - if err != nil { - return fmt.Errorf("failed to remove %v while trying to remove stale lock. err: %v", lockPath, err) - } - err = os.Mkdir(lockPath, 0o700) - if err != nil { - return fmt.Errorf("failed to recreate cache lock after removing stale lock. %v, err: %v", lockPath, err) - } - } + return fmt.Errorf("failed to lock cache. lockPath: %v", lockPath) } return nil } @@ -255,30 +266,38 @@ func (ssm *fileBasedSecureStorageManager) getCredential(tokenSpec *secureTokenSp } err = ssm.lockFile() if err != nil { - logger.Warn("Failed to lock credential cache file.") + logger.Warnf("Failed to lock credential cache file. %v", err) return "" } + defer ssm.unlockFile() - credCache := ssm.readTemporaryCacheFile() - ssm.unlockFile() - cred, ok := ssm.getTokens(credCache)[credentialsKey] - if !ok { - return "" - } + ret := "" + ssm.withCacheFile(func(cacheFile *os.File) { + credCache, err := ssm.readTemporaryCacheFile(cacheFile) + if err != nil { + logger.Warnf("Error while reading cache file. %v", err) + return + } + cred, ok := ssm.getTokens(credCache)[credentialsKey] + if !ok { + return + } - credStr, ok := cred.(string) - if !ok { - return "" - } + credStr, ok := cred.(string) + if !ok { + return + } - return credStr + ret = credStr + }) + return ret } func (ssm *fileBasedSecureStorageManager) credFilePath() string { return filepath.Join(ssm.credDirPath, credCacheFileName) } -func (ssm *fileBasedSecureStorageManager) ensurePermissions() error { +func (ssm *fileBasedSecureStorageManager) ensurePermissions(cacheFile *os.File) error { dirInfo, err := os.Stat(ssm.credDirPath) if err != nil { return err @@ -288,7 +307,7 @@ func (ssm *fileBasedSecureStorageManager) ensurePermissions() error { return fmt.Errorf("incorrect permissions(%o, expected 700) for %s", dirInfo.Mode().Perm(), ssm.credDirPath) } - fileInfo, err := os.Stat(ssm.credFilePath()) + fileInfo, err := cacheFile.Stat() if err != nil { return err } @@ -300,51 +319,68 @@ func (ssm *fileBasedSecureStorageManager) ensurePermissions() error { return nil } -func (ssm *fileBasedSecureStorageManager) ensureOwner(filePath string) error { - currentUser, err := user.Current() - if err != nil { +func (ssm *fileBasedSecureStorageManager) ensureOwnerForDir(filePath string) error { + ownerUID, err := providePathOwner(filePath) + if err != nil && !errors.Is(err, os.ErrNotExist) { return err } - dirOwnerUID, err := provideFileOwner(filePath) + return ssm.ensureOwner(ownerUID) +} + +func (ssm *fileBasedSecureStorageManager) ensureOwnerForFile(file *os.File) error { + ownerUID, err := provideFileOwner(file) 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(dirOwnerUID)) != currentUser.Uid { + if strconv.Itoa(int(ownerId)) != currentUser.Uid { return errors.New("incorrect owner of " + ssm.credDirPath) } return nil } -func (ssm *fileBasedSecureStorageManager) readTemporaryCacheFile() map[string]any { - if err := ssm.ensurePermissions(); err != nil { - logger.Warnf("Failed to ensure permission for temporary cache file. %v.\n", err) - return map[string]any{} +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.ensureOwner(ssm.credDirPath); err != nil { - logger.Warn("Failed to ensure owner for %v. %v", ssm.credDirPath, err) - return map[string]any{} + 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.ensureOwner(ssm.credFilePath()); err != nil { - logger.Warn("Failed to ensure owner for %v. %v", ssm.credFilePath(), err) - return map[string]any{} + 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 := os.ReadFile(ssm.credFilePath()) + jsonData, err := io.ReadAll(cacheFile) if err != nil { logger.Warnf("Failed to read credential cache file. %v.\n", err) - return map[string]any{} + return map[string]any{}, nil + } + if _, err = cacheFile.Seek(0, 0); err != nil { + return map[string]any{}, fmt.Errorf("cannot seek to the beginning of a cache file. %v", err) + } + + if len(jsonData) == 0 { + // Happens when the file didn't exist before. + return map[string]any{}, nil } credentialsMap := map[string]any{} err = json.Unmarshal(jsonData, &credentialsMap) if err != nil { - logger.Warnf("Failed to unmarshal credential cache file. %v.\n", err) + return map[string]any{}, fmt.Errorf("Failed to unmarshal credential cache file. %v.\n", err) } - return credentialsMap + return credentialsMap, nil } func (ssm *fileBasedSecureStorageManager) deleteCredential(tokenSpec *secureTokenSpec) { @@ -360,20 +396,26 @@ func (ssm *fileBasedSecureStorageManager) deleteCredential(tokenSpec *secureToke } defer ssm.unlockFile() - credCache := ssm.readTemporaryCacheFile() - delete(ssm.getTokens(credCache), credentialsKey) + ssm.withCacheFile(func(cacheFile *os.File) { + credCache, err := ssm.readTemporaryCacheFile(cacheFile) + if err != nil { + logger.Warnf("Error while reading cache file. %v", err) + return + } + delete(ssm.getTokens(credCache), credentialsKey) - err = ssm.writeTemporaryCacheFile(credCache) - if err != nil { - logger.Warnf("Set credential failed. Unable to write cache. %v", err) - } + err = ssm.writeTemporaryCacheFile(credCache, cacheFile) + if err != nil { + logger.Warnf("Set credential failed. Unable to write cache. %v", err) + } + }) } -func (ssm *fileBasedSecureStorageManager) writeTemporaryCacheFile(cache map[string]any) error { - if err := ssm.ensureOwner(ssm.credDirPath); err != nil { +func (ssm *fileBasedSecureStorageManager) writeTemporaryCacheFile(cache map[string]any, cacheFile *os.File) error { + if err := ssm.ensureOwnerForDir(ssm.credDirPath); err != nil { return err } - if err := ssm.ensureOwner(ssm.credFilePath()); err != nil { + if err := ssm.ensureOwnerForDir(ssm.credFilePath()); err != nil { logger.Warn("Failed to ensure owner for %v. %v", ssm.credFilePath(), err) return err } @@ -383,20 +425,14 @@ func (ssm *fileBasedSecureStorageManager) writeTemporaryCacheFile(cache map[stri return fmt.Errorf("failed to marshal credential cache map. %w", err) } - stat, err := os.Stat(ssm.credFilePath()) - if err != nil && !errors.Is(err, os.ErrNotExist) { - return err - } - if err == nil { - if stat.Mode().Perm() != 0o600&os.ModePerm { - return fmt.Errorf("incorrect %v permission, expected 0600, got %v", ssm.credFilePath(), stat.Mode().Perm()) - } + if err = cacheFile.Truncate(0); err != nil { + return fmt.Errorf("error while truncating credentials cache. %v", err) } - - err = os.WriteFile(ssm.credFilePath(), bytes, 0600) + _, err = cacheFile.Write(bytes) if err != nil { return fmt.Errorf("failed to write the credential cache file: %w", err) } + cacheFile.Seek(0, 0) return nil } diff --git a/secure_storage_manager_test.go b/secure_storage_manager_test.go index 832250f85..51330c43d 100644 --- a/secure_storage_manager_test.go +++ b/secure_storage_manager_test.go @@ -19,9 +19,12 @@ func TestBuildCredCacheDirPath(t *testing.T) { assertNilF(t, err) defer os.RemoveAll(testRoot2) - assertNilF(t, os.Setenv("CACHE_DIR_TEST_NOT_EXISTING", "/tmp/not_existing_dir")) - assertNilF(t, os.Setenv("CACHE_DIR_TEST_1", testRoot1)) - assertNilF(t, os.Setenv("CACHE_DIR_TEST_2", testRoot2)) + env1 := overrideEnv("CACHE_DIR_TEST_NOT_EXISTING", "/tmp/not_existing_dir") + defer env1.rollback() + env2 := overrideEnv("CACHE_DIR_TEST_1", testRoot1) + defer env2.rollback() + env3 := overrideEnv("CACHE_DIR_TEST_2", testRoot2) + defer env3.rollback() t.Run("cannot find any dir", func(t *testing.T) { _, err := buildCredCacheDirPath([]cacheDirConf{ @@ -115,10 +118,27 @@ func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) { }) t.Run("unlock stale cache", func(t *testing.T) { + tokenSpec := newMfaTokenSpec("stale", "cache") + assertNilF(t, os.Mkdir(ssm.lockPath(), 0o700)) + time.Sleep(1000 * time.Millisecond) + ssm.setCredential(tokenSpec, "unlocked") + assertEqualE(t, ssm.getCredential(tokenSpec), "unlocked") + }) + + t.Run("wait for other process to unlock cache", func(t *testing.T) { + tokenSpec := newMfaTokenSpec("stale", "cache") startTime := time.Now() assertNilF(t, os.Mkdir(ssm.lockPath(), 0o700)) - ssm.getCredential(newMfaTokenSpec("ignored", "ignored")) - assertTrueE(t, time.Since(startTime).Milliseconds() > 1000) + time.Sleep(500 * time.Millisecond) + go func() { + time.Sleep(500 * time.Millisecond) + assertNilF(t, os.Remove(ssm.lockPath())) + }() + ssm.setCredential(tokenSpec, "unlocked") + totalDurationMillis := time.Now().Sub(startTime).Milliseconds() + assertEqualE(t, ssm.getCredential(tokenSpec), "unlocked") + println(totalDurationMillis) + assertTrueE(t, totalDurationMillis > 1000 && totalDurationMillis < 1200) }) t.Run("should not modify keys other than tokens", func(t *testing.T) { From a424b3662dbd54985eec5f82994cbb101fd0d063 Mon Sep 17 00:00:00 2001 From: Piotr Fus Date: Wed, 12 Mar 2025 08:18:28 +0100 Subject: [PATCH 05/10] Remove file perm checking on writing --- secure_storage_manager.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/secure_storage_manager.go b/secure_storage_manager.go index 9b2d27c2f..8cff0fb14 100644 --- a/secure_storage_manager.go +++ b/secure_storage_manager.go @@ -415,10 +415,6 @@ func (ssm *fileBasedSecureStorageManager) writeTemporaryCacheFile(cache map[stri if err := ssm.ensureOwnerForDir(ssm.credDirPath); err != nil { return err } - if err := ssm.ensureOwnerForDir(ssm.credFilePath()); err != nil { - logger.Warn("Failed to ensure owner for %v. %v", ssm.credFilePath(), err) - return err - } bytes, err := json.Marshal(cache) if err != nil { From ffad6d7326cb68c2b6a5cc20e8dbaf96ccabfc57 Mon Sep 17 00:00:00 2001 From: Piotr Fus Date: Wed, 12 Mar 2025 09:27:03 +0100 Subject: [PATCH 06/10] Fix linter --- secure_storage_manager.go | 3 +-- secure_storage_manager_test.go | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/secure_storage_manager.go b/secure_storage_manager.go index 8cff0fb14..1bc3a9722 100644 --- a/secure_storage_manager.go +++ b/secure_storage_manager.go @@ -377,7 +377,7 @@ func (ssm *fileBasedSecureStorageManager) readTemporaryCacheFile(cacheFile *os.F credentialsMap := map[string]any{} err = json.Unmarshal(jsonData, &credentialsMap) if err != nil { - return map[string]any{}, fmt.Errorf("Failed to unmarshal credential cache file. %v.\n", err) + return map[string]any{}, fmt.Errorf("failed to unmarshal credential cache file. %v", err) } return credentialsMap, nil @@ -428,7 +428,6 @@ func (ssm *fileBasedSecureStorageManager) writeTemporaryCacheFile(cache map[stri if err != nil { return fmt.Errorf("failed to write the credential cache file: %w", err) } - cacheFile.Seek(0, 0) return nil } diff --git a/secure_storage_manager_test.go b/secure_storage_manager_test.go index 51330c43d..9dae69eea 100644 --- a/secure_storage_manager_test.go +++ b/secure_storage_manager_test.go @@ -135,9 +135,8 @@ func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) { assertNilF(t, os.Remove(ssm.lockPath())) }() ssm.setCredential(tokenSpec, "unlocked") - totalDurationMillis := time.Now().Sub(startTime).Milliseconds() + totalDurationMillis := time.Since(startTime).Milliseconds() assertEqualE(t, ssm.getCredential(tokenSpec), "unlocked") - println(totalDurationMillis) assertTrueE(t, totalDurationMillis > 1000 && totalDurationMillis < 1200) }) From e3b1c37fcc61396b5099b2f439e9da4c2eeb0e46 Mon Sep 17 00:00:00 2001 From: Piotr Fus Date: Thu, 13 Mar 2025 08:02:23 +0100 Subject: [PATCH 07/10] Create cache dir with correct perm --- secure_storage_manager.go | 51 +++++++++++++++------------------- secure_storage_manager_test.go | 6 ++-- 2 files changed, 26 insertions(+), 31 deletions(-) diff --git a/secure_storage_manager.go b/secure_storage_manager.go index 1bc3a9722..ba914fc40 100644 --- a/secure_storage_manager.go +++ b/secure_storage_manager.go @@ -124,12 +124,14 @@ func lookupCacheDir(envVar string, pathSegments ...string) (string, error) { } cacheDir := filepath.Join(envVal, filepath.Join(pathSegments...)) + parentOfCacheDir := cacheDir[:strings.LastIndex(cacheDir, "/")] - if err = os.MkdirAll(cacheDir, os.FileMode(0o755)); err != nil { + if err = os.MkdirAll(parentOfCacheDir, os.FileMode(0755)); err != nil { return "", err } - if err = os.Chmod(cacheDir, os.FileMode(0700)); err != nil { + // We don't check if permissions are incorrect here if a directory exists, because we check it later. + if err = os.Mkdir(cacheDir, os.FileMode(0700)); err != nil && !errors.Is(err, os.ErrExist) { return "", err } @@ -164,6 +166,17 @@ func (ssm *fileBasedSecureStorageManager) getTokens(data map[string]any) map[str return tokens } +func (ssm *fileBasedSecureStorageManager) withLock(action func(cacheFile *os.File)) { + err := ssm.lockFile() + if err != nil { + logger.Warnf("Unable to lock cache. %v", err) + return + } + defer ssm.unlockFile() + + ssm.withCacheFile(action) +} + func (ssm *fileBasedSecureStorageManager) withCacheFile(action func(*os.File)) { cacheFile, err := os.OpenFile(ssm.credFilePath(), os.O_CREATE|os.O_RDWR, 0600) if err != nil { @@ -184,14 +197,8 @@ func (ssm *fileBasedSecureStorageManager) setCredential(tokenSpec *secureTokenSp logger.Warn(err) return } - err = ssm.lockFile() - if err != nil { - logger.Warnf("Set credential failed. Unable to lock cache. %v", err) - return - } - defer ssm.unlockFile() - ssm.withCacheFile(func(cacheFile *os.File) { + ssm.withLock(func(cacheFile *os.File) { credCache, err := ssm.readTemporaryCacheFile(cacheFile) if err != nil { logger.Warnf("Error while reading cache file. %v", err) @@ -233,7 +240,7 @@ func (ssm *fileBasedSecureStorageManager) lockFile() error { locked := false for i := 0; i < numRetries; i++ { - err := os.Mkdir(lockPath, 0o700) + err := os.Mkdir(lockPath, 0700) if err != nil { if errors.Is(err, os.ErrExist) { time.Sleep(retryInterval) @@ -264,15 +271,9 @@ func (ssm *fileBasedSecureStorageManager) getCredential(tokenSpec *secureTokenSp logger.Warn(err) return "" } - err = ssm.lockFile() - if err != nil { - logger.Warnf("Failed to lock credential cache file. %v", err) - return "" - } - defer ssm.unlockFile() ret := "" - ssm.withCacheFile(func(cacheFile *os.File) { + ssm.withLock(func(cacheFile *os.File) { credCache, err := ssm.readTemporaryCacheFile(cacheFile) if err != nil { logger.Warnf("Error while reading cache file. %v", err) @@ -303,7 +304,7 @@ func (ssm *fileBasedSecureStorageManager) ensurePermissions(cacheFile *os.File) return err } - if dirInfo.Mode().Perm() != 0o700&os.ModePerm { + if dirInfo.Mode().Perm() != 0700&os.ModePerm { return fmt.Errorf("incorrect permissions(%o, expected 700) for %s", dirInfo.Mode().Perm(), ssm.credDirPath) } @@ -312,7 +313,7 @@ func (ssm *fileBasedSecureStorageManager) ensurePermissions(cacheFile *os.File) return err } - if fileInfo.Mode().Perm() != 0o600&os.ModePerm { + if fileInfo.Mode().Perm() != 0600&os.ModePerm { return fmt.Errorf("incorrect permissions(%v, expected 600) for credential file", fileInfo.Mode().Perm()) } @@ -335,7 +336,7 @@ func (ssm *fileBasedSecureStorageManager) ensureOwnerForFile(file *os.File) erro return ssm.ensureOwner(ownerUID) } -func (ssm *fileBasedSecureStorageManager) ensureOwner(ownerId uint32) error { +func (ssm *fileBasedSecureStorageManager) ensureOwner(ownerID uint32) error { currentUser, err := user.Current() if err != nil { return err @@ -343,7 +344,7 @@ func (ssm *fileBasedSecureStorageManager) ensureOwner(ownerId uint32) error { if errors.Is(err, os.ErrNotExist) { return nil } - if strconv.Itoa(int(ownerId)) != currentUser.Uid { + if strconv.Itoa(int(ownerID)) != currentUser.Uid { return errors.New("incorrect owner of " + ssm.credDirPath) } return nil @@ -389,14 +390,8 @@ func (ssm *fileBasedSecureStorageManager) deleteCredential(tokenSpec *secureToke logger.Warn(err) return } - err = ssm.lockFile() - if err != nil { - logger.Warnf("Set credential failed. Unable to lock cache. %v", err) - return - } - defer ssm.unlockFile() - ssm.withCacheFile(func(cacheFile *os.File) { + ssm.withLock(func(cacheFile *os.File) { credCache, err := ssm.readTemporaryCacheFile(cacheFile) if err != nil { logger.Warnf("Error while reading cache file. %v", err) diff --git a/secure_storage_manager_test.go b/secure_storage_manager_test.go index 9dae69eea..73355ab18 100644 --- a/secure_storage_manager_test.go +++ b/secure_storage_manager_test.go @@ -119,7 +119,7 @@ func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) { t.Run("unlock stale cache", func(t *testing.T) { tokenSpec := newMfaTokenSpec("stale", "cache") - assertNilF(t, os.Mkdir(ssm.lockPath(), 0o700)) + assertNilF(t, os.Mkdir(ssm.lockPath(), 0700)) time.Sleep(1000 * time.Millisecond) ssm.setCredential(tokenSpec, "unlocked") assertEqualE(t, ssm.getCredential(tokenSpec), "unlocked") @@ -128,7 +128,7 @@ func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) { t.Run("wait for other process to unlock cache", func(t *testing.T) { tokenSpec := newMfaTokenSpec("stale", "cache") startTime := time.Now() - assertNilF(t, os.Mkdir(ssm.lockPath(), 0o700)) + assertNilF(t, os.Mkdir(ssm.lockPath(), 0700)) time.Sleep(500 * time.Millisecond) go func() { time.Sleep(500 * time.Millisecond) @@ -144,7 +144,7 @@ func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) { content := []byte(`{ "otherKey": "otherValue" }`) - err = os.WriteFile(ssm.credFilePath(), content, 0o600) + err = os.WriteFile(ssm.credFilePath(), content, 0600) assertNilF(t, err) ssm.setCredential(newMfaTokenSpec("somehost.com", "someUser"), "someToken") result, err := os.ReadFile(ssm.credFilePath()) From c72f84b1f3ad3d953de6eda43aa6c3b7a92c258c Mon Sep 17 00:00:00 2001 From: Piotr Fus Date: Thu, 13 Mar 2025 08:54:04 +0100 Subject: [PATCH 08/10] Fix dir perm verification --- os_specific_posix.go | 8 ---- os_specific_windows.go | 4 -- secure_storage_manager.go | 67 +++++++++++----------------------- secure_storage_manager_test.go | 24 +++++++++++- 4 files changed, 45 insertions(+), 58 deletions(-) diff --git a/os_specific_posix.go b/os_specific_posix.go index 1de40f3be..2d01f5c04 100644 --- a/os_specific_posix.go +++ b/os_specific_posix.go @@ -8,14 +8,6 @@ import ( "syscall" ) -func providePathOwner(filepath string) (uint32, error) { - info, err := os.Stat(filepath) - if err != nil { - return 0, err - } - return provideOwnerFromStat(info, filepath) -} - func provideFileOwner(file *os.File) (uint32, error) { info, err := file.Stat() if err != nil { diff --git a/os_specific_windows.go b/os_specific_windows.go index d30a27200..293123e08 100644 --- a/os_specific_windows.go +++ b/os_specific_windows.go @@ -7,10 +7,6 @@ import ( "os" ) -func providePathOwner(filepath string) (uint32, error) { - return 0, errors.New("providePathOwner is unsupported on windows") -} - func provideFileOwner(file *os.File) (uint32, error) { return 0, errors.New("provideFileOwner is unsupported on windows") } diff --git a/secure_storage_manager.go b/secure_storage_manager.go index ba914fc40..75d50df0e 100644 --- a/secure_storage_manager.go +++ b/secure_storage_manager.go @@ -180,7 +180,7 @@ 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) { @@ -188,6 +188,21 @@ func (ssm *fileBasedSecureStorageManager) withCacheFile(action func(*os.File)) { 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,45 +313,20 @@ 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 @@ -344,22 +334,13 @@ func (ssm *fileBasedSecureStorageManager) ensureOwner(ownerID uint32) error { 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) diff --git a/secure_storage_manager_test.go b/secure_storage_manager_test.go index 73355ab18..f9c85b0c7 100644 --- a/secure_storage_manager_test.go +++ b/secure_storage_manager_test.go @@ -69,7 +69,7 @@ func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) { defer credCacheDirEnvOverride.rollback() ssm, err := newFileBasedSecureStorageManager() assertNilF(t, err) - + t.Run("store single token", func(t *testing.T) { tokenSpec := newMfaTokenSpec("host.com", "johndoe") cred := "token123" @@ -173,6 +173,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) { From e0e2ab29094c33b36c18d1bc1a6bb020b047f0a4 Mon Sep 17 00:00:00 2001 From: Piotr Fus Date: Thu, 13 Mar 2025 09:13:27 +0100 Subject: [PATCH 09/10] Close cache dir file descriptor --- secure_storage_manager.go | 5 +++++ secure_storage_manager_test.go | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/secure_storage_manager.go b/secure_storage_manager.go index 75d50df0e..329f02d87 100644 --- a/secure_storage_manager.go +++ b/secure_storage_manager.go @@ -193,6 +193,11 @@ func (ssm *fileBasedSecureStorageManager) withCacheFile(action func(*os.File)) { if err != nil { logger.Warnf("cannot access %v. %v", ssm.credDirPath, err) } + defer func(file *os.File) { + if err := file.Close(); err != nil { + logger.Warnf("cannot release file descriptor for %v. %v", cacheDir, err) + } + }(cacheFile) if err := ssm.ensurePermissionsAndOwner(cacheFile, 0600); err != nil { logger.Warnf("failed to ensure permission for temporary cache file. %v", err) diff --git a/secure_storage_manager_test.go b/secure_storage_manager_test.go index f9c85b0c7..bcedc1248 100644 --- a/secure_storage_manager_test.go +++ b/secure_storage_manager_test.go @@ -69,7 +69,7 @@ func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) { defer credCacheDirEnvOverride.rollback() ssm, err := newFileBasedSecureStorageManager() assertNilF(t, err) - + t.Run("store single token", func(t *testing.T) { tokenSpec := newMfaTokenSpec("host.com", "johndoe") cred := "token123" From 057b65ed8a0df1b0d4267ed8b02c3bb87a693514 Mon Sep 17 00:00:00 2001 From: Piotr Fus Date: Thu, 13 Mar 2025 11:49:13 +0100 Subject: [PATCH 10/10] Fix order by --- arrow_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arrow_test.go b/arrow_test.go index 76a9a33e8..e8c0fd7ab 100644 --- a/arrow_test.go +++ b/arrow_test.go @@ -590,7 +590,7 @@ func TestArrowMemoryCleanedUp(t *testing.T) { mem, ) - rows := dbt.mustQueryContext(ctx, "select 1 UNION select 2") + rows := dbt.mustQueryContext(ctx, "select 1 UNION select 2 order by 1") defer rows.Close() var v int assertTrueF(t, rows.Next())