Example of verifying OCI referenced images using sigstore-go #30

rdimitrov · 2023-11-29T00:08:19Z

Summary

The following PR provides an example of verifying OCI referenced images using sigstore-go.

Details:

Uses the sigstore-go CLI as an example and adds on top a --ociImage flag
There's no need to pass any additional bundle alongside the image reference.
It builds the bundle by fetching the related image manifests and extracts and normalises the necessary data to construct it.
Added a document providing an example use of this

I realise I might have missed some corner case so I'll appreciate your feedback on how to make this better 😃

Thanks in advance!

Release Note

Example of verifying OCI referenced images using sigstore-go.

Documentation

kommendorkapten · 2023-12-01T13:04:37Z

Awesome!

docs/oci-image-verification.md

kommendorkapten

Thanks @rdimitrov , this is great! 🚀

One nit comment, otherwise this looks really good.

ping: @haydentherapper @codysoyland

kommendorkapten · 2023-12-01T14:18:12Z

To clarify, the purpose of sigstore-go is not to have functionality like this, it should be in cosign. But until cosign is ready to work with bundles, having an example in this repo is fine IMHO.

haydentherapper · 2023-12-01T15:31:09Z

Overall I’m fine adding this, but I would caution having the script be much more than an example. I don’t expect this will evolve into a more fleshed out CLI.

We will be working on adding support for the bundle in Cosign very shortly, you can track progress in sigstore/cosign#3139.

cmd/sigstore-go/main.go

haydentherapper · 2023-12-01T15:41:04Z

@rdimitrov, how did you envision the purpose of this code? I don’t want to put words in your mouth if you see this as more than an example. If so, then I’m a little hesitant to put so much in without tests, but I also don’t want this to become cosign.

what do you think about putting some caveats in the documentation that this is meant as an example?

haydentherapper · 2023-12-03T03:39:10Z

cmd/sigstore-go/main.go

+			InclusionPromise: &protorekor.InclusionPromise{
+				SignedEntryTimestamp: signedEntryTimestamp,
+			},
+			InclusionProof:    nil,


Inclusion proofs are required. If the OCI image doesn’t store it, then it can’t be used to build a valid bundle.

I was using the example of https://github.com/sigstore/sigstore-go/blob/main/examples/bundle-provenance.json as a reference, so thus why it's like that. Can you give me some pointers on how to construct the necessary data for this?

The test image (see the oci-image-verification.md) is signed with cosign, it appears cosign does not upload the inclusion proof, is that correct @haydentherapper ? If correct, thoughts of adding inclusion proofs when uploading to an OCI registry.

The code could be modified to verify it the inclusion proof is present, if not it could be fetched from Rekor prior to creating the bundle (it's present in the entries endpoint, how to view via curl: curl -s 'https://rekor.sigstore.dev/api/v1/log/entries?logIndex=54521426'.

Edit: asked a question if cosign should upload inclusion proof.

For a transition period, maybe using the bundle format v0.1 (i.e only require inclusion promise) is the best way forward, then when we modify cosign, we can start to produce v0.2 bundle versions.

The goal here is to produce a valid signature, correct (not to specifically implement the current cosign behavior)? It sounds like maybe there's also a bug report for cosign here?

You're right that we don't store the inclusion proof - https://github.com/sigstore/cosign/blob/main/pkg/cosign/bundle/rekor.go#L23-L34

Ideally we would be storing the inclusion proof too. For some history, it's because we previously weren't doing anything with inclusion proofs, it's only been recently that we're improving the offline verification from rekor.

I'd rather not make the change in Cosign since the "rekor bundle" format has been stable for some time now. Instead we should implement protobuf bundle support which will include the inclusion proof, since it'll be stored after signing.

I'd personally prefer to not have more v01 bundles without these stronger proofs and instead make the change in Cosign rather than try to translate the existing OCI contents into an old bundle format.

rdimitrov · 2023-12-03T21:30:50Z

Hey, @haydentherapper 👋 Thanks for your feedback! 🙏

@rdimitrov, how did you envision the purpose of this code? I don’t want to put words in your mouth if you see this as more than an example. If so, then I’m a little hesitant to put so much in without tests, but I also don’t want this to become cosign.

That makes a lot of sense 👍 We were talking with Fredrik and we agreed this is not intended to be something more than a proof-of-concept. My intention is to replace the use of the cosign library with something smaller like sigstore-go and so we talked if it'd be worth sharing what I've done so far as another example 👍

what do you think about putting some caveats in the documentation that this is meant as an example?

Of course, updated the doc to explicitly state this 👍

kommendorkapten · 2023-12-04T13:03:25Z

cmd/sigstore-go/main.go

+	}
+	// 4. Construct and verify the bundle
+	pb := protobundle.Bundle{
+		MediaType:            bundle.SigstoreBundleMediaType01,


we should use bundle.SigstoreBundleMediaType02, but this will make the verification to fail (as the inclusion proof is missing as @haydentherapper pointed out). See https://github.com/sigstore/sigstore-go/pull/30/files#r1412940744 for more discussion on inclusion proof.

+1, we shouldn't proliferate v01 bundles.

haydentherapper · 2023-12-06T21:16:21Z

cmd/sigstore-go/main.go

+	}
+	// 4. Construct and verify the bundle
+	pb := protobundle.Bundle{
+		MediaType:            bundle.SigstoreBundleMediaType01,


+1, we shouldn't proliferate v01 bundles.

haydentherapper · 2023-12-06T21:16:58Z

cmd/sigstore-go/main.go

+	return &protobundle.VerificationMaterial{
+		Content:                   signingCert,
+		TlogEntries:               tlogEntries,
+		TimestampVerificationData: nil,


Timestamp data is available too, stored in rfc3161timestamp

haydentherapper · 2023-12-06T21:31:43Z

cmd/sigstore-go/main.go

+			InclusionPromise: &protorekor.InclusionPromise{
+				SignedEntryTimestamp: signedEntryTimestamp,
+			},
+			InclusionProof:    nil,


You're right that we don't store the inclusion proof - https://github.com/sigstore/cosign/blob/main/pkg/cosign/bundle/rekor.go#L23-L34

Ideally we would be storing the inclusion proof too. For some history, it's because we previously weren't doing anything with inclusion proofs, it's only been recently that we're improving the offline verification from rekor.

I'd rather not make the change in Cosign since the "rekor bundle" format has been stable for some time now. Instead we should implement protobuf bundle support which will include the inclusion proof, since it'll be stored after signing.

I'd personally prefer to not have more v01 bundles without these stronger proofs and instead make the change in Cosign rather than try to translate the existing OCI contents into an old bundle format.

codysoyland · 2023-12-06T22:07:58Z

Thanks @rdimitrov for writing this up! It's interesting that the existing cosign signatures can be packaged into a compliant bundle, as that wasn't really the intent of this library. I think many of us see the future of using Sigstore Bundles in cosign would look something like uploading the bundle to an OCI registry as a referring artifact, and verifying the stored bundle directly. Of course this depends on the ability to sign/create bundles, which is as of yet unsupported in sigstore-go and cosign.

I'm a bit hesitant to merge this for a few reasons:

As mentioned above, we would like to push bundles directly into OCI registries, which is not compatible with this implementation. See secondary goals in Support the protobuf bundle format in Cosign cosign#3139
As mentioned by other reviewers, we would like discourage any more creation of v01 bundles.
I don't want to add more dependencies to support OCI/docker here, as that is more the role of cosign.
IMO, the CLI here in sigstore-go is not really something I want to support long-term, and it's intentionally small to demonstrate sigstore-go's core functionality, to use as prior art for people who are trying to use sigstore-go. I have considered moving the CLI into an examples/ directory to emphasize this.
No test coverage.

I'd be open to moving this code into an examples dir with its own go.mod, but otherwise I'd prefer to reject this PR on these bases.

kommendorkapten · 2023-12-07T08:45:32Z

Just a general comment here:

I'd personally prefer to not have more v01 bundles

I don't believe the intent with this example code is to actually store the bundles anywhere? I was always under the impression that this example code is a way for a client to verify a bundle with sigstore-go instead cosign, by creating an in-memory representation, verify it, then throw it away. In that sense, verifying a v1 bundle is as safe as verifying it with cosign.

kommendorkapten · 2023-12-07T08:47:08Z

I'd be open to moving this code into an examples dir with its own go.mod, but otherwise I'd prefer to reject this PR on these bases.

yes, this is a great option, as this is just an example client using sigstore-go 👍

rdimitrov · 2023-12-07T10:35:43Z

@kommendorkapten @haydentherapper @codysoyland - Thanks all for your feedback! 💯I appreciate it very much! 🙏

I think @kommendorkapten stated all of this correctly 👍 The bundle is used strictly as an intermediate format so I can leverage sigstore-go to verify an OCI image in runtime. I don't intend to produce and output the constructed bundle, store it somewhere, etc. This is just an example of using sigstore-go to verify an image and not intended to a tool for others to use.

I'll try to summarise what I've understood so far (correct me if I'm wrong):

The inclusionProof is necessary to allow for offline verification, but obsolete for online verification.
Required in v2, but not in v1.
Its presence will be addressed once the protobuf bundle support in cosign is implemented.
Until then, signing with cosign does not produce enough information to construct it and there are no plans to include it in cosign's rekor bundle.

Based on this:

This approach is safe to be used in the use case of doing online verifications only.
It will allow to act as an intermediate step to switch seamlessly to v2 once the protobuf bundle signing support is implemented and begins to roll out as part of the OCI image manifests.
An open question here is - once this happens, how will cosign handle verifying an image signed with the current (old) format?

I'd be open to moving this code into an examples dir with its own go.mod, but otherwise I'd prefer to reject this PR on these bases.

@codysoyland - I was debating whether I should put it there in the first place. I can rework this PR so it aligns with this 👍

Thanks again for your support! 🙏

Signed-off-by: Radoslav Dimitrov <[email protected]>

Co-authored-by: Fredrik Skogman <[email protected]> Signed-off-by: Radoslav Dimitrov <[email protected]>

Signed-off-by: Radoslav Dimitrov <[email protected]>

haydentherapper · 2023-12-07T15:33:50Z

@rdimitrov Thanks for the clarification. My concerns are largely mitigated if this is just an intermediate format and not persisted.

To Cody’s last point, do we want to add some tests even if this is an example?

docs/oci-image-verification.md

codysoyland · 2023-12-07T16:17:27Z

@codysoyland - I was debating whether I should put it there in the first place. I can rework this PR so it aligns with this 👍

Thanks @rdimitrov for understanding and moving this to an example! I just ran through the doc and everything works on my local machine. 👍🏻

Co-authored-by: Cody Soyland <[email protected]> Signed-off-by: Radoslav Dimitrov <[email protected]>

rdimitrov · 2023-12-07T16:33:39Z

Thanks @codysoyland and @haydentherapper! 🙏

Updated the PR title and description to match, let me know if there's anything else blocking this 👍

kommendorkapten force-pushed the ociimage branch from 68621d8 to 7de91ff Compare December 1, 2023 13:10

kommendorkapten reviewed Dec 1, 2023

View reviewed changes

docs/oci-image-verification.md Outdated Show resolved Hide resolved

kommendorkapten previously approved these changes Dec 1, 2023

View reviewed changes

haydentherapper requested changes Dec 1, 2023

View reviewed changes

cmd/sigstore-go/main.go Outdated Show resolved Hide resolved

haydentherapper reviewed Dec 3, 2023

View reviewed changes

rdimitrov dismissed kommendorkapten’s stale review via 5bb35d5 December 3, 2023 16:53

kommendorkapten reviewed Dec 4, 2023

View reviewed changes

evankanderson mentioned this pull request Dec 4, 2023

signing auto-generated SBOM using cosign ko-build/ko#603

Open

rdimitrov force-pushed the ociimage branch from a31fa6f to f12ffa4 Compare December 6, 2023 01:16

haydentherapper reviewed Dec 6, 2023

View reviewed changes

rdimitrov and others added 7 commits December 7, 2023 14:03

Add a doc for verifying OCI images with sigstore-go

cfa77c4

Signed-off-by: Radoslav Dimitrov <[email protected]>

Add oci image verification support to sigstore-go

b12f300

Signed-off-by: Radoslav Dimitrov <[email protected]>

Ignore conformance binary and DS_Store

6c77bcd

Signed-off-by: Radoslav Dimitrov <[email protected]>

Fix linting error

715514e

Signed-off-by: Radoslav Dimitrov <[email protected]>

Update docs/oci-image-verification.md

6165afa

Co-authored-by: Fredrik Skogman <[email protected]> Signed-off-by: Radoslav Dimitrov <[email protected]>

State explicitly that this is just an example use of sigstore-go

687d9a8

Signed-off-by: Radoslav Dimitrov <[email protected]>

Move the image example to examples/oci-image-verification

7e36895

Signed-off-by: Radoslav Dimitrov <[email protected]>

rdimitrov force-pushed the ociimage branch from f12ffa4 to 7e36895 Compare December 7, 2023 12:38

codysoyland reviewed Dec 7, 2023

View reviewed changes

docs/oci-image-verification.md Outdated Show resolved Hide resolved

Update docs/oci-image-verification.md

78a7298

Co-authored-by: Cody Soyland <[email protected]> Signed-off-by: Radoslav Dimitrov <[email protected]>

rdimitrov changed the title ~~Support for verifying OCI referenced images~~ Example of verifying OCI referenced images using sigstore-go Dec 7, 2023

codysoyland approved these changes Dec 7, 2023

View reviewed changes

haydentherapper approved these changes Dec 7, 2023

View reviewed changes

codysoyland merged commit 6a8bf18 into sigstore:main Dec 7, 2023
8 checks passed

rdimitrov deleted the ociimage branch December 7, 2023 20:14

kommendorkapten mentioned this pull request Dec 19, 2023

Unifying timestamp verification across SETs and TSA signed timestamps #44

Closed

rdimitrov mentioned this pull request Jan 11, 2024

Bump sigstore-go version mindersec/minder#2106

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Example of verifying OCI referenced images using sigstore-go #30

Example of verifying OCI referenced images using sigstore-go #30

rdimitrov commented Nov 29, 2023 •

edited

Loading

kommendorkapten commented Dec 1, 2023

kommendorkapten left a comment

kommendorkapten commented Dec 1, 2023

haydentherapper commented Dec 1, 2023

haydentherapper commented Dec 1, 2023

haydentherapper Dec 3, 2023

rdimitrov Dec 3, 2023

kommendorkapten Dec 4, 2023 •

edited

Loading

kommendorkapten Dec 4, 2023

evankanderson Dec 4, 2023

haydentherapper Dec 6, 2023

rdimitrov commented Dec 3, 2023

kommendorkapten Dec 4, 2023

haydentherapper Dec 6, 2023

haydentherapper Dec 6, 2023

haydentherapper Dec 6, 2023

haydentherapper Dec 6, 2023

codysoyland commented Dec 6, 2023 •

edited

Loading

kommendorkapten commented Dec 7, 2023

kommendorkapten commented Dec 7, 2023

rdimitrov commented Dec 7, 2023

haydentherapper commented Dec 7, 2023

codysoyland commented Dec 7, 2023

rdimitrov commented Dec 7, 2023

Example of verifying OCI referenced images using sigstore-go #30

Example of verifying OCI referenced images using sigstore-go #30

Conversation

rdimitrov commented Nov 29, 2023 • edited Loading

Summary

Release Note

Documentation

kommendorkapten commented Dec 1, 2023

kommendorkapten left a comment

Choose a reason for hiding this comment

kommendorkapten commented Dec 1, 2023

haydentherapper commented Dec 1, 2023

haydentherapper commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kommendorkapten Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdimitrov commented Dec 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codysoyland commented Dec 6, 2023 • edited Loading

kommendorkapten commented Dec 7, 2023

kommendorkapten commented Dec 7, 2023

rdimitrov commented Dec 7, 2023

haydentherapper commented Dec 7, 2023

codysoyland commented Dec 7, 2023

rdimitrov commented Dec 7, 2023

rdimitrov commented Nov 29, 2023 •

edited

Loading

kommendorkapten Dec 4, 2023 •

edited

Loading

codysoyland commented Dec 6, 2023 •

edited

Loading