Implement credsStore in credential config files #1179

matejvasek · 2021-03-15T20:20:17Z

Config file on macOS may contain "credsStore" property indicating what cred store should be used.
This patch checks for its presence and uses it if appropriate.

Signed-off-by: Matej Vasek mvasek@redhat.com

mtrmac · 2021-03-15T21:07:18Z

(Drive-by comment, by no means a review:)

This is not macOS specific
Read support without write support can have surprising behavior
See also add support for default credential store #737 (and Auth configs in registries.conf #971 ).

matejvasek · 2021-03-15T23:27:56Z

@QiWang19 does #971 fixes this?

matejvasek · 2021-03-17T16:59:39Z

@vrothberg do you know whether #971 fixes this?

QiWang19 · 2021-03-17T17:13:43Z

@matejvasek Current #971 plans to embed credential helper configurations into registries.conf. Not the same syntax as credsStore.
There were PRs for this credsStore, but didn't get a chance to be merged. #656

vrothberg · 2021-03-31T14:26:00Z

Would #1193 help?

lkingland · 2021-04-01T09:40:31Z

This seems to be working for me.

Given the following ~/.docker/config.json

{
  "HttpHeaders" : {
    "User-Agent" : "Docker-Client/19.03.12 (darwin)"
  },
  "credsStore" : "desktop",
  "auths" : {
    "https://index.docker.io/v1/" : {

    }
  },
  "experimental" : "enabled",
  "stackOrchestrator" : "swarm"
}

The following program:

package main

import (
        "fmt"
        "github.com/containers/image/v5/pkg/docker/config"
        "log"
)

func main() {
        dauth, err := config.GetAllCredentials(nil)
        if err != nil {
                log.Fatal(err)
        }
        fmt.Printf("dauths: %+v\n", dauth)
}

accesses credentials from the credsStore:

> go run main.go
dauths: map[index.docker.io:{Username:[REDACTED] Password:[REDACTED] IdentityToken:}]

lkingland · 2021-04-01T09:42:34Z

Would #1193 help?

Unfortunately, when trying with #1193, the Username and Password output from the aforementioned program are blank

rhuss · 2021-06-14T18:51:08Z

I have a similar issue, in that config.GetAllCredentials(nil) does not find my real passwords, but returns bogus (possibly stale) passwords. Even when I do a docker logout the passwords are still present.

I'm a bit puzzled as I checked all credentials helpers available (docker-credential-desktop, docker-credential-ecr-login, docker-credential-osxkeychain) and none has the bogus password that containers/images find.

My config.json looks the same:

{
	"auths": {
		"index.docker.io": {},
		"public.ecr.aws": {},
		"quay.io": {}
	},
	"credsStore": "desktop",
	"experimental": "enabled",
	"stackOrchestrator": "swarm"
}%

Any help is highly appreciated.

matejvasek · 2021-06-14T18:53:49Z

@vrothberg ^^^ any idea what could be issue for Roland?

mtrmac · 2021-06-14T19:08:56Z

@rhuss this seems to have nothing to do with this proposed PR, please file a separate issue.

rhuss · 2021-06-15T15:01:01Z

@rhuss this seems to have nothing to do with this proposed PR, please file a separate issue.

I added my comments here, as I think that is the issue for implementing support for credential helpers, right ? --> #111 (comment)

mtrmac · 2021-06-18T17:58:56Z

Would #1193 help?

Unfortunately, when trying with #1193, the Username and Password output from the aforementioned program are blank

#1193 requires the configuration to be present in a registries.conf option named credential-helpers, not in credsStore.

rhatdan · 2021-06-24T09:51:56Z

@matejvasek Are you still working on this?

matejvasek · 2021-07-07T12:25:00Z

@rhatdan for now I made a workaround in our SW. I could get back to this if there is no one else willing to work on this. For now my priority is to fix containers/podman#10776.

Employ "CredsStore" field of standard docker config. Signed-off-by: Matej Vasek <mvasek@redhat.com>

The test was reading "~/.docker/config" which can cause test failure. Signed-off-by: Matej Vasek <mvasek@redhat.com>

Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek · 2021-08-23T21:13:02Z

@mtrmac @vrothberg PTAL

Signed-off-by: Matej Vasek <mvasek@redhat.com>

mtrmac

Thanks.

The implementation looks reasonable reading each operation in isolation, but does this actually work end-to-end?

The user sets $XDG_RUNTIME_DIR/containers/auth.json to contain a credsStore; that works fine for a while, but is wiped on a reboot. Not good.
The user sets ~/.docker/config.json to contain a credsStore; that allows reading fine, and survives reboot, but the first modification (SetCredentials/RemoveAuthentication only reads auth.json) only reads …/auth.json, doesn’t see any credsStore entry in there, and happily writes credentials to …/auth.json. That’s not good either.

(Yes, this was already broken with credHelpers; in that case it’s more plausible to argue that the read-only path is good enough because the helper might point at a cloud-specific helper and credentials for that registry are never updated via this package. That’s not the case if this is intended to handle all credentials.

I realize that this is not an issue on macOS, where we don’t have $XDG_RUNTIME_DIR and the configuration is persistent; having a macOS-only feature is somewhat a stretch, having it available and badly broken on Linux is not really acceptable.

[That’s why we have ended up implementing the registries.conf helper configuration.])

@vrothberg PTAL independently, this is tricky and I could well be missing something.

mtrmac · 2021-08-23T21:58:52Z

pkg/docker/config/config.go

@@ -156,6 +160,13 @@ func GetAllCredentials(sys *types.SystemContext) (map[string]types.DockerAuthCon
 				for registry := range auths.AuthConfigs {
 					addRegistry(registry)
 				}
+				if auths.CredsStore != "" {
+					if creds, err := listAuthsFromCredHelper(auths.CredsStore); err == nil {


Why is it OK to completely ignore errors here? I’d expect this to work similarly to a helper returned by CredentialHelpers.

mtrmac · 2021-08-23T21:59:48Z

pkg/docker/config/config.go

@@ -440,6 +451,14 @@ func RemoveAllAuthentication(sys *types.SystemContext) error {
 				}
 				auths.CredHelpers = make(map[string]string)
 				auths.AuthConfigs = make(map[string]dockerAuthConfig)
+				if auths.CredsStore != "" {
+					if creds, err := listAuthsFromCredHelper(auths.CredsStore); err == nil {


Why is it OK to completely ignore errors here? I’d expect this to work similarly to a helper returned by CredentialHelpers.

mtrmac · 2021-08-23T22:00:08Z

pkg/docker/config/config.go

+				if auths.CredsStore != "" {
+					if creds, err := listAuthsFromCredHelper(auths.CredsStore); err == nil {
+						for registry := range creds {
+							_ = deleteAuthFromCredHelper(auths.CredsStore, registry)


mtrmac · 2021-08-23T22:02:26Z

pkg/docker/config/config.go

@@ -646,6 +665,12 @@ func findAuthentication(ref reference.Named, registry, path string, legacyFormat
 		return getAuthFromCredHelper(ch, registry)
 	}

+	if auths.CredsStore != "" {
+		if cred, err := getAuthFromCredHelper(auths.CredsStore, registry); err == nil {


Why is it OK to completely ignore errors here?

mtrmac · 2021-08-23T22:07:15Z

pkg/docker/config/config_test.go

@@ -559,6 +559,7 @@ func TestGetAuthFailsOnBadInput(t *testing.T) {
 }

 func TestGetAllCredentials(t *testing.T) {


Please add tests for the other modified functions as well.

Please add tests for the other modified functions as well.

Are those functions properly tested right now anyway? I believe they are not tested with respect to helpers right now.

I believe they are not tested with respect to helpers right now.

For instance the mock helper does noting for the store operation so you can really assert it written anything when calling the SetCredentials function.

mtrmac · 2021-08-23T22:08:42Z

pkg/docker/config/config_test.go

+		SystemRegistriesConfPath:    "",
+		SystemRegistriesConfDirPath: "",


This is (sadly?) the same as leaving the values unset, in particular it doesn’t disable any ordinary users’ configuration. So if this data matters, it should point at real files.

@mtrmac Do I need to set it? I think that for this test AuthFilePath is enough, isn't it?

mtrmac · 2021-08-23T22:10:28Z

pkg/docker/config/config_test.go

+	os.Setenv("HOME", dir)
+	return func() {
+		if oldHomeExists {
+			os.Setenv("HOME", oldHome)


I don’t immediately see that this is necessary (the existing GetAllCredentials test got by without), and anyway, AFAIK it doesn’t work; containers/storage/pkg/homedir.Get() caches the value throughout its lifetime, so a single-test override is either completely ineffective, or is effective only if that test happens to run first.

So, to make some of the code testable, we had to introduce getCredentialsWithHomeDir and I suspect something similar would be necessary in other cases that tried to override HOME.

@mtrmac without this the test is failing on my machine because the GetAllCredentials function also returns credentials from my docker config.

I you don't have $HOME/.docker/config.json on your machine it probably works. But for me it's failing.

caches the value throughout its lifetime, so a single-test override is either completely ineffective, or is effective only if that test happens to run first.

I don't what to do about this.

The test is touching not only testing config files but also actual user config files, that's not good.

mtrmac · 2021-08-23T22:16:40Z

pkg/docker/config/config.go

@@ -440,6 +451,14 @@ func RemoveAllAuthentication(sys *types.SystemContext) error {
 				}
 				auths.CredHelpers = make(map[string]string)
 				auths.AuthConfigs = make(map[string]dockerAuthConfig)
+				if auths.CredsStore != "" {


RemoveAuthentication also needs adding the support.

mtrmac · 2021-08-23T22:18:07Z

pkg/docker/config/config.go

@@ -85,6 +86,9 @@ func SetCredentials(sys *types.SystemContext, key, username, password string) (s
 					}
 					return false, setAuthToCredHelper(ch, key, username, password)
 				}
+				if auths.CredsStore != "" {
+					return false, setAuthToCredHelper(auths.CredsStore, key, username, password)
+				}


Please keep the order of handling CredHelpers/CredsStore/AuthConfigs consistent throughout the package, to make it easier to check that all operations handle the same data. (I guess in the order above, unless there’s a specific reason to do something else.)

vrothberg

Really just found a mini nit in addition to @mtrmac's comments.

vrothberg · 2021-08-24T08:03:20Z

pkg/docker/config/config.go

@@ -59,7 +60,7 @@ var (
 // Returns a human-redable description of the location that was updated.
 // NOTE: The return value is only intended to be read by humans; its form is not an API,
 // it may change (or new forms can be added) any time.
-func SetCredentials(sys *types.SystemContext, key, username, password string) (string, error) {
+func SetCredentials(sys *types.SystemContext,  key, username, password string) (string, error) {


Nit: not sure why the extra space is needed.

matejvasek · 2021-08-24T10:13:49Z

I realize that this is not an issue on macOS, where we don’t have $XDG_RUNTIME_DIR and the configuration is persistent; having a macOS-only feature is somewhat a stretch, having it available and badly broken on Linux is not really acceptable.

@mtrmac actually this is not just macOS specific, you can use credStore also on Linux (and Windows).

matejvasek · 2021-08-24T10:23:18Z

The user sets $XDG_RUNTIME_DIR/containers/auth.json to contain a credsStore; that works fine for a while, but is wiped on a reboot. Not good.

@mtrmac so user has to login after each reboot with podman?

matejvasek · 2021-08-24T10:32:14Z

The user sets $XDG_RUNTIME_DIR/containers/auth.json to contain a credsStore; that works fine for a while, but is wiped on a reboot. Not good.

Doesn't this also apply to already used credHelpers field?

matejvasek · 2021-08-24T10:44:14Z

Aren't most objections against credsStore also valid to already implemented credHelpers?

matejvasek · 2021-08-24T10:51:04Z

What about if we implemented only read only part?

The function didn't work as expected, it only worked if the test using it was run as the first test since HOME envvar is cached somewhere. Signed-off-by: Matej Vasek <mvasek@redhat.com>

Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek · 2021-09-13T16:04:56Z

@mtrmac What about if we implemented only read only part?

matejvasek · 2021-09-14T11:32:06Z

@mtrmac why is containers/auth.json not persistent?

Signed-off-by: Matej Vasek <mvasek@redhat.com>

rhuss mentioned this pull request Jun 15, 2021

docker credentials retrieval should check external stores (credsStore) #111

Open

matejvasek added 3 commits August 23, 2021 20:41

Use credstore form docker config

e12be70

Employ "CredsStore" field of standard docker config. Signed-off-by: Matej Vasek <mvasek@redhat.com>

Fix test reading non-testing config

1045cb0

The test was reading "~/.docker/config" which can cause test failure. Signed-off-by: Matej Vasek <mvasek@redhat.com>

Add test for credStore

f448235

Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek force-pushed the creds-store-fix branch from 1009379 to f448235 Compare August 23, 2021 21:11

Fix style

7ed9a4b

Signed-off-by: Matej Vasek <mvasek@redhat.com>

mtrmac changed the title ~~fix: use credsStore from config file on macOS~~ fix: use credsStore from config file Aug 23, 2021

mtrmac reviewed Aug 23, 2021

View reviewed changes

mtrmac changed the title ~~fix: use credsStore from config file~~ Implement credsStore in credential config files Aug 23, 2021

vrothberg reviewed Aug 24, 2021

View reviewed changes

matejvasek added 2 commits August 31, 2021 19:16

revert withTmpHome

ac05c2c

The function didn't work as expected, it only worked if the test using it was run as the first test since HOME envvar is cached somewhere. Signed-off-by: Matej Vasek <mvasek@redhat.com>

fixup

174d302

Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek force-pushed the creds-store-fix branch from f178089 to 05e5876 Compare September 14, 2021 11:55

add tests

7cac62b

Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek force-pushed the creds-store-fix branch from 05e5876 to 7cac62b Compare September 14, 2021 12:07

more error handling

e5c4f94

Signed-off-by: Matej Vasek <mvasek@redhat.com>

mtrmac added the kind/feature A request for, or a PR adding, new functionality label Dec 7, 2022

		@@ -559,6 +559,7 @@ func TestGetAuthFailsOnBadInput(t *testing.T) {
		}

		func TestGetAllCredentials(t *testing.T) {

		SystemRegistriesConfPath: "",
		SystemRegistriesConfDirPath: "",

Implement credsStore in credential config files #1179

Are you sure you want to change the base?

Implement credsStore in credential config files #1179

Conversation

matejvasek commented Mar 15, 2021

mtrmac commented Mar 15, 2021

matejvasek commented Mar 15, 2021

matejvasek commented Mar 17, 2021

QiWang19 commented Mar 17, 2021

vrothberg commented Mar 31, 2021

lkingland commented Apr 1, 2021

lkingland commented Apr 1, 2021 • edited

rhuss commented Jun 14, 2021

matejvasek commented Jun 14, 2021

mtrmac commented Jun 14, 2021

rhuss commented Jun 15, 2021

mtrmac commented Jun 18, 2021

rhatdan commented Jun 24, 2021

matejvasek commented Jul 7, 2021

matejvasek commented Aug 23, 2021

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejvasek Aug 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejvasek commented Aug 24, 2021

matejvasek commented Aug 24, 2021

matejvasek commented Aug 24, 2021

matejvasek commented Aug 24, 2021 • edited

matejvasek commented Aug 24, 2021

matejvasek commented Sep 13, 2021

matejvasek commented Sep 14, 2021

lkingland commented Apr 1, 2021 •

edited

matejvasek Aug 24, 2021 •

edited

matejvasek commented Aug 24, 2021 •

edited