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

Minor fixes in the docker-credentials form #1691

Merged
merged 3 commits into from
Apr 24, 2020
Merged

Conversation

andresmgot
Copy link
Contributor

@andresmgot andresmgot commented Apr 23, 2020

Description of the change

I learned the hard way that https://index.docker.io/v1 is not valid (it requires the trailing /), should we enforce that?

@absoludity
Copy link
Contributor

I learned the hard way that https://index.docker.io/v1 is not valid (it requires the trailing /), should we enforce that?

As in k8s won't use the secret for dockerhub if the trailing slash isn't included? If that's the case, yes, we may even want a radio allowing selecting between dockerhub and a custom one. Or do you mean kubeapps treated it differently (it shouldn't have).

@absoludity
Copy link
Contributor

I learned the hard way that https://index.docker.io/v1 is not valid (it requires the trailing /), should we enforce that?

As in k8s won't use the secret for dockerhub if the trailing slash isn't included? If that's the case, yes, we may even want a radio allowing selecting between dockerhub and a custom one. Or do you mean kubeapps treated it differently (it shouldn't have).

I'm not 100% but https://github.com/kubernetes/kubernetes/blob/master/pkg/credentialprovider/keyring.go#L106 woulds seem to strip off any /v1/ when adding the credential to the basic provider (haven't checked if that's used). But the way I read that code, it should also be fine to just use the domain "index.docker.io" but the docs say otherwise, so unsure.

I also found while playing today that I could only use the domain of my private registry for the image pull secret, it wouldn't work with the full URL with path etc. Still need to dig some more.

@andresmgot
Copy link
Contributor Author

andresmgot commented Apr 24, 2020

I have tested it a bit more but yes, as you point out, the paths /v1/ and /v2/ get specifically removed but it doesn't do that with /v1 or /v2. Then the docker auth cli is just interested in the domain AFAIK (https://github.com/moby/moby/blob/f6a5ccf492e8eab969ffad8404117806b4a15a35/registry/auth.go#L204). This is what I have tested:

domains ==> work

docker.io yes
index.docker.io yes
index.docker.io/v1 no
index.docker.io/v1/ yes
index.docker.io/v1/ yes
https://index.docker.io yes
https://index.docker.io/v1 no
https://index.docker.io/v1/ yes
https://index.docker.io/v2/ yes

The protocol is irrelevant and both domains docker.io and index.docker.io work. For the sake of simplicity I will leave the placeholder as docker.io.

@andresmgot andresmgot merged commit c3c5dd2 into master Apr 24, 2020
@absoludity
Copy link
Contributor

The protocol is irrelevant and both domains docker.io and index.docker.io work. For the sake of simplicity I will leave the placeholder as docker.io.

Careful, the fact that it works regardless of the protocol doesn't mean it's irrelevant. There may be some security reason, such as old docker clients using http (and sending the credentials over http) if the protocol is not specified. Or it could depend on the credential store used.

Since the k8s docks specifically say to use "https://index.docker.io/v1/", including the protocol (see https://kubernetes.io/docs/tasks/configure-pod-container/pull-image-private-registry/#create-a-secret-by-providing-credentials-on-the-command-line ) I think we should use it.

@andresmgot andresmgot deleted the blankSpaceCreds branch September 8, 2020 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants