Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support legacy registries in exernal stores #23100

Merged
merged 1 commit into from
Sep 7, 2016

Conversation

runcom
Copy link
Member

@runcom runcom commented May 30, 2016

Tackling and fixing #22910

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

@runcom
Copy link
Member Author

runcom commented May 31, 2016

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

@thaJeztah
Copy link
Member

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

@stevvooe
Copy link
Contributor

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
Copy link
Contributor

icecrime commented Jun 1, 2016

Failure in gccgo and janky?

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

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

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
Copy link
Member Author

runcom commented Jun 10, 2016

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

@runcom runcom modified the milestone: 1.12.0 Jun 10, 2016
@calavera
Copy link
Contributor

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
Copy link
Contributor

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

@thaJeztah
Copy link
Member

ping @aaronlehmann @stevvooe what should we do here?

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

func (c *nativeStore) getLegacyServerAddress(serverAddress string) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@runcom runcom Jul 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To the code?

@stevvooe
Copy link
Contributor

stevvooe commented Jul 2, 2016

Add a comment and do a rebase.

@runcom
Copy link
Member Author

runcom commented Jul 2, 2016

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
Copy link
Member

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
Copy link
Member Author

runcom commented Jul 14, 2016

@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
Copy link
Member

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

@runcom
Copy link
Member Author

runcom commented Sep 1, 2016

@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
Copy link
Contributor

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
Copy link
Member Author

runcom commented Sep 2, 2016

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

@runcom runcom force-pushed the fix-stores branch 2 times, most recently from e3d12ba to 4a4e811 Compare September 3, 2016 16:23
@runcom runcom changed the title [WIP] support legacy registries in exernal stores support legacy registries in exernal stores Sep 3, 2016
@vdemeester
Copy link
Member

LGTM 🐸
/cc @aaronlehmann @stevvooe

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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
Copy link
Member Author

runcom commented Sep 6, 2016

@aaronlehmann should be fixed now

@runcom runcom force-pushed the fix-stores branch 2 times, most recently from 013d2a2 to 7e499ee Compare September 6, 2016 19:08
if _, ok := dockerCli.ConfigFile().AuthConfigs[s]; ok {
loggedIn = true
regToLogout = s
break
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@runcom runcom Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and added an integration test, PTAL

@runcom runcom force-pushed the fix-stores branch 2 times, most recently from 572f895 to 669d426 Compare September 6, 2016 21:17
var (
serverAddress string
isDefaultRegistry bool
)
if opts.serverAddress != "" {
serverAddress = opts.serverAddress
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@aaronlehmann
Copy link
Contributor

LGTM

@aaronlehmann aaronlehmann merged commit 86124ca into moby:master Sep 7, 2016
@runcom runcom deleted the fix-stores branch September 7, 2016 16:48
MiguelMoll added a commit to convox/rack that referenced this pull request Nov 16, 2016
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)
MiguelMoll added a commit to convox/rack that referenced this pull request Nov 16, 2016
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)
MiguelMoll added a commit to convox/rack that referenced this pull request Nov 16, 2016
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)
MiguelMoll added a commit to convox/rack that referenced this pull request Nov 17, 2016
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)
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.

9 participants