From 429dfdd10a4bae74a8764ca88e24e3cb2063cc2b Mon Sep 17 00:00:00 2001 From: gabrnavarro Date: Mon, 9 Mar 2026 17:48:19 +0100 Subject: [PATCH] fix: address issue #31866 --- go.mod | 2 +- pkg/action/package.go | 6 +- pkg/provenance/sign.go | 33 +++++++++++ pkg/provenance/sign_test.go | 52 +++++++++++++++++- .../testdata/hashtest-keywords-1.2.3.tgz | Bin 0 -> 464 bytes .../testdata/hashtest-keywords.sha256 | 1 + .../testdata/hashtest-keywords/Chart.yaml | 12 ++++ 7 files changed, 100 insertions(+), 6 deletions(-) create mode 100644 pkg/provenance/testdata/hashtest-keywords-1.2.3.tgz create mode 100644 pkg/provenance/testdata/hashtest-keywords.sha256 create mode 100644 pkg/provenance/testdata/hashtest-keywords/Chart.yaml diff --git a/go.mod b/go.mod index 6ab73f33a..d5849f571 100644 --- a/go.mod +++ b/go.mod @@ -38,7 +38,7 @@ require ( golang.org/x/crypto v0.48.0 golang.org/x/term v0.40.0 golang.org/x/text v0.34.0 - gopkg.in/yaml.v3 v3.0.1 // indirect + gopkg.in/yaml.v3 v3.0.1 k8s.io/api v0.35.1 k8s.io/apiextensions-apiserver v0.35.1 k8s.io/apimachinery v0.35.1 diff --git a/pkg/action/package.go b/pkg/action/package.go index 86426b412..4a503ae24 100644 --- a/pkg/action/package.go +++ b/pkg/action/package.go @@ -26,7 +26,6 @@ import ( "github.com/Masterminds/semver/v3" "golang.org/x/term" - "sigs.k8s.io/yaml" ci "helm.sh/helm/v4/pkg/chart" "helm.sh/helm/v4/pkg/chart/loader" @@ -176,8 +175,9 @@ func (p *Package) Clearsign(filename string) error { return errors.New("invalid chart apiVersion") } - // Marshal chart metadata to YAML bytes - metadataBytes, err := yaml.Marshal(ch.Metadata) + // Marshal chart metadata to YAML bytes suitable for provenance files. + // Uses flow style for sequences to avoid PGP dash-escaping of YAML list items. + metadataBytes, err := provenance.MarshalMetadata(ch.Metadata) if err != nil { return fmt.Errorf("failed to marshal chart metadata: %w", err) } diff --git a/pkg/provenance/sign.go b/pkg/provenance/sign.go index 45d4fe1a5..16a55ca71 100644 --- a/pkg/provenance/sign.go +++ b/pkg/provenance/sign.go @@ -28,6 +28,7 @@ import ( "github.com/ProtonMail/go-crypto/openpgp" "github.com/ProtonMail/go-crypto/openpgp/clearsign" "github.com/ProtonMail/go-crypto/openpgp/packet" + yaml3 "gopkg.in/yaml.v3" "sigs.k8s.io/yaml" ) @@ -35,6 +36,38 @@ var defaultPGPConfig = packet.Config{ DefaultHash: crypto.SHA512, } +// MarshalMetadata serializes metadata to YAML suitable for use in a provenance file. +// +// Unlike standard YAML marshaling, this function uses flow style (inline) for +// sequence (array) fields. This avoids PGP clear-text signature "dash escaping", +// where lines beginning with '-' are prefixed with '- ', which would cause YAML +// list items to be double-nested (e.g., "- - foo" instead of "- foo") in the +// provenance file. +func MarshalMetadata(meta any) ([]byte, error) { + // Marshal to block-style YAML first + yamlBytes, err := yaml.Marshal(meta) + if err != nil { + return nil, err + } + // Parse into a yaml.v3 Node tree so we can set flow style on sequences + var node yaml3.Node + if err := yaml3.Unmarshal(yamlBytes, &node); err != nil { + return nil, err + } + setFlowStyleForSequences(&node) + return yaml3.Marshal(&node) +} + +// setFlowStyleForSequences recursively sets flow style on all YAML sequence nodes. +func setFlowStyleForSequences(node *yaml3.Node) { + if node.Kind == yaml3.SequenceNode { + node.Style = yaml3.FlowStyle + } + for _, n := range node.Content { + setFlowStyleForSequences(n) + } +} + // SumCollection represents a collection of file and image checksums. // // Files are of the form: diff --git a/pkg/provenance/sign_test.go b/pkg/provenance/sign_test.go index eef01c52e..fe5b00e20 100644 --- a/pkg/provenance/sign_test.go +++ b/pkg/provenance/sign_test.go @@ -28,7 +28,6 @@ import ( "github.com/ProtonMail/go-crypto/openpgp/packet" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "sigs.k8s.io/yaml" "helm.sh/helm/v4/pkg/chart/v2/loader" ) @@ -71,6 +70,10 @@ const ( // with shasum. // shasum -a 256 hashtest-1.2.3.tgz > testdata/hashtest.sha256 testSumfile = "testdata/hashtest.sha256" + + // testChartfileWithKeywords is a chart archive that includes keywords and sources + // fields to test that they are not double-nested in the provenance file. + testChartfileWithKeywords = "testdata/hashtest-keywords-1.2.3.tgz" ) // testMessageBlock represents the expected message block for the testdata/hashtest chart. @@ -93,7 +96,7 @@ func loadChartMetadataForSigning(t *testing.T, chartPath string) []byte { t.Fatal(err) } - metadataBytes, err := yaml.Marshal(chart.Metadata) + metadataBytes, err := MarshalMetadata(chart.Metadata) if err != nil { t.Fatal(err) } @@ -121,6 +124,51 @@ func TestMessageBlock(t *testing.T) { } } +// TestMessageBlockKeywordsAndSources verifies that keywords and sources +// are not double-nested in the provenance file metadata. +// Regression test for https://github.com/helm/helm/issues/31866 +func TestMessageBlockKeywordsAndSources(t *testing.T) { + metadataBytes := loadChartMetadataForSigning(t, testChartfileWithKeywords) + + // Verify the metadata YAML does not contain double-nested arrays + metadataStr := string(metadataBytes) + if strings.Contains(metadataStr, "- - ") { + t.Errorf("metadata contains double-nested array entries (- - ), indicating keywords/sources are incorrectly wrapped:\n%s", metadataStr) + } + + // Verify the metadata contains correctly formatted keywords and sources using flow style. + // Flow style avoids PGP dash-escaping, which would otherwise double-nest list items. + assert.Contains(t, metadataStr, "keywords: [foo, bar]", "keywords should use flow style to avoid PGP dash-escaping") + assert.Contains(t, metadataStr, "sources:", "sources field should be present") + + // Verify the metadata can be round-tripped: unmarshal back into Metadata and check fields + chart, err := loader.LoadFile(testChartfileWithKeywords) + if err != nil { + t.Fatal(err) + } + assert.Equal(t, []string{"foo", "bar"}, chart.Metadata.Keywords, "keywords should be a flat string slice") + assert.Equal(t, []string{"https://example.com", "https://example.org"}, chart.Metadata.Sources, "sources should be a flat string slice") + + // Verify the full message block is well-formed + archiveData, err := os.ReadFile(testChartfileWithKeywords) + if err != nil { + t.Fatal(err) + } + out, err := messageBlock(archiveData, filepath.Base(testChartfileWithKeywords), metadataBytes) + if err != nil { + t.Fatal(err) + } + + // Parse the message block back and verify checksums section is valid + sc, err := parseMessageBlock(out.Bytes()) + if err != nil { + t.Fatal(err) + } + if _, ok := sc.Files["hashtest-keywords-1.2.3.tgz"]; !ok { + t.Error("chart file not found in message block checksums") + } +} + func TestParseMessageBlock(t *testing.T) { sc, err := parseMessageBlock([]byte(testMessageBlock)) if err != nil { diff --git a/pkg/provenance/testdata/hashtest-keywords-1.2.3.tgz b/pkg/provenance/testdata/hashtest-keywords-1.2.3.tgz new file mode 100644 index 0000000000000000000000000000000000000000..784e11cbf3a474b3d278b8e635a5230741a4f5d8 GIT binary patch literal 464 zcmV;>0Wba^iwFRiv#n_W1MQYeZrd;rhTSgNOAN9P&8HO`$ifK%*nm+a%>(F?mFY^b z^(e{sagFYBgq$N6=#fI%v4cW}f}%o;*7`OhafaZK(*H}BYB|tqtW7&oHgej!!C7`y!l2$~e4I$?W&m@aNGH;urj%k${oL|NjL0 z{amPM@3gUTmiA!IP^uE$gVIZZ z#@dD^&fU81#-|`GUK3cvE`n_psB{cAyEmb>Xgk+iSP= zQYD2wc%#@mo|&)6t#aR7VIy&