Skip to content

Commit fa1bd47

Browse files
authored
REC-195: recommend -store=file when keyring is not available (#66)
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.
1 parent c89d256 commit fa1bd47

4 files changed

Lines changed: 70 additions & 1 deletion

File tree

cmd/engflow_auth/main.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,11 @@ func (r *appState) import_(cliCtx *cli.Context) error {
191191
storeURLs = append(storeURLs, clusterURL)
192192
}
193193

194+
// Check early if the keyring works.
195+
if err := r.testKeyringBeforeStore(storeURLs[0].Host); err != nil {
196+
return err
197+
}
198+
194199
var storeErrs []error
195200
for _, storeURL := range storeURLs {
196201
if err := r.storeToken(storeURL.Host, token.Token); err != nil {
@@ -228,6 +233,11 @@ func (r *appState) login(cliCtx *cli.Context) error {
228233
}
229234
oauthURL := oauthEndpoint(clusterURL)
230235

236+
// Check early if the keyring works.
237+
if err := r.testKeyringBeforeStore(clusterURL.Host); err != nil {
238+
return err
239+
}
240+
231241
// Tokens fetched during the process will be associated with each URL in
232242
// storeURLs in the token store
233243
storeURLs := []*url.URL{clusterURL}

cmd/engflow_auth/main_test.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,20 @@ func TestRun(t *testing.T) {
372372
wantCode: autherr.CodeAuthFailure,
373373
wantErr: "fetch_token_fail",
374374
},
375+
{
376+
desc: "login token load failure",
377+
args: []string{"login", "cluster.example.com"},
378+
authenticator: &fakeAuth{
379+
deviceResponse: &oauth2.DeviceAuthResponse{
380+
VerificationURIComplete: "https://cluster.example.com/with/auth/code",
381+
},
382+
},
383+
keyringStore: &oauthtoken.FakeTokenStore{
384+
LoadErr: errors.New("token_load_fail"),
385+
},
386+
wantCode: autherr.CodeTokenStoreFailure,
387+
wantErr: "-store=file", // error message recommends -store=file
388+
},
375389
{
376390
desc: "login token store failure",
377391
args: []string{"login", "cluster.example.com"},
@@ -628,6 +642,15 @@ func TestRun(t *testing.T) {
628642
machineInput: strings.NewReader(`{"token":{"access_token":"token_data"},"cluster_host":"cluster.example.com"}`),
629643
keyringStore: oauthtoken.NewFakeTokenStore().WithPanic("do not call"),
630644
},
645+
{
646+
desc: "import with keyring load failure",
647+
args: []string{"login", "cluster.example.com"},
648+
keyringStore: &oauthtoken.FakeTokenStore{
649+
LoadErr: errors.New("token_load_fail"),
650+
},
651+
wantCode: autherr.CodeTokenStoreFailure,
652+
wantErr: "-store=file", // error message recommends -store=file
653+
},
631654
}
632655
for _, tc := range testCases {
633656
t.Run(tc.desc, func(t *testing.T) {

cmd/engflow_auth/tokens.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,34 @@ import (
2020
"io/fs"
2121
"os"
2222

23+
"github.com/EngFlow/auth/internal/autherr"
2324
"github.com/EngFlow/auth/internal/oauthtoken"
2425
"github.com/golang-jwt/jwt/v5"
2526
"golang.org/x/oauth2"
2627
)
2728

29+
// testKeyringBeforeStore attempts to load a token for the given hostname from
30+
// the encrypted keyring.
31+
//
32+
// If the file store is being used (-store=file) or if the token is missing or
33+
// is returned successfully, this method returns nil.
34+
//
35+
// If there's an error loading the token, this method returns an actionable
36+
// error, suggesting the user try -store=file.
37+
//
38+
// This should be called by commands that write the token store, before
39+
// the user goes through the web login flow.
40+
func (r *appState) testKeyringBeforeStore(hostname string) error {
41+
if r.writeFileStore {
42+
// Not using keyring.
43+
return nil
44+
}
45+
if _, err := r.keyringStore.Load(hostname); err != nil && !errors.Is(err, fs.ErrNotExist) {
46+
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)
47+
}
48+
return nil
49+
}
50+
2851
// loadToken loads a token for the given cluster or returns an error equivalent
2952
// to fs.ErrNotExist if the token is not found in any store.
3053
//

internal/oauthtoken/fake.go

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package oauthtoken
1616

1717
import (
18+
"errors"
1819
"fmt"
1920
"io/fs"
2021
"time"
@@ -53,7 +54,7 @@ func (f *FakeTokenStore) Load(cluster string) (*oauth2.Token, error) {
5354
}
5455
token, ok := f.Tokens[cluster]
5556
if !ok {
56-
return nil, fmt.Errorf("%s: token not found", cluster)
57+
return nil, &tokenNotFoundError{cluster: cluster}
5758
}
5859
return token, nil
5960
}
@@ -133,3 +134,15 @@ func NewFakeTokenForSubject(subject string) *oauth2.Token {
133134
Expiry: expiry,
134135
}
135136
}
137+
138+
type tokenNotFoundError struct {
139+
cluster string
140+
}
141+
142+
func (e *tokenNotFoundError) Error() string {
143+
return fmt.Sprintf("%s: token not found", e.cluster)
144+
}
145+
146+
func (e *tokenNotFoundError) Is(err error) bool {
147+
return errors.Is(err, fs.ErrNotExist)
148+
}

0 commit comments

Comments
 (0)