Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

login: print a big warning when using --password #270

Merged
merged 1 commit into from
Jul 8, 2017

Conversation

tych0
Copy link
Contributor

@tych0 tych0 commented Jun 29, 2017

Task command lines are world readable via /proc/pid/cmdline, so this isn't
safe.

@codecov-io
Copy link

codecov-io commented Jun 29, 2017

Codecov Report

Merging #270 into master will decrease coverage by 1.58%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
- Coverage   48.44%   46.85%   -1.59%     
==========================================
  Files         173      172       -1     
  Lines       11748    11692      -56     
==========================================
- Hits         5691     5478     -213     
- Misses       5696     5902     +206     
+ Partials      361      312      -49

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

if opts.password != "" {
fmt.Fprintf(dockerCli.Err(), "Using --password via the CLI is insecure.\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to fmt.Fprintln()?

Perhaps prefix with WARNING! ;

fmt.Fprintln(dockerCli.Err(), "WARNING! Using --password via the CLI is insecure.")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Task command lines are world readable via /proc/pid/cmdline, so this isn't
safe.

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
@tych0 tych0 force-pushed the warn-only-about-password-on-cli branch from 952d7da to c269ad2 Compare July 3, 2017 14:47
Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐸
/cc @n4ss

@n4ss
Copy link
Contributor

n4ss commented Jul 3, 2017

LGTM!

Copy link
Contributor

@boaz0 boaz0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🤠

@vieux
Copy link
Contributor

vieux commented Jul 4, 2017

LGTM ping @thaJeztah

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, darn missed the ping

LGTM, thanks!

@thaJeztah thaJeztah merged commit c99530b into docker:master Jul 8, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.07.0 milestone Jul 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants