From ee018608f6fbf381fac1bae9759164a65c6a0b1f Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Fri, 20 Feb 2026 16:58:32 +0000 Subject: [PATCH] fix: handle OCI digest algorithm prefix in chart downloader (#31601) * fix: strip digest algorithm prefix before hex decoding OCI references with tag+digest (e.g., chart:1.0@sha256:abc...) failed with "invalid byte" error because the sha256: prefix was passed to hex.DecodeString(). Signed-off-by: Evans Mungai * Add cmd test for OCI references with tag+digest Signed-off-by: Evans Mungai * Move oci registry push result to a struct Signed-off-by: Evans Mungai * Review comments from PR review Signed-off-by: Evans Mungai * Fix failing test Signed-off-by: Evans Mungai --------- Signed-off-by: Evans Mungai --- pkg/chart/common/util/jsonschema.go | 8 ++-- pkg/cmd/pull_test.go | 52 +++++++++++++++++++++++++ pkg/downloader/chart_downloader.go | 22 ++++++++++- pkg/downloader/chart_downloader_test.go | 32 +++++++++++++++ pkg/repo/v1/repotest/server.go | 17 +++++++- 5 files changed, 123 insertions(+), 8 deletions(-) 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 522233a7d..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) @@ -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/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.