Support updating registry credentials scoped to namespaces/repos #1288

saschagrunert · 2021-07-01T09:06:26Z

We now error on login if repositories or repository namespaces are used
for other credential helpers than the AuthenticationFileHelper. On
logout we ignore them and debug log a warning that nothing has been
modified.

The functions SetCredentials (for login) as well as
RemoveAuthentication (for logout) already feature support for path
based registries for the AuthenticationFileHelper. This patch adds
unit tests to ensure that the support will not break in the future.

Refers to #1276

saschagrunert · 2021-07-01T09:09:03Z

I was checking the functionality during my follow-up mentioned in #1278 (review). Looks like we're mostly set in c/image, whereas c/common would require some slight adaption to make login/logout work correctly.

rhatdan · 2021-07-01T12:38:08Z

LGTM

mtrmac · 2021-07-01T13:06:49Z

The functions SetCredentials (for login) as well as
RemoveAuthentication (for logout) already feature support for path
based registries. This patch adds unit tests to ensure that the support
will not break in the future.

That’s not good, because the functions blindly send such values to credential helpers, which may work very badly (see https://issues.redhat.com/browse/RUN-1246?focusedCommentId=16344129&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16344129 ).

Also, if we are not adding new functions, the documentation of SetCredentials/RemoveAuthentication should be expanded to document that new input is allowed (well, some new input; do we want to allow tagged/digested references?).

WRT credential helpers, for now it would be consistent to just refuse to write namespaced credentials to them (and RemoveAuthentication then needs to account for that by not failing if trying to remove something that we would never add/look up). Eventually we might need to either propose fixes to the helpers, or maintain a list of known good/bad helpers…

saschagrunert · 2021-07-01T13:51:47Z

@mtrmac would it be better to normalize the input registry for every credentials helper other than AuthenticationFileHelper or test the input registry for being namespaced and fail in that case if the helper is not the AuthenticationFileHelper?

mtrmac · 2021-07-01T14:00:48Z

If the user requests to record a login for quay.io/foo and later for quay.io/bar, it wouldn’t do to store both to quay.io.

IIRC from a quick skim, we already ~silently continue if a helper refuses data; so I think we can continue to do that and just refuse to even try with non-AuthenticationFileHelper helpers.

Non-blocking? It might be useful to provide a typed error for such a case, or at least to special-case the multiErr handling here, so that the * login commands can report a meaningful “namespaced credentials are not supported with credential helpers” error.

saschagrunert · 2021-07-01T14:35:34Z

Added a new error type ErrNamespacedRegistry if the registry is namespaced and a non auth.json credentials helper is being used.

TomSweeneyRedHat · 2021-07-01T17:21:06Z

@saschagrunert looks like you need a rebase here.

saschagrunert · 2021-07-02T09:12:50Z

The branch is not on the latest master but I don't see any conflicts :)

vrothberg

I have troubles understanding the meaning of "namespaced registry". It mostly relates to using the "namespaced" adjective (all registries have namespaces) but also c/image usually uses "repo[sitory]" rather than "namespace".

I am also a bit worried of the implications of the new error. Can we be certain that registry+repo/path is not supported for creds helpers? I tried looking in the (scarse) docs (https://pkg.go.dev/github.com/docker/[email protected]/credentials#Credentials) but could not find much info on what a "ServerURL" is.

mtrmac · 2021-07-02T20:46:35Z

I have troubles understanding the meaning of "namespaced registry". It mostly relates to using the "namespaced" adjective (all registries have namespaces) but also c/image usually uses "repo[sitory]" rather than "namespace".

Yeah the key is not a “registry”, so a “namespaces registry” is not ideal. We should think of a name and use it consistently throughout.

host[:port] = registry
host[:port]/[ns1/…/nsX/]name = repository
host[:port]/[ns1/…]/nsX (meant as a parent of …/nsX/name) = namespace; sometimes namespaces might also include leaf repositories (?)
any of the above = ???

containers-registries.conf(5) just define a similar syntax (but with leading wildcards) for the prefix field, and then talks about “like the prefix field”.

containers-policy.json(5) talks about “scopes” (but in a more generic transport-dependent way).

I don’t have a good suggestion. “Credentials scope”? “Credential [storage] key”? Both are pretty bad.

I am also a bit worried of the implications of the new error. Can we be certain that registry+repo/path is not supported for creds helpers? I tried looking in the (scarse) docs (https://pkg.go.dev/github.com/docker/[email protected]/credentials#Credentials) but could not find much info on what a "ServerURL" is.

See the various helper implementations in that repo:

osxkeychain parses the serverURL into protocol://host:port/path, and stores all of that (but not possible URL query/fragment parts)
pass and secretservice seemingly treat serverURL as an opaque byte sequence with no parsing/normalization
wincred does approximate matching where basically only the hostname must match.

For now this is already ~decided by #1278 , which does not call helpers for any non-registry keys, and this just does the same thing on the write path; if we don’t look for per-repo credentials in helpers, it doesn’t make much sense to report success on storing the credentials in those helpers. Though we can certainly revisit, and now before a release would be the best time :)

mtrmac

To be explicit, I’m treating this PR as a full “add support for editing namespaced credentials” feature addition. If that’s a bad assumption, please correct me.

pkg/docker/config/config.go

pkg/docker/config/config_test.go

pkg/docker/config/config.go

saschagrunert · 2021-07-05T08:13:05Z

Alright, just a few points are left open, mostly about the term "namespaced" and "registry". How about:

changing "registry" to "key", mentioning in the docs that it should be a "registry" for any credentials helper than the auth file, which supports full "repositories"
chaning the ErrNamespacedRegistry to ErrRepositoryPath or ErrUnsupportedRepository, …

mtrmac

So that it isn’t forgotten, we will need a solution for namespaces-only logins, maybe containers/common#659 (comment) .

pkg/docker/config/config.go

mtrmac · 2021-07-09T13:21:55Z

Alright, just a few points are left open, mostly about the term "namespaced" and "registry". How about:

changing "registry" to "key",

*shrug* works for me. @vrothberg ?

mentioning in the docs that it should be a "registry" for any credentials helper than the auth file, which supports full "repositories"

repositories and repository namespaces

chaning the ErrNamespacedRegistry to ErrRepositoryPath or ErrUnsupportedRepository, …

If nothing is checking for that value (as is currently the case), maybe we can just not provide an API and this specific instance goes away (OTOH naming still affects most of the PR throughout anyway).

vrothberg · 2021-07-09T13:57:51Z

SGTM

saschagrunert · 2021-07-12T08:45:10Z

Implemented the review suggestions, PTAL @mtrmac @vrothberg

mtrmac

So that it isn’t forgotten, we will need a solution for namespaces-only logins, maybe containers/common#659 (comment) .

docs/containers-auth.json.5.md

pkg/docker/config/config.go

saschagrunert · 2021-07-13T07:51:50Z

@mtrmac thank you for the review! I addressed your requested changes, PTAL again.

docs/containers-auth.json.5.md

pkg/docker/config/config.go

TomSweeneyRedHat · 2021-07-14T00:05:10Z

Couple small nits and you need to rebase @saschagrunert

We now error on login if repositories or repository namespaces are used for other credential helpers than the `AuthenticationFileHelper`. On logout we ignore them and debug log a warning that nothing has been modified. The functions `SetCredentials` (for login) as well as `RemoveAuthentication` (for logout) already feature support for path based registries for the `AuthenticationFileHelper`. This patch adds unit tests to ensure that the support will not break in the future. Signed-off-by: Sascha Grunert <[email protected]>

saschagrunert · 2021-07-14T07:19:51Z

Rebased and addressed @TomSweeneyRedHat's comments.

mtrmac

LGTM. Thanks!

Are you planning to work on containers/common#659 (comment) as well, to make everything fit together?

mtrmac · 2021-07-16T14:35:17Z

pkg/docker/config/config.go

 	return nameParts[0]
 }

+// stripScheme striped the http|https scheme from the provided URL.
+func stripScheme(url string) string {


(Non-blocking: This can be inlined back into its only caller.)

TomSweeneyRedHat · 2021-07-16T15:02:40Z

LGTM
The skopeo test failed, looks like a flake trying to setup a connection to a registry.

saschagrunert · 2021-07-16T17:53:00Z

LGTM. Thanks!

Are you planning to work on containers/common#659 (comment) as well, to make everything fit together?

Yes, I’ll continue working on it next week. :)

saschagrunert force-pushed the login-logout-tests branch from ce03f42 to 8fd6290 Compare July 1, 2021 09:17

saschagrunert force-pushed the login-logout-tests branch 2 times, most recently from a12697b to ecdb320 Compare July 1, 2021 14:31

saschagrunert changed the title ~~Add test cases for path based registry login and logout~~ Error on namespaced registries for credential helpers Jul 2, 2021

vrothberg reviewed Jul 2, 2021

View reviewed changes

mtrmac reviewed Jul 2, 2021

View reviewed changes

saschagrunert force-pushed the login-logout-tests branch 3 times, most recently from 33a364e to e1a9b66 Compare July 5, 2021 08:22

mtrmac reviewed Jul 9, 2021

View reviewed changes

pkg/docker/config/config.go Outdated Show resolved Hide resolved

pkg/docker/config/config.go Outdated Show resolved Hide resolved

pkg/docker/config/config.go Outdated Show resolved Hide resolved

pkg/docker/config/config.go Outdated Show resolved Hide resolved

mtrmac changed the title ~~Error on namespaced registries for credential helpers~~ Support updating registry credentials scoped to namespaces/repos Jul 10, 2021

saschagrunert force-pushed the login-logout-tests branch 5 times, most recently from d41f449 to 0e01664 Compare July 12, 2021 08:40

mtrmac reviewed Jul 12, 2021

View reviewed changes

saschagrunert force-pushed the login-logout-tests branch from 0e01664 to 6c25f71 Compare July 13, 2021 07:49

TomSweeneyRedHat reviewed Jul 13, 2021

View reviewed changes

docs/containers-auth.json.5.md Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Jul 13, 2021

View reviewed changes

pkg/docker/config/config.go Outdated Show resolved Hide resolved

saschagrunert force-pushed the login-logout-tests branch from 6c25f71 to 2935f06 Compare July 14, 2021 07:19

mtrmac reviewed Jul 16, 2021

View reviewed changes

rhatdan merged commit d695b98 into containers:main Jul 16, 2021

saschagrunert deleted the login-logout-tests branch July 16, 2021 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support updating registry credentials scoped to namespaces/repos #1288

Support updating registry credentials scoped to namespaces/repos #1288

saschagrunert commented Jul 1, 2021 •

edited

Loading

saschagrunert commented Jul 1, 2021

rhatdan commented Jul 1, 2021

mtrmac commented Jul 1, 2021

saschagrunert commented Jul 1, 2021

mtrmac commented Jul 1, 2021

saschagrunert commented Jul 1, 2021

TomSweeneyRedHat commented Jul 1, 2021

saschagrunert commented Jul 2, 2021

vrothberg left a comment

mtrmac commented Jul 2, 2021 •

edited

Loading

mtrmac left a comment

saschagrunert commented Jul 5, 2021 •

edited

Loading

mtrmac left a comment

mtrmac commented Jul 9, 2021

vrothberg commented Jul 9, 2021

saschagrunert commented Jul 12, 2021

mtrmac left a comment

saschagrunert commented Jul 13, 2021

TomSweeneyRedHat commented Jul 14, 2021

saschagrunert commented Jul 14, 2021

mtrmac left a comment

mtrmac Jul 16, 2021

TomSweeneyRedHat commented Jul 16, 2021

saschagrunert commented Jul 16, 2021

Support updating registry credentials scoped to namespaces/repos #1288

Support updating registry credentials scoped to namespaces/repos #1288

Conversation

saschagrunert commented Jul 1, 2021 • edited Loading

saschagrunert commented Jul 1, 2021

rhatdan commented Jul 1, 2021

mtrmac commented Jul 1, 2021

saschagrunert commented Jul 1, 2021

mtrmac commented Jul 1, 2021

saschagrunert commented Jul 1, 2021

TomSweeneyRedHat commented Jul 1, 2021

saschagrunert commented Jul 2, 2021

vrothberg left a comment

Choose a reason for hiding this comment

mtrmac commented Jul 2, 2021 • edited Loading

mtrmac left a comment

Choose a reason for hiding this comment

saschagrunert commented Jul 5, 2021 • edited Loading

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Jul 9, 2021

vrothberg commented Jul 9, 2021

saschagrunert commented Jul 12, 2021

mtrmac left a comment

Choose a reason for hiding this comment

saschagrunert commented Jul 13, 2021

TomSweeneyRedHat commented Jul 14, 2021

saschagrunert commented Jul 14, 2021

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Jul 16, 2021

Choose a reason for hiding this comment

TomSweeneyRedHat commented Jul 16, 2021

saschagrunert commented Jul 16, 2021

saschagrunert commented Jul 1, 2021 •

edited

Loading

mtrmac commented Jul 2, 2021 •

edited

Loading

saschagrunert commented Jul 5, 2021 •

edited

Loading