Auth configs in registries.conf #971

QiWang19 · 2020-06-24T20:07:33Z

Allow credential-helpers = [] and credential-helper = "" configuration in registries.conf as defautl store for credentials.

credential-helpers = ["secretservice"] # general credential helpers
[[registry]]
credential-helper = "secretservice" # credhelper for specfic registry
location = "example.com/foo"

close containers/podman#4119
Signed-off-by: Qi Wang [email protected]

mtrmac

First pass.

My primary concerns, on which I don’t have a strong opinion yet — more things that I need to look into:

How does the credential helper configuration actually work? Do we ship a default configuration using a helper? Do we need the helper lists and the error handling complications they bring? This was probably discussed earlier, somewhere.
Is the helper configuration per-registry or per-namespace? We probably don’t want to introduce the more granular configuration in this PR, so we should at least reject such configurations.

pkg/docker/config/config.go

pkg/sysregistriesv2/system_registries_v2.go

pkg/docker/config/config.go

mtrmac

Thanks for the document, I think I’m now caught up.

If I’m reading that document correctly conceptually we need:

a file “credential helper” that invokes the current JSON implementation (not a separate binary, just a special case). That would not change much code, but move quite a bit into setAuthToCredHelper and the like, so it might make sense as a separate PR or at least a separate commit (+ we have to handle a JSON file specifying file as a credential helper, and reject infinite recursion).
- Related to this, it’s certainly something to think about it might make more sense to talk about credential “stores” rather than “helpers” if we add the fake file value. Let’s maybe worry about that later after the structure is prototyped.
sysregistriesv2 defaulting to something, I guess ["file"], if there is no configuration at all (updated systems with earlier configuration or no registries.conf at all).
When iterating through the list of configured credential helpers, some kind of logic to differentiate between a credential helper that is not set up in the current environment and a helper that is set up but just failed. I’m not sure what that would look like.

pkg/docker/config/config.go

mtrmac · 2020-06-30T20:10:28Z

pkg/docker/config/config.go

+	if len(credConfigs) > 0 {
+		if err := func(credConfigs []string, registry, username, password string) error {
+			for _, credConfig := range credConfigs {
+				if err = setAuthToCredHelper(credConfig, registry, username, password); err != nil {


So this should (log? and) try the next helper if this one is not set up. Right now the second or any further helpers are never tried at all — and if any fail, the errors are silently ignored.

Outstanding / depends on “helper installed but not set up” detection.

pkg/docker/config/config.go

mtrmac · 2020-06-30T20:15:41Z

pkg/docker/config/config.go

@@ -55,7 +56,27 @@ var (

 // SetAuthentication stores the username and password in the auth.json file


Throughout, the documentation should be updated to say that the credentials are stored/read/…, but not refer to any specific files.

Most of the functions still need updated documentation.

pkg/sysregistriesv2/system_registries_v2.go

mtrmac · 2020-06-30T20:57:20Z

pkg/sysregistriesv2/system_registries_v2.go

+	if len(config.CredsStore) > 0 {
+		return config.CredsStore, nil
+	}
+	return nil, nil


(We might eventually need the file fallback here.)

Prefer to add ["file"] configuration in a second PR. So if there's a "file" in the list, return the original list. If "file" does not exist, append it to the end of the list and can fall back to file when dealing with the credential configs?

No — if the user configures only a desktop keyring, we should not be overriding that decision and writing to files. Only if registries.conf/registries.d/* don’t contain any helper configuration, we would implicitly assume ["file"].

(With only this PR, we do always fall back to files, because we first use the helpers, and then always fall back to readJSON/modifyJSON, but that other PR would change that so that the JSON code only runs if file is specified.)

I agree with @mtrmac

mtrmac · 2020-06-30T21:03:29Z

Eventually, it would be useful to have tests for all of this; is definitely important enough. But that can certainly wait until after the structure of the code, and the details of the design, settle.

QiWang19 · 2020-07-06T13:31:19Z

@mtrmac PTAL

QiWang19 · 2020-07-13T13:45:35Z

@mtrmac PTAL. I added some tests in a separate commit. The CI fails with test-skopeo, is it a regression caused by bumpping go-multierror from 1.0.0 to 1.1.0?

QiWang19 · 2020-07-15T19:22:42Z

@mtrmac PTAL.

mtrmac · 2020-07-16T22:24:47Z

The CI fails with test-skopeo, is it a regression caused by bumpping go-multierror from 1.0.0 to 1.1.0?

Yes; #987 should fix that.

QiWang19 · 2020-07-21T20:01:00Z

@mtrmac PTAL

mtrmac

ACK on the API.

The major outstanding question is detecting installed but unavailable credential helpers; and then the code iterating through helpers needs to recognize that case.

Experimenting on Fedora 28:

With gnome-keyring not installed at all:

$ echo '{"ServerURL":"URL","Username":"user","Secret":"secret"}'| bin/docker-credential-secretservice store
** Message: 18:38:08.383: Remote error from secret service: org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.secrets was not provided by any .service files
The name org.freedesktop.secrets was not provided by any .service files
$ bin/docker-credential-secretservice list
Error from list function in secretservice_linux.c likely due to error in secretservice library
$ echo foo | bin/docker-credential-secretservice get 
GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.secrets was not provided by any .service files

which looks OK and clear enough, except for the list action; that might need an upstream fix (+ propagating that into distributions).

With gnome-keyring installed, but in a ssh session (= keyring not unlocked, and it’s possible but annoying to unlock it with a CLI):
```
$ echo '{"ServerURL":"URL","Username":"user","Secret":"secret"}'| bin/docker-credential-secretservice store
** Message: 18:41:51.235: Remote error from secret service: org.freedesktop.Secret.Error.IsLocked: Cannot create an item in a locked collection
Cannot create an item in a locked collection
$ bin/docker-credential-secretservice list
{}
$ echo foo | bin/docker-credential-secretservice get 
credentials not found in native keychain
```
From first principles, if there is a keyring, we probably do want to use it (= fail if it is locked, as above), but I’m a bit worried that “ssh to a server that has a lot of packages installed” might be a fairly common case; and there isn’t nice CLI to unlock the keyring (for the record, kill the existing daemon, if any, run gnome-keyring-daemon --replace --unlock and pass the password (no newline) on stdin; it silently succeeds if the password is wrong).

pkg/docker/config/config.go

mtrmac · 2020-07-25T15:29:51Z

pkg/docker/config/config.go

@@ -239,12 +327,31 @@ func RemoveAllAuthentication(sys *types.SystemContext) error {
 			}
 			logrus.Debugf("error removing credentials from kernel keyring")
 		}
+		for _, helper := range auths.CredHelpers {


auth.CredHelpers is a registry→helper map, so this should only remove the entry for that registry (if it exists), not all entries in that helper.

mtrmac · 2020-07-25T15:35:48Z

pkg/sysregistriesv2/system_registries_v2.go

+		// ignore the credHelper if it is set for inner namespace
+		if reg.CredentialHelper != "" && strings.Contains(reg.Prefix, "/") {
+			reg.CredentialHelper = ""
+			logrus.Errorf("credential-helper can only be configured for registry domain, not for inner namespaces %s", reg.Prefix)


Return an error value here, like parseLocation does above.

mtrmac · 2020-07-25T15:36:21Z

pkg/sysregistriesv2/system_registries_v2.go

@@ -570,6 +581,51 @@ func UnqualifiedSearchRegistries(ctx *types.SystemContext) ([]string, error) {
 	return config.UnqualifiedSearchRegistries, nil
 }

+// AllConfiguredCredentialHelpers returns a list of all credential helpers that are configred for some registry
+// Almost all caller should use CredentialHelpersForRegistry to get configuration applicable to a specific registry.


Suggested change

// Almost all caller should use CredentialHelpersForRegistry to get configuration applicable to a specific registry.

// Almost all callers should use CredentialHelpersForRegistry to get configuration applicable to a specific registry.

mtrmac · 2020-07-25T15:45:24Z

pkg/docker/config/config_test.go

+	defer func() {
+		os.Setenv("PATH", origPath)
+	}()
+	err = os.Chmod(filepath.Join(path, "testdata", "docker-credential-helper-registry"), os.ModePerm)


This should be 0755 at most; can’t the script just be committed as executable in git, so that we avoid this?

mtrmac · 2020-07-25T15:46:36Z

pkg/docker/config/config_test.go

@@ -83,6 +83,19 @@ func TestGetAuth(t *testing.T) {
 		os.Setenv("XDG_RUNTIME_DIR", origXDG)
 	}()

+	// override PATH for executing credHelper
+	path, err := os.Getwd()


This is not PATH; cwd, currentDirectory, packageDirectory, or even completely generic dir would be less confusing. (In both instances.)

mtrmac · 2020-07-25T15:47:29Z

pkg/docker/config/testdata/docker-credential-helper-registry

+#!/bin/bash
+
+ACTION="${1}"
+shift


Isn’t this unnecessary?

mtrmac · 2020-07-25T15:47:45Z

pkg/docker/config/testdata/docker-credential-helper-registry

+    exit 0
+    ;;
+list)
+    read REGISTRY


Suggested change

read REGISTRY

read UNUSED

TomSweeneyRedHat · 2020-07-30T14:16:56Z

Are there any changes to man pages with this? Or will that be in a separate PR?

QiWang19 · 2020-07-30T14:25:47Z

Yes, containers-registries.conf.5.md needs update.

QiWang19 · 2020-08-18T19:06:49Z

@mtrmac PTAL

QiWang19 · 2020-08-18T19:44:01Z

we should be able to detect the error gnome-keyring not installed or keyring locked, and fall back to current auth files or use ["file"] as credential-helper after implemented in the future PR, does this make sense?

QiWang19 · 2021-01-08T20:12:01Z

Is this a blocker of this PR docker/docker-credential-helpers#191? What else we need to get this PR in?

umohnani8 · 2021-01-18T19:19:09Z

@mtrmac @rhatdan can this be completed or is there something else blocking it? (apart from the rebase requirement)

rhatdan · 2021-01-20T10:26:32Z

I would love to get this in.
@QiWang19 Please rebase and get this to pass tests.

pkg/sysregistriesv2/system_registries_v2.go

rhatdan · 2021-01-21T13:59:06Z

LGTM

QiWang19 · 2021-01-21T18:54:48Z

@mtrmac PTAL

umohnani8 · 2021-02-08T17:19:27Z

@vrothberg @mtrmac @rhatdan PTAL, would like to get this moving along.
@QiWang19 this may need a rebase.

rhatdan · 2021-02-12T19:02:58Z

@umohnani8 Could you take over the PR and get it rebased.

Allow credential-helpers = [] and credential-helper = "" configuration in registries.conf as defautl store for credentials. Signed-off-by: Qi Wang <[email protected]>

umohnani8 · 2021-02-23T14:21:20Z

Looks like @QiWang19 is still following up with this.

QiWang19 · 2021-03-02T15:13:55Z

No new reviews for a long time, is this PR a feasible solution we want to avoid keeping the solution in plaintext? Should I close this PR?

rhatdan · 2021-03-02T19:48:07Z

@vrothberg PTAL

vrothberg

Apologies for the late review.

Mostly nits but I have one question on the design. Currently, we can configure exactly one credhelper per registry but we can configure multiple global ones. Should we leave the door open for more than one helper per registry and turn the string into a []string?

I have absolutely no feeling for it but want to point out that string is not extensible.

docs/containers-registries.conf.5.md

pkg/docker/config/config.go

vrothberg · 2021-03-12T10:09:18Z

pkg/sysregistriesv2/system_registries_v2.go

@@ -301,6 +306,11 @@ func (config *V2RegistriesConf) postProcessRegistries() error {
 			}
 		}

+		// ignore the credHelper if it is set for inner namespace
+		if reg.CredentialHelper != "" && strings.Contains(reg.Prefix, "/") {


@QiWang19, can you elaborate why we have that restriction?

The strings.Contains(reg.Prefix, "/") check is an overestimation since a registry may be running on domain.com/registry. I haven't seen it yet but it's technically possible so I want to make sure to fully understand the restriction. We may need to mention the reasoning also in the man page.

vrothberg · 2021-03-12T10:13:16Z

pkg/sysregistriesv2/system_registries_v2.go

+		}
+	}
+
+	var result []string


Maps in golang are non-deterministic, so we have to move result up and append to it in both for loops. That will preserve the order and make it deterministic.

vrothberg · 2021-03-12T10:13:55Z

pkg/sysregistriesv2/system_registries_v2.go

@@ -663,6 +673,51 @@ func GetShortNameMode(ctx *types.SystemContext) (types.ShortNameMode, error) {
 	return config.shortNameMode, err
 }

+// AllConfiguredCredentialHelpers returns a list of all credential helpers that are configured for some registry


Suggested change

// AllConfiguredCredentialHelpers returns a list of all credential helpers that are configured for some registry

// AllConfiguredCredentialHelpers returns a list of all credential helpers including the global credential helpers and the ones configured for specific registries in the given order.

vrothberg · 2021-03-12T10:15:31Z

pkg/sysregistriesv2/system_registries_v2.go

+	if err != nil {
+		return nil, err
+	}
+	for _, reg := range config.partialV2.Registries {


Please use FindRegistry instead.

vrothberg · 2021-03-12T10:18:14Z

pkg/sysregistriesv2/system_registries_v2.go

+// CredentialHelpersForRegistry returns a list of credential helpers to try to use for the given registry,
+// in the order to try.


Suggested change

// CredentialHelpersForRegistry returns a list of credential helpers to try to use for the given registry,

// in the order to try.

// CredentialHelpersForRegistry returns the configured credential helper for the specified registry or the global credential helpers in order.

rhatdan · 2021-03-15T11:01:34Z

@vrothberg Could you take this over and drive it home. I think @QiWang19 is going to be too busy with CRI-O issues. is that correct @QiWang19 ?

QiWang19 · 2021-03-15T19:22:44Z

@vrothberg Could you take this over and drive it home. I think @QiWang19 is going to be too busy with CRI-O issues. is that correct @QiWang19 ?

yes, @vrothberg feel free to take this over.

vrothberg · 2021-03-16T10:01:34Z

@vrothberg Could you take this over and drive it home. I think @QiWang19 is going to be too busy with CRI-O issues. is that correct @QiWang19 ?

yes, @vrothberg feel free to take this over.

Will do. Thanks for working on this, @QiWang19 !

rhatdan · 2021-03-23T18:33:38Z

@vrothberg Do you think we could get this in, and cut a new release?

vrothberg · 2021-03-24T12:28:54Z

@vrothberg Do you think we could get this in, and cut a new release?

When do you need it?

rhatdan · 2021-03-24T12:35:09Z

Probably post podman 3.1 at this point. But I would like to get this into podman and Buildah.

vrothberg · 2021-03-24T12:39:20Z

Probably post podman 3.1 at this point. But I would like to get this into podman and Buildah.

Excellent, that relaxes me a bit. I want to get the buildah source PR out today and will find time tomorrow.

vrothberg · 2021-03-30T11:15:22Z

Replaced by #1193

QiWang19 force-pushed the registries-conf-creds branch from 182740a to 836e734 Compare June 25, 2020 15:41

mtrmac reviewed Jun 25, 2020

View reviewed changes

QiWang19 force-pushed the registries-conf-creds branch 2 times, most recently from 7e48fa0 to e7aa290 Compare June 30, 2020 19:58

mtrmac reviewed Jun 30, 2020

View reviewed changes

QiWang19 force-pushed the registries-conf-creds branch 2 times, most recently from 432fb1b to d7049e0 Compare July 2, 2020 15:11

QiWang19 force-pushed the registries-conf-creds branch 4 times, most recently from 903a079 to 573fca6 Compare July 12, 2020 02:31

QiWang19 force-pushed the registries-conf-creds branch from 573fca6 to 1c77934 Compare July 15, 2020 19:21

QiWang19 force-pushed the registries-conf-creds branch 2 times, most recently from 8b053e7 to 577ec24 Compare July 20, 2020 17:45

mtrmac reviewed Jul 25, 2020

View reviewed changes

QiWang19 force-pushed the registries-conf-creds branch 2 times, most recently from 6a6d9a6 to 5574452 Compare July 30, 2020 00:49

QiWang19 force-pushed the registries-conf-creds branch from 5574452 to d3c18c8 Compare July 31, 2020 19:58

QiWang19 force-pushed the registries-conf-creds branch 2 times, most recently from fa45f9c to 8d65474 Compare August 18, 2020 19:06

rhatdan reviewed Jan 20, 2021

View reviewed changes

pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved

QiWang19 force-pushed the registries-conf-creds branch 2 times, most recently from 716b72c to 2762353 Compare January 20, 2021 19:32

QiWang19 force-pushed the registries-conf-creds branch from 2762353 to 2ddfb76 Compare January 21, 2021 18:55

QiWang19 linked an issue Jan 21, 2021 that may be closed by this pull request

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

Open

QiWang19 force-pushed the registries-conf-creds branch from 2ddfb76 to ade9d06 Compare February 8, 2021 19:58

Auth configs in registries.conf

4eed7e2

Allow credential-helpers = [] and credential-helper = "" configuration in registries.conf as defautl store for credentials. Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the registries-conf-creds branch from ade9d06 to 4eed7e2 Compare February 18, 2021 16:53

vrothberg reviewed Mar 12, 2021

View reviewed changes

mtrmac mentioned this pull request Mar 15, 2021

Implement credsStore in credential config files #1179

Open

vrothberg closed this Mar 30, 2021

rhuss mentioned this pull request Jun 15, 2021

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

Open

		@@ -55,7 +56,27 @@ var (

		// SetAuthentication stores the username and password in the auth.json file

	// Almost all caller should use CredentialHelpersForRegistry to get configuration applicable to a specific registry.
	// Almost all callers should use CredentialHelpersForRegistry to get configuration applicable to a specific registry.

	// AllConfiguredCredentialHelpers returns a list of all credential helpers that are configured for some registry
	// AllConfiguredCredentialHelpers returns a list of all credential helpers including the global credential helpers and the ones configured for specific registries in the given order.

		// CredentialHelpersForRegistry returns a list of credential helpers to try to use for the given registry,
		// in the order to try.

Auth configs in registries.conf #971

Auth configs in registries.conf #971

Conversation

QiWang19 commented Jun 24, 2020 • edited Loading

mtrmac left a comment

Choose a reason for hiding this comment

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

mtrmac commented Jun 30, 2020

QiWang19 commented Jul 6, 2020

QiWang19 commented Jul 13, 2020 • edited Loading

QiWang19 commented Jul 15, 2020

mtrmac commented Jul 16, 2020

QiWang19 commented Jul 21, 2020

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

TomSweeneyRedHat commented Jul 30, 2020

QiWang19 commented Jul 30, 2020

QiWang19 commented Aug 18, 2020

QiWang19 commented Aug 18, 2020

QiWang19 commented Jan 8, 2021

umohnani8 commented Jan 18, 2021

rhatdan commented Jan 20, 2021

rhatdan commented Jan 21, 2021

QiWang19 commented Jan 21, 2021

umohnani8 commented Feb 8, 2021

rhatdan commented Feb 12, 2021

umohnani8 commented Feb 23, 2021

QiWang19 commented Mar 2, 2021

rhatdan commented Mar 2, 2021

vrothberg 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

rhatdan commented Mar 15, 2021

QiWang19 commented Mar 15, 2021

vrothberg commented Mar 16, 2021

rhatdan commented Mar 23, 2021

vrothberg commented Mar 24, 2021

rhatdan commented Mar 24, 2021

vrothberg commented Mar 24, 2021

vrothberg commented Mar 30, 2021

QiWang19 commented Jun 24, 2020 •

edited

Loading

QiWang19 commented Jul 13, 2020 •

edited

Loading