diff --git a/pkg/chart/common/util/jsonschema.go b/pkg/chart/common/util/jsonschema.go index 6d7f32604..873c08fdd 100644 --- a/pkg/chart/common/util/jsonschema.go +++ b/pkg/chart/common/util/jsonschema.go @@ -83,7 +83,7 @@ func ValidateAgainstSchema(ch chart.Charter, values map[string]interface{}) erro slog.Debug("chart name", "chart-name", chrt.Name()) err := ValidateAgainstSingleSchema(values, chrt.Schema()) if err != nil { - sb.WriteString(fmt.Sprintf("%s:\n", chrt.Name())) + fmt.Fprintf(&sb, "%s:\n", chrt.Name()) sb.WriteString(err.Error()) } } @@ -103,10 +103,8 @@ func ValidateAgainstSchema(ch chart.Charter, values map[string]interface{}) erro subchartValues, ok := raw.(map[string]any) if !ok { - sb.WriteString(fmt.Sprintf( - "%s:\ninvalid type for values: expected object (map), got %T\n", - sub.Name(), raw, - )) + fmt.Fprintf(&sb, "%s:\ninvalid type for values: expected object (map), got %T\n", + sub.Name(), raw) continue } diff --git a/pkg/cmd/pull_test.go b/pkg/cmd/pull_test.go index 96631fe05..6d5eb1482 100644 --- a/pkg/cmd/pull_test.go +++ b/pkg/cmd/pull_test.go @@ -22,6 +22,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strings" "testing" "helm.sh/helm/v4/pkg/repo/v1/repotest" @@ -506,3 +507,54 @@ func TestPullFileCompletion(t *testing.T) { checkFileCompletion(t, "pull", false) checkFileCompletion(t, "pull repo/chart", false) } + +// TestPullOCIWithTagAndDigest tests pulling an OCI chart with both tag and digest specified. +// This is a regression test for https://github.com/helm/helm/issues/31600 +func TestPullOCIWithTagAndDigest(t *testing.T) { + srv := repotest.NewTempServer( + t, + repotest.WithChartSourceGlob("testdata/testcharts/*.tgz*"), + ) + defer srv.Stop() + + ociSrv, err := repotest.NewOCIServer(t, srv.Root()) + if err != nil { + t.Fatal(err) + } + result := ociSrv.RunWithReturn(t) + + contentCache := t.TempDir() + outdir := t.TempDir() + + // Test: pull with tag and digest (the fixed bug from issue #31600) + // Previously this failed with "encoding/hex: invalid byte: U+0073 's'" + ref := fmt.Sprintf("oci://%s/u/ocitestuser/oci-dependent-chart:0.1.0@%s", + ociSrv.RegistryURL, result.PushedChart.Manifest.Digest) + + cmd := fmt.Sprintf("pull %s -d '%s' --registry-config %s --content-cache %s --plain-http", + ref, + outdir, + filepath.Join(srv.Root(), "config.json"), + contentCache, + ) + + _, _, err = executeActionCommand(cmd) + if err != nil { + t.Fatalf("pull with tag+digest failed: %v", err) + } + + // Verify the file was downloaded + // When digest is present, the filename uses the digest format (e.g. chart@sha256-hex.tgz) + expectedFile := filepath.Join(outdir, "oci-dependent-chart-0.1.0.tgz") + if _, err := os.Stat(expectedFile); err != nil { + // Try the digest-based filename; parse algorithm:hex to avoid fixed-offset assumptions + algorithm, digestPart, ok := strings.Cut(result.PushedChart.Manifest.Digest, ":") + if !ok { + t.Fatalf("digest must be in algorithm:hex format, got %q", result.PushedChart.Manifest.Digest) + } + expectedFile = filepath.Join(outdir, fmt.Sprintf("oci-dependent-chart@%s-%s.tgz", algorithm, digestPart)) + if _, err := os.Stat(expectedFile); err != nil { + t.Errorf("expected chart file not found: %v", err) + } + } +} diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 13eb2bd62..d21a79335 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -125,10 +125,15 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven var digest32 [32]byte if hash != "" { // if there is a hash, populate the other formats - digest, err = hex.DecodeString(hash) + // Strip the algorithm prefix (e.g., "sha256:") if present + digest, err = hex.DecodeString(stripDigestAlgorithm(hash)) if err != nil { return "", nil, err } + if len(digest) != 32 { + return "", nil, fmt.Errorf("invalid digest length: %d", len(digest)) + } + copy(digest32[:], digest) if pth, err := c.Cache.Get(digest32, CacheChart); err == nil { fdata, err := os.ReadFile(pth) @@ -180,7 +185,7 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven } } if !found { - body, err = g.Get(u.String() + ".prov") + body, err = g.Get(u.String()+".prov", c.Options...) if err != nil { if c.Verify == VerifyAlways { return destfile, ver, fmt.Errorf("failed to fetch provenance %q", u.String()+".prov") @@ -231,10 +236,14 @@ func (c *ChartDownloader) DownloadToCache(ref, version string) (string, *provena c.Options = append(c.Options, getter.WithAcceptHeader("application/gzip,application/octet-stream")) // Check the cache for the file - digest, err := hex.DecodeString(digestString) + // Strip the algorithm prefix (e.g., "sha256:") if present + digest, err := hex.DecodeString(stripDigestAlgorithm(digestString)) if err != nil { return "", nil, fmt.Errorf("unable to decode digest: %w", err) } + if digestString != "" && len(digest) != 32 { + return "", nil, fmt.Errorf("invalid digest length: %d", len(digest)) + } var digest32 [32]byte copy(digest32[:], digest) @@ -584,3 +593,12 @@ func loadRepoConfig(file string) (*repo.File, error) { } return r, 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 { + if idx := strings.Index(digest, ":"); idx >= 0 { + return digest[idx+1:] + } + return digest +} diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index 777e21a6e..8a7514a8e 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -23,6 +23,7 @@ import ( "path/filepath" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "helm.sh/helm/v4/internal/test/ensure" @@ -486,3 +487,34 @@ func TestDownloadToCache(t *testing.T) { c.Keyring = "" }) } + +func TestStripDigestAlgorithm(t *testing.T) { + tests := map[string]struct { + input string + expected string + }{ + "sha256 prefixed digest": { + input: "sha256:aef46c66a7f2d5a12a7e3f54a64790daf5c9a9e66af3f46955efdaa6c900341d", + expected: "aef46c66a7f2d5a12a7e3f54a64790daf5c9a9e66af3f46955efdaa6c900341d", + }, + "sha512 prefixed digest": { + input: "sha512:abcdef1234567890", + expected: "abcdef1234567890", + }, + "plain hex digest without prefix": { + input: "aef46c66a7f2d5a12a7e3f54a64790daf5c9a9e66af3f46955efdaa6c900341d", + expected: "aef46c66a7f2d5a12a7e3f54a64790daf5c9a9e66af3f46955efdaa6c900341d", + }, + "empty string": { + input: "", + expected: "", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + result := stripDigestAlgorithm(tt.input) + assert.Equalf(t, tt.expected, result, "stripDigestAlgorithm(%q) = %q, want %q", tt.input, result, tt.expected) + }) + } +} diff --git a/pkg/getter/getter_test.go b/pkg/getter/getter_test.go index 83920e809..3a09b4d82 100644 --- a/pkg/getter/getter_test.go +++ b/pkg/getter/getter_test.go @@ -60,7 +60,8 @@ func TestProvidersWithTimeout(t *testing.T) { if err != nil { t.Error(err) } - client, err := getter.(*HTTPGetter).httpClient() + httpGetter := getter.(*HTTPGetter) + client, err := httpGetter.httpClient(httpGetter.opts) if err != nil { t.Error(err) } diff --git a/pkg/getter/httpgetter.go b/pkg/getter/httpgetter.go index 110f45c54..2bc12bdbf 100644 --- a/pkg/getter/httpgetter.go +++ b/pkg/getter/httpgetter.go @@ -37,13 +37,15 @@ type HTTPGetter struct { // Get performs a Get from repo.Getter and returns the body. func (g *HTTPGetter) Get(href string, options ...Option) (*bytes.Buffer, error) { + // Create a local copy of options to avoid data races when Get is called concurrently + opts := g.opts for _, opt := range options { - opt(&g.opts) + opt(&opts) } - return g.get(href) + return g.get(href, opts) } -func (g *HTTPGetter) get(href string) (*bytes.Buffer, error) { +func (g *HTTPGetter) get(href string, opts getterOptions) (*bytes.Buffer, error) { // Set a helm specific user agent so that a repo server and metrics can // separate helm calls from other tools interacting with repos. req, err := http.NewRequest(http.MethodGet, href, nil) @@ -51,18 +53,18 @@ func (g *HTTPGetter) get(href string) (*bytes.Buffer, error) { return nil, err } - if g.opts.acceptHeader != "" { - req.Header.Set("Accept", g.opts.acceptHeader) + if opts.acceptHeader != "" { + req.Header.Set("Accept", opts.acceptHeader) } req.Header.Set("User-Agent", version.GetUserAgent()) - if g.opts.userAgent != "" { - req.Header.Set("User-Agent", g.opts.userAgent) + if opts.userAgent != "" { + req.Header.Set("User-Agent", opts.userAgent) } // Before setting the basic auth credentials, make sure the URL associated // with the basic auth is the one being fetched. - u1, err := url.Parse(g.opts.url) + u1, err := url.Parse(opts.url) if err != nil { return nil, fmt.Errorf("unable to parse getter URL: %w", err) } @@ -74,13 +76,13 @@ func (g *HTTPGetter) get(href string) (*bytes.Buffer, error) { // Host on URL (returned from url.Parse) contains the port if present. // This check ensures credentials are not passed between different // services on different ports. - if g.opts.passCredentialsAll || (u1.Scheme == u2.Scheme && u1.Host == u2.Host) { - if g.opts.username != "" && g.opts.password != "" { - req.SetBasicAuth(g.opts.username, g.opts.password) + if opts.passCredentialsAll || (u1.Scheme == u2.Scheme && u1.Host == u2.Host) { + if opts.username != "" && opts.password != "" { + req.SetBasicAuth(opts.username, opts.password) } } - client, err := g.httpClient() + client, err := g.httpClient(opts) if err != nil { return nil, err } @@ -110,51 +112,52 @@ func NewHTTPGetter(options ...Option) (Getter, error) { return &client, nil } -func (g *HTTPGetter) httpClient() (*http.Client, error) { - if g.opts.transport != nil { +func (g *HTTPGetter) httpClient(opts getterOptions) (*http.Client, error) { + if opts.transport != nil { return &http.Client{ - Transport: g.opts.transport, - Timeout: g.opts.timeout, + Transport: opts.transport, + Timeout: opts.timeout, }, nil } - g.once.Do(func() { - g.transport = &http.Transport{ + // Check if we need custom TLS configuration + needsCustomTLS := (opts.certFile != "" && opts.keyFile != "") || opts.caFile != "" || opts.insecureSkipVerifyTLS + + if needsCustomTLS { + // Create a new transport for custom TLS to avoid race conditions + transport := &http.Transport{ DisableCompression: true, Proxy: http.ProxyFromEnvironment, - // Being nil would cause the tls.Config default to be used - // "NewTLSConfig" modifies an empty TLS config, not the default one - TLSClientConfig: &tls.Config{}, } - }) - if (g.opts.certFile != "" && g.opts.keyFile != "") || g.opts.caFile != "" || g.opts.insecureSkipVerifyTLS { tlsConf, err := tlsutil.NewTLSConfig( - tlsutil.WithInsecureSkipVerify(g.opts.insecureSkipVerifyTLS), - tlsutil.WithCertKeyPairFiles(g.opts.certFile, g.opts.keyFile), - tlsutil.WithCAFile(g.opts.caFile), + tlsutil.WithInsecureSkipVerify(opts.insecureSkipVerifyTLS), + tlsutil.WithCertKeyPairFiles(opts.certFile, opts.keyFile), + tlsutil.WithCAFile(opts.caFile), ) if err != nil { return nil, fmt.Errorf("can't create TLS config for client: %w", err) } - g.transport.TLSClientConfig = tlsConf + transport.TLSClientConfig = tlsConf + + return &http.Client{ + Transport: transport, + Timeout: opts.timeout, + }, nil } - if g.opts.insecureSkipVerifyTLS { - if g.transport.TLSClientConfig == nil { - g.transport.TLSClientConfig = &tls.Config{ - InsecureSkipVerify: true, - } - } else { - g.transport.TLSClientConfig.InsecureSkipVerify = true + // Use shared transport for default case (no custom TLS) + g.once.Do(func() { + g.transport = &http.Transport{ + DisableCompression: true, + Proxy: http.ProxyFromEnvironment, + TLSClientConfig: &tls.Config{}, } - } + }) - client := &http.Client{ + return &http.Client{ Transport: g.transport, - Timeout: g.opts.timeout, - } - - return client, nil + Timeout: opts.timeout, + }, nil } diff --git a/pkg/getter/httpgetter_test.go b/pkg/getter/httpgetter_test.go index b27b9f5d2..3047ed6f6 100644 --- a/pkg/getter/httpgetter_test.go +++ b/pkg/getter/httpgetter_test.go @@ -577,7 +577,7 @@ func TestHttpClientInsecureSkipVerify(t *testing.T) { func verifyInsecureSkipVerify(t *testing.T, g *HTTPGetter, caseName string, expectedValue bool) *http.Transport { t.Helper() - returnVal, err := g.httpClient() + returnVal, err := g.httpClient(g.opts) if err != nil { t.Fatal(err) @@ -601,7 +601,7 @@ func verifyInsecureSkipVerify(t *testing.T, g *HTTPGetter, caseName string, expe func TestDefaultHTTPTransportReuse(t *testing.T) { g := HTTPGetter{} - httpClient1, err := g.httpClient() + httpClient1, err := g.httpClient(g.opts) if err != nil { t.Fatal(err) @@ -613,7 +613,7 @@ func TestDefaultHTTPTransportReuse(t *testing.T) { transport1 := (httpClient1.Transport).(*http.Transport) //nolint:staticcheck - httpClient2, err := g.httpClient() + httpClient2, err := g.httpClient(g.opts) if err != nil { t.Fatal(err) @@ -635,7 +635,7 @@ func TestHTTPTransportOption(t *testing.T) { g := HTTPGetter{} g.opts.transport = transport - httpClient1, err := g.httpClient() + httpClient1, err := g.httpClient(g.opts) if err != nil { t.Fatal(err) @@ -651,7 +651,7 @@ func TestHTTPTransportOption(t *testing.T) { t.Fatalf("Expected transport option to be applied") } - httpClient2, err := g.httpClient() + httpClient2, err := g.httpClient(g.opts) if err != nil { t.Fatal(err) diff --git a/pkg/repo/v1/repotest/server.go b/pkg/repo/v1/repotest/server.go index 12b96de5a..22b4c8060 100644 --- a/pkg/repo/v1/repotest/server.go +++ b/pkg/repo/v1/repotest/server.go @@ -153,6 +153,10 @@ type OCIServerRunConfig struct { type OCIServerOpt func(config *OCIServerRunConfig) +type OCIServerRunResult struct { + PushedChart *ociRegistry.PushResult +} + func WithDependingChart(c *chart.Chart) OCIServerOpt { return func(config *OCIServerRunConfig) { config.DependingChart = c @@ -210,6 +214,11 @@ func NewOCIServer(t *testing.T, dir string) (*OCIServer, error) { } func (srv *OCIServer) Run(t *testing.T, opts ...OCIServerOpt) { + t.Helper() + _ = srv.RunWithReturn(t, opts...) +} + +func (srv *OCIServer) RunWithReturn(t *testing.T, opts ...OCIServerOpt) *OCIServerRunResult { t.Helper() cfg := &OCIServerRunConfig{} for _, fn := range opts { @@ -284,7 +293,9 @@ func (srv *OCIServer) Run(t *testing.T, opts ...OCIServerOpt) { srv.Client = registryClient c := cfg.DependingChart if c == nil { - return + return &OCIServerRunResult{ + PushedChart: result, + } } dependingRef := fmt.Sprintf("%s/u/ocitestuser/%s:%s", @@ -308,6 +319,10 @@ func (srv *OCIServer) Run(t *testing.T, opts ...OCIServerOpt) { result.Manifest.Digest, result.Manifest.Size, result.Config.Digest, result.Config.Size, result.Chart.Digest, result.Chart.Size) + + return &OCIServerRunResult{ + PushedChart: result, + } } // Root gets the docroot for the server.