Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(registry): address anonymous pull issue #12424

Merged
merged 1 commit into from
Sep 29, 2023
Merged

fix(registry): address anonymous pull issue #12424

merged 1 commit into from
Sep 29, 2023

Conversation

hiddeco
Copy link
Contributor

@hiddeco hiddeco commented Sep 28, 2023

What this PR does / why we need it:

The assumption that either a username and/or password OR an error is returned appears to be wrong, and results in an error later on which looks something like the following:

failed to authorize: failed to fetch anonymous token: unexpected status
from GET request to https://auth.docker.io/token?scope=repository%3AXXX%2FYYY%3Apull&service=registry.docker.io:
401 Unauthorized

To mitigate this, confirm we actually have one of the values before setting the Authorization header.

Should fix #12423

Special notes for your reviewer:

This could (and should) really have more test coverage, but this is beyond the time I have available at present.

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 28, 2023
@hiddeco hiddeco changed the title registry: fix anonymous pull fix(registry): address anonymous pull issue Sep 28, 2023
hiddeco
hiddeco commented Sep 28, 2023
View reviewed changes
pkg/registry/client.go
@@ -105,10 +105,11 @@ func NewClient(options ...ClientOption) (*Client, error) {
if err != nil {
return nil, errors.New("unable to retrieve credentials")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not directly related to this issue, but I am wondering if swallowing the error over doing fmt.Errorf("unable to retrieve credentials: %w, err) is the best thing to do?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't swallow the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fill a separate PR to address this to allow you to move on with this quickly.

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 28, 2023
@mattfarina mattfarina added this to the 3.13.1 milestone Sep 28, 2023
@mattfarina mattfarina added the Has One Approval This PR has one approval. It still needs a second approval to be merged. label Sep 28, 2023
@mattfarina
Copy link
Collaborator

Note, this still needs a second maintainer to approve.

@hiddeco hiddeco mentioned this pull request Sep 28, 2023
Merged
3 tasks
@amanibhavam amanibhavam mentioned this pull request Sep 29, 2023
Closed
@gjenkins8
Copy link
Contributor

👍 looks to me to fix the issue:

$ git checkout fix-anonymous-pull
$ make
$ ./bin/helm version
version.BuildInfo{Version:"v3.13+unreleased", GitCommit:"2e94320d1b131b64c6935e8487a64540a4244a11", GitTreeState:"dirty", GoVersion:"go1.21.1"}
$ ./bin/helm pull oci://registry-1.docker.io/bitnamicharts/redis
Pulled: registry-1.docker.io/bitnamicharts/redis:18.1.0
Digest: sha256:764d21e398932804ac32b18abba1c25ad383a15d9876f7016191a0e681defa40

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 29, 2023
@joejulian joejulian force-pushed the fix-anonymous-pull branch 2 times, most recently from b3c3a97 to 2e94320 Compare September 29, 2023 07:04
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 29, 2023
@joejulian
Copy link
Contributor

Sorry for the push... Hit the wrong repo. Consider pulling in b3c3a97 which makes it testable and adds unit tests.

@hiddeco @joejulian
fix(registry): address anonymous pull issue
fe4c01f 
The assumption that either a username and/or password OR an error is
returned appears to be wrong, and results in an error later on which
looks something like the following:

```
failed to authorize: failed to fetch anonymous token: unexpected status
from GET request to https://auth.docker.io/token?scope=repository%3AXXX%2FYYY%3Apull&service=registry.docker.io:
401 Unauthorized
```

To mitigate this, confirm we actually have one of the values before
setting the `Authorization` header.

Co-authored-by: Joe Julian <me@joejulian.name>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 29, 2023
@hiddeco
Copy link
Contributor Author

hiddeco commented Sep 29, 2023

@joejulian have applied your patch with some minor tweaks, think this should be good to go now.

@mattfarina mattfarina merged commit c288f0b into helm:main Sep 29, 2023
5 checks passed
@hiddeco hiddeco deleted the fix-anonymous-pull branch September 29, 2023 21:27
@hiddeco hiddeco mentioned this pull request Sep 30, 2023
Closed
@glasser
Copy link

glasser commented Oct 2, 2023

Is there a planned patch release for this PR?

@nikolaigut
Copy link

Is there a planned patch release for this PR?

Yes, the release is planned in the Milestone 3.13.1 (https://github.com/helm/helm/milestone/134).

@manslaughter03 manslaughter03 mentioned this pull request Oct 5, 2023
Closed
@RichardChen820 RichardChen820 mentioned this pull request Oct 10, 2023
Closed
@peikk0 peikk0 mentioned this pull request Oct 11, 2023
Closed
@mattfarina mattfarina added needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. picked Indicates that a PR has been cherry-picked into the next release candidate. and removed Has One Approval This PR has one approval. It still needs a second approval to be merged. needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. labels Oct 12, 2023
This was referenced Oct 17, 2023
Closed
Closed
@krancour krancour mentioned this pull request Oct 24, 2023
Closed
@mattfarina mattfarina mentioned this pull request Oct 25, 2023
Merged
@gabe565 gabe565 mentioned this pull request Jan 9, 2024
Closed
@jbw976 jbw976 mentioned this pull request Apr 5, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattfarina mattfarina mattfarina approved these changes

@joejulian joejulian joejulian approved these changes

Assignees
No one assigned
Labels
picked Indicates that a PR has been cherry-picked into the next release candidate. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
3.13.1
Development

Successfully merging this pull request may close these issues.

helm 3.13.0 helm pull fails with 401 unauthorized
6 participants
@hiddeco @mattfarina @gjenkins8 @joejulian @glasser @nikolaigut