Add OCI signing as part of existing publish pipeline#290
Add OCI signing as part of existing publish pipeline#290SgtCoDFish wants to merge 1 commit intocert-manager:masterfrom
Conversation
The aim is for this to replace the need for hack/push_and_sign_chart.sh and to make charts.jetstack.io downstream of the OCI registry. Signed-off-by: Ashley Davis <ashley.davis@cyberark.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR extends the existing release publish pipeline to publish Helm charts to an OCI registry and sign/verify those OCI artifacts (intended to replace hack/push_and_sign_chart.sh and make charts.jetstack.io downstream of the OCI registry).
Changes:
- Add cosign helpers for signing/verifying with explicit option flags.
- Add Helm OCI helpers (
helm push,crane copy) and a newgcb publishaction (helmchartoci) to push/sign/verify charts (including non-vtag copy). - Update the Cloud Build publish job to install/use cosign v3, crane, and helm; plumb a new substitution/flag for the OCI registry target.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/sign/cosign/cosign.go | Adds optioned cosign sign/verify wrappers used by OCI chart signing. |
| pkg/release/helm/oci.go | Introduces helper wrappers for helm push to OCI and crane copy tag operations. |
| gcb/publish/cloudbuild.yaml | Installs cosign v3, crane, and helm; passes new OCI-related args/substitution into cmrel gcb publish. |
| cmd/cmrel/cmd/publish.go | Adds CLI flag + build substitution for the published Helm chart OCI registry. |
| cmd/cmrel/cmd/gcb_publish.go | Adds new publish action helmchartoci implementing push/sign/verify logic; adds helm/crane paths + OCI registry options. |
| cmd/cmrel/cmd/const.go | Adds a default OCI registry constant for Helm chart publishing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := signOCIImages(ctx, o, pushedContent); err != nil { | ||
| return fmt.Errorf("failed to sign images: %w", err) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
With the Cloud Build config upgrading cosign to v3, the container image signing path (now signOCIImages) still relies on cosign.Sign(...), which uses cosign defaults. Elsewhere (chart signing) you explicitly disable cosign v3 defaults like --use-signing-config / --new-bundle-format / tlog behavior to preserve existing behavior. Consider aligning image signing with the chart signing flags (e.g., add an optioned signing helper) to avoid behavior changes or failures after the cosign v3 upgrade.
| - name: docker.io/library/golang:1.26-alpine@sha256:c2a1f7b2095d046ae14b286b18413a05bb82c9bca9b25fe7ff5efef0f0826166 | ||
| entrypoint: sh | ||
| args: | ||
| - -c | ||
| - | | ||
| apk add --no-cache curl | ||
| curl https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 | sh | ||
| mv /usr/local/bin/helm /go/bin/helm |
There was a problem hiding this comment.
The Helm installer step downloads and executes an unpinned script from the Helm repo’s main branch (curl ... | sh). This is a supply-chain risk and can make builds non-reproducible; additionally, piping into sh may fail if the script expects bash. Prefer installing a pinned Helm release artifact (or a pinned script commit) and verify checksums/signatures; if you keep the script approach, invoke it with bash and pin the commit/ref.
| ociURL := fmt.Sprintf("oci://%s", o.PublishedHelmChartOCIRegistry) | ||
|
|
||
| // Push chart to OCI registry (helm automatically pushes .prov if it exists) | ||
| log.Printf("Pushing chart to %s", ociURL) | ||
| if err := helm.PushChartToOCI(ctx, chartPath, ociURL, o.HelmPath); err != nil { |
There was a problem hiding this comment.
published-helm-chart-oci-registry is described as an OCI registry, but pushHelmChartOCI always prefixes it with oci://. If a caller passes an oci://... value (which is common in Helm docs), this will produce an invalid URL (oci://oci://...) and also break the cosign/crane refs built from the same string. Consider normalizing the input once (e.g., trim optional oci:// prefix, validate it’s host/path only) before constructing ociURL, chartRef, and destRef.
| var publishActionMap map[string]publishAction = map[string]publishAction{ | ||
| "helmchartpr": pushHelmChartPR, | ||
| "helmchartoci": pushHelmChartOCI, | ||
| "githubrelease": pushGitHubRelease, | ||
| "pushcontainerimages": pushContainerImages, | ||
|
|
||
| // helmchartpr has been deprecated in favour of helmchartoci | ||
| // "helmchartpr": pushHelmChartPR, | ||
| } |
There was a problem hiding this comment.
The action map removes helmchartpr entirely but the comment says it is “deprecated”. As written, users specifying --publish-actions=helmchartpr will now error out, which is a breaking CLI change rather than a deprecation. Consider keeping helmchartpr as an alias to the new OCI implementation (and log a warning), or implement an explicit migration path in canonicalizeAndVerifyPublishActions.
| func pushHelmChartOCI(ctx context.Context, o *gcbPublishOptions, rel *release.Unpacked) error { | ||
| log.Printf("Pushing Helm chart to OCI registry %q", o.PublishedHelmChartOCIRegistry) | ||
|
|
||
| // Verify tools are available | ||
| log.Printf("Verifying helm installation") | ||
| if err := shell.Command(ctx, "", o.HelmPath, "version"); err != nil { | ||
| return fmt.Errorf("failed to verify helm installation: %w", err) | ||
| } |
There was a problem hiding this comment.
New publish behavior is introduced in pushHelmChartOCI (helm push, crane copy, cosign sign/verify) but there are no tests covering the command construction/branching (skip-signing, non-v tag copy). Since this package already has tests, consider adding unit tests by refactoring command execution behind an interface so you can assert the expected helm/crane/cosign invocations without running external binaries.
| "--tlog-upload=" + fmt.Sprintf("%t", opts.TlogUpload), | ||
| "--new-bundle-format=" + fmt.Sprintf("%t", opts.NewBundleFormat), | ||
| "--use-signing-config=" + fmt.Sprintf("%t", opts.UseSigningConfig), |
There was a problem hiding this comment.
SignWithOptions builds boolean flag values via fmt.Sprintf("%t", ...). Using strconv.FormatBool avoids pulling in fmt just for boolean formatting and is simpler/cheaper.
The aim is for this to replace the need for hack/push_and_sign_chart.sh and to make charts.jetstack.io downstream of the OCI registry.