Normalize auth key before calling `SetAuthentication` #11430

saschagrunert · 2021-09-03T07:44:36Z

Recent changes in c/image caused the SetAuthentication API to be more
restrictive in terms of validating the key (server) input. To ensure
that manually modified or entries in ~/.docker/config.json still work,
we now strip the leading http[s]:// prefix.

Fixes #11235

rhatdan · 2021-09-03T10:55:54Z

Shouldn't this be in containers/common/pkg/auth, then Buildah and other tools like Skopeo and CRI-O could use it.

vrothberg · 2021-09-03T11:05:30Z

Shouldn't this be in containers/common/pkg/auth, then Buildah and other tools like Skopeo and CRI-O could use it.

I don't think so, it's used for the REST API.

pkg/auth/auth.go

vrothberg

LGTM

golint isn't happy at the moment. @mtrmac PTAL

mtrmac

[NO TESTS NEEDED]

???

We have user reports and reproducers. Why not turn that into a “we didn’t think about it before, and we need to record this case for the future” unit test?

mtrmac · 2021-09-03T16:49:02Z

pkg/auth/auth.go

+// the resulting registry.
+func stripHTTPSPrefix(server string) string {
+	stripped := strings.TrimPrefix(server, "http://")
+	return strings.TrimPrefix(stripped, "https://")


This turns the proverbial https://index.docker.io/v1/ into index.docker.io/v1/, a repo-namespaced value (and an invalid one at that.)

Added normalization cases for docker registries as well.

This is still so different from https://github.com/containers/image/blob/4fcb52359b759a276a760fb28c939c357b3aea48/pkg/docker/config/config.go#L732 . Why?

http{,s}://-prefixed entries are not namespaced; but it is supposed to be possible to have a docker.io/vendor namespaced entry.

mtrmac · 2021-09-03T16:52:26Z

We have user reports and reproducers. Why not turn that into a “we didn’t think about it before, and we need to record this case for the future” unit test?

It could even be interesting to have a test that provides a file directly to c/image/docker/config (via AuthFilePath), and data via this code path, and ensures that the lookups return the same data.

Recent changes in c/image caused the `SetAuthentication` API to be more restrictive in terms of validating the `key` (`server`) input. To ensure that manually modified or entries in `~/.docker/config.json` still work, we now strip the leading `http[s]://` prefix. Fixes containers#11235 Signed-off-by: Sascha Grunert <[email protected]>

saschagrunert · 2021-09-09T07:17:57Z

[NO TESTS NEEDED]

???

We have user reports and reproducers. Why not turn that into a “we didn’t think about it before, and we need to record this case for the future” unit test?

Sure, I added unit tests around the covered use cases.

openshift-ci · 2021-09-09T07:22:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [saschagrunert]

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

rhatdan · 2021-09-09T10:41:35Z

/lgtm

mtrmac · 2021-09-09T13:57:03Z

pkg/auth/auth_test.go

+			name:             "normalize https:// prefix",
+			server:           "http://my-registry.local/username",
+			shouldErr:        false,
+			expectedContains: `"my-registry.local/username":`,


This is inconsistent with https://github.com/containers/image/blob/4fcb52359b759a276a760fb28c939c357b3aea48/pkg/docker/config/config_test.go#L938 .

mtrmac · 2021-09-09T13:57:41Z

pkg/auth/auth_test.go

+			name:             "normalize docker registry with https prefix",
+			server:           "http://index.docker.io/v1/",
+			shouldErr:        false,
+			expectedContains: `"index.docker.io":`,


I’d expect the key to be docker.io here, although ultimately it works.

mtrmac · 2021-09-09T13:58:47Z

pkg/auth/auth_test.go

+			name:             "normalize docker registry without https prefix",
+			server:           "docker.io/v2/",
+			shouldErr:        false,
+			expectedContains: `"docker.io":`,


This should be docker.io/v2, or rejected as invalid due to the trailing slash. (it never matches in actual files).

mtrmac · 2021-09-11T04:08:20Z

containers/image#1373 could help with this problem by making GetAllCredentials return normalized auth keys (in particular, docker.io for the old https://index.docker.io/v1/ key values), which AFAICS changes what clients send over the remote API.

But the server should still be doing its own normalization, at the very least to support older clients, incl. possibly (?) projectatomic/docker .

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2021

saschagrunert mentioned this pull request Sep 3, 2021

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

Closed

saschagrunert force-pushed the normalize-key branch from 62a698e to e5a4a65 Compare September 3, 2021 08:38

vrothberg reviewed Sep 3, 2021

View reviewed changes

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

vrothberg mentioned this pull request Sep 3, 2021

Error: credentials key has http[s]://prefix #11435

Closed

saschagrunert force-pushed the normalize-key branch 2 times, most recently from e7bbc5a to 2276004 Compare September 3, 2021 11:39

vrothberg reviewed Sep 3, 2021

View reviewed changes

saschagrunert force-pushed the normalize-key branch from 2276004 to e0b32c2 Compare September 3, 2021 12:06

mtrmac reviewed Sep 3, 2021

View reviewed changes

saschagrunert force-pushed the normalize-key branch from e0b32c2 to bbdaf83 Compare September 9, 2021 07:17

openshift-ci bot assigned rhatdan Sep 9, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021

openshift-merge-robot merged commit e73574a into containers:main Sep 9, 2021

saschagrunert deleted the normalize-key branch September 9, 2021 11:43

mtrmac reviewed Sep 9, 2021

View reviewed changes

mtrmac mentioned this pull request Sep 11, 2021

Namespaced credentials key enhancements containers/image#1373

Merged

mtrmac mentioned this pull request Sep 11, 2021

Fix HTTP credentials passing #11538

Merged

edsantiago mentioned this pull request Sep 30, 2021

sdnotify test: accept MAINPID anywhere #11815

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize auth key before calling `SetAuthentication` #11430

Normalize auth key before calling `SetAuthentication` #11430

saschagrunert commented Sep 3, 2021 •

edited

Loading

rhatdan commented Sep 3, 2021

vrothberg commented Sep 3, 2021

vrothberg left a comment

mtrmac left a comment

mtrmac Sep 3, 2021

saschagrunert Sep 9, 2021

mtrmac Sep 9, 2021

mtrmac commented Sep 3, 2021

saschagrunert commented Sep 9, 2021

openshift-ci bot commented Sep 9, 2021

rhatdan commented Sep 9, 2021

mtrmac Sep 9, 2021

mtrmac Sep 9, 2021

mtrmac Sep 9, 2021

mtrmac commented Sep 11, 2021

Normalize auth key before calling SetAuthentication #11430

Normalize auth key before calling SetAuthentication #11430

Conversation

saschagrunert commented Sep 3, 2021 • edited Loading

rhatdan commented Sep 3, 2021

vrothberg commented Sep 3, 2021

vrothberg left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Sep 3, 2021

Choose a reason for hiding this comment

saschagrunert Sep 9, 2021

Choose a reason for hiding this comment

mtrmac Sep 9, 2021

Choose a reason for hiding this comment

mtrmac commented Sep 3, 2021

saschagrunert commented Sep 9, 2021

openshift-ci bot commented Sep 9, 2021

rhatdan commented Sep 9, 2021

mtrmac Sep 9, 2021

Choose a reason for hiding this comment

mtrmac Sep 9, 2021

Choose a reason for hiding this comment

mtrmac Sep 9, 2021

Choose a reason for hiding this comment

mtrmac commented Sep 11, 2021

Normalize auth key before calling `SetAuthentication` #11430

Normalize auth key before calling `SetAuthentication` #11430

saschagrunert commented Sep 3, 2021 •

edited

Loading