fix: set [pull,push] scope when helm push to a registry(use token auth) - v4 by kimsungmin1 · Pull Request #31211 · helm/helm

kimsungmin1 · 2025-08-28T06:04:40Z

What this PR does / why we need it:
this pr is same content with #31129
helm v4 also have same problem
make proper scope [pull,push] when helm push a chart to a registry(use token auth)

Special notes for your reviewer:

this pr is about issue #31117 helm make [pull] scope request when push a chart to a registry
so it's hard to give a proper token (becuase helm want a pull permission for a new object(repository path is not exists yet))

old version helm(~3.17)
helm make multiscope [pull],[pull,push] request when push to a registry
https://auth.ociregistry.io/token?scope=repository:myrepo:pull&scope=repository:myrepo:pull,push&service=oci.registry.io"

latest version helm(3.18)
helm make scope [pull] request first
https://auth.ociregistry.io/token?scope=repository:myrepo:pull&service=oci.registry.io"
and then make scope [pull, push] request
https://auth.ociregistry.io/token?scope=repository:myrepo:pull,push&service=oci.registry.io"

this pr version
helm make [pull,push] scope request
https://auth.ociregistry.io/token?scope=repository:myrepo:pull,push&service=oci.registry.io"

i checked actions in my forked repository
please check v3 pr again #31129

If applicable:

this PR contains user facing changes (the docs needed label should be applied if so)
this PR contains unit tests
this PR has been tested for backwards compatibility

TerryHowe · 2025-09-06T21:55:30Z

I made some changes to the registry test that are causing conflicts

kimsungmin1 · 2025-09-08T08:01:01Z

I made some changes to the registry test that are causing conflicts
@TerryHowe
i resolved the conflict

Copilot

Pull Request Overview

This PR addresses an issue with OCI registry authentication scopes in Helm v4 when pushing charts to registries that use token authentication. The fix ensures that Helm requests the appropriate [pull,push] scope during push operations instead of making separate [pull] and [pull,push] requests.

Adds proper scope hinting for push operations using WithScopeHint function
Introduces comprehensive test coverage for registry scope validation
Updates existing test infrastructure to support token authentication testing

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/registry/client.go	Adds `WithScopeHint` function and integrates scope hinting into push operations
pkg/registry/utils_test.go	Extends test setup to support token authentication configuration
pkg/registry/client_scope_test.go	New test suite specifically for validating registry authentication scopes
pkg/registry/client_tls_test.go	Updates setup call to include auth parameter
pkg/registry/client_insecure_tls_test.go	Updates setup call to include auth parameter
pkg/registry/client_http_test.go	Updates setup call to include auth parameter
go.mod	Adds dependency for JWT token handling

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

TerryHowe

Generally looks good, just some comments from copilot.

Tests ran for me.

kimsungmin1 · 2025-09-09T01:02:31Z

Generally looks good, just some comments from copilot.

Tests ran for me.

@TerryHowe
i fixed the typo thank you

TerryHowe

/lgtm

kimsungmin1 · 2025-09-25T06:21:27Z

@mattfarina could you review this pr again?

kimsungmin1 · 2025-10-01T03:59:52Z

Is there any way I can get an approximate date when this PR can be merged?

TerryHowe · 2025-10-14T22:51:09Z

We could still use this.

kimsungmin1 · 2025-10-16T02:04:51Z

conflict resolved

TerryHowe

/lgtm

sabre1041

LGTM

TerryHowe · 2025-12-01T12:22:30Z

Closes: #31567

TerryHowe · 2025-12-04T02:51:58Z

Maybe if @gjenkins8 can get a chance to look at this

Signed-off-by: kimsm28 <sm28.kim@samsung.com>

…erver listener management - Change DockerRegistryHost to use 127.0.0.1 for HTTP tests and helm-test-registry for TLS tests to match certificate hostname - Add defer ln.Close() to prevent resource leak in token auth server setup - Restore requestURL variable and assertion in test body instead of handler to avoid potential race condition Signed-off-by: kimsm28 <sm28.kim@samsung.com>

- Change 'bellow' to 'below' (spelling correction in multiple places) - Change 'WithScopeHint' to 'withScopeHint' to match actual function name Signed-off-by: kimsm28 <sm28.kim@samsung.com>

…sertions - Fix data race by using channel instead of shared variable for requestURL - Make assertions more robust by parsing URL and checking query parameters instead of exact string matching Signed-off-by: kimsm28 <sm28.kim@samsung.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Terry Howe <terrylhowe@gmail.com>

The scope test configured a TLS registry while advertising an http token realm. oras refuses to send credentials to an http token realm when the registry itself was contacted over https, so the auth server was never reached and the test timed out ("timeout waiting for auth request"). Use a plain-http registry so the registry and token-realm schemes match; the scope carried in the token request is what this test verifies, and that is independent of TLS. Also make the auth handler's channel send non-blocking so that a client retry can never block the handler and stall the push/pull flow, which addresses the review comment about a potential deadlock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: kimsm28 <sm28.kim@samsung.com>

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: kimsm28 <sm28.kim@samsung.com>

kimsungmin1 · 2026-07-02T12:52:51Z

@kimsungmin1 this has two approvals from last year, but:

most importantly, CI is failing

also Copilot also made two good comments about avoiding deadlock

hello @scottrigby
i fixed 2 problems
can you run the workflows to check this PR?

- Use (*net.ListenConfig).Listen with the test context instead of net.Listen (noctx). - Use suite.NoError instead of suite.Nil for the error check (testifylint). - Drop the unnecessary leading blank lines in the scope test funcs (whitespace). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: kimsm28 <sm28.kim@samsung.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.

+		ln, err := lnCfg.Listen(suite.T().Context(), "tcp", "127.0.0.1:0")
+		suite.NoError(err, "no error finding free port for test auth server")
+		defer ln.Close()
+
+		//set test auth server host
+		suite.AuthServerHost = ln.Addr().String()


+	listener, err := lnCfg.Listen(suite.T().Context(), "tcp", suite.AuthServerHost)
+	suite.NoError(err, "no error creating server listener")
+
+	ts := httptest.NewUnstartedServer(handler)
+	ts.Listener = listener
+	ts.Start()


+	listener, err := lnCfg.Listen(suite.T().Context(), "tcp", suite.AuthServerHost)
+	suite.NoError(err, "no error creating server listener")
+
+	ts := httptest.NewUnstartedServer(handler)
+	ts.Listener = listener
+	ts.Start()


+// add actions when request a registry authentication token(jwt)
+// example1. when we want to pull 'testrepo/local-subchart' we can send below url, and 'pull' is the action
+// auth?scope=repository%3Atestrepo%2Flocal-subchart%3Apull&service=testservice
+// example2. when we want to push 'testrepo/local-subchart' we can send below url, and 'pull%2Cpush' are the actions
+// auth?scope=repository%3Atestrepo%2Flocal-subchart%3Apull%2Cpush&service=testservice
+// we can set the actions like below
+// example) ctx = withScopeHint(ctx, repository, auth.ActionPush, auth.ActionPull)
+func withScopeHint(ctx context.Context, repo *remote.Repository, actions ...string) context.Context {


Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: kimsungmin1 <142377392+kimsungmin1@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

+// example2. when we want to push 'testrepo/local-subchart' we can send below url, and 'pull%2Cpush' are the actions
+// auth?scope=repository%3Atestrepo%2Flocal-subchart%3Apull%2Cpush&service=testservice
+// we can set the actions like below
+// example) ctx = withScopeHint(ctx, repository, auth.ActionPush, auth.ActionPull)


TerryHowe

/lgtm

…istry When pushing a chart to an OCI registry that uses token auth, the auth client first requested a [pull]-only scope token, which fails for a repository path that does not yet exist, making it hard to mint a valid token. Hint the [pull,push] scope up front so a single valid token request is made. Ports the fix from helm#31211 to the oras-go v3 code path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Terry Howe <terrylhowe@gmail.com>

This one-line fix is already in main, from #31862. but that PR relies on other refactor PRs, and all are too broad to apply to the patch release branch. Signed-off-by: Scott Rigby <scott@r6by.com>

pull-request-size Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 28, 2025

TerryHowe requested changes Aug 28, 2025

View reviewed changes

Comment thread pkg/registry/utils_test.go Outdated

mattfarina previously requested changes Aug 28, 2025

View reviewed changes

Comment thread pkg/registry/utils_test.go Outdated

kimsungmin1 requested review from TerryHowe and mattfarina August 29, 2025 01:45

benoittgt reviewed Aug 29, 2025

View reviewed changes

Comment thread go.mod Outdated

TerryHowe mentioned this pull request Sep 6, 2025

fix: set [pull,push] scope when helm push to a registry(use token auth) #31129

Closed

3 tasks

TerryHowe requested a review from Copilot September 8, 2025 14:43

Copilot AI reviewed Sep 8, 2025

View reviewed changes

Comment thread pkg/registry/client_scope_test.go Outdated

Comment thread pkg/registry/client_scope_test.go Outdated

TerryHowe reviewed Sep 8, 2025

View reviewed changes

TerryHowe approved these changes Sep 9, 2025

View reviewed changes

TerryHowe added the keep open label Oct 14, 2025

kimsungmin1 force-pushed the main-fix-push-scope branch from daa7019 to 6d3278c Compare October 16, 2025 01:43

TerryHowe previously approved these changes Oct 16, 2025

View reviewed changes

sabre1041 previously approved these changes Nov 20, 2025

View reviewed changes

TerryHowe mentioned this pull request Nov 27, 2025

Unable to push charts to Quay with recent versions of helm #31567

Closed

TerryHowe added Has One Approval This PR has one approval. It still needs a second approval to be merged. Needs v3 backport Label PRs for v4/main, which are still applicable to v3 so need a separate backport PR labels Dec 1, 2025

gjenkins8 reviewed Dec 5, 2025

View reviewed changes

Comment thread pkg/registry/client_scope_test.go Outdated

kimsungmin1 dismissed sabre1041’s stale review via a7f6b08 December 10, 2025 15:29

kimsungmin1 force-pushed the main-fix-push-scope branch from a7f6b08 to 2dc1ee9 Compare December 10, 2025 15:48

kimsungmin1 and others added 7 commits July 2, 2026 21:34

fix variable naming requestUrl -> requestURL

8883c9c

Signed-off-by: kimsm28 <sm28.kim@samsung.com>

fix typos in withScopeHint function comment

ee181f4

- Change 'bellow' to 'below' (spelling correction in multiple places) - Change 'WithScopeHint' to 'withScopeHint' to match actual function name Signed-off-by: kimsm28 <sm28.kim@samsung.com>

Update pkg/registry/client.go

26ada49

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Terry Howe <terrylhowe@gmail.com>

chore: go mod tidy after rebase on main

4e9ca06

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: kimsm28 <sm28.kim@samsung.com>

kimsungmin1 force-pushed the main-fix-push-scope branch from 5c2997c to 4e9ca06 Compare July 2, 2026 12:49

Copilot AI review requested due to automatic review settings July 2, 2026 12:57

Copilot started reviewing on behalf of kimsungmin1 July 2, 2026 12:58 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

Potential fix for pull request finding

d539556

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: kimsungmin1 <142377392+kimsungmin1@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 3, 2026 12:28

Copilot started reviewing on behalf of kimsungmin1 July 3, 2026 12:29 View session

Copilot AI reviewed Jul 3, 2026

View reviewed changes

TerryHowe approved these changes Jul 3, 2026

View reviewed changes

TerryHowe merged commit 543b94d into helm:main Jul 3, 2026
6 checks passed

kimsungmin1 mentioned this pull request Jul 15, 2026

fix: set [pull,push] scope when helm push to a registry(use token auth) (backport) #32362

Open

3 tasks

scottrigby added bug Categorizes issue or PR as related to a bug. needs-pick Indicates that a PR needs to be 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. keep open labels Aug 12, 2026

scottrigby added this to the 4.2.4 milestone Aug 13, 2026

scottrigby added picked Indicates that a PR has been cherry-picked into the next release candidate. and removed needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. labels Aug 13, 2026

rancherbot mentioned this pull request Aug 14, 2026

rddepman: bump helm from 4.2.3 to 4.2.4 rancher-sandbox/rancher-desktop#10809

Open

Uh oh!

Conversation

kimsungmin1 commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TerryHowe commented Sep 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimsungmin1 commented Sep 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

TerryHowe left a comment

Choose a reason for hiding this comment

Uh oh!

kimsungmin1 commented Sep 9, 2025

Uh oh!

TerryHowe left a comment

Choose a reason for hiding this comment

Uh oh!

kimsungmin1 commented Sep 25, 2025

Uh oh!

kimsungmin1 commented Oct 1, 2025

Uh oh!

TerryHowe commented Oct 14, 2025

Uh oh!

kimsungmin1 commented Oct 16, 2025

Uh oh!

TerryHowe left a comment

Choose a reason for hiding this comment

Uh oh!

sabre1041 left a comment

Choose a reason for hiding this comment

Uh oh!

TerryHowe commented Dec 1, 2025

Uh oh!

TerryHowe commented Dec 4, 2025

Uh oh!

Uh oh!

kimsungmin1 commented Jul 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

TerryHowe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

kimsungmin1 commented Aug 28, 2025 •

edited

Loading

TerryHowe commented Sep 6, 2025 •

edited

Loading