Skip to content

Commit

Permalink
REC-110: don't use keyring when an unencrypted file token is present (#…
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@jayconrod
jayconrod authored Nov 27, 2024
1 parent ccd6177 commit 8ed260b
Show file tree
Hide file tree
Showing 3 changed files with 128 additions and 33 deletions.
89 changes: 78 additions & 11 deletions cmd/engflow_auth/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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")
},
},
{
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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")
},
},
{
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
}
}
}
50 changes: 29 additions & 21 deletions cmd/engflow_auth/tokens.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"errors"
"fmt"
"io/fs"
"os"

"github.com/EngFlow/auth/internal/oauthtoken"
"github.com/golang-jwt/jwt/v5"
Expand All @@ -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.
Expand All @@ -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)
}
}
Expand Down Expand Up @@ -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...)
}
22 changes: 21 additions & 1 deletion internal/oauthtoken/fake.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 8ed260b

Please sign in to comment.