REC-110: don't use keyring when an unencrypted file token is present (#60)

On Linux, the keyring library can prompt the user for a password if it's
not automatically unlocked. This causes engflow_auth to hang when
invoked as a credential helper because neither stdin nor stdout are
connected to a terminal.

With this change, the get command (and anything else that calls
loadToken) now checks for a token created with -store=file first, then
falls back to the keyring library if that's not present. This reverses
the previous order.

When storing a token into the keyring, storeToken now deletes the token
created with -store=file, if present, preventing it from taking
precedence.

Together, these changes that mean when -store=file is used, the keyring
library should only be used by the logout command, which attempts to
delete both tokens.

Author: jayconrod

cmd/engflow_auth/main_test.go

@@ -126,7 +126,7 @@ func TestRun(t *testing.T) {
 		wantUsageErr         string
 		wantStdoutContaining []string
 		wantStderrContaining []string
-		wantStored           []string
+		checkState           func(t *testing.T, root *appState)
 	}{
 		{
 			desc:     "no subcommand",
@@ -226,6 +226,14 @@ func TestRun(t *testing.T) {
 			fileStore:            oauthtoken.NewFakeTokenStore().WithLoadErr(errors.New("fake error")),
 			wantStdoutContaining: []string{`"x-engflow-auth-token"`},
 		},
+		{
+			desc:                 "get from file does not call keyring",
+			args:                 []string{"get"},
+			machineInput:         strings.NewReader(`{"uri": "https://cluster.example.com"}`),
+			keyringStore:         oauthtoken.NewFakeTokenStore().WithPanic("do not call"),
+			fileStore:            oauthtoken.NewFakeTokenStore().WithTokenForSubject("cluster.example.com", "alice"),
+			wantStdoutContaining: []string{`"x-engflow-auth-token"`},
+		},
 		{
 			desc: "version prints build metadata",
 			args: []string{"version"},
@@ -265,9 +273,12 @@ func TestRun(t *testing.T) {
 					VerificationURIComplete: "https://cluster.example.com/with/auth/code",
 				},
 			},
-			wantStored: []string{
-				"cluster.example.com",
-				"cluster.local.example.com",
+			checkState: func(t *testing.T, root *appState) {
+				checkTokenStoreContains(
+					t,
+					root.keyringStore,
+					"cluster.example.com",
+					"cluster.local.example.com")
 			},
 		},
 		{
@@ -418,6 +429,33 @@ func TestRun(t *testing.T) {
 			},
 			wantStderrContaining: []string{"Login identity has changed"},
 		},
+		{
+			desc: "login with keyring deletes file",
+			args: []string{"login", "cluster.example.com"},
+			authenticator: &fakeAuth{
+				deviceResponse: &oauth2.DeviceAuthResponse{
+					VerificationURIComplete: "https://cluster.example.com/with/auth/code",
+				},
+				token: oauthtoken.NewFakeTokenForSubject("alice"),
+			},
+			fileStore: oauthtoken.NewFakeTokenStore().WithTokenForSubject("cluster.example.com", "alice"),
+			checkState: func(t *testing.T, root *appState) {
+				if _, err := root.fileStore.Load("cluster.example.com"); err == nil {
+					t.Error("token was not deleted from file store")
+				}
+			},
+		},
+		{
+			desc: "login with file should not call keyring",
+			args: []string{"login", "-store=file", "cluster.example.com"},
+			authenticator: &fakeAuth{
+				deviceResponse: &oauth2.DeviceAuthResponse{
+					VerificationURIComplete: "https://cluster.example.com/with/auth/code",
+				},
+				token: oauthtoken.NewFakeTokenForSubject("alice"),
+			},
+			keyringStore: oauthtoken.NewFakeTokenStore().WithPanic("do not call"),
+		},
 		{
 			desc:     "logout without cluster",
 			args:     []string{"logout"},
@@ -539,18 +577,21 @@ func TestRun(t *testing.T) {
 			args:         []string{"import"},
 			machineInput: strings.NewReader(`{"token":{"access_token":"token_data"},"cluster_host":"cluster.example.com"}`),
 			keyringStore: oauthtoken.NewFakeTokenStore(),
-			wantStored: []string{
-				"cluster.example.com",
+			checkState: func(t *testing.T, root *appState) {
+				checkTokenStoreContains(t, root.keyringStore, "cluster.example.com")
 			},
 		},
 		{
 			desc:         "import with alias",
 			args:         []string{"import"},
 			machineInput: strings.NewReader(`{"token":{"access_token":"token_data"},"cluster_host":"cluster.example.com","aliases":["cluster.local.example.com"]}`),
 			keyringStore: oauthtoken.NewFakeTokenStore(),
-			wantStored: []string{
-				"cluster.example.com",
-				"cluster.local.example.com",
+			checkState: func(t *testing.T, root *appState) {
+				checkTokenStoreContains(
+					t,
+					root.keyringStore,
+					"cluster.example.com",
+					"cluster.local.example.com")
 			},
 		},
 		{
@@ -570,6 +611,23 @@ func TestRun(t *testing.T) {
 			wantCode:     autherr.CodeBadParams,
 			wantErr:      "token data contains invalid cluster",
 		},
+		{
+			desc:         "import with keyring deletes file",
+			args:         []string{"import"},
+			machineInput: strings.NewReader(`{"token":{"access_token":"token_data"},"cluster_host":"cluster.example.com"}`),
+			fileStore:    oauthtoken.NewFakeTokenStore().WithTokenForSubject("cluster.example.com", "alice"),
+			checkState: func(t *testing.T, root *appState) {
+				if _, err := root.fileStore.Load("cluster.example.com"); err == nil {
+					t.Error("token was not deleted from file store")
+				}
+			},
+		},
+		{
+			desc:         "import with file should not call keyring",
+			args:         []string{"import", "-store=file"},
+			machineInput: strings.NewReader(`{"token":{"access_token":"token_data"},"cluster_host":"cluster.example.com"}`),
+			keyringStore: oauthtoken.NewFakeTokenStore().WithPanic("do not call"),
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
@@ -633,9 +691,18 @@ func TestRun(t *testing.T) {
 					t.Logf("\n====== BEGIN APP STDERR ======\n%s\n====== END APP STDERR ======\n\n", stderr.String())
 				}
 			}
-			if tokenStore, ok := tc.keyringStore.(*oauthtoken.FakeTokenStore); ok {
-				assert.Subset(t, tokenStore.Tokens, tc.wantStored)
+			if tc.checkState != nil {
+				tc.checkState(t, root)
 			}
 		})
 	}
 }
+
+func checkTokenStoreContains(t *testing.T, ts oauthtoken.LoadStorer, clusters ...string) {
+	for _, cluster := range clusters {
+		_, err := ts.Load(cluster)
+		if err != nil {
+			t.Error(err)
+		}
+	}
+}

cmd/engflow_auth/tokens.go

@@ -18,6 +18,7 @@ import (
 	"errors"
 	"fmt"
 	"io/fs"
+	"os"
 
 	"github.com/EngFlow/auth/internal/oauthtoken"
 	"github.com/golang-jwt/jwt/v5"
@@ -27,23 +28,39 @@ import (
 // loadToken loads a token for the given cluster or returns an error equivalent
 // to fs.ErrNotExist if the token is not found in any store.
 //
+// NOTE(REC-110): loadToken attempts to load from the file store first, falling
+// back to the keyring if an unencrypted token is not found. On Linux, the
+// keyring library may try to prompt the user for a password, hanging forever
+// because stdin and stdout are not normally connected to a terminal. We should
+// avoid calling it at all if the token is not stored there.
+//
 // loadToken may contain logic specific to this app and should be called
 // by commands instead of calling LoadStorer.Load directly.
 func (r *appState) loadToken(cluster string) (*oauth2.Token, error) {
-	var errs []error
-	backends := []oauthtoken.LoadStorer{r.keyringStore, r.fileStore}
-	for _, backend := range backends {
-		token, err := backend.Load(cluster)
-		if err == nil {
+	token, fileErr := r.fileStore.Load(cluster)
+	if fileErr == nil {
+		return token, nil
+	}
+	var keyringErr error
+	if !r.writeFileStore {
+		token, keyringErr = r.keyringStore.Load(cluster)
+		if keyringErr == nil {
 			return token, nil
 		}
-		errs = append(errs, err)
 	}
-	return nil, fmt.Errorf("failed to load token from %d backend(s): %w", len(backends), errors.Join(errs...))
+	return nil, fmt.Errorf("failed to load token: %w", errors.Join(fileErr, keyringErr))
 }
 
 // storeToken stores a token for the given cluster in one of the backends.
 //
+// NOTE(REC-110): when -store=file is used, storeToken only writes to the file
+// store and ignores the keyring store. When -store=file is not used,
+// storedToken writes to the keyring store deletes then token from the file
+// store if present. On Linux, the keyring library may try to prompt the user
+// for a password, causing loadToken to hang forever when invoked later.
+// So if -store=file is used, we should avoid calling the keyring library,
+// now or in the future.
+//
 // storeToken may contain logic specific to this app and should be called
 // by commands instead of calling LoadStorer.Store directly. For example,
 // storeToken prints a message if the token's subject has changed.
@@ -56,6 +73,9 @@ func (r *appState) storeToken(cluster string, token *oauth2.Token) error {
 	if r.writeFileStore {
 		return r.fileStore.Store(cluster, token)
 	} else {
+		if err := r.fileStore.Delete(cluster); err != nil && !errors.Is(err, fs.ErrNotExist) {
+			fmt.Fprintf(os.Stderr, "warning: attempting to delete existing file token: %v", err)
+		}
 		return r.keyringStore.Store(cluster, token)
 	}
 }
@@ -105,19 +125,7 @@ func (r *appState) deleteToken(cluster string) error {
 		}
 	}
 	if err := errors.Join(nonNotFoundErrs...); err != nil {
-		return fmt.Errorf("failed to delete token from %d backend(s): %w", len(backends), err)
+		return fmt.Errorf("failed to delete token: %w", err)
 	}
-	return &multiBackendNotFoundError{backendsCount: len(backends)}
-}
-
-type multiBackendNotFoundError struct {
-	backendsCount int
-}
-
-func (m *multiBackendNotFoundError) Error() string {
-	return fmt.Sprintf("token for cluster not found after trying %d token storage backends", m.backendsCount)
-}
-
-func (m *multiBackendNotFoundError) Is(err error) bool {
-	return err == fs.ErrNotExist
+	return errors.Join(errs...)
 }

internal/oauthtoken/fake.go

@@ -26,8 +26,14 @@ import (
 // FakeTokenStore is a test implementation of LoadStorer that stores tokens in
 // memory instead of the system keychain.
 type FakeTokenStore struct {
-	Tokens                       map[string]*oauth2.Token
+	Tokens map[string]*oauth2.Token
+
+	// Error values to be returned by Load, Store, and Delete, if not nil.
 	LoadErr, StoreErr, DeleteErr error
+
+	// Value to panic with in Load, Store, and Delete, if not nil.
+	// Used to test that a method is NOT called.
+	PanicValue any
 }
 
 var _ LoadStorer = (*FakeTokenStore)(nil)
@@ -39,6 +45,9 @@ func NewFakeTokenStore() *FakeTokenStore {
 }
 
 func (f *FakeTokenStore) Load(cluster string) (*oauth2.Token, error) {
+	if f.PanicValue != nil {
+		panic(f.PanicValue)
+	}
 	if f.LoadErr != nil {
 		return nil, f.LoadErr
 	}
@@ -50,6 +59,9 @@ func (f *FakeTokenStore) Load(cluster string) (*oauth2.Token, error) {
 }
 
 func (f *FakeTokenStore) Store(cluster string, token *oauth2.Token) error {
+	if f.PanicValue != nil {
+		panic(f.PanicValue)
+	}
 	if f.StoreErr != nil {
 		return f.StoreErr
 	}
@@ -58,6 +70,9 @@ func (f *FakeTokenStore) Store(cluster string, token *oauth2.Token) error {
 }
 
 func (f *FakeTokenStore) Delete(cluster string) error {
+	if f.PanicValue != nil {
+		panic(f.PanicValue)
+	}
 	if f.DeleteErr != nil {
 		return f.DeleteErr
 	}
@@ -92,6 +107,11 @@ func (f *FakeTokenStore) WithDeleteErr(err error) *FakeTokenStore {
 	return f
 }
 
+func (f *FakeTokenStore) WithPanic(value any) *FakeTokenStore {
+	f.PanicValue = value
+	return f
+}
+
 func NewFakeTokenForSubject(subject string) *oauth2.Token {
 	now := time.Now()
 	expiry := now.Add(time.Hour)