Feature: Allow cosign to sign digests before they are uploaded. #2959

mattmoor · 2023-05-07T18:32:02Z

🎁 This feature allows cosign to sign a digest that doesn't exist yet, to support scenarios such as signing an image prior to pushing it.

This adapts the ideas from the two prior approaches, which were closed by stale-bot.

Fixes: #1905

/kind feature

Release Note

cosign can now be used to sign digests before the image has been pushed to a registry (this only supports shallow signing of digests, so tag-based signing and recursive signing are not supported because both need the image structure available)

Documentation

codecov · 2023-05-07T18:36:04Z

Codecov Report

Merging #2959 (1968ec2) into main (3121a17) will increase coverage by 0.54%.
The diff coverage is 81.31%.

@@            Coverage Diff             @@
##             main    #2959      +/-   ##
==========================================
+ Coverage   30.26%   30.81%   +0.54%     
==========================================
  Files         151      152       +1     
  Lines        9469     9552      +83     
==========================================
+ Hits         2866     2943      +77     
- Misses       6158     6163       +5     
- Partials      445      446       +1

Impacted Files	Coverage Δ
cmd/cosign/cli/attest/attest.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/sign/sign.go	`14.56% <0.00%> (-0.08%)`	⬇️
pkg/oci/remote/remote.go	`38.25% <0.00%> (ø)`
pkg/oci/mutate/mutate.go	`83.82% <83.33%> (+2.54%)`	⬆️
pkg/oci/remote/unknown.go	`100.00% <100.00%> (ø)`

hectorj2f

lgtm

mattmoor · 2023-05-07T18:52:34Z

I changed it to a hardcoded digest since the macos leg doesn't have sha256sum.

mattmoor · 2023-05-07T19:00:03Z

I'm going to fix cosign attest too while I'm in here, since it has the same problem.

mattmoor · 2023-05-08T02:05:44Z

Ok, so the CI failure is a legit failure, and I'm glad it caught this because there's a subtle bug with this approach!

Essentially the ociempty approach is subtly flawed in that it doesn't allow the signing/attesting logic to access pre-existing signatures and attestations, so sign and attest always overwrite the existing signatures instead of augmenting them.

I think this will require some changes more integrated into ociremote.SignedEntity itself, which needs some thought.

mattmoor · 2023-05-08T02:21:27Z

Ok, this no longer uses ociempty and I introduced an ociremote.SignedUnknown that we can explicitly fallback on (e.g. when we see the new ErrEntityNotFound) to support these cases.

It felt wrong to automatically do this in SignedEntity since it would break existing walks that assume a SignedEntity must be one of oci.Signed{Entity,Image}, but I actually like this a lot more than the prior approach because this is digestable (we know the digest), but it doesn't pretend to be a v1.Image (or index).

mattmoor · 2023-05-08T02:22:57Z

Blah, it still needs some work to pass tests.

🎁 This feature allows `cosign` to sign and attest a digest that doesn't exist yet, to support scenarios such as signing an image prior to pushing it. This adapts the ideas from the two prior approaches, which were closed by stale-bot. Fixes: sigstore#1905 /kind feature Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

mattmoor · 2023-05-08T14:23:44Z

This is RFAL!

imjasonh · 2023-05-08T15:41:44Z

test/e2e_test_secrets.sh

+  "foo": "bar"
+}
+EOF
+./cosign attest --key ${signing_key} --type custom --predicate "${PREDICATE_FILE}" "$img3@sha256:17b14220441083f55dfa21e1deb3720457d3c2d571219801d629b43c53b99627"


Can you attest something else and check that both are present, to guard against the overwriting behavior you discovered?

There's already a Go test that checks exactly that (which is what caught the bug!), and I'm not sure doing it for the "unknown" case (or in bash) adds much value 😅

jonjohnsonjr · 2023-05-08T16:29:11Z

cmd/cosign/cli/attest/attest.go

-	}
+	// We don't actually need to access the remote entity to attach things to it
+	// so we use a placeholder here.
+	se := ociremote.SignedUnknown(digest)


WDYT about s/Unknown/Digest/ everywhere? Unknown doesn't feel quite right to me.

I worry that Digest isn't clear enough, since it is effectively what SignedEntity is (especially after my PR to add Digest to oci.SignedEntity).

That said, I don't feel particularly strongly about the naming here :bikeshed:

pkg/oci/remote/unknown.go

pkg/oci/remote/unknown_test.go

jonjohnsonjr · 2023-05-08T16:34:35Z

cmd/cosign/cli/sign/sign.go

@@ -179,7 +179,9 @@ func SignCmd(ro *options.RootOptions, ko options.KeyOpts, signOpts options.SignO

 		if digest, ok := ref.(name.Digest); ok && !signOpts.Recursive {
 			se, err := ociremote.SignedEntity(ref, opts...)
-			if err != nil {
+			if err == ociremote.ErrEntityNotFound {
+				se = ociremote.SignedUnknown(digest)


Are you sure we always want to do this on 404?

I think yes, but do you have an alternative in mind?

Co-authored-by: Jon Johnson <jonjohnsonjr@gmail.com> Signed-off-by: Matt Moore <mattomata@gmail.com>

imjasonh · 2023-05-21T14:55:26Z

Ping. I think this looks good (I have no power to approve), and I'm not sure any of the remaining comments are blocking. I don't want to lose momentum on this long-standing feature request. 🤞

cpanato

This lgtm
Will wait for @jonjohnsonjr because made some comments here

jonjohnsonjr · 2023-05-23T17:11:29Z

This is not great because OCI referrers stuff expects a whole descriptor (digest+size+mediaType). It would be better if this took a Descriptor, assuming we want to adopt the OCI referrers stuff down the road.

imjasonh · 2023-05-23T17:22:22Z

This is not great because OCI referrers stuff expects a whole descriptor (digest+size+mediaType). It would be better if this took a Descriptor, assuming we want to adopt the OCI referrers stuff down the road.

Good point. However, in that case, I think we can just set the subject without a size and maybe with some default mediaType, to denote "I'm attaching to something I don't know anything else about".

"subject": {
  "digest": "sha256:abcdef...",
  "size": 0,
  "mediaType": "application/octet-stream"
}

We can either establish this convention and use it, or try to specify this better in OCI image-spec. Supporting this case has been a goal of OCI referrers, and if implementers like cosign don't know how to achieve it then OCI should help them. Or if they want to solve it with this convention, that works too, we just need registries not to validate that the size+mediaType matches reality.

mattmoor · 2023-05-23T17:31:33Z

This is not great because OCI referrers stuff expects a whole descriptor (digest+size+mediaType). It would be better if this took a Descriptor, assuming we want to adopt the OCI referrers stuff down the road.

Digests need to be fully self-describing, the absence of this property was (as you know) a CVE.

jonjohnsonjr · 2023-05-23T18:22:59Z

Digests need to be fully self-describing, the absence of this property was (as you know) a CVE.

Sure, but it's nice to know the size prior to decoding the payload, which you can do once you have an entrypoint into the dag. If you want to go against the grain, that's fine, but I'd file an issue with OCI.

imjasonh · 2023-06-02T13:42:50Z

Ping. I think this looks good (I have no power to approve), and I'm not sure any of the remaining comments are blocking. I don't want to lose momentum on this long-standing feature request. 🤞

priyawadhwa · 2023-06-02T17:37:35Z

I'll go ahead and merge since it seems like next steps are to file an issue with OCI (sorry if I'm reading this thread wrong!).

mattmoor requested review from imjasonh and priyawadhwa May 7, 2023 18:32

mattmoor force-pushed the sign-without-accessing branch from 8059c2f to cd988ee Compare May 7, 2023 18:32

hectorj2f previously approved these changes May 7, 2023

View reviewed changes

mattmoor dismissed hectorj2f’s stale review via d6d95ed May 7, 2023 18:47

mattmoor force-pushed the sign-without-accessing branch from cd988ee to d6d95ed Compare May 7, 2023 18:47

mattmoor force-pushed the sign-without-accessing branch from d6d95ed to 885c5a7 Compare May 7, 2023 19:01

mattmoor force-pushed the sign-without-accessing branch from 885c5a7 to 8a77fc8 Compare May 8, 2023 02:17

mattmoor force-pushed the sign-without-accessing branch from 8a77fc8 to 2122ce1 Compare May 8, 2023 02:36

mattmoor force-pushed the sign-without-accessing branch from 2122ce1 to 0bd02a0 Compare May 8, 2023 02:59

mattmoor requested a review from hectorj2f May 8, 2023 14:23

hectorj2f previously approved these changes May 8, 2023

View reviewed changes

imjasonh reviewed May 8, 2023

View reviewed changes

jonjohnsonjr reviewed May 8, 2023

View reviewed changes

Apply suggestions from code review

1968ec2

Co-authored-by: Jon Johnson <jonjohnsonjr@gmail.com> Signed-off-by: Matt Moore <mattomata@gmail.com>

mattmoor dismissed hectorj2f’s stale review via 1968ec2 May 9, 2023 01:51

mattmoor requested review from jonjohnsonjr and imjasonh May 9, 2023 02:24

cpanato approved these changes May 21, 2023

View reviewed changes

priyawadhwa merged commit f9ee498 into sigstore:main Jun 2, 2023

github-actions bot added this to the v1.14.0 milestone Jun 2, 2023

wlynch mentioned this pull request Jun 14, 2023

Unable to sign image with kaniko when using --no-push arg tektoncd/chains#803

Open

gl172 mentioned this pull request Jul 17, 2023

Sign images before upload to registry buildpacks-community/kpack#1266

Closed

This was referenced Sep 9, 2023

fix: update error in SignedEntity to be more descriptive #3233

Merged

fix: allow cosign download sbom when image is absent #3245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Allow cosign to sign digests before they are uploaded. #2959

Feature: Allow cosign to sign digests before they are uploaded. #2959

mattmoor commented May 7, 2023

codecov bot commented May 7, 2023 •

edited

Loading

hectorj2f left a comment

mattmoor commented May 7, 2023

mattmoor commented May 7, 2023

mattmoor commented May 8, 2023

mattmoor commented May 8, 2023

mattmoor commented May 8, 2023

mattmoor commented May 8, 2023

imjasonh May 8, 2023

mattmoor May 8, 2023

jonjohnsonjr May 8, 2023

mattmoor May 8, 2023

jonjohnsonjr May 8, 2023

mattmoor May 8, 2023

imjasonh commented May 21, 2023

cpanato left a comment

jonjohnsonjr commented May 23, 2023

imjasonh commented May 23, 2023

mattmoor commented May 23, 2023

jonjohnsonjr commented May 23, 2023

imjasonh commented Jun 2, 2023

priyawadhwa commented Jun 2, 2023

Feature: Allow cosign to sign digests before they are uploaded. #2959

Feature: Allow cosign to sign digests before they are uploaded. #2959

Conversation

mattmoor commented May 7, 2023

Release Note

Documentation

codecov bot commented May 7, 2023 • edited Loading

Codecov Report

hectorj2f left a comment

Choose a reason for hiding this comment

mattmoor commented May 7, 2023

mattmoor commented May 7, 2023

mattmoor commented May 8, 2023

mattmoor commented May 8, 2023

mattmoor commented May 8, 2023

mattmoor commented May 8, 2023

imjasonh May 8, 2023

Choose a reason for hiding this comment

mattmoor May 8, 2023

Choose a reason for hiding this comment

jonjohnsonjr May 8, 2023

Choose a reason for hiding this comment

mattmoor May 8, 2023

Choose a reason for hiding this comment

jonjohnsonjr May 8, 2023

Choose a reason for hiding this comment

mattmoor May 8, 2023

Choose a reason for hiding this comment

imjasonh commented May 21, 2023

cpanato left a comment

Choose a reason for hiding this comment

jonjohnsonjr commented May 23, 2023

imjasonh commented May 23, 2023

mattmoor commented May 23, 2023

jonjohnsonjr commented May 23, 2023

imjasonh commented Jun 2, 2023

priyawadhwa commented Jun 2, 2023

codecov bot commented May 7, 2023 •

edited

Loading