Support IdentityToken in registry authn #829

yihuaf · 2020-02-21T23:44:05Z

Adding the support for using identitytoken in the .docker/config.json
files. It's part of oauth2 and Azure Container Registry is one of the case that uses this.

Since containers/image implemented it's own docker registry client, we want to make it working with oauth2. The identitytoken can be used to get an access/bear token in place of password. Instead of setting basicAuth in the request, we follow the oauth2 definition to send a post request with grant_type.

Reference:

Docker registry oauth2 documentation: https://docs.docker.com/registry/spec/auth/oauth/
Azure Container Registry oauth2 documentation: https://github.com/Azure/acr/blob/7eddc1c5866ce5cc80e95df37d9d119e70d8c15e/docs/AAD-OAuth.md

Test: Tested with podman and logging into azure container registry, which uses identitytoken.

Concern: This patch changed the config.GetAuthentication interface, which is used by podman directly. Not sure if this is intended behaviour for podman to access GetAuthentication directly.

close #748
This is the base to fix these: containers/skopeo#533 and containers/podman#4357.

Signed-off-by: yihuaf fang.yihua.eric@gmail.com

mtrmac

Thanks! Just a quick first look.

docker/docker_client.go

pkg/docker/config/config.go

mtrmac · 2020-02-22T10:31:15Z

Concern: This patch changed the config.GetAuthentication interface, which is used by podman directly. Not sure if this is intended behaviour for podman to access GetAuthentication directly.

It is a public API, and most of the config subpackage exists for podman {login, logout}, so ti is intended in that sense. This PR should keep that code, unmodified, working for user name+password autentication. Podman might well want to add extra support for the identity token, and in that case it can migrate to a new API.

yihuaf · 2020-02-22T18:45:49Z

Concern: This patch changed the config.GetAuthentication interface, which is used by podman directly. Not sure if this is intended behaviour for podman to access GetAuthentication directly.

It is a public API, and most of the config subpackage exists for podman {login, logout}, so ti is intended in that sense. This PR should keep that code, unmodified, working for user name+password autentication. Podman might well want to add extra support for the identity token, and in that case it can migrate to a new API.

I can keep the "GetAuthentication" interface intact, and create a new interface for the bearer token code path. "GetAuthentication" can internally call the new function and only expose username and password. Any preference on the name of the new API? "GetAuthenticationDocker" or "GetAuthenticationOauth2"?

yihuaf · 2020-02-26T19:02:59Z

@mtrmac PTAL at the latest change. Addressed your comments.

rhatdan · 2020-03-03T18:08:59Z

@vrothberg PTAL

vrothberg

LGTM but I want @mtrmac to have another look.

I guess we need to support that in the kernel keyring once we enable support for it.

yihuaf · 2020-03-07T00:28:24Z

Force push to rebase to latest master.

mtrmac · 2020-03-07T03:08:05Z

I can keep the "GetAuthentication" interface intact, and create a new interface for the bearer token code path. "GetAuthentication" can internally call the new function and only expose username and password. Any preference on the name of the new API? "GetAuthenticationDocker" or "GetAuthenticationOauth2"?

(I know, bikeshedding…) GetAuthenticationStruct? It is not only for OAuth2. Or, maybe, I never really liked the “authentication” noun here - GetCredentials?

mtrmac

Thanks, and I apologize for the delay.

This is a full review now. Highlights:

The GetAuthenticationOAuth2 naming
Behavior of GetAuthentication with OAuth2 configuration
The account removal on GET token requests.

docker/docker_client.go

pkg/docker/config/config.go

pkg/docker/config/config_test.go

docker/docker_client.go

yihuaf · 2020-03-09T21:00:54Z

Addressed the review. Created a separate commit so it's easier to track the change. I can squash the commits together if needed. @mtrmac

yihuaf · 2020-03-10T23:01:50Z

Force push to rebase master.

vrothberg · 2020-03-11T18:13:58Z

I will do another review tomorrow (unless it's merged before :-)). But before merging, @yihuaf could you squash the commits into one?

yihuaf · 2020-03-11T18:22:53Z

I will do another review tomorrow (unless it's merged before :-)). But before merging, @yihuaf could you squash the commits into one?

I can certainly do that :)

mtrmac

Thanks for the update!

docker/docker_client.go

pkg/docker/config/config.go

pkg/docker/config/config_test.go

docker/docker_client.go

pkg/docker/config/config.go

mtrmac

One last nit.

docker/docker_client.go

pkg/docker/config/config_test.go

Adding the support for using identitytoken in the .docker/config.json files. Azure Container Registry is one of the case that uses this. Signed-off-by: yihuaf <fang.yihua.eric@gmail.com>

yihuaf · 2020-03-13T23:09:06Z

One last nit.

done.

mtrmac

LGTM. Thanks!

yihuaf force-pushed the dev/yihuaf/authfile branch from f435102 to 513fe8c Compare February 21, 2020 23:45

yihuaf changed the title ~~Support IdentityToken in registry authn~~ [WIP] Support IdentityToken in registry authn Feb 22, 2020

yihuaf force-pushed the dev/yihuaf/authfile branch 7 times, most recently from 357c295 to 78f0400 Compare February 22, 2020 06:09

mtrmac reviewed Feb 22, 2020

View reviewed changes

docker/docker_client.go Show resolved Hide resolved

docker/docker_client.go Outdated Show resolved Hide resolved

pkg/docker/config/config.go Outdated Show resolved Hide resolved

pkg/docker/config/config.go Outdated Show resolved Hide resolved

yihuaf force-pushed the dev/yihuaf/authfile branch from 78f0400 to 4e92f3a Compare February 24, 2020 19:55

yihuaf changed the title ~~[WIP] Support IdentityToken in registry authn~~ Support IdentityToken in registry authn Feb 24, 2020

yihuaf requested a review from mtrmac February 24, 2020 19:58

yihuaf force-pushed the dev/yihuaf/authfile branch from 4e92f3a to 02cce69 Compare February 26, 2020 19:02

vrothberg reviewed Mar 4, 2020

View reviewed changes

yihuaf force-pushed the dev/yihuaf/authfile branch 2 times, most recently from 3c6aa72 to 617ad75 Compare March 7, 2020 00:28

mtrmac reviewed Mar 7, 2020

View reviewed changes

mtrmac mentioned this pull request Mar 7, 2020

docker: Pass Bearer Token directly #689

Closed

yihuaf force-pushed the dev/yihuaf/authfile branch from 617ad75 to 3fa066e Compare March 9, 2020 20:58

yihuaf requested a review from mtrmac March 9, 2020 21:58

yihuaf force-pushed the dev/yihuaf/authfile branch from 3fa066e to be0e42f Compare March 10, 2020 23:01

yihuaf requested a review from vrothberg March 11, 2020 18:05

yihuaf force-pushed the dev/yihuaf/authfile branch from be0e42f to 8dfc0a0 Compare March 11, 2020 18:26

mtrmac reviewed Mar 12, 2020

View reviewed changes

vrothberg reviewed Mar 12, 2020

View reviewed changes

docker/docker_client.go Outdated Show resolved Hide resolved

docker/docker_client.go Outdated Show resolved Hide resolved

pkg/docker/config/config.go Outdated Show resolved Hide resolved

yihuaf force-pushed the dev/yihuaf/authfile branch 3 times, most recently from 7f0b781 to a77e2c2 Compare March 13, 2020 23:03

mtrmac reviewed Mar 13, 2020

View reviewed changes

docker/docker_client.go Outdated Show resolved Hide resolved

pkg/docker/config/config_test.go Outdated Show resolved Hide resolved

Support IdentityToken in registry authn

31d443d

Adding the support for using identitytoken in the .docker/config.json files. Azure Container Registry is one of the case that uses this. Signed-off-by: yihuaf <fang.yihua.eric@gmail.com>

yihuaf force-pushed the dev/yihuaf/authfile branch from a77e2c2 to 31d443d Compare March 13, 2020 23:08

yihuaf requested review from mtrmac and vrothberg March 13, 2020 23:09

mtrmac approved these changes Mar 13, 2020

View reviewed changes

mtrmac merged commit bf94267 into containers:master Mar 13, 2020

yihuaf deleted the dev/yihuaf/authfile branch March 14, 2020 01:03

yihuaf mentioned this pull request Mar 14, 2020

IdentityToken doesn't seem to work for auth containers/skopeo#533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support IdentityToken in registry authn #829

Support IdentityToken in registry authn #829

yihuaf commented Feb 21, 2020 •

edited

Loading

mtrmac left a comment

mtrmac commented Feb 22, 2020

yihuaf commented Feb 22, 2020

yihuaf commented Feb 26, 2020

rhatdan commented Mar 3, 2020

vrothberg left a comment

yihuaf commented Mar 7, 2020

mtrmac commented Mar 7, 2020

mtrmac left a comment

yihuaf commented Mar 9, 2020 •

edited

Loading

yihuaf commented Mar 10, 2020

vrothberg commented Mar 11, 2020

yihuaf commented Mar 11, 2020

mtrmac left a comment

mtrmac left a comment

yihuaf commented Mar 13, 2020

mtrmac left a comment

Support IdentityToken in registry authn #829

Support IdentityToken in registry authn #829

Conversation

yihuaf commented Feb 21, 2020 • edited Loading

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Feb 22, 2020

yihuaf commented Feb 22, 2020

yihuaf commented Feb 26, 2020

rhatdan commented Mar 3, 2020

vrothberg left a comment

Choose a reason for hiding this comment

yihuaf commented Mar 7, 2020

mtrmac commented Mar 7, 2020

mtrmac left a comment

Choose a reason for hiding this comment

yihuaf commented Mar 9, 2020 • edited Loading

yihuaf commented Mar 10, 2020

vrothberg commented Mar 11, 2020

yihuaf commented Mar 11, 2020

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

yihuaf commented Mar 13, 2020

mtrmac left a comment

Choose a reason for hiding this comment

yihuaf commented Feb 21, 2020 •

edited

Loading

yihuaf commented Mar 9, 2020 •

edited

Loading