docker: fix unauthenticated pulls from gcr.io

[runcom]

./skopeo inspect gcr.io/google_containers/pause-amd64:3.0 currently fails while pulling from a gcr.io repository which requires auth is ok. This was my bad before the shutdown. This patch fixes both unauthenticated and authenticated pulls.

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

[runcom]

@mtrmac PTAL, this is breaking F25 atomic users https://bugzilla.redhat.com/show_bug.cgi?id=1409873

I'll update skopeo and docker once this is in.

[mtrmac]

Apart from tests failing, could you document what exactly is the behavior (e.g. HTTP request/response sequences) we are trying to support here? It is really not obvious from the code.

My best guess is that the server is returning a different WWW-Authenticate scheme on ping() and an actual request; but if so, it kinda seems that we would be better of actually implementing the Unimplemented: WWW-Authenticate Bearer replaced by %#v case ~in full, instead of blindly assuming that any non-StatusUnauthorized failure must be a request for a Basic auth.

[dustymabe]

hey all - would be great if we can streamline this so that we can possibly get this in for an F25 two week atomic release this week.

[runcom]

Tests failure seems unrelated @mtrmac, should be related to containers/storage#11

instead of blindly assuming that any non-StatusUnauthorized failure must be a request for a Basic auth.

I thought this would be the only case, hence I did this way. GCR registry is really weird though and does some really weird stuff (like you need the gcloud CLI to docker login with just a token and other stuff...). It's not fully similar to the upstream Docker registry and I thought special casing it would be far better now than hunting down every corner case we have with GCR. I couldn't find any reliable documentation on GCR API either so far.

[mtrmac]

It's not fully similar to the upstream Docker registry and I thought special casing it would be far better now than hunting down every corner case we have with GCR.

I don’t really have an opinion on that, my point is that it’s impossible to tell from the code what the special case is.

(I do admit I haven’t tried to set this up myself so far; I’m thinking that at least one of the code structure, comments, or test cases, should be clear enough so that this is not necessary. Otherwise, when it is undocumented and unknown what is the special case and the needed behavior, there is very high likelihood that anyone editing the code in the future will break the workaround because they will have no idea what it is or perhaps even that there is a workaround in there at all.)

[runcom]

Alright, I'll try to restructure that if statement and document what it does (I'm not fully sure what else I could do honestly)

[mtrmac]

Yeah, we don’t really have a testing infrastructure for dockerClient so far; please add a comment block showing something like what we expected to happen, what is happening instead, what we need to do to compensate. (And, if known, why is this deviating from the expectation.)

[mtrmac]

OK, adding debugging prints (headers abbreviated):

GET /v2/ HTTP/1.1
Host: gcr.io

→

HTTP/1.1 401 Unauthorized
Transfer-Encoding: chunked
Docker-Distribution-Api-Version: registry/2.0
Server: Docker Registry
Www-Authenticate: Bearer realm="https://gcr.io/v2/token",service="gcr.io"

45
{"errors":[{"code":"UNAUTHORIZED","message":"Unauthorized access."}]}
0

BUT subsequently

GET /v2/google_containers/pause-amd64/manifests/3.0 HTTP/1.1
Host: gcr.io
User-Agent: Go-http-client/1.1

→

HTTP/1.1 200 OK
Content-Type: application/vnd.docker.distribution.manifest.v1+prettyjws
Docker-Content-Digest: sha256:163ac025575b775d1c0f9bf0bdd0f086883171eb475b5068e7defa4ca9e76516

{
   "schemaVersion": 1,

…

I guess it makes sense (all of the server may require authentication, but an individual repository does not need it), though it does seem to be in violation of https://github.com/docker/distribution/blob/master/docs/spec/api.md#api-version-check .

Anyway, ultimately it means that this PR is necessary; most of the objections, if any, should be directed at #191, or perhaps dockerClient should not use the /v2/ ping to get expected auth parameters at all, and only interpret WWW-Authenticate from the actual API endpoints it needs to use (which would be a much bigger refactoring)

[mtrmac]

Is it really just StatusOK? What about StatusNotFound, StatusAccepted (both used in deleteImage and PutBlob), StatusCreated (PutBlob, Putmanifest)?

Or perhaps this should really be if res.StatusCode != http.StatusUnauthorized? I don’t readily have a reproducer for #191 so I don’t know which of the conditions should get us into the testReq2 path.

[runcom]

I believe status != 401 would be more appropriate (meaning, I don't have a clear vision on other API endpoints)

[runcom]

@mtrmac push forced with (I hope) better documentation on the code path (please feel free to reword it if it's not clear to you)

[mtrmac]

👍

Thanks! That is perfect.

(I will eventually get to #191 in my mail backlog and perhaps look into cleaning up the other path more, but this particular PR is great now.)

[runcom]

(I will eventually get to #191 in my mail backlog and perhaps look into cleaning up the other path more, but this particular PR is great now.)

thanks a lot @mtrmac (sorry if #191 slipped in w/o proper review, I guess it was my fault)

[mtrmac]

Nah, things were urgent and I wasn’t really reading mail. Don’t worry about it :) In the worst case, the #191 case will be broken sometime in the future and we will have an opportunity to revisit and clean up.

[runcom]

lgtm

tests failure shouldn't impact anyone except this repo's CI since skopeo and docker vendors containers/storage (that's failing)

[ncdc]

@runcom do you have an ETA on this + a new F25 docker build?

[ncdc]

I built this by hand and can confirm at least that gcr.io pulls work again

[runcom]

@ncdc I'm running Docker integration tests with this patch before committing this to Docker and rebuild in F25, shouldn't take that long (just some hours at this point, tomorrow morning at last if something goes wrong)

[runcom]

@ncdc @dustymabe https://bodhi.fedoraproject.org/updates/docker-1.12.5-4.git03508cc.fc25