Add auto option that falls back to pgpass on keychain failures (e.g. not installed)

Author: Askir

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)
 		}

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{},
+		}
 	}
 }
 

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)
+	}
+}

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