From 871780c0515ebf1ef7246dbcb003bf19767f48e5 Mon Sep 17 00:00:00 2001 From: ashvinctrl Date: Thu, 24 Sep 2026 23:01:40 +0530 Subject: [PATCH] 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) + }) +}