Skip to content

[release/1.6] remotes: add handling for missing basic auth credentials#9236

Merged
estesp merged 3 commits intocontainerd:release/1.6from
dmcgowan:backport-1.6-fix-basic-auth-error
Oct 17, 2023
Merged

[release/1.6] remotes: add handling for missing basic auth credentials#9236
estesp merged 3 commits intocontainerd:release/1.6from
dmcgowan:backport-1.6-fix-basic-auth-error

Conversation

@dmcgowan
Copy link
Copy Markdown
Member

When a credential handler is provided but no basic auth credentials are provided, handle the error specifically rather than treating the credentials as not implemented. This allows a clearer error to be provided to users rather than a confusing not implemented error or generic unauthorized error.

Add unit tests for the basic auth case.

Backport #9234

(cherry picked from commit 51c9ffe)

@thaJeztah
Copy link
Copy Markdown
Member

This failure (on Windows) looks like it may be related? Are we missing a patch somewhere in this branch?

=== FAIL: remotes/docker TestWrongBasicAuthResolver (0.03s)
    resolver_test.go:300: pulling from host 127.0.0.1:50134 failed with status code [manifests sometag]: 403 Forbidden

FabHof and others added 3 commits October 16, 2023 17:36
Signed-off-by: Fabian Hoffmann <[email protected]>
(cherry picked from commit 894e780)
Signed-off-by: Derek McGowan <[email protected]>
Signed-off-by: Fabian Hoffmann <[email protected]>
(cherry picked from commit b90c466)
Signed-off-by: Derek McGowan <[email protected]>
When a credential handler is provided but no basic auth credentials
are provided, handle the error specifically rather than treating
the credentials as not implemented. This allows a clearer error to
be provided to users rather than a confusing not implemented error
or generic unauthorized error.

Add unit tests for the basic auth case.

Signed-off-by: Derek McGowan <[email protected]>
(cherry picked from commit 51c9ffe)
Signed-off-by: Derek McGowan <[email protected]>
@dmcgowan
Copy link
Copy Markdown
Member Author

I should pull in #6617 to fix the tests

@dmcgowan dmcgowan force-pushed the backport-1.6-fix-basic-auth-error branch from f5ac51d to e529741 Compare October 17, 2023 00:44
@laurazard
Copy link
Copy Markdown
Member

Should be fine now? Maybe we can get it merged.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, all green now!

LGTM

@estesp estesp merged commit 5477dff into containerd:release/1.6 Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants