From 77651d803c652676c66bc1286a4d8202a02df530 Mon Sep 17 00:00:00 2001 From: Jascha Date: Wed, 17 Dec 2025 08:05:17 +0100 Subject: [PATCH 1/2] Add auto option that falls back to pgpass on keychain failures (e.g. not installed) --- internal/tiger/common/connection.go | 2 + internal/tiger/common/password_storage.go | 87 +++++++- .../tiger/common/password_storage_test.go | 188 +++++++++++++++++- internal/tiger/config/config.go | 6 +- 4 files changed, 277 insertions(+), 6 deletions(-) diff --git a/internal/tiger/common/connection.go b/internal/tiger/common/connection.go index 8b55ac40..901a80a3 100644 --- a/internal/tiger/common/connection.go +++ b/internal/tiger/common/connection.go @@ -100,6 +100,8 @@ func GetPassword(service api.Service, role string) (string, error) { return "", fmt.Errorf("no password found in keyring for this service") case *PgpassStorage: return "", fmt.Errorf("no password found in ~/.pgpass for this service") + case *AutoFallbackStorage: + return "", fmt.Errorf("no password found for this service (checked keyring and ~/.pgpass)") default: return "", fmt.Errorf("failed to retrieve password: %w", err) } diff --git a/internal/tiger/common/password_storage.go b/internal/tiger/common/password_storage.go index 89d2dc0b..32a051cd 100644 --- a/internal/tiger/common/password_storage.go +++ b/internal/tiger/common/password_storage.go @@ -330,6 +330,82 @@ func (n *NoStorage) GetStorageResult(err error, password string) PasswordStorage } } +// AutoFallbackStorage tries keyring first, falls back to pgpass on any error +type AutoFallbackStorage struct { + keyring *KeyringStorage + pgpass *PgpassStorage + lastMethod string // tracks which method was used for GetStorageResult +} + +func (a *AutoFallbackStorage) Save(service api.Service, password string, role string) error { + // Try keyring first + if err := a.keyring.Save(service, password, role); err == nil { + a.lastMethod = "keyring" + return nil + } + // Any keyring error -> fall back to pgpass + if err := a.pgpass.Save(service, password, role); err != nil { + a.lastMethod = "auto" + return err + } + a.lastMethod = "pgpass" + return nil +} + +func (a *AutoFallbackStorage) Get(service api.Service, role string) (string, error) { + // Try keyring first + if password, err := a.keyring.Get(service, role); err == nil { + return password, nil + } + // Any keyring error -> try pgpass + return a.pgpass.Get(service, role) +} + +func (a *AutoFallbackStorage) Remove(service api.Service, role string) error { + // Try to remove from both (best effort) + keyringErr := a.keyring.Remove(service, role) + pgpassErr := a.pgpass.Remove(service, role) + + // Success if removed from at least one location + if keyringErr == nil || pgpassErr == nil { + return nil + } + // If both failed, return a combined error + return fmt.Errorf("keyring: %v, pgpass: %v", keyringErr, pgpassErr) +} + +func (a *AutoFallbackStorage) GetStorageResult(err error, password string) PasswordStorageResult { + if err != nil { + return PasswordStorageResult{ + Success: false, + Method: a.lastMethod, + Message: fmt.Sprintf("Failed to save password: %s", sanitizeErrorMessage(err, password)), + } + } + + // Return method-specific success message + switch a.lastMethod { + case "keyring": + return PasswordStorageResult{ + Success: true, + Method: "keyring", + Message: "Password saved to system keyring for automatic authentication", + } + case "pgpass": + return PasswordStorageResult{ + Success: true, + Method: "pgpass", + Message: "Password saved to ~/.pgpass for automatic authentication", + } + default: + return PasswordStorageResult{ + Success: true, + Method: "auto", + Message: "Password saved for automatic authentication", + } + } +} + // GetPasswordStorage returns the appropriate PasswordStorage implementation based on configuration func GetPasswordStorage() PasswordStorage { storageMethod := viper.GetString("password_storage") @@ -340,8 +416,17 @@ func GetPasswordStorage() PasswordStorage { return &PgpassStorage{} case "none": return &NoStorage{} + case "auto": + return &AutoFallbackStorage{ + keyring: &KeyringStorage{}, + pgpass: &PgpassStorage{}, + } default: - return &KeyringStorage{} // Default to keyring + // Default to auto for best compatibility across environments + return &AutoFallbackStorage{ + keyring: &KeyringStorage{}, + pgpass: &PgpassStorage{}, + } } } diff --git a/internal/tiger/common/password_storage_test.go b/internal/tiger/common/password_storage_test.go index b04d28f2..70bd30d6 100644 --- a/internal/tiger/common/password_storage_test.go +++ b/internal/tiger/common/password_storage_test.go @@ -295,8 +295,9 @@ func TestGetPasswordStorage(t *testing.T) { {"keyring", "keyring", "*common.KeyringStorage"}, {"pgpass", "pgpass", "*common.PgpassStorage"}, {"none", "none", "*common.NoStorage"}, - {"default", "", "*common.KeyringStorage"}, // Default case - {"invalid", "invalid", "*common.KeyringStorage"}, // Falls back to default + {"auto", "auto", "*common.AutoFallbackStorage"}, + {"default", "", "*common.AutoFallbackStorage"}, // Default case now uses auto + {"invalid", "invalid", "*common.AutoFallbackStorage"}, // Falls back to auto } for _, tt := range tests { @@ -937,3 +938,186 @@ func TestSanitizeErrorMessage(t *testing.T) { }) } } + +// Test AutoFallbackStorage using pgpass as fallback (since keyring may not be available in CI) +func TestAutoFallbackStorage_Integration(t *testing.T) { + // Set up test service name for keyring + config.SetTestServiceName(t) + + // Create a temporary directory for pgpass + tempDir := t.TempDir() + originalHome := os.Getenv("HOME") + os.Setenv("HOME", tempDir) + defer os.Setenv("HOME", originalHome) + + storage := &AutoFallbackStorage{ + keyring: &KeyringStorage{}, + pgpass: &PgpassStorage{}, + } + service := createTestService("auto-fallback-test-service") + password := "test-password-auto" + role := "tsdbadmin" + + // Test Save - should succeed using either keyring or pgpass + err := storage.Save(service, password, role) + if err != nil { + t.Fatalf("AutoFallbackStorage.Save() failed: %v", err) + } + + // Verify lastMethod is set to either keyring or pgpass + if storage.lastMethod != "keyring" && storage.lastMethod != "pgpass" { + t.Errorf("AutoFallbackStorage.lastMethod = %q, want 'keyring' or 'pgpass'", storage.lastMethod) + } + + // Test Get - should retrieve the password + retrieved, err := storage.Get(service, role) + if err != nil { + t.Fatalf("AutoFallbackStorage.Get() failed: %v", err) + } + if retrieved != password { + t.Errorf("AutoFallbackStorage.Get() = %q, want %q", retrieved, password) + } + + // Test Remove - should succeed + err = storage.Remove(service, role) + if err != nil { + t.Fatalf("AutoFallbackStorage.Remove() failed: %v", err) + } + + // Verify password is gone + _, err = storage.Get(service, role) + if err == nil { + t.Error("AutoFallbackStorage.Get() should fail after Remove()") + } +} + +// Test AutoFallbackStorage GetStorageResult with keyring success +func TestAutoFallbackStorage_GetStorageResult_KeyringSuccess(t *testing.T) { + storage := &AutoFallbackStorage{ + keyring: &KeyringStorage{}, + pgpass: &PgpassStorage{}, + lastMethod: "keyring", + } + testPassword := "test-password" + + result := storage.GetStorageResult(nil, testPassword) + if !result.Success { + t.Errorf("GetStorageResult() success should be true, got: %t", result.Success) + } + if result.Method != "keyring" { + t.Errorf("GetStorageResult() method should be 'keyring', got: %s", result.Method) + } + if !strings.Contains(result.Message, "Password saved to system keyring") { + t.Errorf("GetStorageResult() should mention keyring, got: %s", result.Message) + } + if strings.Contains(result.Message, testPassword) { + t.Error("GetStorageResult() should never include actual passwords") + } +} + +// Test AutoFallbackStorage GetStorageResult with pgpass fallback success +func TestAutoFallbackStorage_GetStorageResult_PgpassSuccess(t *testing.T) { + storage := &AutoFallbackStorage{ + keyring: &KeyringStorage{}, + pgpass: &PgpassStorage{}, + lastMethod: "pgpass", + } + testPassword := "test-password" + + result := storage.GetStorageResult(nil, testPassword) + if !result.Success { + t.Errorf("GetStorageResult() success should be true, got: %t", result.Success) + } + if result.Method != "pgpass" { + t.Errorf("GetStorageResult() method should be 'pgpass', got: %s", result.Method) + } + if !strings.Contains(result.Message, "Password saved to ~/.pgpass") { + t.Errorf("GetStorageResult() should mention pgpass, got: %s", result.Message) + } + if strings.Contains(result.Message, testPassword) { + t.Error("GetStorageResult() should never include actual passwords") + } +} + +// Test AutoFallbackStorage GetStorageResult with failure +func TestAutoFallbackStorage_GetStorageResult_Failure(t *testing.T) { + storage := &AutoFallbackStorage{ + keyring: &KeyringStorage{}, + pgpass: &PgpassStorage{}, + lastMethod: "auto", + } + testPassword := "test-password" + testError := fmt.Errorf("both storage methods failed") + + result := storage.GetStorageResult(testError, testPassword) + if result.Success { + t.Errorf("GetStorageResult() success should be false, got: %t", result.Success) + } + if result.Method != "auto" { + t.Errorf("GetStorageResult() method should be 'auto', got: %s", result.Method) + } + if !strings.Contains(result.Message, "Failed to save password") { + t.Errorf("GetStorageResult() should mention failure, got: %s", result.Message) + } + if strings.Contains(result.Message, testPassword) { + t.Error("GetStorageResult() should never include actual passwords") + } +} + +// Test AutoFallbackStorage Remove succeeds if at least one location succeeds +func TestAutoFallbackStorage_Remove_PartialSuccess(t *testing.T) { + // Set up test service name for keyring + config.SetTestServiceName(t) + + // Create a temporary directory for pgpass + tempDir := t.TempDir() + originalHome := os.Getenv("HOME") + os.Setenv("HOME", tempDir) + defer os.Setenv("HOME", originalHome) + + // Save password only to pgpass (not keyring) + pgpassStorage := &PgpassStorage{} + service := createTestService("partial-remove-test") + password := "test-password" + role := "tsdbadmin" + + err := pgpassStorage.Save(service, password, role) + if err != nil { + t.Fatalf("PgpassStorage.Save() failed: %v", err) + } + + // Now remove using AutoFallbackStorage - should succeed even if keyring fails + autoStorage := &AutoFallbackStorage{ + keyring: &KeyringStorage{}, + pgpass: &PgpassStorage{}, + } + + err = autoStorage.Remove(service, role) + if err != nil { + t.Errorf("AutoFallbackStorage.Remove() should succeed when at least one location succeeds, got: %v", err) + } +} + +// Security test: Ensure AutoFallbackStorage never includes passwords in messages +func TestAutoFallbackStorage_SecurityTest_NoPasswordInResults(t *testing.T) { + testPassword := "super-secret-password-123" + testError := fmt.Errorf("failed to save %s", testPassword) + + storage := &AutoFallbackStorage{ + keyring: &KeyringStorage{}, + pgpass: &PgpassStorage{}, + lastMethod: "auto", + } + + // Test success case + successResult := storage.GetStorageResult(nil, testPassword) + if strings.Contains(successResult.Message, testPassword) { + t.Errorf("GetStorageResult() success should never include password, got: %s", successResult.Message) + } + + // Test error case + errorResult := storage.GetStorageResult(testError, testPassword) + if strings.Contains(errorResult.Message, testPassword) { + t.Errorf("GetStorageResult() error should never include password, got: %s", errorResult.Message) + } +} diff --git a/internal/tiger/config/config.go b/internal/tiger/config/config.go index ad261725..d843a747 100644 --- a/internal/tiger/config/config.go +++ b/internal/tiger/config/config.go @@ -66,7 +66,7 @@ const ( DefaultDocsMCPURL = "https://mcp.tigerdata.com/docs" DefaultGatewayURL = "https://console.cloud.timescale.com/api" DefaultOutput = "table" - DefaultPasswordStorage = "keyring" + DefaultPasswordStorage = "auto" DefaultReleasesURL = "https://cli.tigerdata.com" DefaultVersionCheckInterval = 24 * time.Hour ) @@ -368,8 +368,8 @@ func (c *Config) UpdateField(key string, value any) (any, error) { if !ok { return nil, fmt.Errorf("password_storage must be string, got %T", value) } - if s != "keyring" && s != "pgpass" && s != "none" { - return nil, fmt.Errorf("invalid password_storage value: %s (must be keyring, pgpass, or none)", s) + if s != "auto" && s != "keyring" && s != "pgpass" && s != "none" { + return nil, fmt.Errorf("invalid password_storage value: %s (must be auto, keyring, pgpass, or none)", s) } c.PasswordStorage = s validated = s From 511cb544fb32bf88e41732192b6e0f0d2b2c9f0a Mon Sep 17 00:00:00 2001 From: Jascha Date: Fri, 19 Dec 2025 10:58:11 +0100 Subject: [PATCH 2/2] Cleanup code slightly --- internal/tiger/common/password_storage.go | 58 ++++---- .../tiger/common/password_storage_test.go | 133 +----------------- 2 files changed, 35 insertions(+), 156 deletions(-) diff --git a/internal/tiger/common/password_storage.go b/internal/tiger/common/password_storage.go index 32a051cd..41413288 100644 --- a/internal/tiger/common/password_storage.go +++ b/internal/tiger/common/password_storage.go @@ -2,6 +2,7 @@ package common import ( "bufio" + "errors" "fmt" "os" "path/filepath" @@ -339,26 +340,44 @@ type AutoFallbackStorage struct { func (a *AutoFallbackStorage) Save(service api.Service, password string, role string) error { // Try keyring first - if err := a.keyring.Save(service, password, role); err == nil { - a.lastMethod = "keyring" + a.lastMethod = "keyring" + keyringErr := a.keyring.Save(service, password, role) + if keyringErr == nil { return nil } + // Any keyring error -> fall back to pgpass - if err := a.pgpass.Save(service, password, role); err != nil { - a.lastMethod = "auto" - return err - } a.lastMethod = "pgpass" - return nil + pgpassErr := a.pgpass.Save(service, password, role) + if pgpassErr == nil { + return nil + } + + // Both failed - return combined error + return errors.Join( + fmt.Errorf("keyring: %w", keyringErr), + fmt.Errorf("pgpass: %w", pgpassErr), + ) } func (a *AutoFallbackStorage) Get(service api.Service, role string) (string, error) { // Try keyring first - if password, err := a.keyring.Get(service, role); err == nil { + password, keyringErr := a.keyring.Get(service, role) + if keyringErr == nil { return password, nil } + // Any keyring error -> try pgpass - return a.pgpass.Get(service, role) + password, pgpassErr := a.pgpass.Get(service, role) + if pgpassErr == nil { + return password, nil + } + + // Both failed + return "", errors.Join( + fmt.Errorf("keyring: %w", keyringErr), + fmt.Errorf("pgpass: %w", pgpassErr), + ) } func (a *AutoFallbackStorage) Remove(service api.Service, role string) error { @@ -370,8 +389,11 @@ func (a *AutoFallbackStorage) Remove(service api.Service, role string) error { if keyringErr == nil || pgpassErr == nil { return nil } - // If both failed, return a combined error - return fmt.Errorf("keyring: %v, pgpass: %v", keyringErr, pgpassErr) + // Both failed + return errors.Join( + fmt.Errorf("keyring: %w", keyringErr), + fmt.Errorf("pgpass: %w", pgpassErr), + ) } func (a *AutoFallbackStorage) GetStorageResult(err error, password string) PasswordStorageResult { @@ -391,18 +413,12 @@ func (a *AutoFallbackStorage) GetStorageResult(err error, password string) Passw Method: "keyring", Message: "Password saved to system keyring for automatic authentication", } - case "pgpass": + default: return PasswordStorageResult{ Success: true, Method: "pgpass", Message: "Password saved to ~/.pgpass for automatic authentication", } - default: - return PasswordStorageResult{ - Success: true, - Method: "auto", - Message: "Password saved for automatic authentication", - } } } @@ -416,13 +432,7 @@ func GetPasswordStorage() PasswordStorage { return &PgpassStorage{} case "none": return &NoStorage{} - case "auto": - return &AutoFallbackStorage{ - keyring: &KeyringStorage{}, - pgpass: &PgpassStorage{}, - } default: - // Default to auto for best compatibility across environments return &AutoFallbackStorage{ keyring: &KeyringStorage{}, pgpass: &PgpassStorage{}, diff --git a/internal/tiger/common/password_storage_test.go b/internal/tiger/common/password_storage_test.go index 70bd30d6..fe70221e 100644 --- a/internal/tiger/common/password_storage_test.go +++ b/internal/tiger/common/password_storage_test.go @@ -296,7 +296,7 @@ func TestGetPasswordStorage(t *testing.T) { {"pgpass", "pgpass", "*common.PgpassStorage"}, {"none", "none", "*common.NoStorage"}, {"auto", "auto", "*common.AutoFallbackStorage"}, - {"default", "", "*common.AutoFallbackStorage"}, // Default case now uses auto + {"default", "", "*common.AutoFallbackStorage"}, {"invalid", "invalid", "*common.AutoFallbackStorage"}, // Falls back to auto } @@ -990,134 +990,3 @@ func TestAutoFallbackStorage_Integration(t *testing.T) { t.Error("AutoFallbackStorage.Get() should fail after Remove()") } } - -// Test AutoFallbackStorage GetStorageResult with keyring success -func TestAutoFallbackStorage_GetStorageResult_KeyringSuccess(t *testing.T) { - storage := &AutoFallbackStorage{ - keyring: &KeyringStorage{}, - pgpass: &PgpassStorage{}, - lastMethod: "keyring", - } - testPassword := "test-password" - - result := storage.GetStorageResult(nil, testPassword) - if !result.Success { - t.Errorf("GetStorageResult() success should be true, got: %t", result.Success) - } - if result.Method != "keyring" { - t.Errorf("GetStorageResult() method should be 'keyring', got: %s", result.Method) - } - if !strings.Contains(result.Message, "Password saved to system keyring") { - t.Errorf("GetStorageResult() should mention keyring, got: %s", result.Message) - } - if strings.Contains(result.Message, testPassword) { - t.Error("GetStorageResult() should never include actual passwords") - } -} - -// Test AutoFallbackStorage GetStorageResult with pgpass fallback success -func TestAutoFallbackStorage_GetStorageResult_PgpassSuccess(t *testing.T) { - storage := &AutoFallbackStorage{ - keyring: &KeyringStorage{}, - pgpass: &PgpassStorage{}, - lastMethod: "pgpass", - } - testPassword := "test-password" - - result := storage.GetStorageResult(nil, testPassword) - if !result.Success { - t.Errorf("GetStorageResult() success should be true, got: %t", result.Success) - } - if result.Method != "pgpass" { - t.Errorf("GetStorageResult() method should be 'pgpass', got: %s", result.Method) - } - if !strings.Contains(result.Message, "Password saved to ~/.pgpass") { - t.Errorf("GetStorageResult() should mention pgpass, got: %s", result.Message) - } - if strings.Contains(result.Message, testPassword) { - t.Error("GetStorageResult() should never include actual passwords") - } -} - -// Test AutoFallbackStorage GetStorageResult with failure -func TestAutoFallbackStorage_GetStorageResult_Failure(t *testing.T) { - storage := &AutoFallbackStorage{ - keyring: &KeyringStorage{}, - pgpass: &PgpassStorage{}, - lastMethod: "auto", - } - testPassword := "test-password" - testError := fmt.Errorf("both storage methods failed") - - result := storage.GetStorageResult(testError, testPassword) - if result.Success { - t.Errorf("GetStorageResult() success should be false, got: %t", result.Success) - } - if result.Method != "auto" { - t.Errorf("GetStorageResult() method should be 'auto', got: %s", result.Method) - } - if !strings.Contains(result.Message, "Failed to save password") { - t.Errorf("GetStorageResult() should mention failure, got: %s", result.Message) - } - if strings.Contains(result.Message, testPassword) { - t.Error("GetStorageResult() should never include actual passwords") - } -} - -// Test AutoFallbackStorage Remove succeeds if at least one location succeeds -func TestAutoFallbackStorage_Remove_PartialSuccess(t *testing.T) { - // Set up test service name for keyring - config.SetTestServiceName(t) - - // Create a temporary directory for pgpass - tempDir := t.TempDir() - originalHome := os.Getenv("HOME") - os.Setenv("HOME", tempDir) - defer os.Setenv("HOME", originalHome) - - // Save password only to pgpass (not keyring) - pgpassStorage := &PgpassStorage{} - service := createTestService("partial-remove-test") - password := "test-password" - role := "tsdbadmin" - - err := pgpassStorage.Save(service, password, role) - if err != nil { - t.Fatalf("PgpassStorage.Save() failed: %v", err) - } - - // Now remove using AutoFallbackStorage - should succeed even if keyring fails - autoStorage := &AutoFallbackStorage{ - keyring: &KeyringStorage{}, - pgpass: &PgpassStorage{}, - } - - err = autoStorage.Remove(service, role) - if err != nil { - t.Errorf("AutoFallbackStorage.Remove() should succeed when at least one location succeeds, got: %v", err) - } -} - -// Security test: Ensure AutoFallbackStorage never includes passwords in messages -func TestAutoFallbackStorage_SecurityTest_NoPasswordInResults(t *testing.T) { - testPassword := "super-secret-password-123" - testError := fmt.Errorf("failed to save %s", testPassword) - - storage := &AutoFallbackStorage{ - keyring: &KeyringStorage{}, - pgpass: &PgpassStorage{}, - lastMethod: "auto", - } - - // Test success case - successResult := storage.GetStorageResult(nil, testPassword) - if strings.Contains(successResult.Message, testPassword) { - t.Errorf("GetStorageResult() success should never include password, got: %s", successResult.Message) - } - - // Test error case - errorResult := storage.GetStorageResult(testError, testPassword) - if strings.Contains(errorResult.Message, testPassword) { - t.Errorf("GetStorageResult() error should never include password, got: %s", errorResult.Message) - } -}