fix: registry argument to be only the hostport instead full URL #16394

[thepabloaguilar]

Hi all, I'm opening this PR to fix a very tiny bug where Argo is passing a wrong argument while authenticating against a private Helm repository!

Thanks to @aarozhkov which have made the investigation finding where the problem was!

Note

As this is not a complex bug I've decided to open a PR without the full triage process, but we've an open bug issue linked with this PR!

Basically the authentication is made targeting a host and it's what the lib argo uses, oras-go, for Helm Repo auth is doing when we provide static credentials using the auth.StaticCredential function:

// StaticCredential specifies static credentials for the given host.
func StaticCredential(registry string, cred Credential) CredentialFunc {
	if registry == "docker.io" {
		// it is expected that traffic targeting "docker.io" will be redirected
		// to "registry-1.docker.io"
		// reference: https://github.com/moby/moby/blob/v24.0.0-beta.2/registry/config.go#L25-L48
		registry = "registry-1.docker.io"
	}
	return func(_ context.Context, hostport string) (Credential, error) {
		if hostport == registry {
			return cred, nil
		}
		return EmptyCredential, nil
	}
}

Source

The problem is that argo is passing the full provided repository URL, when making the comparison hostport == registry we end up with something like "localhost:1234" == "localhost:1234/my-repo"

For the sake of curiosity the hostport is get from the net.Request.Host field, you can check it here

To fix that we need to pass only the hostport part of the given repo URL, I've decided to use strings.Cut function over the tagsURL variable as the repo URL will can have or not the scheme and that variable already trim the https:// part!

Important

Versions that can receive this fix:

• 2.10
• 2.9
• 2.8

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

Closes #16394

[thepabloaguilar]

Hi @ishitasequeira and @todaywasawesome, sorry for tagging you both here! I'd like to know if I need to open other PRs for the release/versions branch I've put in the PR (2.10, 2.9, 2.8)! Just trying to

I've tagged you 'cause you were the first two people that have interacted in the latest commits from master

[ishitasequeira]

Thanks @thepabloaguilar the changes make sense.. @crenshaw-dev can you give a second pair of eye to see if there any concerns?

[thepabloaguilar]

Thanks @ishitasequeira, I've just rebased my branch with master!

[jannfis]

I was thinking a little more about the effects of this change. AFAIK, this might break the following use case (which we probably don't have a proper test for yet):

• You have a private repo on https://example.com/my-repo
• You have another private repo in https://example.com/your-repo

Now with this change, credentials would only ever be looked up for example.com.

Is that a correct assumption?

[thepabloaguilar]

@jannfis I don't know if I fully understand your question but when we call a repo that needs authentication the response for the first request will be 401 Unauthorized with the auth challenge in WWW-Authenticate (oras support both Basic and Bearer challenges) and indeed it'll always try to match the the credentials to the host but that doesn't mean we're making an authentication to a host so you can access both repositories!

So if we make these requests will end up with something like:

• https://example.com/my-repo
  • First Response:
    • Code: 401
    • Headers: WWW-Authenticate: Basic realm="helm repo to get tags"
  • Match the provided credentials to example.com
• https://example.com/your-repo
  • First Response:
    • Code: 401
    • Headers: WWW-Authenticate: Bearer SOME_BEARER_STUFF
  • Match the provided credentials to example.com

So even we're matching the same host, each repo will have its own authentication mechanism!
And it's worthy to say the host matching part it's just to ensure we're providing the credentials to the right host, in the end if you have two repos like in your example we'll have two different sources in argo which means also two different credentials!

[jannfis]

in the end if you have two repos like in your example we'll have two different sources in argo which means also two different credentials!

Ah that makes sense. I was assuming that StaticCredentials would have multiple credentials in it, and was configured from all the repositories.

[thepabloaguilar]

@jannfis exactly, another thing to mention is if a cache is provided to argo, it doesn't cache based on just the hostport but the full chart URL (eg: docker.io/my-repo/my-chart) and the only cached thing is the response!

[thepabloaguilar]

Hey @jannfis, do you think we can proceed with this PR or have you seen other possible problem?

[thepabloaguilar]

@pasha-codefresh @gdsoumya maybe you could help here too, if possible, please?

[thepabloaguilar]

I guess we need just one more approve here! @ishitasequeira and @jannfis could you help, if possible?

[jannfis]

LGTM

[thepabloaguilar]

This fix can be cherry picked to versions 2.10, 2.9, and 2.8! Could you do it, please, if possible?

I'm tagging you 'cause I saw you were the last three persons executing the cherry-pick command in past PRs!

cc @jannfis @crenshaw-dev @alexmt

Note

The original issue is about version 2.8 but I'm having the same problem using 2.9 and looking the code base the version 2.10 also has the same problem

[thepabloaguilar]

Maybe you can also help with the cherry pick @ishitasequeira?

[guilhermebr]

+1 for the cherry-pick! we need this fix in 2.9

[ishitasequeira]

/cherry-pick release-2.10

[ishitasequeira]

/cherry-pick release-2.9

[ishitasequeira]

/cherry-pick release-2.8

[thepabloaguilar]

@ishitasequeira I guess you forgot to put - in the branches name 😅

It should be release-2.10, release-2.9, release-2.8

[ishitasequeira]

@thepabloaguilar , thanks for catching that.

[thepabloaguilar]

No problem @ishitasequeira, and sorry for bothering you!