build-push-action icon indicating copy to clipboard operation
build-push-action copied to clipboard

WIP: enable signing with cosign

Open imjasonh opened this issue 2 years ago • 5 comments

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.

imjasonh avatar Nov 09 '23 19:11 imjasonh

@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 avatar Nov 14 '23 14:11 favonia

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.)

favonia avatar Nov 14 '23 14:11 favonia

@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.

imjasonh avatar Nov 14 '23 15:11 imjasonh

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.

kipz avatar Nov 15 '23 10:11 kipz

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.

imjasonh avatar Dec 13 '23 14:12 imjasonh