docker credentials retrieval should check external stores (credsStore) about image HOT 11 OPEN

@mtrmac in reply to #111 (comment) - is this something you'd consider accepting a PR for?

(support for credsStore); sure, that would be great.

Modifying [dockerConfigFile]
…
plus some shuffling in the func findAuthentication(registry, path string, legacyFormat bool) (string, string, error) seemed like a reasonable place to start
Yes, plus SetAuthentication/RemoveAuthentication/RemoveAllAuthentication; after one works, the rest should be straightforward. credsStore and credHelpers AFAICS use the same protocol.

• but it looks like some refactoring of associated unit tests would be required as well.

AFAICS the pkg/docker/config subpackage does not have any unit tests for this yet (sadly), so this should not be a pressing problem. Of course new tests would be also very welcome :)

from image.

What is the status of this issue, which I consider a severe limitation of this library if used outside the context of podman/buildah for talking with a registry?

There have been several attempts to fix this over the last five years, but none made it:

• #656
• #971
• #1179

But still, if you are e.g. on macOs and don't have the podman/buildah specific way for dealing with registry, the 'docker-standardized` way of dealing with auth is not supported yet, although the implementation looks kind of trivial.

You can argue that supporting Docker's credential helper mechanism might be out of scope, but that would greatly reduce value for this library. Although, you should then also not support any config formats like ~/.docker/config.json)

Or you should support the full way how Docker deals with registry authentication when using docker login. I.e. having a a ~/.docker/config.json like

{
	"auths": {
		"https://index.docker.io/v1/": {}
	},
	"credsStore": "desktop",
	"experimental": "enabled",
	"stackOrchestrator": "swarm"
}

and you have stored the credentials in docker-credential-desktop (check with docker-credential-desktop list), then your credential should be picked up from the store and not falsely assumed to be empty because of the empty-valued auths map, which for Docker is just an indicator to which registry you are logged in.

Any of the previous PR would solve this issue (I assume), so can please someone clarifies why they are not picked up (with some non-zero priority ?)

from image.

fwiw, a related issue is that the old Docker config location ~/.dockercfg takes precedence over the newer one at ~/.docker/config.json. The order should be reversed.

File a separate issue, please. It’s going to be lost here.

from image.

#339 added support for credHelpers; we do not support credsStore yet.

from image.

@mtrmac in reply to #111 (comment) - is this something you'd consider accepting a PR for?

When Docker-for-Mac is installed the ~/.docker/config.json file contains something similar to the following:

{
        "auths": {
        },
        "HttpHeaders": {
                "User-Agent": "Docker-Client/18.09.0-ce-beta1 (darwin)"
        },
        "credsStore": "osxkeychain"
}

which means the docker-credential-osxkeychain credential helper is used - at least by the docker cli. That isn't the case when containers/image loads ~/.docker/config.json - which I think can lead to confusion when a Mac user tries to use skopeo with their osxkeychain (at least it confused me).

Modifying dockerConfigFile to look something like this:

type dockerConfigFile struct {
	AuthConfigs map[string]dockerAuthConfig `json:"auths"`
	CredHelpers map[string]string           `json:"credHelpers,omitempty"`
	CredsStore  string                      `json:"credsStore,omitempty"`
}

plus some shuffling in the func findAuthentication(registry, path string, legacyFormat bool) (string, string, error) seemed like a reasonable place to start - but it looks like some refactoring of associated unit tests would be required as well.

Can you think of a different way to handle this - one that you'd prefer before I look for a way to make the tests pass? If I understand correctly vendoring more of the Docker cli is less than ideal (which is completely understandable).

from image.

AFAICS the pkg/docker/config subpackage does not have any unit tests for this yet (sadly), so this should not be a pressing problem.

There have been some pkg/docker/config unit tests hiding over in docker/docker_client_test.go since #96. I'm moving them into pkg/docker/config in #588 and restructuring a bit to decouple the auth-getting implementation from SystemContext (although of course it won't actually be decoupled until we can drop the properties I'm deprecating in #588 ;).

from image.

@wking @mtrmac @vrothberg This looks like it is still being worked on?

from image.

I don't think that's something being worked on but leave the decision of closing or not to @mtrmac.

from image.

This is a valid feature request (and I don’t know about anybody working on it either).

from image.

fwiw, a related issue is that the old Docker config location ~/.dockercfg takes precedence over the newer one at ~/.docker/config.json. The order should be reversed.

from image.

please someone clarifies why they are not picked up (with some non-zero priority ?)

There’s fairly extensive discussion in those PRs. I’m not going to summarize all of this here, sorry, that’s basically equivalent to actually solving those concerns.

from image.