pull/32686/merge
Ashvin 4 days ago committed by GitHub
commit db1726c4ff
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -937,7 +937,7 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) (
dl.Verify = downloader.VerifyAlways dl.Verify = downloader.VerifyAlways
} }
if c.RepoURL != "" { if c.RepoURL != "" {
chartURL, err := repo.FindChartInRepoURL( chartURL, digest, err := repo.FindChartInRepoURLWithDigest(
c.RepoURL, c.RepoURL,
name, name,
getter.All(settings), getter.All(settings),
@ -951,6 +951,7 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) (
return "", err return "", err
} }
name = chartURL name = chartURL
dl.IndexDigest = digest
// Only pass the user/pass on when the user has said to or when the // 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. // location of the chart repo and the chart are the same domain.

@ -47,8 +47,10 @@ import (
ci "helm.sh/helm/v4/pkg/chart" ci "helm.sh/helm/v4/pkg/chart"
"helm.sh/helm/v4/internal/test" "helm.sh/helm/v4/internal/test"
"helm.sh/helm/v4/internal/test/ensure"
"helm.sh/helm/v4/pkg/chart/common" "helm.sh/helm/v4/pkg/chart/common"
chart "helm.sh/helm/v4/pkg/chart/v2" chart "helm.sh/helm/v4/pkg/chart/v2"
"helm.sh/helm/v4/pkg/cli"
"helm.sh/helm/v4/pkg/kube" "helm.sh/helm/v4/pkg/kube"
kubefake "helm.sh/helm/v4/pkg/kube/fake" kubefake "helm.sh/helm/v4/pkg/kube/fake"
"helm.sh/helm/v4/pkg/registry" "helm.sh/helm/v4/pkg/registry"
@ -1253,3 +1255,19 @@ func TestInstallRelease_WaitOptionsPassedDownstream(t *testing.T) {
// Verify that WaitOptions were passed to GetWaiter // Verify that WaitOptions were passed to GetWaiter
is.NotEmpty(failer.RecordedWaitOptions, "WaitOptions should be 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")
}
}

@ -117,7 +117,7 @@ func (p *Pull) Run(chartRef string) (string, error) {
downloadSourceRef := chartRef downloadSourceRef := chartRef
if p.RepoURL != "" { if p.RepoURL != "" {
chartURL, err := repo.FindChartInRepoURL( chartURL, digest, err := repo.FindChartInRepoURLWithDigest(
p.RepoURL, p.RepoURL,
chartRef, chartRef,
getter.All(p.Settings), getter.All(p.Settings),
@ -131,6 +131,7 @@ func (p *Pull) Run(chartRef string) (string, error) {
return out.String(), err return out.String(), err
} }
downloadSourceRef = chartURL downloadSourceRef = chartURL
c.IndexDigest = digest
} }
saved, v, err := c.DownloadTo(downloadSourceRef, p.Version, dest) saved, v, err := c.DownloadTo(downloadSourceRef, p.Version, dest)

@ -17,16 +17,21 @@ limitations under the License.
package action package action
import ( import (
"bytes"
"net/http" "net/http"
"net/http/httptest" "net/http/httptest"
"os" "os"
"path/filepath"
"strings"
"testing" "testing"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
"helm.sh/helm/v4/internal/test/ensure"
"helm.sh/helm/v4/pkg/cli" "helm.sh/helm/v4/pkg/cli"
"helm.sh/helm/v4/pkg/registry" "helm.sh/helm/v4/pkg/registry"
"helm.sh/helm/v4/pkg/repo/v1/repotest"
) )
func TestNewPull(t *testing.T) { func TestNewPull(t *testing.T) {
@ -47,7 +52,16 @@ func TestPullSetRegistryClient(t *testing.T) {
} }
func TestPullRun_ChartNotFound(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) require.NoError(t, err)
defer srv.Close() defer srv.Close()
@ -76,3 +90,36 @@ func startLocalServerForTests(t *testing.T, handler http.Handler) (*httptest.Ser
return httptest.NewServer(handler), nil 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")
}

@ -85,6 +85,13 @@ type ChartDownloader struct {
// Cache specifies the cache implementation to use. // Cache specifies the cache implementation to use.
Cache Cache 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. // 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 { if pth, err := c.Cache.Get(digest32, CacheChart); err == nil {
fdata, err := os.ReadFile(pth) fdata, err := os.ReadFile(pth)
if err == nil { if err == nil {
found = true if verr := verifyIndexDigest(ref, u, hash, digest32, fdata); verr != nil {
data = bytes.NewBuffer(fdata) // An entry that does not hash to the digest it is filed
slog.Debug("found chart in cache", "id", hash) // 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 { if err != nil {
return "", nil, err return "", nil, err
} }
if err := verifyIndexDigest(ref, u, hash, digest32, data.Bytes()); err != nil {
return "", nil, err
}
} }
name := filepath.Base(u.Path) name := filepath.Base(u.Path)
@ -248,18 +266,29 @@ func (c *ChartDownloader) DownloadToCache(ref, version string) (string, *provena
copy(digest32[:], digest) copy(digest32[:], digest)
var pth string var pth string
var cached bool
// only fetch from the cache if we have a digest // only fetch from the cache if we have a digest
if len(digest) > 0 { if len(digest) > 0 {
pth, err = c.Cache.Get(digest32, CacheChart) cachePath, cerr := c.Cache.Get(digest32, CacheChart)
if err == nil { switch {
slog.Debug("found chart in cache", "id", digestString) 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) 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 // Get file not in the cache
data, gerr := g.Get(u.String(), c.Options...) data, gerr := g.Get(u.String(), c.Options...)
@ -267,6 +296,12 @@ func (c *ChartDownloader) DownloadToCache(ref, version string) (string, *provena
return "", nil, gerr 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 // Generate the digest
if len(digest) == 0 { if len(digest) == 0 {
digest32 = sha256.Sum256(data.Bytes()) digest32 = sha256.Sum256(data.Bytes())
@ -396,7 +431,7 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (string, *url
if errors.Is(err, ErrNoOwnerRepo) { if errors.Is(err, ErrNoOwnerRepo) {
// Make sure to add the ref URL as the URL for the getter // Make sure to add the ref URL as the URL for the getter
c.Options = append(c.Options, getter.WithURL(ref)) c.Options = append(c.Options, getter.WithURL(ref))
return "", u, nil return c.IndexDigest, u, nil
} }
return "", u, err return "", u, err
} }
@ -417,7 +452,7 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (string, *url
getter.WithPassCredentialsAll(rc.PassCredentialsAll), getter.WithPassCredentialsAll(rc.PassCredentialsAll),
) )
} }
return "", u, nil return c.IndexDigest, u, nil
} }
// See if it's of the form: repo/path_to_chart // 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 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. // stripDigestAlgorithm removes the algorithm prefix (e.g., "sha256:") from a digest string.
// If no prefix is present, the original string is returned unchanged. // If no prefix is present, the original string is returned unchanged.
func stripDigestAlgorithm(digest string) string { func stripDigestAlgorithm(digest string) string {

@ -16,10 +16,12 @@ limitations under the License.
package downloader package downloader
import ( import (
"bytes"
"crypto/sha256" "crypto/sha256"
"encoding/hex" "encoding/hex"
"net/http" "net/http"
"net/http/httptest" "net/http/httptest"
"net/url"
"os" "os"
"path/filepath" "path/filepath"
"testing" "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")
})
}

@ -275,7 +275,11 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
fmt.Fprintf(m.Out, "Saving %d charts\n", len(deps)) fmt.Fprintf(m.Out, "Saving %d charts\n", len(deps))
var saveError error 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 { for _, dep := range deps {
// No repository means the chart is in charts directory // No repository means the chart is in charts directory
if dep.Repository == "" { if dep.Repository == "" {
@ -318,13 +322,13 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
// Any failure to resolve/download a chart should fail: // Any failure to resolve/download a chart should fail:
// https://github.com/helm/helm/issues/1439 // 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 { if err != nil {
saveError = fmt.Errorf("could not find %s: %w", churl, err) saveError = fmt.Errorf("could not find %s: %w", churl, err)
break 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) fmt.Fprintf(m.Out, "Already downloaded %s from repo %s\n", dep.Name, dep.Repository)
continue continue
} }
@ -340,6 +344,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
ContentCache: m.ContentCache, ContentCache: m.ContentCache,
RegistryClient: m.RegistryClient, RegistryClient: m.RegistryClient,
Getters: m.Getters, Getters: m.Getters,
IndexDigest: digest,
Options: []getter.Option{ Options: []getter.Option{
getter.WithBasicAuth(username, password), getter.WithBasicAuth(username, password),
getter.WithPassCredentialsAll(passCredentialsAll), getter.WithPassCredentialsAll(passCredentialsAll),
@ -364,7 +369,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
break 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". // 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 // repoURL is the repository to search
// //
// If it finds a URL that is "relative", it will prepend the repoURL. // 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) { 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 { for _, cr := range repos {
@ -735,17 +740,18 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*
entry, err = findEntryByName(name, cr) entry, err = findEntryByName(name, cr)
if err != nil { if err != nil {
// TODO: Consider refactoring this function to reduce the number of returned values while preserving behavior. // 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 var ve *repo.ChartVersion
ve, err = findVersionedEntry(version, entry) ve, err = findVersionedEntry(version, entry)
if err != nil { 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]) url, err = repo.ResolveReferenceURL(repoURL, ve.URLs[0])
if err != nil { 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 username = cr.Config.Username
password = cr.Config.Password password = cr.Config.Password
passCredentialsAll = cr.Config.PassCredentialsAll passCredentialsAll = cr.Config.PassCredentialsAll
@ -754,14 +760,14 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*
certFile = cr.Config.CertFile certFile = cr.Config.CertFile
keyFile = cr.Config.KeyFile 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 { 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) 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. // findEntryByName finds an entry in the chart repository whose name matches the given name.

@ -18,6 +18,8 @@ package downloader
import ( import (
"bytes" "bytes"
"io/fs" "io/fs"
"net/http"
"net/http/httptest"
"os" "os"
"path/filepath" "path/filepath"
"testing" "testing"
@ -27,9 +29,11 @@ import (
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
"sigs.k8s.io/yaml" "sigs.k8s.io/yaml"
"helm.sh/helm/v4/internal/test/ensure"
chart "helm.sh/helm/v4/pkg/chart/v2" chart "helm.sh/helm/v4/pkg/chart/v2"
"helm.sh/helm/v4/pkg/chart/v2/loader" "helm.sh/helm/v4/pkg/chart/v2/loader"
chartutil "helm.sh/helm/v4/pkg/chart/v2/util" 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/getter"
"helm.sh/helm/v4/pkg/repo/v1" "helm.sh/helm/v4/pkg/repo/v1"
"helm.sh/helm/v4/pkg/repo/v1/repotest" "helm.sh/helm/v4/pkg/repo/v1/repotest"
@ -66,7 +70,7 @@ func TestFindChartURL(t *testing.T) {
version := "0.1.0" version := "0.1.0"
repoURL := "http://example.com/charts" 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) require.NoError(t, err)
assert.Equal(t, "https://charts.helm.sh/stable/alpine-0.1.0.tgz", churl, "Unexpected URL %q", churl) 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" version = "1.2.3"
repoURL = "https://example-https-insecureskiptlsverify.com" 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) require.NoError(t, err)
assert.True(t, insecureSkipTLSVerify, "Unexpected insecureSkipTLSVerify %t", insecureSkipTLSVerify) assert.True(t, insecureSkipTLSVerify, "Unexpected insecureSkipTLSVerify %t", insecureSkipTLSVerify)
@ -92,7 +96,7 @@ func TestFindChartURL(t *testing.T) {
version = "1.2.3" version = "1.2.3"
repoURL = "http://example.com/helm" 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) require.NoError(t, err)
assert.Equal(t, "http://example.com/helm/charts/foo-1.2.3.tgz", churl, "Unexpected URL %q", churl) 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)) 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")
}

@ -174,6 +174,15 @@ func WithInsecureSkipTLSVerify(insecureSkipTLSVerify bool) FindChartInRepoURLOpt
// FindChartInRepoURL finds chart in chart repository pointed by repoURL // FindChartInRepoURL finds chart in chart repository pointed by repoURL
// without adding repo to repositories // without adding repo to repositories
func FindChartInRepoURL(repoURL, chartName string, getters getter.Providers, options ...FindChartInRepoURLOption) (string, error) { 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{} opts := findChartInRepoURLOptions{}
for _, option := range options { for _, option := range options {
option(&opts) option(&opts)
@ -197,11 +206,11 @@ func FindChartInRepoURL(repoURL, chartName string, getters getter.Providers, opt
} }
r, err := NewChartRepository(&c, getters) r, err := NewChartRepository(&c, getters)
if err != nil { if err != nil {
return "", err return "", "", err
} }
idx, err := r.DownloadIndexFile() idx, err := r.DownloadIndexFile()
if err != nil { 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() { defer func() {
os.RemoveAll(filepath.Join(r.CachePath, helmpath.CacheChartsFile(r.Config.Name))) 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 // Read the index file for the repository to get chart information and return chart URL
repoIndex, err := LoadIndexFile(idx) repoIndex, err := LoadIndexFile(idx)
if err != nil { if err != nil {
return "", err return "", "", err
} }
errMsg := fmt.Sprintf("chart %q", chartName) 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) cv, err := repoIndex.Get(chartName, opts.ChartVersion)
if err != nil { if err != nil {
return "", ChartNotFoundError{ return "", "", ChartNotFoundError{
Chart: errMsg, Chart: errMsg,
RepoURL: repoURL, RepoURL: repoURL,
} }
} }
if len(cv.URLs) == 0 { 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] chartURL := cv.URLs[0]
absoluteChartURL, err := ResolveReferenceURL(repoURL, chartURL) absoluteChartURL, err := ResolveReferenceURL(repoURL, chartURL)
if err != nil { 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. // ResolveReferenceURL resolves refURL relative to baseURL.

@ -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) 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) { func TestErrorFindChartInRepoURL(t *testing.T) {
g := getter.All(&cli.EnvSettings{ g := getter.All(&cli.EnvSettings{
RepositoryCache: t.TempDir(), RepositoryCache: t.TempDir(),

Loading…
Cancel
Save