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