From 91624b1f902a3801352e307100add2cb26ef0b28 Mon Sep 17 00:00:00 2001 From: ashvinctrl Date: Sat, 26 Sep 2026 12:46:17 +0530 Subject: [PATCH] fix(downloader): check --repo and dependency downloads against the index digest helm pull --repo, chart lookups with --repo and dependency build/update look the chart up in the index themselves and pass only its absolute URL to the downloader, which resolves no digest for a bare URL, so the index digest check never ran on those paths. Add repo.FindChartInRepoURLWithDigest and a ChartDownloader.IndexDigest field so those callers can hand over the digest of the entry they picked, and have the dependency manager pass the digest of the entry it selected. Signed-off-by: ashvinctrl --- pkg/action/install.go | 3 +- pkg/action/install_test.go | 18 +++++++++ pkg/action/pull.go | 3 +- pkg/action/pull_test.go | 49 +++++++++++++++++++++++- pkg/downloader/chart_downloader.go | 16 ++++++-- pkg/downloader/chart_downloader_test.go | 41 ++++++++++++++++++++ pkg/downloader/manager.go | 22 ++++++----- pkg/downloader/manager_test.go | 50 +++++++++++++++++++++++-- pkg/repo/v1/chartrepo.go | 23 ++++++++---- pkg/repo/v1/chartrepo_test.go | 11 ++++++ 10 files changed, 209 insertions(+), 27 deletions(-) diff --git a/pkg/action/install.go b/pkg/action/install.go index 6fc919366..2eb3e027d 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -937,7 +937,7 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) ( dl.Verify = downloader.VerifyAlways } if c.RepoURL != "" { - chartURL, err := repo.FindChartInRepoURL( + chartURL, digest, err := repo.FindChartInRepoURLWithDigest( c.RepoURL, name, getter.All(settings), @@ -951,6 +951,7 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) ( return "", err } name = chartURL + dl.IndexDigest = digest // Only pass the user/pass on when the user has said to or when the // location of the chart repo and the chart are the same domain. diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index 2d83abe27..67fdf20cc 100644 --- a/pkg/action/install_test.go +++ b/pkg/action/install_test.go @@ -47,8 +47,10 @@ import ( ci "helm.sh/helm/v4/pkg/chart" "helm.sh/helm/v4/internal/test" + "helm.sh/helm/v4/internal/test/ensure" "helm.sh/helm/v4/pkg/chart/common" chart "helm.sh/helm/v4/pkg/chart/v2" + "helm.sh/helm/v4/pkg/cli" "helm.sh/helm/v4/pkg/kube" kubefake "helm.sh/helm/v4/pkg/kube/fake" "helm.sh/helm/v4/pkg/registry" @@ -1253,3 +1255,19 @@ func TestInstallRelease_WaitOptionsPassedDownstream(t *testing.T) { // Verify that WaitOptions were passed to GetWaiter is.NotEmpty(failer.RecordedWaitOptions, "WaitOptions should be passed to GetWaiter") } + +func TestLocateChart_RepoURLRejectsChartNotMatchingIndexDigest(t *testing.T) { + ensure.HelmHome(t) + srv := tamperedRepoServer(t) + settings := cli.New() + + c := &ChartPathOptions{RepoURL: srv.URL(), Version: "0.1.0"} + _, err := c.LocateChart("signtest", settings) + require.ErrorContains(t, err, "does not match the digest recorded for it in the repository index") + + entries, err := os.ReadDir(settings.ContentCache) + if !errors.Is(err, fs.ErrNotExist) { + require.NoError(t, err) + assert.Empty(t, entries, "rejected chart must not be written to the content cache") + } +} diff --git a/pkg/action/pull.go b/pkg/action/pull.go index 168011d40..bf81741f1 100644 --- a/pkg/action/pull.go +++ b/pkg/action/pull.go @@ -117,7 +117,7 @@ func (p *Pull) Run(chartRef string) (string, error) { downloadSourceRef := chartRef if p.RepoURL != "" { - chartURL, err := repo.FindChartInRepoURL( + chartURL, digest, err := repo.FindChartInRepoURLWithDigest( p.RepoURL, chartRef, getter.All(p.Settings), @@ -131,6 +131,7 @@ func (p *Pull) Run(chartRef string) (string, error) { return out.String(), err } downloadSourceRef = chartURL + c.IndexDigest = digest } saved, v, err := c.DownloadTo(downloadSourceRef, p.Version, dest) diff --git a/pkg/action/pull_test.go b/pkg/action/pull_test.go index a483de248..24c5f1824 100644 --- a/pkg/action/pull_test.go +++ b/pkg/action/pull_test.go @@ -17,16 +17,21 @@ limitations under the License. package action import ( + "bytes" "net/http" "net/http/httptest" "os" + "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "helm.sh/helm/v4/internal/test/ensure" "helm.sh/helm/v4/pkg/cli" "helm.sh/helm/v4/pkg/registry" + "helm.sh/helm/v4/pkg/repo/v1/repotest" ) func TestNewPull(t *testing.T) { @@ -47,7 +52,16 @@ func TestPullSetRegistryClient(t *testing.T) { } func TestPullRun_ChartNotFound(t *testing.T) { - srv, err := startLocalServerForTests(t, nil) + fileBytes, err := os.ReadFile("../repo/v1/testdata/local-index.yaml") + require.NoError(t, err) + // The fixture's placeholder digest is not a valid sha256, and --repo pulls + // check the index digest, so give it a well-formed one to let the pull get + // as far as the missing archive. + fileBytes = bytes.ReplaceAll(fileBytes, []byte("sha256:1234567890abcdef"), []byte("sha256:"+strings.Repeat("0", 64))) + srv, err := startLocalServerForTests(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, err := w.Write(fileBytes) + assert.NoError(t, err) + })) require.NoError(t, err) defer srv.Close() @@ -76,3 +90,36 @@ func startLocalServerForTests(t *testing.T, handler http.Handler) (*httptest.Ser return httptest.NewServer(handler), nil } + +// tamperedRepoServer serves a chart repository whose index records the digest +// of signtest-0.1.0.tgz while the archive it serves has changed since. +func tamperedRepoServer(t *testing.T) *repotest.Server { + t.Helper() + srv := repotest.NewTempServer(t, repotest.WithChartSourceGlob("../downloader/testdata/signtest-0.1.0.tgz")) + t.Cleanup(srv.Stop) + + served := filepath.Join(srv.Root(), "signtest-0.1.0.tgz") + original, err := os.ReadFile(served) + require.NoError(t, err) + require.NoError(t, os.WriteFile(served, append(original, "appended by a rewritten mirror"...), 0o644)) + return srv +} + +func TestPullRun_RepoURLRejectsChartNotMatchingIndexDigest(t *testing.T) { + ensure.HelmHome(t) + srv := tamperedRepoServer(t) + + config := actionConfigFixture(t) + client := NewPull(WithConfig(config)) + client.Settings = cli.New() + client.RepoURL = srv.URL() + client.Version = "0.1.0" + client.DestDir = t.TempDir() + + _, err := client.Run("signtest") + require.ErrorContains(t, err, "does not match the digest recorded for it in the repository index") + + entries, err := os.ReadDir(client.DestDir) + require.NoError(t, err) + assert.Empty(t, entries, "rejected chart must not be written to the destination") +} diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index f6d79bd23..7e875c659 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 + + // IndexDigest is the digest a repository index records for the chart when + // the caller has already resolved the index entry to an absolute URL and + // passes that URL as the ref, as `--repo` and dependency downloads do. The + // downloaded archive is checked against it. It is ignored for any other + // kind of ref. + IndexDigest string } // DownloadTo retrieves a chart. Depending on the settings, it may also download a provenance file. @@ -424,7 +431,7 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (string, *url if errors.Is(err, ErrNoOwnerRepo) { // Make sure to add the ref URL as the URL for the getter c.Options = append(c.Options, getter.WithURL(ref)) - return "", u, nil + return c.IndexDigest, u, nil } return "", u, err } @@ -445,7 +452,7 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (string, *url getter.WithPassCredentialsAll(rc.PassCredentialsAll), ) } - return "", u, nil + return c.IndexDigest, u, nil } // See if it's of the form: repo/path_to_chart @@ -629,8 +636,9 @@ func loadRepoConfig(file string) (*repo.File, error) { // only as a cache key, so a repository that served an archive not matching its // own index was accepted without complaint. // -// It is a no-op when the index carries no digest, which is the case for a chart -// referenced by a bare URL, so those keep working as before. OCI references are +// It is a no-op when there is no index digest to check against, which is the +// case for a chart referenced by a bare URL with no IndexDigest set, so those +// keep working as before. OCI references are // excluded on purpose: the digest resolved for them identifies a manifest // rather than the archive bytes, and the registry client already checks it. func verifyIndexDigest(ref string, u *url.URL, digestString string, want [sha256.Size]byte, data []byte) error { diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index 9ff69fedd..1a68a1651 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -664,3 +664,44 @@ func TestIndexDigestVerificationScope(t *testing.T) { assert.NoError(t, err) }) } + +func TestDownload_ResolvedURLCheckedAgainstIndexDigest(t *testing.T) { + // Callers such as `helm pull --repo` and dependency downloads look the + // chart up in the index themselves and pass on only its absolute URL, so + // the index digest has to come in through IndexDigest. + setup := func(t *testing.T) (*ChartDownloader, string) { + t.Helper() + srv, c, _ := tamperedChartServer(t, t.TempDir()) + idx, err := repo.LoadIndexFile(filepath.Join(srv.Root(), "index.yaml")) + require.NoError(t, err) + cv, err := idx.Get("signtest", "0.1.0") + require.NoError(t, err) + require.NotEmpty(t, cv.Digest) + c.IndexDigest = cv.Digest + return c, srv.URL() + "/signtest-0.1.0.tgz" + } + + t.Run("DownloadTo", func(t *testing.T) { + c, chartURL := setup(t) + dest := t.TempDir() + _, _, err := c.DownloadTo(chartURL, "", dest) + require.ErrorContains(t, err, "does not match the digest recorded for it in the repository index") + + entries, err := os.ReadDir(dest) + require.NoError(t, err) + assert.Empty(t, entries, "rejected chart must not be written to the destination") + }) + + t.Run("DownloadToCache", func(t *testing.T) { + c, chartURL := setup(t) + _, _, err := c.DownloadToCache(chartURL, "") + require.ErrorContains(t, err, "does not match the digest recorded for it in the repository index") + + digestBytes, err := hex.DecodeString(stripDigestAlgorithm(c.IndexDigest)) + require.NoError(t, err) + var want [sha256.Size]byte + copy(want[:], digestBytes) + _, err = c.Cache.Get(want, CacheChart) + assert.Error(t, err, "rejected chart must not be written to the content cache") + }) +} diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 2e94d26ae..13a7baeb2 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -318,7 +318,7 @@ 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) + churl, digest, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err := m.findChartURL(dep.Name, dep.Version, dep.Repository, repos) if err != nil { saveError = fmt.Errorf("could not find %s: %w", churl, err) break @@ -340,6 +340,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { ContentCache: m.ContentCache, RegistryClient: m.RegistryClient, Getters: m.Getters, + IndexDigest: digest, Options: []getter.Option{ getter.WithBasicAuth(username, password), getter.WithPassCredentialsAll(passCredentialsAll), @@ -722,9 +723,9 @@ func (m *Manager) parallelRepoUpdate(repos []*repo.Entry) error { // repoURL is the repository to search // // 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 string, repos map[string]*repo.ChartRepository) (url, digest, 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 + return fmt.Sprintf("%s/%s:%s", repoURL, name, version), "", "", "", false, false, "", "", "", nil } for _, cr := range repos { @@ -735,17 +736,18 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]* entry, err = findEntryByName(name, cr) if err != nil { // TODO: Consider refactoring this function to reduce the number of returned values while preserving behavior. - return url, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err + return url, digest, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err } var ve *repo.ChartVersion ve, err = findVersionedEntry(version, entry) if err != nil { - return url, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err + return url, digest, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err } url, err = repo.ResolveReferenceURL(repoURL, ve.URLs[0]) if err != nil { - return url, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err + return url, digest, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err } + digest = ve.Digest username = cr.Config.Username password = cr.Config.Password passCredentialsAll = cr.Config.PassCredentialsAll @@ -754,14 +756,14 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]* certFile = cr.Config.CertFile keyFile = cr.Config.KeyFile - return url, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err + return url, digest, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err } - url, err = repo.FindChartInRepoURL(repoURL, name, m.Getters, repo.WithChartVersion(version), repo.WithClientTLS(certFile, keyFile, caFile)) + url, digest, err = repo.FindChartInRepoURLWithDigest(repoURL, name, m.Getters, repo.WithChartVersion(version), repo.WithClientTLS(certFile, keyFile, caFile)) if err == nil { - return url, username, password, false, false, "", "", "", nil + return url, digest, username, password, false, false, "", "", "", nil } err = fmt.Errorf("chart %s not found in %s: %w", name, repoURL, err) - return url, username, password, false, false, "", "", "", err + return url, digest, username, password, false, false, "", "", "", err } // findEntryByName finds an entry in the chart repository whose name matches the given name. diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index e40bbbac1..4681bb217 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -27,9 +27,11 @@ import ( "github.com/stretchr/testify/require" "sigs.k8s.io/yaml" + "helm.sh/helm/v4/internal/test/ensure" chart "helm.sh/helm/v4/pkg/chart/v2" "helm.sh/helm/v4/pkg/chart/v2/loader" chartutil "helm.sh/helm/v4/pkg/chart/v2/util" + "helm.sh/helm/v4/pkg/cli" "helm.sh/helm/v4/pkg/getter" "helm.sh/helm/v4/pkg/repo/v1" "helm.sh/helm/v4/pkg/repo/v1/repotest" @@ -66,7 +68,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) require.NoError(t, err) assert.Equal(t, "https://charts.helm.sh/stable/alpine-0.1.0.tgz", churl, "Unexpected URL %q", churl) @@ -79,7 +81,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) require.NoError(t, err) assert.True(t, insecureSkipTLSVerify, "Unexpected insecureSkipTLSVerify %t", insecureSkipTLSVerify) @@ -92,7 +94,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) require.NoError(t, err) assert.Equal(t, "http://example.com/helm/charts/foo-1.2.3.tgz", churl, "Unexpected URL %q", churl) @@ -655,3 +657,45 @@ func TestWriteLock(t *testing.T) { assert.Error(t, writeLock(filePath, lock, false)) }) } + +func TestDownloadAll_RejectsDependencyNotMatchingIndexDigest(t *testing.T) { + tests := []struct { + name string + configured bool + }{ + {name: "repository in repositories.yaml", configured: true}, + {name: "repository not in repositories.yaml", configured: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ensure.HelmHome(t) + contentCache := t.TempDir() + srv, _, _ := tamperedChartServer(t, contentCache) + + repoConfig := filepath.Join(srv.Root(), "repositories.yaml") + if !tt.configured { + repoConfig = filepath.Join(t.TempDir(), "repositories.yaml") + } + chartPath := t.TempDir() + m := &Manager{ + Out: new(bytes.Buffer), + ChartPath: chartPath, + RepositoryConfig: repoConfig, + RepositoryCache: srv.Root(), + ContentCache: contentCache, + Getters: getter.All(&cli.EnvSettings{}), + } + dep := &chart.Dependency{ + Name: "signtest", + Repository: srv.URL(), + Version: "0.1.0", + } + + err := m.downloadAll([]*chart.Dependency{dep}) + require.ErrorContains(t, err, "does not match the digest recorded for it in the repository index") + + _, err = os.Stat(filepath.Join(chartPath, "charts", "signtest-0.1.0.tgz")) + assert.ErrorIs(t, err, fs.ErrNotExist, "rejected dependency must not be saved to charts/") + }) + } +} diff --git a/pkg/repo/v1/chartrepo.go b/pkg/repo/v1/chartrepo.go index da42128bf..025736cc2 100644 --- a/pkg/repo/v1/chartrepo.go +++ b/pkg/repo/v1/chartrepo.go @@ -174,6 +174,15 @@ func WithInsecureSkipTLSVerify(insecureSkipTLSVerify bool) FindChartInRepoURLOpt // FindChartInRepoURL finds chart in chart repository pointed by repoURL // without adding repo to repositories func FindChartInRepoURL(repoURL, chartName string, getters getter.Providers, options ...FindChartInRepoURLOption) (string, error) { + chartURL, _, err := FindChartInRepoURLWithDigest(repoURL, chartName, getters, options...) + return chartURL, err +} + +// FindChartInRepoURLWithDigest is FindChartInRepoURL that also returns the +// digest the repository index records for the chart archive, so the caller can +// check the archive it downloads from the returned URL against it. The digest +// is empty when the index entry does not carry one. +func FindChartInRepoURLWithDigest(repoURL, chartName string, getters getter.Providers, options ...FindChartInRepoURLOption) (string, string, error) { opts := findChartInRepoURLOptions{} for _, option := range options { option(&opts) @@ -197,11 +206,11 @@ func FindChartInRepoURL(repoURL, chartName string, getters getter.Providers, opt } r, err := NewChartRepository(&c, getters) if err != nil { - return "", err + return "", "", err } idx, err := r.DownloadIndexFile() if err != nil { - return "", fmt.Errorf("looks like %q is not a valid chart repository or cannot be reached: %w", repoURL, err) + return "", "", fmt.Errorf("looks like %q is not a valid chart repository or cannot be reached: %w", repoURL, err) } defer func() { os.RemoveAll(filepath.Join(r.CachePath, helmpath.CacheChartsFile(r.Config.Name))) @@ -211,7 +220,7 @@ func FindChartInRepoURL(repoURL, chartName string, getters getter.Providers, opt // Read the index file for the repository to get chart information and return chart URL repoIndex, err := LoadIndexFile(idx) if err != nil { - return "", err + return "", "", err } errMsg := fmt.Sprintf("chart %q", chartName) @@ -220,24 +229,24 @@ func FindChartInRepoURL(repoURL, chartName string, getters getter.Providers, opt } cv, err := repoIndex.Get(chartName, opts.ChartVersion) if err != nil { - return "", ChartNotFoundError{ + return "", "", ChartNotFoundError{ Chart: errMsg, RepoURL: repoURL, } } if len(cv.URLs) == 0 { - return "", fmt.Errorf("%s has no downloadable URLs", errMsg) + return "", "", fmt.Errorf("%s has no downloadable URLs", errMsg) } chartURL := cv.URLs[0] absoluteChartURL, err := ResolveReferenceURL(repoURL, chartURL) if err != nil { - return "", fmt.Errorf("failed to make chart URL absolute: %w", err) + return "", "", fmt.Errorf("failed to make chart URL absolute: %w", err) } - return absoluteChartURL, nil + return absoluteChartURL, cv.Digest, nil } // ResolveReferenceURL resolves refURL relative to baseURL. diff --git a/pkg/repo/v1/chartrepo_test.go b/pkg/repo/v1/chartrepo_test.go index f0e5839ac..ab0ebb03d 100644 --- a/pkg/repo/v1/chartrepo_test.go +++ b/pkg/repo/v1/chartrepo_test.go @@ -196,6 +196,17 @@ func TestFindChartInRepoURL(t *testing.T) { assert.Equalf(t, "https://charts.helm.sh/stable/nginx-0.1.0.tgz", chartURL, "%s is not the valid URL", chartURL) } +func TestFindChartInRepoURLWithDigest(t *testing.T) { + srv, err := startLocalServerForTests(nil) + require.NoError(t, err) + defer srv.Close() + + chartURL, digest, err := FindChartInRepoURLWithDigest(srv.URL, "nginx", getter.All(&cli.EnvSettings{}), WithChartVersion("0.2.0")) + require.NoError(t, err) + assert.Equal(t, "https://charts.helm.sh/stable/nginx-0.2.0.tgz", chartURL) + assert.Equal(t, "sha256:1234567890abcdef", digest) +} + func TestErrorFindChartInRepoURL(t *testing.T) { g := getter.All(&cli.EnvSettings{ RepositoryCache: t.TempDir(),