From c564583a5b624470b306a42a6fcde2283bb16a62 Mon Sep 17 00:00:00 2001 From: ashvinctrl Date: Fri, 25 Sep 2026 21:37:26 +0530 Subject: [PATCH] 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") +}