implement Cosign verification for HelmCharts #925

souleb · 2022-10-03T12:45:33Z

fixes #914

If implemented, users will be able to enable chart verification for OCI based helm charts.

The remote builder will not attempt to download the chart if an artifact exist with the same name and version and the force option is false. It will try to verify the chart if:

we are on the first reconciliation
the HelmtChartSpec has changed (generation drift)
the previous reconciliation resulted in a failed artifact verification
there is no artifact in storage

The helmChart metadata does not contain the digest so we cannot rely on it to detect drift on the remote.

⚠️ We do not enable verification for umbrella charts stored in Git with dependencies resolved from OCI registries, as we assume that each dependency need its own key.

Tests

aws + ecr with credentials and autologin
gcp + artifact registry with autologin and keyless

Signed-off-by: Soule BA soule@weave.works

api/v1beta2/helmchart_types.go

stefanprodan

Can you please add a cosign keyless e2e test for https://github.com/stefanprodan/podinfo/pkgs/container/charts%2Fpodinfo

stefanprodan

Can you please add the verify field to the HelmChart API spec doc.

souleb · 2022-10-17T21:11:07Z

In the OCIRepository controller there is a loop to collect keys from a secret and perform verification using that list. The thing is that we break from the list as soon as a signature verification is successful with a key from the list. Is that the intended behavior, or shall we successful verify with all keys?

stefanprodan · 2022-10-18T07:44:35Z

Is that the intended behavior, or shall we successful verify with all keys?

We shouldn't verify all keys, if one is a match than it's all good IMO. This way we allow people to do key rotation, old artifacts will still be valid if the old key is in the secret. The same we do for SOPS and decryption keys.

controllers/helmchart_controller.go

internal/helm/registry/auth.go

internal/helm/repository/chart_repository.go

internal/helm/chart/builder_remote.go

controllers/helmchart_controller.go

internal/oci/verifier.go

stefanprodan

We're missing summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition) from the helmchart_controller.

api/v1beta2/helmchart_types.go

internal/helm/registry/auth.go

internal/helm/repository/chart_repository.go

docs/spec/v1beta2/ocirepositories.md

docs/spec/v1beta2/helmcharts.md

controllers/helmchart_controller.go

souleb · 2022-10-19T14:45:23Z

We shouldn't verify all keys, if one is a match than it's all good IMO. This way we allow people to do key rotation, old > artifacts will still be valid if the old key is in the secret. The same we do for SOPS and decryption keys.

I have change the code and documentation to take this into account.

controllers/helmchart_controller.go

souleb · 2022-10-20T12:09:30Z

We discussed this with @darkowlzz and we think that we could persist a given chart digest after successfully building in the Status of the resource. We could then pass it to the chartBuilder in future reconciliation in order to enable skipping verification.

Thoughts @stefanprodan @hiddeco @makkes

Edit: The other option is to have Helm surface a digest field in the chart metadata as it does with the version. We would store it as part of the generated artifact.

stefanprodan · 2022-10-20T12:20:28Z

In OCIRepository we persist the digest in the revision field in the format <tag>/<digest>, we could do the same for HelmCharts.

hiddeco · 2022-10-20T12:34:59Z

Can someone remind me why we continue to prefer <tag>/<digest> over <tag>@<digest>? Which is the more common format used to tie a named reference to a specific unique reference/digest, including while working with the OCI spec itself.

stefanprodan · 2022-10-20T12:52:35Z

Can someone remind me why we continue to prefer / over @?

To be consistent with how we report the revision to consumers. The current format is <label>/<sha-hex> without having the SHA type encoded. Please create an RFC if you wish to change this in across Flux, also NC that relies on this format. If we do change it, then I propose we add the SHA type as well, so for Git it would be <branch|tag>@sha1:<commit-sha> and for OCI would be <tag>@sha256:<digest-hex>.

hiddeco · 2022-10-20T13:08:08Z

If we do change it, then I propose we add the SHA type as well

This has overlap with the already proposed #855.

If implemented the oras registry loginOption will only be used internaly with the specific ChartRepo struct. This will permit reusing more easily feature developped with googlecontainerregistry authn. Signed-off-by: Soule BA <soule@weave.works>

This remove test case for contextual login on oci://123456789000.dkr.ecr.us-east-2.amazonaws.com. This is not longer a wrong url since fluxcd/pkg@f7c66eb and we no longer error on nil auth. Signed-off-by: Soule BA <soule@weave.works>

If implemented, users will be able to enable chart verification for OCI based helm charts. Signed-off-by: Soule BA <soule@weave.works>

Signed-off-by: Soule BA <soule@weave.works>

docs/spec/v1beta2/helmcharts.md

darkowlzz

Suggested one last change.
Everything else looks good to me.

controllers/helmchart_controller.go

Delete a failed verification condition at the beginning of the source reconciliation and set `SourceVerifiedCondition` to false approprietly. Set the `BuildOptions.Verify` to true as long as Verify is enabled in the API fields. Signed-off-by: Soule BA <soule@weave.works>

darkowlzz

LGTM!

hiddeco

Did not do an as thorough review as @darkowlzz, but did look at the outline and some common things I tend to nitpick on which all appear to be OK. Based on this, it looks good to me.

Thank you @souleb 🙇

stefanprodan

LGTM

Thanks @souleb and @darkowlzz 🏅

souleb · 2022-10-21T13:42:58Z

Following this comment, it has bee decided to always perform verification for now.

In a follow up Pull request we will propose pulling the digest in remote.GetChartVersion() and persist it in the spec.Status in the format <tag>/<digest>. We could then use it in the remoteBuilder to decide whether to verify.

souleb marked this pull request as draft October 3, 2022 12:45

souleb force-pushed the cosign-verify-helm branch from 141b3de to 86330bf Compare October 3, 2022 13:33

souleb added area/helm Helm related issues and pull requests area/oci OCI related issues and pull requests labels Oct 4, 2022

souleb force-pushed the cosign-verify-helm branch from 18597a5 to 697f885 Compare October 4, 2022 19:52

stefanprodan reviewed Oct 6, 2022

View reviewed changes

api/v1beta2/helmchart_types.go Outdated Show resolved Hide resolved

stefanprodan reviewed Oct 6, 2022

View reviewed changes

souleb force-pushed the cosign-verify-helm branch from 697f885 to 05db6bd Compare October 12, 2022 13:37

souleb marked this pull request as ready for review October 12, 2022 13:53

souleb requested review from stefanprodan, darkowlzz and hiddeco October 12, 2022 13:54

souleb force-pushed the cosign-verify-helm branch from 05db6bd to 53dfbd8 Compare October 12, 2022 13:55

stefanprodan reviewed Oct 17, 2022

View reviewed changes

makkes reviewed Oct 18, 2022

View reviewed changes

stefanprodan reviewed Oct 18, 2022

View reviewed changes

souleb force-pushed the cosign-verify-helm branch from 53dfbd8 to c3f7109 Compare October 18, 2022 23:34

souleb mentioned this pull request Oct 19, 2022

Enable Cosign verification of Helm charts stored as OCI artifacts in container registries fluxcd/helm-controller#545

Merged

stefanprodan requested a review from makkes October 19, 2022 11:49

darkowlzz reviewed Oct 19, 2022

View reviewed changes

darkowlzz reviewed Oct 20, 2022

View reviewed changes

controllers/helmchart_controller.go Outdated Show resolved Hide resolved

souleb force-pushed the cosign-verify-helm branch from c3f7109 to 21e648e Compare October 20, 2022 11:52

stefanprodan mentioned this pull request Oct 20, 2022

Prepare for v0.36 release fluxcd/flux2#3228

Closed

15 tasks

souleb force-pushed the cosign-verify-helm branch from 21e648e to 39a6832 Compare October 20, 2022 12:21

hiddeco added this to the GA milestone Oct 20, 2022

hiddeco mentioned this pull request Oct 20, 2022

RFC-0005: Artifact Revision format and introduction of Digest fluxcd/flux2#3233

Merged

souleb added 5 commits October 21, 2022 10:00

Remove test case on aws

55dd799

This remove test case for contextual login on oci://123456789000.dkr.ecr.us-east-2.amazonaws.com. This is not longer a wrong url since fluxcd/pkg@f7c66eb and we no longer error on nil auth. Signed-off-by: Soule BA <soule@weave.works>

implement Cosign verification for HelmCharts

0e97547

If implemented, users will be able to enable chart verification for OCI based helm charts. Signed-off-by: Soule BA <soule@weave.works>

adding verfication section to HelmChart api doc

5355fb3

Signed-off-by: Soule BA <soule@weave.works>

addressing review comments

25673ac

Signed-off-by: Soule BA <soule@weave.works>

souleb force-pushed the cosign-verify-helm branch 2 times, most recently from 46466de to a794e28 Compare October 21, 2022 08:24

darkowlzz reviewed Oct 21, 2022

View reviewed changes

docs/spec/v1beta2/helmcharts.md Show resolved Hide resolved

souleb force-pushed the cosign-verify-helm branch from a794e28 to b179285 Compare October 21, 2022 09:25

darkowlzz reviewed Oct 21, 2022

View reviewed changes

controllers/helmchart_controller.go Outdated Show resolved Hide resolved

souleb force-pushed the cosign-verify-helm branch from b179285 to 06a5559 Compare October 21, 2022 13:21

darkowlzz approved these changes Oct 21, 2022

View reviewed changes

hiddeco approved these changes Oct 21, 2022

View reviewed changes

stefanprodan approved these changes Oct 21, 2022

View reviewed changes

stefanprodan merged commit 09cae78 into fluxcd:main Oct 21, 2022

souleb deleted the cosign-verify-helm branch October 21, 2022 14:00

developer-guy mentioned this pull request Nov 1, 2022

Prove the Authenticity of the Helm OCI repositories blog post fluxcd/website#1240

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement Cosign verification for HelmCharts #925

implement Cosign verification for HelmCharts #925

souleb commented Oct 3, 2022 •

edited

Loading

stefanprodan left a comment

stefanprodan left a comment

souleb commented Oct 17, 2022

stefanprodan commented Oct 18, 2022

stefanprodan left a comment

souleb commented Oct 19, 2022

souleb commented Oct 20, 2022 •

edited

Loading

stefanprodan commented Oct 20, 2022 •

edited

Loading

hiddeco commented Oct 20, 2022

stefanprodan commented Oct 20, 2022 •

edited

Loading

hiddeco commented Oct 20, 2022

darkowlzz left a comment

darkowlzz left a comment

hiddeco left a comment

stefanprodan left a comment

souleb commented Oct 21, 2022 •

edited

Loading

implement Cosign verification for HelmCharts #925

implement Cosign verification for HelmCharts #925

Conversation

souleb commented Oct 3, 2022 • edited Loading

Tests

stefanprodan left a comment

Choose a reason for hiding this comment

stefanprodan left a comment

Choose a reason for hiding this comment

souleb commented Oct 17, 2022

stefanprodan commented Oct 18, 2022

stefanprodan left a comment

Choose a reason for hiding this comment

souleb commented Oct 19, 2022

souleb commented Oct 20, 2022 • edited Loading

stefanprodan commented Oct 20, 2022 • edited Loading

hiddeco commented Oct 20, 2022

stefanprodan commented Oct 20, 2022 • edited Loading

hiddeco commented Oct 20, 2022

darkowlzz left a comment

Choose a reason for hiding this comment

darkowlzz left a comment

Choose a reason for hiding this comment

hiddeco left a comment

Choose a reason for hiding this comment

stefanprodan left a comment

Choose a reason for hiding this comment

souleb commented Oct 21, 2022 • edited Loading

souleb commented Oct 3, 2022 •

edited

Loading

souleb commented Oct 20, 2022 •

edited

Loading

stefanprodan commented Oct 20, 2022 •

edited

Loading

stefanprodan commented Oct 20, 2022 •

edited

Loading

souleb commented Oct 21, 2022 •

edited

Loading