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

tych0 · 2017-06-20T23:53:04Z

This isn't safe, since every commands arguments are available via
/proc//cmdline.

Let's print a nasty warning and then add a --password-file option, so
people can use a password file instead if they want automated access.

Signed-off-by: Tycho Andersen tycho@tycho.ws

codecov-io · 2017-06-21T00:01:50Z

Codecov Report

Merging #218 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
+ Coverage   46.83%   46.84%   +<.01%     
==========================================
  Files         172      172              
  Lines       11692    11692              
==========================================
+ Hits         5476     5477       +1     
+ Misses       5904     5903       -1     
  Partials      312      312

cpuguy83

LGTM

cpuguy83 · 2017-06-21T00:04:33Z

Needs a rebase.

tych0 · 2017-06-21T01:31:40Z

Done, looks like the previous merge broke the build, so I fixed that too, let me know if my fix is wrong; I don't know anything about this code :)

tych0 · 2017-06-21T01:36:33Z

Hmm. And it seems the linter is complaining about this... but if we try it the way it was (the way the linter is suggesting), it doesn't compile.

dnephin · 2017-06-21T03:40:49Z

but if we try it the way it was (the way the linter is suggesting), it doesn't compile.

It builds (at least on go 1.8.3) but the linter crashes.

dnephin · 2017-06-21T03:33:21Z

cli/command/registry/login.go

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

+	if opts.password != "" {
+		fmt.Fprintf(os.Stderr, "Using --password via the CLI is insecure. Please use --password-file.\n")


os.Stderr should be dockerCli.Err()

dnephin · 2017-06-21T03:44:51Z

cli/command/registry/login.go

+		fmt.Fprintf(os.Stderr, "Using --password via the CLI is insecure. Please use --password-file.\n")
+	}
+
+	if opts.passwordFile != "" {


We should also support reading from stdin if passwordFile == "-". There is code to handle that from an arg in other commands. I'm not sure if we have a function for it yet, or if maybe we could extract one.

Other places this is done are docker build, docker import, docker secret create

dnephin · 2017-06-21T03:45:38Z

cli/command/registry/login.go

+		 * conforms to the principle of least surprise, but I could be
+		 * wrong :)
+		 */
+		if contents[len(contents)-1] == '\n' {


Anytime I have a long comment like this I think it's a good idea to split it into a new function. That way the comment can serve as a docstring on the function instead of cluttering up the logic within a function. Even if the function is only 3 lines, that's fine.

Maybe something like normalizePasswordFileContent() ?

dnephin · 2017-06-21T03:51:24Z

I'm looking into the other 2 linter errors. I don't see them failing on master.

dnephin · 2017-06-21T04:17:12Z

Linter errors are fixed in #221 if you want to rebase on that commit (or just wait until it gets merged). There's some explanation of the problem as well.

tych0 · 2017-06-21T16:12:01Z

On Tue, Jun 20, 2017 at 9:40 PM, Daniel Nephin ***@***.***> wrote: but if we try it the way it was (the way the linter is suggesting), it doesn't compile. It does compile, CI was green. Are you building outside of a container with an older version of GO maybe?

I'm just typing "make" :). My host go version is 1.7.4.

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#218 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAv612gVBfkO3vY_T-GdtvVU4OzGvozOks5sGJDCgaJpZM4OAS0D> .

dnephin · 2017-06-21T16:19:43Z

You can use make -f docker.Makefile ... to run in a conatiner: https://github.com/docker/cli#development

tych0 · 2017-06-21T18:10:01Z

Thanks; seems that still didn't work for me (it didn't use the current version, where there was an obvious syntax error, but instead compiled successfully). Anyway, I dropped that commit and addressed all your feed back. I'm happy to rebase once #221 goes in, just ping me.

dnephin

One minor comment about the comment, otherwise looks good.

Thanks!

dnephin · 2017-06-21T18:52:45Z

cli/command/registry/login.go

+ * For users that do have a \n as the last character of their password, they
+ * need to store it as \n\n. I think this conforms to the principle of least
+ * surprise, but I could be wrong :)
+ */


I believe the go convention is to use // single line comments for everything instead of /* ... */

tych0 · 2017-06-21T19:55:03Z

Fixed, thanks!

dnephin

LGTM

tych0 · 2017-06-27T14:27:44Z

I've just rebased this now that #221 is merged, so this should be good to go as well.

n4ss · 2017-06-27T18:39:29Z

@vdemeester big improvement on the discussion we had. I'll open an issue to make sure that users simply don't provide passwords on the commandline, either stdin or from file

vdemeester

Thanks for the PR, two comments/questions :

Not a huge fan of -f short flag, doesn't tell me it's for password, user or both. -pf or no short flag
We should make those mutually exclusive (or warn that password file --password-file override --password)

(cc @thaJeztah)

This isn't safe, since every commands arguments are available via /proc/<pid>/cmdline. Let's print a nasty warning and then add a --password-file option, so people can use a password file instead if they want automated access. Signed-off-by: Tycho Andersen <tycho@tycho.ws>

tych0 · 2017-06-28T15:37:23Z

The CLI library doesn't seem to like -pf:

panic: shorthand is more than one character

goroutine 1 [running]:
panic(0xd24a00, 0xc4203b1050)
	/usr/lib/go-1.7/src/runtime/panic.go:500 +0x1a1
github.com/docker/cli/vendor/github.com/spf13/pflag.(*FlagSet).AddFlag(0xc420400900, 0xc420410000)
	/home/tycho/packages/go/src/github.com/docker/cli/vendor/github.com/spf13/pflag/flag.go:738 +0x61f
github.com/docker/cli/vendor/github.com/spf13/pflag.(*FlagSet).VarPF(0xc420400900, 0x135e120, 0xc42035d1f0, 0xe85c29, 0xd, 0xe7974b, 0x2, 0xeb2e26, 0x34, 0xc420403f40)
	/home/tycho/packages/go/src/github.com/docker/cli/vendor/github.com/spf13/pflag/flag.go:706 +0x141
github.com/docker/cli/vendor/github.com/spf13/pflag.(*FlagSet).VarP(0xc420400900, 0x135e120, 0xc42035d1f0, 0xe85c29, 0xd, 0xe7974b, 0x2, 0xeb2e26, 0x34)
	/home/tycho/packages/go/src/github.com/docker/cli/vendor/github.com/spf13/pflag/flag.go:712 +0x8e
github.com/docker/cli/vendor/github.com/spf13/pflag.(*FlagSet).StringVarP(0xc420400900, 0xc42035d1f0, 0xe85c29, 0xd, 0xe7974b, 0x2, 0x0, 0x0, 0xeb2e26, 0x34)
	/home/tycho/packages/go/src/github.com/docker/cli/vendor/github.com/spf13/pflag/string.go:42 +0xab
github.com/docker/cli/cli/command/registry.NewLoginCommand(0x13620e0, 0xc4201cf740, 0xc4203f8480)
	/home/tycho/packages/go/src/github.com/docker/cli/cli/command/registry/login.go:44 +0x2e1
github.com/docker/cli/cli/command/commands.AddCommands(0xc4203ae480, 0xc4201cf740)
	/home/tycho/packages/go/src/github.com/docker/cli/cli/command/commands/commands.go:51 +0x175
main.newDockerCommand(0xc4201cf740, 0xc42002e008)
	/home/tycho/packages/go/src/github.com/docker/cli/cmd/docker/docker.go:68 +0x4ca
main.main()
	/home/tycho/packages/go/src/github.com/docker/cli/cmd/docker/docker.go:166 +0xbd

so I've just dropped the short flag for now, and made them mutually exclusive.

thaJeztah · 2017-06-28T20:36:27Z

Looks like I'm a bit late to the party; I'm

👍 on a big red warning when using --password
👎 on adding a password-file option

Having a password file means: hey! you should store your password in a plain-text file on your machine!

I don't think we should promote this

What if, instead, we allow the password to be piped in? Looks like it's currently not supported;

# or whatever means to pipe the password
$ echo $SECRET | docker login -u myname
Error: Cannot perform an interactive login from a non TTY device

thaJeztah

can you split the "password file" and "warning" parts to two separate PR's? I think they need separate discussions

tych0 · 2017-06-29T15:19:21Z

Done, see #270 and #271 (note that 271 has 270's commit in it too, because I update the message there).

jikuma · 2017-07-12T09:41:30Z

please dont just have --password-file options but also have environment variables like DOCKER_ServerURL, DOCKER_Username and DOCKER_Secret. This can help in atomating the docker login process.

tych0 · 2017-07-12T14:35:31Z

Putting secrets in environment variables is just as unsafe as putting them in the command line (the task's initial environment is available via /proc/pid/environ). There are some other ways this leaks too: https://diogomonica.com/2017/03/27/why-you-shouldnt-use-env-variables-for-secret-data/

So the patches will only support taking passwords via stdin.

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

cpuguy83 approved these changes Jun 21, 2017

View reviewed changes

tych0 force-pushed the warn-about-password-on-cli branch from fa92e4d to 92c3ecf Compare June 21, 2017 01:31

cpuguy83 added status/2-code-review and removed status/0-triage labels Jun 21, 2017

dnephin reviewed Jun 21, 2017

View reviewed changes

tych0 force-pushed the warn-about-password-on-cli branch from 92c3ecf to a7264ac Compare June 21, 2017 18:08

dnephin reviewed Jun 21, 2017

View reviewed changes

tych0 force-pushed the warn-about-password-on-cli branch from a7264ac to c754a2d Compare June 21, 2017 19:54

dnephin approved these changes Jun 21, 2017

View reviewed changes

tych0 force-pushed the warn-about-password-on-cli branch from c754a2d to 7677b98 Compare June 27, 2017 14:27

vdemeester requested changes Jun 28, 2017

View reviewed changes

tych0 force-pushed the warn-about-password-on-cli branch from 7677b98 to ea37ec4 Compare June 28, 2017 15:37

thaJeztah requested changes Jun 28, 2017

View reviewed changes

tych0 closed this Jun 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tych0 commented Jun 20, 2017

codecov-io commented Jun 21, 2017 •

edited

Loading

cpuguy83 left a comment

cpuguy83 commented Jun 21, 2017

tych0 commented Jun 21, 2017

tych0 commented Jun 21, 2017

dnephin commented Jun 21, 2017 •

edited

Loading

dnephin Jun 21, 2017

dnephin Jun 21, 2017 •

edited

Loading

dnephin Jun 21, 2017

dnephin commented Jun 21, 2017

dnephin commented Jun 21, 2017

tych0 commented Jun 21, 2017 via email

dnephin commented Jun 21, 2017

tych0 commented Jun 21, 2017

dnephin left a comment

dnephin Jun 21, 2017

tych0 commented Jun 21, 2017

dnephin left a comment

tych0 commented Jun 27, 2017

n4ss commented Jun 27, 2017 •

edited

Loading

vdemeester left a comment •

edited

Loading

tych0 commented Jun 28, 2017

thaJeztah commented Jun 28, 2017 •

edited

Loading

thaJeztah left a comment

tych0 commented Jun 29, 2017

jikuma commented Jul 12, 2017

tych0 commented Jul 12, 2017

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

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

Conversation

tych0 commented Jun 20, 2017

codecov-io commented Jun 21, 2017 • edited Loading

Codecov Report

cpuguy83 left a comment

Choose a reason for hiding this comment

cpuguy83 commented Jun 21, 2017

tych0 commented Jun 21, 2017

tych0 commented Jun 21, 2017

dnephin commented Jun 21, 2017 • edited Loading

dnephin Jun 21, 2017

Choose a reason for hiding this comment

dnephin Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

dnephin Jun 21, 2017

Choose a reason for hiding this comment

dnephin commented Jun 21, 2017

dnephin commented Jun 21, 2017

tych0 commented Jun 21, 2017 via email

dnephin commented Jun 21, 2017

tych0 commented Jun 21, 2017

dnephin left a comment

Choose a reason for hiding this comment

dnephin Jun 21, 2017

Choose a reason for hiding this comment

tych0 commented Jun 21, 2017

dnephin left a comment

Choose a reason for hiding this comment

tych0 commented Jun 27, 2017

n4ss commented Jun 27, 2017 • edited Loading

vdemeester left a comment • edited Loading

Choose a reason for hiding this comment

tych0 commented Jun 28, 2017

thaJeztah commented Jun 28, 2017 • edited Loading

thaJeztah left a comment

Choose a reason for hiding this comment

tych0 commented Jun 29, 2017

jikuma commented Jul 12, 2017

tych0 commented Jul 12, 2017

codecov-io commented Jun 21, 2017 •

edited

Loading

dnephin commented Jun 21, 2017 •

edited

Loading

dnephin Jun 21, 2017 •

edited

Loading

n4ss commented Jun 27, 2017 •

edited

Loading

vdemeester left a comment •

edited

Loading

thaJeztah commented Jun 28, 2017 •

edited

Loading