From f95960fdd38f354bd8bc7bdf75c4e0ff544176f0 Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Tue, 25 Mar 2025 11:26:58 -0700 Subject: [PATCH] REC-195: recommend -store=file when keyring is not available In the login and import commands, test if we can load a token from the keyring before doing anything. If the keyring is not available, print a message that recommends -store=file. Previously, we could go through the whole login flow then fail to store the token. This was frustrating for the user since it wasn't clear what to do next (keyring errors are obscure), and once they figure it out, then have to go through the login flow again. --- cmd/engflow_auth/main.go | 10 ++++++++++ cmd/engflow_auth/main_test.go | 23 +++++++++++++++++++++++ cmd/engflow_auth/tokens.go | 23 +++++++++++++++++++++++ internal/oauthtoken/fake.go | 15 ++++++++++++++- 4 files changed, 70 insertions(+), 1 deletion(-) diff --git a/cmd/engflow_auth/main.go b/cmd/engflow_auth/main.go index 3ef2f2f..0d9cbe9 100644 --- a/cmd/engflow_auth/main.go +++ b/cmd/engflow_auth/main.go @@ -191,6 +191,11 @@ func (r *appState) import_(cliCtx *cli.Context) error { storeURLs = append(storeURLs, clusterURL) } + // Check early if the keyring works. + if err := r.testKeyringBeforeStore(storeURLs[0].Host); err != nil { + return err + } + var storeErrs []error for _, storeURL := range storeURLs { if err := r.storeToken(storeURL.Host, token.Token); err != nil { @@ -228,6 +233,11 @@ func (r *appState) login(cliCtx *cli.Context) error { } oauthURL := oauthEndpoint(clusterURL) + // Check early if the keyring works. + if err := r.testKeyringBeforeStore(clusterURL.Host); err != nil { + return err + } + // Tokens fetched during the process will be associated with each URL in // storeURLs in the token store storeURLs := []*url.URL{clusterURL} diff --git a/cmd/engflow_auth/main_test.go b/cmd/engflow_auth/main_test.go index f66d025..1d1b3da 100644 --- a/cmd/engflow_auth/main_test.go +++ b/cmd/engflow_auth/main_test.go @@ -372,6 +372,20 @@ func TestRun(t *testing.T) { wantCode: autherr.CodeAuthFailure, wantErr: "fetch_token_fail", }, + { + desc: "login token load failure", + args: []string{"login", "cluster.example.com"}, + authenticator: &fakeAuth{ + deviceResponse: &oauth2.DeviceAuthResponse{ + VerificationURIComplete: "https://cluster.example.com/with/auth/code", + }, + }, + keyringStore: &oauthtoken.FakeTokenStore{ + LoadErr: errors.New("token_load_fail"), + }, + wantCode: autherr.CodeTokenStoreFailure, + wantErr: "-store=file", // error message recommends -store=file + }, { desc: "login token store failure", args: []string{"login", "cluster.example.com"}, @@ -628,6 +642,15 @@ func TestRun(t *testing.T) { machineInput: strings.NewReader(`{"token":{"access_token":"token_data"},"cluster_host":"cluster.example.com"}`), keyringStore: oauthtoken.NewFakeTokenStore().WithPanic("do not call"), }, + { + desc: "import with keyring load failure", + args: []string{"login", "cluster.example.com"}, + keyringStore: &oauthtoken.FakeTokenStore{ + LoadErr: errors.New("token_load_fail"), + }, + wantCode: autherr.CodeTokenStoreFailure, + wantErr: "-store=file", // error message recommends -store=file + }, } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { diff --git a/cmd/engflow_auth/tokens.go b/cmd/engflow_auth/tokens.go index d20c581..a578891 100644 --- a/cmd/engflow_auth/tokens.go +++ b/cmd/engflow_auth/tokens.go @@ -20,11 +20,34 @@ import ( "io/fs" "os" + "github.com/EngFlow/auth/internal/autherr" "github.com/EngFlow/auth/internal/oauthtoken" "github.com/golang-jwt/jwt/v5" "golang.org/x/oauth2" ) +// testKeyringBeforeStore attempts to load a token for the given hostname from +// the encrypted keyring. +// +// If the file store is being used (-store=file) or if the token is missing or +// is returned successfully, this method returns nil. +// +// If there's an error loading the token, this method returns an actionable +// error, suggesting the user try -store=file. +// +// This should be called by commands that write the token store, before +// the user goes through the web login flow. +func (r *appState) testKeyringBeforeStore(hostname string) error { + if r.writeFileStore { + // Not using keyring. + return nil + } + if _, err := r.keyringStore.Load(hostname); err != nil && !errors.Is(err, fs.ErrNotExist) { + return autherr.CodedErrorf(autherr.CodeTokenStoreFailure, "failed to initialize encrypted keyring: %w\n\tNon-interactive environments typically don't support encrypted keyrings.\n\tUse -store=file to use unencrypted storage instead.", err) + } + return nil +} + // 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. // diff --git a/internal/oauthtoken/fake.go b/internal/oauthtoken/fake.go index ba6c1d1..289c1bb 100644 --- a/internal/oauthtoken/fake.go +++ b/internal/oauthtoken/fake.go @@ -15,6 +15,7 @@ package oauthtoken import ( + "errors" "fmt" "io/fs" "time" @@ -53,7 +54,7 @@ func (f *FakeTokenStore) Load(cluster string) (*oauth2.Token, error) { } token, ok := f.Tokens[cluster] if !ok { - return nil, fmt.Errorf("%s: token not found", cluster) + return nil, &tokenNotFoundError{cluster: cluster} } return token, nil } @@ -133,3 +134,15 @@ func NewFakeTokenForSubject(subject string) *oauth2.Token { Expiry: expiry, } } + +type tokenNotFoundError struct { + cluster string +} + +func (e *tokenNotFoundError) Error() string { + return fmt.Sprintf("%s: token not found", e.cluster) +} + +func (e *tokenNotFoundError) Is(err error) bool { + return errors.Is(err, fs.ErrNotExist) +}