Fix: Provenance file chart.yaml doesn't match the source#31916
Fix: Provenance file chart.yaml doesn't match the source#31916gabrnavarro wants to merge 1 commit intohelm:mainfrom
Conversation
pull-request-size
bot
added
the
size/L
There was a problem hiding this comment.
Pull request overview
Fixes Helm provenance (.prov) generation so chart metadata containing YAML list fields (e.g., keywords, sources) is serialized in flow style, preventing PGP cleartext “dash escaping” from corrupting the embedded Chart.yaml content.
Changes:
- Add
provenance.MarshalMetadatato emit YAML with flow-style sequences. - Use
provenance.MarshalMetadataduringhelm package --signprovenance generation. - Add a regression test and new chart fixture containing
keywords/sources.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/provenance/sign.go | Introduces MarshalMetadata and sequence flow-style rewriting via yaml.v3 nodes. |
| pkg/action/package.go | Switches chart metadata marshaling for signing to provenance.MarshalMetadata. |
| pkg/provenance/sign_test.go | Adds a regression test and updates helper to use MarshalMetadata. |
| pkg/provenance/testdata/hashtest-keywords/Chart.yaml | New fixture chart metadata containing list fields. |
| pkg/provenance/testdata/hashtest-keywords.sha256 | Checksum for the new keywords test fixture. |
| pkg/provenance/testdata/hashtest-keywords-1.2.3.tgz | Packaged chart fixture with list fields. |
| go.mod | Promotes gopkg.in/yaml.v3 to a direct dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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) | ||
| } |
There was a problem hiding this comment.
In this regression test, the "- - " check is applied to metadataBytes before the data goes through clearsign.Encode (where dash-escaping happens). That means it would not have caught the original bug, which only appears in the on-disk .prov content. Consider asserting on the raw sig string returned by ClearSign (or a saved .prov) to ensure it does not contain "\n- - " for list items, and optionally verify that clearsign.Decode(sig).Plaintext still round-trips.
| // 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") |
There was a problem hiding this comment.
The comment says the metadata is "round-tripped" by unmarshalling metadataBytes, but the test is actually re-loading the chart archive via loader.LoadFile and never parses metadataBytes. Either unmarshal metadataBytes back into a chart.Metadata (or a struct that contains Keywords/Sources) or adjust the comment/test to reflect what is being validated.
What changed
When signing a chart that contains list fields in
Chart.yaml(e.g.,keywordsorsources), the provenance file now correctly serializes those lists using YAML flow style (e.g.,keywords: [foo, bar]) instead of YAML block style (e.g.,- foo).A new exported
MarshalMetadatafunction was added topkg/provenancethat handles this serialization.pkg/action/package.gowas updated to use this function.A regression test and test fixture were added to ensure the fix is covered.
Why
PGP clear-text signatures (RFC 4880) require "dash escaping": any line beginning with
-must be prefixed with-in the signed body. YAML block-style list items start with-, so when chart metadata containing lists (likekeywordsorsources) was embedded in a provenance file, the PGP encoder would transform- foointo- - foo.While Helm's verification code correctly un-escapes these when parsing the
block.Plaintext, the raw provenance file stored on disk contained corrupted YAML. Users who tried to read or copy theChart.yamlsection from the.provfile would find double-nested arrays that couldn't be parsed as valid YAML.Fixes #31866
Testing
TestMessageBlockKeywordsAndSourcesregression test that verifies:-entries)keywords: [foo, bar]serializes without double-nestingtestdata/hashtest-keywords-1.2.3.tgztest fixture containing a chart withkeywordsandsourcesfields