From cbc360ec343b77aadd60e7f29d5da3083ff323d2 Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Tue, 26 Nov 2024 14:52:46 -0800 Subject: [PATCH 1/2] REC-110: don't use keyring when an unencrypted file token is present 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. --- cmd/engflow_auth/main_test.go | 89 ++++++++++++++++++++++++++++++----- cmd/engflow_auth/tokens.go | 50 +++++++++++--------- internal/oauthtoken/fake.go | 15 ++++++ 3 files changed, 122 insertions(+), 32 deletions(-) diff --git a/cmd/engflow_auth/main_test.go b/cmd/engflow_auth/main_test.go index a137986..f66d025 100644 --- a/cmd/engflow_auth/main_test.go +++ b/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,8 +577,8 @@ 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") }, }, { @@ -548,9 +586,12 @@ func TestRun(t *testing.T) { 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) + } + } +} diff --git a/cmd/engflow_auth/tokens.go b/cmd/engflow_auth/tokens.go index c1694f8..d20c581 100644 --- a/cmd/engflow_auth/tokens.go +++ b/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...) } diff --git a/internal/oauthtoken/fake.go b/internal/oauthtoken/fake.go index 7425ee6..f10f2b9 100644 --- a/internal/oauthtoken/fake.go +++ b/internal/oauthtoken/fake.go @@ -28,6 +28,7 @@ import ( type FakeTokenStore struct { Tokens map[string]*oauth2.Token LoadErr, StoreErr, DeleteErr error + PanicValue any } var _ LoadStorer = (*FakeTokenStore)(nil) @@ -39,6 +40,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 +54,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 +65,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 +102,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) From 693c53ed837c8ed205cd274b3dabb30bd08b0026 Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Wed, 27 Nov 2024 08:04:51 -0800 Subject: [PATCH 2/2] comment on FakeTokenStore fields --- internal/oauthtoken/fake.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/oauthtoken/fake.go b/internal/oauthtoken/fake.go index f10f2b9..ba6c1d1 100644 --- a/internal/oauthtoken/fake.go +++ b/internal/oauthtoken/fake.go @@ -26,9 +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 - PanicValue any + + // 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)