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

Add sign --sign-container-identity CLI #2984

Merged
merged 1 commit into from Jun 5, 2023

Conversation

saschagrunert
Copy link
Contributor

@saschagrunert saschagrunert commented May 16, 2023

Summary

The --sign-container-identity flag allows setting the critical.identity.docker-reference field in the signature to a different container image reference. This is particularly useful when using proxy mirrors like registry.k8s.io, where end-users have no chance to actually assume the underlying registry. This change allows signing images using the mirror/proxy identifier, while validation can then happen without requiring any additional remapping.

Refers to containers/image#1952, sigstore/sigstore#1166

Release Note

Added sign --sign-identity CLI flag.

Documentation

TBD.

@codecov
Copy link

codecov bot commented May 16, 2023

Codecov Report

Merging #2984 (5aac8e4) into main (83c08ce) will increase coverage by 0.51%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2984      +/-   ##
==========================================
+ Coverage   30.45%   30.96%   +0.51%     
==========================================
  Files         152      153       +1     
  Lines        9586     9676      +90     
==========================================
+ Hits         2919     2996      +77     
- Misses       6210     6222      +12     
- Partials      457      458       +1     
Impacted Files Coverage Δ
cmd/cosign/cli/options/sign.go 0.00% <0.00%> (ø)
cmd/cosign/cli/sign.go 0.00% <0.00%> (ø)
cmd/cosign/cli/sign/sign.go 14.52% <0.00%> (-0.11%) ⬇️

... and 4 files with indirect coverage changes

Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

I strongly support this feature; it is necessary not only for proxies, but also for many staging workflows (creating an image signed with the production key, with a production identity, but not publishing it yet and storing it in an internal registry).

@@ -108,4 +109,7 @@ func (o *SignOptions) AddFlags(cmd *cobra.Command) {

cmd.Flags().BoolVar(&o.IssueCertificate, "issue-certificate", false,
"issue a code signing certificate from Fulcio, even if a key is provided")

cmd.Flags().StringVar(&o.DockerReference, "docker-reference", "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Skopeo names this parameter --sign-identity, and I think that captures the intent a bit more accurately; both in being more explicit about what the field means, and in not referring to a specific implementation. (The “simple signing” JSON payload field name should be viewed as a historical artifact.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also happy with --sign-identity, what do the other maintainers think?

Copy link
Contributor

Choose a reason for hiding this comment

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

It’s a little confusing to reuse identity, since we use identity already to refer to who is doing the signing. Maybe sign-container-identity?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @haydentherapper here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to --sign-container-identity naming.

cpanato
cpanato previously approved these changes May 19, 2023
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

lgtm

we just need to remove the replace when the PR from sigstore/sigstore is merged

@cpanato cpanato requested a review from znewman01 May 19, 2023 09:07
@saschagrunert saschagrunert force-pushed the docker-reference branch 2 times, most recently from 06e3f22 to d6db445 Compare May 22, 2023 08:11
@saschagrunert saschagrunert changed the title RFC: Add sign --docker-reference CLI RFC: Add sign --sign-identity CLI May 22, 2023
@saschagrunert saschagrunert changed the title RFC: Add sign --sign-identity CLI WIP: Add sign --sign-identity CLI May 22, 2023
Copy link
Contributor

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for the change.

I see there's a little bit of worry about the flag name. I'm empathetic: it's a little weird to say the image has an identity, especially when there's also a signer identity in play.

I don't want to bikeshed too much further, but maybe the name is a little less important if we can make the help text a bit more explicit and to add an example to the cosign sign docs.

My way too wordy version, written to check my own understanding here, would be:

cosign sign uses the "simple signing" format for signatures, which links an image with a given digest to a docker-reference: basically, "image sha256@... is alpine:latest." By default, we infer the docker reference based on what you sign; this flag allows you to override that (for instance, to specify a tag instead of a digest, or when using a proxy).

Maybe something like:

--sign-identity: claim that the image being signed has the given identity (instead of inferring this from the provided image identifier): sign my-registry.io/foo@sha256:... as my-registry.io/foo:v1 or even other-registry.io/bar:latest.

And then add an example like:

# sign a container image, identifying it by tag
cosign sign --sign-identity registry.io/foo:latest my-proxy.io/foo@sha256:abcdef 

# sign a container image fetched via a proxy, identifying it by the upstream Docker reference.
cosign sign --sign-identity upstream-registry.io/foo:latest my-proxy.io/foo@sha256:abcdef 

Feel free to revise any of the above. But I think if the docs are really clear we can live with an imperfect name.

(This suggests that maybe we eventually want some sugar we'd want for using a tag as the --sign-identity: maybe --sign-identity :tag. But we can deal with that later).

@mtrmac
Copy link
Contributor

mtrmac commented May 31, 2023

All of the above makes sense to me.

For a bit more context to consider, separately from his PR, I’d afterwards like to replace/discourage the currently-recommended cosign sign repo@sha256:digest command, because the signature does not include a tag, and matching the tag is an important part of protecting the user against registries/proxies etc. substituting images (giving the user a correctly signed, but old known-vulnerable application version when the user asks for a recent version).

I’m a bit struggling with a good UI… one alternative is to make the --sign-identity option strongly recommended in all flows:

$ cosign sign --sign-identity registry.io/foo:latest my-proxy.io/foo@sha256:abcdef 

My preference would be, instead, to add yet another option:

$ cosign sign --digest sha256:abcdef registry.io/foo:latest

because that can be built “naturally”: a naive user says cosign sign …foo:latest, gets a warning/error “please pin what you want to sign, using cosign sign --digest … foo:latest”, and the user just adds one more option; vs. recommending the user to use --sign-identity, which would require reformulating more of the command line.

That would be an alternative for adding the --sign-identity :tag sugar; --sign-identity would almost exclusively be used when the registry/repo part differs, making the short-form unnecessary.

@znewman01
Copy link
Contributor

All SGTM. I think this PR represents a great first step in that direction.

@hectorj2f
Copy link
Contributor

I also find confusing a lot this flag --sign-identity. In a first look, I am thinking we are specifying the identity that signed the image instead of what is been discussed here. Could we choose a better or more descriptive flag sign-identity-docker-ref or similar ?

@saschagrunert saschagrunert changed the title WIP: Add sign --sign-identity CLI Add sign --sign-identity CLI Jun 2, 2023
@saschagrunert saschagrunert changed the title Add sign --sign-identity CLI Add sign --sign-identity CLI Jun 2, 2023
@saschagrunert saschagrunert changed the title Add sign --sign-identity CLI Add sign --sign-container-identity CLI Jun 2, 2023
@saschagrunert
Copy link
Contributor Author

Switched to --sign-container-identity for naming, we may want to wait for a sigstore/sigstore release as well.

Copy link
Contributor

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

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

I think I'm happy modulo the request for additional examples in the docs (and maybe a slightly longer flag description)!

The `--sign-container-identity` flag allows setting the
`critical.identity.docker-reference` field in the signature to a
different container image reference. This is particularly useful when
using proxy mirrors like `registry.k8s.io`, where end-users have no
chance to actually assume the underlying registry. This change allows
signing images using the mirror/proxy identifier, while validation can
then happen without requiring any additional remapping.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@znewman01 znewman01 merged commit 255ac31 into sigstore:main Jun 5, 2023
26 checks passed
@github-actions github-actions bot added this to the v1.14.0 milestone Jun 5, 2023
@saschagrunert saschagrunert deleted the docker-reference branch June 6, 2023 07:27
saschagrunert added a commit to saschagrunert/release-sdk that referenced this pull request Jun 15, 2023
After the merge of sigstore/cosign#2984,
we now can use the new option to support it within the release-sdk as
well.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
saschagrunert added a commit to saschagrunert/release-sdk that referenced this pull request Jun 15, 2023
After the merge of sigstore/cosign#2984,
we now can use the new option to support it within the release-sdk as
well.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
saschagrunert added a commit to saschagrunert/release-sdk that referenced this pull request Jun 19, 2023
After the merge of sigstore/cosign#2984,
we now can use the new option to support it within the release-sdk as
well.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
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

6 participants