docker: mimic docker upstream registry authentication #211

runcom · 2017-01-17T18:08:17Z

Signed-off-by: Antonio Murdaca [email protected]

runcom · 2017-01-17T18:22:02Z

@mtrmac tested this out for gcr.io issues as well and it's fine hopefully...

mtrmac · 2017-01-17T18:24:01Z

docker/docker_client.go

+	ping := url.URL{
+		Host:   req.URL.Host,
+		Scheme: req.URL.Scheme,
+		Path:   "/v2/",


How is this, at the high level, supposed to help with #191 / #195 ? We still use the WWW-Authenticate value from /v2/, only now there is a lot more infrastructure around storing and retrieving the value.

we don't make the extra call to retrieve the header - which is likely fixing https://bugzilla.redhat.com/show_bug.cgi?id=1413987 also (I'm contacting the op to test this also)

@mtrmac bottom line is, this is taken directly from docker (well, the way not the actual code). I believe if it works there, it'll work here as well. And again, this is removing a problematic request that we were making when setting up authentication to the registries.

funny thing is that #191 actually fixed https://bugzilla.redhat.com/show_bug.cgi?id=1413987 but then #195 broke it again :)

mtrmac · 2017-01-17T18:28:34Z

@mtrmac tested this out for gcr.io issues as well and it's fine hopefully...

From a quick look I can't see how this is fixing the "we don’t know what authentication will be necessary for a specific URL in advance” issue of #191 / #195 ; AFAICT all this PR ultimately does, compared to the pre-#191 state, is that for getting Bearer tokens, we now hard-code in the client the Scope value instead of getting it from the registry.

It seems that I am overlooking something critical about this PR; what is it?

runcom · 2017-01-17T18:29:38Z

It seems that I am overlooking something critical about this PR; what is it?

this is not doing an extra request to check for authentication but just mimic what docker does (and gcr works with docker also)

mtrmac · 2017-01-17T18:54:15Z

docker/docker_client.go

@@ -446,8 +416,10 @@ func (c *dockerClient) ping() (*pingResponse, error) {
 		if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusUnauthorized {
 			return nil, errors.Errorf("error pinging repository, response code %d", resp.StatusCode)
 		}
+		chs := parseAuthHeader(resp.Header)
+		c.saveChallenges(chs, resp.Request.URL)


This is the only saveChallenges call everywhere ever; if that’s all that is needed, we can go back to a single dockerClient.wwwAuthenticate and have the code noticeably simpler (no map, no URL normalization)

baude · 2017-01-17T19:00:03Z

/me watches too

runcom · 2017-01-18T09:30:39Z

To add more context and debug, @mtrmac I have a skopeo branch with c/image pre-#191 + debug #201:

$ ./skopeo inspect docker://gcr.io/test-registry-153313/busybox

ERRO[0000] ===REQ===
GET /v2/ HTTP/1.1
Host: gcr.io
User-Agent: Go-http-client/1.1
Docker-Distribution-Api-Version: registry/2.0
Accept-Encoding: gzip


===REQ===

ERRO[0000] ===RES===
HTTP/1.1 401 Unauthorized
Connection: close
Transfer-Encoding: chunked
Alt-Svc: quic=":443"; ma=2592000; v="35,34"
Cache-Control: private
Content-Type: application/json
Date: Wed, 18 Jan 2017 09:20:28 GMT
Docker-Distribution-Api-Version: registry/2.0
Server: Docker Registry
Www-Authenticate: Bearer realm="https://gcr.io/v2/token",service="gcr.io"
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 1; mode=block


===RES===

ERRO[0000] ===REQ===
GET /v2/test-registry-153313/busybox/manifests/latest HTTP/1.1
Host: gcr.io
User-Agent: Go-http-client/1.1
Accept: application/vnd.oci.image.manifest.v1+json
Accept: application/vnd.docker.distribution.manifest.v2+json
Accept: application/vnd.docker.distribution.manifest.v1+prettyjws
Accept: application/vnd.docker.distribution.manifest.v1+json
Accept: application/vnd.docker.distribution.manifest.list.v2+json

Docker-Distribution-Api-Version: registry/2.0
Accept-Encoding: gzip


===REQ===

ERRO[0000] ===RES===
HTTP/1.1 401 Unauthorized
Connection: close
Transfer-Encoding: chunked
Alt-Svc: quic=":443"; ma=2592000; v="35,34"
Cache-Control: private
Content-Type: application/json
Date: Wed, 18 Jan 2017 09:20:29 GMT
Docker-Distribution-Api-Version: registry/2.0
Server: Docker Registry
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 1; mode=block


===RES===

ERRO[0000] ===REQ===
GET /v2/test-registry-153313/busybox/manifests/latest HTTP/1.1
Host: gcr.io
User-Agent: Go-http-client/1.1
Accept: application/vnd.oci.image.manifest.v1+json
Accept: application/vnd.docker.distribution.manifest.v2+json
Accept: application/vnd.docker.distribution.manifest.v1+prettyjws
Accept: application/vnd.docker.distribution.manifest.v1+json
Accept: application/vnd.docker.distribution.manifest.list.v2+json
Docker-Distribution-Api-Version: registry/2.0
Accept-Encoding: gzip


===REQ===

ERRO[0001] ===RES===
HTTP/1.1 401 Unauthorized
Connection: close
Transfer-Encoding: chunked
Alt-Svc: quic=":443"; ma=2592000; v="35,34"
Cache-Control: private
Content-Type: application/json
Date: Wed, 18 Jan 2017 09:20:29 GMT
Docker-Distribution-Api-Version: registry/2.0
Server: Docker Registry
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 1; mode=block


===RES===

FATA[0001] unauthorized: Permission denied for "latest" from request "/v2/test-registry-153313/busybox/manifests/latest".

@mtrmac it seems clear that the call to manifest w/o authentication (which we avoid with this PR) is returning an invalid response 401 which doesn't contain the WWW-Authenticate header, thus no challenges, and in pre-#191 that meant no auth needed (as shown by this if https://github.com/containers/image/pull/191/files#diff-ea0cc6f6c1d1908aa2c017e7bc817c61R267).

There's no doubt gcr.io is violating the registry API by not returning the www-authenticate header. We can't do anything about it. The fix for this would be this PR which just re-use the www-authenticate header from /v2/ (gcr.io is not violating this at least...).
The question "why #191 needed Basic auth to avoid that 401 on "manifest""? I don't know...maybe it works like that but nowhere is documented that that's the thing to do in that situation. docker/docker behaves as we're behaving in this PR (thus, I'm inclined to say this is the right fix for #191, avoiding calling "manifest" w/o credentials).

runcom · 2017-01-18T09:38:53Z

@mtrmac as for #195, here's the request stack (c/image w/o #195 + #201 for debugging):

$ ./skopeo inspect docker://gcr.io/google_containers/pause-amd64:3.0

ERRO[0000] ===REQ===
GET /v2/ HTTP/1.1
Host: gcr.io
User-Agent: Go-http-client/1.1
Docker-Distribution-Api-Version: registry/2.0
Accept-Encoding: gzip


===REQ===

ERRO[0000] ===RES===
HTTP/1.1 401 Unauthorized
Connection: close
Transfer-Encoding: chunked
Alt-Svc: quic=":443"; ma=2592000; v="35,34"
Cache-Control: private
Content-Type: application/json
Date: Wed, 18 Jan 2017 09:36:30 GMT
Docker-Distribution-Api-Version: registry/2.0
Server: Docker Registry
Www-Authenticate: Bearer realm="https://gcr.io/v2/token",service="gcr.io"
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 1; mode=block


===RES===

ERRO[0000] ===REQ===
GET /v2/google_containers/pause-amd64/manifests/3.0 HTTP/1.1
Host: gcr.io
User-Agent: Go-http-client/1.1
Accept: application/vnd.oci.image.manifest.v1+json
Accept: application/vnd.docker.distribution.manifest.v2+json
Accept: application/vnd.docker.distribution.manifest.v1+prettyjws
Accept: application/vnd.docker.distribution.manifest.v1+json
Accept: application/vnd.docker.distribution.manifest.list.v2+json
Docker-Distribution-Api-Version: registry/2.0
Accept-Encoding: gzip


===REQ===

ERRO[0000] ===RES===
HTTP/1.1 200 OK
Connection: close
Content-Length: 4589
Alt-Svc: quic=":443"; ma=2592000; v="35,34"
Content-Type: application/vnd.docker.distribution.manifest.v1+prettyjws
Date: Wed, 18 Jan 2017 09:36:30 GMT
Docker-Content-Digest: sha256:163ac025575b775d1c0f9bf0bdd0f086883171eb475b5068e7defa4ca9e76516
Docker-Distribution-Api-Version: registry/2.0
Server: Docker Registry
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 1; mode=block


===RES===

ERRO[0000] ===REQ===
GET /v2/google_containers/pause-amd64/manifests/3.0 HTTP/1.1
Host: gcr.io
User-Agent: Go-http-client/1.1
Accept: application/vnd.oci.image.manifest.v1+json
Accept: application/vnd.docker.distribution.manifest.v2+json
Accept: application/vnd.docker.distribution.manifest.v1+prettyjws
Accept: application/vnd.docker.distribution.manifest.v1+json
Accept: application/vnd.docker.distribution.manifest.list.v2+json
Authorization: Basic Og==
Docker-Distribution-Api-Version: registry/2.0
Accept-Encoding: gzip


===REQ===

ERRO[0001] ===RES===
HTTP/1.1 401 Unauthorized
Connection: close
Transfer-Encoding: chunked
Alt-Svc: quic=":443"; ma=2592000; v="35,34"
Cache-Control: private
Content-Type: application/json
Date: Wed, 18 Jan 2017 09:36:30 GMT
Docker-Distribution-Api-Version: registry/2.0
Server: Docker Registry
Www-Authenticate: Basic realm=""
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 1; mode=block


===RES===

FATA[0001] Unimplemented: WWW-Authenticate Bearer replaced by "basic"

I believe this is just another case of "gcr.io behaves really wrong...". Full stop.

runcom · 2017-01-18T09:39:40Z

Note that both issues go away with this PR (#211). I'm leaning towards behaving like docker/docker which didn't have this sort of issues with gcr.io (since gcr.io apparently respects /v2/ with WWW-Authenticate):

$ ./skopeo inspect docker://gcr.io/google_containers/pause-amd64:3.0
{
    "Name": "gcr.io/google_containers/pause-amd64",
    "Tag": "3.0",
    "Digest": "sha256:163ac025575b775d1c0f9bf0bdd0f086883171eb475b5068e7defa4ca9e76516",
    "RepoTags": [
        "3.0"
    ],
    "Created": "2016-05-04T06:26:41.522308365Z",
    "DockerVersion": "1.9.1",
    "Labels": {},
    "Architecture": "amd64",
    "Os": "linux",
    "Layers": [
        "sha256:a3ed95caeb02ffe68cdd9fd84406680ae93d633cb16422d00e8a7c22955b46d4",
        "sha256:f112334343777b75be77ec1f835e3bbbe7d7bd46e27b6a2ae35c6b3cfea0987c",
        "sha256:a3ed95caeb02ffe68cdd9fd84406680ae93d633cb16422d00e8a7c22955b46d4"
    ]
}
$ ./skopeo inspect docker://gcr.io/test-registry-153313/busybox
{
    "Name": "gcr.io/test-registry-153313/busybox",
    "Digest": "sha256:1ded4559e2aab2ab3464aae5170ef64afc15bea324a0861b543e2c56a3f29711",
    "RepoTags": [
        "latest"
    ],
    "Created": "2016-10-07T21:03:58.469866982Z",
    "DockerVersion": "1.12.1",
    "Labels": {},
    "Architecture": "amd64",
    "Os": "linux",
    "Layers": [
        "sha256:00ab491ee9d7fb52a25a99cb6f35db6b25750ded7303c5398dd6aeee054e0616"
    ]
}

runcom · 2017-01-19T17:23:24Z

Rebased, @mtrmac would you take a look at this?

runcom · 2017-01-19T19:26:49Z

AHEM.....what's that CI error?! Tests are running just fine on my laptop

mtrmac · 2017-01-19T20:30:33Z

AHEM.....what's that CI error?! Tests are running just fine on my laptop

distribution/distribution@6170ac5 (+ distribution/distribution@dbc336e ). Looking into that now.

On the positive note, we may be able to get rid of containers/image/docker/reference, again.

runcom · 2017-01-20T08:49:56Z

rebased again, should be good to review now

mtrmac

ACK on the general approach. Feel free to merge if this is urgent; if not I will do a detailed review later today or on Monday.

mtrmac · 2017-01-20T08:57:37Z

docker/wwwauthenticate.go

+	}
+	return addr
+}
+


None of the additions in this file are used.

mtrmac · 2017-01-20T08:58:26Z

docker/docker_client.go

+type requestOptions struct {
+	remoteName string
+	actions    string
+}


requestOptions feels rather generic. Call this authScope, or, perhaps preferably, just use 2 string parameters?

authScope sounds fine, will change it.

on a second though, makeRequest and makeRequestToResolvedURL already take too many param IMO. I would rather compact those options (and others) in a generic requestOptions struct. Mandatory arguments continue to be passed as function arguments though.

runcom · 2017-01-20T11:48:15Z

@mtrmac I've reworked a bit makeRequest[ToResolvedURL], PTAL

mtrmac · 2017-01-20T20:41:15Z

docker/docker_client.go

+type authScope struct {
+	remoteName string
+	actions    string
+}


Actually, looking at this more, the authScope is fixed for the life of a dockerClient, and remoteName can be computed from a dockerReference; so can we set the scope in newDockerClient and drop it from makeRequest* entirely? Sorry for not noticing this earlier.

Would then having the requestOptions still be worthwhile?

mtrmac · 2017-01-20T20:42:32Z

docker/docker_client.go

+	scope   authScope
+	headers map[string][]string
+	stream  io.Reader
+}


I don’t really feel strongly about introducing this… having named optional parameters is nice, I suppose. Though if we end up having 2 or 3, it is not a big deal.

mtrmac · 2017-01-20T20:43:50Z

docker/docker_client.go

 		c.scheme = pr.scheme
 	}

 	url = fmt.Sprintf(baseURL, c.scheme, c.registry) + url
-	return c.makeRequestToResolvedURL(method, url, headers, stream, -1, true)
+	return c.makeRequestToResolvedURL(method, url, opts, -1, true)
 }

 // makeRequestToResolvedURL creates and executes a http.Request with the specified parameters, adding authentication and TLS options for the Docker client.
 // streamLen, if not -1, specifies the length of the data expected on stream.


If we do requestOptions, streamLen should be probably in there as well (but then every caller with a non-nil stream will have to take care to set streamLen to -1).

mtrmac · 2017-01-20T20:48:25Z

docker/docker_client.go

@@ -446,8 +399,9 @@ func (c *dockerClient) ping() (*pingResponse, error) {
 		if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusUnauthorized {
 			return nil, errors.Errorf("error pinging repository, response code %d", resp.StatusCode)
 		}
+		c.challenges = parseAuthHeader(resp.Header)


This feels unclean; other values are only set via the pr structure, and c.challenges differs for no obvious reason.

(We could probably get rid of pingResponse altogether, have the nested function return a (*http.Response, scheme string, error), and move the resp.Header.Get() calls and the data transfers which now happen at the top of makeRequest to the successful termination path of ping. Not sure whether that would be the cleanest way, it’s just one of many options. Just adding challenges to pingResponse would also work. Or dropping pingReponse and making all the c. assignments in here.)

mtrmac · 2017-01-20T20:50:39Z

docker/docker_image_dest.go

@@ -150,7 +164,8 @@ func (d *dockerImageDestination) PutBlob(stream io.Reader, inputInfo types.BlobI
 	// TODO: check inputInfo.Digest == computedDigest https://github.com/containers/image/pull/70#discussion_r77646717
 	locationQuery.Set("digest", computedDigest.String())
 	uploadLocation.RawQuery = locationQuery.Encode()
-	res, err = d.c.makeRequestToResolvedURL("PUT", uploadLocation.String(), map[string][]string{"Content-Type": {"application/octet-stream"}}, nil, -1, true)
+	opts.stream = nil


(This reuse of opts and having to correct it works but it’s also a bit ugly; it means having to think about the state of opts, instead of just using it as a single-shot named parameters.)

mtrmac · 2017-01-20T20:52:15Z

docker/docker_image_src.go

+		},
+		headers: headers,
+	}
+	res, err := s.c.makeRequest("GET", url, opts)


I wouldn’t be opposed to (but won’t insist on) inlining this:

res, err := s.c.makeRequeset("GET", url, requestOptions{ … })

(and the same throughout, if scope is left anywhere.)

sorry, I meant “… (and the same throughout, if opts is left after we possibly remove scope from it.)"

mtrmac · 2017-01-20T20:57:20Z

docker/docker_client.go

-			// code is not 401.
-			// We just skip this case as it's not standard on docker/distribution
-			// registries (https://github.com/docker/distribution/blob/master/docs/spec/api.md#api-version-check)
-			if res.StatusCode != http.StatusUnauthorized {


Some similarly explicit comments should remain in this method, explaining at least that we intentionally use the WWW-Authenticate values from /v2/ and not from the destination URL, which we completely ignore, because 1) that’s what docker does and 2) because gcr.io is sending 401-without-WWW-Authenticate in the real requests, and pointing to dumps in this PR.).

runcom · 2017-01-24T13:20:24Z

@mtrmac PTAL (given tests fail because we're missing #223)

Layer compression

mtrmac

Thanks!

Two minor nits, but feel free to merge as is.

LGTM pending tests.

mtrmac · 2017-01-24T14:50:49Z

docker/docker_client.go

+	scope         authScope
+}
+
+type authScope struct {


Consider inlining the fields into dockerClient directly, but this is perfectly fine as well.

mtrmac · 2017-01-24T14:52:24Z

docker/docker_client.go

-	if c.wwwAuthenticate != "" && sendAuth {
-		if err := c.setupRequestAuth(req); err != nil {
+	if sendAuth {
+		if err := c.setupRequestAuth(req, c.scope.remoteName, c.scope.actions); err != nil {


c.scope* does not have to be a parameter, the method has direct access to c.

Yeah, overlooked this

runcom · 2017-01-25T14:00:11Z

this should be good to go as well, giving I'm not sure if we prefer getting #223 in before

mtrmac · 2017-01-25T14:15:08Z

👍, but I’d prefer waiting on #223 if that can be done reasonably quickly.

Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2017-01-29T20:36:44Z

@mtrmac should be OK to pull this in now.

runcom · 2017-01-30T15:38:54Z

👍

A registry is expected to provide a scope value when proposing a challenge. If it fails to do so, it is possible to infer it from the user request. Currently, Podman and Docker do this because they use the challenge provided by a "ping" request to fetch a Bearer token, when possible. This was done in Podman just to allow compatibility with gcr.io. See [1] for reference. [1] containers/image#211 Signed-off-by: Athos Ribeiro <[email protected]>

runcom mentioned this pull request Jan 17, 2017

docker: fixup requests authentication #210

Closed

runcom force-pushed the fix-registry-auth branch from 43aa484 to faebe39 Compare January 17, 2017 18:09

mtrmac reviewed Jan 17, 2017

View reviewed changes

runcom force-pushed the fix-registry-auth branch from faebe39 to 4e410de Compare January 18, 2017 09:15

runcom force-pushed the fix-registry-auth branch from 4e410de to f1ea4b4 Compare January 19, 2017 17:23

runcom force-pushed the fix-registry-auth branch from f1ea4b4 to 4e59d5f Compare January 20, 2017 08:49

mtrmac reviewed Jan 20, 2017

View reviewed changes

runcom force-pushed the fix-registry-auth branch 3 times, most recently from eca4631 to 1e168f0 Compare January 20, 2017 11:47

mtrmac reviewed Jan 20, 2017

View reviewed changes

runcom force-pushed the fix-registry-auth branch 2 times, most recently from a5852be to dbf8fec Compare January 24, 2017 13:19

giuseppe pushed a commit to giuseppe/image that referenced this pull request Jan 24, 2017

Merge pull request containers#211 from mtrmac/compress

98bfef9

Layer compression

mtrmac approved these changes Jan 24, 2017

View reviewed changes

runcom force-pushed the fix-registry-auth branch from dbf8fec to 3716a78 Compare January 25, 2017 13:59

docker: mimic docker upstream registry authentication

35b00eb

Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the fix-registry-auth branch from 3716a78 to 35b00eb Compare January 29, 2017 18:35

runcom merged commit d23efe9 into containers:master Jan 30, 2017

runcom deleted the fix-registry-auth branch January 30, 2017 15:39

mtrmac mentioned this pull request May 10, 2017

Document and possibly clean up gcr.io authentication support #200

Closed

mtrmac mentioned this pull request Aug 10, 2018

Fix requested token actions when deleting images #487

Merged

dongsupark mentioned this pull request Nov 6, 2018

docker: export functions and variables for a distribution-spec cert tests #527

Closed

ezekg mentioned this pull request Jan 24, 2025

docker transport does not respect per-repo authn challenges if /v2/ does not return 401 #2754

Open

docker: mimic docker upstream registry authentication #211

docker: mimic docker upstream registry authentication #211

Uh oh!

Conversation

runcom commented Jan 17, 2017

Uh oh!

runcom commented Jan 17, 2017

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!

mtrmac commented Jan 17, 2017

Uh oh!

runcom commented Jan 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baude commented Jan 17, 2017

Uh oh!

runcom commented Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Jan 18, 2017

Uh oh!

runcom commented Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Jan 19, 2017

Uh oh!

runcom commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Jan 19, 2017

Uh oh!

runcom commented Jan 20, 2017

Uh oh!

mtrmac left a comment

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!

runcom commented Jan 20, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Jan 24, 2017

Uh oh!

mtrmac left a comment

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

runcom commented Jan 18, 2017 •

edited

Loading

runcom commented Jan 18, 2017 •

edited

Loading

runcom commented Jan 19, 2017 •

edited

Loading

mtrmac commented Jan 25, 2017 •

edited by runcom

Loading

runcom commented Jan 30, 2017 •

edited

Loading