feat(notation): add support for notation in HelmChart and OCIRepository configuration #1075

JasonTheDeveloper · 2023-04-17T06:32:59Z

This PR introduces notation as an additional option to cosign as a provider to verify the signature of source helmchart/ocirepository deployment.

This PR is related to issue #1072.

Testing

To test the changes to helmchart and ocirepository I have included a couple example configuration deployments under config/testdata/helmchart-from-oci and /workspaces/source-controller/config/testdata/ocirepository. These manifests are also deployed when running make e2e.

These manifests deploy a forked version of stefanprodan's podinfo, singed using notation and stored in my ghcr found here: https://github.com/JasonTheDeveloper?tab=packages&repo_name=podinfo.

Sample trust store policy and public key needed for verification can also be found here: https://github.com/JasonTheDeveloper/podinfo/tree/feat/notation/.notation.

Todo

~~Expand on unit tests~~
~~Update documentation~~
~~Add examples~~
Update Flux CLI subcommand flux create secret to generate notation configuration

makkes · 2023-11-23T10:36:39Z

Thanks @JasonTheDeveloper for creating this PR! I'd be happy to help getting this over the line. Are you planning to add the missing tests and documentation anytime soon? Happy to chat on the CNCF Slack as well.

JasonTheDeveloper · 2023-12-04T16:15:39Z

Thanks @JasonTheDeveloper for creating this PR! I'd be happy to help getting this over the line. Are you planning to add the missing tests and documentation anytime soon? Happy to chat on the CNCF Slack as well.

@makkes - I have added the missing tests. I'll be adding the documentation later today. Apologies for taking so long to getting around to finishing off this PR. Originally, I was holding off until notation 1.0.0 was released to the public just in case there were any major changes prior to going public. Looks like that happened a couple months ago 😅

internal/oci/notation.go

souleb

Thanks for this contribution @JasonTheDeveloper. I left some comments

internal/controller/helmchart_controller.go

internal/controller/ocirepository_controller.go

internal/oci/notation.go

souleb

can you put this in a separate PR?

.devcontainer.json

internal/oci/notation.go

internal/oci/verifier.go

internal/controller/helmchart_controller.go

internal/controller/ocirepository_controller.go

docs/spec/v1beta2/helmcharts.md

docs/spec/v1beta2/ocirepositories.md

souleb · 2024-01-29T16:05:25Z

can you also rebase your branch?

Makefile

docs/spec/v1beta2/helmcharts.md

go.mod

hack/ci/e2e.sh

internal/oci/notation.go

internal/oci/notation_test.go

internal/oci/notation.go

internal/controller/helmchart_controller.go

internal/helm/getter/client_opts.go

internal/oci/verifier.go

internal/oci/notation.go

internal/controller/helmchart_controller.go

internal/controller/ocirepository_controller_test.go

internal/controller/ocirepository_controller.go

internal/oci/cosign/cosign_test.go

internal/oci/notation/notation.go

internal/controller/ocirepository_controller.go

JasonTheDeveloper · 2024-03-13T09:17:12Z

We could check for skip and set trustStores/trustedIdentities to an empty string array + log so that notation doesn't complain.

Yes maybe this could be a function that take a trustPolicy and return a cleaned version that we can use for the verifier instantiation.

I've created a new func in 86e98fa and added unit tests in a4c93d9.

go.mod

internal/helm/common/string_resource.go

internal/oci/notation/notation_test.go

internal/oci/notation/notation.go

darkowlzz

I did some manual testing with OCIRepository, tried various things to break the implementation but so far everything looks good to me.
TestOCIRepository_reconcileSource_verifyOCISourceTrustPolicyNotation tests are great for confidence in the correctness.

I've left a few suggestions for minor improvements.

Will do final OCI HelmChart testing next.

internal/oci/notation/notation.go

internal/controller/ocirepository_controller.go

internal/oci/verifier.go

darkowlzz

I did manual testing for OCI HelmChart verification and it also worked very well.
I'm confident with the results.

Going through the code, I found some potential bugs and inconsistencies, nothing major. Left comment for those. Please have a look.
Once these and the previous comments are addressed, I will approve this PR.

Thanks for all the work on this.

internal/helm/repository/oci_chart_repository.go

internal/controller/helmchart_controller.go

internal/oci/notation/notation_test.go

internal/controller/ocirepository_controller.go

internal/controller/helmchart_controller.go

internal/controller/ocirepository_controller.go

internal/oci/notation/notation.go

darkowlzz

LGTM!
Thanks for spending so much time on getting this right.

Would be great if you could squash all the commits to a few relevant commits if possible.

JasonTheDeveloper · 2024-03-26T10:18:47Z

@darkowlzz thank you so much for the approval! I tried to create separate squash commits to group things into "like" commits but it was taking too long so instead squashed everything into the one commit. Hope that's fine.

souleb · 2024-03-26T10:21:23Z

@JasonTheDeveloper do you mind rewriting the commit message. You may want to have a look at https://github.com/fluxcd/flux2/blob/main/CONTRIBUTING.md#format-of-the-commit-message

stefanprodan

LGTM

Awesome contribution! Thanks @JasonTheDeveloper and @jagpreetstamber 🥇

Introduces a new verification provider `notation` to verify notation signed artifacts. Currently only cosign is supported and that is a problem if the end user utilises notation. --------- Signed-off-by: Jason <[email protected]> Signed-off-by: JasonTheDeveloper <[email protected]> Signed-off-by: Jagpreet Singh Tamber <[email protected]> Co-authored-by: souleb <[email protected]> Co-authored-by: Jagpreet Singh Tamber <[email protected]> Co-authored-by: Sunny <[email protected]>

JasonTheDeveloper · 2024-03-26T10:34:22Z

@souleb something like this? Add verification support for notation signed artifacts

souleb

LGTM.

Thanks for your outstanding contribution and your willingness to accommodate and work through all the changes requested.

makkes mentioned this pull request Nov 20, 2023

Add notation support #1287

Closed

makkes self-assigned this Nov 23, 2023

makkes added area/security Security related issues and pull requests area/oci OCI related issues and pull requests area/api API related issues and pull requests labels Nov 23, 2023

JasonTheDeveloper force-pushed the feat/notation branch 2 times, most recently from 7b8500a to 83d7208 Compare November 29, 2023 06:54

github-advanced-security bot found potential problems Dec 4, 2023

View reviewed changes

internal/oci/notation.go Fixed Show fixed Hide fixed

JasonTheDeveloper marked this pull request as ready for review December 7, 2023 07:23

souleb reviewed Dec 8, 2023

View reviewed changes

JasonTheDeveloper force-pushed the feat/notation branch from 1daaaf8 to d068a41 Compare December 14, 2023 06:02

souleb reviewed Dec 20, 2023

View reviewed changes

JasonTheDeveloper force-pushed the feat/notation branch from 2342a3e to ffb46dc Compare January 10, 2024 08:28

souleb reviewed Jan 11, 2024

View reviewed changes

souleb reviewed Jan 15, 2024

View reviewed changes

souleb requested a review from darkowlzz January 29, 2024 16:05

JasonTheDeveloper force-pushed the feat/notation branch from 686b3ca to e35dc54 Compare January 29, 2024 20:43

JasonTheDeveloper requested review from souleb and stefanprodan February 2, 2024 11:22

darkowlzz reviewed Feb 5, 2024

View reviewed changes

JasonTheDeveloper mentioned this pull request Feb 9, 2024

feat(notation): sign artifacts using notation stefanprodan/podinfo#341

Merged

stefanprodan assigned darkowlzz, stefanprodan and souleb and unassigned makkes Feb 9, 2024

darkowlzz reviewed Feb 9, 2024

View reviewed changes

internal/controller/ocirepository_controller.go Outdated Show resolved Hide resolved

darkowlzz reviewed Mar 12, 2024

View reviewed changes

JasonTheDeveloper force-pushed the feat/notation branch from 03e7172 to 158184e Compare March 14, 2024 07:24

jagpreetstamber force-pushed the feat/notation branch from 1ceb15b to d0d5832 Compare March 14, 2024 14:01

souleb reviewed Mar 15, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

souleb reviewed Mar 15, 2024

View reviewed changes

internal/helm/common/string_resource.go Outdated Show resolved Hide resolved

souleb reviewed Mar 15, 2024

View reviewed changes

internal/oci/notation/notation_test.go Outdated Show resolved Hide resolved

internal/oci/notation/notation.go Outdated Show resolved Hide resolved

internal/oci/notation/notation.go Outdated Show resolved Hide resolved

internal/oci/notation/notation.go Show resolved Hide resolved

jagpreetstamber force-pushed the feat/notation branch from 7581f69 to 99b8cb5 Compare March 15, 2024 15:01

darkowlzz reviewed Mar 21, 2024

View reviewed changes

internal/oci/notation/notation.go Outdated Show resolved Hide resolved

internal/controller/ocirepository_controller.go Show resolved Hide resolved

internal/controller/ocirepository_controller.go Outdated Show resolved Hide resolved

internal/oci/verifier.go Show resolved Hide resolved

darkowlzz reviewed Mar 22, 2024

View reviewed changes

souleb reviewed Mar 25, 2024

View reviewed changes

internal/controller/ocirepository_controller.go Outdated Show resolved Hide resolved

souleb reviewed Mar 25, 2024

View reviewed changes

internal/oci/notation/notation.go Outdated Show resolved Hide resolved

internal/oci/notation/notation.go Outdated Show resolved Hide resolved

darkowlzz approved these changes Mar 25, 2024

View reviewed changes

This was referenced Mar 25, 2024

Add a subcommand to create a notation secret fluxcd/flux2#4692

Closed

Update the oci cheatsheet for notation fluxcd/website#1878

Closed

JasonTheDeveloper force-pushed the feat/notation branch 3 times, most recently from 54d3c3c to 6d61914 Compare March 26, 2024 10:12

stefanprodan approved these changes Mar 26, 2024

View reviewed changes

JasonTheDeveloper force-pushed the feat/notation branch from 6d61914 to 553945a Compare March 26, 2024 10:32

souleb approved these changes Mar 26, 2024

View reviewed changes

souleb merged commit 55a2cdb into fluxcd:main Mar 26, 2024
10 checks passed

This was referenced Apr 2, 2024

Add notation verification provider to API fluxcd/helm-controller#930

Merged

Generate unique names for notation secrets #1427

Merged

Prepare for Flux v2.3.0 fluxcd/flux2#4712

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(notation): add support for notation in HelmChart and OCIRepository configuration #1075

feat(notation): add support for notation in HelmChart and OCIRepository configuration #1075

JasonTheDeveloper commented Apr 17, 2023 •

edited

Loading

makkes commented Nov 23, 2023

JasonTheDeveloper commented Dec 4, 2023

souleb left a comment

souleb left a comment

souleb commented Jan 29, 2024

JasonTheDeveloper commented Mar 13, 2024

darkowlzz left a comment

darkowlzz left a comment

darkowlzz left a comment

JasonTheDeveloper commented Mar 26, 2024

souleb commented Mar 26, 2024

stefanprodan left a comment

JasonTheDeveloper commented Mar 26, 2024

souleb left a comment

feat(notation): add support for notation in HelmChart and OCIRepository configuration #1075

feat(notation): add support for notation in HelmChart and OCIRepository configuration #1075

Conversation

JasonTheDeveloper commented Apr 17, 2023 • edited Loading

Testing

Todo

makkes commented Nov 23, 2023

JasonTheDeveloper commented Dec 4, 2023

souleb left a comment

Choose a reason for hiding this comment

souleb left a comment

Choose a reason for hiding this comment

souleb commented Jan 29, 2024

JasonTheDeveloper commented Mar 13, 2024

darkowlzz left a comment

Choose a reason for hiding this comment

darkowlzz left a comment

Choose a reason for hiding this comment

darkowlzz left a comment

Choose a reason for hiding this comment

JasonTheDeveloper commented Mar 26, 2024

souleb commented Mar 26, 2024

stefanprodan left a comment

Choose a reason for hiding this comment

JasonTheDeveloper commented Mar 26, 2024

souleb left a comment

Choose a reason for hiding this comment

JasonTheDeveloper commented Apr 17, 2023 •

edited

Loading