Avoid leaking passwords into shell history and audits events by supporting reading passwords from stdin #881

patdowney · 2021-10-10T08:01:59Z

Proposed changes

closes #882

Adds a --passwordStdin flag to the appropriate mongocli commands in order to avoid the specified password showing up in terminal history or system audit records by allowing it to be passed in via stdin.

This will allow things like

$ vault read -field=value my/path/to/secret | mongocli atlas dbuser create --username ${USER} --role clusterMonitor,backup --projectId ${PROJECT} --passwordStdin

Checklist

I have signed the MongoDB CLA
I have added tests that prove my fix is effective or that my feature works
I have added any necessary documentation (if appropriate)
I have updated e2e/E2E-TESTS.md (if a new command or e2e test has been added)
I have run make fmt and formatted my code

gssbzn · 2021-10-11T11:15:07Z

Hi @patdowney first thanks for the contribution but given the scope of it would you mind opening an issue first to discuss the feature?

There are a couple of things it would be nice to agree first before focusing on the code details.

Main one I want to call out, many of the commands you are changing would read input from stdin when the --password is omitted but I guess you're being affected by AlecAivazis/survey#328, I think it would be better to fix the issue at the root but we could discuss alternatives in an issue in this repo if you prefer

patdowney · 2021-10-11T16:19:10Z

@gssbzn Absolutely. I've raised #882 Re: AlecAivazis/survey#328 - fixing that issue looks like it would help with my use case, although their FAQ does say "This means that reading from piped stdin or writing to piped stdout is not supported, and likely to break your application in these situations" so I'm not sure when that issue will be addressed!

This patch should not change any existing behaviour, so not passing in --password will still end up with the user being prompted in a user friendly way.

gssbzn · 2021-10-18T16:13:27Z

thanks for the issue, let's discuss these changes there

gssbzn · 2021-10-28T08:21:21Z

Hi @patdowney I se you've done some changes based on my comments from #882, is this PR ready for review?

patdowney · 2021-10-28T10:42:10Z

Yes please!

gssbzn · 2021-11-04T11:02:49Z

internal/prompt/prompt_test.go

@@ -0,0 +1,15 @@
+package prompt


let's please add

Suggested change

package prompt

//go:build unit

// +build unit

package prompt

gssbzn

sorry for the delay on this

one initial things that's causing our internal CI to fail, the new files are missing the copyright header, you can fix this by running make addcopy

gssbzn

another failure from our CI

 [2021/11/03 10:51:51.817] FAILURE: Failed ()
 [2021/11/03 10:51:51.817] === RUN   TestDBUsers/Create
 [2021/11/03 10:51:51.817]     dbusers_test.go:65:
 [2021/11/03 10:51:51.817]         	Error Trace:	dbusers_test.go:202
 [2021/11/03 10:51:51.817]         	            				dbusers_test.go:65
 [2021/11/03 10:51:51.817]         	Error:      	Not equal:
 [2021/11/03 10:51:51.817]         	            	expected: "passW0rd"
 [2021/11/03 10:51:51.817]         	            	actual  : ""
 [2021/11/03 10:51:51.817] 
 [2021/11/03 10:51:51.817]         	            	Diff:
 [2021/11/03 10:51:51.817]         	            	--- Expected
 [2021/11/03 10:51:51.817]         	            	+++ Actual
 [2021/11/03 10:51:51.817]         	            	@@ -1 +1 @@
 [2021/11/03 10:51:51.817]         	            	-passW0rd
 [2021/11/03 10:51:51.817]         	            	+
 [2021/11/03 10:51:51.817]         	Test:       	TestDBUsers/Create
 [2021/11/03 10:51:51.817]     --- FAIL: TestDBUsers/Create (0.10s)

 [2021/11/03 10:51:51.884] FAILURE: Failed ()
 [2021/11/03 10:51:51.884] === RUN   TestDBUsers/CreatePasswordStdin
 [2021/11/03 10:51:51.884]     dbusers_test.go:83:
 [2021/11/03 10:51:51.884]         	Error Trace:	dbusers_test.go:194
 [2021/11/03 10:51:51.884]         	            				dbusers_test.go:83
 [2021/11/03 10:51:51.884]         	Error:      	Received unexpected error:
 [2021/11/03 10:51:51.884]         	            	exit status 1
 [2021/11/03 10:51:51.884]         	Test:       	TestDBUsers/CreatePasswordStdin
 [2021/11/03 10:51:51.884]     dbusers_test.go:83: unexpected error: invalid character 'E' looking for beginning of value
 [2021/11/03 10:51:51.884]     --- FAIL: TestDBUsers/CreatePasswordStdin (0.04s)

internal/prompt/prompt.go

gssbzn · 2021-11-04T12:04:06Z

Also before we can accept your contribution we need you to sign our CLA

patdowney · 2021-11-04T14:20:26Z

Thanks @gssbzn . I'll address your comments, hopefully this weekend. I thought I'd already signed the CLA?

gssbzn · 2021-11-04T14:27:05Z

I thought I'd already signed the CLA?

I could not find your github username on our database, if you can double check if you did that'd be great

patdowney · 2021-11-04T14:31:25Z

Think I've figured it out, I signed the form but didn't click the further confirmation link that's sent afterwards. Will take care of that this weekend as well

patdowney · 2021-11-04T14:34:31Z

Actually, have just found the confirmed email. I signed it on 2021-10-02, GitHub username on the form is dioadconsulting maybe I got that bit wrong

gssbzn · 2021-11-04T14:44:34Z

GitHub username on the form is dioadconsulting maybe I got that bit wrong

Thanks that was it, I can see dioadconsulting, would you mind signing again with the correct username?

This is to avoid leaking passwords into shell history and auditd events

gssbzn · 2021-11-08T14:37:07Z

Thanks again for all the changes, code looks good I have some recommendations on the tests and I'm waiting for our CI ro run to make sure there's no other thing to look at

e2e/atlas/dbusers_test.go

e2e/atlas/ldap_test.go

Remove assertion on BindPassword property from response. It doesn't exist.

colm-quinn · 2021-11-15T11:12:47Z

This is failing the linting check currently:

  Error: paramTypeCombine: func(t *testing.T, cliPath string, projectID string) could be replaced with func(t *testing.T, cliPath, projectID string) (gocritic)

I'd recommend running those checks locally with make lint and/or make fix-lint to catch the issues as you are committing changes.

colm-quinn · 2021-11-15T11:20:01Z

internal/cli/atlas/dbusers/create.go

@@ -131,7 +139,7 @@ func (opts *CreateOpts) validate() error {
 		return errors.New("missing role for the user")
 	}

-	if opts.isExternal() && opts.password != "" {
+	if opts.isExternal() && (opts.password != "") {


[q/np] Do we need the extra brackets here?

thanks @colm-quinn. Addressed in latest.

andreaangiolillo

LGTM. You just need to fix the lint issue.

e2e/atlas/ldap_test.go

andreaangiolillo · 2021-11-15T11:24:33Z

internal/cli/atlas/security/ldap/save.go

+func (opts *SaveOpts) Prompt() error {
+	if opts.bindPassword != "" {
+		return nil
+	}
+
+	if !opts.IsTerminalInput() {
+		_, err := fmt.Fscanln(opts.InReader, &opts.bindPassword)
+		return err
+	}
+
+	prompt := &survey.Password{
+		Message: "Password:",
+	}
+
+	return survey.AskOne(prompt, &opts.bindPassword)
+}
+


[nit] I think we could add this function under InputOpts and pass the password as an input parameter so that we can reuse this piece of code. Then you can call this function in any commands that require this prompt message

@andreaangiolillo - is this a change you'd like me to introduce as part of this PR?

It is up to you. We can add this improvement in another PR if you want ; )

gssbzn · 2021-11-15T15:43:25Z

e2e/atlas/ldap_test.go

-		if err := json.Unmarshal(resp, &configuration); a.NoError(err) {
-			a.Equal(ldapHostname, configuration.LDAP.Hostname)
-		}
+		testLDAPSaveCmd(t, cmd)


[2021/11/15 15:41:25.385] FAILURE: Failed () [2021/11/15 15:41:25.385] === RUN TestLDAPWithFlags/Save [2021/11/15 15:41:25.385] ldap_test.go:132: [2021/11/15 15:41:25.385] Error Trace: ldap_test.go:270 [2021/11/15 15:41:25.385] ldap_test.go:132 [2021/11/15 15:41:25.385] Error: Not equal: [2021/11/15 15:41:25.385] expected: "admin" [2021/11/15 15:41:25.385] actual : "" [2021/11/15 15:41:25.385] [2021/11/15 15:41:25.385] Diff: [2021/11/15 15:41:25.385] --- Expected [2021/11/15 15:41:25.385] +++ Actual [2021/11/15 15:41:25.385] @@ -1 +1 @@ [2021/11/15 15:41:25.385] -admin [2021/11/15 15:41:25.385] + [2021/11/15 15:41:25.385] Test: TestLDAPWithFlags/Save [2021/11/15 15:41:25.385] --- FAIL: TestLDAPWithFlags/Save (0.09s)

gssbzn · 2021-11-15T15:44:08Z

e2e/atlas/ldap_test.go

+		passwordStdin := bytes.NewBuffer([]byte(ldapBindPassword))
+		cmd.Stdin = passwordStdin
+
+		testLDAPSaveCmd(t, cmd)


[2021/11/15 15:41:25.641] FAILURE: Failed () [2021/11/15 15:41:25.641] === RUN TestLDAPWithStdin/Save [2021/11/15 15:41:25.641] ldap_test.go:218: [2021/11/15 15:41:25.641] Error Trace: ldap_test.go:270 [2021/11/15 15:41:25.641] ldap_test.go:218 [2021/11/15 15:41:25.641] Error: Not equal: [2021/11/15 15:41:25.641] expected: "admin" [2021/11/15 15:41:25.641] actual : "" [2021/11/15 15:41:25.641] [2021/11/15 15:41:25.641] Diff: [2021/11/15 15:41:25.641] --- Expected [2021/11/15 15:41:25.641] +++ Actual [2021/11/15 15:41:25.641] @@ -1 +1 @@ [2021/11/15 15:41:25.641] -admin [2021/11/15 15:41:25.641] + [2021/11/15 15:41:25.641] Test: TestLDAPWithStdin/Save [2021/11/15 15:41:25.641] --- FAIL: TestLDAPWithStdin/Save (0.10s)

gssbzn · 2021-11-15T15:45:21Z

e2e/cloud_manager/dbusers_test.go

+		passwordStdin := bytes.NewBuffer([]byte("passW0rd"))
+		cmd.Stdin = passwordStdin
+
+		testCreatePasswordCmd(t, cmd)


[2021/11/15 15:28:14.823] FAILURE: Failed () [2021/11/15 15:28:14.823] === RUN TestDBUsers/CreatePasswordStdin [2021/11/15 15:28:14.823] dbusers_test.go:92: unexpected error: exit status 1, resp: Error: PUT https://cloud-dev.mongodb.com/api/public/v1.0/groups/61927b10ab680d0601bf033a/automationConfig: 400 (request "") Invalid config: Each auth.usersWanted must have a unique user id. Duplicate found: user-126@admin [2021/11/15 15:28:14.823] --- FAIL: TestDBUsers/CreatePasswordStdin (0.15s)

ah, same stuff as the atlast tests, will take a look at pulling these out into TestDBUsersWithFlags and TestDBUsersWithStdin too.

gssbzn

Some of the E2E tests are still failing, LDAP and cloud manager/ops manager dbusers

…populated

… and TestDBUsersWith Stdin

gssbzn

🎉 thanks so much @patdowney
Tests are looking good and I can confirm the CLA has been signed, we'll be merging this shortly

colm-quinn

LGTM, thanks for those changes and working through all the comments - this is a great contribution!

gssbzn · 2021-11-17T10:43:15Z

Internal ticket: CLOUDP-106015

patdowney · 2021-11-17T23:07:56Z

Great 👍 Thanks for your patience!

patdowney requested a review from a team as a code owner October 10, 2021 08:01

patdowney requested review from a team and fmenezes October 10, 2021 08:01

gssbzn removed the request for review from fmenezes October 11, 2021 11:15

patdowney mentioned this pull request Oct 11, 2021

Avoid leaking passwords into shell history and audits events by supporting reading passwords from stdin #882

Closed

gssbzn reviewed Nov 4, 2021

View reviewed changes

gssbzn requested changes Nov 4, 2021

View reviewed changes

gssbzn reviewed Nov 4, 2021

View reviewed changes

internal/prompt/prompt.go Outdated Show resolved Hide resolved

patdowney added 2 commits November 5, 2021 22:36

Introduce InputOpts to add support for reading passwords from stdin

8ff9bcf

This is to avoid leaking passwords into shell history and auditd events

Remove redundant modifications to prompt

e043ddb

gssbzn reviewed Nov 8, 2021

View reviewed changes

patdowney added 2 commits November 8, 2021 21:49

Fix linting errors, add use require helper

0a17f1c

Separate DBUser tests into Flag and Stdin parts

dabf1cf

gssbzn reviewed Nov 9, 2021

View reviewed changes

e2e/atlas/ldap_test.go Outdated Show resolved Hide resolved

Split LDAP tests into WithFlags and WithStdin

e835987

Remove assertion on BindPassword property from response. It doesn't exist.

colm-quinn reviewed Nov 15, 2021

View reviewed changes

andreaangiolillo reviewed Nov 15, 2021

View reviewed changes

fix linting

860efc6

gssbzn reviewed Nov 15, 2021

View reviewed changes

patdowney added 2 commits November 15, 2021 22:54

remove assertion against configuration.LDAP.BindPassword as it's not …

b4fd015

…populated

Separate TestDBUsers test for cloud manager into TestDBUsersWithFlags…

f9db653

… and TestDBUsersWith Stdin

gssbzn requested review from colm-quinn and andreaangiolillo November 16, 2021 15:18

gssbzn approved these changes Nov 16, 2021

View reviewed changes

colm-quinn approved these changes Nov 16, 2021

View reviewed changes

melissamahoney-mongodb approved these changes Nov 17, 2021

View reviewed changes

gssbzn merged commit db352de into mongodb:master Nov 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid leaking passwords into shell history and audits events by supporting reading passwords from stdin #881

Avoid leaking passwords into shell history and audits events by supporting reading passwords from stdin #881

patdowney commented Oct 10, 2021 •

edited by gssbzn

Loading

gssbzn commented Oct 11, 2021

patdowney commented Oct 11, 2021

gssbzn commented Oct 18, 2021

gssbzn commented Oct 28, 2021

patdowney commented Oct 28, 2021

gssbzn Nov 4, 2021 •

edited

Loading

gssbzn left a comment

gssbzn left a comment •

edited

Loading

gssbzn commented Nov 4, 2021

patdowney commented Nov 4, 2021

gssbzn commented Nov 4, 2021

patdowney commented Nov 4, 2021

patdowney commented Nov 4, 2021

gssbzn commented Nov 4, 2021

gssbzn commented Nov 8, 2021

colm-quinn commented Nov 15, 2021

colm-quinn Nov 15, 2021

patdowney Nov 15, 2021

andreaangiolillo left a comment

andreaangiolillo Nov 15, 2021

patdowney Nov 15, 2021

andreaangiolillo Nov 15, 2021

gssbzn Nov 15, 2021

gssbzn Nov 15, 2021

gssbzn Nov 15, 2021 •

edited

Loading

patdowney Nov 15, 2021

gssbzn left a comment

gssbzn left a comment

colm-quinn left a comment

gssbzn commented Nov 17, 2021

patdowney commented Nov 17, 2021

Avoid leaking passwords into shell history and audits events by supporting reading passwords from stdin #881

Avoid leaking passwords into shell history and audits events by supporting reading passwords from stdin #881

Conversation

patdowney commented Oct 10, 2021 • edited by gssbzn Loading

Proposed changes

Checklist

gssbzn commented Oct 11, 2021

patdowney commented Oct 11, 2021

gssbzn commented Oct 18, 2021

gssbzn commented Oct 28, 2021

patdowney commented Oct 28, 2021

gssbzn Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

gssbzn left a comment

Choose a reason for hiding this comment

gssbzn left a comment • edited Loading

Choose a reason for hiding this comment

gssbzn commented Nov 4, 2021

patdowney commented Nov 4, 2021

gssbzn commented Nov 4, 2021

patdowney commented Nov 4, 2021

patdowney commented Nov 4, 2021

gssbzn commented Nov 4, 2021

gssbzn commented Nov 8, 2021

colm-quinn commented Nov 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaangiolillo left a 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

gssbzn Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gssbzn left a comment

Choose a reason for hiding this comment

gssbzn left a comment

Choose a reason for hiding this comment

colm-quinn left a comment

Choose a reason for hiding this comment

gssbzn commented Nov 17, 2021

patdowney commented Nov 17, 2021

patdowney commented Oct 10, 2021 •

edited by gssbzn

Loading

gssbzn Nov 4, 2021 •

edited

Loading

gssbzn left a comment •

edited

Loading

gssbzn Nov 15, 2021 •

edited

Loading