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 c564583a5b624470b306a42a6fcde2283bb16a62 Mon Sep 17 00:00:00 2001 From: ashvinctrl Date: Fri, 25 Sep 2026 21:37:26 +0530 Subject: [PATCH 3/4] fix(downloader): key cached OCI charts by their chart layer digest For an OCI reference pinned to a digest, the content cache stored the chart archive under the manifest digest. The archive can never hash to that key, so a cache entry could not be checked and a modified one was served as-is by DownloadTo and DownloadToCache. Fetch the pinned manifest (the registry client verifies it against the pin) and use its chart layer digest as the cache key instead. Every entry is then keyed by its own content, and the digest checks already used for repository charts now cover OCI charts too. Signed-off-by: ashvinctrl --- pkg/downloader/chart_downloader.go | 86 ++++++++++++--- pkg/downloader/chart_downloader_test.go | 138 +++++++++++++++++++++--- 2 files changed, 192 insertions(+), 32 deletions(-) diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index f6d79bd23..29b7f09e7 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -19,6 +19,7 @@ import ( "bytes" "crypto/sha256" "encoding/hex" + "encoding/json" "errors" "fmt" "io" @@ -29,6 +30,8 @@ import ( "path/filepath" "strings" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" + "helm.sh/helm/v4/internal/fileutil" ifs "helm.sh/helm/v4/internal/third_party/dep/fs" "helm.sh/helm/v4/internal/urlutil" @@ -106,7 +109,7 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven c.Cache = &DiskCache{Root: c.ContentCache} slog.Debug("set up default downloader cache") } - hash, u, err := c.ResolveChartVersion(ref, version) + hash, u, err := c.resolveCacheDigest(ref, version) if err != nil { return "", nil, err } @@ -138,7 +141,7 @@ 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 { + if verr := verifyIndexDigest(ref, 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. @@ -160,7 +163,7 @@ 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 { + if err := verifyIndexDigest(ref, hash, digest32, data.Bytes()); err != nil { return "", nil, err } } @@ -234,7 +237,7 @@ func (c *ChartDownloader) DownloadToCache(ref, version string) (string, *provena slog.Debug("set up default downloader cache") } - digestString, u, err := c.ResolveChartVersion(ref, version) + digestString, u, err := c.resolveCacheDigest(ref, version) if err != nil { return "", nil, err } @@ -268,7 +271,7 @@ func (c *ChartDownloader) DownloadToCache(ref, version string) (string, *provena // 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 { + if verr := verifyCachedChart(ref, digestString, digest32, cachePath); verr != nil { slog.Debug("discarding cache entry that does not match its digest", "id", digestString) _ = os.Remove(cachePath) } else { @@ -291,7 +294,7 @@ func (c *ChartDownloader) DownloadToCache(ref, version string) (string, *provena // 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 { + if verr := verifyIndexDigest(ref, digestString, digest32, data.Bytes()); verr != nil { return "", nil, verr } @@ -620,6 +623,60 @@ func loadRepoConfig(file string) (*repo.File, error) { return r, nil } +// resolveCacheDigest resolves ref like ResolveChartVersion, but returns the +// digest the content cache should use for the chart archive. +// +// For a repository chart that is the index digest, which is already the sha256 +// of the archive. For an OCI reference pinned to a digest it is not: that digest +// names the manifest, so an archive cached under it could never be checked +// against its key. In that case the manifest is fetched (the registry client +// verifies it against the pinned digest) and the digest of its chart layer is +// returned instead, which keeps every cache entry keyed by its own content. +func (c *ChartDownloader) resolveCacheDigest(ref, version string) (string, *url.URL, error) { + d, u, err := c.ResolveChartVersion(ref, version) + if err != nil || d == "" || u.Scheme != registry.OCIScheme { + return d, u, err + } + layer, err := c.ociChartLayerDigest(u) + if err != nil { + return "", nil, fmt.Errorf("unable to resolve chart layer for %s: %w", ref, err) + } + return layer, u, nil +} + +// ociChartLayerDigest fetches only the manifest u points at and returns the +// sha256 digest of its chart layer. +func (c *ChartDownloader) ociChartLayerDigest(u *url.URL) (string, error) { + generic := c.RegistryClient.Generic() + result, err := generic.PullGeneric(strings.TrimPrefix(u.String(), registry.OCIScheme+"://"), registry.GenericPullOptions{ + AllowedMediaTypes: []string{ocispec.MediaTypeImageManifest}, + }) + if err != nil { + return "", err + } + data, err := generic.GetDescriptorData(result.MemoryStore, result.Manifest) + if err != nil { + return "", err + } + var manifest ocispec.Manifest + if err := json.Unmarshal(data, &manifest); err != nil { + return "", err + } + for _, layer := range manifest.Layers { + if layer.MediaType != registry.ChartLayerMediaType && layer.MediaType != registry.LegacyChartLayerMediaType { + continue + } + if layer.Digest.Algorithm() != "sha256" { + return "", fmt.Errorf("unsupported chart layer digest algorithm %q", layer.Digest.Algorithm()) + } + if err := layer.Digest.Validate(); err != nil { + return "", err + } + return layer.Digest.String(), nil + } + return "", fmt.Errorf("manifest does not contain a layer with mediatype %s", registry.ChartLayerMediaType) +} + // verifyIndexDigest checks chart archive bytes against the sha256 digest the // repository index publishes for them. // @@ -630,11 +687,10 @@ func loadRepoConfig(file string) (*repo.File, error) { // 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) { +// referenced by a bare URL, so those keep working as before. For an OCI +// reference the digest is the chart layer digest from resolveCacheDigest. +func verifyIndexDigest(ref, digestString string, want [sha256.Size]byte, data []byte) error { + if digestString == "" { return nil } return compareChartDigest(ref, sha256.Sum256(data), want) @@ -643,8 +699,8 @@ func verifyIndexDigest(ref string, u *url.URL, digestString string, want [sha256 // 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) { +func verifyCachedChart(ref, digestString string, want [sha256.Size]byte, path string) error { + if digestString == "" { return nil } f, err := os.Open(path) @@ -661,10 +717,6 @@ func verifyCachedChart(ref string, u *url.URL, digestString string, want [sha256 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", diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index 9ff69fedd..7e049dea6 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -19,12 +19,13 @@ import ( "bytes" "crypto/sha256" "encoding/hex" + "net" "net/http" "net/http/httptest" - "net/url" "os" "path/filepath" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -640,27 +641,134 @@ func TestDownloadToCache_DiscardsCacheEntryNotMatchingItsDigest(t *testing.T) { 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")) + t.Run("mismatch is rejected", func(t *testing.T) { + err := verifyIndexDigest("ref", 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) + t.Run("match is accepted", func(t *testing.T) { + err := verifyIndexDigest("ref", 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")) + err := verifyIndexDigest("ref", "", [sha256.Size]byte{}, []byte("anything")) assert.NoError(t, err) }) } + +// ociChartDownloader pushes testdata/signtest-0.1.0.tgz to an in-process +// registry and returns a downloader wired to it, the chart reference pinned to +// the pushed manifest digest, and the push result. +func ociChartDownloader(t *testing.T, contentCache string) (*ChartDownloader, string, *registry.PushResult) { + t.Helper() + dir := t.TempDir() + srv, err := repotest.NewOCIServer(t, dir) + require.NoError(t, err) + go srv.ListenAndServe() + dialer := &net.Dialer{Timeout: time.Second} + require.Eventually(t, func() bool { + conn, err := dialer.DialContext(t.Context(), "tcp", srv.RegistryURL) + if err != nil { + return false + } + conn.Close() + return true + }, 30*time.Second, 20*time.Millisecond) + + client, err := registry.NewClient( + registry.ClientOptCredentialsFile(filepath.Join(dir, "config.json")), + registry.ClientOptPlainHTTP(), + ) + require.NoError(t, err) + require.NoError(t, client.Login(srv.RegistryURL, + registry.LoginOptBasicAuth(srv.TestUsername, srv.TestPassword), + registry.LoginOptInsecure(true), + registry.LoginOptPlainText(true))) + + archive, err := os.ReadFile("testdata/signtest-0.1.0.tgz") + require.NoError(t, err) + pushed, err := client.Push(archive, srv.RegistryURL+"/u/ocitestuser/signtest:0.1.0") + require.NoError(t, err) + + settings := &cli.EnvSettings{ContentCache: contentCache} + c := &ChartDownloader{ + Out: os.Stderr, + Verify: VerifyNever, + ContentCache: contentCache, + Getters: getter.All(settings), + Options: []getter.Option{getter.WithRegistryClient(client)}, + RegistryClient: client, + Cache: &DiskCache{Root: contentCache}, + } + ref := "oci://" + srv.RegistryURL + "/u/ocitestuser/signtest@" + pushed.Manifest.Digest + return c, ref, pushed +} + +func digestKey(t *testing.T, d string) [sha256.Size]byte { + t.Helper() + b, err := hex.DecodeString(stripDigestAlgorithm(d)) + require.NoError(t, err) + require.Len(t, b, sha256.Size) + var k [sha256.Size]byte + copy(k[:], b) + return k +} + +func TestDownloadToCache_OCIKeyedByChartLayerDigest(t *testing.T) { + contentCache := t.TempDir() + c, ref, pushed := ociChartDownloader(t, contentCache) + layer := digestKey(t, pushed.Chart.Digest) + + pth, _, err := c.DownloadToCache(ref, "0.1.0") + require.NoError(t, err) + got, err := os.ReadFile(pth) + require.NoError(t, err) + assert.Equal(t, layer, sha256.Sum256(got), "cached chart must hash to its chart layer digest") + + // The entry must be filed under the chart layer digest, where it can be + // checked, and not under the manifest digest, where it could not. + layerPath, err := c.Cache.Get(layer, CacheChart) + require.NoError(t, err) + assert.Equal(t, layerPath, pth) + _, err = c.Cache.Get(digestKey(t, pushed.Manifest.Digest), CacheChart) + assert.ErrorIs(t, err, os.ErrNotExist) +} + +func TestDownloadToCache_OCIDiscardsCacheEntryNotMatchingItsDigest(t *testing.T) { + contentCache := t.TempDir() + c, ref, pushed := ociChartDownloader(t, contentCache) + layer := digestKey(t, pushed.Chart.Digest) + + poison := []byte("not the chart this digest names") + _, err := c.Cache.Put(layer, bytes.NewBuffer(poison), CacheChart) + require.NoError(t, err) + + pth, _, err := c.DownloadToCache(ref, "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.Equal(t, layer, sha256.Sum256(got), "served chart must hash to its chart layer digest") +} + +func TestDownloadTo_OCIIgnoresCacheEntryNotMatchingItsDigest(t *testing.T) { + contentCache := t.TempDir() + dest := t.TempDir() + c, ref, pushed := ociChartDownloader(t, contentCache) + layer := digestKey(t, pushed.Chart.Digest) + + // Before this change an OCI chart was cached under its manifest digest. + // Neither an entry like that nor a bad one under the layer digest may be + // served in place of the chart. + poison := []byte("not the chart this digest names") + _, err := c.Cache.Put(digestKey(t, pushed.Manifest.Digest), bytes.NewBuffer(poison), CacheChart) + require.NoError(t, err) + _, err = c.Cache.Put(layer, bytes.NewBuffer(poison), CacheChart) + require.NoError(t, err) + + saved, _, err := c.DownloadTo(ref, "0.1.0", dest) + require.NoError(t, err) + got, err := os.ReadFile(saved) + require.NoError(t, err) + assert.Equal(t, layer, sha256.Sum256(got), "saved chart must hash to its chart layer digest") +} From 298d0a11e16d0d90f3c94782c26a1ce8f01572f3 Mon Sep 17 00:00:00 2001 From: ashvinctrl Date: Fri, 25 Sep 2026 21:47:39 +0530 Subject: [PATCH 4/4] fix(downloader): skip the cache for OCI refs pinned to an image index A digest-pinned OCI ref can name an image index rather than a manifest. The chart layer lookup expected a manifest and failed, so charts behind an index stopped downloading at all. Return no cache digest for those, so they are downloaded directly the same way digest-only refs are. Also add a test for a digest-only ref. Signed-off-by: ashvinctrl --- pkg/downloader/chart_downloader.go | 10 +++- pkg/downloader/chart_downloader_test.go | 76 +++++++++++++++++++++++-- 2 files changed, 79 insertions(+), 7 deletions(-) diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 29b7f09e7..5f5c954b4 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -632,6 +632,10 @@ func loadRepoConfig(file string) (*repo.File, error) { // against its key. In that case the manifest is fetched (the registry client // verifies it against the pinned digest) and the digest of its chart layer is // returned instead, which keeps every cache entry keyed by its own content. +// +// If the pinned digest names something other than an image manifest, such as +// an image index, no cache digest is returned. The chart is then downloaded +// rather than read from the cache, as it already is for a digest-only ref. func (c *ChartDownloader) resolveCacheDigest(ref, version string) (string, *url.URL, error) { d, u, err := c.ResolveChartVersion(ref, version) if err != nil || d == "" || u.Scheme != registry.OCIScheme { @@ -645,7 +649,8 @@ func (c *ChartDownloader) resolveCacheDigest(ref, version string) (string, *url. } // ociChartLayerDigest fetches only the manifest u points at and returns the -// sha256 digest of its chart layer. +// sha256 digest of its chart layer, or "" if u does not point at an image +// manifest. func (c *ChartDownloader) ociChartLayerDigest(u *url.URL) (string, error) { generic := c.RegistryClient.Generic() result, err := generic.PullGeneric(strings.TrimPrefix(u.String(), registry.OCIScheme+"://"), registry.GenericPullOptions{ @@ -654,6 +659,9 @@ func (c *ChartDownloader) ociChartLayerDigest(u *url.URL) (string, error) { if err != nil { return "", err } + if result.Manifest.MediaType != ocispec.MediaTypeImageManifest { + return "", nil + } data, err := generic.GetDescriptorData(result.MemoryStore, result.Manifest) if err != nil { return "", err diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index 7e049dea6..c7ea23ac9 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -17,8 +17,10 @@ package downloader import ( "bytes" + "context" "crypto/sha256" "encoding/hex" + "encoding/json" "net" "net/http" "net/http/httptest" @@ -27,8 +29,13 @@ import ( "testing" "time" + godigest "github.com/opencontainers/go-digest" + specs "github.com/opencontainers/image-spec/specs-go" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "oras.land/oras-go/v2/registry/remote" + "oras.land/oras-go/v2/registry/remote/auth" "helm.sh/helm/v4/internal/test/ensure" "helm.sh/helm/v4/pkg/cli" @@ -659,8 +666,8 @@ func TestIndexDigestVerificationScope(t *testing.T) { // ociChartDownloader pushes testdata/signtest-0.1.0.tgz to an in-process // registry and returns a downloader wired to it, the chart reference pinned to -// the pushed manifest digest, and the push result. -func ociChartDownloader(t *testing.T, contentCache string) (*ChartDownloader, string, *registry.PushResult) { +// the pushed manifest digest, the push result and the registry. +func ociChartDownloader(t *testing.T, contentCache string) (*ChartDownloader, string, *registry.PushResult, *repotest.OCIServer) { t.Helper() dir := t.TempDir() srv, err := repotest.NewOCIServer(t, dir) @@ -702,7 +709,7 @@ func ociChartDownloader(t *testing.T, contentCache string) (*ChartDownloader, st Cache: &DiskCache{Root: contentCache}, } ref := "oci://" + srv.RegistryURL + "/u/ocitestuser/signtest@" + pushed.Manifest.Digest - return c, ref, pushed + return c, ref, pushed, srv } func digestKey(t *testing.T, d string) [sha256.Size]byte { @@ -717,7 +724,7 @@ func digestKey(t *testing.T, d string) [sha256.Size]byte { func TestDownloadToCache_OCIKeyedByChartLayerDigest(t *testing.T) { contentCache := t.TempDir() - c, ref, pushed := ociChartDownloader(t, contentCache) + c, ref, pushed, _ := ociChartDownloader(t, contentCache) layer := digestKey(t, pushed.Chart.Digest) pth, _, err := c.DownloadToCache(ref, "0.1.0") @@ -737,7 +744,7 @@ func TestDownloadToCache_OCIKeyedByChartLayerDigest(t *testing.T) { func TestDownloadToCache_OCIDiscardsCacheEntryNotMatchingItsDigest(t *testing.T) { contentCache := t.TempDir() - c, ref, pushed := ociChartDownloader(t, contentCache) + c, ref, pushed, _ := ociChartDownloader(t, contentCache) layer := digestKey(t, pushed.Chart.Digest) poison := []byte("not the chart this digest names") @@ -754,7 +761,7 @@ func TestDownloadToCache_OCIDiscardsCacheEntryNotMatchingItsDigest(t *testing.T) func TestDownloadTo_OCIIgnoresCacheEntryNotMatchingItsDigest(t *testing.T) { contentCache := t.TempDir() dest := t.TempDir() - c, ref, pushed := ociChartDownloader(t, contentCache) + c, ref, pushed, _ := ociChartDownloader(t, contentCache) layer := digestKey(t, pushed.Chart.Digest) // Before this change an OCI chart was cached under its manifest digest. @@ -772,3 +779,60 @@ func TestDownloadTo_OCIIgnoresCacheEntryNotMatchingItsDigest(t *testing.T) { require.NoError(t, err) assert.Equal(t, layer, sha256.Sum256(got), "saved chart must hash to its chart layer digest") } + +func TestDownloadTo_OCIDigestOnlyRef(t *testing.T) { + c, ref, pushed, _ := ociChartDownloader(t, t.TempDir()) + + // With no version the registry client resolves no digest for the ref, so + // the cache is not consulted and the chart is downloaded directly. + saved, _, err := c.DownloadTo(ref, "", t.TempDir()) + require.NoError(t, err) + got, err := os.ReadFile(saved) + require.NoError(t, err) + assert.Equal(t, digestKey(t, pushed.Chart.Digest), sha256.Sum256(got)) +} + +func TestDownloadToCache_OCIImageIndexRoot(t *testing.T) { + contentCache := t.TempDir() + c, _, pushed, srv := ociChartDownloader(t, contentCache) + layer := digestKey(t, pushed.Chart.Digest) + + // Wrap the chart manifest in an image index and move the tag onto it. + repo, err := remote.NewRepository(srv.RegistryURL + "/u/ocitestuser/signtest") + require.NoError(t, err) + repo.PlainHTTP = true + repo.Client = &auth.Client{Credential: auth.StaticCredential(srv.RegistryURL, auth.Credential{ + Username: srv.TestUsername, + Password: srv.TestPassword, + })} + ctx := context.Background() + manifest, err := repo.Resolve(ctx, "0.1.0") + require.NoError(t, err) + index, err := json.Marshal(ocispec.Index{ + Versioned: specs.Versioned{SchemaVersion: 2}, + MediaType: ocispec.MediaTypeImageIndex, + Manifests: []ocispec.Descriptor{manifest}, + }) + require.NoError(t, err) + indexDesc := ocispec.Descriptor{ + MediaType: ocispec.MediaTypeImageIndex, + Digest: godigest.FromBytes(index), + Size: int64(len(index)), + } + require.NoError(t, repo.PushReference(ctx, indexDesc, bytes.NewReader(index), "0.1.0")) + ref := "oci://" + srv.RegistryURL + "/u/ocitestuser/signtest@" + indexDesc.Digest.String() + + pth, _, err := c.DownloadToCache(ref, "0.1.0") + require.NoError(t, err, "a chart behind an image index must still download") + got, err := os.ReadFile(pth) + require.NoError(t, err) + assert.Equal(t, layer, sha256.Sum256(got)) + _, err = c.Cache.Get(digestKey(t, indexDesc.Digest.String()), CacheChart) + require.ErrorIs(t, err, os.ErrNotExist, "chart must not be cached under the index digest") + + saved, _, err := c.DownloadTo(ref, "0.1.0", t.TempDir()) + require.NoError(t, err) + got, err = os.ReadFile(saved) + require.NoError(t, err) + assert.Equal(t, layer, sha256.Sum256(got)) +}