Add support for path based registry in login/logout #659

saschagrunert · 2021-07-01T11:20:11Z

Login and logout now supports paths in auths.json.

saschagrunert · 2021-07-01T11:26:02Z

Interesting behavior for login: Because of the normalization of the registry in
https://github.com/containers/image/blob/bca1df8/pkg/docker/config/config.go#L643

We now report a credential re-use when logging into quay.io/something by using an auth.json of:

{
  "auths": {
    "quay.io/saschagrunert": { "auth": "…" }
  }
}

Explanation: As last resort we normalize quay.io/saschagrunert to quay.io for the entry as well as the provided reference, which means they match in the end in:
https://github.com/containers/image/blob/bca1df8/pkg/docker/config/config.go#L647

rhatdan · 2021-07-01T12:37:22Z

@vrothberg @mtrmac PTAL

vrothberg

Please elaborate a bit more in the commit message about the design and changes. This makes it easier to match intention against changes.

I just quickly skimmed as I don't have time for a detailed review but we need to consider backwards compat.

Currently for all tools login registry.com/foo/bar creates credentials for registry.com. We need to preserve that behavior to prevent regressions. Storing the entire path should be opt-in via LoginOptions.

pkg/auth/auth.go

mtrmac

First quick skim:

See containers/image#1288 (comment) : if the user configures credential helpers only without auth files, this might not be supportable, and we should fail somewhat cleanly.

We now report a credential re-use when logging into quay.io/something by using an auth.json of:

Hum, that looks like a design flaw at a first glance. Something for me to think about more.

Currently for all tools login registry.com/foo/bar creates credentials for registry.com. We need to preserve that behavior to prevent regressions.

Ouch… Yes we do. Do we also want to deprecate and warn on the old behavior? I have no idea how widespread the use of such input is.

pkg/auth/auth.go

mtrmac · 2021-07-01T13:21:10Z

pkg/auth/auth.go

-		if err := docker.CheckAuth(ctx, systemContext, authConfig.Username, authConfig.Password, server); err == nil {
-			fmt.Fprintln(opts.Stdout, "Existing credentials are valid. Already logged in to", server)
+		fmt.Fprintln(opts.Stdout, "Authenticating with existing credentials for", server)
+		if err := docker.CheckAuth(ctx, systemContext, authConfig.Username, authConfig.Password, serverDomain); err == nil {


Shouldn’t this check the relevant repo, not just the general server? But then again, if there were credentials for quay.io and the user explicitly wanted to create credentials for quay.io/repo (or docker.io/ns vs docker.io/ns/repo), what should happen?

That means another new function in c/image … and thinking what exactly we should be checking for.

Still outstanding, though giving up might be the right answer, short-term at least.

… but we need to make sure that it is possible to change credentials from valid-for-registry-but-not-repo to valid-for-repo. Right now that seems possible but the user must explicitly specify --username; is that good enough?

Should be try both, the full key as well as the domain?

I think testing only key would make most sense. If we wanted to try domain as well to tell the user “you asked to login to quay.io/mitr/test-server but you already have credentials for quay.io”, wouldn’t we want to do the same check for the intermediate quay.io/mitr namespace as well?

That line of thought is not clearly nonsensical, but it’s also fiddly enough that doing the simplest thing and only checking key seems cleaner to me.

pkg/auth/auth.go

vrothberg · 2021-07-02T07:54:05Z

Ouch… Yes we do. Do we also want to deprecate and warn on the old behavior?

I have no idea how widespread the use of such input is.

Same here. I browsed a number of docs on Podman & Docker and couldn't spot examples using anything else than the registry root. Maybe I am overly cautious? The man pages of buildah/podman/skopeo all state podman login [options] [registry]. The behavior of trimming paths etc. isn't documented afaics.

mtrmac · 2021-07-02T13:29:33Z

Ouch… Yes we do. Do we also want to deprecate and warn on the old behavior?

I have no idea how widespread the use of such input is.

Same here. I browsed a number of docs on Podman & Docker and couldn't spot examples using anything else than the registry root.

git’s history is a dangerously powerful tool — this originally comes from containers/podman#1905 , which links to a user report. One user report, sure.

mtrmac · 2021-07-02T13:32:32Z

We now report a credential re-use when logging into quay.io/something by using an auth.json of:

Hum, that looks like a design flaw at a first glance. Something for me to think about more.

Looking back at https://issues.redhat.com/browse/RUN-1246?focusedCommentId=16344129&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16344129 , and the Feb 2016 behavior change, I think what makes most sense is for c/image, in the non-legacy format case, to interpret hostPort/repo (without a schema prefix) as only a repo credential, i.e. not match it for other-repo lookups on the same registry.

Does that sound reasonable?

pkg/auth/auth.go

vrothberg · 2021-07-06T08:55:18Z

Does that sound reasonable?

Can you give an example?

mtrmac · 2021-07-07T10:55:39Z

For auth keys in a non-legacyFormat file:

example.com matches anything in example.com (works that way now)
https://example.com/v1 matches anything in example.com (works that way now)
any other URL-like string with example.com (I guess anything starting with http:// or https://) matches anything in example.com (works that way now)
example.com/foo currently matches example.com/foo first, but then is normalized to example.com and matches everything. The proposal is that such values (with no protocol prefix in a non-legacy file) are not normalized and only match the example.com/foo namespace.

Alternatively, we could make the “legacy” switch a tri-state: .dockercfg/.docker/config.json/auth.json, and not normalize example.com/foo only in the auth.json case. I’m not sure there’s a strong reason making the config.json/auth.json distinction now (are we certain none of Podman/Buildah login has ever allowed writing a non-hostPort key?).

vrothberg · 2021-07-07T11:42:25Z

Does that sound reasonable?

Yes, that sounds reasonable. Thank you!

[...] are we certain none of Podman/Buildah login has ever allowed writing a non-hostPort key?

No idea.

pkg/auth/auth.go

mtrmac · 2021-07-09T12:22:50Z

pkg/auth/auth.go

-		if err := docker.CheckAuth(ctx, systemContext, authConfig.Username, authConfig.Password, server); err == nil {
-			fmt.Fprintln(opts.Stdout, "Existing credentials are valid. Already logged in to", server)
+		fmt.Fprintln(opts.Stdout, "Authenticating with existing credentials for", server)
+		if err := docker.CheckAuth(ctx, systemContext, authConfig.Username, authConfig.Password, serverDomain); err == nil {


Still outstanding, though giving up might be the right answer, short-term at least.

pkg/auth/auth.go

mtrmac · 2021-07-09T12:28:26Z

pkg/auth/auth.go

-		if err := docker.CheckAuth(ctx, systemContext, authConfig.Username, authConfig.Password, server); err == nil {
-			fmt.Fprintln(opts.Stdout, "Existing credentials are valid. Already logged in to", server)
+		fmt.Fprintln(opts.Stdout, "Authenticating with existing credentials for", server)
+		if err := docker.CheckAuth(ctx, systemContext, authConfig.Username, authConfig.Password, serverDomain); err == nil {


… but we need to make sure that it is possible to change credentials from valid-for-registry-but-not-repo to valid-for-repo. Right now that seems possible but the user must explicitly specify --username; is that good enough?

mtrmac

I think we will also need #659 (comment) .

pkg/auth/auth.go

pkg/auth/cli.go

pkg/auth/auth.go

As mentioned in containers/common#659 (comment), we now do not normalize the registry when being in non legacy format. This means we do not match "quay.io/foo" if "quay.io/bar" is provided as credentials, because both would be normalized to "quay.io" before this patch. We also add a new corner case where auth.json has been external modified to contain an entry like "https://quay.io/v1/", which now also matches if "quay.io/foo" has been provided via the credentials. Signed-off-by: Sascha Grunert <[email protected]>

rhatdan · 2021-07-20T12:57:37Z

/approve
@mtrmac @vrothberg PTAL

openshift-ci · 2021-07-20T12:57:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

As mentioned in containers/common#659 (comment), we now do not normalize the registry when being in non legacy format. This means we do not match "quay.io/foo" if "quay.io/bar" is provided as credentials, because both would be normalized to "quay.io" before this patch. We also add a new corner case where auth.json has been external modified to contain an entry like "https://quay.io/v1/", which now also matches if "quay.io/foo" has been provided via the credentials. Signed-off-by: Sascha Grunert <[email protected]>

rhatdan · 2021-07-21T10:10:09Z

pkg/auth/cli.go

@@ -48,6 +50,7 @@ func GetLoginFlags(flags *LoginOptions) *pflag.FlagSet {
 	fs.StringVarP(&flags.Username, "username", "u", "", "Username for registry")
 	fs.BoolVar(&flags.StdinPassword, "password-stdin", false, "Take the password from stdin")
 	fs.BoolVar(&flags.GetLoginSet, "get-login", false, "Return the current login user for the registry")
+	fs.BoolVar(&flags.AcceptRepositories, "accept-repositories", false, "Allow namespaces or repositories rather than just registries")


Who is ever going to do this? Shouldn't this be default and not an option?

I think the migration strategy would be to introduce it as optional flag which can be later on turned on by default. What are your thoughts on that @vrothberg @mtrmac?

This was discussed in this PR already around #659 (comment) : At least one person did do this in containers/podman#1690 .

I think any new callers of c/common/pkg/auth, if any (i.e. new tools) should fairly clearly only use the new semantics (reject URLs and not ignore the within-registry paths).

As for the three current callers… just flipping this flag feels problematic given the known users, and that we would change the semantics of some inputs. Shouldn’t there be at least a period where the tools warn but still accept the old syntax?

Note: Even after we flip the flag to default on, we will have to silently accept it afterwards. That’s a bit of an argument in favor of not having the flag in the first place — I personally don’t think it’s a strong enough reason alone, though.

I would rather have it support both formats and not blow up in the users face. If at some time in the future Podman 4.0, you want to block the old format we could to it then. No reason for humans to have to deal with this.

We are changing the semantics of the quay.io/foo:label input as used by that user above. There’s no way for users to “not deal with this”: either we ask for an opt-in flag to use the new semantics (as this PR does), or we change the existing semantics and potentially break users.

[We could continue to silently support https://host.com/and/this/is/ignored indefinitely, though, at some risk of confusing users who might think the path means something.]

As mentioned in containers/common#659 (comment), we now do not normalize the registry when being in non legacy format. This means we do not match "quay.io/foo" if "quay.io/bar" is provided as credentials, because both would be normalized to "quay.io" before this patch. We also add a new corner case where auth.json has been external modified to contain an entry like "https://quay.io/v1/", which now also matches if "quay.io/foo" has been provided via the credentials. Signed-off-by: Sascha Grunert <[email protected]>

saschagrunert · 2021-07-21T12:55:42Z

Rebased on top of the latest c/image changes and changed docker.CheckAuth to use key instead of registry.

pkg/auth/auth.go

mtrmac · 2021-07-22T17:30:34Z

pkg/auth/cli.go

@@ -48,6 +50,7 @@ func GetLoginFlags(flags *LoginOptions) *pflag.FlagSet {
 	fs.StringVarP(&flags.Username, "username", "u", "", "Username for registry")
 	fs.BoolVar(&flags.StdinPassword, "password-stdin", false, "Take the password from stdin")
 	fs.BoolVar(&flags.GetLoginSet, "get-login", false, "Return the current login user for the registry")
+	fs.BoolVar(&flags.AcceptRepositories, "accept-repositories", false, "Allow namespaces or repositories rather than just registries")


We are changing the semantics of the quay.io/foo:label input as used by that user above. There’s no way for users to “not deal with this”: either we ask for an opt-in flag to use the new semantics (as this PR does), or we change the existing semantics and potentially break users.

[We could continue to silently support https://host.com/and/this/is/ignored indefinitely, though, at some risk of confusing users who might think the path means something.]

pkg/auth/auth.go

We now add a new configuration option to opt-in for path based registry authentication in containers-auth.json. This affects login and logout, which means if the option is enabled we can now use `my-registry.local/path/to/image` to save or remove the credentials from the auth.json. If the option is enabled, then we enforce a stricter validation of the input. For example it is not allowed input `http[s]://` prefixed keys. Signed-off-by: Sascha Grunert <[email protected]>

mtrmac

/lgtm

Thanks!

openshift-ci · 2021-07-23T10:34:05Z

@mtrmac: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Thanks!

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

saschagrunert · 2021-07-23T10:38:06Z

@rhatdan @vrothberg PTAL and /lgtm

rhatdan · 2021-07-24T08:43:21Z

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress label Jul 1, 2021

saschagrunert mentioned this pull request Jul 1, 2021

WIP: Test c/common #659 containers/podman#10834

Closed

saschagrunert force-pushed the login-logout-registry-path branch from 1bdd9ac to 8c4453f Compare July 1, 2021 11:41

vrothberg reviewed Jul 1, 2021

View reviewed changes

pkg/auth/auth.go Outdated Show resolved Hide resolved

mtrmac reviewed Jul 1, 2021

View reviewed changes

openshift-ci bot added the needs-rebase label Jul 2, 2021

saschagrunert force-pushed the login-logout-registry-path branch from 8c4453f to 72f28e0 Compare July 5, 2021 08:50

openshift-ci bot removed the needs-rebase label Jul 5, 2021

saschagrunert commented Jul 5, 2021

View reviewed changes

pkg/auth/auth.go Outdated Show resolved Hide resolved

saschagrunert force-pushed the login-logout-registry-path branch from 72f28e0 to a6a5a1e Compare July 5, 2021 08:55

saschagrunert changed the title ~~WIP: Add support for path based registry in login/logout~~ Add support for path based registry in login/logout Jul 5, 2021

openshift-ci bot removed the do-not-merge/work-in-progress label Jul 5, 2021

mtrmac reviewed Jul 9, 2021

View reviewed changes

pkg/auth/auth.go Outdated Show resolved Hide resolved

pkg/auth/auth.go Outdated Show resolved Hide resolved

pkg/auth/auth.go Show resolved Hide resolved

pkg/auth/cli.go Outdated Show resolved Hide resolved

pkg/auth/auth.go Show resolved Hide resolved

openshift-ci bot added the needs-rebase label Jul 13, 2021

saschagrunert mentioned this pull request Jul 16, 2021

Support updating registry credentials scoped to namespaces/repos containers/image#1288

Merged

saschagrunert force-pushed the login-logout-registry-path branch 3 times, most recently from f108447 to 72429d7 Compare July 19, 2021 10:26

saschagrunert commented Jul 19, 2021

View reviewed changes

pkg/auth/auth.go Outdated Show resolved Hide resolved

saschagrunert force-pushed the login-logout-registry-path branch from 72429d7 to f91a5bf Compare July 19, 2021 10:44

saschagrunert mentioned this pull request Jul 20, 2021

Remove GetCredentials deprecation containers/image#1305

Merged

saschagrunert force-pushed the login-logout-registry-path branch 2 times, most recently from 79ab5f6 to d7d3944 Compare July 20, 2021 10:17

openshift-ci bot removed the needs-rebase label Jul 20, 2021

saschagrunert mentioned this pull request Jul 20, 2021

WIP: Test c/common #659 containers/podman#10990

Closed

saschagrunert force-pushed the login-logout-registry-path branch from d7d3944 to 0a891b5 Compare July 20, 2021 10:52

openshift-ci bot added the approved label Jul 20, 2021

rhatdan reviewed Jul 21, 2021

View reviewed changes

saschagrunert force-pushed the login-logout-registry-path branch from 0a891b5 to 23b8aae Compare July 21, 2021 12:54

mtrmac reviewed Jul 22, 2021

View reviewed changes

saschagrunert force-pushed the login-logout-registry-path branch from 23b8aae to f8136e7 Compare July 23, 2021 07:41

saschagrunert mentioned this pull request Jul 23, 2021

build(deps): bump github.com/containers/image/v5 from 5.13.2 to 5.14.0 #686

Closed

mtrmac reviewed Jul 23, 2021

View reviewed changes

openshift-ci bot assigned rhatdan Jul 24, 2021

openshift-ci bot added the lgtm label Jul 24, 2021

openshift-merge-robot merged commit 5257b4d into containers:main Jul 24, 2021

saschagrunert deleted the login-logout-registry-path branch July 26, 2021 06:48

saschagrunert mentioned this pull request Jul 29, 2021

Add support for repositories (registry namespaced) on login/logout and push/pull containers/podman#11054

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for path based registry in login/logout #659

Add support for path based registry in login/logout #659

saschagrunert commented Jul 1, 2021

saschagrunert commented Jul 1, 2021 •

edited

Loading

rhatdan commented Jul 1, 2021

vrothberg left a comment

mtrmac left a comment

mtrmac Jul 1, 2021

mtrmac Jul 9, 2021

mtrmac Jul 9, 2021

saschagrunert Jul 19, 2021

mtrmac Jul 19, 2021

vrothberg commented Jul 2, 2021

mtrmac commented Jul 2, 2021

mtrmac commented Jul 2, 2021

vrothberg commented Jul 6, 2021

mtrmac commented Jul 7, 2021 •

edited

Loading

vrothberg commented Jul 7, 2021

mtrmac Jul 9, 2021

mtrmac Jul 9, 2021

mtrmac left a comment

rhatdan commented Jul 20, 2021

openshift-ci bot commented Jul 20, 2021

rhatdan Jul 21, 2021

saschagrunert Jul 21, 2021

mtrmac Jul 21, 2021

rhatdan Jul 21, 2021

mtrmac Jul 22, 2021

saschagrunert commented Jul 21, 2021

mtrmac Jul 22, 2021

mtrmac left a comment

openshift-ci bot commented Jul 23, 2021

saschagrunert commented Jul 23, 2021

rhatdan commented Jul 24, 2021

Add support for path based registry in login/logout #659

Add support for path based registry in login/logout #659

Conversation

saschagrunert commented Jul 1, 2021

saschagrunert commented Jul 1, 2021 • edited Loading

rhatdan commented Jul 1, 2021

vrothberg 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

vrothberg commented Jul 2, 2021

mtrmac commented Jul 2, 2021

mtrmac commented Jul 2, 2021

vrothberg commented Jul 6, 2021

mtrmac commented Jul 7, 2021 • edited Loading

vrothberg commented Jul 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

rhatdan commented Jul 20, 2021

openshift-ci bot commented Jul 20, 2021

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

saschagrunert commented Jul 21, 2021

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 23, 2021

saschagrunert commented Jul 23, 2021

rhatdan commented Jul 24, 2021

saschagrunert commented Jul 1, 2021 •

edited

Loading

mtrmac commented Jul 7, 2021 •

edited

Loading