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

@@ -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,

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)

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

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)

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
 	}
 }
 

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
 	}
 

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) {

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

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
 	}
 }
 

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)
 		}
 	}

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)
 }
 

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