Implement login with private registry by mhennings · Pull Request #1564 · moby/moby

mhennings · 2013-08-16T14:16:58Z

To improve the use of docker with a private registry the login
command is extended with a parameter for the server address.

While implementing i noticed that two problems hindered authentication to a
private registry:

the resolve of the authentication did not match during push
because the looked up key was for example localhost:8080 but
the stored one would have been https://localhost:8080

Besides The lookup needs to still work if the https->http fallback
is used
During pull of an image no authentication is sent, which
means all repositories are expected to be private.

These points are fixed now. The changes are implemented in
a way to be compatible to existing behavior both in the
API as also with the private registry.

Update:

login does not require the full url any more, you can login
to the repository prefix:

example:
docker logon localhost:8080

Fixed corner corner cases:

When login is done during pull and push the registry endpoint is used and
not the central index
When Remote sends a 401 during pull, it is now correctly delegating to
CmdLogin
After a Login is done pull and push are using the newly entered login data,
and not the previous ones. This one seems to be also broken in master, too.
Auth config is now transfered in a parameter instead of the body when
/images/create is called.

mhennings · 2013-08-16T14:18:07Z

this fixes #1357

mhennings · 2013-08-16T14:18:11Z

@samalba please have a look

samalba · 2013-08-16T15:19:49Z

Thanks Marco for the contribution, it looks really good. I'll have a closer look today, especially to test it.

cc @shin-

avaerkazmer · 2013-08-18T06:30:59Z

Played around with this one at some length; looks like there's still a couple of warts:

login command doesn't hit the right url if you do the obvious thing like docker login 127.0.0.1:8080. It will complain to varying degrees until you login with the crazy format docker login http://127.0.0.1:8080/v1/. It shouldn't be that nasty.
If you're not logged in (in conf) when hitting a private registry for the first time (and it replies 401), the login will just prompt to login to the main index (and this isn't even obvious).

Basically, could use a bit more polishing + test cases.

But with the right incantations it seems to work.

mhennings · 2013-08-18T11:44:06Z

@avaer Thank you for pointing out theese corner cases. I've updated the commit to cover theses issues.

avaerkazmer · 2013-08-18T20:08:29Z

Cool, seems to be working as advertised now. Eagerly waiting for this in mainline :).

shin- · 2013-08-19T14:57:07Z

Looks good, I think it would also be useful to support this:

# docker login http://localhost:5000
2013/08/19 16:53:07 Invalid Registry endpoint: Get http://localhost:5000_ping: unknown port tcp/5000_ping

mhennings · 2013-08-19T20:42:59Z

@shin-

While i think that its better to support only well defined formats, I see that a user might run into that case.
So if a user comes now with a url without a path, it is now expanded to /v1/

crosbymichael · 2013-08-19T21:14:22Z

registry/registry.go

What is this magic number 9?

to test if the url has a path it is significant if it has a / after 'http://' or 'https://' which is the 8th char.

I personally think that doing tricks to urls is mostly wrong. So the implementation was originally only handling the two most significant cases:

Full URL

Prefix format

Though shin- has a point, that users might just do that and wonder.

mhennings · 2013-08-21T08:26:59Z

I updated the commit to rebase and fix a bug with the way I transfer the authConfig to the docker api.

mhennings · 2013-08-21T18:32:27Z

@shin- @samalba
i would appreciate if you could have another look at this.

shin- · 2013-08-22T13:37:54Z

Hey,

I think you're misisng an import in api.go :(

# go build
# github.com/dotcloud/docker
../../src/github.com/dotcloud/docker/api.go:400: undefined: base64

mhennings · 2013-08-22T13:45:10Z

@shin- thanks, made an error during rebase/merge. its fixed now.

shin- · 2013-08-22T14:12:41Z

Alright, LGTM 👍

vieux · 2013-08-22T17:25:52Z

@mhennings any particular reason to pass the authConfig via parameter in postImagesCreate ?
It's not consistent with postImagesPush

Maybe we could add parameter support in postImagesPush (but keep body undocumented to maintain compatibility)

mhennings · 2013-08-22T18:12:23Z

@vieux it had to be passed as a parameter because the same api endpoint is used for

docker import - < xyz.tar

If you like to move the authConfig on postImagePush into a parameter I can do that.

mhennings · 2013-08-22T18:51:17Z

@vieux Ok, ive moved the parameter

I've also rebased and made sure LoadConfig is called, as that PR went in in the meantime

mhennings · 2013-08-23T14:33:01Z

The last change you see here does two things:

a rebase to current master
when a image is pulled during the run cmd the correct auth is sent to the server.

To make it easer to keep track of these small additons i've started to put commits on top

crosbymichael · 2013-08-26T20:13:33Z

api.go

Why are you base64 encoding entire JSON object? Why would you not pass authentication data via the Authentication header?

I'm not sure having the authConfig data in an Authentication header is the best idea -- the header implies that you're authenticating to the API itself. Here, we're passing auth data that will be used by the API further down the line to perform certain operations, so I like the idea of having it as a parameter better.

@shin- Is there a good reason to base64 the entire json obj?

Encoding the full object is the way it was done before and i do not see a reason to change that.
The only difference is that it is now a parameter because it would conflict as the body.

if i replace that with a pseudo basic auth i would create new problems

not all relevant parameters are transfered (server address is needed)

future changes to enable stronger auth would be restricted without need

the code becomes incompatible to current api

users could expect full basic auth behaviour which is not intended at these routes

encoding as base64 is required to be url safe

@mhennings I don't get why you say Encoding the full object is the way it was done before,
you added "encoding/base64" yourself, before it was simply sent has body.

@vieux

It was not encoded as base64 but sent as a full json object in the body of the request of the "push" part.
There was no authentication during pull.

I understood the question to be regarding sending a full object instead of a basic auth header.

the base64 is fine AFAIC. URL-encoding a JSON dict would get ugly really fast.

mhennings · 2013-08-27T21:32:49Z

@crosbymichael @shin- @vieux

If there are questions open i would be happy if we could resolve them.

If not -- maybe we could merge this?

Kind regards,

Marco

kyleconroy · 2013-08-28T14:23:57Z

I'm a bit confused on why you need the "server address" field in the authConfig. Isn't this redundant information? ~/.dockercfg already stores the server address as the key to authConfig object.

mhennings · 2013-08-28T14:49:06Z

@kyleconroy
The ~/.dockerconfig contains the Server Address in the section. And this remains unchanged. But when doing a login request to the docker deamon the client process needs to submit the server to login to to the docker daemon.

This is because the ~/.dockerconfig file is read by the client process and not the docker daemon.

When doing a pull or push the server address is taken from the repository prefix (so this also remains as it is now).

Hope this helps understanding the changes.

mhennings · 2013-08-30T21:22:53Z

The http header changes of @shin- are merged, and the changeset was rebased. (the history is long, because i did a full cherry-pick to ease the rebase)

vieux · 2013-08-30T21:30:06Z

@shin- I think you should keep support for body in postImagesPush (and maybe remove it in the next version 1.5) or update this PR to version API 1.5.

It's a big change, it break compat.

shin- · 2013-08-30T21:47:37Z

Yep, I was thinking about that. Let's bump the API version to 1.5 and include this change + #1701

vieux · 2013-08-30T21:48:22Z

Great

crosbymichael · 2013-09-03T18:24:48Z

@mhennings Can you please rebase your changes?

To improve the use of docker with a private registry the login command is extended with a parameter for the server address. While implementing i noticed that two problems hindered authentication to a private registry: 1. the resolve of the authentication did not match during push because the looked up key was for example localhost:8080 but the stored one would have been https://localhost:8080 Besides The lookup needs to still work if the https->http fallback is used 2. During pull of an image no authentication is sent, which means all repositories are expected to be private. These points are fixed now. The changes are implemented in a way to be compatible to existing behavior both in the API as also with the private registry. Update: - login does not require the full url any more, you can login to the repository prefix: example: docker logon localhost:8080 Fixed corner corner cases: - When login is done during pull and push the registry endpoint is used and not the central index - When Remote sends a 401 during pull, it is now correctly delegating to CmdLogin - After a Login is done pull and push are using the newly entered login data, and not the previous ones. This one seems to be also broken in master, too. - Auth config is now transfered in a parameter instead of the body when /images/create is called.

…he run cmd.

mhennings · 2013-09-03T19:13:21Z

@crosbymichael

Done. There where some minor merges. I have tested that it still works.

aslakhellesoy · 2013-09-10T08:23:44Z

@mhennings does this change mean I can run my own docker-registry and force people to authenticate for both pull and push? (I want people to authenticate when pulling images from my registry).

mhennings · 2013-09-10T08:30:05Z

@aslakhellesoy
Yes, pull and push can use authentication.

Please note that you need to setup basic auth on the server side, too.
(for example by using a proxy)

lukewpatterson · 2013-09-28T06:18:24Z

@mhennings, you said to _aslakhellesoy - "Yes, pull and push can use authentication. ... you need to setup basic auth on the server side, too."

Have you successfully authenticated and pushed/pulled using basic auth? Your changes only enabled a custom index for auth, right? I'm asking because I've tried and tried using basic auth, with no luck. Please see also docker-archive/docker-registry#82 (comment)

Does anyone have a full example of how to use basic auth with a private registry? That'd be awesome.

mhennings · 2013-10-08T21:53:37Z

@lukewpatterson in theory it can be done by a proxy the handles basic auth.
but some parts of the code expect a auth token to be supplied by the /auth call.

after thinking about it it seems more realistic to implement it inside the open sourced registry.

crosbymichael reviewed Aug 19, 2013
View reviewed changes

crosbymichael reviewed Aug 26, 2013
View reviewed changes

keeb-zz mentioned this pull request Aug 31, 2013

Improve port mapping information in the API #1701

Merged

samalba mentioned this pull request Sep 3, 2013

Central index auth is sent to any index #1764

Closed

mhennings and others added 6 commits September 3, 2013 20:59

Move authConfig to a Parameter on postImagePush, too

da3bb9a

Send corrent endpoint authentication when an image is pulled during t…

ad322d7

…he run cmd.

Merge on current master

a260347

Pass auth config through headers rather than as URL param

d04beb7

Use base64 encoding

dd4aab8

Move auth header on run cmd

ded9732

This was referenced Sep 4, 2013

Hosting on Dotcloud with Basic Auth Enabled docker-archive/docker-registry#71

Closed

Bumped API version to 1.5 #1796

Merged

vieux merged commit ded9732 into moby:master Sep 9, 2013

mhennings deleted the 1357-implement-login-with-private-registry branch September 10, 2013 08:30

mhennings mentioned this pull request Sep 10, 2013

Usage of a private index server not possible any more due to removal of DOCKER_INDEX_URL, #1357

Closed

jlhawn mentioned this pull request Jan 30, 2014

Prevent Tests from creating users on Prod Index #3832

Merged

thaJeztah mentioned this pull request Apr 30, 2023

pkg/jsonmessage: JSONMessage.Display(): remove special case for 401 errors #45442

Merged

thaJeztah mentioned this pull request Jul 10, 2025

daemon/server: remove compatibility with API v1.4 auth-config on push #50371

Merged

Conversation

mhennings commented Aug 16, 2013

Uh oh!

mhennings commented Aug 16, 2013

Uh oh!

mhennings commented Aug 16, 2013

Uh oh!

samalba commented Aug 16, 2013

Uh oh!

avaerkazmer commented Aug 18, 2013

Uh oh!

mhennings commented Aug 18, 2013

Uh oh!

avaerkazmer commented Aug 18, 2013

Uh oh!

shin- commented Aug 19, 2013

Uh oh!

mhennings commented Aug 19, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhennings commented Aug 21, 2013

Uh oh!

mhennings commented Aug 21, 2013

Uh oh!

shin- commented Aug 22, 2013

Uh oh!

mhennings commented Aug 22, 2013

Uh oh!

shin- commented Aug 22, 2013

Uh oh!

vieux commented Aug 22, 2013

Uh oh!

mhennings commented Aug 22, 2013

Uh oh!

mhennings commented Aug 22, 2013

Uh oh!

mhennings commented Aug 23, 2013

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!

mhennings commented Aug 27, 2013

Uh oh!

kyleconroy commented Aug 28, 2013

Uh oh!

mhennings commented Aug 28, 2013

Uh oh!

mhennings commented Aug 30, 2013

Uh oh!

vieux commented Aug 30, 2013

Uh oh!

shin- commented Aug 30, 2013

Uh oh!

vieux commented Aug 30, 2013

Uh oh!

crosbymichael commented Sep 3, 2013

Uh oh!

mhennings commented Sep 3, 2013

Uh oh!

aslakhellesoy commented Sep 10, 2013

Uh oh!

mhennings commented Sep 10, 2013

Uh oh!

lukewpatterson commented Sep 28, 2013

Uh oh!

mhennings commented Oct 8, 2013