diff --git a/pkg/action/install.go b/pkg/action/install.go index 6fc919366..2eb3e027d 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -937,7 +937,7 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) ( dl.Verify = downloader.VerifyAlways } if c.RepoURL != "" { - chartURL, err := repo.FindChartInRepoURL( + chartURL, digest, err := repo.FindChartInRepoURLWithDigest( c.RepoURL, name, getter.All(settings), @@ -951,6 +951,7 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) ( return "", err } name = chartURL + dl.IndexDigest = digest // Only pass the user/pass on when the user has said to or when the // location of the chart repo and the chart are the same domain. diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index 2d83abe27..67fdf20cc 100644 --- a/pkg/action/install_test.go +++ b/pkg/action/install_test.go @@ -47,8 +47,10 @@ import ( ci "helm.sh/helm/v4/pkg/chart" "helm.sh/helm/v4/internal/test" + "helm.sh/helm/v4/internal/test/ensure" "helm.sh/helm/v4/pkg/chart/common" chart "helm.sh/helm/v4/pkg/chart/v2" + "helm.sh/helm/v4/pkg/cli" "helm.sh/helm/v4/pkg/kube" kubefake "helm.sh/helm/v4/pkg/kube/fake" "helm.sh/helm/v4/pkg/registry" @@ -1253,3 +1255,19 @@ func TestInstallRelease_WaitOptionsPassedDownstream(t *testing.T) { // Verify that WaitOptions were passed to GetWaiter is.NotEmpty(failer.RecordedWaitOptions, "WaitOptions should be passed to GetWaiter") } + +func TestLocateChart_RepoURLRejectsChartNotMatchingIndexDigest(t *testing.T) { + ensure.HelmHome(t) + srv := tamperedRepoServer(t) + settings := cli.New() + + c := &ChartPathOptions{RepoURL: srv.URL(), Version: "0.1.0"} + _, err := c.LocateChart("signtest", settings) + require.ErrorContains(t, err, "does not match the digest recorded for it in the repository index") + + entries, err := os.ReadDir(settings.ContentCache) + if !errors.Is(err, fs.ErrNotExist) { + require.NoError(t, err) + assert.Empty(t, entries, "rejected chart must not be written to the content cache") + } +} diff --git a/pkg/action/pull.go b/pkg/action/pull.go index 168011d40..bf81741f1 100644 --- a/pkg/action/pull.go +++ b/pkg/action/pull.go @@ -117,7 +117,7 @@ func (p *Pull) Run(chartRef string) (string, error) { downloadSourceRef := chartRef if p.RepoURL != "" { - chartURL, err := repo.FindChartInRepoURL( + chartURL, digest, err := repo.FindChartInRepoURLWithDigest( p.RepoURL, chartRef, getter.All(p.Settings), @@ -131,6 +131,7 @@ func (p *Pull) Run(chartRef string) (string, error) { return out.String(), err } downloadSourceRef = chartURL + c.IndexDigest = digest } saved, v, err := c.DownloadTo(downloadSourceRef, p.Version, dest) diff --git a/pkg/action/pull_test.go b/pkg/action/pull_test.go index a483de248..24c5f1824 100644 --- a/pkg/action/pull_test.go +++ b/pkg/action/pull_test.go @@ -17,16 +17,21 @@ limitations under the License. package action import ( + "bytes" "net/http" "net/http/httptest" "os" + "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "helm.sh/helm/v4/internal/test/ensure" "helm.sh/helm/v4/pkg/cli" "helm.sh/helm/v4/pkg/registry" + "helm.sh/helm/v4/pkg/repo/v1/repotest" ) func TestNewPull(t *testing.T) { @@ -47,7 +52,16 @@ func TestPullSetRegistryClient(t *testing.T) { } func TestPullRun_ChartNotFound(t *testing.T) { - srv, err := startLocalServerForTests(t, nil) + fileBytes, err := os.ReadFile("../repo/v1/testdata/local-index.yaml") + require.NoError(t, err) + // The fixture's placeholder digest is not a valid sha256, and --repo pulls + // check the index digest, so give it a well-formed one to let the pull get + // as far as the missing archive. + fileBytes = bytes.ReplaceAll(fileBytes, []byte("sha256:1234567890abcdef"), []byte("sha256:"+strings.Repeat("0", 64))) + srv, err := startLocalServerForTests(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, err := w.Write(fileBytes) + assert.NoError(t, err) + })) require.NoError(t, err) defer srv.Close() @@ -76,3 +90,36 @@ func startLocalServerForTests(t *testing.T, handler http.Handler) (*httptest.Ser return httptest.NewServer(handler), nil } + +// tamperedRepoServer serves a chart repository whose index records the digest +// of signtest-0.1.0.tgz while the archive it serves has changed since. +func tamperedRepoServer(t *testing.T) *repotest.Server { + t.Helper() + srv := repotest.NewTempServer(t, repotest.WithChartSourceGlob("../downloader/testdata/signtest-0.1.0.tgz")) + t.Cleanup(srv.Stop) + + served := filepath.Join(srv.Root(), "signtest-0.1.0.tgz") + original, err := os.ReadFile(served) + require.NoError(t, err) + require.NoError(t, os.WriteFile(served, append(original, "appended by a rewritten mirror"...), 0o644)) + return srv +} + +func TestPullRun_RepoURLRejectsChartNotMatchingIndexDigest(t *testing.T) { + ensure.HelmHome(t) + srv := tamperedRepoServer(t) + + config := actionConfigFixture(t) + client := NewPull(WithConfig(config)) + client.Settings = cli.New() + client.RepoURL = srv.URL() + client.Version = "0.1.0" + client.DestDir = t.TempDir() + + _, err := client.Run("signtest") + require.ErrorContains(t, err, "does not match the digest recorded for it in the repository index") + + entries, err := os.ReadDir(client.DestDir) + require.NoError(t, err) + assert.Empty(t, entries, "rejected chart must not be written to the destination") +} diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 712c80ad2..7e875c659 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -85,6 +85,13 @@ type ChartDownloader struct { // Cache specifies the cache implementation to use. Cache Cache + + // IndexDigest is the digest a repository index records for the chart when + // the caller has already resolved the index entry to an absolute URL and + // passes that URL as the ref, as `--repo` and dependency downloads do. The + // downloaded archive is checked against it. It is ignored for any other + // kind of ref. + IndexDigest string } // DownloadTo retrieves a chart. Depending on the settings, it may also download a provenance file. @@ -138,9 +145,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 +167,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 +266,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 +296,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()) @@ -396,7 +431,7 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (string, *url if errors.Is(err, ErrNoOwnerRepo) { // Make sure to add the ref URL as the URL for the getter c.Options = append(c.Options, getter.WithURL(ref)) - return "", u, nil + return c.IndexDigest, u, nil } return "", u, err } @@ -417,7 +452,7 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (string, *url getter.WithPassCredentialsAll(rc.PassCredentialsAll), ) } - return "", u, nil + return c.IndexDigest, u, nil } // See if it's of the form: repo/path_to_chart @@ -592,6 +627,60 @@ 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 there is no index digest to check against, which is the +// case for a chart referenced by a bare URL with no IndexDigest set, 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..1a68a1651 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,195 @@ 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) + 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) + }) +} + +func TestDownload_ResolvedURLCheckedAgainstIndexDigest(t *testing.T) { + // Callers such as `helm pull --repo` and dependency downloads look the + // chart up in the index themselves and pass on only its absolute URL, so + // the index digest has to come in through IndexDigest. + setup := func(t *testing.T) (*ChartDownloader, string) { + t.Helper() + srv, c, _ := tamperedChartServer(t, t.TempDir()) + idx, err := repo.LoadIndexFile(filepath.Join(srv.Root(), "index.yaml")) + require.NoError(t, err) + cv, err := idx.Get("signtest", "0.1.0") + require.NoError(t, err) + require.NotEmpty(t, cv.Digest) + c.IndexDigest = cv.Digest + return c, srv.URL() + "/signtest-0.1.0.tgz" + } + + t.Run("DownloadTo", func(t *testing.T) { + c, chartURL := setup(t) + dest := t.TempDir() + _, _, err := c.DownloadTo(chartURL, "", dest) + require.ErrorContains(t, err, "does not match the digest recorded for it in the repository index") + + entries, err := os.ReadDir(dest) + require.NoError(t, err) + assert.Empty(t, entries, "rejected chart must not be written to the destination") + }) + + t.Run("DownloadToCache", func(t *testing.T) { + c, chartURL := setup(t) + _, _, err := c.DownloadToCache(chartURL, "") + require.ErrorContains(t, err, "does not match the digest recorded for it in the repository index") + + digestBytes, err := hex.DecodeString(stripDigestAlgorithm(c.IndexDigest)) + require.NoError(t, err) + var want [sha256.Size]byte + copy(want[:], digestBytes) + _, err = c.Cache.Get(want, CacheChart) + assert.Error(t, err, "rejected chart must not be written to the content cache") + }) +} diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 2e94d26ae..131b7098f 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -275,7 +275,11 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { fmt.Fprintf(m.Out, "Saving %d charts\n", len(deps)) var saveError error - churls := make(map[string]struct{}) + // Downloads are deduplicated by digest as well as URL, so an archive that + // another repository listed under a different digest, or none, is still + // checked against this one. + type download struct{ url, digest string } + churls := make(map[download]struct{}) for _, dep := range deps { // No repository means the chart is in charts directory if dep.Repository == "" { @@ -318,13 +322,13 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { // Any failure to resolve/download a chart should fail: // https://github.com/helm/helm/issues/1439 - churl, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err := m.findChartURL(dep.Name, dep.Version, dep.Repository, repos) + churl, digest, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err := m.findChartURL(dep.Name, dep.Version, dep.Repository, repos) if err != nil { saveError = fmt.Errorf("could not find %s: %w", churl, err) break } - if _, ok := churls[churl]; ok { + if _, ok := churls[download{churl, digest}]; ok { fmt.Fprintf(m.Out, "Already downloaded %s from repo %s\n", dep.Name, dep.Repository) continue } @@ -340,6 +344,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { ContentCache: m.ContentCache, RegistryClient: m.RegistryClient, Getters: m.Getters, + IndexDigest: digest, Options: []getter.Option{ getter.WithBasicAuth(username, password), getter.WithPassCredentialsAll(passCredentialsAll), @@ -364,7 +369,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { break } - churls[churl] = struct{}{} + churls[download{churl, digest}] = struct{}{} } // TODO: this should probably be refactored to be a []error, so we can capture and provide more information rather than "last error wins". @@ -722,9 +727,9 @@ func (m *Manager) parallelRepoUpdate(repos []*repo.Entry) error { // repoURL is the repository to search // // If it finds a URL that is "relative", it will prepend the repoURL. -func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*repo.ChartRepository) (url, username, password string, insecureSkipTLSVerify, passCredentialsAll bool, caFile, certFile, keyFile string, err error) { +func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*repo.ChartRepository) (url, digest, username, password string, insecureSkipTLSVerify, passCredentialsAll bool, caFile, certFile, keyFile string, err error) { if registry.IsOCI(repoURL) { - return fmt.Sprintf("%s/%s:%s", repoURL, name, version), "", "", false, false, "", "", "", nil + return fmt.Sprintf("%s/%s:%s", repoURL, name, version), "", "", "", false, false, "", "", "", nil } for _, cr := range repos { @@ -735,17 +740,18 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]* entry, err = findEntryByName(name, cr) if err != nil { // TODO: Consider refactoring this function to reduce the number of returned values while preserving behavior. - return url, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err + return url, digest, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err } var ve *repo.ChartVersion ve, err = findVersionedEntry(version, entry) if err != nil { - return url, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err + return url, digest, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err } url, err = repo.ResolveReferenceURL(repoURL, ve.URLs[0]) if err != nil { - return url, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err + return url, digest, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err } + digest = ve.Digest username = cr.Config.Username password = cr.Config.Password passCredentialsAll = cr.Config.PassCredentialsAll @@ -754,14 +760,14 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]* certFile = cr.Config.CertFile keyFile = cr.Config.KeyFile - return url, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err + return url, digest, username, password, insecureSkipTLSVerify, passCredentialsAll, caFile, certFile, keyFile, err } - url, err = repo.FindChartInRepoURL(repoURL, name, m.Getters, repo.WithChartVersion(version), repo.WithClientTLS(certFile, keyFile, caFile)) + url, digest, err = repo.FindChartInRepoURLWithDigest(repoURL, name, m.Getters, repo.WithChartVersion(version), repo.WithClientTLS(certFile, keyFile, caFile)) if err == nil { - return url, username, password, false, false, "", "", "", nil + return url, digest, username, password, false, false, "", "", "", nil } err = fmt.Errorf("chart %s not found in %s: %w", name, repoURL, err) - return url, username, password, false, false, "", "", "", err + return url, digest, username, password, false, false, "", "", "", err } // findEntryByName finds an entry in the chart repository whose name matches the given name. diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index e40bbbac1..524b9c46c 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -18,6 +18,8 @@ package downloader import ( "bytes" "io/fs" + "net/http" + "net/http/httptest" "os" "path/filepath" "testing" @@ -27,9 +29,11 @@ import ( "github.com/stretchr/testify/require" "sigs.k8s.io/yaml" + "helm.sh/helm/v4/internal/test/ensure" chart "helm.sh/helm/v4/pkg/chart/v2" "helm.sh/helm/v4/pkg/chart/v2/loader" chartutil "helm.sh/helm/v4/pkg/chart/v2/util" + "helm.sh/helm/v4/pkg/cli" "helm.sh/helm/v4/pkg/getter" "helm.sh/helm/v4/pkg/repo/v1" "helm.sh/helm/v4/pkg/repo/v1/repotest" @@ -66,7 +70,7 @@ func TestFindChartURL(t *testing.T) { version := "0.1.0" repoURL := "http://example.com/charts" - churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err := m.findChartURL(name, version, repoURL, repos) + churl, _, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err := m.findChartURL(name, version, repoURL, repos) require.NoError(t, err) assert.Equal(t, "https://charts.helm.sh/stable/alpine-0.1.0.tgz", churl, "Unexpected URL %q", churl) @@ -79,7 +83,7 @@ func TestFindChartURL(t *testing.T) { version = "1.2.3" repoURL = "https://example-https-insecureskiptlsverify.com" - churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, repos) + churl, _, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, repos) require.NoError(t, err) assert.True(t, insecureSkipTLSVerify, "Unexpected insecureSkipTLSVerify %t", insecureSkipTLSVerify) @@ -92,7 +96,7 @@ func TestFindChartURL(t *testing.T) { version = "1.2.3" repoURL = "http://example.com/helm" - churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, repos) + churl, _, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, repos) require.NoError(t, err) assert.Equal(t, "http://example.com/helm/charts/foo-1.2.3.tgz", churl, "Unexpected URL %q", churl) @@ -655,3 +659,83 @@ func TestWriteLock(t *testing.T) { assert.Error(t, writeLock(filePath, lock, false)) }) } + +func TestDownloadAll_RejectsDependencyNotMatchingIndexDigest(t *testing.T) { + tests := []struct { + name string + configured bool + }{ + {name: "repository in repositories.yaml", configured: true}, + {name: "repository not in repositories.yaml", configured: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ensure.HelmHome(t) + contentCache := t.TempDir() + srv, _, _ := tamperedChartServer(t, contentCache) + + repoConfig := filepath.Join(srv.Root(), "repositories.yaml") + if !tt.configured { + repoConfig = filepath.Join(t.TempDir(), "repositories.yaml") + } + chartPath := t.TempDir() + m := &Manager{ + Out: new(bytes.Buffer), + ChartPath: chartPath, + RepositoryConfig: repoConfig, + RepositoryCache: srv.Root(), + ContentCache: contentCache, + Getters: getter.All(&cli.EnvSettings{}), + } + dep := &chart.Dependency{ + Name: "signtest", + Repository: srv.URL(), + Version: "0.1.0", + } + + err := m.downloadAll([]*chart.Dependency{dep}) + require.ErrorContains(t, err, "does not match the digest recorded for it in the repository index") + + _, err = os.Stat(filepath.Join(chartPath, "charts", "signtest-0.1.0.tgz")) + assert.ErrorIs(t, err, fs.ErrNotExist, "rejected dependency must not be saved to charts/") + }) + } +} + +func TestDownloadAll_ChecksEachIndexDigestForSharedURL(t *testing.T) { + ensure.HelmHome(t) + contentCache := t.TempDir() + srv, _, _ := tamperedChartServer(t, contentCache) + + // A second repository lists the same archive URL without a digest. When + // its entry is downloaded first, the one that does carry a digest must + // still be checked rather than skipped as already downloaded. + idx, err := repo.LoadIndexFile(filepath.Join(srv.Root(), "index.yaml")) + require.NoError(t, err) + for _, cv := range idx.Entries["signtest"] { + cv.Digest = "" + } + noDigest, err := yaml.Marshal(idx) + require.NoError(t, err) + other := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(noDigest) + })) + t.Cleanup(other.Close) + + chartPath := t.TempDir() + m := &Manager{ + Out: new(bytes.Buffer), + ChartPath: chartPath, + RepositoryConfig: filepath.Join(t.TempDir(), "repositories.yaml"), + RepositoryCache: srv.Root(), + ContentCache: contentCache, + Getters: getter.All(&cli.EnvSettings{}), + } + deps := []*chart.Dependency{ + {Name: "signtest", Repository: other.URL, Version: "0.1.0"}, + {Name: "signtest", Repository: srv.URL(), Version: "0.1.0", Alias: "signtest-indexed"}, + } + + err = m.downloadAll(deps) + require.ErrorContains(t, err, "does not match the digest recorded for it in the repository index") +} diff --git a/pkg/repo/v1/chartrepo.go b/pkg/repo/v1/chartrepo.go index da42128bf..025736cc2 100644 --- a/pkg/repo/v1/chartrepo.go +++ b/pkg/repo/v1/chartrepo.go @@ -174,6 +174,15 @@ func WithInsecureSkipTLSVerify(insecureSkipTLSVerify bool) FindChartInRepoURLOpt // FindChartInRepoURL finds chart in chart repository pointed by repoURL // without adding repo to repositories func FindChartInRepoURL(repoURL, chartName string, getters getter.Providers, options ...FindChartInRepoURLOption) (string, error) { + chartURL, _, err := FindChartInRepoURLWithDigest(repoURL, chartName, getters, options...) + return chartURL, err +} + +// FindChartInRepoURLWithDigest is FindChartInRepoURL that also returns the +// digest the repository index records for the chart archive, so the caller can +// check the archive it downloads from the returned URL against it. The digest +// is empty when the index entry does not carry one. +func FindChartInRepoURLWithDigest(repoURL, chartName string, getters getter.Providers, options ...FindChartInRepoURLOption) (string, string, error) { opts := findChartInRepoURLOptions{} for _, option := range options { option(&opts) @@ -197,11 +206,11 @@ func FindChartInRepoURL(repoURL, chartName string, getters getter.Providers, opt } r, err := NewChartRepository(&c, getters) if err != nil { - return "", err + return "", "", err } idx, err := r.DownloadIndexFile() if err != nil { - return "", fmt.Errorf("looks like %q is not a valid chart repository or cannot be reached: %w", repoURL, err) + return "", "", fmt.Errorf("looks like %q is not a valid chart repository or cannot be reached: %w", repoURL, err) } defer func() { os.RemoveAll(filepath.Join(r.CachePath, helmpath.CacheChartsFile(r.Config.Name))) @@ -211,7 +220,7 @@ func FindChartInRepoURL(repoURL, chartName string, getters getter.Providers, opt // Read the index file for the repository to get chart information and return chart URL repoIndex, err := LoadIndexFile(idx) if err != nil { - return "", err + return "", "", err } errMsg := fmt.Sprintf("chart %q", chartName) @@ -220,24 +229,24 @@ func FindChartInRepoURL(repoURL, chartName string, getters getter.Providers, opt } cv, err := repoIndex.Get(chartName, opts.ChartVersion) if err != nil { - return "", ChartNotFoundError{ + return "", "", ChartNotFoundError{ Chart: errMsg, RepoURL: repoURL, } } if len(cv.URLs) == 0 { - return "", fmt.Errorf("%s has no downloadable URLs", errMsg) + return "", "", fmt.Errorf("%s has no downloadable URLs", errMsg) } chartURL := cv.URLs[0] absoluteChartURL, err := ResolveReferenceURL(repoURL, chartURL) if err != nil { - return "", fmt.Errorf("failed to make chart URL absolute: %w", err) + return "", "", fmt.Errorf("failed to make chart URL absolute: %w", err) } - return absoluteChartURL, nil + return absoluteChartURL, cv.Digest, nil } // ResolveReferenceURL resolves refURL relative to baseURL. diff --git a/pkg/repo/v1/chartrepo_test.go b/pkg/repo/v1/chartrepo_test.go index f0e5839ac..ab0ebb03d 100644 --- a/pkg/repo/v1/chartrepo_test.go +++ b/pkg/repo/v1/chartrepo_test.go @@ -196,6 +196,17 @@ func TestFindChartInRepoURL(t *testing.T) { assert.Equalf(t, "https://charts.helm.sh/stable/nginx-0.1.0.tgz", chartURL, "%s is not the valid URL", chartURL) } +func TestFindChartInRepoURLWithDigest(t *testing.T) { + srv, err := startLocalServerForTests(nil) + require.NoError(t, err) + defer srv.Close() + + chartURL, digest, err := FindChartInRepoURLWithDigest(srv.URL, "nginx", getter.All(&cli.EnvSettings{}), WithChartVersion("0.2.0")) + require.NoError(t, err) + assert.Equal(t, "https://charts.helm.sh/stable/nginx-0.2.0.tgz", chartURL) + assert.Equal(t, "sha256:1234567890abcdef", digest) +} + func TestErrorFindChartInRepoURL(t *testing.T) { g := getter.All(&cli.EnvSettings{ RepositoryCache: t.TempDir(),