Password stdin #271

tych0 · 2017-06-29T15:18:22Z

Add a --password-stdin argument to the login command, and update the warning message from #270 to tell people about it.

dnephin · 2017-06-30T19:41:44Z

cli/command/registry/login.go

+		fmt.Fprintf(dockerCli.Err(), "Using --password via the CLI is insecure. Use --password-stdin\n")
+		if opts.passwordStdin {
+			return errors.Errorf("--password and --password-stdin are mutually exclusive")
+		}


I wonder if it would make sense to use -p - to mean stdin. Usually - is only for files, but maybe it would be ok to use that convention here as well.

It would break everyone whose password was "-", so it seems better not to.

Hopefully - is not an acceptable password on any registries, and they all require at least a few more characters..

dnephin · 2017-06-30T19:43:38Z

cli/command/registry/login.go

+	if opts.passwordStdin {
+		if opts.user == "" {
+			return errors.Errorf("Must provide --username with --password-stdin")
+		}


This doesn't seem to be a requirement for --password, why is it necessary for password-stdin ?

If you don't supply a username via --username, it asks via stdin, which then may eat parts of your password, depending on what characters it has in it (e.g. \n). Seems better to just disallow it to prevent confusion to me.

codecov-io · 2017-07-03T14:58:52Z

Codecov Report

Merging #271 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #271   +/-   ##
=======================================
  Coverage   48.68%   48.68%           
=======================================
  Files         186      186           
  Lines       12416    12416           
=======================================
  Hits         6045     6045           
  Misses       5996     5996           
  Partials      375      375

n4ss · 2017-07-03T15:13:02Z

cli/command/registry/login.go

+			return err
+		}
+
+		if contents[len(contents)-1] == '\n' {


We should still check that contents != "" before doing that.

Also on Windows we have CRLF so we should test "\r\n" when running on windows.

Do we have helper functions for this @thaJeztah @vdemeester ?

cc @johnstep

Good point. in fact, we should probably just use the stdlib to do all of this. Fixed, thanks!

n4ss · 2017-07-03T15:19:36Z

cli/command/registry/login.go

+		}
+
+		if contents[len(contents)-1] == '\n' {
+			opts.password = string(contents[:len(contents)-1])


same here for the CRLF, make sure we don't include the '\r'

aaronlehmann · 2017-07-06T22:41:10Z

cli/command/registry/login.go

+		}
+
+		opts.password = strings.TrimSuffix(string(contents), "\n")
+		if runtime.GOOS == "windows" {


Any reason to limit this to Windows? The password could end up with a CR if it's piped from a file with DOS line endings, or pasted from a weird source. I don't think it will matter much in practice, but applying the same trimming on both platforms would be a slight simplification.

Done, thanks.

tych0 · 2017-07-11T14:30:55Z

Ping. Any movement on this?

n4ss · 2017-07-11T14:31:57Z

LGTM!

n4ss · 2017-07-11T14:32:13Z

/cc @vdemeester

vdemeester

Design LGTM 👼

aaronlehmann · 2017-07-11T16:53:51Z

cli/command/registry/login.go

@@ -47,6 +51,27 @@ func runLogin(dockerCli command.Cli, opts loginOptions) error {
 	ctx := context.Background()
 	clnt := dockerCli.Client()

+	if opts.password != "" {
+		fmt.Fprintln(dockerCli.Err(), "WARNING! Using --password via the CLI is insecure. Use --password-stdin")


Should there be a period at the end of this? If Using --password via the CLI is insecure is formatted as a sentence, I think Use --password-stdin should be as well.

aaronlehmann · 2017-07-11T16:55:41Z

cli/command/registry/login.go

+	if opts.password != "" {
+		fmt.Fprintln(dockerCli.Err(), "WARNING! Using --password via the CLI is insecure. Use --password-stdin")
+		if opts.passwordStdin {
+			return errors.Errorf("--password and --password-stdin are mutually exclusive")


errors.New

And I'm wondering if there's a way to phrase this that's more friendly to people who don't speak english as a first language, but I don't have any good ideas right now.

Sure, I've left it as is now and fixed everything else. If someone has a better idea, let me know and I'll change it.

aaronlehmann · 2017-07-11T16:55:51Z

cli/command/registry/login.go

+
+	if opts.passwordStdin {
+		if opts.user == "" {
+			return errors.Errorf("Must provide --username with --password-stdin")


errors.New.

This: * conflicts with --password (naturally) * conflicts with the absence of --username (both can't be grabbed by the stdin) * strips a trailing newline off the password if it exists Signed-off-by: Tycho Andersen <tycho@tycho.ws>

aaronlehmann · 2017-07-11T18:40:27Z

LGTM

thaJeztah

LGTM --password-stdin is a bit verbose, but don't have a better solution (perhaps a -i shortcut)?

We probably want this in 17.07, so I'm ok merging as-is, but we also need;

changes to the completion scripts https://github.com/docker/cli/tree/master/contrib/completion
update to the documentation (https://github.com/docker/cli/blob/master/docs/reference/commandline/login.md)
update to the man-page (https://github.com/docker/cli/blob/master/man/src/login.md)

tych0 · 2017-07-13T00:01:10Z

Sure, I can work up docs patches, feel free to merge.

tych0 mentioned this pull request Jun 29, 2017

login: warn about --password on command line #218

Closed

GordonTheTurtle added the status/0-triage label Jun 29, 2017

dnephin added status/1-design-review and removed status/0-triage labels Jun 29, 2017

dnephin reviewed Jun 30, 2017

View reviewed changes

tych0 force-pushed the password-stdin branch from 078bd7a to 65e2839 Compare July 3, 2017 14:50

n4ss reviewed Jul 3, 2017

View reviewed changes

tych0 force-pushed the password-stdin branch from 65e2839 to a3ea86b Compare July 3, 2017 15:46

aaronlehmann reviewed Jul 6, 2017

View reviewed changes

tych0 force-pushed the password-stdin branch from a3ea86b to 4b72c26 Compare July 7, 2017 14:14

vdemeester reviewed Jul 11, 2017

View reviewed changes

vdemeester added status/2-code-review and removed status/1-design-review labels Jul 11, 2017

aaronlehmann reviewed Jul 11, 2017

View reviewed changes

tych0 force-pushed the password-stdin branch from 4b72c26 to 0b59dfb Compare July 11, 2017 18:16

login: add a --password-stdin argument

7d18477

This: * conflicts with --password (naturally) * conflicts with the absence of --username (both can't be grabbed by the stdin) * strips a trailing newline off the password if it exists Signed-off-by: Tycho Andersen <tycho@tycho.ws>

tych0 force-pushed the password-stdin branch from 0b59dfb to 7d18477 Compare July 11, 2017 18:16

thaJeztah approved these changes Jul 12, 2017

View reviewed changes

thaJeztah merged commit ae38f20 into docker:master Jul 13, 2017

GordonTheTurtle added this to the 17.07.0 milestone Jul 13, 2017

thaJeztah added the impact/changelog label Jul 13, 2017

alindeman mentioned this pull request Nov 30, 2017

Passes password to 'docker login' via stdin heroku/heroku-container-registry#27

Merged

0dd mentioned this pull request Aug 18, 2021

Docker Version should be greater then 17.07 wchaws/serverless-image-handler#41

Open

thaJeztah mentioned this pull request Sep 19, 2022

command should not accept API key as argument (--api-key) docker/base-cli-plugin#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Password stdin #271

Password stdin #271

tych0 commented Jun 29, 2017

dnephin Jun 30, 2017

tych0 Jun 30, 2017

dnephin Jun 30, 2017

dnephin Jun 30, 2017

tych0 Jun 30, 2017

codecov-io commented Jul 3, 2017 •

edited

Loading

n4ss Jul 3, 2017

n4ss Jul 3, 2017

vdemeester Jul 3, 2017

tych0 Jul 3, 2017

n4ss Jul 3, 2017

aaronlehmann Jul 6, 2017

tych0 Jul 7, 2017

tych0 commented Jul 11, 2017

n4ss commented Jul 11, 2017

n4ss commented Jul 11, 2017

vdemeester left a comment

aaronlehmann Jul 11, 2017

aaronlehmann Jul 11, 2017

tych0 Jul 11, 2017

aaronlehmann Jul 11, 2017

aaronlehmann commented Jul 11, 2017

thaJeztah left a comment

tych0 commented Jul 13, 2017

Password stdin #271

Password stdin #271

Conversation

tych0 commented Jun 29, 2017

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

codecov-io commented Jul 3, 2017 • edited Loading

Codecov Report

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

tych0 commented Jul 11, 2017

n4ss commented Jul 11, 2017

n4ss commented Jul 11, 2017

vdemeester 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

aaronlehmann commented Jul 11, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

tych0 commented Jul 13, 2017

codecov-io commented Jul 3, 2017 •

edited

Loading