Store login MFA secret with tokenhelper (#17040)

* Store login MFA secret with tokenhelper
* Clean up and refactor tokenhelper paths
* Refactor totp test code for re-use
* Add login MFA command tests
* Use longer sleep times and sha512 for totp test
* Add changelog

changelog/17040.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+login: Store token in tokenhelper for interactive login MFA
+```

command/base.go

@@ -15,7 +15,6 @@ import (
 	"github.com/hashicorp/vault/api"
 	"github.com/hashicorp/vault/command/token"
 	"github.com/hashicorp/vault/helper/namespace"
-	"github.com/hashicorp/vault/sdk/logical"
 	"github.com/mattn/go-isatty"
 	"github.com/mitchellh/cli"
 	"github.com/pkg/errors"
@@ -220,44 +219,55 @@ func (c *BaseCommand) DefaultWrappingLookupFunc(operation, path string) string {
 	return api.DefaultWrappingLookupFunc(operation, path)
 }
 
-func (c *BaseCommand) isInteractiveEnabled(mfaConstraintLen int) bool {
-	if mfaConstraintLen != 1 || !isatty.IsTerminal(os.Stdin.Fd()) {
-		return false
-	}
-
-	if !c.flagNonInteractive {
-		return true
+// getValidationRequired checks to see if the secret exists and has an MFA
+// requirement. If MFA is required and the number of constraints is greater than
+// 1, we can assert that interactive validation is not required.
+func (c *BaseCommand) getMFAValidationRequired(secret *api.Secret) bool {
+	if secret != nil && secret.Auth != nil && secret.Auth.MFARequirement != nil {
+		if c.flagMFA == nil && len(secret.Auth.MFARequirement.MFAConstraints) == 1 {
+			return true
+		} else if len(secret.Auth.MFARequirement.MFAConstraints) > 1 {
+			return true
+		}
 	}
 
 	return false
 }
 
-// getMFAMethodInfo returns MFA method information only if one MFA method is
-// configured.
-func (c *BaseCommand) getMFAMethodInfo(mfaConstraintAny map[string]*logical.MFAConstraintAny) MFAMethodInfo {
-	for _, mfaConstraint := range mfaConstraintAny {
+// getInteractiveMFAMethodInfo returns MFA method information only if operating
+// in interactive mode and one MFA method is configured.
+func (c *BaseCommand) getInteractiveMFAMethodInfo(secret *api.Secret) *MFAMethodInfo {
+	if secret == nil || secret.Auth == nil || secret.Auth.MFARequirement == nil {
+		return nil
+	}
+
+	mfaConstraints := secret.Auth.MFARequirement.MFAConstraints
+	if c.flagNonInteractive || len(mfaConstraints) != 1 || !isatty.IsTerminal(os.Stdin.Fd()) {
+		return nil
+	}
+
+	for _, mfaConstraint := range mfaConstraints {
 		if len(mfaConstraint.Any) != 1 {
-			return MFAMethodInfo{}
+			return nil
 		}
 
-		return MFAMethodInfo{
+		return &MFAMethodInfo{
 			methodType:  mfaConstraint.Any[0].Type,
 			methodID:    mfaConstraint.Any[0].ID,
 			usePasscode: mfaConstraint.Any[0].UsesPasscode,
 		}
 	}
 
-	return MFAMethodInfo{}
+	return nil
 }
 
-func (c *BaseCommand) validateMFA(reqID string, methodInfo MFAMethodInfo) int {
+func (c *BaseCommand) validateMFA(reqID string, methodInfo MFAMethodInfo) (*api.Secret, error) {
 	var passcode string
 	var err error
 	if methodInfo.usePasscode {
 		passcode, err = c.UI.AskSecret(fmt.Sprintf("Enter the passphrase for methodID %q of type %q:", methodInfo.methodID, methodInfo.methodType))
 		if err != nil {
-			c.UI.Error(fmt.Sprintf("failed to read the passphrase with error %q. please validate the login by sending a request to sys/mfa/validate", err.Error()))
-			return 2
+			return nil, fmt.Errorf("failed to read passphrase: %w. please validate the login by sending a request to sys/mfa/validate", err)
 		}
 	} else {
 		c.UI.Warn("Asking Vault to perform MFA validation with upstream service. " +
@@ -271,32 +281,10 @@ func (c *BaseCommand) validateMFA(reqID string, methodInfo MFAMethodInfo) int {
 
 	client, err := c.Client()
 	if err != nil {
-		c.UI.Error(err.Error())
-		return 2
-	}
-
-	secret, err := client.Sys().MFAValidate(reqID, mfaPayload)
-	if err != nil {
-		c.UI.Error(err.Error())
-		if secret != nil {
-			OutputSecret(c.UI, secret)
-		}
-		return 2
-	}
-	if secret == nil {
-		// Don't output anything unless using the "table" format
-		if Format(c.UI) == "table" {
-			c.UI.Info("Success! Data written to: sys/mfa/validate")
-		}
-		return 0
-	}
-
-	// Handle single field output
-	if c.flagField != "" {
-		return PrintRawField(c.UI, secret, c.flagField)
+		return nil, err
 	}
 
-	return OutputSecret(c.UI, secret)
+	return client.Sys().MFAValidate(reqID, mfaPayload)
 }
 
 type FlagSetBit uint

command/login.go

@@ -228,21 +228,27 @@ func (c *LoginCommand) Run(args []string) int {
 		return 2
 	}
 
-	if secret != nil && secret.Auth != nil && secret.Auth.MFARequirement != nil {
-		if c.isInteractiveEnabled(len(secret.Auth.MFARequirement.MFAConstraints)) {
-			// Currently, if there is only one MFA method configured, the login
-			// request is validated interactively
-			methodInfo := c.getMFAMethodInfo(secret.Auth.MFARequirement.MFAConstraints)
-			if methodInfo.methodID != "" {
-				return c.validateMFA(secret.Auth.MFARequirement.MFARequestID, methodInfo)
-			}
+	// If there is only one MFA method configured and c.NonInteractive flag is
+	// unset, the login request is validated interactively.
+	//
+	// interactiveMethodInfo here means that `validateMFA` will complete the MFA
+	// by prompting for a password or directing you to a push notification. In
+	// this scenario, no external validation is needed.
+	interactiveMethodInfo := c.getInteractiveMFAMethodInfo(secret)
+	if interactiveMethodInfo != nil {
+		c.UI.Warn("Initiating Iteractive MFA Validation...")
+		secret, err = c.validateMFA(secret.Auth.MFARequirement.MFARequestID, *interactiveMethodInfo)
+		if err != nil {
+			c.UI.Error(err.Error())
+			return 2
 		}
+	} else if c.getMFAValidationRequired(secret) {
+		// Warn about existing login token, but return here, since the secret
+		// won't have any token information if further validation is required.
+		c.checkForAndWarnAboutLoginToken()
 		c.UI.Warn(wrapAtLength("A login request was issued that is subject to "+
 			"MFA validation. Please make sure to validate the login by sending another "+
 			"request to sys/mfa/validate endpoint.") + "\n")
-
-		// We return early to prevent success message from being printed
-		c.checkForAndWarnAboutLoginToken()
 		return OutputSecret(c.UI, secret)
 	}
 

command/login_test.go

@@ -10,6 +10,7 @@ import (
 	credToken "github.com/hashicorp/vault/builtin/credential/token"
 	credUserpass "github.com/hashicorp/vault/builtin/credential/userpass"
 	"github.com/hashicorp/vault/command/token"
+	"github.com/hashicorp/vault/helper/testhelpers"
 	"github.com/hashicorp/vault/vault"
 )
 
@@ -428,6 +429,91 @@ func TestLoginCommand_Run(t *testing.T) {
 		}
 	})
 
+	t.Run("login_mfa_single_phase", func(t *testing.T) {
+		t.Parallel()
+
+		client, closer := testVaultServer(t)
+		defer closer()
+
+		ui, cmd := testLoginCommand(t)
+
+		userclient, entityID, methodID := testhelpers.SetupLoginMFATOTP(t, client)
+		cmd.client = userclient
+
+		enginePath := testhelpers.RegisterEntityInTOTPEngine(t, client, entityID, methodID)
+		totpCode := testhelpers.GetTOTPCodeFromEngine(t, client, enginePath)
+
+		// login command bails early for test clients, so we have to explicitly set this
+		cmd.client.SetMFACreds([]string{methodID + ":" + totpCode})
+		code := cmd.Run([]string{
+			"-method", "userpass",
+			"username=testuser1",
+			"password=testpassword",
+		})
+		if exp := 0; code != exp {
+			t.Errorf("expected %d to be %d", code, exp)
+		}
+
+		tokenHelper, err := cmd.TokenHelper()
+		if err != nil {
+			t.Fatal(err)
+		}
+		storedToken, err := tokenHelper.Get()
+		if err != nil {
+			t.Fatal(err)
+		}
+		output = ui.OutputWriter.String() + ui.ErrorWriter.String()
+		t.Logf("\n%+v", output)
+		if !strings.Contains(output, storedToken) {
+			t.Fatalf("expected stored token: %q, got: %q", storedToken, output)
+		}
+	})
+
+	t.Run("login_mfa_two_phase", func(t *testing.T) {
+		t.Parallel()
+
+		client, closer := testVaultServer(t)
+		defer closer()
+
+		ui, cmd := testLoginCommand(t)
+
+		userclient, entityID, methodID := testhelpers.SetupLoginMFATOTP(t, client)
+		cmd.client = userclient
+
+		_ = testhelpers.RegisterEntityInTOTPEngine(t, client, entityID, methodID)
+
+		// clear the MFA creds just to be sure
+		cmd.client.SetMFACreds([]string{})
+
+		code := cmd.Run([]string{
+			"-method", "userpass",
+			"username=testuser1",
+			"password=testpassword",
+		})
+		if exp := 0; code != exp {
+			t.Errorf("expected %d to be %d", code, exp)
+		}
+
+		expected := methodID
+		output = ui.OutputWriter.String() + ui.ErrorWriter.String()
+		t.Logf("\n%+v", output)
+		if !strings.Contains(output, expected) {
+			t.Fatalf("expected stored token: %q, got: %q", expected, output)
+		}
+
+		tokenHelper, err := cmd.TokenHelper()
+		if err != nil {
+			t.Fatal(err)
+		}
+		storedToken, err := tokenHelper.Get()
+		if storedToken != "" {
+			t.Fatal("expected empty stored token")
+		}
+		if err != nil {
+			t.Fatal(err)
+		}
+	})
+
 	t.Run("communication_failure", func(t *testing.T) {
 		t.Parallel()
 

command/write.go

@@ -158,15 +158,16 @@ func handleWriteSecretOutput(c *BaseCommand, path string, secret *api.Secret, er
 		return 0
 	}
 
-	if secret != nil && secret.Auth != nil && secret.Auth.MFARequirement != nil {
-		if c.isInteractiveEnabled(len(secret.Auth.MFARequirement.MFAConstraints)) {
-			// Currently, if there is only one MFA method configured, the login
-			// request is validated interactively
-			methodInfo := c.getMFAMethodInfo(secret.Auth.MFARequirement.MFAConstraints)
-			if methodInfo.methodID != "" {
-				return c.validateMFA(secret.Auth.MFARequirement.MFARequestID, methodInfo)
-			}
+	// Currently, if there is only one MFA method configured, the login
+	// request is validated interactively
+	methodInfo := c.getInteractiveMFAMethodInfo(secret)
+	if methodInfo != nil {
+		secret, err = c.validateMFA(secret.Auth.MFARequirement.MFARequestID, *methodInfo)
+		if err != nil {
+			c.UI.Error(err.Error())
+			return 2
 		}
+	} else if c.getMFAValidationRequired(secret) {
 		c.UI.Warn(wrapAtLength("A login request was issued that is subject to "+
 			"MFA validation. Please make sure to validate the login by sending another "+
 			"request to sys/mfa/validate endpoint.") + "\n")