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 <sharmaashvin27@gmail.com>

pkg/downloader/chart_downloader.go

@@ -138,12 +138,20 @@ 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 {
+				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)
 				}
 			}
 		}
+	}
 
 	if !found {
 		c.Options = append(c.Options, getter.WithAcceptHeader("application/gzip,application/octet-stream"))
@@ -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)
+		cachePath, cerr := c.Cache.Get(digest32, CacheChart)
-		if err == nil {
+		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 {
-		slog.Debug("attempting to download chart", "ref", ref, "version", version)
-		if err != nil && !os.IsNotExist(err) {
-			return "", nil, err
 	}
+	if !cached {
+		slog.Debug("attempting to download chart", "ref", ref, "version", version)
 
 		// 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 {

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)
+	})
+}