Skip to content

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

Merged
thaJeztah merged 1 commit intodocker:masterfrom
tych0:warn-only-about-password-on-cli
Jul 8, 2017
Merged

login: print a big warning when using --password#270
thaJeztah merged 1 commit intodocker:masterfrom
tych0:warn-only-about-password-on-cli

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

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
nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025
[17.10] re-vndr swarmkit to 1d2bc2e
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