types: Add SystemContext.GetAuth #588

wking · 2019-02-22T15:16:22Z

The previous system is difficult to configure, with several SystemContext properties feeding the auth-getting logic. It also assumed that there was either a single username/password (DockerAuthConfig) or that the auth information was on disk somewhere. With this pull request, I'm pushing all of the complexity down into a user-supplied getter (falling back to the old logic if the getter is unset). That makes it easy to plug in your own alternative without worrying about the default logic.

The Docker-config AuthStore has a public Config property to make it easy for callers to drop in their own configuration for read-only access without having to go through either the filesystem or JSON. I've made the backing types public to support that use-case, and set custom JSON serialization for the newly-public Auth structure to allow Go callers to avoid having to understand the base64 wrapping used in the JSON serialization.

I've also shifted a number of auth-getting unit tests from docker_client_test.go into config_test.go, since they only excercise config.go logic.

Ideally the auth interface would support challenge/response auth like HTTP's, but for now it just uses Docker's "we'll be able to hard-code authorization for each authority" approach.

mtrmac

Could you back out and explain what is it you are, at the high level, trying to implement?

“difficult to configure” motivating a single +768-647 commit makes it pretty difficult to reason about, and this introduces rather many abstractions and public APIs that, at a first glance, seem either overly general for the task, or, worse, look general but are actually quite restricted.

Right now, it’s pretty difficult to evaluate.

Providing a callback to get docker/distribution credentials, sure, I can imagine uses, in principle. I can’t see at all how the complaint about a config file being on-disk motivates the store abstraction or making the data format a public API (guessing – is this to use OpenShift secrets as []byte data? That might be useful, but the proposed config.AuthStore seems like a very inconvenient way to attach it to a types.SystemContext).

The detailed review comments are just random observations on the current code, but really we should talk about the use cases and needs first.

mtrmac · 2019-02-23T03:43:10Z

pkg/docker/config/config.go

@@ -1,29 +1,67 @@
 package config


+284-180 lines in a single commit 😭

credentials/single/single.go

mtrmac · 2019-02-23T03:51:03Z

credentials/single/single.go

@@ -0,0 +1,42 @@
+// Package single implements a basic credentials helper with a single username and secret.


(It’s not at all obvious to me that this warrants a top-level namespace here, but that depends on much else.)

mtrmac · 2019-02-23T03:55:34Z

pkg/docker/config/config.go

+}
+
+// Config holds the full authorization configuration.
+type Config struct {


Why should this be public? (Note that this is not the full definition of the Docker structure.)

If the goal is to provide callback-based GetAuth capabilities, fine, then the callers have no reason to care about these fields at all; they can submit a callback without dealing with the Docker format. And if the goal is to allow callers to submit a Docker-like config in memory without creating a file, they still don’t need this type exposed.

And if the goal is to allow callers to submit a Docker-like config in memory without creating a file, they still don’t need this type exposed.

That's the goal (for the OpenShift installer, which has the pull-secret portion of this file in memory as []byte). And while we could have an internal config holding the structured data with a LoadFromBytes([]byte) or similar method, it seemed easier and more convenient to just make the structured configuration public (e.g. see the unit tests, which no longer need the makeTestAuthConfig helper). Do you have concerns about having this be public?

pkg/docker/config/config.go

mtrmac · 2019-02-23T04:03:50Z

types/types.go

 type DockerAuthConfig struct {
 	Username string
 	Password string
 }

+// AuthGetter retrieves credentials for HTTP(S) access.  It returns
+// the username and secret as strings.
+type AuthGetter func(serverURL string) (string, string, error)


It’s very much not obvious to me that it makes sense to have a single callback for all kinds of “HTTP(S)” access, no matter the protocol on top of HTTPS, or authentication mechanism. (Sure, the AuthFilePath name is similarly misleading; we originally thought that it would somehow be generic, but it clearly is not.)

WRT serverURL, in docker/distribution the lookup key is not an URL!

WRT serverURL, in docker/distribution the lookup key is not an URL!

The interface I'm implementing is credentials.Helper, which uses serverURL. This getter is the Get method from that interface. I'm fine using a different string if you want for the input argument (personally, I prefer "authority", or adjusting the docstring to whatever you want me to use instead, just tell me what to paste in ;).

... we originally thought that it would somehow be generic, but it clearly is not...

HTTP(S) auth is a generic thing. Unfortunately, the Docker APIs don't really conform, since they don't provide a way to feed in a challenge string. Should we come up with our own API (or lean on one that already exists in Go) here, and then special-case the Docker approach with dummy challenge?

mtrmac · 2019-02-23T04:05:13Z

types/types.go

-	// If not "", overrides the default path for the authentication file
+	// If not "", overrides the default path for the authentication file.  Ignored for reading if AuthConfig is not "".
+	//
+	// Deprecated: Use the GetAuth property and github.com/containers/image/pkg/docker/config's AuthStore instead.


It’s not obviously attractive to force the users to create extra objects when they could just set a clearly-named field. Sure, the consumer of the data might need to define what happens if multiple fields are set, or refuse such configurations, but that seems worth it.

Sure, the consumer of the data might need to define what happens if multiple fields are set, or refuse such configurations, but that seems worth it.

SystemContext seems complicated enough already, to me it is a clear win to factor out the auth-configuration properties into other locations. For folks who currently use SystemContext, the transitions seem pretty clear to me, although I can work up migration docs if that would help address your concerns (or even just give us something more concrete to anchor this discussion).

mtrmac · 2019-02-23T04:17:19Z

pkg/docker/config/config.go

+// SetAuthentication stores the username and password in the auth.json file
+//
+// Deprecated: Use an AuthStore.
+func SetAuthentication(sys *types.SystemContext, registry, username, password string) error {


If AuthStore is supposed to be a replacement, it needs to support SystemContext.

If AuthStore is supposed to be a replacement, it needs to support SystemContext.

I don't think so. The Docker client needs a way to get credentials for a given authority, and that's what the new GetAuth property provides. Users will need a way to create that getter, and that's what the two new AuthStore structures provide. The old SystemContext properties either configure a single credential (which can be replaced easily with the new single.AuthStore) or the path lookup for the Docker-config AuthStore (which can be replaced by creating your own config.AuthStore with Path set however they like). The rest of SystemContext seems unrelated to managing authentication, right?

mtrmac · 2019-02-23T04:17:58Z

pkg/docker/config/config.go

 	})
 }

 // GetAuthentication returns the registry credentials stored in
 // either auth.json file or .docker/config.json
 // If an entry is not found empty strings are returned for the username and password
+//
+// Deprecated: Use an AuthStore.


If AuthStore is supposed to be a replacement, it needs to make it easy to use all relevant fields of SystemContext.

mtrmac · 2019-02-23T04:23:08Z

types/types.go

-	// if nil, the library tries to parse ~/.docker/config.json to retrieve credentials
+	// if nil, the library tries to parse ~/.docker/config.json to retrieve credentials.
+	//
+	// Deprecated: Use the GetAuth property and github.com/containers/image/credentials/single's AuthStore instead.


It’s not obviously attractive to force the users to create extra objects when they could just set a clearly-named field. Sure, the consumer of the data might need to define what happens if multiple fields are set, or refuse such configurations, but that seems worth it.

Plumbing together creation of a single.Credential+single.AuthStore, and somehow making an AuthGetter out of that, seems way too much code compared to a DockerAuthConfig: &types.DockerAuthConfig{…}

Plumbing together creation of a single.Credential+single.AuthStore, and somehow making an AuthGetter out of that...

$ git describe v1.3-49-g44801dd $ cat test.go package main import ( "fmt" "github.com/containers/image/types" "github.com/containers/image/credentials/single" "github.com/docker/docker-credential-helpers/credentials" ) func main() { authStore := single.AuthStore(credentials.Credentials{ ServerURL: "example.org", Username: "alice", Secret: "secret", }) sys := &types.SystemContext{ GetAuth: authStore.Get, } username, secret, err := sys.GetAuth("example.org") fmt.Println(username) fmt.Println(secret) fmt.Println(err) } $ go run test.go alice secret <nil>

That doesn't seem too complicated to me, and it saves you from having to explain how DockerAuthConfig interacts with a GetAuth or other SystemContext properties.

wking · 2019-02-27T22:41:42Z

I'll rebase this once #589 lands, so I can rebase this on top to shrink the diff size.

rhatdan · 2019-03-01T08:58:39Z

Looks like it has merged.

wking · 2019-03-01T09:48:43Z

Rebased onto master (around #589) with 44801dd -> 245b3b3, which also fixes a few bugs.

The previous system is difficult to configure, with several SystemContext properties feeding the auth-getting logic. It also assumed that there was either a single username/password (DockerAuthConfig) or that the auth information was on disk somewhere. With this commit, I'm pushing all of the complexity down into a user-supplied getter (falling back to the old logic if the getter is unset). That makes it easy to plug in your own alternative without worrying about the default logic. The Docker-config AuthStore has a public Config property to make it easy for callers to drop in their own configuration for read-only access without having to go through either the filesystem or JSON. I've made the backing types public to support that use-case, and set custom JSON serialization for the newly-public Auth structure to allow Go callers to avoid having to understand the base64 wrapping used in the JSON serialization. Ideally the auth interface would support challenge/response auth like [1], but for now it just uses Docker's "we'll be able to hard-code authorization for each authority" approach. URI authorities are specified in [2]. [1]: https://tools.ietf.org/html/rfc7235 [2]: https://tools.ietf.org/html/rfc3986#section-3.2 Signed-off-by: W. Trevor King <[email protected]>

wking · 2019-03-06T20:24:32Z

Rebased onto master (no conflicts) with 245b3b3 -> 4800205 to resolve the "This branch is out-of-date with the base branch" warning.

Since e2b31b2 (bootkube: Supply machine-os-content to MCO, 2019-01-29, openshift#1149), we have been using the machine-os-content image to seed the machine-config operator. With this commit, use the RHCOS build ID from that image's annotations to calculate our AMI, etc. as well. This gives one less degree of freedom for breaking things ;). Users who want to test clusters based on a different RHCOS build should bump the value in their release image, just like users testing operator updates and other changes. The new pkg/asset/release subpackage allows users to continue using pkg/rhcos without pulling in all of containers/image as a dependency. The pull-secret handling is a bit of a hack, leaning on the fact that users are likely providing clean secrets from [1]. Hopefully soon containers/image will grow an API for injecting in-memory bytes into their more-robust Docker-auth-config parser, but my attempt at that [2] is unlikely to land in the next few days, so I've cludged together a minimal implementation here. [1]: https://cloud.openshift.com/clusters/install#pull-secret [2]: containers/image#588

wking · 2019-03-18T14:20:00Z

I'm going to hold off on rebasing this until #597 settles.

rhatdan · 2019-04-25T09:01:46Z

@wking #597 merged can you rebase.

wking · 2019-09-24T21:55:14Z

I took a run at rebasing this, but the new keyring stuff that started in #619 is too confusing for me ;). If anyone else wants to pick this up and rebase it, please go ahead without waiting on me.

mtrmac · 2023-11-27T22:04:43Z

Four years later, the credential code has changed a lot.

The implementation approach of mirroring the semantics of github.com/docker/docker-credential-helpers/credentials.Helper is no longer sufficient (if it ever was, because the helper implementations are IIRC wildly inconsistent about the lookup keys the support) because at least some of those helpers key off the host name, but we now support more granular credential namespaces (quay.io/openshift-release-dev).

History has also shown that as the capabilities keep growing, file formats must accommodate both old and new readers/writers, making it unattractive to expose the details of the data structures like Config.{Auths,CredHelpers}.

I do, now, agree that many of the c/image subpackages should have their own configuration API without adding everything to SystemContext. pkg/docker/config is one of the places where introducing that is hard, because it depends on pkg/sysregistriesv2, so that one would have to be extended to have a non-SystemContext API first. (And, now that we unwisely bumped the major number to opt into Go module semantics, we would still need to keep all the SystemContext APIs around.)

Overall, I’m afraid this would need to be re-implemented from scratch either way, by now.

wking force-pushed the get-auth branch from 8e7876c to 8ce2b8b Compare February 22, 2019 15:17

This was referenced Feb 22, 2019

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

Open

pkg/rhcos/release: Extract RHCOS build from release image openshift/installer#1286

Closed

mtrmac reviewed Feb 23, 2019

View reviewed changes

wking force-pushed the get-auth branch 2 times, most recently from 5e011a3 to 44801dd Compare February 25, 2019 20:42

wking mentioned this pull request Feb 25, 2019

pkg/docker/config: Bring auth tests from docker/docker_client_test #589

Merged

wking force-pushed the get-auth branch from 44801dd to 245b3b3 Compare March 1, 2019 09:48

wking force-pushed the get-auth branch from 245b3b3 to 4800205 Compare March 6, 2019 20:24

This was referenced Mar 8, 2019

additional authentication files #595

Closed

pkg/docker/config: Deprecate GetUserLoggedIn #597

Merged

mtrmac mentioned this pull request May 11, 2019

Reset SystemContext.DockerAuthConfig when using mirrors at different registries #625

Merged

mtrmac mentioned this pull request Aug 17, 2021

Docker APIv2 build endpoint bug with X-Registry-Config containers/podman#11235

Closed

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

mtrmac closed this Nov 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

types: Add SystemContext.GetAuth #588

types: Add SystemContext.GetAuth #588

wking commented Feb 22, 2019 •

edited

Loading

mtrmac left a comment

mtrmac Feb 23, 2019

mtrmac Feb 23, 2019

mtrmac Feb 23, 2019

wking Feb 25, 2019

mtrmac Feb 23, 2019

wking Feb 25, 2019

mtrmac Feb 23, 2019

wking Feb 25, 2019

mtrmac Feb 23, 2019

wking Feb 25, 2019

mtrmac Feb 23, 2019

mtrmac Feb 23, 2019

wking Feb 25, 2019

wking commented Feb 27, 2019 •

edited

Loading

rhatdan commented Mar 1, 2019

wking commented Mar 1, 2019

wking commented Mar 6, 2019

wking commented Mar 18, 2019

rhatdan commented Apr 25, 2019

wking commented Sep 24, 2019

mtrmac commented Nov 27, 2023

		@@ -0,0 +1,42 @@
		// Package single implements a basic credentials helper with a single username and secret.

types: Add SystemContext.GetAuth #588

types: Add SystemContext.GetAuth #588

Conversation

wking commented Feb 22, 2019 • edited Loading

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

wking commented Feb 27, 2019 • edited Loading

rhatdan commented Mar 1, 2019

wking commented Mar 1, 2019

wking commented Mar 6, 2019

wking commented Mar 18, 2019

rhatdan commented Apr 25, 2019

wking commented Sep 24, 2019

mtrmac commented Nov 27, 2023

wking commented Feb 22, 2019 •

edited

Loading

wking commented Feb 27, 2019 •

edited

Loading