Open imjasonh opened 8 months ago
@imjasonh I'm puzzled by a few perspectives of this PR and maybe you could help me understand:
cosign
for each tag. An analogy in POSIX file systems is that tags are symbolic links and cosign works with file content.--recursive
is needed.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.
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 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.
- 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.
- 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.
- I'm not completely convinced by the usefulness of
run_id
withoutrun_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 thatworkflow_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.
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.
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.
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 withcosign
, a popular container image signing solution, part of Sigstore.When
sign: true
(andpush: true
, which requirestags:
to be specified), after building and pushing any images, thecosign
CLI will be invoked to sign the images. It's the callers responsibility to installcosign
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:
toolkit
in docker/actions-toolkit (if you prefer)--recursive
cosign: true