Use the intercepted token always. If the keyring matches, use it from there (#4355)

* Use the intercepted token if present, use it through the keyring if keyring contains it

* fixup! Use the intercepted token if present, use it through the keyring if keyring contains it

* Apply suggestions from code review

Co-authored-by: Jim <jlambert@hashicorp.com>

---------

Co-authored-by: Jim <jlambert@hashicorp.com>

internal/clientcache/cmd/daemon/addtoken.go

@@ -110,13 +110,12 @@ func (c *AddTokenCommand) Run(args []string) int {
 		return base.CommandCliError
 	}
 
-	keyringType, tokenName, err := c.DiscoverKeyringTokenInfo()
-	if err != nil {
-		c.PrintCliError(err)
-		return base.CommandCliError
-	}
+	// We ignore the error here since not having a keyring on the platform
+	// returns an error but can be treated as the keyring type being set to
+	// none.
+	keyringType, tokenName, _ := c.DiscoverKeyringTokenInfo()
 
-	resp, apiErr, err := c.Add(ctx, client, keyringType, tokenName)
+	resp, apiErr, err := c.Add(ctx, c.UI, client, keyringType, tokenName)
 	if err != nil {
 		c.PrintCliError(err)
 		return base.CommandCliError
@@ -136,37 +135,55 @@ func (c *AddTokenCommand) Run(args []string) int {
 	return base.CommandSuccess
 }
 
-func (c *AddTokenCommand) Add(ctx context.Context, apiClient *api.Client, keyringType, tokenName string) (*api.Response, *api.Error, error) {
+// Add builds the UpsertTokenRequest using the client's address and token,
+// and trying to leverage the keyring. It then sends the request to the daemon.
+// The passed in cli.Ui is used to print out any errors when looking up the
+// auth token from the keyring. This allows background operations calling this
+// method to pass in a silent UI to suppress any output.
+func (c *AddTokenCommand) Add(ctx context.Context, ui cli.Ui, apiClient *api.Client, keyringType, tokenName string) (*api.Response, *api.Error, error) {
 	pa := daemon.UpsertTokenRequest{
 		BoundaryAddr: apiClient.Addr(),
 	}
-	switch keyringType {
-	case "", base.NoneKeyring:
-		token := apiClient.Token()
-		if parts := strings.SplitN(token, "_", 4); len(parts) == 3 {
-			pa.AuthTokenId = strings.Join(parts[:2], "_")
-		} else {
-			return nil, nil, errors.New("The found auth token is not in the proper format.")
-		}
-		if c.FlagOutputCurlString {
-			pa.AuthToken = "/*token*/"
-		} else {
-			pa.AuthToken = token
-		}
-	default:
-		at := c.ReadTokenFromKeyring(keyringType, tokenName)
-		if at == nil {
-			return nil, nil, errors.New("No auth token could be read from the keyring to send to daemon.")
-		}
-		pa.Keyring = &daemon.KeyringToken{
-			KeyringType: keyringType,
-			TokenName:   tokenName,
-		}
-		pa.AuthTokenId = at.Id
+	token := apiClient.Token()
+	if token == "" {
+		return nil, nil, errors.New("The client auth token is empty.")
+	}
+
+	if parts := strings.SplitN(token, "_", 4); len(parts) == 3 {
+		pa.AuthTokenId = strings.Join(parts[:2], "_")
+	} else {
+		return nil, nil, errors.New("The client provided auth token is not in the proper format.")
 	}
 	var opts []client.Option
 	if c.FlagOutputCurlString {
 		opts = append(opts, client.WithOutputCurlString())
+		pa.AuthToken = "/*token*/"
+	} else {
+		pa.AuthToken = token
+	}
+
+	switch keyringType {
+	case "", base.NoneKeyring:
+		// we don't need to do anything else in this situation since the keyring
+		// doesn't play a part in the request.
+	default:
+		// Just because the keyring type is set doesn't mean the token to add
+		// is contained in it. For example, if the token was intercepted
+		// from an authentication request with '-format json' then token is
+		// not stored in the keyring, even if a keyring is provided.
+
+		// 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)
+		if at != nil && (token == "" || pa.AuthTokenId == at.Id) {
+			pa.Keyring = &daemon.KeyringToken{
+				KeyringType: keyringType,
+				TokenName:   tokenName,
+			}
+			// since the auth token is stored in the keyring, we can share it
+			// through the keyring instead of passing it through the request.
+			pa.AuthToken = ""
+		}
 	}
 
 	dotPath, err := DefaultDotDirectory(ctx)

internal/clientcache/cmd/daemon/command_wrapper.go

@@ -7,6 +7,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -93,6 +94,15 @@ func (w *CommandWrapper) startDaemon(ctx context.Context) bool {
 	return err == nil || strings.Contains(stdErr.String(), "already running")
 }
 
+// silentUi should not be used in situations where the UI is expected to be
+// prompt the user for input.
+func silentUi() *cli.BasicUi {
+	return &cli.BasicUi{
+		Writer:      io.Discard,
+		ErrorWriter: io.Discard,
+	}
+}
+
 // addTokenToCache runs AddTokenCommand with the token used in, or retrieved by
 // the wrapped command.
 func (w *CommandWrapper) addTokenToCache(ctx context.Context, token string) bool {
@@ -118,7 +128,9 @@ func (w *CommandWrapper) addTokenToCache(ctx context.Context, token string) bool
 		return false
 	}
 
-	_, apiErr, err := com.Add(ctx, client, keyringType, tokName)
+	// We do not want to print errors out from our background interactions with
+	// the daemon so use the silentUi to toss out anything that shouldn't be used
+	_, apiErr, err := com.Add(ctx, silentUi(), client, keyringType, tokName)
 	return err == nil && apiErr == nil
 }
 

internal/cmd/base/keyring.go

@@ -14,6 +14,7 @@ import (
 
 	"github.com/hashicorp/boundary/api/authtokens"
 	nkeyring "github.com/jefferai/keyring"
+	"github.com/mitchellh/cli"
 	zkeyring "github.com/zalando/go-keyring"
 )
 
@@ -108,7 +109,7 @@ func (c *Command) DiscoverKeyringTokenInfo() (string, string, error) {
 	return keyringType, tokenName, nil
 }
 
-func (c *Command) ReadTokenFromKeyring(keyringType, tokenName string) *authtokens.AuthToken {
+func ReadTokenFromKeyring(ui cli.Ui, keyringType, tokenName string) *authtokens.AuthToken {
 	var token string
 	var err error
 
@@ -120,10 +121,10 @@ func (c *Command) ReadTokenFromKeyring(keyringType, tokenName string) *authtoken
 		token, err = zkeyring.Get(StoredTokenName, tokenName)
 		if err != nil {
 			if err == zkeyring.ErrNotFound {
-				c.UI.Error("No saved credential found, continuing without")
+				ui.Error("No saved credential found, continuing without")
 			} else {
-				c.UI.Error(fmt.Sprintf("Error reading auth token from keyring: %s", err))
-				c.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.")
+				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.")
 			}
 			token = ""
 		}
@@ -137,15 +138,15 @@ func (c *Command) ReadTokenFromKeyring(keyringType, tokenName string) *authtoken
 
 		kr, err := nkeyring.Open(krConfig)
 		if err != nil {
-			c.UI.Error(fmt.Sprintf("Error opening keyring: %s", err))
-			c.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.")
+			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
 		}
 
 		item, err := kr.Get(tokenName)
 		if err != nil {
-			c.UI.Error(fmt.Sprintf("Error fetching token from keyring: %s", err))
-			c.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.")
+			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
 		}
 
@@ -156,13 +157,13 @@ func (c *Command) ReadTokenFromKeyring(keyringType, tokenName string) *authtoken
 		tokenBytes, err := base64.RawStdEncoding.DecodeString(token)
 		switch {
 		case err != nil:
-			c.UI.Error(fmt.Errorf("Error base64-unmarshaling stored token from system credential store: %w", err).Error())
+			ui.Error(fmt.Errorf("Error base64-unmarshaling stored token from system credential store: %w", err).Error())
 		case len(tokenBytes) == 0:
-			c.UI.Error("Zero length token after decoding stored token from system credential store")
+			ui.Error("Zero length token after decoding stored token from system credential store")
 		default:
 			var authToken authtokens.AuthToken
 			if err := json.Unmarshal(tokenBytes, &authToken); err != nil {
-				c.UI.Error(fmt.Sprintf("Error unmarshaling stored token information after reading from system credential store: %s", err))
+				ui.Error(fmt.Sprintf("Error unmarshaling stored token information after reading from system credential store: %s", err))
 			} else {
 				return &authToken
 			}
@@ -171,6 +172,10 @@ func (c *Command) ReadTokenFromKeyring(keyringType, tokenName string) *authtoken
 	return nil
 }
 
+func (c *Command) ReadTokenFromKeyring(keyringType, tokenName string) *authtokens.AuthToken {
+	return ReadTokenFromKeyring(c.UI, keyringType, tokenName)
+}
+
 func TokenIdFromToken(token string) (string, error) {
 	split := strings.Split(token, "_")
 	if len(split) < 3 {

internal/cmd/commands/authenticate/funcs.go

@@ -25,6 +25,9 @@ func saveAndOrPrintToken(c *base.Command, result *authmethods.AuthenticateResult
 		return base.CommandCliError
 	}
 	opts := base.GetOpts(opt...)
+	if opts.WithInterceptedToken != nil {
+		*opts.WithInterceptedToken = token.Token
+	}
 
 	switch base.Format(c.UI) {
 	case "table":
@@ -98,18 +101,10 @@ func saveAndOrPrintToken(c *base.Command, result *authmethods.AuthenticateResult
 	switch {
 	case gotErr:
 		c.UI.Warn(fmt.Sprintf("The token was not successfully saved to a system keyring. The token is:\n\n%s\n\nIt must be manually passed in via the BOUNDARY_TOKEN env var or -token flag. Storing the token can also be disabled via -keyring-type=none.", token.Token))
-
-		if opts.WithInterceptedToken != nil {
-			*opts.WithInterceptedToken = token.Token
-		}
 	case c.FlagKeyringType == "none":
 		c.UI.Warn("\nStoring the token in a keyring was disabled. The token is:")
 		c.UI.Output(token.Token)
 		c.UI.Warn("Please be sure to store it safely!")
-
-		if opts.WithInterceptedToken != nil {
-			*opts.WithInterceptedToken = token.Token
-		}
 	}
 
 	return base.CommandSuccess