Issues/1449 - Improve Linux Keyring Support with Keyctl Fallback (#1451)

Co-authored-by: Mauricio Bonetti <[email protected]>
Co-authored-by: Juan Antonio Osorio <[email protected]>

Author: 3 people

go.mod

@@ -299,6 +299,6 @@ require (
 	go.opentelemetry.io/otel/trace v1.37.0
 	golang.org/x/crypto v0.41.0 // indirect
 	golang.org/x/exp v0.0.0-20250408133849-7e4ce0ab07d0 // indirect
-	golang.org/x/sys v0.35.0 // indirect
+	golang.org/x/sys v0.35.0
 	k8s.io/client-go v0.33.4
 )

pkg/secrets/factory.go

@@ -8,13 +8,14 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"sync"
 
 	"github.com/adrg/xdg"
-	"github.com/zalando/go-keyring"
 	"golang.org/x/term"
 
 	"github.com/stacklok/toolhive/pkg/logger"
 	"github.com/stacklok/toolhive/pkg/process"
+	"github.com/stacklok/toolhive/pkg/secrets/keyring"
 )
 
 const (
@@ -27,6 +28,18 @@ const (
 	keyringService = "toolhive"
 )
 
+var (
+	keyringProvider keyring.Provider
+	keyringOnce     sync.Once
+)
+
+func getKeyringProvider() keyring.Provider {
+	keyringOnce.Do(func() {
+		keyringProvider = keyring.NewCompositeProvider()
+	})
+	return keyringProvider
+}
+
 // ProviderType represents an enum of the types of available secrets providers.
 type ProviderType string
 
@@ -157,20 +170,10 @@ var ErrKeyringNotAvailable = errors.New("OS keyring is not available. " +
 	"Please use a different secrets provider (e.g., 1password) " +
 	"or ensure your system has a keyring service available")
 
-// IsKeyringAvailable tests if the OS keyring is available by attempting to set and delete a test value.
+// IsKeyringAvailable tests if any keyring backend is available
 func IsKeyringAvailable() bool {
-	testKey := "toolhive-keyring-test"
-	testValue := "test"
-
-	// Try to set a test value
-	if err := keyring.Set(keyringService, testKey, testValue); err != nil {
-		return false
-	}
-
-	// Clean up the test value
-	_ = keyring.Delete(keyringService, testKey)
-
-	return true
+	provider := getKeyringProvider()
+	return provider.IsAvailable()
 }
 
 // CreateSecretProvider creates the specified type of secrets provider.
@@ -214,14 +217,15 @@ func CreateSecretProviderWithPassword(managerType ProviderType, password string)
 // If optionalPassword is provided and keyring is not yet setup, it uses that password and stores it.
 // Otherwise, it uses the current functionality (read from keyring or stdin).
 func GetSecretsPassword(optionalPassword string) ([]byte, error) {
-	// Attempt to load the password from the OS keyring.
-	keyringSecret, err := keyring.Get(keyringService, keyringService)
+	provider := getKeyringProvider()
+
+	// Attempt to load the password from the keyring
+	keyringSecret, err := provider.Get(keyringService, keyringService)
 	if err == nil {
 		return []byte(keyringSecret), nil
 	}
 
-	// We need to determine if the error is due to a lack of keyring on the
-	// system or if the keyring is available but nothing was stored.
+	// Handle key not found
 	if errors.Is(err, keyring.ErrNotFound) {
 		var password []byte
 
@@ -233,9 +237,6 @@ func GetSecretsPassword(optionalPassword string) ([]byte, error) {
 			password = []byte(optionalPassword)
 		} else {
 			// Keyring is available but no password stored - this should only happen during setup
-			// We cannot ask for a password in a detached process.
-			// We should never trigger this, but this ensures that if there's a bug
-			// then it's easier to find.
 			if process.IsDetached() {
 				return nil, fmt.Errorf("detached process detected, cannot ask for password")
 			}
@@ -248,10 +249,9 @@ func GetSecretsPassword(optionalPassword string) ([]byte, error) {
 			}
 		}
 
-		// TODO GET function should not be saving anything into keyring
 		// Store the password in the keyring for future use
-		logger.Info("writing password to os keyring")
-		err = keyring.Set(keyringService, keyringService, string(password))
+		logger.Info(fmt.Sprintf("writing password to %s", provider.Name()))
+		err = provider.Set(keyringService, keyringService, string(password))
 		if err != nil {
 			return nil, fmt.Errorf("failed to store password in keyring: %w", err)
 		}
@@ -260,7 +260,7 @@ func GetSecretsPassword(optionalPassword string) ([]byte, error) {
 	}
 
 	// Assume any other keyring error means keyring is not available
-	return nil, fmt.Errorf("OS keyring is not available: %w", err)
+	return nil, fmt.Errorf("keyring is not available: %w", err)
 }
 
 func readPasswordStdin() ([]byte, error) {
@@ -281,7 +281,8 @@ func readPasswordStdin() ([]byte, error) {
 
 // ResetKeyringSecret clears out the secret from the keystore (if present).
 func ResetKeyringSecret() error {
-	return keyring.DeleteAll(keyringService)
+	provider := getKeyringProvider()
+	return provider.DeleteAll(keyringService)
 }
 
 // GenerateSecurePassword generates a cryptographically secure random password

pkg/secrets/keyring/composite.go

@@ -0,0 +1,116 @@
+// Package keyring provides a composite keyring provider that supports multiple backends.
+// It supports macOS Keychain, Windows Credential Manager, and Linux D-Bus Secret Service,
+// with keyctl as a fallback on Linux systems.
+package keyring
+
+import (
+	"fmt"
+	"runtime"
+
+	"github.com/stacklok/toolhive/pkg/logger"
+)
+
+const linuxOS = "linux"
+
+type compositeProvider struct {
+	providers []Provider
+	active    Provider
+}
+
+// NewCompositeProvider creates a new composite keyring provider that combines multiple backends.
+// It uses zalando/go-keyring as the primary provider and keyctl as a fallback on Linux.
+func NewCompositeProvider() Provider {
+	var providers []Provider
+
+	// Add zalando/go-keyring as primary provider
+	// Handles macOS, Windows, and Linux D-Bus natively
+	zkProvider := NewZalandoKeyringProvider()
+	providers = append(providers, zkProvider)
+
+	// Add keyctl provider as fallback ONLY on Linux
+	if runtime.GOOS == linuxOS {
+		if keyctl, err := NewKeyctlProvider(); err == nil {
+			providers = append(providers, keyctl)
+		}
+	}
+
+	return &compositeProvider{
+		providers: providers,
+	}
+}
+
+func (c *compositeProvider) getActiveProvider() Provider {
+	if c.active != nil && c.active.IsAvailable() {
+		return c.active
+	}
+
+	for _, provider := range c.providers {
+		if provider.IsAvailable() {
+			if c.active == nil || c.active.Name() != provider.Name() {
+				// Log provider selection if logger is available
+				// Use fmt.Printf as fallback since logger might not be initialized in tests
+				c.logProviderSelection(provider.Name())
+			}
+			c.active = provider
+			return provider
+		}
+	}
+
+	return nil
+}
+
+// logProviderSelection safely logs the provider selection
+func (*compositeProvider) logProviderSelection(providerName string) {
+	// Try to use the logger, but don't panic if it's not available
+	defer func() {
+		if r := recover(); r != nil {
+			// Logger not available, use fallback
+			fmt.Printf("Using keyring provider: %s\n", providerName)
+		}
+	}()
+
+	logger.Info(fmt.Sprintf("Using keyring provider: %s", providerName))
+}
+
+func (c *compositeProvider) Set(service, key, value string) error {
+	provider := c.getActiveProvider()
+	if provider == nil {
+		return fmt.Errorf("no keyring provider available")
+	}
+	return provider.Set(service, key, value)
+}
+
+func (c *compositeProvider) Get(service, key string) (string, error) {
+	provider := c.getActiveProvider()
+	if provider == nil {
+		return "", fmt.Errorf("no keyring provider available")
+	}
+	return provider.Get(service, key)
+}
+
+func (c *compositeProvider) Delete(service, key string) error {
+	provider := c.getActiveProvider()
+	if provider == nil {
+		return fmt.Errorf("no keyring provider available")
+	}
+	return provider.Delete(service, key)
+}
+
+func (c *compositeProvider) DeleteAll(service string) error {
+	provider := c.getActiveProvider()
+	if provider == nil {
+		return fmt.Errorf("no keyring provider available")
+	}
+	return provider.DeleteAll(service)
+}
+
+func (c *compositeProvider) IsAvailable() bool {
+	return c.getActiveProvider() != nil
+}
+
+func (c *compositeProvider) Name() string {
+	if provider := c.getActiveProvider(); provider != nil {
+		return provider.Name()
+	}
+	return "None Available"
+}