Add token pass-thru for AuthConfig #17741

dhiltgen · 2015-11-06T00:07:39Z

This change allows API clients to retrieve an authentication token from
a registry, and then pass that token directly to the API.

Example usage:

REPO_USER=dhiltgen
read -s PASSWORD
REPO=privateorg/repo
AUTH_URL=https://auth.docker.io/token
TOKEN=$(curl -s -u "${REPO_USER}:${PASSWORD}" "${AUTH_URL}?scope=repository:${REPO}:pull&service=registry.docker.io" |
    jq -r ".token")

HEADER=$(echo "{\"registrytoken\":\"${TOKEN}\"}"|base64 -w 0 )
curl -s -D - -H "X-Registry-Auth: ${HEADER}" -X POST "http://localhost:2376/images/create?fromImage=${REPO}"

Signed-off-by: Daniel Hiltgen daniel.hiltgen@docker.com

calavera · 2015-11-06T23:50:17Z

@docker/security what do you think about this?

NathanMcCauley · 2015-11-07T07:32:10Z

This makes a lot of sense to me. Design LGTM.

calavera · 2015-11-09T18:24:34Z

Token sounds like a very generic name to me. Should we use something more specific, like RegistryToken or AuthToken? I'm open to suggestions, and to keep it like this if other people agree.

NathanMcCauley · 2015-11-09T19:16:41Z

I like RegistryToken in order to distinguish it from AuthToken which will be confused with #15365 and #17412.

dhiltgen · 2015-11-10T15:57:11Z

Name change sounds fine to me. If nobody else speaks up, I'll update this and the related PRs to make the change.

LK4D4 · 2015-11-14T00:49:38Z

LGTM

calavera · 2015-11-17T09:10:49Z

Is there a way to test this?

PS: We definitely need documentation before merging it 😸

dhiltgen · 2015-11-19T21:29:31Z

Unit test and API level documentation added. I wasn't sure if there was someplace better to doc it, since AuthConfig wasn't really described in any of the docs I found.

dhiltgen · 2015-11-19T21:36:51Z

Not sure what to make of the CI build failures - janky seems to be a GPG key download failure. The windows one is a bit harder to make heads or tails of, but I don't see anything related to my change in the log.

dhiltgen · 2015-11-24T22:40:14Z

Hmm... looks like the graph package just went poof - so I need to figure out where this logic needs to shift to.

aaronlehmann · 2015-11-24T22:48:08Z

This code moved to the distribution package.

thaJeztah · 2015-11-24T22:48:15Z

@dhiltgen yup, huge PR was just merged for content addressability; #17924, apologies for that ☺️

dhiltgen · 2015-11-24T23:38:57Z

Thanks all - I've migrated my change over to the new layout, and am just running some final tests now to make sure nothing broke in the transition - I'll update the PR shortly.

dhiltgen · 2015-11-24T23:59:56Z

Updated based on new layout.

calavera · 2015-11-25T17:08:15Z

LGTM

thaJeztah · 2015-11-25T17:23:04Z

not in docs review yet, but can you also add a note to the API changelog; https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api.md#v122-api-changes

abronan · 2015-12-02T18:55:49Z

LGTM (but needs a rebase, sorry @dhiltgen)

thaJeztah · 2015-12-02T19:10:43Z

docs/reference/api/docker_remote_api_v1.22.md

+
+    {
+        "registrytoken": "9cbaf023786cd7..."
+    }


the code examples probably need to be indented more (4 spaces) to show up as code blocks, see the rendered page;

This change allows API clients to retrieve an authentication token from a registry, and then pass that token directly to the API. Example usage: REPO_USER=dhiltgen read -s PASSWORD REPO=privateorg/repo AUTH_URL=https://auth.docker.io/token TOKEN=$(curl -s -u "${REPO_USER}:${PASSWORD}" "${AUTH_URL}?scope=repository:${REPO}:pull&service=registry.docker.io" | jq -r ".token") HEADER=$(echo "{\"registrytoken\":\"${TOKEN}\"}"|base64 -w 0 ) curl -s -D - -H "X-Registry-Auth: ${HEADER}" -X POST "http://localhost:2376/images/create?fromImage=${REPO}" Signed-off-by: Daniel Hiltgen <daniel.hiltgen@docker.com>

dhiltgen · 2015-12-03T19:42:18Z

Rebased and got markdown to cooperate for the code blocks. (additional indentation didn't seem to work so I resorted to the triple-back-tick)

thaJeztah · 2015-12-04T13:00:06Z

Thanks, @dhiltgen, markdown can be a PITA with whitespace and lists. I ran make docs on your PR and it rendered correctly now \o/

docs LGTM

ping @jamtur01 @SvenDowideit @moxiegirl for review

moxiegirl · 2015-12-06T15:24:26Z

LGTM

Add token pass-thru for AuthConfig

thaJeztah · 2015-12-06T15:40:17Z

Thanks @dhiltgen!

vdemeester · 2015-12-06T15:40:33Z

🎉

pdevine · 2015-12-07T21:49:18Z

🎆

stevvooe · 2016-01-20T00:13:03Z

A little disappointing that this wasn't reviewed by anyone from the distribution team.

There are few issues:

Tokens are typically treated as short-lived. They can be long-lived, but most of the fallback flow doesn't assume this.
How will this handle authentication failures that require the resolution of further permissions? There are protocols in V2 (using 401 status codes) that can instruct the client to request further permissions.

thaJeztah · 2016-01-20T22:17:43Z

@stevvooe please don't comment on closed tickets

In your opinion, do we need changes to this PR before 1.10? If so, I can open an issue for that. Apologies that distribution didn't fully review this

stevvooe · 2016-01-20T22:34:46Z

@stevvooe please don't comment on closed tickets

This is one of the few cases where it's acceptable. 🐹

In your opinion, do we need changes to this PR before 1.10? If so, I can open an issue for that. Apologies that distribution didn't fully review this

I have no clue. We have no proposal showing why this is needed, other solutions considered, how this will interoperate with the existing push/pull flow or how we can support this going into the future.

I'd like to have seen a more expressive approach that includes exchanging a token on garant for a registry token. This would also have solved the unencrypted credentials issue. The difference being we can issue tokens that have require a higher level of protection and thus can have a longer expiry.

stevvooe · 2016-01-21T03:29:52Z

distribution/registry.go

+}
+
+func (th *existingTokenHandler) AuthorizeRequest(req *http.Request, params map[string]string) error {
+	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", th.token))


After some thought, we may need to restrict the token to a particular host. Otherwise, we risk credential leak upon redirect. Also, tokens should also be included only on https requests, unless otherwise specified.

For example, this will send the token to s3, leaking it to AWS.

GordonTheTurtle added area/distribution status/0-triage labels Nov 6, 2015

This was referenced Nov 6, 2015

Add token pass-thru for AuthConfig samalba/dockerclient#184

Merged

Add token pass-thru for Authconfig docker-archive/classicswarm#1384

Merged

vdemeester added status/1-design-review and removed status/0-triage labels Nov 6, 2015

calavera added status/2-code-review and removed status/1-design-review labels Nov 9, 2015

dhiltgen force-pushed the pull_token branch from e9ae20f to 0212cd8 Compare November 11, 2015 15:57

dhiltgen force-pushed the pull_token branch 4 times, most recently from 7fdfb90 to 1f8a78e Compare November 19, 2015 21:25

dhiltgen force-pushed the pull_token branch from 1f8a78e to 9b4b4fb Compare November 24, 2015 23:57

tonistiigi removed the status/2-code-review label Nov 30, 2015

calavera added impact/api impact/changelog labels Nov 30, 2015

thaJeztah reviewed Dec 2, 2015
View reviewed changes

dhiltgen force-pushed the pull_token branch 5 times, most recently from 48d2907 to 4491b31 Compare December 3, 2015 19:39

dhiltgen force-pushed the pull_token branch from 4491b31 to 8dce8e9 Compare December 3, 2015 19:40

moxiegirl added the status/4-merge label Dec 6, 2015

thaJeztah added a commit that referenced this pull request Dec 6, 2015

Merge pull request #17741 from dhiltgen/pull_token

715f6a1

Add token pass-thru for AuthConfig

thaJeztah merged commit 715f6a1 into moby:master Dec 6, 2015

thaJeztah added this to the 1.10.0 milestone Jan 15, 2016

vikstrous mentioned this pull request Jan 16, 2016

Bump version to v1.10.0 #19370

Merged

stevvooe reviewed Jan 21, 2016
View reviewed changes

icecrime removed the status/3-docs-review label Jan 22, 2016

lukeck mentioned this pull request Jan 27, 2016

Incorrect changelog issue ref #19776

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add token pass-thru for AuthConfig #17741

Add token pass-thru for AuthConfig #17741

dhiltgen commented Nov 6, 2015

calavera commented Nov 6, 2015

NathanMcCauley commented Nov 7, 2015

calavera commented Nov 9, 2015

NathanMcCauley commented Nov 9, 2015

dhiltgen commented Nov 10, 2015

LK4D4 commented Nov 14, 2015

calavera commented Nov 17, 2015

dhiltgen commented Nov 19, 2015

dhiltgen commented Nov 19, 2015

dhiltgen commented Nov 24, 2015

aaronlehmann commented Nov 24, 2015

thaJeztah commented Nov 24, 2015

dhiltgen commented Nov 24, 2015

dhiltgen commented Nov 24, 2015

calavera commented Nov 25, 2015

thaJeztah commented Nov 25, 2015

abronan commented Dec 2, 2015

thaJeztah Dec 2, 2015

dhiltgen commented Dec 3, 2015

thaJeztah commented Dec 4, 2015

moxiegirl commented Dec 6, 2015

thaJeztah commented Dec 6, 2015

vdemeester commented Dec 6, 2015

pdevine commented Dec 7, 2015

stevvooe commented Jan 20, 2016

thaJeztah commented Jan 20, 2016

stevvooe commented Jan 20, 2016

stevvooe Jan 21, 2016

Add token pass-thru for AuthConfig #17741

Add token pass-thru for AuthConfig #17741

Conversation

dhiltgen commented Nov 6, 2015

calavera commented Nov 6, 2015

NathanMcCauley commented Nov 7, 2015

calavera commented Nov 9, 2015

NathanMcCauley commented Nov 9, 2015

dhiltgen commented Nov 10, 2015

LK4D4 commented Nov 14, 2015

calavera commented Nov 17, 2015

dhiltgen commented Nov 19, 2015

dhiltgen commented Nov 19, 2015

dhiltgen commented Nov 24, 2015

aaronlehmann commented Nov 24, 2015

thaJeztah commented Nov 24, 2015

dhiltgen commented Nov 24, 2015

dhiltgen commented Nov 24, 2015

calavera commented Nov 25, 2015

thaJeztah commented Nov 25, 2015

abronan commented Dec 2, 2015

thaJeztah Dec 2, 2015

Choose a reason for hiding this comment

dhiltgen commented Dec 3, 2015

thaJeztah commented Dec 4, 2015

moxiegirl commented Dec 6, 2015

thaJeztah commented Dec 6, 2015

vdemeester commented Dec 6, 2015

pdevine commented Dec 7, 2015

stevvooe commented Jan 20, 2016

thaJeztah commented Jan 20, 2016

stevvooe commented Jan 20, 2016

stevvooe Jan 21, 2016

Choose a reason for hiding this comment