From 75856bf51b9272350c91393372e3a3b640e8ae65 Mon Sep 17 00:00:00 2001 From: Denis Nutiu Date: Sat, 6 Jun 2026 11:22:44 +0300 Subject: [PATCH] fix: repo credentials not resolved when two aliases share the same url Signed-off-by: Denis Nutiu --- pkg/downloader/chart_downloader.go | 9 +- pkg/downloader/manager.go | 48 +++++++++-- pkg/downloader/manager_test.go | 127 +++++++++++++++++++++++++++-- 3 files changed, 171 insertions(+), 13 deletions(-) diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 22c6c71a3..ec78ccf72 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -85,6 +85,13 @@ type ChartDownloader struct { // Cache specifies the cache implementation to use. Cache Cache + + // CredentialsResolved indicates that credentials (username/password) have + // already been set in Options and should not be overridden by repository + // scanning in ResolveChartVersion. This prevents incorrect credential + // selection when multiple repositories share the same URL with different + // authentication details. + CredentialsResolved bool } // DownloadTo retrieves a chart. Depending on the settings, it may also download a provenance file. @@ -410,7 +417,7 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (string, *url if rc.CertFile != "" || rc.KeyFile != "" || rc.CAFile != "" { c.Options = append(c.Options, getter.WithTLSClientConfig(rc.CertFile, rc.KeyFile, rc.CAFile)) } - if rc.Username != "" && rc.Password != "" { + if rc.Username != "" && rc.Password != "" && !c.CredentialsResolved { c.Options = append( c.Options, getter.WithBasicAuth(rc.Username, rc.Password), diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index b19a1f446..5272d7452 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -113,7 +113,8 @@ func (m *Manager) Build() error { } } - if _, err := m.resolveRepoNames(req); err != nil { + repoNames, err := m.resolveRepoNames(req) + if err != nil { return err } @@ -144,7 +145,7 @@ func (m *Manager) Build() error { } // Now we need to fetch every package here into charts/ - return m.downloadAll(lock.Dependencies) + return m.downloadAll(lock.Dependencies, repoNames) } // Update updates a local charts directory. @@ -200,7 +201,7 @@ func (m *Manager) Update() error { } // Now we need to fetch every package here into charts/ - if err := m.downloadAll(lock.Dependencies); err != nil { + if err := m.downloadAll(lock.Dependencies, repoNames); err != nil { return err } @@ -242,7 +243,7 @@ func (m *Manager) resolve(req []*chart.Dependency, repoNames map[string]string) // // It will delete versions of the chart that exist on disk and might cause // a conflict. -func (m *Manager) downloadAll(deps []*chart.Dependency) error { +func (m *Manager) downloadAll(deps []*chart.Dependency, repoNames map[string]string) error { repos, err := m.loadChartRepositories() if err != nil { return err @@ -315,7 +316,8 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { // Any failure to resolve/download a chart should fail: // https://github.com/helm/helm/issues/1439 - churl, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err := m.findChartURL(dep.Name, dep.Version, dep.Repository, repos) + repoName := repoNames[dep.Name] + churl, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err := m.findChartURL(dep.Name, dep.Version, dep.Repository, repoName, repos) if err != nil { saveError = fmt.Errorf("could not find %s: %w", churl, err) break @@ -343,6 +345,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { getter.WithInsecureSkipVerifyTLS(insecureSkipTLSVerify), getter.WithTLSClientConfig(certFile, keyFile, caFile), }, + CredentialsResolved: true, } version := "" @@ -719,14 +722,45 @@ func (m *Manager) parallelRepoUpdate(repos []*repo.Entry) error { // 'name' is the name of the chart. Version is an exact semver, or an empty string. If empty, the // newest version will be returned. // -// repoURL is the repository to search +// repoURL is the repository to search. +// +// repoName is the name (alias) of the repository as configured in repositories.yaml. +// When provided, the repository is looked up by name rather than by URL, ensuring that +// the correct credentials are used when multiple repositories share the same URL. // // If it finds a URL that is "relative", it will prepend the repoURL. -func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*repo.ChartRepository) (url, username, password string, insecureSkipTLSVerify, passCredentialsAll bool, caFile, certFile, keyFile string, err error) { +func (m *Manager) findChartURL(name, version, repoURL, repoName string, repos map[string]*repo.ChartRepository) (url, username, password string, insecureSkipTLSVerify, passCredentialsAll bool, caFile, certFile, keyFile string, err error) { if registry.IsOCI(repoURL) { return fmt.Sprintf("%s/%s:%s", repoURL, name, version), "", "", false, false, "", "", "", nil } + // First, try to look up the repository by name. This ensures correct + // credentials are used when multiple repositories share the same URL. + if repoName != "" { + if cr, ok := repos[repoName]; ok { + var entry repo.ChartVersions + entry, err = findEntryByName(name, cr) + if err == nil { + var ve *repo.ChartVersion + ve, err = findVersionedEntry(version, entry) + if err == nil { + url, err = repo.ResolveReferenceURL(repoURL, ve.URLs[0]) + if err == nil { + username = cr.Config.Username + password = cr.Config.Password + passCredentialsAll = cr.Config.PassCredentialsAll + insecureSkipTLSVerify = cr.Config.InsecureSkipTLSVerify + caFile = cr.Config.CAFile + certFile = cr.Config.CertFile + keyFile = cr.Config.KeyFile + return + } + } + } + } + } + + // Fall back to scanning repos by URL for backward compatibility. for _, cr := range repos { if urlutil.Equal(repoURL, cr.Config.URL) { var entry repo.ChartVersions diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index 9e27f183f..f68a35c4c 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -71,7 +71,7 @@ func TestFindChartURL(t *testing.T) { version := "0.1.0" repoURL := "http://example.com/charts" - churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err := m.findChartURL(name, version, repoURL, repos) + churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err := m.findChartURL(name, version, repoURL, "", repos) if err != nil { t.Fatal(err) } @@ -96,7 +96,7 @@ func TestFindChartURL(t *testing.T) { version = "1.2.3" repoURL = "https://example-https-insecureskiptlsverify.com" - churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, repos) + churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, "", repos) if err != nil { t.Fatal(err) } @@ -121,7 +121,7 @@ func TestFindChartURL(t *testing.T) { version = "1.2.3" repoURL = "http://example.com/helm" - churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, repos) + churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, "", repos) if err != nil { t.Fatal(err) } @@ -143,6 +143,123 @@ func TestFindChartURL(t *testing.T) { } } +// TestFindChartURL_DuplicateRepoURL tests that when two repositories share the same +// URL but have different aliases and credentials, findChartURL correctly returns the +// credentials for the specified repo alias (name) rather than picking a random +// matching entry by URL. +func TestFindChartURL_DuplicateRepoURL(t *testing.T) { + dir := t.TempDir() + + // Create a repositories.yaml with two entries sharing the same URL + reposConfig := `apiVersion: v1 +repositories: + - name: repo-alias-1 + url: "http://example.com/charts" + username: "user1" + password: "pass1" + pass_credentials_all: true + - name: repo-alias-2 + url: "http://example.com/charts" + username: "user2" + password: "pass2" +` + repoConfigFile := filepath.Join(dir, "repositories.yaml") + if err := os.WriteFile(repoConfigFile, []byte(reposConfig), 0644); err != nil { + t.Fatal(err) + } + + // Create an index file for each repo alias (same content since they share the same URL) + index := repo.NewIndexFile() + index.Entries["alpine"] = repo.ChartVersions{ + { + Metadata: &chart.Metadata{ + Name: "alpine", + Version: "0.1.0", + }, + URLs: []string{ + "https://charts.helm.sh/stable/alpine-0.1.0.tgz", + }, + }, + } + + // Write index files with the naming pattern expected by loadChartRepositories: + // /-index.yaml + for _, name := range []string{"repo-alias-1", "repo-alias-2"} { + idxPath := filepath.Join(dir, name+"-index.yaml") + if err := index.WriteFile(idxPath, 0644); err != nil { + t.Fatal(err) + } + } + + m := &Manager{ + Out: new(bytes.Buffer), + RepositoryConfig: repoConfigFile, + RepositoryCache: dir, + } + + repos, err := m.loadChartRepositories() + if err != nil { + t.Fatal(err) + } + + name := "alpine" + version := "0.1.0" + repoURL := "http://example.com/charts" + + // Look up by repo-alias-1 name -> should get user1/pass1 + churl, username, password, _, passCredentialsAll, _, _, _, err := m.findChartURL(name, version, repoURL, "repo-alias-1", repos) + if err != nil { + t.Fatal(err) + } + if churl != "https://charts.helm.sh/stable/alpine-0.1.0.tgz" { + t.Errorf("Unexpected URL %q", churl) + } + if username != "user1" { + t.Errorf("Expected username 'user1', got %q", username) + } + if password != "pass1" { + t.Errorf("Expected password 'pass1', got %q", password) + } + if !passCredentialsAll { + t.Errorf("Expected passCredentialsAll true, got %t", passCredentialsAll) + } + + // Look up by repo-alias-2 name -> should get user2/pass2 + churl, username, password, _, passCredentialsAll, _, _, _, err = m.findChartURL(name, version, repoURL, "repo-alias-2", repos) + if err != nil { + t.Fatal(err) + } + if churl != "https://charts.helm.sh/stable/alpine-0.1.0.tgz" { + t.Errorf("Unexpected URL %q", churl) + } + if username != "user2" { + t.Errorf("Expected username 'user2', got %q", username) + } + if password != "pass2" { + t.Errorf("Expected password 'pass2', got %q", password) + } + // passCredentialsAll should be false for repo-alias-2 (not set in config) + if passCredentialsAll { + t.Errorf("Expected passCredentialsAll false, got %t", passCredentialsAll) + } + + // Without a repo name (empty string), falls back to URL-based scan. + // This is backward-compatible behavior. + churl, username, password, _, _, _, _, _, err = m.findChartURL(name, version, repoURL, "", repos) + if err != nil { + t.Fatal(err) + } + if churl != "https://charts.helm.sh/stable/alpine-0.1.0.tgz" { + t.Errorf("Unexpected URL %q", churl) + } + // Without a repo name, it falls back to URL-based matching. + // Either repo's credentials could be returned since map iteration is nondeterministic. + // We just verify that some credentials were returned. + if username == "" { + t.Error("Expected non-empty username from URL-based fallback") + } +} + func TestGetRepoNames(t *testing.T) { b := bytes.NewBuffer(nil) m := &Manager{ @@ -267,7 +384,7 @@ func TestDownloadAll(t *testing.T) { if err := os.MkdirAll(filepath.Join(chartPath, "tmpcharts"), 0755); err != nil { t.Fatal(err) } - if err := m.downloadAll([]*chart.Dependency{signDep, localDep}); err != nil { + if err := m.downloadAll([]*chart.Dependency{signDep, localDep}, nil); err != nil { t.Error(err) } @@ -296,7 +413,7 @@ version: 0.1.0` Version: "0.1.0", } - err = m.downloadAll([]*chart.Dependency{badLocalDep}) + err = m.downloadAll([]*chart.Dependency{badLocalDep}, nil) if err == nil { t.Fatal("Expected error for bad dependency name") }