diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 712c80ad2..5f5c954b4 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,9 +141,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, 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 +163,9 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven if err != nil { return "", nil, err } + if err := verifyIndexDigest(ref, hash, digest32, data.Bytes()); err != nil { + return "", nil, err + } } name := filepath.Base(u.Path) @@ -223,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 } @@ -248,18 +262,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, 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 +292,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, digestString, digest32, data.Bytes()); verr != nil { + return "", nil, verr + } + // Generate the digest if len(digest) == 0 { digest32 = sha256.Sum256(data.Bytes()) @@ -592,6 +623,116 @@ 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. +// +// 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 { + 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, 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{ + AllowedMediaTypes: []string{ocispec.MediaTypeImageManifest}, + }) + 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 + } + 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. +// +// 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. 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) +} + +// 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, digestString string, want [sha256.Size]byte, path string) error { + if 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 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..c7ea23ac9 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -16,16 +16,26 @@ limitations under the License. package downloader import ( + "bytes" + "context" "crypto/sha256" "encoding/hex" + "encoding/json" + "net" "net/http" "net/http/httptest" "os" "path/filepath" "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" @@ -511,3 +521,318 @@ 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") + + // 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) { + 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) + + 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 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", "", [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, 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) + 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, srv +} + +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") +} + +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)) +}