Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cd): sign released container images and Helm chart #160

Merged
merged 15 commits into from
Apr 5, 2024

Conversation

hainenber
Copy link
Contributor

@hainenber hainenber commented Mar 27, 2024

Description

Sign released container images with Cosign and Helm chart with helm package --sign

Related Issue

If this pull request is related to any issue, please mention it here. Additionally, make sure that the issue is assigned to you before submitting this pull request.

Closes #141

Checklist

  • I have read the contributing documantation.
  • I signed and signed-off the commits (git commit -S -s ...)
  • I have correctly attributed the author(s) of the code.
  • I have tested the changes locally.
  • I have followed the project's style guidelines.
  • I have updated the documentation, if necessary.
  • I have added tests, if applicable.

Screenshots (if applicable)

N/A

Additional Notes

In order for testing this change, please create a GPG private key and a Cosign private key and deposit as GH secrets for this repo


Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.

@hainenber hainenber requested a review from a team as a code owner March 27, 2024 16:51
@nddq nddq added type/enhancement New feature or request area/infra Test, Release, or CI Infrastructure labels Mar 27, 2024
@rbtr
Copy link
Collaborator

rbtr commented Mar 27, 2024

@hainenber thanks for picking this up, I'll give this a thorough review ASAP.

One initial question - I noticed in the GH sample workflow they don't have any explicit keys. I think this is because they are using OIDC integration to do "keyless" signing?

This seems preferable since we wouldn't need to manage GPG keys as secrets. Do you think that's something we can do here?

@hainenber
Copy link
Contributor Author

Sure, I think it's feasible if GH decided to demonstrate their sample workflow as such. Will tinker around a bit

@hainenber hainenber force-pushed the sign-released-images branch from 0195776 to a1784eb Compare March 28, 2024 14:08
@hainenber
Copy link
Contributor Author

All relevant container images and Helm chart are now signed with cosign in keyless manner. There's 1 caveat with signing Helm chart though; it'll generate a cosign.bundle - a base64-encoded string containing both certificate and signature.

I decide to make it into a build artifact so release engineers can bundle (no pun, I swear) into a release if needed.

@rbtr rbtr added the priority/1 P1 label Mar 28, 2024
Copy link
Collaborator

@rbtr rbtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks! Left some feedback based on my testing

@rbtr
Copy link
Collaborator

rbtr commented Mar 28, 2024

I did some experimenting with this and I couldn't quite get it to work: https://github.com/rbtr/retina/actions/runs/8474428281
Here are the changes I made: rbtr@04c7fe6

Ran in to a few problems:

  • the signing step uses the image digest. We don't have that image loaded in docker, it's in the buildx context, so we have to pull it or get the digest earlier and save it. If we try to pull it, we run in to platform issues - it's tricky to pull an image for a platform that's different from the host, especially if it's Windows.
  • I can't figure out the command to verify a signature...it should be something like cosign verify ghcr.io/rbtr/retina/retina-agent@sha256:1d38a1d76f032a95ea502ec1d52dc2fc1475aa18c6a6c9ed41841c9a7fe2f0d8 --certificate-identity "https://github.com/rbtr/retina" --certificate-oidc-issuer "https://token.actions.githubusercontent.com" but I just get Error: no matching signatures. I think the certificate-identity is wrong? But when I use certificate-identity-regexp "*", still nothing.

Overall I think this is very close with the changes suggested earlier, if we can figure out a solution to the digest problem. For acceptance criteria I would love to see a published signed image (and be able to verify it with cosign) to demonstrate that this functions 🙂

@hainenber
Copy link
Contributor Author

I did some experimenting with this and I couldn't quite get it to work: https://github.com/rbtr/retina/actions/runs/8474428281 Here are the changes I made: rbtr@04c7fe6

Ran in to a few problems:

  • the signing step uses the image digest. We don't have that image loaded in docker, it's in the buildx context, so we have to pull it or get the digest earlier and save it. If we try to pull it, we run in to platform issues - it's tricky to pull an image for a platform that's different from the host, especially if it's Windows.
  • I can't figure out the command to verify a signature...it should be something like cosign verify ghcr.io/rbtr/retina/retina-agent@sha256:1d38a1d76f032a95ea502ec1d52dc2fc1475aa18c6a6c9ed41841c9a7fe2f0d8 --certificate-identity "https://github.com/rbtr/retina" --certificate-oidc-issuer "https://token.actions.githubusercontent.com" but I just get Error: no matching signatures. I think the certificate-identity is wrong? But when I use certificate-identity-regexp "*", still nothing.

Overall I think this is very close with the changes suggested earlier, if we can figure out a solution to the digest problem. For acceptance criteria I would love to see a published signed image (and be able to verify it with cosign) to demonstrate that this functions 🙂

Thanks for your effort on testing out the changes on your forked repo! My apologies for having this under the radar as I tend to disable GH actions in my forked repos to cut CI costs

Re: 1st point, I'm trying to store image digest with this --metadata-file. This should be able to cover this concern once I'm done with another round of tinkering.

Onto the 2nd point, from cosign docs, it seems the OIDC issuer for GH-stored images is "https://github.com/login/oauth" and the cert's identity is the signer's address. Another round of tinkering is needed to validate this ofc.

Onto the lab, hopefully a short round this time.

To save up your effort to other Retina's more pressuring issues, I'll convert this one to Draft and will seek your reviews once it deems as worthy.

@hainenber hainenber marked this pull request as draft March 29, 2024 15:02
@rbtr
Copy link
Collaborator

rbtr commented Mar 29, 2024

Thanks for your effort on testing out the changes on your forked repo! My apologies for having this under the radar as I tend to disable GH actions in my forked repos to cut CI costs

Re: 1st point, I'm trying to store image digest with this --metadata-file. This should be able to cover this concern once I'm done with another round of tinkering.

Onto the 2nd point, from cosign docs, it seems the OIDC issuer for GH-stored images is "https://github.com/login/oauth" and the cert's identity is the signer's address. Another round of tinkering is needed to validate this ofc.

Onto the lab, hopefully a short round this time.

To save up your effort to other Retina's more pressuring issues, I'll convert this one to Draft and will seek your reviews once it deems as worthy.

Happy to collab on this, no problem at all. This is an important feature to me 🙂
I can keep pulling it into my fork or you can PR there to get Actions runs? Let me know how I can help here.

hainenber and others added 5 commits April 4, 2024 22:17
Signed-off-by: hainenber <dotronghai96@gmail.com>
Signed-off-by: hainenber <dotronghai96@gmail.com>
Signed-off-by: hainenber <dotronghai96@gmail.com>
Signed-off-by: hainenber <dotronghai96@gmail.com>
Co-authored-by: Evan Baker <rbtr@users.noreply.github.com>
Signed-off-by: Đỗ Trọng Hải <41283691+hainenber@users.noreply.github.com>
@hainenber hainenber force-pushed the sign-released-images branch from e366233 to 06ceea0 Compare April 4, 2024 15:17
Signed-off-by: hainenber <dotronghai96@gmail.com>
@hainenber hainenber force-pushed the sign-released-images branch from 06ceea0 to bbd14e2 Compare April 4, 2024 15:18
hainenber and others added 3 commits April 4, 2024 22:20
Signed-off-by: hainenber <dotronghai96@gmail.com>
Signed-off-by: hainenber <dotronghai96@gmail.com>
@hainenber hainenber marked this pull request as ready for review April 4, 2024 15:44
@hainenber
Copy link
Contributor Author

hi @rbtr, I've fixed the signing for released images, charts and manifest. These are the successful actions in my forked repo

image

You can verify the artifacts via cosign verify <oci_image_url> --certificate-oidc-issuer https://token.actions.githubusercontent.com --certificate-identity="https://github.com/hainenber/retina/.github/workflows/release-images.yaml@refs/heads/sign-released-images"". I'm verifying a manifest here

image

@rbtr
Copy link
Collaborator

rbtr commented Apr 4, 2024

thanks @hainenber! I will test this out 🙂

Signed-off-by: hainenber <dotronghai96@gmail.com>
@hainenber
Copy link
Contributor Author

Please do! Bon apetit :D

Copy link
Contributor

@nddq nddq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some nit comments, thanks!

rbtr
rbtr previously approved these changes Apr 4, 2024
Copy link
Collaborator

@rbtr rbtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally LGTM! This is great, thanks @hainenber 🍻
Aside from anyone else's comments, my last ask would be to updated the README and docs.
I'm thinking we add a block such as:

### Verify signed images
Retina images published to GHCR are cryptographically signed. You can verify their provenance with [`sigstore/cosign`](https://github.com/sigstore/cosign):

```shell
REPO=microsoft/retina # or your repo
IMAGE=retina-operator # or other image to verify
TAG=v0.0.6 # or other tag to verify OR replace with the image SHA256
cosign verify ghcr.io/$REPO/$IMAGE:$TAG --certificate-oidc-issuer https://token.actions.githubusercontent.com --certificate-identity-regexp="https://github.com/$REPO" -o text
```

to the README, and duplicate it to a site page such as docs/installation/verify-signed-images.md so that it's on retina.sh also 🙂

Signed-off-by: hainenber <dotronghai96@gmail.com>
Signed-off-by: hainenber <dotronghai96@gmail.com>
@hainenber
Copy link
Contributor Author

@rbtr I've addressed Quang's comments and added relevant documentation as suggested by you. I think yours is already succinct so pardon me for a whole copy-paste :D

@rbtr rbtr requested a review from nddq April 5, 2024 20:50
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
rbtr
rbtr previously approved these changes Apr 5, 2024
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
Copy link
Collaborator

@rbtr rbtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hainenber I committed newlines at the end of your markdown files to make the linter happy, everything else LGTM

@rbtr rbtr added this pull request to the merge queue Apr 5, 2024
Merged via the queue into microsoft:main with commit ef779b6 Apr 5, 2024
21 checks passed
nddq pushed a commit that referenced this pull request May 6, 2024
# Description

Sign released container images with Cosign and Helm chart with `helm
package --sign`

## Related Issue

If this pull request is related to any issue, please mention it here.
Additionally, make sure that the issue is assigned to you before
submitting this pull request.

Closes #141 

## Checklist

- [x] I have read the [contributing
documantation](https://retina.sh/docs/contributing).
- [x] I signed and signed-off the commits (`git commit -S -s ...`)
- [x] I have correctly attributed the author(s) of the code.
- [ ] I have tested the changes locally.
- [ ] I have followed the project's style guidelines.
- [ ] I have updated the documentation, if necessary.
- [ ] I have added tests, if applicable.

## Screenshots (if applicable)

N/A

## Additional Notes

In order for testing this change, please create a GPG private key and a
Cosign private key and deposit as GH secrets for this repo

---

Please refer to the [CONTRIBUTING.md](../CONTRIBUTING.md) file for more
information on how to contribute to this project.

---------

Signed-off-by: hainenber <dotronghai96@gmail.com>
Signed-off-by: Đỗ Trọng Hải <41283691+hainenber@users.noreply.github.com>
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
Co-authored-by: Evan Baker <rbtr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/infra Test, Release, or CI Infrastructure priority/1 P1 type/enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Sign container images
3 participants