From 1a6320794e98b8a18e5a89587a24e67afb51f9a7 Mon Sep 17 00:00:00 2001 From: Johan Brandhorst-Satzkorn Date: Mon, 23 Dec 2024 09:21:18 -0800 Subject: [PATCH] cmd: propagate keyring reading errors (#5360) Reading a token from the keyring can sometimes fail for reasons other than the token not being present in the keyring. This PR introduces a new error used to indicate that the token requested could not be found in the keyring and propagates errors from the keyring up to the caller, so that the caller can act on them. Additionally, this fixes a bug in the client cache where it would incorrectly assume that a token had been removed from the keyring, when in fact there was an error reading from the keyring. --- internal/clientcache/cmd/cache/addtoken.go | 2 +- .../clientcache/cmd/search/search_test.go | 15 ++++++--- .../clientcache/internal/cache/refresh.go | 5 ++- .../clientcache/internal/cache/repository.go | 2 +- .../internal/cache/repository_test.go | 4 +-- .../internal/cache/repository_token.go | 10 +++--- .../internal/cache/repository_token_test.go | 15 +++++++++ .../clientcache/internal/daemon/server.go | 2 +- .../internal/daemon/token_handler_test.go | 4 +-- internal/cmd/base/base.go | 6 ++-- internal/cmd/base/keyring.go | 33 +++++++++++-------- internal/cmd/commands/config/token.go | 6 +++- 12 files changed, 71 insertions(+), 33 deletions(-) diff --git a/internal/clientcache/cmd/cache/addtoken.go b/internal/clientcache/cmd/cache/addtoken.go index 62d858673..5bc20060d 100644 --- a/internal/clientcache/cmd/cache/addtoken.go +++ b/internal/clientcache/cmd/cache/addtoken.go @@ -174,7 +174,7 @@ func (c *AddTokenCommand) Add(ctx context.Context, ui cli.Ui, apiClient *api.Cli // Try to read the token from the keyring in a best effort way. Ignore // any errors since the keyring may not be present on the system. - at := base.ReadTokenFromKeyring(ui, keyringType, tokenName) + at, _ := base.ReadTokenFromKeyring(ui, keyringType, tokenName) if at != nil && (token == "" || pa.AuthTokenId == at.Id) { pa.Keyring = &daemon.KeyringToken{ KeyringType: keyringType, diff --git a/internal/clientcache/cmd/search/search_test.go b/internal/clientcache/cmd/search/search_test.go index 1e5e134df..3b02507de 100644 --- a/internal/clientcache/cmd/search/search_test.go +++ b/internal/clientcache/cmd/search/search_test.go @@ -32,8 +32,8 @@ func (r *testCommander) Client(opt ...base.Option) (*api.Client, error) { return client, nil } -func (r *testCommander) ReadTokenFromKeyring(k, a string) *authtokens.AuthToken { - return r.at[a] +func (r *testCommander) ReadTokenFromKeyring(k, a string) (*authtokens.AuthToken, error) { + return r.at[a], nil } func TestSearch(t *testing.T) { @@ -50,7 +50,10 @@ func TestSearch(t *testing.T) { Token: "at_2_token", ExpirationTime: time.Now().Add(time.Minute), } - cmd := &testCommander{t: t, at: map[string]*authtokens.AuthToken{"tokenname": at, "unsupported": unsupportedAt}} + cmd := &testCommander{ + t: t, + at: map[string]*authtokens.AuthToken{"tokenname": at, "unsupported": unsupportedAt}, + } boundaryTokenReaderFn := func(ctx context.Context, addr, authToken string) (*authtokens.AuthToken, error) { switch authToken { case at.Token: @@ -67,12 +70,16 @@ func TestSearch(t *testing.T) { wg.Add(1) go func() { defer wg.Done() - srv.Serve( + err := srv.Serve( t, daemon.WithBoundaryTokenReaderFunc(ctx, boundaryTokenReaderFn), daemon.WithReadyToServeNotificationCh(context.Background(), readyNotificationCh), ) + if err != nil { + t.Error("Failed to serve daemon:", err) + } }() + t.Cleanup(wg.Wait) <-readyNotificationCh srv.AddKeyringToken(t, "address", "keyringtype", "tokenname", at.Id, boundaryTokenReaderFn) srv.AddKeyringToken(t, "address", "keyringtype", "unsupported", unsupportedAt.Id, boundaryTokenReaderFn) diff --git a/internal/clientcache/internal/cache/refresh.go b/internal/clientcache/internal/cache/refresh.go index a7193ac96..aff0febdd 100644 --- a/internal/clientcache/internal/cache/refresh.go +++ b/internal/clientcache/internal/cache/refresh.go @@ -13,6 +13,7 @@ import ( "github.com/hashicorp/boundary/api" "github.com/hashicorp/boundary/api/authtokens" + "github.com/hashicorp/boundary/internal/cmd/base" "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/event" "github.com/hashicorp/boundary/internal/util" @@ -82,8 +83,10 @@ func (r *RefreshService) cleanAndPickAuthTokens(ctx context.Context, u *user) (m return nil, errors.Wrap(ctx, err, op, errors.WithMsg("for user %v, auth token %q", u, t.Id)) } for _, kt := range keyringTokens { - at := r.repo.tokenKeyringFn(kt.KeyringType, kt.TokenName) + at, err := r.repo.tokenKeyringFn(kt.KeyringType, kt.TokenName) switch { + case err != nil && !errors.Is(err, base.ErrNoToken): + event.WriteSysEvent(ctx, op, "failed to get token from keyring", "keyring", kt.KeyringType, "token name", kt.TokenName, "error", err) case at == nil, at.Id != kt.AuthTokenId, at.UserId != t.UserId: event.WriteSysEvent(ctx, op, "Removed keyring token since the keyring contents have changed since being cached", "keyring", kt.KeyringType, "token name", kt.TokenName, "old auth token id", kt.AuthTokenId) // delete the keyring token if the auth token in the keyring diff --git a/internal/clientcache/internal/cache/repository.go b/internal/clientcache/internal/cache/repository.go index eb5883df5..3140ab68a 100644 --- a/internal/clientcache/internal/cache/repository.go +++ b/internal/clientcache/internal/cache/repository.go @@ -20,7 +20,7 @@ const ( ) // KeyringTokenLookupFn takes a token name and returns the token from the keyring -type KeyringTokenLookupFn func(keyring string, tokenName string) *authtokens.AuthToken +type KeyringTokenLookupFn func(keyring string, tokenName string) (*authtokens.AuthToken, error) // BoundaryTokenReaderFn reads an auth token's resource information from boundary type BoundaryTokenReaderFn func(ctx context.Context, addr string, authToken string) (*authtokens.AuthToken, error) diff --git a/internal/clientcache/internal/cache/repository_test.go b/internal/clientcache/internal/cache/repository_test.go index 81efc1b6d..c4cab8b32 100644 --- a/internal/clientcache/internal/cache/repository_test.go +++ b/internal/clientcache/internal/cache/repository_test.go @@ -26,8 +26,8 @@ type ringToken struct { // mapBasedAuthTokenKeyringLookup provides a fake KeyringTokenLookupFn that uses // the provided map to perform lookups for the tokens func mapBasedAuthTokenKeyringLookup(m map[ringToken]*authtokens.AuthToken) KeyringTokenLookupFn { - return func(k, t string) *authtokens.AuthToken { - return m[ringToken{k, t}] + return func(k, t string) (*authtokens.AuthToken, error) { + return m[ringToken{k, t}], nil } } diff --git a/internal/clientcache/internal/cache/repository_token.go b/internal/clientcache/internal/cache/repository_token.go index 1e7f8f7b2..77823c16f 100644 --- a/internal/clientcache/internal/cache/repository_token.go +++ b/internal/clientcache/internal/cache/repository_token.go @@ -196,7 +196,10 @@ func (r *Repository) AddKeyringToken(ctx context.Context, bAddr string, token Ke return errors.New(ctx, errors.InvalidParameter, op, "boundary address is empty", errors.WithoutEvent()) } kt := token.clone() - keyringStoredAt := r.tokenKeyringFn(kt.KeyringType, kt.TokenName) + keyringStoredAt, err := r.tokenKeyringFn(kt.KeyringType, kt.TokenName) + if err != nil { + return errors.Wrap(ctx, err, op, errors.WithMsg("unable to lookup token in keyring, keyring type: %q, token name: %q", kt.KeyringType, kt.TokenName), errors.WithoutEvent()) + } if keyringStoredAt == nil { return errors.New(ctx, errors.InvalidParameter, op, "unable to find token in the keyring specified", errors.WithoutEvent()) } @@ -224,7 +227,7 @@ func (r *Repository) AddKeyringToken(ctx context.Context, bAddr string, token Ke at = keyringStoredAt } } - _, err := r.rw.DoTx(ctx, db.StdRetryCnt, db.ExpBackoff{}, func(reader db.Reader, writer db.Writer) error { + if _, err := r.rw.DoTx(ctx, db.StdRetryCnt, db.ExpBackoff{}, func(reader db.Reader, writer db.Writer) error { if err := upsertUserAndAuthToken(ctx, reader, writer, bAddr, at); err != nil { return errors.Wrap(ctx, err, op, errors.WithoutEvent()) } @@ -236,8 +239,7 @@ func (r *Repository) AddKeyringToken(ctx context.Context, bAddr string, token Ke return errors.Wrap(ctx, err, op, errors.WithoutEvent()) } return nil - }) - if err != nil { + }); err != nil { return err } diff --git a/internal/clientcache/internal/cache/repository_token_test.go b/internal/clientcache/internal/cache/repository_token_test.go index c41845f31..20c7ff2d5 100644 --- a/internal/clientcache/internal/cache/repository_token_test.go +++ b/internal/clientcache/internal/cache/repository_token_test.go @@ -154,6 +154,21 @@ func TestRepository_AddKeyringToken(t *testing.T) { } }) } + + t.Run("When the keyring function errors", func(t *testing.T) { + keyringFn := func(keyring, tokenName string) (*authtokens.AuthToken, error) { + return nil, stderrors.New("keyring lookup function failed") + } + r, err := NewRepository(ctx, s, &sync.Map{}, keyringFn, sliceBasedAuthTokenBoundaryReader(boundaryAuthTokens)) + require.NoError(t, err) + + err = r.AddKeyringToken(ctx, "address", KeyringToken{ + KeyringType: "k", + TokenName: "t", + AuthTokenId: "at_1", + }) + assert.ErrorContains(t, err, "keyring lookup function failed") + }) } func TestRepository_AddRawToken(t *testing.T) { diff --git a/internal/clientcache/internal/daemon/server.go b/internal/clientcache/internal/daemon/server.go index 2c0037c34..3f6b900b5 100644 --- a/internal/clientcache/internal/daemon/server.go +++ b/internal/clientcache/internal/daemon/server.go @@ -48,7 +48,7 @@ const ( // and retrieve the keyring and token information used by a command. type Commander interface { ClientProvider - ReadTokenFromKeyring(keyringType, tokenName string) *authtokens.AuthToken + ReadTokenFromKeyring(keyringType, tokenName string) (*authtokens.AuthToken, error) } // ClientProvider is an interface that provides an api.Client diff --git a/internal/clientcache/internal/daemon/token_handler_test.go b/internal/clientcache/internal/daemon/token_handler_test.go index b6bcbdaeb..d9a3960cd 100644 --- a/internal/clientcache/internal/daemon/token_handler_test.go +++ b/internal/clientcache/internal/daemon/token_handler_test.go @@ -33,8 +33,8 @@ type ringToken struct { // mapBasedAuthTokenKeyringLookup provides a fake KeyringTokenLookupFn that uses // the provided map to perform lookups for the tokens func mapBasedAuthTokenKeyringLookup(m map[ringToken]*authtokens.AuthToken) cache.KeyringTokenLookupFn { - return func(k, t string) *authtokens.AuthToken { - return m[ringToken{k, t}] + return func(k, t string) (*authtokens.AuthToken, error) { + return m[ringToken{k, t}], nil } } diff --git a/internal/cmd/base/base.go b/internal/cmd/base/base.go index 2e2c0618d..2e9cbc8c1 100644 --- a/internal/cmd/base/base.go +++ b/internal/cmd/base/base.go @@ -350,8 +350,10 @@ func (c *Command) Client(opt ...Option) (*api.Client, error) { return nil, err } - authToken := c.ReadTokenFromKeyring(keyringType, tokenName) - if authToken != nil { + authToken, err := c.ReadTokenFromKeyring(keyringType, tokenName) + if err != nil { + c.UI.Error(err.Error()) + } else { c.client.SetToken(authToken.Token) } } diff --git a/internal/cmd/base/keyring.go b/internal/cmd/base/keyring.go index c745f3624..3a2a5219e 100644 --- a/internal/cmd/base/keyring.go +++ b/internal/cmd/base/keyring.go @@ -31,6 +31,9 @@ const ( PassPrefix = "HashiCorp_Boundary" ) +// ErrNoToken is returned when no matching token could be found in the keyring +var ErrNoToken = errors.New("no token found") + func (c *Command) DiscoverKeyringTokenInfo() (string, string, error) { // Stops the underlying library from invoking a dbus call that ends up // freezing some systems @@ -109,13 +112,16 @@ func (c *Command) DiscoverKeyringTokenInfo() (string, string, error) { return keyringType, tokenName, nil } -func ReadTokenFromKeyring(ui cli.Ui, keyringType, tokenName string) *authtokens.AuthToken { +// ReadTokenFromKeyring will attempt to read the token with the name tokenName from the keyring +// described by keyringType. It will return ErrNoToken if no token is found in the provided keyring. +func ReadTokenFromKeyring(ui cli.Ui, keyringType, tokenName string) (*authtokens.AuthToken, error) { + const op = "base.ReadTokenFromKeyring" var token string var err error switch keyringType { case NoneKeyring: - return nil + return nil, ErrNoToken case WincredKeyring, KeychainKeyring: token, err = zkeyring.Get(StoredTokenName, tokenName) @@ -123,10 +129,9 @@ func ReadTokenFromKeyring(ui cli.Ui, keyringType, tokenName string) *authtokens. if err == zkeyring.ErrNotFound { ui.Error("No saved credential found, continuing without") } else { - ui.Error(fmt.Sprintf("Error reading auth token from keyring: %s", err)) ui.Warn("Token must be provided via BOUNDARY_TOKEN env var or -token flag. Reading the token can also be disabled via -keyring-type=none.") + return nil, fmt.Errorf("%s: error reading auth token from keyring: %w", op, err) } - token = "" } default: @@ -138,16 +143,14 @@ func ReadTokenFromKeyring(ui cli.Ui, keyringType, tokenName string) *authtokens. kr, err := nkeyring.Open(krConfig) if err != nil { - ui.Error(fmt.Sprintf("Error opening keyring: %s", err)) ui.Warn("Token must be provided via BOUNDARY_TOKEN env var or -token flag. Reading the token can also be disabled via -keyring-type=none.") - break + return nil, fmt.Errorf("%s: failed to open keyring: %w", op, err) } item, err := kr.Get(tokenName) if err != nil { - ui.Error(fmt.Sprintf("Error fetching token from keyring: %s", err)) ui.Warn("Token must be provided via BOUNDARY_TOKEN env var or -token flag. Reading the token can also be disabled via -keyring-type=none.") - break + return nil, fmt.Errorf("%s: failed to get token from keyring: %w", op, err) } token = string(item.Data) @@ -157,22 +160,24 @@ func ReadTokenFromKeyring(ui cli.Ui, keyringType, tokenName string) *authtokens. tokenBytes, err := base64.RawStdEncoding.DecodeString(token) switch { case err != nil: - ui.Error(fmt.Errorf("Error base64-unmarshaling stored token from system credential store: %w", err).Error()) + return nil, fmt.Errorf("error base64-unmarshaling stored token from system credential store: %w", err) case len(tokenBytes) == 0: - ui.Error("Zero length token after decoding stored token from system credential store") + return nil, errors.New("zero length token after decoding stored token from system credential store") default: var authToken authtokens.AuthToken if err := json.Unmarshal(tokenBytes, &authToken); err != nil { - ui.Error(fmt.Sprintf("Error unmarshaling stored token information after reading from system credential store: %s", err)) + return nil, fmt.Errorf("error unmarshaling stored token information after reading from system credential store: %s", err) } else { - return &authToken + return &authToken, nil } } } - return nil + return nil, ErrNoToken } -func (c *Command) ReadTokenFromKeyring(keyringType, tokenName string) *authtokens.AuthToken { +// ReadTokenFromKeyring will attempt to read the token with the name tokenName from the keyring +// described by keyringType. It will return ErrNoToken if no token is found in the provided keyring. +func (c *Command) ReadTokenFromKeyring(keyringType, tokenName string) (*authtokens.AuthToken, error) { return ReadTokenFromKeyring(c.UI, keyringType, tokenName) } diff --git a/internal/cmd/commands/config/token.go b/internal/cmd/commands/config/token.go index 01a28ab0c..facab174d 100644 --- a/internal/cmd/commands/config/token.go +++ b/internal/cmd/commands/config/token.go @@ -136,7 +136,11 @@ func (c *TokenCommand) Run(args []string) (ret int) { c.UI.Error(err.Error()) return base.CommandCliError } - authToken = c.ReadTokenFromKeyring(keyringType, tokenName) + authToken, err = c.ReadTokenFromKeyring(keyringType, tokenName) + if err != nil { + c.UI.Error(err.Error()) + return base.CommandCliError + } } else { // Fallback to env/CLI but we can only get just the token value this way, at // least for now