Use pass backend #451

tych0 · 2017-08-17T15:54:49Z

To the end of encrypting passwords by default, there is now a pass backend, which will be useable on headless linuxen. Let's prefer that instead of the secretservice backend when choosing a default password backend on linux.

vdemeester

LGTM 🦁

dnephin

LGTM

dnephin · 2017-08-17T16:08:41Z

vendor.conf

 github.com/docker/distribution b38e5838b7b2f2ad48e06ec4b500011976080621
 github.com/docker/docker d58ffa0364c04d03a8f25704d7f0489ee6cd9634
-github.com/docker/docker-credential-helpers v0.5.1
+github.com/docker/docker-credential-helpers 3c90bd29a46b943b2a9842987b58fb91a7c1819b


Should we tag a v0.6 release so we can keep using a tag?

What's the release policy on this repo?

tych0 · 2017-08-17T16:38:03Z

On Thu, Aug 17, 2017 at 04:08:48PM +0000, Daniel Nephin wrote: dnephin approved this pull request. LGTM > @@ -8,7 +8,7 @@ github.com/cpuguy83/go-md2man a65d4d2de4d5f7c74868dfa9b202a3c8be315aaa github.com/davecgh/go-spew 346938d642f2ec3594ed81d874461961cd0faa76 github.com/docker/distribution b38e5838b7b2f2ad48e06ec4b500011976080621 github.com/docker/docker d58ffa0364c04d03a8f25704d7f0489ee6cd9634 -github.com/docker/docker-credential-helpers v0.5.1 +github.com/docker/docker-credential-helpers 3c90bd29a46b943b2a9842987b58fb91a7c1819b Should we tag a v0.5.3 release so we can keep using a tag? What's the release policy on this repo?

Happy to update if necessary. If we do tag, can we merge docker/docker-credential-helpers#84 first?

vdemeester · 2017-08-17T16:45:24Z

@tych0 yes 👼 once it's merged in docker-credential-helpers I'll make a 0.6.0 release 😉

andrewhsu · 2017-08-30T01:13:33Z

Does this handle migration of credentials from older format?

Also, any suitable tests we should have verify expected functionality of this PR?

tych0 · 2017-08-30T05:16:39Z

On Wed, Aug 30, 2017 at 01:13:35AM +0000, Andrew Hsu wrote: Does this handle migration of credentials from older format?

It doesn't migrate credentials from the older format, although we could implement that. It simply uses them in whatever cred store they're currently stored in (including the base64 encoded native one). For new hosts with pass configured it will use that cred store.

Also, any suitable tests we should have verify expected functionality of this PR?

Yes, if you test `docker login`, a `docker login` with `pass` configured would be sufficient, and we could probably test that the raw password file has the right contents at the end of a `docker login` too, just to be sure.

tych0 · 2017-08-30T21:40:15Z

Hey guys, just a heads up, I've updated this version to a newer version than the 0.6.0 tag, because that includes a better initialization check. I can revert to the tag if we really want, but since a few other things aren't, and it offers a relevant feature, I figured I'd just use the hash.

I've also rebased on top of the latest master. Let me know if anything looks funny. Thanks!

codecov-io · 2017-08-30T21:42:47Z

Codecov Report

Merging #451 into master will decrease coverage by 0.46%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #451      +/-   ##
==========================================
- Coverage   49.52%   49.05%   -0.47%     
==========================================
  Files         207      201       -6     
  Lines       17146    16454     -692     
==========================================
- Hits         8491     8072     -419     
+ Misses       8223     7962     -261     
+ Partials      432      420      -12

dnephin

still LGTM

tych0 · 2017-09-20T17:13:24Z

Ping, any movement on this?

In the next patch, we'll use this to implement some logic about which password backend to use. Signed-off-by: Tycho Andersen <tycho@docker.com>

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

thaJeztah

Generally LGTM, but a couple of things:

Can we test/double check upgrading if a user currently uses the "secret service" credentials helper? I.e., if a user uses that currently, we should make docker keep using that (similar to the way we handle graph-drivers; if a previous directory of a graph-driver is detected, that one is used)
This needs an update to the documentation; https://github.com/docker/cli/blob/master/docs/reference/commandline/login.md#credentials-store. We need to mention the new helper, and describe what the defaults are / priority is
The docs also mention that credsStore must be present in the config.json to make docker use a credentials helper; is that still the case?

thaJeztah · 2017-09-21T08:37:47Z

vendor.conf

 github.com/docker/distribution edc3ab29cdff8694dd6feb85cfeb4b5f1b38ed9c
 github.com/docker/docker 84144a8c66c1bb2af8fa997288f51ef2719971b4
-github.com/docker/docker-credential-helpers v0.5.1
+github.com/docker/docker-credential-helpers 3c90bd29a46b943b2a9842987b58fb91a7c1819b


We should update to a tagged release probably

As I mentioned in the comment above, there is no tagged version with commits relevant to this patch, so unless we tag a 0.6.1, I don't think we should.

n4ss

Lgtm

tych0 · 2017-09-21T14:20:36Z

On Thu, Sep 21, 2017 at 02:07:00AM -0700, Sebastiaan van Stijn wrote: thaJeztah requested changes on this pull request. Generally LGTM, but a couple of things: - Can we test/double check upgrading if a user currently uses the "secret service" credentials helper? I.e., if a user uses that currently, we should make docker keep using that (similar to the way we handle graph-drivers; if a previous directory of a graph-driver is detected, that one is used)

It already does this. If it uses the secret service helper, it sets the credsStore key to "secretservice", and the defaults are no longer used.

- This needs an update to the documentation; https://github.com/docker/cli/blob/master/docs/reference/commandline/login.md#credentials-store. We need to mention the new helper, and describe what the defaults are / priority is

Sure, I can update those.

- The docs also mention that `credsStore` must be present in the `config.json` to make docker use a credentials helper; is that still the case?

Well, it depends :). If you have a pre-configured set of credentials then yes, but if you're doing a new login and have the binary, it will auto-detect that and use it, and save the credsStore key for future reference.

tych0 · 2017-09-21T14:33:32Z

Ok, I've updated the docs. I think this should be good to go now.

mdlinville · 2017-09-21T16:32:46Z

docs/reference/commandline/login.md


+### Default behavior
+
+By default, Docker will look for the native binary on each of the platforms,


s/will look/looks

Maybe a table for the default backend per OS?

mdlinville · 2017-09-21T16:33:27Z

docs/reference/commandline/login.md

+special case is that on Linux, Docker will fall back to the "secretservice"
+binary if it cannot find the "pass" binary.
+
+*NOTE:* If you do not supply a credential helper binary, Docker will store your


Just Note, not NOTE

s/will store/stores

tych0 · 2017-09-21T16:49:32Z

On Thu, Sep 21, 2017 at 09:33:47AM -0700, Misty Stanley-Jones wrote: mstanleyjones requested changes on this pull request. > @@ -97,6 +98,16 @@ For example, to use `docker-credential-osxkeychain`: If you are currently logged in, run `docker logout` to remove the credentials from the file and run `docker login` again. +### Default behavior + +By default, Docker will look for the native binary on each of the platforms, s/will look/looks Maybe a table for the default backend per OS?

I thought about that, but the point is mostly to state that linux is a bit different and that it tries pass and falls back to secret service, and that doesn't really fit nicely in the table :( Anyway, I've updated the other bits, thanks!

…

> @@ -97,6 +98,16 @@ For example, to use `docker-credential-osxkeychain`: If you are currently logged in, run `docker logout` to remove the credentials from the file and run `docker login` again. +### Default behavior + +By default, Docker will look for the native binary on each of the platforms, +i.e. "osxkeychain" on macOS, "wincred" on windows, and "pass" on Linux. A +special case is that on Linux, Docker will fall back to the "secretservice" +binary if it cannot find the "pass" binary. + +*NOTE:* If you do not supply a credential helper binary, Docker will store your Just Note, not NOTE s/will store/stores -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #451 (review)

mdlinville

Docs LGTM

thaJeztah · 2017-09-26T09:52:05Z

and save the credsStore key for future reference.

Oh! I forgot (or wasn't aware) the client saves the credStore to the config.json after detection; I thought it checked each time if the user did not set this.

thaJeztah

sorry for the delay; some minor nits, but no showstopper if we want this merged before those are addressed

thaJeztah · 2017-09-26T09:54:45Z

docs/reference/commandline/login.md

+case is that on Linux, Docker will fall back to the "secretservice" binary if
+it cannot find the "pass" binary.
+
+*Note:* If you do not supply a credential helper binary, Docker stores your


This should be formatted as a note;

> **Note**: If you do not supply a credential helper binary, Docker stores your > password in cleartext (base64 encoded) in your home directory!

Should this mention the actual file this is saved in as well (~/.docker/config.json)?

I see this is being referred to a couple lines up as well;

You can log into any public or private repository for which you have credentials. When you log in, the command stores encoded credentials in $HOME/.docker/config.json on Linux or %USERPROFILE%/.docker/config.json on Windows.

Also add a big warning about cleartext passwords. Signed-off-by: Tycho Andersen <tycho@docker.com>

tych0 · 2017-09-26T14:32:17Z

Ok, I tried to address those (I'm not sure what you meant by formatted as a note, so I just moved the text into the paragraph, since it probably should be there anyway).

thaJeztah

LGTM

thaJeztah · 2017-09-26T14:45:09Z

@tych0 @seemethere are there changes needed for packaging?

tych0 · 2017-09-26T14:46:05Z

Yep, we talked a few weeks ago about it and I think we're all on the same page.

tych0 · 2017-09-26T16:52:19Z

Oh, but we didn't actually talk about the OSX or windows packaging/install files. @seemethere do you know who does that?

vdemeester · 2017-09-26T16:55:30Z

@tych0 for docker4desktop, there is already osx-keychain and wincreds support (cc @ebriney @simonferquel) 😉

tych0 · 2017-09-26T17:06:11Z

Cool, are we distributing the credhelpers binaries in the packages?

vdemeester · 2017-09-26T17:06:50Z

I'm pretty sure we do yes 👼

tych0 · 2017-09-26T17:08:32Z

Sweet, so that part is solved already then :)

ebriney · 2017-09-27T07:46:31Z

Yes, on windows and on mac, it is enabled by default. Thanks for this PR!

[18.03] vndr swarmkit to 49a9d7f

GordonTheTurtle added the status/0-triage label Aug 17, 2017

tych0 force-pushed the use-pass-backend branch from 6aecb92 to 3f5aac8 Compare August 17, 2017 15:55

vdemeester added status/1-design-review and removed status/0-triage labels Aug 17, 2017

vdemeester approved these changes Aug 17, 2017

View reviewed changes

dnephin added impact/changelog impact/documentation labels Aug 17, 2017

dnephin approved these changes Aug 17, 2017

View reviewed changes

tych0 force-pushed the use-pass-backend branch from 3f5aac8 to 4ad5943 Compare August 30, 2017 21:38

dnephin approved these changes Sep 1, 2017

View reviewed changes

Tycho Andersen added 3 commits September 20, 2017 11:14

defaultCredentialStore: make this a function

4cf1849

In the next patch, we'll use this to implement some logic about which password backend to use. Signed-off-by: Tycho Andersen <tycho@docker.com>

version bump for docker-credential-helpers

6bd5c63

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

prefer pass backend if it is available

713e87e

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

tych0 force-pushed the use-pass-backend branch from 4ad5943 to 713e87e Compare September 20, 2017 17:16

thaJeztah requested changes Sep 21, 2017

View reviewed changes

n4ss approved these changes Sep 21, 2017

View reviewed changes

tych0 requested a review from mdlinville as a code owner September 21, 2017 14:32

mdlinville suggested changes Sep 21, 2017

View reviewed changes

tych0 force-pushed the use-pass-backend branch from 90c56fb to 5956ad4 Compare September 21, 2017 16:42

mdlinville approved these changes Sep 21, 2017

View reviewed changes

thaJeztah requested changes Sep 26, 2017

View reviewed changes

document the pass backend and default behavior

a2f1167

Also add a big warning about cleartext passwords. Signed-off-by: Tycho Andersen <tycho@docker.com>

tych0 force-pushed the use-pass-backend branch from 5956ad4 to a2f1167 Compare September 26, 2017 14:31

thaJeztah approved these changes Sep 26, 2017

View reviewed changes

thaJeztah merged commit be8dab2 into docker:master Sep 26, 2017

GordonTheTurtle added this to the 17.10.0 milestone Sep 26, 2017

n4ss mentioned this pull request Sep 26, 2017

Proposal: deprecate base64 credentials in config.json #559

Open

thaJeztah mentioned this pull request Feb 15, 2018

Failed to use the client credentials store when running headless moby/moby#25169

Closed

thaJeztah mentioned this pull request Mar 27, 2019

docker login requires X11 support moby/moby#38948

Closed

thaJeztah mentioned this pull request Jul 12, 2019

docker build regression: GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.secrets was not provided by any .service files moby/buildkit#1078

Closed

rajivnathan mentioned this pull request Nov 28, 2019

Investigate keychain/keyring for managing Docker credentials on PFE eclipse-archived/codewind#1305

Open

2 tasks

thaJeztah mentioned this pull request Aug 20, 2022

pass: return an error when a cred doesn't exist docker/docker-credential-helpers#221

Merged

nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025

Merge pull request docker#451 from andrewhsu/md

3ab4d93

[18.03] vndr swarmkit to 49a9d7f


		### Default behavior

		By default, Docker will look for the native binary on each of the platforms,

Use pass backend #451

Use pass backend #451

Uh oh!

Conversation

tych0 commented Aug 17, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin Aug 17, 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!

tych0 commented Aug 17, 2017 via email

Uh oh!

vdemeester commented Aug 17, 2017

Uh oh!

andrewhsu commented Aug 30, 2017

Uh oh!

tych0 commented Aug 30, 2017 via email

Uh oh!

tych0 commented Aug 30, 2017

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

tych0 commented Sep 20, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

tych0 Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

n4ss left a comment

Choose a reason for hiding this comment

Uh oh!

tych0 commented Sep 21, 2017 via email

Uh oh!

tych0 commented Sep 21, 2017

Uh oh!

mdlinville Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

mdlinville Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

tych0 commented Sep 21, 2017 via email

Uh oh!

mdlinville left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 26, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

tych0 commented Sep 26, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 26, 2017

Uh oh!

dnephin Aug 17, 2017 •

edited

Loading

codecov-io commented Aug 30, 2017 •

edited

Loading

tych0 commented Sep 26, 2017 •

edited

Loading