support legacy registries in exernal stores #23100

runcom · 2016-05-30T10:49:17Z

Tackling and fixing #22910

Signed-off-by: Antonio Murdaca runcom@redhat.com

runcom · 2016-05-31T17:49:49Z

naked pings @thaJeztah @icecrime @vdemeester @calavera @cpuguy83 :)

thaJeztah · 2016-05-31T20:29:30Z

I'd like to have @dmcgowan @aaronlehmann @stevvooe to have a look as well (for registry/distribution)

stevvooe · 2016-05-31T21:17:33Z

I'm not wildly familiar with the behavior. My initial concern is that we need to be careful about making certain endpoints the same that are not to avoid credential leak. The following are pitfalls that must be avoided:

https should not equal http - do not leak https credentials over http.
https://domain is not the same as xxx://domain:443 - avoid leaking credentials with plain text over port 443.

Looking at the original complaint in #22910, I'm wondering if the mistake here is stripping the scheme but I am not sure I understand this issue deeply enough.

icecrime · 2016-06-01T15:19:27Z

Failure in gccgo and janky?

20:24:16 --- FAIL: TestNativeStoreGetMissingCredentials (0.00s)
20:24:16    native_store_test.go:286: unknown argument "get" with ""

aaronlehmann · 2016-06-01T17:29:58Z

I think the root cause of this is that docker login is allowed to take a fully qualified URL with a scheme, but commands like docker pull and docker push always use the reference grammar, which omits the scheme.

As I understand the problem, docker login https://example.com will store https://example.com in an external credentials store, and when we try to pull example.com/image, the lookup in the credential store is for example.com, which doesn't match.

As @stevvooe points out, ignoring the scheme stored with an authentication credential is potentially problematic. If the user specified creds to be associated with a https:// URL, it would be inappropriate for us to ever use those credentials over plaintext HTTP. That said, I don't think we ever allow sending creds over plaintext HTTP, so this might not actually be a problem. Still, it feels strange to allow a user to specify https:// in docker login, and then strip off the https://.

Is there any good reason to allow schemes to be specified to docker login? It seems like it would be a lot more consistent if docker login didn't accept schemes, like docker pull and docker push. It may be an accident that a fully-qualified URL is accepted.

But changing this might break some existing users, for example the ECR case discussed in #22910.

runcom · 2016-06-10T09:05:51Z

It may be an accident that a fully-qualified URL is accepted.

this behavior has been kept since old Dockers so we still maintain it with when retrieving creds from the filestore - the problem may be that we don't do the same with the external store

calavera · 2016-06-16T23:46:30Z

Is there any good reason to allow schemes to be specified to docker login

Is there a good reason to strip the scheme from the url, turning it into a different thing? example.com is not the same as https://example.com. I ask because I honestly don't know why pull and push have that behavior.

aaronlehmann · 2016-06-16T23:50:28Z

I don't know either why pull and push work like that.

thaJeztah · 2016-07-01T17:41:04Z

ping @aaronlehmann @stevvooe what should we do here?

stevvooe · 2016-07-02T00:29:03Z

cliconfig/credentials/native_store.go

@@ -100,6 +110,16 @@ func (c *nativeStore) GetAll() (map[string]types.AuthConfig, error) {
 	return auths, nil
 }

+func (c *nativeStore) getLegacyServerAddress(serverAddress string) string {


Could you add a comment here explaining what a "legacy" address is?

it's a registry address with http[s]:// prefixed and possibly a path url in there (which will be stripped)

To the code?

stevvooe · 2016-07-02T00:29:17Z

Add a comment and do a rebase.

runcom · 2016-07-02T10:00:08Z

rebased, added a comment to @stevvooe's comment, this PR still lacks of everything I said in the first comment and which make the PR itself not ready to code review

Current status:

ppl can login prepending the scheme to the registry
with the above point - pull, push operations are ok with the filestore (.docker/config.json)
...but the above isn't working with external stores
logout does not work at all with registries saved with https|http when logging out using just the hostname
probably other things

thaJeztah · 2016-07-14T16:10:16Z

ping @runcom build is failing;

10:06:59 # github.com/docker/docker/cliconfig/credentials
10:06:59 cliconfig/credentials/native_store.go:49: undefined: errCredentialsNotFound
10:06:59 cliconfig/credentials/native_store.go:72: undefined: errCredentialsNotFound
10:06:59 cliconfig/credentials/native_store.go:133: undefined: errCredentialsNotFound

runcom · 2016-07-14T16:12:28Z

@thaJeztah right, I'm holding a rebase again because I did not understand what we should do with this PR, given my comment in #23100 (comment) which lists some outstanding issues

thaJeztah · 2016-07-14T16:47:31Z

@runcom alright, yeah, came here because I don't know either what's the best solution, but spotted it was failing CI 😊

runcom · 2016-09-01T16:01:26Z

@aaronlehmann @stevvooe any hint on how to go ahead with this one? give:

ppl can login prepending the scheme to the registry
with the above point - pull, push operations are ok with the filestore (.docker/config.json)
...but the above isn't working with external stores
logout does not work at all with registries saved with https|http when logging out using just the hostname probably other things

aaronlehmann · 2016-09-02T17:03:53Z

I'm okay with accepting a scheme in docker login and stripping out the scheme before saving the registry name to the credentials store. We will always use HTTPS unless the user explicitly sets up the daemon to allow plain HTTP for that registry, so while this is a bit kludgy, it doesn't pose a real risk of exposing credentials over unencrypted HTTP against the user's intentions.

runcom · 2016-09-02T17:18:10Z

@aaronlehmann alright, I'll adapt the PR accordingly, thanks

vdemeester · 2016-09-06T14:07:05Z

LGTM 🐸
/cc @aaronlehmann @stevvooe

aaronlehmann · 2016-09-06T16:53:25Z

registry/auth.go

+func ConvertToHostname(url string) string {
+	stripped := url
+	if strings.HasPrefix(url, "http://") {
+		stripped = strings.Replace(url, "http://", "", 1)


stripped = strings.TrimPrefix(url, "http://")

aaronlehmann · 2016-09-06T18:45:01Z

There are some build errors:

18:40:35 registry/auth.go:224: syntax error: unexpected ResolveAuthConfig, expecting (
18:40:35 registry/auth.go:249: syntax error: unexpected string at end of statement

But otherwise these changes look right, thanks.

runcom · 2016-09-06T18:56:50Z

@aaronlehmann should be fixed now

aaronlehmann · 2016-09-06T20:00:44Z

api/client/registry/logout.go

+		if _, ok := dockerCli.ConfigFile().AuthConfigs[s]; ok {
+			loggedIn = true
+			regToLogout = s
+			break


Wondering if we should log out from all of the registries from regsToTry which are found, in case someone ends up in a state where the same registry is stored in multiple ways. It would be very unexpected if docker logout only removed one copy but not the others.

Make sense, I'll fix this and add an integration test if possible.

Fixed and added an integration test, PTAL

aaronlehmann · 2016-09-06T22:46:27Z

api/client/registry/login.go

+	var (
+		serverAddress     string
+		isDefaultRegistry bool
+	)
 	if opts.serverAddress != "" {
 		serverAddress = opts.serverAddress


Shouldn't isDefaultRegistry be set to true if this matches IndexServer?

I'm not sure that ever worked, if you want I can add it

@aaronlehmann fixed

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

aaronlehmann · 2016-09-07T16:44:39Z

LGTM

Issue with newer docker versions if https is used via login causes push/pull commands to fail. Fixed slated for docker 1.13.0 - moby/moby#22910 - moby/moby#23100 (comment)

GordonTheTurtle added the status/0-triage label May 30, 2016

runcom added status/1-design-review and removed status/0-triage labels May 30, 2016

icecrime added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 1, 2016

runcom modified the milestone: 1.12.0 Jun 10, 2016

stevvooe reviewed Jul 2, 2016
View reviewed changes

runcom force-pushed the fix-stores branch from eeea443 to 56c26f3 Compare July 2, 2016 09:58

runcom mentioned this pull request Aug 28, 2016

CredsStore fails with private repositories #22910

Closed

runcom force-pushed the fix-stores branch 2 times, most recently from e3d12ba to 4a4e811 Compare September 3, 2016 16:23

runcom changed the title ~~[WIP] support legacy registries in exernal stores~~ support legacy registries in exernal stores Sep 3, 2016

runcom added the status/2-code-review label Sep 3, 2016

aaronlehmann reviewed Sep 6, 2016
View reviewed changes

runcom force-pushed the fix-stores branch 3 times, most recently from 7cc47cf to 008b06a Compare September 6, 2016 18:39

runcom force-pushed the fix-stores branch from 008b06a to 4086307 Compare September 6, 2016 18:56

runcom force-pushed the fix-stores branch 2 times, most recently from 013d2a2 to 7e499ee Compare September 6, 2016 19:08

aaronlehmann reviewed Sep 6, 2016
View reviewed changes

runcom force-pushed the fix-stores branch 2 times, most recently from 572f895 to 669d426 Compare September 6, 2016 21:17

aaronlehmann reviewed Sep 6, 2016
View reviewed changes

support legacy registries in exernal stores

67d752a

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

runcom force-pushed the fix-stores branch from 669d426 to 67d752a Compare September 7, 2016 07:45

aaronlehmann merged commit 86124ca into moby:master Sep 7, 2016

runcom deleted the fix-stores branch September 7, 2016 16:48

runcom mentioned this pull request Sep 20, 2016

Be benevolent to .docker/config.json file containers/image#96

Merged

MiguelMoll mentioned this pull request Nov 16, 2016

Add last line to docker error convox/rack#1397

Closed

runcom mentioned this pull request Jun 14, 2017

Protocol should not be stripped out of the registry URL on login docker/cli#190

Open

freesoft mentioned this pull request Jun 30, 2017

Pull from store/oracle/serverjre spotify/dockerfile-maven#16

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support legacy registries in exernal stores #23100

support legacy registries in exernal stores #23100

runcom commented May 30, 2016 •

edited

Loading

runcom commented May 31, 2016

thaJeztah commented May 31, 2016

stevvooe commented May 31, 2016

icecrime commented Jun 1, 2016 •

edited

Loading

aaronlehmann commented Jun 1, 2016

runcom commented Jun 10, 2016

calavera commented Jun 16, 2016

aaronlehmann commented Jun 16, 2016

thaJeztah commented Jul 1, 2016

stevvooe Jul 2, 2016

runcom Jul 2, 2016 •

edited

Loading

stevvooe Jul 5, 2016

stevvooe commented Jul 2, 2016

runcom commented Jul 2, 2016

thaJeztah commented Jul 14, 2016

runcom commented Jul 14, 2016

thaJeztah commented Jul 14, 2016

runcom commented Sep 1, 2016

aaronlehmann commented Sep 2, 2016

runcom commented Sep 2, 2016

vdemeester commented Sep 6, 2016

aaronlehmann Sep 6, 2016

aaronlehmann commented Sep 6, 2016

runcom commented Sep 6, 2016

aaronlehmann Sep 6, 2016

runcom Sep 6, 2016 •

edited

Loading

runcom Sep 6, 2016

aaronlehmann Sep 6, 2016

runcom Sep 7, 2016

runcom Sep 7, 2016

aaronlehmann commented Sep 7, 2016

support legacy registries in exernal stores #23100

support legacy registries in exernal stores #23100

Conversation

runcom commented May 30, 2016 • edited Loading

runcom commented May 31, 2016

thaJeztah commented May 31, 2016

stevvooe commented May 31, 2016

icecrime commented Jun 1, 2016 • edited Loading

aaronlehmann commented Jun 1, 2016

runcom commented Jun 10, 2016

calavera commented Jun 16, 2016

aaronlehmann commented Jun 16, 2016

thaJeztah commented Jul 1, 2016

Choose a reason for hiding this comment

runcom Jul 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Jul 2, 2016

runcom commented Jul 2, 2016

thaJeztah commented Jul 14, 2016

runcom commented Jul 14, 2016

thaJeztah commented Jul 14, 2016

runcom commented Sep 1, 2016

aaronlehmann commented Sep 2, 2016

runcom commented Sep 2, 2016

vdemeester commented Sep 6, 2016

Choose a reason for hiding this comment

aaronlehmann commented Sep 6, 2016

runcom commented Sep 6, 2016

Choose a reason for hiding this comment

runcom Sep 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Sep 7, 2016

runcom commented May 30, 2016 •

edited

Loading

icecrime commented Jun 1, 2016 •

edited

Loading

runcom Jul 2, 2016 •

edited

Loading

runcom Sep 6, 2016 •

edited

Loading