types,docker: add DockerAuthConfig #113

runcom · 2016-10-01T09:26:53Z

The automatic credentials retrieval built into containers/image doesn't play at all with external credentials store. Furthermore, we need a way to pass credentials to containers/image from docker/docker (projectatomic/docker#200). This also fixes a bunch of docker's integration tests in projectatomic/docker#200.

This patch just gives precedence to any ctx.DockerAuthConfig field before diving deep into the system to lookup credentials into ~/.docker/config.json. So no real behavior change in skopeo so far. We'll need to tackle containers/container-libs#278, at which point we'll made the automatic retrieval optional as per #41 (comment).

Close #41
Close containers/skopeo#106

This is also needed by cri-o/cri-o#8 since there it's k8s itself to provide those credentials and we need to honor that (instead of looking on the system).

@mtrmac PTAL this is blocking projectatomic/docker#200 as said also.

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

runcom · 2016-10-02T15:34:48Z

LGTM

mtrmac

Nice, something like this is clearly needed.

mtrmac · 2016-10-03T15:50:41Z

types/types.go

 	DockerCertPath              string // If not "", a directory containing "cert.pem" and "key.pem" used when talking to a Docker Registry
 	DockerInsecureSkipTLSVerify bool   // Allow contacting docker registries over HTTP, or HTTPS with failed TLS verification. Note that this does not affect other TLS connections.
+	// if nil, the library tries to parse ~/.docker/config.json to retrieve credentials
+	// FIXME: the automatic credentials retrieval doesn't play with external stores also, see containers/image#111


This is unrelated to this field AFAICS; if we are already tracking this as an issue, no need to add a FIXME here.

mtrmac · 2016-10-03T15:53:39Z

docker/docker_client.go

+		if ctx.DockerAuthConfig.Auth != "" {
+			return decodeDockerAuth(ctx.DockerAuthConfig.Auth)
+		}
+		return ctx.DockerAuthConfig.Username, ctx.DockerAuthConfig.Password, nil


Why support both Auth and Username/Password here? Do we already have real users for both which would benefit? Then we need to document which overrides what, it just seems like unnecessary API complexity.

I’d like to have only the more explicit and type-safe username/password pair, if there were no other reason to prefer the Auth one or to supply both.

(It is not completely certain that we can always ignore the hostname here, but it is likely enough to be the common case; we can always add a DockerAuthConfigMap later, so this is fine.)

Ocid can provide username/password from k8s or skopeo can. K8s and OpenShift can just provide Auth.

K8s and OpenShift can just provide Auth.

AFAICT for access to the Docker endpoint for OpenShift, one would set {Username:"unused",Password:openShiftTokenString}, at least based on what ~/.kube/config is storing. Does the API really give the user the Docker base64 blob somewhere?

Alright, not sure about origin then, but that struct is exactly the same in k8s so it might happen k8s sends it that way

mtrmac · 2016-10-03T15:56:27Z

types/types.go

+	IdentityToken string
+	// RegistryToken is a bearer token to be sent to a registry
+	// FIXME: currently not used
+	RegistryToken string


I guess, drop the unused fields if we don’t know how we would be using them?

Especially the ServerAddress makes no sense to me considering the way getAuth is called.

(Do we even need an extra struct, or just more members in SystemContext?

Ocid is going to be an user of this whole struct as soon as containers/image is plumbed there.. This is somehow copied from that..

Then we need to also implement all of that?

I guess so, but we're far from fully doing that so this whole PR is just unblocking projectatomic/docker#200 while adding this struct witch is the same in docker and k8s api

So, grepping docker/docker:

RegistryToken is set nowhere in the code (but could be provided by a third-party client using the Remote API)

IdentityToken is refresh_token from https://github.com/docker/distribution/blob/master/docs/spec/auth/token.md , something we neither use nor generate (but we could, I suppose)

ServerAddress seems to be a data structure artifact (look how cliconfig/configfile/file.go:LoadFromReader overrides the value in the unmarshaled struct) and/or a semantic mix-up (registry/service.go uses AuthConfig.ServerAddress to look up registry hostnames)

Auth also an artifact of reusing the same structure for internal data and config storage, it is not really used anywhere, except cliconfig/configfile/file.go, which decodes the field and zeroes it.

Grepping openshift/origin:

No use of ReghistryToken, IdentityToken

AFAICS from a very imprecise grep the only real use is in vendor/k8s.io/kubernetes/pkg/credentialprovider/keyring.go , which only sets Username, Password, Email; I might have missed something.

mtrmac · 2016-10-03T15:58:10Z

types/types.go

+type DockerAuthConfig struct {
+	Username string
+	Password string
+	Auth     string


Auth, if we have it at all (see elsewhere) should be documented.

mtrmac · 2016-10-03T15:58:58Z

docker/docker_client.go


-func getAuth(hostname string) (string, string, error) {
+func getAuth(ctx *types.SystemContext, hostname string) (string, string, error) {
+	if ctx.DockerAuthConfig != nil {


This needs a ctx != nil &&

runcom · 2016-10-06T12:58:11Z

@mtrmac not sure where we're on this one. Would you have anything changed? I'd love to keep that struct as is right now since it reflects what's in k8s and since we didn't make any API stability promise we can always change it later.

mtrmac · 2016-10-06T13:10:07Z

I’d rather have a struct which contains only the fields we are implementing; and to add other fields later if and when we implement them, and only those which make sense. That way, we actually benefit from our ability to change the API later, not by adding fields just in case we might possibly use some of them in the future.

runcom · 2016-10-06T13:17:08Z

I’d rather have a struct which contains only the fields we are implementing; and to add other fields later if and when we implement them, and only those which make sense. That way, we actually benefit from our ability to change the API later, not by adding fields just in case we might possibly use some of them in the future.

ack, User, Password and Auth right? I'll remove the others

mtrmac · 2016-10-06T13:19:32Z

Not Auth; Auth is set to "" just after parsing the config.

… actually, one thing I forgot: apparently the Docker Remote API allows sending an arbitrary struct (see the RegistryToken comment); so it’s not obvious that it is tenable for Docker to just ignore some fields. I suppose something should fail if a field is unimplemented and ignored; whether that is docker/docker or this package then doesn’t matter that much.

runcom · 2016-10-06T13:23:50Z

Shall we leave that to docker/docker then?

mtrmac · 2016-10-06T14:20:36Z

I think so, that’s the API server. Unless k8s happens to do exactly the same thing which would be shared?

(Another minor point: If we are declaring a new struct in containers/image/types, the original docker/engine-api/types.AuthConfig is not directly assignable to ours anyway, is it? It needs to be a filed-by-field copy in any case.)

runcom · 2016-10-06T14:22:16Z

(Another minor point: If we are declaring a new struct in containers/image/types, the original docker/engine-api/types.AuthConfig is not directly assignable to ours anyway, is it? It needs to be a filed-by-field copy in any case.)

right, I will take care of that.

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

runcom · 2016-10-07T15:07:25Z

@mtrmac PTAL

mtrmac · 2016-10-07T15:52:53Z

Yeah, but tests are still broken after #115.

runcom · 2016-10-07T16:02:36Z

Fixing those

runcom · 2016-10-10T12:52:32Z

Yeah, but tests are still broken after #115.

tests fixed over there containers/skopeo#226

runcom · 2016-10-10T17:53:41Z

I'll make a skopeo PR for this change after I merge #115. Then I'll merge this one if you LGTM this as is (as I think you did?)

runcom · 2016-10-10T18:06:47Z

I've restarted the travis build here meanwhile..

runcom · 2016-10-10T18:33:46Z

tests green here

mtrmac · 2016-10-11T09:52:59Z

👍

runcom force-pushed the authConfig branch 4 times, most recently from 681a6d9 to d99691d Compare October 1, 2016 18:25

mtrmac requested changes Oct 3, 2016

View reviewed changes

types,docker: add DockerAuthConfig

98d6c00

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

runcom force-pushed the authConfig branch from d99691d to 98d6c00 Compare October 7, 2016 15:07

runcom mentioned this pull request Oct 10, 2016

vendor containers/image for DockerAuthConfig containers/skopeo#228

Merged

mtrmac approved these changes Oct 11, 2016

View reviewed changes

mtrmac merged commit 843ea02 into containers:master Oct 11, 2016

runcom deleted the authConfig branch October 11, 2016 10:03

types,docker: add DockerAuthConfig #113

types,docker: add DockerAuthConfig #113

Uh oh!

Conversation

runcom commented Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Oct 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

mtrmac Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Oct 6, 2016

Uh oh!

mtrmac commented Oct 6, 2016

Uh oh!

runcom commented Oct 6, 2016

Uh oh!

mtrmac commented Oct 6, 2016

Uh oh!

runcom commented Oct 6, 2016

Uh oh!

mtrmac commented Oct 6, 2016

Uh oh!

runcom commented Oct 6, 2016

Uh oh!

runcom commented Oct 7, 2016

Uh oh!

mtrmac commented Oct 7, 2016

Uh oh!

runcom commented Oct 7, 2016

Uh oh!

runcom commented Oct 10, 2016

Uh oh!

runcom commented Oct 10, 2016

Uh oh!

runcom commented Oct 10, 2016

Uh oh!

runcom commented Oct 10, 2016

Uh oh!

mtrmac commented Oct 11, 2016 • edited by runcom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Oct 1, 2016 •

edited

Loading

runcom commented Oct 2, 2016 •

edited

Loading

mtrmac Oct 3, 2016 •

edited

Loading

mtrmac Oct 3, 2016 •

edited

Loading

mtrmac Oct 3, 2016 •

edited

Loading

mtrmac commented Oct 11, 2016 •

edited by runcom

Loading