Add authentication package to containers/image/pkg #333

umohnani8 · 2017-08-29T15:37:44Z

This package is used in authenticating a user for kpod login
and can be used for authentication in kpod push, pull etc.

Signed-off-by: umohnani8 [email protected]

rhatdan · 2017-08-29T15:40:24Z

Why not just include this with the kpod login pull request?

umohnani8 · 2017-08-29T15:42:11Z

@rhatdan it is with the kpod login pull request cri-o/cri-o#810.
Since I made changes to containers/image, I thought I needed to open one here also.

mtrmac · 2017-08-29T15:52:54Z

A one-minute look:

The top level of the package could benefit from a bit more documentation about what this does, especially for readers who don’t know what (kpod login) is.
The new file probably shouldn’t be in containers/image/vendor/github.com/containers/image/…
This seems to be an username/password storage and verification system, only for docker/distribution servers. If so, how does it interact with ~/.docker/config.json ? Does one override the other, is only one of the files used at a time? Shouldn’t docker/docker_client.go automatically use this file as well? (And shouldn’t they share the request setup / token-related code?)

umohnani8 · 2017-08-29T16:04:49Z

whoops, my bad. I messed up while applying the patch - I have fixed that.

nalind · 2017-08-29T19:00:26Z

pkg/authentication/authentication.go

+	"os"
+	"strings"
+
+	"github.com/errors"


github.com/pkg/errors?

nalind · 2017-08-29T19:03:40Z

pkg/authentication/authentication.go

+	}
+	dir := os.Getenv("HOME") + "/.config/containers"
+	if _, err := os.Stat(dir); os.IsNotExist(err) {
+		if err = os.Mkdir(dir, os.ModePerm); err != nil {


This is probably not the right set of permissions to apply to the directory.

0600, is probably what you want.

nalind · 2017-08-29T19:03:48Z

pkg/authentication/authentication.go

+			return "", errors.Wrapf(err, "error creating directory %q", dir)
+		}
+	}
+	return dir + "/auth.json", nil


I'd prefer using path/filepath.Join() to build these file paths.

Using filepath.Join.

runcom · 2017-08-30T09:22:27Z

This package is docker specific, I'd put it under ./docker/ first. Also, I believe you can reuse many functions already in the docker package that deals with authn (or abstract them out in another generic pkg):

https://github.com/containers/image/blob/master/docker/wwwauthenticate.go
https://github.com/containers/image/blob/master/docker/docker_client.go#L31
https://github.com/containers/image/blob/master/docker/docker_client.go#L401

...and the like.

I would love this to be abstract though. One way would be to support our own credentials file, but we should have a mechanism to read from docker creds also (that's what most people expect I guess).

umohnani8 · 2017-08-30T13:32:42Z

@runcom I did not realize this would end up being docker specific. You want me to scratch what I did, and use functions from the Docker package instead?

mtrmac · 2017-08-30T17:51:27Z

Is there perhaps a previous conversation were I can catch up about what this is intended to do?

Are we defining a completely new configuration format? If so, is it intended to be docker-specific or generalized… for what kind of other uses? Is it already frozen, or is defining the format a part of this PR?
Is containers/image expected to use the configuration by default, without an explicit opt-in? For all transports or docker:// only? How does it interact with ~/.docker/config.json?

It’s difficult to form an opinion about an implementation when I don’t know what it should be implementing :)

mtrmac · 2017-08-30T18:00:22Z

It’s difficult to form an opinion about an implementation when I don’t know what it should be implementing :)

As a random example, consider today’s #334 . Is the new config file expected to support a similar mechanism? Now? Sometime in the future? Never? Exactly the same mechanism, or a somewhat different one?

Or is all of this, perhaps, intended to support exactly the same config file, only supporting a different file name as an alternative?

umohnani8 · 2017-08-30T18:02:20Z

@mtrmac I forwarded an email chain to you about a conversation we had about this.

I believe we want the authentication package to aid in the kpod login command, where the user can log into any container registry and their creds will be stored in ~/.config/containers/auth.json or in CRIO_AUTH_PATH/auth.json if CRIO_AUTH_PATH is set.

I know for GetAuthentication it checks ~/.config/container/auth,json first and if it can't find the registry there it falls back to ~/.docker/config.json

I thought it would be generalized and not docker specific, but now I am not so sure. Maybe @rhatdan can explain further.

mtrmac · 2017-08-30T18:40:13Z

I thought it would be generalized and not docker specific, but now I am not so sure.

Yeah. The docker/distribution-specificity of the code is fairly simple to deal with — we could separate the "read/write data" functionality from the "check whether the user-provided password is valid” functionality, perhaps partially, perhaps entirely and have only kpod login call both of them [pedantically, there’s no fundamental reason to prevent a kpod login for a registry which is down, so the check is not an essential component—though it is a very convenient and useful one in practice].

The docker/distribution-specificity of the format is worse: if we want the format to be generic, we can’t have an universal auths map without somehow allowing credentials for various kinds of services (in the worst case, several different services running on the same host name!).

But if designing such a new generic thing were out of scope and this were about the file name, I’d much prefer sharing a single implementation between ~/docker/config.json and ~/.config/containers/auth.json, so that non-trivial things like #334 an automatically apply to both. (Or would we not want to do exactly #334 for the new file name either, because of the naming of the plugins?)

rhatdan · 2017-08-30T18:57:02Z

I would like to keep this simple for now, if we want to add more complex authentication in the future, I am all for it. (Kerberos?) But for now, I am looking for the equiv of docker login so that kpod and buildah can cache the login information.

mtrmac · 2017-08-30T19:07:14Z

But for now, I am looking for the equiv of docker login so that kpod and buildah can cache the login information.

Then we could just keep using ~/.docker/config.json, and only add the editing functionality?

rhatdan · 2017-08-30T19:11:39Z

I would prefer the default to not be docker/config.json, but ~/config/containers/config.json. But have a fail over to ~/.docker/config.json. Our tools should just read it not write it. Bottom line I would like us to take advantage of docker login, not not necessary for us to allow docker pull to take advantage of kpod login. I would rather not have to add the letters docker to our man pages or help docs.

mtrmac · 2017-08-30T20:28:10Z

OK, so how about this:

We have no ambition to define a substantively new format for now; only a new location.
The existing parser from c/i/docker/docker_client.go:getAuth (which already has a fallback to another file name) is somehow merged with the parser from this PR, and together with the editing functions from this PR (but not checkAuthentication), extracted into a new package (perhaps c/i/docker/config; or c/i/pkg/docker-config, or whatever).
c/i/docker/docker_client.go is modified to use this new package
The checkAuthentication part of this is merged into c/i/docker/docker_client.go, reusing as much as possible, exposed as a new public function in that package.
kpod login calls the new authentication checker from c/i/docker, and if successful, adds the credential using the new subpackage.

WDY’all think?

mtrmac · 2017-08-30T20:53:14Z

pkg/authentication/authentication.go

+	if server == "docker.io" {
+		return "https://registry-1.docker.io"
+	}
+	return server


What is using this? There is no user in this repo.

It seems weird to return a URL-like string in one case and an undecorated host-name in the other; and it also feels like the kind of internal implementation detail of the protocol that might be better to hide inside a package instead of exposing it is a very public API point.

Yes I agree.

@mtrmac where would you suggest I put this?

Why is this needed?

Since there is no default registry, and docker.io maps to registry-1.docker.io, I thought it would be good to add this in the case the user types kpod login docker.io. Maybe containers/image/docker/docker_client.go does this somewhere, not sure but I will look into it.

Yes I would figure this happens somewhere else in the stack or in DNS. Hard coding this could cause us issues if Docker inc in the future changes docker.io to pint at registry-foobar.docker.io.

Sorry, I didn’t realize that both the plain-host name and URI forms are actually valid keys in auths (or at least I do have both in my ~/.docker/config.json—except that I have index.docker.io/v1/ in there, not registry-1.docker.io).

This mapping is actually hard-coded in the clients, not in DNS, sadly.

If possible, I’d prefer this to be completely hidden from external users: let them say just docker.io as if it were any example.com, and do this mapping internally in the config file parser/editor package.

rhatdan · 2017-08-30T21:09:58Z

@mtrmac I am fine with what you are defining.

TomSweeneyRedHat · 2017-08-30T23:09:19Z

@mtrmac reusing the format inside the file at the moment sounds fine by me as does the rest of your proposal. It might be I'm just tired tonight, but just wanted to make sure this sequence works in your plan.

A kpod/buildah command that requires authentication but no creds are provided is invoked.
We first check the cache that kpod login has created to get the credentials.
If not found, then we check the current cache from docker login to get the credentials.
kpod/buildah command then passes the credentials it has, which might be none, along to the registry which will do it's thing.

If that works in your mind, I'm hip.

rhatdan · 2017-08-31T09:06:17Z

Exactly.

umohnani8 · 2017-08-31T13:36:08Z

@mtrmac @rhatdan Will get started on the new plan of getting this working.

mtrmac · 2017-08-31T14:59:39Z

A kpod/buildah command that requires authentication but no creds are provided is invoked.
We first check the cache that kpod login has created to get the credentials.
If not found, then we check the current cache from docker login to get the credentials.
kpod/buildah command then passes the credentials it has, which might be none, along to the registry which will do it's thing.

Right. To be explicit, the docker: transport is automatically reading the docker file, and I think it would make sense to read the new one automatically as well; i.e. kpod/buildah should have to do nothing to use the credentials from (* login); it would need to set up types.SystemContext only if the credentials came from a different place, perhaps a command-line option or a kpod/buildah tool-specific unshared config file.

umohnani8 · 2017-09-20T17:25:24Z

skopeo test fixes in containers/skopeo#422

mtrmac

Thanks for fixing the golint errors as well. (In the future, please separate such things into separate commits—but it’s not necessary to split it out now.)

mtrmac · 2017-09-20T17:51:26Z

pkg/docker/config/config.go

+			return ctx.AuthFilePath
+		}
+		if ctx.RootForImplicitAbsolutePaths != "" {
+			return ctx.RootForImplicitAbsolutePaths


This should be RootForImplicitAbsolutePaths + default runtimeDir + authCfg + authCfgFileName (e.g. $root/run/$uid/containers/auth.json.

runtimeDir = the first applicable from:

$RootForImplicitAbsolutePaths/run/$uid

$XDG_RUNTIME_DIR

/run/$uid

auth path = $AuthFilePath if set; otherwise $runtimeDir/containers/auth.json

mtrmac · 2017-09-20T17:54:08Z

docker/docker_client.go

 //
 // debugging: https://github.com/containers/image/pull/211#issuecomment-273426236 and follows up
 func (c *dockerClient) setupRequestAuth(req *http.Request) error {
+	var scope string


(Non-blocking: Consider moving the scope declaration just before its first use; it’s used only in case "bearer".)

mtrmac · 2017-09-20T18:44:47Z

LGTM (assuming the skopeo failures will be dealt with somehow.)

@runcom PTAL

umohnani8 · 2017-09-25T14:43:54Z

@runcom PTAL

runcom · 2017-09-25T16:21:11Z

docker/docker_client.go

+	// docker V1 registry.
+	ErrV1NotSupported = errors.New("can't talk to a V1 docker registry")
+	// ErrUnauthorized is returned when the status code returned is 401
+	ErrUnauthorized = errors.New("unable to retrieve auth token: 401 unauthorized")


drop "401 unauthorized", that's an implementation detail we don't want to expose directly in the API

I see it was this way in the past, but we should not expose the error, can you make it private? (lowercasing it)

… This should probably say something more generic like “invalid credentials”, it is returned also on the Authorization: Basic path where no tokens are involved.

I see it was this way in the past, but we should not expose the error,

This should allow callers of CheckAuth to distinguish between unexpected failures (e.g. network down) and an invalid username/password.

So I can leave the error exposed and just change the error message?

I just remembered, I am using this error in kpod login to know when a 401 is returned so that the message printed for the user is along the paths of "invalid username/password", hence making it public.

So given the places this error is used it must not contain "401 ..." cause it's HTTP related and should probably be named something like ErrUnauthorizedForCredentials (better naming is advised here but just ErrUnauthorized is misleading).

got it, fixed.

runcom · 2017-09-25T16:22:35Z

docker/docker_client.go

 				service, _ := challenge.Parameters["service"] // Will be "" if not present
-				scope := fmt.Sprintf("repository:%s:%s", c.scope.remoteName, c.scope.actions)
+				var scope string
+				if c.scope.remoteName != "" && c.scope.actions != "" {


why this if was added? I'll keep it how it was

Because a “log in” action (i.e. only verify that the username/password are valid) don’t really have a scope, and Docker seems to do the same thing.

awesome, thx

thanks @mtrmac :)

runcom · 2017-09-25T16:23:39Z

pkg/docker/config/config.go

+var (
+	// ErrNotLoggedIn is returned for users not logged into a registry
+	// that they are trying to logout of
+	ErrNotLoggedIn = errors.Errorf("not logged in")


errors.New

runcom · 2017-09-26T09:25:33Z

docker/docker_client.go

+	resp, err := newLoginClient.makeRequest(ctx, "GET", "/v2/", nil, nil)
+	if err != nil {
+		return err
+	}


you forgot a defer resp.Body.Close()?

umohnani8 · 2017-09-27T14:10:09Z

@mtrmac @runcom PTAL. Added a new function RemoveAllAuthentication to delete all stored credentials in the auth file.

mtrmac

Works for me; @runcom please do look at the CredHelpers removal though.

mtrmac · 2017-09-27T15:48:04Z

pkg/docker/config/config.go

+// RemoveAllAuthentication deletes all the credentials stored in auth.json
+func RemoveAllAuthentication(ctx *types.SystemContext) error {
+	return modifyJSON(ctx, func(auths *dockerConfigFile) (bool, error) {
+		auths.CredHelpers = make(map[string]string)


Hum, this is a bit surprising. Depending on how one looks at it, it either makes perfect sense or it is extremely unexpected.

I guess, why not — @runcom I just want to make sure you have noticed this.

THis is for
kpod logout --all

Basically you want to cleanup and logout of your container workflow. In most cases not really necessary since the default location is in /run/USER/UID, but it is better to be safe and not leave these credentials around.

The way CredHelpers work for logging in is that the user must set them up in the config file before starting the log-in process; and it seems reasonable that in a (log in; log out; log in) sequence, the credentials should be stored in the same place (i.e. that the users’ configuration should not be thrown away).

OTOH then kpod logout --all would either have to ignore the helpers-using domains entirely, or clean everything in the helper, and both can be pretty unexpected (removing items from a server-side shared keyring).

As you say, with /run being the default location, it’s likely enough that users of credential helpers will be overriding the path and knowing not to call logout --all, and this is at least conservative in not deleting too much.

(I guess I could argue that logout --all is a problematic concept in itself, but that’s not going to help, is it?)

Another option rather then doing this through the library would be to
rm -f $autffile
from
kpod logout --all.

I guess do appreciate that would get rid of me with my pesky questions, but it doesn’t really avoid having to make a decision.

As I said at the very start, “I guess, why not”, all I want here is to make sure this choice has been made deliberately.

Just wondering - how is doing rm -f $authfile any different from clearing the CredHelpers field in the authfile? At the end of the day you are still getting rid of the CredHelpers no?

@umohnani8 yup, this is just a matter of whether you want it in the library or not.

ah okay. So what is the decision? I don't see a problem with it being in the library just because to me both approaches does the same thing at the end of the day. But you guys know better :)

I can live with either design.

@runcom still needs to approve the PR as a whole, so it’s ultimately up to him.

@runcom PTAL

umohnani8 · 2017-10-02T13:50:50Z

@runcom PTAL

This package is used in authenticating a user for kpod login and can be used for authentication in kpod push, pull etc. Signed-off-by: umohnani8 <[email protected]>

runcom · 2017-10-10T13:57:43Z

LGTM

mtrmac · 2017-10-11T12:15:09Z

@umohnani8 This seems to partially undo #351, at least reintroducing setupCertificates. Could you fix that, please?

umohnani8 · 2017-10-11T13:45:16Z

@mtrmac fixing in #356

There have been redundant calls here since two ref.ref.Hostname() calls were added in aaedc64 (Implement lookaside storage for signatures for Docker registries, 2016-08-11, containers#52). At that point the two calls were separated by a dockerHostname check which could have been shifted by two lines to avoid the doubled function calls. But in f28367e (Add docker/config package to containers/image/pkg, 2017-08-29, containers#333) the dockerHostname check moved to a separate function entirely (newDockerClientWithDetails) while the Domain() calls remained together in newDockerClientFromRef. So now there is no longer any reason for the second call, and this commit drops it. Signed-off-by: W. Trevor King <[email protected]>

Use GetAuthentication directly, replacing: username, err := GetUserLoggedIn(sys, registry) with: username, _, err := GetAuthentication(sys, registry) This is just as convenient and removes the differences between lookup logic from the two functions (e.g. GetUserLoggedIn ignored DockerAuthConfig). GetUserLoggedIn originally landed in f28367e (Add docker/config package to containers/image/pkg, 2017-08-29, containers#333), and neither the commit message nor the pull request motivate the diversion from the GetAuthentication lookup logic. Signed-off-by: W. Trevor King <[email protected]>

Use GetAuthentication directly, replacing: username, err := GetUserLoggedIn(sys, registry) with: username, _, err := GetAuthentication(sys, registry) This is just as convenient and removes the differences between lookup logic from the two functions (e.g. GetUserLoggedIn ignored DockerAuthConfig). I've removed the function instead of deprecating at Miloslav's request [1]. GetUserLoggedIn originally landed in f28367e (Add docker/config package to containers/image/pkg, 2017-08-29, containers#333), and neither the commit message nor the pull request motivate the diversion from the GetAuthentication lookup logic. [1]: containers#597 (comment) Signed-off-by: W. Trevor King <[email protected]>

There have been redundant calls here since two ref.ref.Hostname() calls were added in aaedc64 (Implement lookaside storage for signatures for Docker registries, 2016-08-11, containers#52). At that point the two calls were separated by a dockerHostname check which could have been shifted by two lines to avoid the doubled function calls. But in f28367e (Add docker/config package to containers/image/pkg, 2017-08-29, containers#333) the dockerHostname check moved to a separate function entirely (newDockerClientWithDetails) while the Domain() calls remained together in newDockerClientFromRef. So now there is no longer any reason for the second call, and this commit drops it. Signed-off-by: W. Trevor King <[email protected]>

umohnani8 mentioned this pull request Aug 29, 2017

Add "kpod login" command cri-o/cri-o#810

Merged

umohnani8 force-pushed the authentication branch 2 times, most recently from 7f7067c to b4cf5ec Compare August 29, 2017 16:04

nalind reviewed Aug 29, 2017

View reviewed changes

umohnani8 force-pushed the authentication branch from b4cf5ec to f594141 Compare August 29, 2017 20:23

umohnani8 force-pushed the authentication branch from f594141 to 177d47e Compare August 30, 2017 14:49

mtrmac reviewed Aug 30, 2017

View reviewed changes

umohnani8 mentioned this pull request Sep 5, 2017

Vendor in latest containers/image cri-o/cri-o#840

Merged

umohnani8 force-pushed the authentication branch from 177d47e to 252602d Compare September 6, 2017 16:32

umohnani8 mentioned this pull request Sep 20, 2017

fixing error checking due to update in make lint containers/skopeo#422

Merged

mtrmac reviewed Sep 20, 2017

View reviewed changes

umohnani8 force-pushed the authentication branch from 5402dd9 to d5a13b3 Compare September 20, 2017 18:22

umohnani8 force-pushed the authentication branch 2 times, most recently from 61a4cb4 to fe3ab3d Compare September 21, 2017 18:51

runcom reviewed Sep 25, 2017

View reviewed changes

umohnani8 force-pushed the authentication branch from fe3ab3d to 700c99d Compare September 25, 2017 20:11

runcom reviewed Sep 26, 2017

View reviewed changes

umohnani8 force-pushed the authentication branch 2 times, most recently from b4bdfdc to eec1511 Compare September 27, 2017 14:08

mtrmac reviewed Sep 27, 2017

View reviewed changes

Add docker/config package to containers/image/pkg

f28367e

This package is used in authenticating a user for kpod login and can be used for authentication in kpod push, pull etc. Signed-off-by: umohnani8 <[email protected]>

umohnani8 force-pushed the authentication branch from eec1511 to f28367e Compare October 10, 2017 01:57

runcom merged commit 57b257d into containers:master Oct 10, 2017

wking mentioned this pull request Mar 9, 2019

docker/docker_client: Drop redundant Domain(ref.ref) call #598

Merged

mtrmac mentioned this pull request Mar 11, 2019

pkg/docker/config: Deprecate GetUserLoggedIn #597

Merged

Add authentication package to containers/image/pkg #333

Add authentication package to containers/image/pkg #333

Uh oh!

Conversation

umohnani8 commented Aug 29, 2017

Uh oh!

rhatdan commented Aug 29, 2017

Uh oh!

umohnani8 commented Aug 29, 2017

Uh oh!

mtrmac commented Aug 29, 2017

Uh oh!

umohnani8 commented Aug 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

umohnani8 commented Aug 30, 2017

Uh oh!

mtrmac commented Aug 30, 2017

Uh oh!

mtrmac commented Aug 30, 2017

Uh oh!

umohnani8 commented Aug 30, 2017

Uh oh!

mtrmac commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhatdan commented Aug 30, 2017

Uh oh!

mtrmac commented Aug 30, 2017

Uh oh!

rhatdan commented Aug 30, 2017

Uh oh!

mtrmac commented Aug 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Aug 30, 2017

Uh oh!

TomSweeneyRedHat commented Aug 30, 2017

Uh oh!

rhatdan commented Aug 31, 2017

Uh oh!

umohnani8 commented Aug 31, 2017

Uh oh!

mtrmac commented Aug 31, 2017

Uh oh!

umohnani8 commented Sep 20, 2017

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

runcom commented Aug 30, 2017 •

edited

Loading

mtrmac commented Aug 30, 2017 •

edited

Loading

mtrmac commented Sep 20, 2017 •

edited by runcom

Loading