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

WIP: enable signing with cosign #999

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

imjasonh
Copy link

@imjasonh imjasonh commented Nov 9, 2023

Opening this as draft to gather early feedback. If this is an acceptable change in general I'd love to discuss improvements we could make to it. Everything about this change is open to discussion, and I'm happy to make any change necessary.

This change adds a sign: true (name TBD) to the action, to let folks sign images that were just built-and-pushed with cosign, a popular container image signing solution, part of Sigstore.

When sign: true (and push: true, which requires tags: to be specified), after building and pushing any images, the cosign CLI will be invoked to sign the images. It's the callers responsibility to install cosign prior to building, like it's their responsibility to setup buildx. Images are signed by digest, to avoid race conditions.

If an ID token representing the GitHub Actions identity is not available, signing will fail. This can happen for a few reasons. Typically this means the workflow doesn't have the id-token: write permission.

While building and testing this workflow I've built and signed a few images, which appear in Sigstore's Rekor transparency log: https://search.sigstore.dev/?logIndex=48727071

It's not my intention with this change to support signing with keys -- I'm only interested in supporting keyless signing using the GitHub Actions identity via OIDC.

Lots more info about GitHub Actions OIDC support here: https://docs.github.com/en/actions/deployment/security-hardening-your-deployments/about-security-hardening-with-openid-connect

TODOs:

  • move tool interactions to toolkit in docker/actions-toolkit (if you prefer)
  • sign only the built digest, not each tag (which is unnecessary)
  • sign recursively with --recursive
  • rename input to cosign: true
  • check for the existence of the ID token before attempting to sign, and provide a better error experience.
  • let users specify their own signature annotations -- by default, annotations include the GH workflow run ID, run attempt, etc.

Signed-off-by: Jason Hall <jason@chainguard.dev>
@favonia
Copy link
Contributor

favonia commented Nov 14, 2023

@imjasonh I'm puzzled by a few perspectives of this PR and maybe you could help me understand:

  1. AFAIK, there's no need to run cosign for each tag. An analogy in POSIX file systems is that tags are symbolic links and cosign works with file content.
  2. For multi-arch signatures, maybe --recursive is needed.
  3. I'm not completely convinced by the usefulness of run_id without run_attempt, and maybe you plan to add both. But then, there's a question about whether we should include all the numbers to uniquely identify things. Note that workflow_sha is by default part of the signature, so the question is whether we should distinguish re-runs.

Due to the first point (sign once for each digest), I actually do not feel the current PR will be extremely useful, because the same thing can be achieved with this line and it doesn't seem very complicated:

cosign sign --recursive --yes "NAME@${{ steps.BUILD_STEP_NAME.outputs.digest }}"

However, I'm certainly not against it if it will be actively maintained from now on. 🙂

Note: cosign still have many experimental flags about OICD, for example maybe we should add --oidc-provider github to limit cosign's auto detection, but that flag is still experimental.

@favonia
Copy link
Contributor

favonia commented Nov 14, 2023

If this feature is to be added, as a user, I want the flag name to be cosign instead of sign. I think cosign: true is short, self-explanatory, and compatible with any future signing services we might want to add. (To clarify, I still have doubts about this flag due to the points I mentioned above.)

@imjasonh
Copy link
Author

@imjasonh I'm puzzled by a few perspectives of this PR and maybe you could help me understand:

Thanks for your feedback! This is exactly the kind of response I was hoping to get from this draft PR.

  1. AFAIK, there's no need to run cosign for each tag. An analogy in POSIX file systems is that tags are symbolic links and cosign works with file content.

Excellent point! I'll definitely change this to sign the digest instead.

  1. For multi-arch signatures, maybe --recursive is needed.

Another good call-out. I'll add this to the PR's TODOs in case there's interest in adding this at all.

  1. I'm not completely convinced by the usefulness of run_id without run_attempt, and maybe you plan to add both. But then, there's a question about whether we should include all the numbers to uniquely identify things. Note that workflow_sha is by default part of the signature, so the question is whether we should distinguish re-runs.

Good point. We should definitely annotate the signature with both run_id and run_attempt, I'll note that in the TODOs.

Due to the first point (sign once for each digest), I actually do not feel the current PR will be extremely useful, because the same thing can be achieved with this line and it doesn't seem very complicated:

cosign sign --recursive --yes "NAME@${{ steps.BUILD_STEP_NAME.outputs.digest }}"

However, I'm certainly not against it if it will be actively maintained from now on. 🙂

That's my main interest in doing this, to have it an optional part of the "official" workflow for building and pushing, instead of making everyone do it themselves. Anybody who wants to can also docker buildx build --push themselves, but having it wrapped in an official action makes it harder to do the wrong thing. "Paving the cow paths" in a way.

@kipz
Copy link

kipz commented Nov 15, 2023

I don't think it makes sense for different signing solutions to be plugged in at the GitHub Action level.

I would have thought that in the first instance, we should see if we can make signing pluggable as part of build-kit, then configuring the desired signing solution in this Action would be straight-forward.

@imjasonh
Copy link
Author

I don't think it makes sense for different signing solutions to be plugged in at the GitHub Action level.

I would have thought that in the first instance, we should see if we can make signing pluggable as part of build-kit, then configuring the desired signing solution in this Action would be straight-forward.

That's a reasonable point. If that's how this ends up being possible I'd be more than happy to contribute a cosign plugin at the buildkit layer instead (or someone else from the @sigstore community can, I'm sure)

It'd be great to hear more about the plan for general signing solutions, so we can decide whether it goes in this action or elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants