From 163dfa5caba06613c548b28b8c8d3c11b4c23539 Mon Sep 17 00:00:00 2001 From: zhaojunchang Date: Tue, 7 Apr 2026 14:45:16 +0800 Subject: [PATCH 1/7] feat: (MacOS) add fallback file-based master key storage --- internal/keychain/keychain_darwin.go | 64 ++++++++- internal/keychain/keychain_darwin_test.go | 154 ++++++++++++++++++++++ 2 files changed, 214 insertions(+), 4 deletions(-) create mode 100644 internal/keychain/keychain_darwin_test.go diff --git a/internal/keychain/keychain_darwin.go b/internal/keychain/keychain_darwin.go index ae0aebff..a0d1153f 100644 --- a/internal/keychain/keychain_darwin.go +++ b/internal/keychain/keychain_darwin.go @@ -26,6 +26,10 @@ const keychainTimeout = 5 * time.Second const masterKeyBytes = 32 const ivBytes = 12 const tagBytes = 16 +const fileMasterKeyName = "master.key.file" + +var keyringGet = keyring.Get +var keyringSet = keyring.Set // StorageDir returns the storage directory for a given service name on macOS. func StorageDir(service string) string { @@ -57,7 +61,7 @@ func getMasterKey(service string, allowCreate bool) ([]byte, error) { go func() { defer func() { recover() }() - encodedKey, err := keyring.Get(service, "master.key") + encodedKey, err := keyringGet(service, "master.key") if err == nil { key, decodeErr := base64.StdEncoding.DecodeString(encodedKey) if decodeErr == nil && len(key) == masterKeyBytes { @@ -88,7 +92,7 @@ func getMasterKey(service string, allowCreate bool) ([]byte, error) { } encodedKeyStr := base64.StdEncoding.EncodeToString(key) - setErr := keyring.Set(service, "master.key", encodedKeyStr) + setErr := keyringSet(service, "master.key", encodedKeyStr) if setErr != nil { resCh <- result{key: nil, err: setErr} return @@ -105,6 +109,47 @@ func getMasterKey(service string, allowCreate bool) ([]byte, error) { } } +// getFileMasterKey retrieves the fallback master key from local storage. +// If allowCreate is true, it generates and stores a new fallback master key when missing. +func getFileMasterKey(service string, allowCreate bool) ([]byte, error) { + dir := StorageDir(service) + keyPath := filepath.Join(dir, fileMasterKeyName) + + key, err := os.ReadFile(keyPath) + if err == nil && len(key) == masterKeyBytes { + return key, nil + } + if err == nil && len(key) != masterKeyBytes { + return nil, errors.New("keychain is corrupted") + } + if err != nil && !errors.Is(err, os.ErrNotExist) { + return nil, err + } + if !allowCreate { + return nil, errNotInitialized + } + if err := os.MkdirAll(dir, 0700); err != nil { + return nil, err + } + key = make([]byte, masterKeyBytes) + if _, err := rand.Read(key); err != nil { + return nil, err + } + tmpKeyPath := filepath.Join(dir, fileMasterKeyName+"."+uuid.New().String()+".tmp") + defer os.Remove(tmpKeyPath) + if err := os.WriteFile(tmpKeyPath, key, 0600); err != nil { + return nil, err + } + if err := os.Rename(tmpKeyPath, keyPath); err != nil { + existingKey, readErr := os.ReadFile(keyPath) + if readErr == nil && len(existingKey) == masterKeyBytes { + return existingKey, nil + } + return nil, err + } + return key, nil +} + // encryptData encrypts data using AES-GCM. func encryptData(plaintext string, key []byte) ([]byte, error) { block, err := aes.NewCipher(key) @@ -161,6 +206,11 @@ func platformGet(service, account string) (string, error) { if err != nil { return "", err } + if key, ferr := getFileMasterKey(service, false); ferr == nil { + if plaintext, derr := decryptData(data, key); derr == nil { + return plaintext, nil + } + } key, err := getMasterKey(service, false) if err != nil { return "", err @@ -174,9 +224,15 @@ func platformGet(service, account string) (string, error) { // platformSet stores a value in the macOS keychain. func platformSet(service, account, data string) error { - key, err := getMasterKey(service, true) + key, err := getFileMasterKey(service, false) if err != nil { - return err + key, err = getMasterKey(service, true) + if err != nil { + key, err = getFileMasterKey(service, true) + if err != nil { + return err + } + } } dir := StorageDir(service) if err := vfs.MkdirAll(dir, 0700); err != nil { diff --git a/internal/keychain/keychain_darwin_test.go b/internal/keychain/keychain_darwin_test.go new file mode 100644 index 00000000..90b774e7 --- /dev/null +++ b/internal/keychain/keychain_darwin_test.go @@ -0,0 +1,154 @@ +//go:build darwin + +package keychain + +import ( + "encoding/base64" + "errors" + "os" + "path/filepath" + "testing" + + "github.com/zalando/go-keyring" +) + +func TestPlatformSetFallsBackToFileMasterKey(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + origGet := keyringGet + origSet := keyringSet + keyringGet = func(service, user string) (string, error) { + return "", keyring.ErrNotFound + } + keyringSet = func(service, user, password string) error { + return errors.New("blocked") + } + t.Cleanup(func() { + keyringGet = origGet + keyringSet = origSet + }) + + service := "test-service" + account := "test-account" + secret := "secret-value" + + if err := platformSet(service, account, secret); err != nil { + t.Fatalf("platformSet() error = %v", err) + } + + if _, err := os.Stat(filepath.Join(StorageDir(service), fileMasterKeyName)); err != nil { + t.Fatalf("file master key not created: %v", err) + } + + got, err := platformGet(service, account) + if err != nil { + t.Fatalf("platformGet() error = %v", err) + } + if got != secret { + t.Fatalf("platformGet() = %q, want %q", got, secret) + } +} + +func TestPlatformGetPrefersFileMasterKey(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + fileKey := make([]byte, masterKeyBytes) + for i := range fileKey { + fileKey[i] = byte(i + 1) + } + keychainKey := make([]byte, masterKeyBytes) + for i := range keychainKey { + keychainKey[i] = byte(i + 33) + } + + origGet := keyringGet + origSet := keyringSet + keyringGet = func(service, user string) (string, error) { + return base64.StdEncoding.EncodeToString(keychainKey), nil + } + keyringSet = func(service, user, password string) error { + return nil + } + t.Cleanup(func() { + keyringGet = origGet + keyringSet = origSet + }) + + service := "test-service" + account := "test-account" + secret := "secret-value" + + dir := StorageDir(service) + if err := os.MkdirAll(dir, 0700); err != nil { + t.Fatalf("MkdirAll() error = %v", err) + } + if err := os.WriteFile(filepath.Join(dir, fileMasterKeyName), fileKey, 0600); err != nil { + t.Fatalf("WriteFile(master key) error = %v", err) + } + encrypted, err := encryptData(secret, fileKey) + if err != nil { + t.Fatalf("encryptData() error = %v", err) + } + if err := os.WriteFile(filepath.Join(dir, safeFileName(account)), encrypted, 0600); err != nil { + t.Fatalf("WriteFile(secret) error = %v", err) + } + + got, err := platformGet(service, account) + if err != nil { + t.Fatalf("platformGet() error = %v", err) + } + if got != secret { + t.Fatalf("platformGet() = %q, want %q", got, secret) + } +} + +func TestPlatformSetPrefersExistingFileMasterKey(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + origGet := keyringGet + origSet := keyringSet + keyringGet = func(service, user string) (string, error) { + t.Fatalf("keyringGet should not be called when file master key exists") + return "", nil + } + keyringSet = func(service, user, password string) error { + t.Fatalf("keyringSet should not be called when file master key exists") + return nil + } + t.Cleanup(func() { + keyringGet = origGet + keyringSet = origSet + }) + + service := "test-service" + account := "test-account" + secret := "secret-value" + + dir := StorageDir(service) + if err := os.MkdirAll(dir, 0700); err != nil { + t.Fatalf("MkdirAll() error = %v", err) + } + + fileKey := make([]byte, masterKeyBytes) + for i := range fileKey { + fileKey[i] = byte(i + 1) + } + if err := os.WriteFile(filepath.Join(dir, fileMasterKeyName), fileKey, 0600); err != nil { + t.Fatalf("WriteFile(master key) error = %v", err) + } + + if err := platformSet(service, account, secret); err != nil { + t.Fatalf("platformSet() error = %v", err) + } + + got, err := platformGet(service, account) + if err != nil { + t.Fatalf("platformGet() error = %v", err) + } + if got != secret { + t.Fatalf("platformGet() = %q, want %q", got, secret) + } +} From 5f3cd1df2248ba76666a68bf9b1d6aebbd08e971 Mon Sep 17 00:00:00 2001 From: zhaojunchang Date: Tue, 7 Apr 2026 15:10:40 +0800 Subject: [PATCH 2/7] refactor(keychain): improve master key file handling and corruption checks - Replace temporary file approach with direct file creation - Add explicit corruption checks for existing keys - Ensure atomic operations and proper cleanup on failure --- internal/keychain/keychain_darwin.go | 43 ++++++++++++++++++++--- internal/keychain/keychain_darwin_test.go | 6 ++++ internal/keychain/keychain_other.go | 4 ++- 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/internal/keychain/keychain_darwin.go b/internal/keychain/keychain_darwin.go index a0d1153f..5ded776e 100644 --- a/internal/keychain/keychain_darwin.go +++ b/internal/keychain/keychain_darwin.go @@ -135,16 +135,51 @@ func getFileMasterKey(service string, allowCreate bool) ([]byte, error) { if _, err := rand.Read(key); err != nil { return nil, err } - tmpKeyPath := filepath.Join(dir, fileMasterKeyName+"."+uuid.New().String()+".tmp") - defer os.Remove(tmpKeyPath) - if err := os.WriteFile(tmpKeyPath, key, 0600); err != nil { + + file, err := os.OpenFile(keyPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) + if err != nil { + if errors.Is(err, os.ErrExist) { + existingKey, readErr := os.ReadFile(keyPath) + if readErr == nil && len(existingKey) == masterKeyBytes { + return existingKey, nil + } + if readErr == nil && len(existingKey) != masterKeyBytes { + return nil, errors.New("keychain is corrupted") + } + } + return nil, err + } + + writeFailed := true + defer func() { + if writeFailed { + _ = os.Remove(keyPath) + } + }() + if _, err := file.Write(key); err != nil { + _ = file.Close() + return nil, err + } + if err := file.Close(); err != nil { return nil, err } - if err := os.Rename(tmpKeyPath, keyPath); err != nil { + writeFailed = false + + canonicalKey, err := os.ReadFile(keyPath) + if err == nil && len(canonicalKey) == masterKeyBytes { + return canonicalKey, nil + } + if err == nil && len(canonicalKey) != masterKeyBytes { + return nil, errors.New("keychain is corrupted") + } + if err != nil { existingKey, readErr := os.ReadFile(keyPath) if readErr == nil && len(existingKey) == masterKeyBytes { return existingKey, nil } + if readErr == nil && len(existingKey) != masterKeyBytes { + return nil, errors.New("keychain is corrupted") + } return nil, err } return key, nil diff --git a/internal/keychain/keychain_darwin_test.go b/internal/keychain/keychain_darwin_test.go index 90b774e7..0ae77947 100644 --- a/internal/keychain/keychain_darwin_test.go +++ b/internal/keychain/keychain_darwin_test.go @@ -12,6 +12,8 @@ import ( "github.com/zalando/go-keyring" ) +// TestPlatformSetFallsBackToFileMasterKey verifies writes fall back to a file master key +// when the system keychain cannot create the master key. func TestPlatformSetFallsBackToFileMasterKey(t *testing.T) { home := t.TempDir() t.Setenv("HOME", home) @@ -50,6 +52,8 @@ func TestPlatformSetFallsBackToFileMasterKey(t *testing.T) { } } +// TestPlatformGetPrefersFileMasterKey verifies reads prefer the file-based master key +// before trying the system keychain master key. func TestPlatformGetPrefersFileMasterKey(t *testing.T) { home := t.TempDir() t.Setenv("HOME", home) @@ -104,6 +108,8 @@ func TestPlatformGetPrefersFileMasterKey(t *testing.T) { } } +// TestPlatformSetPrefersExistingFileMasterKey verifies writes stay on the file-based +// master key path once the fallback master key already exists. func TestPlatformSetPrefersExistingFileMasterKey(t *testing.T) { home := t.TempDir() t.Setenv("HOME", home) diff --git a/internal/keychain/keychain_other.go b/internal/keychain/keychain_other.go index d84ad84b..9a1efa18 100644 --- a/internal/keychain/keychain_other.go +++ b/internal/keychain/keychain_other.go @@ -88,9 +88,11 @@ func getMasterKey(service string, allowCreate bool) ([]byte, error) { if readErr == nil && len(existingKey) == masterKeyBytes { return existingKey, nil } + if readErr == nil && len(existingKey) != masterKeyBytes { + return nil, errors.New("keychain is corrupted") + } return nil, err } - return key, nil } From f8876ff07640adcd78aceea868b912248fe205f7 Mon Sep 17 00:00:00 2001 From: zhaojunchang Date: Tue, 7 Apr 2026 15:22:10 +0800 Subject: [PATCH 3/7] docs(keychain): add comments to clarify constants and variables Add descriptive comments to explain the purpose of timeout, crypto parameters, and test variables in the macOS keychain implementation. --- internal/keychain/keychain_darwin.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/keychain/keychain_darwin.go b/internal/keychain/keychain_darwin.go index 5ded776e..38b96de1 100644 --- a/internal/keychain/keychain_darwin.go +++ b/internal/keychain/keychain_darwin.go @@ -22,13 +22,25 @@ import ( "github.com/zalando/go-keyring" ) +// keychainTimeout bounds system keychain access to avoid hanging on blocked prompts. const keychainTimeout = 5 * time.Second + +// masterKeyBytes is the AES-256 key size used to encrypt stored secrets. const masterKeyBytes = 32 + +// ivBytes is the nonce size used by AES-GCM. const ivBytes = 12 + +// tagBytes is the authentication tag size produced by AES-GCM. const tagBytes = 16 + +// fileMasterKeyName is the local fallback master key file name. const fileMasterKeyName = "master.key.file" +// keyringGet is overridden in tests to simulate system keychain reads. var keyringGet = keyring.Get + +// keyringSet is overridden in tests to simulate system keychain writes. var keyringSet = keyring.Set // StorageDir returns the storage directory for a given service name on macOS. From d9960ea2fae690fd0d84fa06284199dcd57febcc Mon Sep 17 00:00:00 2001 From: zhaojunchang Date: Tue, 7 Apr 2026 15:25:46 +0800 Subject: [PATCH 4/7] fix(keychain): use atomic write for master key initialization --- internal/keychain/keychain_other.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/keychain/keychain_other.go b/internal/keychain/keychain_other.go index 9a1efa18..d84ad84b 100644 --- a/internal/keychain/keychain_other.go +++ b/internal/keychain/keychain_other.go @@ -88,11 +88,9 @@ func getMasterKey(service string, allowCreate bool) ([]byte, error) { if readErr == nil && len(existingKey) == masterKeyBytes { return existingKey, nil } - if readErr == nil && len(existingKey) != masterKeyBytes { - return nil, errors.New("keychain is corrupted") - } return nil, err } + return key, nil } From 4c8b49237770409f3ccd159d37b5b475d73882c5 Mon Sep 17 00:00:00 2001 From: zhaojunchang Date: Tue, 7 Apr 2026 16:59:47 +0800 Subject: [PATCH 5/7] fix(keychain): add retry logic for reading master key file Add retry mechanism when reading existing master key file to handle potential race conditions. Return early if read error occurs instead of waiting for all retries. --- internal/keychain/keychain_darwin.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/keychain/keychain_darwin.go b/internal/keychain/keychain_darwin.go index 38b96de1..dd01ea3f 100644 --- a/internal/keychain/keychain_darwin.go +++ b/internal/keychain/keychain_darwin.go @@ -151,13 +151,19 @@ func getFileMasterKey(service string, allowCreate bool) ([]byte, error) { file, err := os.OpenFile(keyPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) if err != nil { if errors.Is(err, os.ErrExist) { - existingKey, readErr := os.ReadFile(keyPath) - if readErr == nil && len(existingKey) == masterKeyBytes { - return existingKey, nil - } - if readErr == nil && len(existingKey) != masterKeyBytes { - return nil, errors.New("keychain is corrupted") + for i := 0; i < 3; i++ { + existingKey, readErr := os.ReadFile(keyPath) + if readErr == nil && len(existingKey) == masterKeyBytes { + return existingKey, nil + } + if readErr != nil { + return nil, readErr + } + if i < 2 { + time.Sleep(5 * time.Millisecond) + } } + return nil, errors.New("keychain is corrupted") } return nil, err } From 645eef4e6c6de291fe33419b65d00b0e7bd79325 Mon Sep 17 00:00:00 2001 From: zhaojunchang Date: Tue, 7 Apr 2026 17:13:15 +0800 Subject: [PATCH 6/7] refactor(keychain): simplify master key validation logic Restructure the key validation flow to reduce redundant checks and improve readability. The corrupted key check is moved after the error handling block for better logical flow. --- internal/keychain/keychain_darwin.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/internal/keychain/keychain_darwin.go b/internal/keychain/keychain_darwin.go index dd01ea3f..c98a725d 100644 --- a/internal/keychain/keychain_darwin.go +++ b/internal/keychain/keychain_darwin.go @@ -184,12 +184,6 @@ func getFileMasterKey(service string, allowCreate bool) ([]byte, error) { writeFailed = false canonicalKey, err := os.ReadFile(keyPath) - if err == nil && len(canonicalKey) == masterKeyBytes { - return canonicalKey, nil - } - if err == nil && len(canonicalKey) != masterKeyBytes { - return nil, errors.New("keychain is corrupted") - } if err != nil { existingKey, readErr := os.ReadFile(keyPath) if readErr == nil && len(existingKey) == masterKeyBytes { @@ -200,7 +194,10 @@ func getFileMasterKey(service string, allowCreate bool) ([]byte, error) { } return nil, err } - return key, nil + if len(canonicalKey) != masterKeyBytes { + return nil, errors.New("keychain is corrupted") + } + return canonicalKey, nil } // encryptData encrypts data using AES-GCM. From 872aa63f1c7551e54e38bc14a0cf5d9c01a46d77 Mon Sep 17 00:00:00 2001 From: zhaojunchang Date: Tue, 7 Apr 2026 18:12:50 +0800 Subject: [PATCH 7/7] refactor(keychain): replace os package with vfs for file operations Use vfs package instead of os for file operations to improve testability and abstract filesystem access. This change makes it easier to mock filesystem operations in tests and provides a consistent interface for file handling. --- internal/keychain/keychain_darwin.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/keychain/keychain_darwin.go b/internal/keychain/keychain_darwin.go index c98a725d..8e92e2bc 100644 --- a/internal/keychain/keychain_darwin.go +++ b/internal/keychain/keychain_darwin.go @@ -127,7 +127,7 @@ func getFileMasterKey(service string, allowCreate bool) ([]byte, error) { dir := StorageDir(service) keyPath := filepath.Join(dir, fileMasterKeyName) - key, err := os.ReadFile(keyPath) + key, err := vfs.ReadFile(keyPath) if err == nil && len(key) == masterKeyBytes { return key, nil } @@ -140,7 +140,7 @@ func getFileMasterKey(service string, allowCreate bool) ([]byte, error) { if !allowCreate { return nil, errNotInitialized } - if err := os.MkdirAll(dir, 0700); err != nil { + if err := vfs.MkdirAll(dir, 0700); err != nil { return nil, err } key = make([]byte, masterKeyBytes) @@ -148,11 +148,11 @@ func getFileMasterKey(service string, allowCreate bool) ([]byte, error) { return nil, err } - file, err := os.OpenFile(keyPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) + file, err := vfs.OpenFile(keyPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) if err != nil { if errors.Is(err, os.ErrExist) { for i := 0; i < 3; i++ { - existingKey, readErr := os.ReadFile(keyPath) + existingKey, readErr := vfs.ReadFile(keyPath) if readErr == nil && len(existingKey) == masterKeyBytes { return existingKey, nil } @@ -171,7 +171,7 @@ func getFileMasterKey(service string, allowCreate bool) ([]byte, error) { writeFailed := true defer func() { if writeFailed { - _ = os.Remove(keyPath) + _ = vfs.Remove(keyPath) } }() if _, err := file.Write(key); err != nil { @@ -183,9 +183,9 @@ func getFileMasterKey(service string, allowCreate bool) ([]byte, error) { } writeFailed = false - canonicalKey, err := os.ReadFile(keyPath) + canonicalKey, err := vfs.ReadFile(keyPath) if err != nil { - existingKey, readErr := os.ReadFile(keyPath) + existingKey, readErr := vfs.ReadFile(keyPath) if readErr == nil && len(existingKey) == masterKeyBytes { return existingKey, nil }