From 5b0aa5862e52ad1e7f8fa9432c66939fb7796ef9 Mon Sep 17 00:00:00 2001 From: Bharat Kunwar Date: Thu, 29 Oct 2026 15:59:24 +0545 Subject: [PATCH 1/2] PLAT-1314/fix: preserve keychain lookup and deletion errors --- README.md | 7 +++ internal/config/config.go | 8 ++- internal/config/keyring_errors_test.go | 85 ++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 2 deletions(-) create mode 100644 internal/config/keyring_errors_test.go diff --git a/README.md b/README.md index c30e81b..716e07d 100644 --- a/README.md +++ b/README.md @@ -122,6 +122,13 @@ API key resolution order (highest precedence first): Keys are obtained from the [Two merchant portal](https://portal.two.inc). +If a keychain lookup fails, twoctl reports the underlying error rather than +claiming no key is stored. On headless Linux without a running Secret +Service/DBus session, the error explains the required backend and suggests +`TWO_API_KEY` for headless use. Explicit `--api-key` and `TWO_API_KEY` values +bypass the keychain. A missing key still prompts you to run `twoctl auth login`; +keychain failures never fall back to storing keys in a file. + ### Configuration directory Set `TWOCTL_CONFIG_HOME` to use a dedicated twoctl configuration directory: diff --git a/internal/config/config.go b/internal/config/config.go index 240a7d8..8b22ccd 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -126,7 +126,11 @@ func resolveKey(flagKey, ctxName string) (string, string, error) { return v, "$" + envVar, nil } if ctxName != "" { - if k, err := keyring.Get(keyringService, keyringAccount(ctxName)); err == nil && k != "" { + k, err := keyring.Get(keyringService, keyringAccount(ctxName)) + if err != nil && !errors.Is(err, keyring.ErrNotFound) { + return "", "", fmt.Errorf("reading keychain for context %q: %w", ctxName, wrapKeyringErr(err)) + } + if err == nil && k != "" { return k, fmt.Sprintf("keychain (context %s)", ctxName), nil } } @@ -410,7 +414,7 @@ func DeleteKey(ctxName string) error { if errors.Is(err, keyring.ErrNotFound) { return nil } - return err + return wrapKeyringErr(err) } // HasStoredKey reports whether the keychain currently holds a key for the diff --git a/internal/config/keyring_errors_test.go b/internal/config/keyring_errors_test.go new file mode 100644 index 0000000..6f1784f --- /dev/null +++ b/internal/config/keyring_errors_test.go @@ -0,0 +1,85 @@ +package config + +import ( + "errors" + "strings" + "testing" + + "github.com/zalando/go-keyring" +) + +func TestResolvePreservesKeychainFailure(t *testing.T) { + for _, message := range []string{ + "could not connect to secret-service", + "org.freedesktop.secrets returned an error", + "DBUS_SESSION_BUS_ADDRESS not set", + "User interaction is not allowed", + } { + t.Run(message, func(t *testing.T) { + withTempHome(t) + t.Setenv("TWO_API_KEY", "") + failure := errors.New(message) + keyring.MockInitWithError(failure) + t.Cleanup(keyring.MockInit) + _, err := Resolve("", "sandbox", "") + if !errors.Is(err, failure) { + t.Fatalf("lost underlying keychain failure: %v", err) + } + if !strings.Contains(err.Error(), "sandbox") { + t.Fatalf("error omitted the affected context: %v", err) + } + }) + } +} + +func TestKeychainOperationsExplainMissingBackend(t *testing.T) { + for name, operation := range map[string]func() error{ + "read": func() error { _, _, err := resolveKey("", "sandbox"); return err }, + "write": func() error { return StoreKey("sandbox", "test-key") }, + "delete": func() error { return DeleteKey("sandbox") }, + } { + t.Run(name, func(t *testing.T) { + t.Setenv("TWO_API_KEY", "") + keyring.MockInitWithError(errors.New("DBUS_SESSION_BUS_ADDRESS not set")) + t.Cleanup(keyring.MockInit) + err := operation() + if err == nil || !strings.Contains(err.Error(), "libsecret") || !strings.Contains(err.Error(), "TWO_API_KEY") { + t.Fatalf("expected backend guidance, got %v", err) + } + }) + } +} + +func TestExplicitKeysBypassUnavailableKeychain(t *testing.T) { + for _, useFlag := range []bool{false, true} { + withTempHome(t) + t.Setenv("TWO_API_KEY", "env-key") + keyring.MockInitWithError(errors.New("DBUS_SESSION_BUS_ADDRESS not set")) + t.Cleanup(keyring.MockInit) + flag, want, source := "", "env-key", "$TWO_API_KEY" + if useFlag { + flag, want, source = "flag-key", "flag-key", "--api-key flag" + } + resolved, err := Resolve(flag, "sandbox", "") + if err != nil { + t.Fatal(err) + } + if resolved.APIKey != want || resolved.Source != source { + t.Fatalf("wrong explicit key/source: %+v", resolved) + } + } +} + +func TestMissingKeyStillReportsNoAPIKey(t *testing.T) { + withTempHome(t) + t.Setenv("TWO_API_KEY", "") + keyring.MockInitWithError(keyring.ErrNotFound) + t.Cleanup(keyring.MockInit) + _, err := Resolve("", "sandbox", "") + if !errors.Is(err, ErrNoAPIKey) { + t.Fatalf("missing key should retain login guidance: %v", err) + } + if err := DeleteKey("sandbox"); err != nil { + t.Fatalf("deleting an absent key should remain a no-op: %v", err) + } +} From 57a71b9d076af2c95586e138c191b30e750ab7fb Mon Sep 17 00:00:00 2001 From: Bharat Date: Thu, 29 Oct 2026 16:47:49 +0545 Subject: [PATCH 2/2] PLAT-1314/fix: recognize headless D-Bus connection failures --- internal/config/config.go | 11 ++++++- internal/config/keyring_errors_test.go | 44 ++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/internal/config/config.go b/internal/config/config.go index 8b22ccd..df1befe 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -15,6 +15,7 @@ package config import ( "errors" "fmt" + "net" "net/url" "os" "path/filepath" @@ -400,13 +401,21 @@ func wrapKeyringErr(err error) error { msg := err.Error() if strings.Contains(msg, "secret-service") || strings.Contains(msg, "org.freedesktop.secrets") || - strings.Contains(msg, "DBUS_SESSION_BUS_ADDRESS") { + strings.Contains(msg, "DBUS_SESSION_BUS_ADDRESS") || + strings.Contains(msg, "dbus-launch") || isKeyringDialError(err) { return fmt.Errorf("no OS keychain backend available - twoctl needs libsecret + a running secret-service (gnome-keyring or KeePassXC). "+ "On headless servers, set TWO_API_KEY in the environment instead. (cause: %w)", err) } return err } +// isKeyringDialError recognizes the socket errors returned directly by godbus. +// Inspect the error chain so wrapped dial failures retain the same guidance. +func isKeyringDialError(err error) bool { + var opErr *net.OpError + return errors.As(err, &opErr) && opErr.Op == "dial" +} + // DeleteKey removes the keychain entry for the given context. A missing // entry is not an error. func DeleteKey(ctxName string) error { diff --git a/internal/config/keyring_errors_test.go b/internal/config/keyring_errors_test.go index 6f1784f..2be4240 100644 --- a/internal/config/keyring_errors_test.go +++ b/internal/config/keyring_errors_test.go @@ -2,6 +2,9 @@ package config import ( "errors" + "fmt" + "net" + "os/exec" "strings" "testing" @@ -83,3 +86,44 @@ func TestMissingKeyStillReportsNoAPIKey(t *testing.T) { t.Fatalf("deleting an absent key should remain a no-op: %v", err) } } + +func TestKeychainOperationsExplainDBusConnectionFailures(t *testing.T) { + failures := []error{ + &exec.Error{Name: "dbus-launch", Err: exec.ErrNotFound}, + &net.OpError{Op: "dial", Net: "unix", Err: errors.New("connect: no such file or directory")}, + &net.OpError{Op: "dial", Net: "tcp", Err: errors.New("connect: connection refused")}, + } + for _, failure := range failures { + t.Run(failure.Error(), func(t *testing.T) { + t.Setenv("TWO_API_KEY", "") + keyring.MockInitWithError(fmt.Errorf("backend: %w", failure)) + t.Cleanup(keyring.MockInit) + for name, operation := range map[string]func() error{ + "read": func() error { _, _, err := resolveKey("", "sandbox"); return err }, + "write": func() error { return StoreKey("sandbox", "test-key") }, + "delete": func() error { return DeleteKey("sandbox") }, + } { + t.Run(name, func(t *testing.T) { + err := operation() + if !errors.Is(err, failure) { + t.Fatalf("lost backend cause: %v", err) + } + if !strings.Contains(err.Error(), "libsecret") || !strings.Contains(err.Error(), "TWO_API_KEY") { + t.Fatalf("missing backend guidance: %v", err) + } + }) + } + }) + } +} + +func TestUnrelatedKeyringErrorsRemainUnchanged(t *testing.T) { + for _, failure := range []error{ + errors.New("User interaction is not allowed"), + &net.OpError{Op: "read", Net: "unix", Err: errors.New("permission denied")}, + } { + if got := wrapKeyringErr(failure); got != failure { + t.Fatalf("unrelated error was reclassified: %v", got) + } + } +}