- Renamed cache file to credential_cache_v1.json

- Added sha256 checksum for credentials key
- Fixed some issues
- Added more tests

Author: sfc-gh-pfus

secure_storage_manager.go

@@ -3,11 +3,12 @@
 package gosnowflake
 
 import (
+	"crypto/sha256"
+	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"fmt"
 	"github.com/99designs/keyring"
-	"golang.org/x/sys/unix"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -24,9 +25,20 @@ const (
 
 const (
 	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
@@ -72,6 +84,7 @@ func newSecureStorageManager() secureStorageManager {
 	case "darwin", "windows":
 		return newKeyringBasedSecureStorageManager()
 	default:
+		logger.Infof("OS %v does not support credentials cache", runtime.GOOS)
 		return newNoopSecureStorageManager()
 	}
 }
@@ -81,9 +94,9 @@ type fileBasedSecureStorageManager struct {
 }
 
 func newFileBasedSecureStorageManager() (*fileBasedSecureStorageManager, error) {
-	credDirPath := buildCredCacheDirPath()
-	if credDirPath == "" {
-		return nil, fmt.Errorf("failed to build cache dir path")
+	credDirPath, err := buildCredCacheDirPath(defaultLinuxCacheDirConf)
+	if err != nil {
+		return nil, err
 	}
 	ssm := &fileBasedSecureStorageManager{
 		credDirPath: credDirPath,
@@ -110,27 +123,21 @@ func lookupCacheDir(envVar string, pathSegments ...string) (string, error) {
 
 	fileInfo, err := os.Stat(envVal)
 	if err != nil {
-		return "", fmt.Errorf("failed to stat %s=%s, due to %w", envVar, envVal, err)
+		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 := envVal
+	cacheDir := filepath.Join(envVal, filepath.Join(pathSegments...))
 
-	if len(pathSegments) > 0 {
-		for _, pathSegment := range pathSegments {
-			err := os.Mkdir(pathSegment, os.ModePerm)
-			if err != nil {
-				return "", fmt.Errorf("failed to create cache directory. %v, err: %w", pathSegment, err)
-			}
-			cacheDir = filepath.Join(cacheDir, pathSegment)
-		}
-		fileInfo, err = os.Stat(cacheDir)
-		if err != nil {
-			return "", fmt.Errorf("failed to stat %s=%s, due to %w", envVar, cacheDir, err)
-		}
+	if err = os.MkdirAll(cacheDir, os.FileMode(0o755)); 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() != 0o700 {
@@ -143,27 +150,18 @@ func lookupCacheDir(envVar string, pathSegments ...string) (string, error) {
 	return cacheDir, nil
 }
 
-func buildCredCacheDirPath() string {
-	type cacheDirConf struct {
-		envVar       string
-		pathSegments []string
-	}
-	confs := []cacheDirConf{
-		{envVar: credCacheDirEnv, pathSegments: []string{}},
-		{envVar: "XDG_CACHE_DIR", pathSegments: []string{"snowflake"}},
-		{envVar: "HOME", pathSegments: []string{".cache", "snowflake"}},
-	}
+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 %w", conf.envVar, err)
+			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
+			return path, nil
 		}
 	}
 
-	return ""
+	return "", errors.New("no credentials cache directory found")
 }
 
 func (ssm *fileBasedSecureStorageManager) getTokens(data map[string]any) map[string]interface{} {
@@ -198,26 +196,23 @@ func (ssm *fileBasedSecureStorageManager) setCredential(tokenSpec *secureTokenSp
 	err = ssm.writeTemporaryCacheFile(credCache)
 	if err != nil {
 		logger.Warnf("Set credential failed. Unable to write cache. %v", err)
-		return
 	}
-
-	return
 }
 
 func (ssm *fileBasedSecureStorageManager) lockPath() string {
 	return filepath.Join(ssm.credDirPath, credCacheFileName+".lck")
 }
 
 func (ssm *fileBasedSecureStorageManager) lockFile() error {
-	const NUM_RETRIES = 10
-	const RETRY_INTERVAL = 100 * time.Millisecond
+	const numRetries = 10
+	const retryInterval = 100 * time.Millisecond
 	lockPath := ssm.lockPath()
 	locked := false
-	for i := 0; i < NUM_RETRIES; i++ {
+	for i := 0; i < numRetries; i++ {
 		err := os.Mkdir(lockPath, 0o700)
 		if err != nil {
 			if errors.Is(err, os.ErrExist) {
-				time.Sleep(RETRY_INTERVAL)
+				time.Sleep(retryInterval)
 				continue
 			}
 			return fmt.Errorf("failed to create cache lock: %v, err: %v", lockPath, err)
@@ -228,13 +223,13 @@ func (ssm *fileBasedSecureStorageManager) lockFile() error {
 
 	if !locked {
 		logger.Warnf("failed to lock cache lock. lockPath: %v.", lockPath)
-		var stat unix.Stat_t
-		err := unix.Stat(lockPath, &stat)
-		if err != nil {
+		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 stat.Ctim.Nano()+time.Second.Nanoseconds() < time.Now().UnixNano() {
+		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)
@@ -290,7 +285,7 @@ func (ssm *fileBasedSecureStorageManager) ensurePermissions() error {
 	}
 
 	if dirInfo.Mode().Perm() != 0o700 {
-		return fmt.Errorf("incorrect permissions(%o, expected 700) for %s.", dirInfo.Mode().Perm(), ssm.credDirPath)
+		return fmt.Errorf("incorrect permissions(%o, expected 700) for %s", dirInfo.Mode().Perm(), ssm.credDirPath)
 	}
 
 	fileInfo, err := os.Stat(ssm.credFilePath())
@@ -302,7 +297,7 @@ func (ssm *fileBasedSecureStorageManager) ensurePermissions() error {
 		logger.Debugf("Incorrect permissions(%o, expected 600) for credential file.", fileInfo.Mode().Perm())
 		err := os.Chmod(ssm.credFilePath(), 0o600)
 		if err != nil {
-			return fmt.Errorf("Failed to chmod credential file: %v", err)
+			return fmt.Errorf("failed to chmod credential file: %v", err)
 		}
 		logger.Debug("Successfully fixed credential file permissions.")
 	}
@@ -324,7 +319,7 @@ func (ssm *fileBasedSecureStorageManager) readTemporaryCacheFile() map[string]an
 	}
 
 	credentialsMap := map[string]any{}
-	err = json.Unmarshal([]byte(jsonData), &credentialsMap)
+	err = json.Unmarshal(jsonData, &credentialsMap)
 	if err != nil {
 		logger.Warnf("Failed to unmarshal credential cache file. %v.\n", err)
 	}
@@ -347,10 +342,7 @@ func (ssm *fileBasedSecureStorageManager) deleteCredential(tokenSpec *secureToke
 	err = ssm.writeTemporaryCacheFile(credCache)
 	if err != nil {
 		logger.Warnf("Set credential failed. Unable to write cache. %v", err)
-		return
 	}
-
-	return
 }
 
 func (ssm *fileBasedSecureStorageManager) writeTemporaryCacheFile(cache map[string]any) error {
@@ -359,6 +351,18 @@ 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().String() != "-rw-------" {
+			if err = os.Chmod(ssm.credFilePath(), 0600); err != nil {
+				return fmt.Errorf("cannot chmod file %v to 600. %v", ssm.credFilePath(), err)
+			}
+		}
+	}
+
 	err = os.WriteFile(ssm.credFilePath(), bytes, 0600)
 	if err != nil {
 		return fmt.Errorf("failed to write the credential cache file: %w", err)
@@ -462,8 +466,9 @@ func (ssm *keyringSecureStorageManager) deleteCredential(tokenSpec *secureTokenS
 }
 
 func buildCredentialsKey(host, user string, credType tokenType) string {
-	credTypeStr := string(credType)
-	return host + ":" + user + ":" + credTypeStr
+	plainCredKey := host + ":" + user + ":" + string(credType)
+	checksum := sha256.New().Sum([]byte(plainCredKey))
+	return hex.EncodeToString(checksum)
 }
 
 type noopSecureStorageManager struct {

secure_storage_manager_test.go

@@ -4,44 +4,94 @@ package gosnowflake
 
 import (
 	"os"
+	"path/filepath"
 	"testing"
+	"time"
 )
 
-type EnvOverride struct {
-	env      string
-	oldValue string
-}
+func TestBuildCredCacheDirPath(t *testing.T) {
+	testRoot1, err := os.MkdirTemp("", "")
+	assertNilF(t, err)
+	defer os.RemoveAll(testRoot1)
+	testRoot2, err := os.MkdirTemp("", "")
+	assertNilF(t, err)
+	defer os.RemoveAll(testRoot2)
 
-func (e *EnvOverride) rollback() {
-	if e.oldValue != "" {
-		os.Setenv(e.env, e.oldValue)
-	} else {
-		os.Unsetenv(e.env)
-	}
-}
+	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().String(), "drwx------")
+	})
 
-func override_env(env string, value string) EnvOverride {
-	oldValue := os.Getenv(env)
-	os.Setenv(env, value)
-	return EnvOverride{env, oldValue}
+	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().String(), "drwx------")
+	})
 }
 
 func TestSnowflakeFileBasedSecureStorageManager(t *testing.T) {
-	//skipOnNonLinux(t, "Not supported on non-linux")
-	os.Mkdir("./testdata", 0777)
-	credCacheDirEnvOverride := override_env(credCacheDirEnv, "./testdata")
+	skipOnWindows(t, "file system permission is different")
+	credCacheDir, err := os.MkdirTemp("", "")
+	assertNilF(t, err)
+	assertNilF(t, os.MkdirAll(credCacheDir, 0777))
+	credCacheDirEnvOverride := overrideEnv(credCacheDirEnv, credCacheDir)
 	defer credCacheDirEnvOverride.rollback()
-	fbss, err := newFileBasedSecureStorageManager()
-	if err != nil {
-		t.Fatal(err)
-	}
+	ssm, err := newFileBasedSecureStorageManager()
+	assertNilF(t, err)
+
+	t.Run("success", 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("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)
+	})
 
-	tokenSpec := newMfaTokenSpec("host.xd", "johndoe")
-	cred := "token123"
-	fbss.setCredential(tokenSpec, cred)
-	assertEqualE(t, fbss.getCredential(tokenSpec), cred)
-	fbss.deleteCredential(tokenSpec)
-	assertEqualE(t, fbss.getCredential(tokenSpec), "")
+	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"`)
+	})
 }
 
 func TestSetAndGetCredentialMfa(t *testing.T) {