Deprecate unencrypted storage by tych0 · Pull Request #561 · docker/cli

tych0 · 2017-09-26T16:55:06Z

This is an implementation of #559.

cc @n4ss @thaJeztah @vdemeester @dnephin @vieux @tiborvass @cpuguy83

mdlinville

Maybe I'm missing it, but you should also mark it as deprecated in Cobra. @thaJeztah can you give guidance on where and how to do this?

dnephin · 2017-09-26T19:49:03Z

cli/config/credentials/file_store.go

+// FileStore implements a credentials store using
 // the docker configuration file to keep the credentials in plain text.
-type fileStore struct {
+type FileStore struct {


Instead of exporting this (something we want to deprecate) how about adding a new IsNotSecure() method to fileStore, and creating the interface in cli/command/registry/login.go to check it?

type notSecure interface { IsNotSecure() bool } if _, isNotSecure := creds.(notSecure); isNotSecure { ... }

I thought about this, but as it stands there are two backends: fileStore and nativeStore, which is what implements the docker credential helpers interface. You could imagine a credential helper that just wrote plain text to a file, which would not be secure, but writing "!isNotSecure" seems to imply that the backend is secure, when it's not: nativeStore can't vouch for what's on the other end.

Ok, then how about IsDeprecated() and type deprecated interface{...} ?

I thought about this too, but in the event that we don't deprecate this based on discussion, I think we should still merge this patch, but tagging it as deprecated would be incorrect. We could cast by GetFilename, since that is unique to the FileStore, but it doesn't really describe what we're trying to detect. So I dunno :)

Feel free to pick some other interface, my concern is just that it doesn't make sense to export this entire struct if all we need is some way detect that it is of this type.

Another option is to export an IsFileStore(). That way we're only exporting one function and not the entire struct.

dnephin · 2017-09-26T19:57:31Z

cli/command/registry/login.go

+		// logins and we don't want to break them. Maybe they'll see
+		// the warning in their logs and fix things. If it is
+		// interactive, let's ask them if they really want to do this.
+		if dockerCli.In().IsTerminal() {


Please extract the entire confirmation block into a new function so the comment can be a godoc string instead of inline comment.

dnephin · 2017-09-26T19:57:51Z

cli/command/registry/login.go

+			for {
+
+				fmt.Fprintf(dockerCli.Err(), "Do you want to continue? (yes/no): ")
+				choice, _, err := bufio.NewReader(dockerCli.In()).ReadLine()


Please use PromptForConfirmation https://github.com/docker/cli/blob/master/cli/command/utils.go#L67-L72

dnephin · 2017-09-26T20:00:34Z

@mstanleyjones I don't think this is related to a specific flag, so I don't know if there is anywhere in cobra to deprecate it.

mdlinville

Docs LGTM

tiborvass · 2017-09-26T22:37:20Z

FYI This is in design review phase.

dnephin

Design LGTM

dnephin · 2017-09-26T22:42:37Z

cli/command/registry/login.go

+const unencryptedWarning = `WARNING! Your password will be stored unencrypted in %s.
+Please configure a credential helper (see
+https://docs.docker.com/engine/reference/commandline/login/#credentials-store)
+to avoid this.


How about:

Configure a credential helper to remove this warning. See https://docs.docker.com/engine/reference/commandline/login/#credentials-store

👍 Better without the "please"

thaJeztah · 2017-09-26T22:51:32Z

docs/deprecated.md

+
+Docker before v17.10 stored user credentials in `~/.docker/config.json`
+unencrypted by default. As of v17.10 it prints a warning with a link to the
+credentials helper page and asks users if they want to continue. Eventually, we


I'd skip the niceties and just, e.g.: "Support for unencrypted password storage will be removed in Docker 18.10."

tiborvass · 2017-09-26T22:57:26Z

docs/deprecated.md

+
+**Deprecated In Release: v17.10.0**
+
+**Target For Removal In Release: v18.10**


I'm not sure this will be well received. As @cpuguy83 mentioned it, what about CI ? Same for any other situation where there is no secure alternative (pass, ...) and that it is run as a script (no tty)

I agree. Warning about it being insecure is good, but I don't think we should actually set a target for removal.

Reasonable questions. I'm not sure what the threat model is here, I guess it must be people concerned about folks outside their organization reading their images off the docker (or other publicly accessible) hub? It seems that anyone who can commit code to the thing in CI can read out these passwords if they're stored in plaintext, so it can't be about internal threats.

If that's the model, perhaps we can do something a-la Google's app-specific passwords (which we could implement before deprecating this)? If it's not, then this is broken and we should in fact remove it.

Ok, so after some discussions with @n4ss, it seems that this is exactly what people are doing. So without some reasonable replacement for this, we can't deprecate this bit. So, I'll just drop the second patch from this PR, and leave it with the big warning patch.

codecov-io · 2017-09-27T16:59:08Z

Codecov Report

❗ No coverage uploaded for pull request base (master@db11cff). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master     #561   +/-   ##
=========================================
  Coverage          ?   49.47%           
=========================================
  Files             ?      208           
  Lines             ?    17159           
  Branches          ?        0           
=========================================
  Hits              ?     8490           
  Misses            ?     8237           
  Partials          ?      432

vdemeester

LGTM 🐯
I'll rebase/fix conflict 👼

Signed-off-by: Tycho Andersen <tycho@docker.com>

thaJeztah · 2018-03-26T15:05:37Z

@gbarr01 wondering if we should have special URLs that we can use when referring to the docs from the command-line. I know some URLs may move, and can become outdated; should we have (e.g.) docs URLs that redirect to where we want them? like https://docs.docker.com/help/credentials-helpers (and have it redirect to https://docs.docker.com/engine/reference/commandline/login/#credentials-store)?

dnephin

LGTM

gbarr01 · 2018-03-26T18:32:32Z

@thaJeztah, sure thing. To confirm, the benefit here that the developer can always use the same base URL (https://docs.docker.com/help/) even if we are pointing credentials-helper to credentials-store.

thaJeztah · 2018-03-27T11:09:53Z

@gbarr01 almost, yes; we could use /help/<some topic> (in this case “credential-helpers”) and have those redirect

thaJeztah

LGTM, we can have a look at better URLs later

tych0 requested review from mdlinville, thaJeztah and vdemeester as code owners September 26, 2017 16:55

GordonTheTurtle added the status/0-triage label Sep 26, 2017

vdemeester added status/1-design-review and removed status/0-triage labels Sep 26, 2017

mdlinville requested a review from gbarr01 September 26, 2017 17:54

mdlinville suggested changes Sep 26, 2017

View reviewed changes

dnephin reviewed Sep 26, 2017

View reviewed changes

mdlinville approved these changes Sep 26, 2017

View reviewed changes

dnephin reviewed Sep 26, 2017

View reviewed changes

thaJeztah reviewed Sep 26, 2017

View reviewed changes

tiborvass reviewed Sep 26, 2017

View reviewed changes

tych0 force-pushed the deprecate-unencrypted-storage branch from 1d400f7 to 341de19 Compare September 27, 2017 16:59

tych0 force-pushed the deprecate-unencrypted-storage branch 2 times, most recently from d71c206 to bbcc7cf Compare September 27, 2017 18:44

vdemeester approved these changes Mar 26, 2018

View reviewed changes

vdemeester force-pushed the deprecate-unencrypted-storage branch from bbcc7cf to da265c3 Compare March 26, 2018 14:07

vdemeester added status/2-code-review and removed status/1-design-review labels Mar 26, 2018

print a big warning about cleartext passwords

4290df3

Signed-off-by: Tycho Andersen <tycho@docker.com>

vdemeester force-pushed the deprecate-unencrypted-storage branch from da265c3 to 4290df3 Compare March 26, 2018 14:18

dnephin approved these changes Mar 26, 2018

View reviewed changes

thaJeztah approved these changes Mar 27, 2018

View reviewed changes

thaJeztah merged commit e6e47d9 into docker:master Mar 27, 2018

GordonTheTurtle added this to the 18.04.0 milestone Mar 27, 2018

This was referenced Apr 5, 2018

gcloud beta auth -> gcloud auth GoogleContainerTools/skaffold#321

Merged

s/gcloud docker -- push/docker push/g everywhere in test-infra kubernetes/test-infra#6765

Merged

Make use of new k8s.gcr.io alias throughout kubernetes/release#515

Merged

n4ss mentioned this pull request Apr 16, 2018

Revert unencryted storage warning prompt #1008

Merged

thaJeztah added impact/deprecation docs/revisit labels Apr 30, 2018


		Deprecated In Release: v17.10.0

		Target For Removal In Release: v18.10

Conversation

tych0 commented Sep 26, 2017

Uh oh!

mdlinville left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin commented Sep 26, 2017

Uh oh!

mdlinville left a comment

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 26, 2018

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

gbarr01 commented Mar 26, 2018

Uh oh!

thaJeztah commented Mar 27, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

dnephin Sep 26, 2017 •

edited

Loading

tiborvass commented Sep 26, 2017 •

edited

Loading

codecov-io commented Sep 27, 2017 •

edited

Loading