From 871780c0515ebf1ef7246dbcb003bf19767f48e5 Mon Sep 17 00:00:00 2001 From: ashvinctrl Date: Thu, 24 Sep 2026 23:01:40 +0530 Subject: [PATCH 1/4] fix(downloader): verify a downloaded chart against the index digest ResolveChartVersion returns the sha256 digest the repository index records for a chart, but DownloadTo and DownloadToCache only ever used it as a cache key. The archive itself was never checked against it, so a repository serving an archive that does not match its own index was accepted without error. DownloadToCache then filed those bytes in the content cache under the digest they failed to match, so every later lookup returned them as a cache hit. Hash the bytes before they are used and reject a mismatch. Entries already in the content cache are checked against the digest they are stored under and dropped when they do not match, since an entry written by an earlier version may not. The check is a no-op when the index carries no digest, which is the case for a chart referenced by a bare URL. OCI references are skipped as well: the digest resolved for them names a manifest rather than the archive bytes, and the registry client already verifies it. Signed-off-by: ashvinctrl --- pkg/downloader/chart_downloader.go | 101 ++++++++++++++-- pkg/downloader/chart_downloader_test.go | 148 ++++++++++++++++++++++++ 2 files changed, 239 insertions(+), 10 deletions(-) diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 712c80ad2..f6d79bd23 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -138,9 +138,17 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven if pth, err := c.Cache.Get(digest32, CacheChart); err == nil { fdata, err := os.ReadFile(pth) if err == nil { - found = true - data = bytes.NewBuffer(fdata) - slog.Debug("found chart in cache", "id", hash) + if verr := verifyIndexDigest(ref, u, hash, digest32, fdata); verr != nil { + // An entry that does not hash to the digest it is filed + // under cannot be trusted, whoever wrote it. Drop it and + // download the chart again rather than serving it. + slog.Debug("discarding cache entry that does not match its digest", "id", hash) + _ = os.Remove(pth) + } else { + found = true + data = bytes.NewBuffer(fdata) + slog.Debug("found chart in cache", "id", hash) + } } } } @@ -152,6 +160,9 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven if err != nil { return "", nil, err } + if err := verifyIndexDigest(ref, u, hash, digest32, data.Bytes()); err != nil { + return "", nil, err + } } name := filepath.Base(u.Path) @@ -248,18 +259,29 @@ func (c *ChartDownloader) DownloadToCache(ref, version string) (string, *provena copy(digest32[:], digest) var pth string + var cached bool // only fetch from the cache if we have a digest if len(digest) > 0 { - pth, err = c.Cache.Get(digest32, CacheChart) - if err == nil { - slog.Debug("found chart in cache", "id", digestString) + cachePath, cerr := c.Cache.Get(digest32, CacheChart) + switch { + case cerr == nil: + // The cache is content addressed, but nothing has been enforcing + // that, so an entry written by an older version of Helm may not + // hash to the name it is filed under. Check before trusting it. + if verr := verifyCachedChart(ref, u, digestString, digest32, cachePath); verr != nil { + slog.Debug("discarding cache entry that does not match its digest", "id", digestString) + _ = os.Remove(cachePath) + } else { + pth = cachePath + cached = true + slog.Debug("found chart in cache", "id", digestString) + } + case !os.IsNotExist(cerr): + return "", nil, cerr } } - if len(digest) == 0 || err != nil { + if !cached { slog.Debug("attempting to download chart", "ref", ref, "version", version) - if err != nil && !os.IsNotExist(err) { - return "", nil, err - } // Get file not in the cache data, gerr := g.Get(u.String(), c.Options...) @@ -267,6 +289,12 @@ func (c *ChartDownloader) DownloadToCache(ref, version string) (string, *provena return "", nil, gerr } + // Check the bytes against the digest the index published for them + // before they are written into the content cache under that digest. + if verr := verifyIndexDigest(ref, u, digestString, digest32, data.Bytes()); verr != nil { + return "", nil, verr + } + // Generate the digest if len(digest) == 0 { digest32 = sha256.Sum256(data.Bytes()) @@ -592,6 +620,59 @@ func loadRepoConfig(file string) (*repo.File, error) { return r, nil } +// verifyIndexDigest checks chart archive bytes against the sha256 digest the +// repository index publishes for them. +// +// An index and the archives it points at are routinely served from different +// hosts, so for an HTTP repository this digest is the only thing binding the +// index a user trusts to the bytes they actually receive. It used to be read +// 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 +// 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 { + if !indexDigestApplies(u, digestString) { + return nil + } + return compareChartDigest(ref, sha256.Sum256(data), want) +} + +// verifyCachedChart checks an archive already in the content cache against the +// digest it is filed under, hashing it as a stream so a large chart is not held +// in memory twice. +func verifyCachedChart(ref string, u *url.URL, digestString string, want [sha256.Size]byte, path string) error { + if !indexDigestApplies(u, digestString) { + return nil + } + f, err := os.Open(path) + if err != nil { + return err + } + defer f.Close() + h := sha256.New() + if _, err := io.Copy(h, f); err != nil { + return err + } + var got [sha256.Size]byte + copy(got[:], h.Sum(nil)) + return compareChartDigest(ref, got, want) +} + +func indexDigestApplies(u *url.URL, digestString string) bool { + return digestString != "" && (u == nil || u.Scheme != registry.OCIScheme) +} + +func compareChartDigest(ref string, got, want [sha256.Size]byte) error { + if got != want { + return fmt.Errorf("chart %q does not match the digest recorded for it in the repository index: expected sha256:%s, got sha256:%s", + ref, hex.EncodeToString(want[:]), hex.EncodeToString(got[:])) + } + return nil +} + // stripDigestAlgorithm removes the algorithm prefix (e.g., "sha256:") from a digest string. // If no prefix is present, the original string is returned unchanged. func stripDigestAlgorithm(digest string) string { diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index 15e127b8c..8acb22025 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -16,10 +16,12 @@ limitations under the License. package downloader import ( + "bytes" "crypto/sha256" "encoding/hex" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" "testing" @@ -511,3 +513,149 @@ func TestStripDigestAlgorithm(t *testing.T) { }) } } + +// writeRepoCacheIndex publishes the generated index under the name the repo +// cache looks for. repotest.Server.LinkIndices symlinks instead of copying, +// which needs a privilege Windows does not grant by default. +func writeRepoCacheIndex(t *testing.T, root string) { + t.Helper() + idx, err := os.ReadFile(filepath.Join(root, "index.yaml")) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(root, "test-index.yaml"), idx, 0o644)) +} + +// tamperedChartServer serves a repository whose index records the real digest +// of signtest-0.1.0.tgz while the archive itself has been replaced. It returns +// the server, a downloader pointed at it, and the bytes now being served. +func tamperedChartServer(t *testing.T, contentCache string) (*repotest.Server, *ChartDownloader, []byte) { + t.Helper() + srv := repotest.NewTempServer(t, repotest.WithChartSourceGlob("testdata/*.tgz*")) + t.Cleanup(srv.Stop) + require.NoError(t, srv.CreateIndex()) + writeRepoCacheIndex(t, srv.Root()) + + served := filepath.Join(srv.Root(), "signtest-0.1.0.tgz") + original, err := os.ReadFile(served) + require.NoError(t, err) + tampered := append(append([]byte(nil), original...), []byte("appended by a rewritten mirror")...) + require.NoError(t, os.WriteFile(served, tampered, 0o644)) + + repoFile := filepath.Join(srv.Root(), "repositories.yaml") + c := &ChartDownloader{ + Out: os.Stderr, + Verify: VerifyNever, + RepositoryConfig: repoFile, + RepositoryCache: srv.Root(), + ContentCache: contentCache, + Getters: getter.All(&cli.EnvSettings{ + RepositoryConfig: repoFile, + RepositoryCache: srv.Root(), + ContentCache: contentCache, + }), + Cache: &DiskCache{Root: contentCache}, + } + return srv, c, tampered +} + +func TestDownloadTo_RejectsChartNotMatchingIndexDigest(t *testing.T) { + contentCache := t.TempDir() + dest := t.TempDir() + _, c, _ := tamperedChartServer(t, contentCache) + + _, _, err := c.DownloadTo("test/signtest", "0.1.0", dest) + require.Error(t, err, "a chart that does not match the index digest must not be accepted") + assert.Contains(t, err.Error(), "does not match the digest recorded for it in the repository index") +} + +func TestDownloadToCache_RejectsChartNotMatchingIndexDigest(t *testing.T) { + contentCache := t.TempDir() + _, c, _ := tamperedChartServer(t, contentCache) + + digestString, _, err := c.ResolveChartVersion("test/signtest", "0.1.0") + require.NoError(t, err) + digestBytes, err := hex.DecodeString(stripDigestAlgorithm(digestString)) + require.NoError(t, err) + var want [sha256.Size]byte + copy(want[:], digestBytes) + + _, _, err = c.DownloadToCache("test/signtest", "0.1.0") + require.Error(t, err, "a chart that does not match the index digest must not be accepted") + assert.Contains(t, err.Error(), "does not match the digest recorded for it in the repository index") + + // The rejected bytes must not have been filed in the content cache under + // the digest they failed to match. + _, err = c.Cache.Get(want, CacheChart) + assert.Error(t, err, "rejected chart must not be written to the content cache") +} + +func TestDownloadToCache_DiscardsCacheEntryNotMatchingItsDigest(t *testing.T) { + srv := repotest.NewTempServer(t, repotest.WithChartSourceGlob("testdata/*.tgz*")) + defer srv.Stop() + require.NoError(t, srv.CreateIndex()) + writeRepoCacheIndex(t, srv.Root()) + + repoFile := filepath.Join(srv.Root(), "repositories.yaml") + contentCache := t.TempDir() + c := ChartDownloader{ + Out: os.Stderr, + Verify: VerifyNever, + RepositoryConfig: repoFile, + RepositoryCache: srv.Root(), + ContentCache: contentCache, + Getters: getter.All(&cli.EnvSettings{ + RepositoryConfig: repoFile, + RepositoryCache: srv.Root(), + ContentCache: contentCache, + }), + Cache: &DiskCache{Root: contentCache}, + } + + digestString, _, err := c.ResolveChartVersion("test/signtest", "0.1.0") + require.NoError(t, err) + digestBytes, err := hex.DecodeString(stripDigestAlgorithm(digestString)) + require.NoError(t, err) + var want [sha256.Size]byte + copy(want[:], digestBytes) + + // Poison the content cache the way an older Helm could have: content that + // does not hash to the key it is stored under. + poison := []byte("not the chart this digest names") + _, err = c.Cache.Put(want, bytes.NewBuffer(poison), CacheChart) + require.NoError(t, err) + + pth, _, err := c.DownloadToCache("test/signtest", "0.1.0") + require.NoError(t, err, "a bad cache entry should be replaced by a fresh download, not returned") + + got, err := os.ReadFile(pth) + require.NoError(t, err) + assert.NotEqual(t, poison, got, "poisoned cache entry must not be served") + assert.Equal(t, want, sha256.Sum256(got), "served chart must hash to the index digest") +} + +func TestIndexDigestVerificationScope(t *testing.T) { + good := []byte("chart bytes") + want := sha256.Sum256(good) + httpURL, err := url.Parse("https://example.com/charts/signtest-0.1.0.tgz") + require.NoError(t, err) + ociURL, err := url.Parse("oci://example.com/charts/signtest:0.1.0") + require.NoError(t, err) + + t.Run("mismatch over http is rejected", func(t *testing.T) { + err := verifyIndexDigest("ref", httpURL, hex.EncodeToString(want[:]), want, []byte("other bytes")) + assert.Error(t, err) + }) + t.Run("match over http is accepted", func(t *testing.T) { + err := verifyIndexDigest("ref", httpURL, hex.EncodeToString(want[:]), want, good) + assert.NoError(t, err) + }) + t.Run("no index digest is a no-op", func(t *testing.T) { + // A chart referenced by a bare URL has no index entry to check against. + err := verifyIndexDigest("ref", httpURL, "", [sha256.Size]byte{}, []byte("anything")) + assert.NoError(t, err) + }) + t.Run("oci is left to the registry client", func(t *testing.T) { + // The digest resolved for an OCI ref names a manifest, not these bytes. + err := verifyIndexDigest("ref", ociURL, hex.EncodeToString(want[:]), want, []byte("other bytes")) + assert.NoError(t, err) + }) +} From 46e958ab6e4ecabdb7f93a725f9904fe0cef633c Mon Sep 17 00:00:00 2001 From: ashvinctrl Date: Fri, 25 Sep 2026 21:17:53 +0530 Subject: [PATCH 2/4] test(downloader): assert a rejected chart leaves nothing in dest Signed-off-by: ashvinctrl --- pkg/downloader/chart_downloader_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index 8acb22025..9ff69fedd 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -565,6 +565,11 @@ func TestDownloadTo_RejectsChartNotMatchingIndexDigest(t *testing.T) { _, _, err := c.DownloadTo("test/signtest", "0.1.0", dest) require.Error(t, err, "a chart that does not match the index digest must not be accepted") assert.Contains(t, err.Error(), "does not match the digest recorded for it in the repository index") + + // Nothing may be left behind in dest for a later step to pick up. + entries, err := os.ReadDir(dest) + require.NoError(t, err) + assert.Empty(t, entries, "rejected chart must not be written to the destination") } func TestDownloadToCache_RejectsChartNotMatchingIndexDigest(t *testing.T) { From 91624b1f902a3801352e307100add2cb26ef0b28 Mon Sep 17 00:00:00 2001 From: ashvinctrl Date: Sat, 26 Sep 2026 12:46:17 +0530 Subject: [PATCH 3/4] 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(), From 662f0b36708c0d83bba2dc5625345f1421ea2887 Mon Sep 17 00:00:00 2001 From: ashvinctrl Date: Sat, 26 Sep 2026 15:02:24 +0530 Subject: [PATCH 4/4] fix(downloader): deduplicate dependency downloads by URL and digest downloadAll skipped a dependency whose archive URL had already been downloaded, keyed on the URL alone. When two repositories list the same archive URL and the first entry has no digest or a different one, the second entry's digest was never checked. Key the skip on the URL and the index digest together. Signed-off-by: ashvinctrl --- pkg/downloader/manager.go | 10 ++++++--- pkg/downloader/manager_test.go | 40 ++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 13a7baeb2..131b7098f 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -275,7 +275,11 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { fmt.Fprintf(m.Out, "Saving %d charts\n", len(deps)) var saveError error - churls := make(map[string]struct{}) + // Downloads are deduplicated by digest as well as URL, so an archive that + // another repository listed under a different digest, or none, is still + // checked against this one. + type download struct{ url, digest string } + churls := make(map[download]struct{}) for _, dep := range deps { // No repository means the chart is in charts directory if dep.Repository == "" { @@ -324,7 +328,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { break } - if _, ok := churls[churl]; ok { + if _, ok := churls[download{churl, digest}]; ok { fmt.Fprintf(m.Out, "Already downloaded %s from repo %s\n", dep.Name, dep.Repository) continue } @@ -365,7 +369,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { break } - churls[churl] = struct{}{} + churls[download{churl, digest}] = struct{}{} } // TODO: this should probably be refactored to be a []error, so we can capture and provide more information rather than "last error wins". diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index 4681bb217..524b9c46c 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -18,6 +18,8 @@ package downloader import ( "bytes" "io/fs" + "net/http" + "net/http/httptest" "os" "path/filepath" "testing" @@ -699,3 +701,41 @@ func TestDownloadAll_RejectsDependencyNotMatchingIndexDigest(t *testing.T) { }) } } + +func TestDownloadAll_ChecksEachIndexDigestForSharedURL(t *testing.T) { + ensure.HelmHome(t) + contentCache := t.TempDir() + srv, _, _ := tamperedChartServer(t, contentCache) + + // A second repository lists the same archive URL without a digest. When + // its entry is downloaded first, the one that does carry a digest must + // still be checked rather than skipped as already downloaded. + idx, err := repo.LoadIndexFile(filepath.Join(srv.Root(), "index.yaml")) + require.NoError(t, err) + for _, cv := range idx.Entries["signtest"] { + cv.Digest = "" + } + noDigest, err := yaml.Marshal(idx) + require.NoError(t, err) + other := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(noDigest) + })) + t.Cleanup(other.Close) + + chartPath := t.TempDir() + m := &Manager{ + Out: new(bytes.Buffer), + ChartPath: chartPath, + RepositoryConfig: filepath.Join(t.TempDir(), "repositories.yaml"), + RepositoryCache: srv.Root(), + ContentCache: contentCache, + Getters: getter.All(&cli.EnvSettings{}), + } + deps := []*chart.Dependency{ + {Name: "signtest", Repository: other.URL, Version: "0.1.0"}, + {Name: "signtest", Repository: srv.URL(), Version: "0.1.0", Alias: "signtest-indexed"}, + } + + err = m.downloadAll(deps) + require.ErrorContains(t, err, "does not match the digest recorded for it in the repository index") +}