interactive CLI for mfa login #14131

hghaf099 · 2022-02-17T19:21:32Z

This PR is a step to improve CLI for mfa login. For a login request that is subject to MFA validation, it interactively asks user for credentials to validate the request, and return the client token.
Also, enabled audit backend in a test to make sure mfa/validate is audited.

Example of the CLI output in interactive mode:

vault write auth/userpass/login/alice -format=json @/tmp/alice_pwd.json

Enter the passphrase for methodID "3b7bac01-9792-d870-5c24-ff70af7ccb0d": 
{
  "request_id": "56ce9b00-d008-faec-4ff2-e46a4f5b8dc5",
  "lease_id": "",
  "lease_duration": 0,
  "renewable": false,
  "data": null,
  "warnings": null,
  "auth": {
    "client_token": "s.d0zwd86n4yuFxMIkr14EQJTB",
    "accessor": "DpMaT5UksUhUOuytgDSkVtrE",
    "policies": [
      "default"
    ],
    "token_policies": [
      "default"
    ],
    "identity_policies": null,
    "metadata": {
      "username": "alice"
    },
    "orphan": true,
    "entity_id": "f69c3147-5994-b078-9cea-94918b7f4ccd",
    "lease_duration": 2764800,
    "renewable": true,
    "mfa_requirement": null
  }
}

* Delete an MFA methodID only if it is not used by an MFA enforcement config * Fixing a bug: mfa/validate is an unauthenticated path, and goes through the handleLoginRequest path

* preventing replay attack on MFA passcodes * using %w instead of %s for error

CLI prints a warning message indicating the login request needs to get validated

ncabatoff · 2022-02-18T18:24:31Z

command/write.go

+				"mfa_payload":    mfaPayload,
+			}
+			path = "sys/mfa/validate"
+			goto WRITE


I would rather see the write code repeated than have this goto.

ncabatoff · 2022-02-18T18:24:52Z

command/write.go

@@ -150,11 +155,97 @@ func (c *WriteCommand) Run(args []string) int {
 		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 mfa/validate endpoint.") + "\n")
+		ok := YesNoPrompt("Would you like to interactively validate MFA methods?", true)
+		if ok {
+			mfaPayload := make(map[string][]string, 0)


Can you extract this into a func so we can use return instead of goto?

ncabatoff · 2022-02-22T17:24:40Z

command/base.go

+				Name:    "non-interactive",
+				Target:  &c.flagNonInteractive,
+				Default: false,
+				Usage:   "It controls a command to be executed in an interactive or non-interactive fashion with a user.",


Suggested change

Usage: "It controls a command to be executed in an interactive or non-interactive fashion with a user.",

Usage: "When set true, prevents asking the user for input via the terminal.",

ncabatoff · 2022-02-22T17:26:14Z

command/base.go

+				Default: false,
+				Usage:   "It controls a command to be executed in an interactive or non-interactive fashion with a user.",
+			})
+
 		}



Can we also use github.com/mattn/go-isatty to override the var to true when we can't ask the user for input (because there's no tty)?

Actually, ui.AskSecret already takes care of that here:
https://github.com/mitchellh/cli/blob/5454ffe87bc5c6d8b6b21c825617755e18a07828/ui.go#L79

I think this is less desirable, because then if the user didn't say -non-interactive, but they're running without a terminal, we'll simply block waiting for input on stdin. This will be a surprising change and it won't be obvious what the problem is.

ncabatoff · 2022-02-22T17:28:23Z

command/write.go

+	var passcode string
+	var err error
+	if usePasscode {
+		passcode, err = c.UI.AskSecret(fmt.Sprintf("Enter the passphrase for methodID %q:", mfaMethodID))


It would be nice if we could tell them the method type, not sure if we have that info.

ncabatoff · 2022-02-22T17:29:00Z

command/write.go

+	if usePasscode {
+		passcode, err = c.UI.AskSecret(fmt.Sprintf("Enter the passphrase for methodID %q:", mfaMethodID))
+		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 mfa/validate", err.Error()))


Suggested change

c.UI.Error(fmt.Sprintf("failed to read the passphrase with error %q. please validate the login by sending a request to mfa/validate", err.Error()))

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

ncabatoff · 2022-02-22T17:29:33Z

command/write.go

+		}
+	} else {
+		// TODO: not sure about printing this message; an error might occur before or during the validate request
+		c.UI.Warn("Please acknowledge the push notification in your authenticator app")


Shouldn't we prompt them (e.g. "hit enter") so we know when they've done it?

Well, at this point, the validate request has not issued yet. This is just a reminder for them to check their authenticator app. There is a chance that the validate request fails even before issuing the notification.

I see. Maybe we can make the message a little more explicit then, something like "Asking Vault to perform MFA validation with upstream service. You should receive a push notification in your authenticator app shortly"? Otherwise if they go and look immediately they might not see the push yet.

command/base.go

raskchanky

Looks good to me, modulo Nick's outstanding comment(s).

…nd non-interactive. A user without a terminal should not be able to use the interactive mode.

hghaf099 · 2022-02-24T19:56:43Z

I tested the following combinations:

method using passcode in interactive mode
method using passcode in non-interactive mode
method not using passcode (DUO) in interactive mode
method not using passcode (DUO) in non-interactive mode

ncabatoff · 2022-02-24T20:08:39Z

vault/external_tests/identity/login_mfa_totp_test.go

@@ -254,6 +272,24 @@ func TestLoginMfaGenerateTOTPRoleTest(t *testing.T) {
 			t.Fatalf("MFA failed: %v", err)
 		}

+		if secret.Auth == nil || secret.Auth.ClientToken == "" {


I'm a bit confused by these test changes - are they related to the new rest of the PR? It looks like we're just checking in the audit log to see if the explicit write we did to sys/mfa/validate resulted in an audit record.

no, I just did not want to push another PR for it. I have made a note in the PR description about it.

ncabatoff

Looks good!

raskchanky and others added 11 commits February 11, 2022 12:52

Login MFA

874c12d

ENT OSS segragation (#14088)

66ee056

Delete method id if not used in an MFA enforcement config (#14063)

a0a3dc0

* Delete an MFA methodID only if it is not used by an MFA enforcement config * Fixing a bug: mfa/validate is an unauthenticated path, and goes through the handleLoginRequest path

adding use_passcode field to DUO config (#14059)

db06633

Merge branch 'main' into vault-4554-oss

90466cc

add changelog

0626c75

preventing replay attack on MFA passcodes (#14056)

02c8b5d

* preventing replay attack on MFA passcodes * using %w instead of %s for error

Improve CLI command for login mfa (#14106)

d360dee

CLI prints a warning message indicating the login request needs to get validated

adding the validity period of a passcode to error messages (#14115)

03b50a5

interactive CLI for mfa login

e7e8dc1

minor fixes

a089d24

vercel bot temporarily deployed to Preview – vault February 17, 2022 19:30 Inactive

vercel bot temporarily deployed to Preview – vault-storybook February 17, 2022 19:30 Inactive

hghaf099 requested review from raskchanky and ncabatoff February 17, 2022 19:32

bail if no input was inserted

ab31046

vercel bot temporarily deployed to Preview – vault-storybook February 17, 2022 19:39 Inactive

vercel bot temporarily deployed to Preview – vault February 17, 2022 19:39 Inactive

Base automatically changed from vault-4554-oss to main February 17, 2022 21:08

Merge branch 'main' into cli-mfalogin-imp

fc27cae

vercel bot deployed to Preview – vault-storybook February 17, 2022 21:31 View deployment

vercel bot deployed to Preview – vault February 17, 2022 21:31 View deployment

change label name

b58be84

vercel bot temporarily deployed to Preview – vault February 18, 2022 01:46 Inactive

vercel bot temporarily deployed to Preview – vault-storybook February 18, 2022 01:46 Inactive

hghaf099 marked this pull request as ready for review February 18, 2022 18:19

hghaf099 marked this pull request as draft February 18, 2022 19:13

ncabatoff reviewed Feb 18, 2022

View reviewed changes

Merge branch 'main' into cli-mfalogin-imp

db05459

interactive CLI when single methodID is returned from login request

2c22714

vercel bot temporarily deployed to Preview – vault February 19, 2022 03:10 Inactive

vercel bot temporarily deployed to Preview – vault-storybook February 19, 2022 03:10 Inactive

minor fix

f33a206

vercel bot temporarily deployed to Preview – vault-storybook February 19, 2022 03:14 Inactive

vercel bot temporarily deployed to Preview – vault February 19, 2022 03:14 Inactive

hghaf099 requested a review from ncabatoff February 22, 2022 15:13

hghaf099 marked this pull request as ready for review February 22, 2022 16:56

adding changelog

d4570ba

vercel bot temporarily deployed to Preview – vault-storybook February 22, 2022 17:01 Inactive

vercel bot temporarily deployed to Preview – vault February 22, 2022 17:01 Inactive

ncabatoff reviewed Feb 22, 2022

View reviewed changes

raskchanky reviewed Feb 22, 2022

View reviewed changes

command/base.go Show resolved Hide resolved

addressing feedback

eeb0da8

vercel bot temporarily deployed to Preview – vault February 22, 2022 18:45 Inactive

vercel bot temporarily deployed to Preview – vault-storybook February 22, 2022 18:45 Inactive

hghaf099 requested a review from ncabatoff February 22, 2022 21:45

hghaf099 added this to the 1.10-rc1 milestone Feb 23, 2022

raskchanky approved these changes Feb 24, 2022

View reviewed changes

a user with a terminal should be able to choose between interactive a…

dc9717c

…nd non-interactive. A user without a terminal should not be able to use the interactive mode.

vercel bot temporarily deployed to Preview – vault February 24, 2022 19:53 Inactive

vercel bot temporarily deployed to Preview – vault-storybook February 24, 2022 19:53 Inactive

ncabatoff reviewed Feb 24, 2022

View reviewed changes

ncabatoff approved these changes Feb 24, 2022

View reviewed changes

hghaf099 merged commit 53519c0 into main Feb 24, 2022

hghaf099 deleted the cli-mfalogin-imp branch February 24, 2022 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interactive CLI for mfa login #14131

interactive CLI for mfa login #14131

hghaf099 commented Feb 17, 2022 •

edited

Loading

ncabatoff Feb 18, 2022

ncabatoff Feb 18, 2022

ncabatoff Feb 22, 2022

ncabatoff Feb 22, 2022

hghaf099 Feb 22, 2022

ncabatoff Feb 24, 2022

ncabatoff Feb 22, 2022

ncabatoff Feb 22, 2022

ncabatoff Feb 22, 2022

hghaf099 Feb 22, 2022

ncabatoff Feb 24, 2022

raskchanky left a comment

hghaf099 commented Feb 24, 2022

ncabatoff Feb 24, 2022

hghaf099 Feb 24, 2022 •

edited

Loading

ncabatoff left a comment

	Usage: "It controls a command to be executed in an interactive or non-interactive fashion with a user.",
	Usage: "When set true, prevents asking the user for input via the terminal.",

	c.UI.Error(fmt.Sprintf("failed to read the passphrase with error %q. please validate the login by sending a request to mfa/validate", err.Error()))
	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()))

interactive CLI for mfa login #14131

interactive CLI for mfa login #14131

Conversation

hghaf099 commented Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raskchanky left a comment

Choose a reason for hiding this comment

hghaf099 commented Feb 24, 2022

Choose a reason for hiding this comment

hghaf099 Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

ncabatoff left a comment

Choose a reason for hiding this comment

hghaf099 commented Feb 17, 2022 •

edited

Loading

hghaf099 Feb 24, 2022 •

edited

Loading