Silent login: use credentials from cred store to login #139

shhsu · 2017-05-30T22:00:03Z

- What I did
As discussed in #130 (comment), Azure Container Registry wants a way for docker login <registry> to proceed to log in if the credential store returns username and password.

This is a minor behavior change for docker login command scenario. In particular: when user call docker login without -p and -u, it used to be that the user would be prompted for new username and password interactively in all situations.

The new behavior would be a lot like docker pull. docker login. With this change, docker would now try to find credentials in the cred store first and try to login with it. Only if it fails with 401 status would docker cli prompt for new username and password.

- How I did it
The surface area of this code change was a little bigger than expected because of some refactoring. Functionality changes are mostly in cli/command/registry/login.go. There are some refactoring going on mainly in cli/command/registry.go because I didn't want to call cli.CredentialsStore(serverAddress).Get(serverAddress) twice in the scenario where docker is prompting for credentials interactively and looking for existing username in the cred store as default parameters.

I actually think the refactor may be useful as I can see similar changes being done to refactor pull, push, etc to save a call to CredentialsStore.Get in similar situation.

- How to verify it

Manually ran docker login with or without -p, -u with default registry. Observed expected behavior change:

Login prompt occurs as normal if the user wasn't logged in.
Login prompt would be omitted if the user was already logged in
Login prompt would occur if cred store credential is incorrect (simulate a credential expired scenario)

Manually ran docker login <registry> on Azure Container Registry

Login prompt is shown as expected, cred store is updated only after login

Manually ran docker pull <registry> without logging in and trigger a login

Login prompt is shown as expected, pull proceeded only after login

codecov-io · 2017-05-30T22:13:34Z

Codecov Report

Merging #139 into master will decrease coverage by 0.26%.
The diff coverage is 64.81%.

@@            Coverage Diff             @@
##           master     #139      +/-   ##
==========================================
- Coverage   53.26%   52.99%   -0.27%     
==========================================
  Files         258      261       +3     
  Lines       16357    16528     +171     
==========================================
+ Hits         8712     8759      +47     
- Misses       7081     7196     +115     
- Partials      564      573       +9

dnephin · 2017-05-31T14:46:04Z

cli/command/registry_test.go

+func TestGetDefaultAuthConfig(t *testing.T) {
+	testCases := []struct {
+		checkCredStore     bool
+		expectErr          bool


Instead of a boolean for the error could you use expectedErr string, and check that the error message contains that string.

dnephin · 2017-05-31T14:47:26Z

cli/command/registry_test.go

+		expectErr          bool
+		expectedAuthServer string
+		expectedUsername   string
+		expectedPassword   string


testify assertions can compare structs, so instead of separate fields for these I think it should be expected authConfig, that way you can just assert.Equal(t, testcase.expected, authconfig)

dnephin · 2017-05-31T14:49:48Z

cli/command/registry/login.go

+		} else if client.IsErrUnauthorized(err) {
+			fmt.Println("Stored credentials invalid or expired")
+		} else {
+			fmt.Println("Login did not succeed")


These should print to dockerCli.Err()

friism · 2017-05-31T15:55:47Z

Ping @n4ss @thaJeztah

shhsu · 2017-05-31T18:12:16Z

@sajayantony @DavidObando

DavidObando · 2017-06-08T19:26:38Z

Hi, I see this is labeled as "design review". We'll be happy to hear of any feedback you may have to light up this scenario.

n4ss · 2017-06-09T12:30:26Z

Hey, very sorry for the delayed answer!

I like the idea of automatically looking in the credstore at login by default to see if something already exists. This makes the experience way more user-friendly.

I know that this system won't be a problem for some of the credstores but I'm trying to come up with an attack scenario against this.

DavidObando · 2017-07-27T19:18:36Z

Hi @n4ss we'd love to hear some feedback on this when you have some.

justincormack · 2018-02-19T11:01:42Z

I can't see any security issues with this. Needs a rebase though!

n4ss · 2018-02-20T23:54:56Z

Double approval, lgtm on rebase as well.

shhsu · 2018-02-21T00:11:20Z

Hold on, Fixing the failed tests...

shhsu · 2018-02-21T21:55:10Z

Rebase done
Also run a couple rounds of manual testing. Seems to work.

n4ss · 2018-02-21T22:00:05Z

ping @thaJeztah @vdemeester ^_^

vdemeester

LGTM 🐯

vdemeester · 2018-02-22T08:38:44Z

cli/command/registry_test.go

 	"testing"

 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	"golang.org/x/net/context"

 	// Prevents a circular import with "github.com/docker/cli/internal/test"
+
+	"fmt"


nit: this import should be on top 👼

thaJeztah · 2018-02-22T10:38:21Z

@shhsu can you address @vdemeester's nit, and squash commits (where applicable; looks like there's some "fix up" commits in there)

…d but both username and password are retrieved in cred store, docker will automatically use the credentials found in the cred store to log in Signed-off-by: shhsu@microsoft.com <shhsu@microsoft.com> Signed-off-by: Peter Hsu <shhsu@microsoft.com> Signed-off-by: shhsu <shhsu@microsoft.com> Signed-off-by: Peter Hsu <shhsu@microsoft.com>

shhsu · 2018-02-22T19:38:45Z

Done

@thaJeztah @vdemeester

thaJeztah

LGTM, thanks!

…-19.03-70f48f223157f5b567a87c563fa4997065fc6fde [19.03] sync to upstream 19.03 70f48f2

GordonTheTurtle added dco/no status/0-triage and removed dco/no labels May 30, 2017

shhsu mentioned this pull request May 30, 2017

Azure Contianer Registry login scenario discussion #130

Closed

GordonTheTurtle added dco/no and removed dco/no labels May 30, 2017

dnephin added status/1-design-review and removed status/0-triage labels May 31, 2017

dnephin reviewed May 31, 2017

View reviewed changes

GordonTheTurtle added dco/no and removed dco/no labels Jun 1, 2017

DavidObando mentioned this pull request Jun 16, 2017

Enable interactive login Azure/acr-docker-credential-helper#2

Open

docker deleted a comment from GordonTheTurtle Jun 20, 2017

justincormack approved these changes Feb 19, 2018

View reviewed changes

GordonTheTurtle added dco/no and removed dco/no labels Feb 21, 2018

docker deleted a comment from GordonTheTurtle Feb 22, 2018

vdemeester approved these changes Feb 22, 2018

View reviewed changes

vdemeester added status/2-code-review and removed status/1-design-review labels Feb 23, 2018

thaJeztah added the impact/changelog label Feb 27, 2018

thaJeztah approved these changes Feb 27, 2018

View reviewed changes

thaJeztah merged commit 2cc6ff1 into docker:master Feb 27, 2018

GordonTheTurtle added this to the 18.04.0 milestone Feb 27, 2018

gbarr01 mentioned this pull request Apr 10, 2018

To doc: Silent login: use credentials from cred store to login docker/docs#6417

Closed

thaJeztah mentioned this pull request Apr 10, 2018

[18.03] backport content trust tests, windows, test-refactoring docker-archive/docker-ce#504

Merged

jamesrgrinter mentioned this pull request Apr 12, 2018

non-interactive docker login fails #1002

Closed

thaJeztah mentioned this pull request Sep 26, 2018

[18.09] backport fix substitution with non-empty env-var #1394

Merged

trapier pushed a commit to trapier/cli that referenced this pull request Sep 30, 2019

Merge pull request docker#139 from GordonTheTurtle/sync-with-upstream…

217c17c

…-19.03-70f48f223157f5b567a87c563fa4997065fc6fde [19.03] sync to upstream 19.03 70f48f2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Silent login: use credentials from cred store to login #139

Silent login: use credentials from cred store to login #139

shhsu commented May 30, 2017 •

edited

Loading

codecov-io commented May 30, 2017 •

edited

Loading

dnephin May 31, 2017

dnephin May 31, 2017

dnephin May 31, 2017

friism commented May 31, 2017

shhsu commented May 31, 2017

DavidObando commented Jun 8, 2017

n4ss commented Jun 9, 2017

DavidObando commented Jul 27, 2017

justincormack commented Feb 19, 2018

n4ss commented Feb 20, 2018

shhsu commented Feb 21, 2018

shhsu commented Feb 21, 2018

n4ss commented Feb 21, 2018

vdemeester left a comment

vdemeester Feb 22, 2018

shhsu Feb 22, 2018

thaJeztah commented Feb 22, 2018

shhsu commented Feb 22, 2018

thaJeztah left a comment

Silent login: use credentials from cred store to login #139

Silent login: use credentials from cred store to login #139

Conversation

shhsu commented May 30, 2017 • edited Loading

codecov-io commented May 30, 2017 • edited Loading

Codecov Report

dnephin May 31, 2017

Choose a reason for hiding this comment

dnephin May 31, 2017

Choose a reason for hiding this comment

dnephin May 31, 2017

Choose a reason for hiding this comment

friism commented May 31, 2017

shhsu commented May 31, 2017

DavidObando commented Jun 8, 2017

n4ss commented Jun 9, 2017

DavidObando commented Jul 27, 2017

justincormack commented Feb 19, 2018

n4ss commented Feb 20, 2018

shhsu commented Feb 21, 2018

shhsu commented Feb 21, 2018

n4ss commented Feb 21, 2018

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Feb 22, 2018

Choose a reason for hiding this comment

shhsu Feb 22, 2018

Choose a reason for hiding this comment

thaJeztah commented Feb 22, 2018

shhsu commented Feb 22, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

shhsu commented May 30, 2017 •

edited

Loading

codecov-io commented May 30, 2017 •

edited

Loading