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

docker client: fix requests made against bearer token services #480

Conversation

@flavio
Copy link
Contributor

@flavio flavio commented Jul 13, 2018

Previous to this commit the requests made against a bearer token service would be lacking the author field.That causes the jwt token to have an empty sub (Subject) field.

The docker registry relies on the sub field to know the author of all the requests. When a registry is configured to send notifications the resulting webhooks calls will be missing the author too.

This commit fixes the issue by reproducing the same request made by the docker client.

Some logs

In the next sections you can find some debugging logs obtained by running docker and skopeo against a docker registry configured to use Portus as bearer token.

docker client

This is the authentication request sent to the bearer token service:

Started GET "/v2/token?account=flavio&client_id=docker&offline_token=true&service=registry.kube.lan" for 127.0.0.1 at 2018-07-13 12:40:57 +0000
Processing by Api::V2::TokensController#show as JSON
  Parameters: {"account"=>"flavio", "client_id"=>"docker", "offline_token"=>"true", "service"=>"registry.kube.lan"}
[... snip ...]
[jwt_token] [claim] {:iss=>"portus.kube.lan", :sub=>"flavio", :aud=>"registry.kube.lan", :iat=>1531485657, :nbf=>1531485652, :exp=>1531485957, :jti=>"2xNhr4Rs2EXaK1iE6BHQbssLt9ewHwtAVZ4tdzvqWo"}
Completed 200 OK in 88ms (Views: 0.2ms | ActiveRecord: 16.0ms)

The GET request has account set to flavio, the same value is present into the final bearer token :sub=>"flavio".

The logs on the docker registry are something like:

time="2018-07-13T13:16:45.399284684Z" level=debug msg="filesystem.Stat(\"/docker/registry/v2/blobs/sha256/74/74f634b1bc1bd74535d5209589734efbd44a25f4e2dc96d78784576a3eb5b335/data\")" auth.user.name=flavio go.version=go1.9.4 http.request.c
ontenttype="application/vnd.docker.distribution.manifest.v2+json" http.request.host=registry.kube.lan http.request.id=f477cba8-5667-4ca8-880f-d1de128d90c4 http.request.method=PUT http.request.remoteaddr=192.168.100.1 http.request.uri="/v2
/busybox/manifests/latest" http.request.useragent="docker/17.09.1-ce go/go1.8.7 git-commit/f4ffd2511ce9 kernel/4.12.14-lp150.12.4-default os/linux arch/amd64 UpstreamClient(Docker-Client/17.09.1-ce \\(linux\\))" instance.id=fffee710-2fcb-
404f-ba86-facf3a046da8 trace.duration=24.81µs trace.file="/home/abuild/rpmbuild/BUILD/distribution-2.6.2/go/src/github.com/docker/distribution/registry/storage/driver/base/base.go" trace.func="github.com/docker/distribution/registry/stora
ge/driver/base.(*Base).Stat" trace.id=675cc50a-00e3-4869-aae9-0b27553106f7 trace.line=137 vars.name=busybox vars.reference=latest version="v2.6.2+unknown"   

As you can see auth.user.name is set to flavio.

skopeo - no patch applied

These logs are produced by running this command:

./skopeo copy --dest-creds flavio:password docker-archive:busy docker://registry.kube.lan/busybox:latest

The bearer token service receives this request from skopeo:

Started GET "/v2/token?scope=repository%3Abusybox%3Apull%2Cpush&service=registry.kube.lan" for 127.0.0.1 at 2018-07-13 12:39:55 +0000                                                                                                         
Processing by Api::V2::TokensController#show as JSON                                                                                                                                                                                          
  Parameters: {"scope"=>"repository:busybox:pull,push", "service"=>"registry.kube.lan"}                                                                                                                                                       
[... snip ...]
[jwt_token] [claim] {:iss=>"portus.kube.lan", :sub=>nil, :aud=>"registry.kube.lan", :iat=>1531485595, :nbf=>1531485590, :exp=>1531485895, :jti=>"BkXTjYbnisNnoYPd3GfshZX6EtJxWhDFigcDkCFLVi", :access=>[{:type=>"repository", :name=>"busybox"
, :actions=>["pull", "push"]}]}  

As you can see the GET request doesn't have the author field, the final jwt token as nil as sub.

Logs from a docker registry using bearer token authentication:

time="2018-07-13T12:33:36.62781205Z" level=debug msg="filesystem.Stat(\"/docker/registry/v2/blobs/sha256/a9/a9d04077e7698059d282d9e89a1676a045b2bd0b33d059b1dde77e80e613b5d3/data\")" auth.user.name= go.version=go1.9.4 http.request.contenttype="application/vnd.docker.distribution.manifest.v2+json" http.request.host=registry.kube.lan http.request.id=df0cf7ae-bd5f-4a4f-a260-253870fa9f2b http.request.method=PUT http.request.remoteaddr=192.168.100.1 http.request.uri="/v2/busybox/manifests/latest" http.request.useragent="Go-http-client/1.1" instance.id=0d684e18-1804-4fe4-b34d-b539548d1a3c trace.duration=36.103µs trace.file="/home/abuild/rpmbuild/BUILD/distribution-2.6.2/go/src/github.com/docker/distribution/registry/storage/driver/base/base.go" trace.func="github.com/docker/distribution/registry/storage/driver/base.(*Base).Stat" trace.id=66639253-abd0-4c72-b877-ced10be60cc8 trace.line=137 vars.name=busybox vars.reference=latest version="v2.6.2+unknown" 

As you can see the auth.user.name is empty.

skopeo - patched

I rebuilt skopeo with this patch applied. That solves the issue, the logs produced on the bearer token and on the registry are like the ones of the docker client.

Previous to this commit the requests made against a bearer token service
would be lacking the `author` field.
That causes the jwt token to have an empty `sub` (Subject) field. [1]

The docker registry relies on the `sub` field to know the author of all
the requests. When a registry is configured to send notifications [2]
the resulting webhooks calls will be missing the author too.

This commit fixes the issue by reproducing the same request made by
the docker client.

[1] https://docs.docker.com/registry/spec/auth/jwt/#getting-a-bearer-token
[2] https://docs.docker.com/registry/configuration/#notifications

Signed-off-by: Flavio Castelli <fcastelli@suse.com>
@runcom
Copy link
Member

@runcom runcom commented Jul 13, 2018

LGTM

Approved with PullApprove

@rhatdan
Copy link
Member

@rhatdan rhatdan commented Jul 13, 2018

@mtrmac PTAL

@mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Jul 13, 2018

Overall, sure, more compatibility with Docker is nice (although this is not in the formal spec at https://github.com/docker/distribution/blob/master/docs/spec/auth/token.md ).

OTOH, a server issuing a token with sub set to whatever the caller asks, independently of the authentication, seems very surprising. Is that intentional?

@runcom
Copy link
Member

@runcom runcom commented Jul 13, 2018

Overall, sure, more compatibility with Docker is nice (although this is not in the formal spec at https://github.com/docker/distribution/blob/master/docs/spec/auth/token.md ).

besides, it seems moby/moby is already doing that as well:

vendor/github.com/docker/distribution/registry/client/auth/session.go
426:                    reqParams.Add("account", username)

(I wasn't aware honestly)

@mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Jul 13, 2018

👍

besides, it seems moby/moby is already doing that as well:

That’s not a “besides”, that’s the only place I could find that the parameter is ever referenced in the current codebase (in particular, I could not find any consumer in the docker/* universe).

The value used to be documented before docker/distribution@fb481ef#diff-825a2678a7f56e92876b5f3813962a1bL135 (September 2015), but even then it was listed as optional and redundant.

As for a consumer, there is the referenced https://github.com/SUSE/Portus/blob/46bae5fdb488a02022ad5b0f1af59938e8942de2/app/controllers/api/v2/tokens_controller.rb#L31 in Portus, and I can’t see that the use of params[:account] is in any way connected to the user (API::Helpers::current_user) used for authorizing access to the relevant repositories. OTOH I don’t really know my way around Rails (in particular I find it very difficult to match references and definitions of anything), so this may be OK.

Overall, if this were a new protocol proposal, I would be pretty strongly against introducing another field which can be inconsistent with the actually authenticated data; but this is not a concern for the client we are not actually maintaining, while differing from moby/moby is a risk. So, merging.

Approved with PullApprove

@mtrmac mtrmac merged commit e68ba97 into containers:master Jul 13, 2018
2 checks passed
2 checks passed
code-review/pullapprove Approved by mtrmac, runcom
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@flavio flavio deleted the SUSE:send-account-field-to-authenticated-docker-registries branch Jul 16, 2018
@flavio
Copy link
Contributor Author

@flavio flavio commented Jul 16, 2018

Overall, if this were a new protocol proposal, I would be pretty strongly against introducing another field which can be inconsistent with the actually authenticated data; but this is not a concern for the client we are not actually maintaining, while differing from moby/moby is a risk. So, merging.

I agree with that. Thanks for having merged it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.