Define a new sigstore Bundle type to include additional verification data and materials #2422

hectorj2f · 2022-11-08T10:44:58Z

Summary

As part of this effort #2331, I'm proposing renaming the original RekorBundle type to a more generic sigstore Bundle type that includes additional data. The new fields added to the sigstore Bundle are inspired from the protobuf specification here: https://github.com/sigstore/protobuf-specs/blob/main/protos/sigstore_bundle.proto.

The next actions include adding support to use a Timestamp Authority server (sigstore/timestamp-authority) to guarantee a signature happened during the cert's validity period.

Release Note

Documentation

Signed-off-by: Hector Fernandez <hector@chainguard.dev>

hectorj2f · 2022-11-08T10:46:55Z

pkg/cosign/bundle/bundle.go

+	// RekorPayload holds metadata about recording a Signature's ephemeral key to
+	// a Rekor transparency log.
+	// Use Payload instead of TransparencyLogEntry to keep backwards compatibility.
+	Payload RekorPayload


I kept the Payload field instead of renaming it to TransparencyLogEntry, as specified in the protobuf spec, to keep backwards compatibility.

hectorj2f · 2022-11-08T10:48:45Z

pkg/oci/static/options_test.go

@@ -87,7 +87,7 @@ func TestOptions(t *testing.T) {
 			LayerMediaType:  ctypes.SimpleSigningMediaType,
 			ConfigMediaType: types.OCIConfigJSON,
 			Annotations: map[string]string{
-				BundleAnnotationKey: "{\"SignedEntryTimestamp\":null,\"Payload\":{\"body\":null,\"integratedTime\":0,\"logIndex\":0,\"logID\":\"\"}}",
+				BundleAnnotationKey: "{\"Payload\":{\"body\":null,\"integratedTime\":0,\"logIndex\":0,\"logID\":\"\"},\"SignedEntryTimestamp\":null,\"EntryTimestampAuthority\":null,\"CertBytes\":null,\"PublicKeyIdentifier\":\"\"}",


This is the main difference with the new bundle type. Whenever we start using a TSA the bundle will look more like:

"Bundle":{"Payload":{"body":null,"integratedTime":0,"logIndex":0,"logID":""},"SignedEntryTimestamp":null,"EntryTimestampAuthority":"MIIEbjADAgEAMIIEZQYJKoZIhvcNAQcCoIIEVjCCBFICAQExDTALBglghkgBZQMEAgEwgdQGCyqGSIb3DQEJEAEEoIHEBIHBMIG+AgEBBgkrBgEEAYO/MAIwMTANBglghkgBZQMEAgEFAAQgLFsLP5xZKtHz5LM1jdA+K9GQFwI20N5BnnKOOgsPpvcCFQDKNZaXbkFPJh0AD1YsJ27y+SllNxgTMjAyMjExMDcxNzEzNDQrMDEwMDADAgEBAhReYQUVtj8oPw/AXX8xVrnIZfq6Y6A0pDIwMDEOMAwGA1UEChMFbG9jYWwxHjAcBgNVBAMTFVRlc3QgVFNBIFRpbWVzdGFtcGluZ6CCAckwggHFMIIBaqADAgECAhRHCu9dHKS97mFo1cH5neJubRibujAKBggqhkjOPQQDAjAoMQ4wDAYDVQQKEwVsb2NhbDEWMBQGA1UEAxMNVGVzdCBUU0EgUm9vdDAeFw0yMjExMDMxMTUzMThaFw0zMTExMDMxMTU2MThaMDAxDjAMBgNVBAoTBWxvY2FsMR4wHAYDVQQDExVUZXN0IFRTQSBUaW1lc3RhbXBpbmcwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAATo3W6NQrpx5D8z5IvgD2DlAgoJMF4KPY9Pj4UfFhfOq029ryszXp3460Z7N+x86bDvyjVrHaeiPnl1HO9Q52zso2owaDAOBgNVHQ8BAf8EBAMCB4AwHQYDVR0OBBYEFHSIhDdTGIsodML/iUOhx7hgo/K7MB8GA1UdIwQYMBaAFBoZYijuouZCvKDtBd0eCyaU2HWoMBYGA1UdJQEB/wQMMAoGCCsGAQUFBwMIMAoGCCqGSM49BAMCA0kAMEYCIQCmPVr5kwYe4Jg9PGO6apgfzSrKAtESgNHpAbE3iIvJhQIhAJIGNxshJcC8LXHRrVWM77no3d3GguSvR01OAPZwE2pqMYIBmDCCAZQCAQEwQDAoMQ4wDAYDVQQKEwVsb2NhbDEWMBQGA1UEAxMNVGVzdCBUU0EgUm9vdAIURwrvXRykve5haNXB+Z3ibm0Ym7owCwYJYIZIAWUDBAIBoIHqMBoGCSqGSIb3DQEJAzENBgsqhkiG9w0BCRABBDAcBgkqhkiG9w0BCQUxDxcNMjIxMTA3MTYxMzQ0WjAvBgkqhkiG9w0BCQQxIgQgGQ4kDSvQ8UJgZaIIKWZg9R0L06St8OBAYrj2VGSq7rcwfQYLKoZIhvcNAQkQAi8xbjBsMGowaAQgXqxJD0nAgg6en9P1bRrU7+6tzxOMn3YThreg7uR6T7EwRDAspCowKDEOMAwGA1UEChMFbG9jYWwxFjAUBgNVBAMTDVRlc3QgVFNBIFJvb3QCFEcK710cpL3uYWjVwfmd4m5tGJu6MAoGCCqGSM49BAMCBEcwRQIhALiLTG7glyyiiHJ33DsWIi24rHpcSnW/EeLfNqaXty4VAiBG7moX2xoOwOfJm8DZveTox9tpcV6NnIN/3LzGs8eSgA==","CertBytes":null, "PublicKeyIdentifier": ""}

hectorj2f · 2022-11-08T12:57:53Z

pkg/cosign/bundle/bundle.go

+	// a Rekor transparency log.
+	// Use Payload instead of TransparencyLogEntry to keep backwards compatibility.
+	Payload RekorPayload
+


I am awaiting feedback before adding other fields such as the messageSignature included in the spec.

Those fields overlap with LocalSignedPayload too. The top level fields in that would ideally be removed, or else it would open mismatch issues.

Good point! That could easily happen.

hectorj2f · 2022-11-08T12:59:55Z

@asraa @priyawadhwa @haydentherapper I'd like to hear your thoughts about this proposal 👀 . I decided to adapt the current implementation to the fields in the protobuf spec. Also, I included a new field EntryTimestampAuthority that will contains the response as received from the TSA server.

haydentherapper · 2022-11-08T16:23:02Z

If we are beginning to adopt the Sigstore bundle format, I would prefer we use exactly what has been discussed and approved. Otherwise we now have a third format effectively. If we want to deviate, we should discuss the change in the protobuf specs repo first imo

cc @kommendorkapten

priyawadhwa

this looks really good! just one question.

priyawadhwa · 2022-11-08T16:40:01Z

pkg/cosign/bundle/bundle.go

+)
+
+type Bundle struct {
+	VerificationData


What's the difference between VerificationData and VerificationMaterial? I wonder if we could come up with more descriptive names so it's more clear :)

It might be better to port that comment to the upstream bundle, but these are terms being used by those protos.
Data is extra auxiliary data for verification (timestamps, rekor entries), but verification material is the direct material (key hint, certificates)

I am gonna port these and other questions to the upstream bundle. Thanks.

haydentherapper · 2022-11-08T16:42:58Z

pkg/cosign/bundle/bundle.go

+	PublicKeyIdentifier string
+}
+
+// TimestampVerificationData contains various timestamped data following RFC3161.


As we discussed in the protobuf-specs repo, SETs are not just timestamp verification data, hence why we went with this nested format - https://github.com/sigstore/protobuf-specs/blob/main/protos/sigstore_bundle.proto#L53-L59

I created one issue here sigstore/protobuf-specs#13. Hopefully we can get more ppl involved on this discussion.

haydentherapper · 2022-11-08T16:44:30Z

pkg/cosign/bundle/bundle.go

+	SignedEntryTimestamp []byte
+
+	// EntryTimestampAuthority contains the recorded entry from timestamp authority server.
+	EntryTimestampAuthority []byte


I would prefer a name that includes RFC3161 rather than Timestamp Authority, as the former makes it clear what the format of the time record is

Yes, it makes sense. I like it.

haydentherapper · 2022-11-08T16:44:45Z

pkg/cosign/bundle/bundle.go

+	EntryTimestampAuthority []byte
+}
+
+func EntryToBundle(tLogEntry *models.LogEntryAnon, signedEntryTimestamp, entryTimestampAuthority, certBytes []byte, pubKeyID string) *Bundle {


Yes, we need tests but I wanted to firstly get some feedback, as I expected things might change.

priyawadhwa · 2022-11-08T16:44:55Z

@haydentherapper this isn't a third format, it's just an addition to the current format. Definitely agree we should also plan for it in the protobuf work, but IIUC that work isn't ready to be used yet (@kommendorkapten please correct me if I'm wrong!). I think it's ok to amend the current format so that we can start testing out this feature & then switch over to the protobufs whenever they're ready.

asraa · 2022-11-08T15:16:15Z

cmd/cosign/cli/attest/attest.go

@@ -62,7 +62,7 @@ func uploadToTlog(ctx context.Context, sv *sign.SignerVerifier, rekorURL string,
 		return nil, err
 	}
 	fmt.Fprintln(os.Stderr, "tlog entry created with index:", *entry.LogIndex)
-	return cbundle.EntryToBundle(entry), nil
+	return cbundle.EntryToBundle(entry, []byte{}, []byte{}, []byte{}, ""), nil


nit: use nil instead of []byte{}?

asraa · 2022-11-08T16:47:32Z

pkg/cosign/bundle/bundle.go

+)
+
+type Bundle struct {
+	VerificationData


It might be better to port that comment to the upstream bundle, but these are terms being used by those protos.
Data is extra auxiliary data for verification (timestamps, rekor entries), but verification material is the direct material (key hint, certificates)

asraa · 2022-11-08T16:51:07Z

pkg/cosign/bundle/bundle.go

+	// a Rekor transparency log.
+	// Use Payload instead of TransparencyLogEntry to keep backwards compatibility.
+	Payload RekorPayload
+


Those fields overlap with LocalSignedPayload too. The top level fields in that would ideally be removed, or else it would open mismatch issues.

haydentherapper · 2022-11-08T16:51:32Z

I think we can begin to adopt the format, lemme know otherwise @kommendorkapten. I'm worried about introducing an internal Bundle structure, as it's going to be harder to differentiate between the new bundle format, this bundle structure, and what Rekor returns. We also had a lot of discussion about what the role of the timestamp is when designing the protobuf and designing it to be future proof, and I think those same issues are popping up here.

If we don't want to adopt the new bundle format yet, I'd prefer we make a struct that makes it clear that it's not another bundle format, but still reuse as much as possible from the bundle format wrt to timestamp fields

asraa · 2022-11-08T16:51:57Z

pkg/cosign/bundle/bundle.go

+	b := &Bundle{}
+	// If none of the verification data is configured then return nil
+	if (tLogEntry == nil || tLogEntry.Verification == nil) && len(entryTimestampAuthority) == 0 {
+		return nil


why return nil over returning an empty bundle? id be worried about accidental dereferences later

I agree with you but tLogEntry == nil || tLogEntry.Verification == nil is reusing the old behavior that also returned a nil bundle.

znewman01 · 2022-11-09T00:10:11Z

I think we can begin to adopt the format, lemme know otherwise @kommendorkapten. I'm worried about introducing an internal Bundle structure, as it's going to be harder to differentiate between the new bundle format, this bundle structure, and what Rekor returns. We also had a lot of discussion about what the role of the timestamp is when designing the protobuf and designing it to be future proof, and I think those same issues are popping up here.

I broadly agree. That said, I don't expect client implementations to keep the protobuf representations throughout the program; rather, I'd expect them to "parse" the protobuf out into language-idiomatic objects that are a little bit smarter than the generated proto code. This helps us reject invalid representations that are valid in protobuf (e.g., invalid DER).

Here's what I'd propose:

We will want a "bundle" type in Golang eventually, so this can become that.
Until the protobuf-specs bundle is a little more mature, this PR should remain internal-only which will allow refactoring once the bundle is stabilized.
However, any deviations from the protobuf spec should be (1) heavily commented, and (2) justified with language-specific reasoning. If there's a non-language-specific objection, let's get it fixed upstream.

znewman01

This is a very-much breaking change to the Golang API. Can we make it official that this API isn't supported by any versioning guarantees?

See also #2365 (reply in thread)

znewman01 · 2022-11-09T00:14:14Z

pkg/cosign/bundle/bundle.go

+// signatures or any additional timestamped verifications.
+type VerificationMaterial struct {
+	// A chain of X.509 certificates.
+	CertBytes []byte


I'd expect this to be a crypto/x509.Certificate

I guess really...is this struct a data format or an object? My preference is always to move parsing as early as possible so we don't have to handle parse errors in the middle of the program logic.

kommendorkapten · 2022-11-09T07:54:04Z

The Sigstore bundle from https://github.com/sigstore/protobuf-specs is in a decent shape I would say. Is it worthy to cut a release in the current stage? Probably no, but I don't think we are miles away. For the npm build provenance we are working on two different implementations of the bundle, generation and verification. One in Go, and one in JavaScript. So we are getting real world testing of the usability now. My feeling is that we will see some small changes in the imminent future, like sigstore/protobuf-specs#10, and cut a release within a few weeks. All the work on the bundle we are doing is expected to be part of sigstore-js and sigstore-go.

Beyond the first release, there are a few expected breaking changes, but the horizon for them is a bit unclear, as we are waiting for e.g. Rekor SET V2.
The bundle is versioned, so it shouldn't be an issue for clients to support multiple versions. If cosign waits until the next bundle release for adoption, I think that can be reasonable too, as long as the this PR doesn't stray away too much and makes the adoption harder.

hectorj2f · 2022-11-10T09:54:25Z

Thanks everyone for the review and suggestions. I'll keep this PR open so I can take over on these changes whenever there aren't major breaking changes expected from the protobuf spec.

In the meantime, I'll try to move on with #2331, and open a different PR with a smaller change that just renames the struct name from RekorBundle to Bundle, and adds a new field EntryRFC3161 to record the timestamped entry from sigstore/timestamp-authority.

kommendorkapten · 2022-11-10T09:59:20Z

If it's of any help, we are ready to merge this PR sigstore/protobuf-specs#12 which adds the generated Go code to the repository, so you don't need to rewrite any structs -- you can import them directly from that repository.

hectorj2f · 2022-12-05T17:45:35Z

Closing this for now! I'll re-open it when starting this work again.

hectorj2f added 2 commits November 8, 2022 11:16

add sigstore bundle type

af8c2b8

Signed-off-by: Hector Fernandez <hector@chainguard.dev>

adapt code and tests to use new Bundle fields

1903874

Signed-off-by: Hector Fernandez <hector@chainguard.dev>

hectorj2f requested review from asraa, haydentherapper and priyawadhwa November 8, 2022 10:44

hectorj2f self-assigned this Nov 8, 2022

hectorj2f commented Nov 8, 2022

View reviewed changes

priyawadhwa reviewed Nov 8, 2022

View reviewed changes

haydentherapper reviewed Nov 8, 2022

View reviewed changes

asraa reviewed Nov 8, 2022

View reviewed changes

hectorj2f mentioned this pull request Nov 8, 2022

Adopt the sigstore bundle format in cosign sigstore/protobuf-specs#13

Closed

znewman01 reviewed Nov 9, 2022

View reviewed changes

hectorj2f mentioned this pull request Nov 10, 2022

Rename RekorBundle type to Bundle #2435

Closed

hectorj2f closed this Dec 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define a new sigstore Bundle type to include additional verification data and materials #2422

Define a new sigstore Bundle type to include additional verification data and materials #2422

hectorj2f commented Nov 8, 2022

hectorj2f Nov 8, 2022

hectorj2f Nov 8, 2022 •

edited

Loading

hectorj2f Nov 8, 2022

asraa Nov 8, 2022

hectorj2f Nov 8, 2022

hectorj2f commented Nov 8, 2022

haydentherapper commented Nov 8, 2022

priyawadhwa left a comment

priyawadhwa Nov 8, 2022

asraa Nov 8, 2022

hectorj2f Nov 8, 2022

haydentherapper Nov 8, 2022

hectorj2f Nov 8, 2022

haydentherapper Nov 8, 2022

hectorj2f Nov 8, 2022

haydentherapper Nov 8, 2022

hectorj2f Nov 8, 2022

priyawadhwa commented Nov 8, 2022

asraa Nov 8, 2022

asraa Nov 8, 2022

asraa Nov 8, 2022

haydentherapper commented Nov 8, 2022

asraa Nov 8, 2022

hectorj2f Nov 8, 2022

znewman01 commented Nov 9, 2022

znewman01 left a comment

znewman01 Nov 9, 2022

kommendorkapten commented Nov 9, 2022

hectorj2f commented Nov 10, 2022

kommendorkapten commented Nov 10, 2022

hectorj2f commented Dec 5, 2022

Define a new sigstore Bundle type to include additional verification data and materials #2422

Define a new sigstore Bundle type to include additional verification data and materials #2422

Conversation

hectorj2f commented Nov 8, 2022

Summary

Release Note

Documentation

Choose a reason for hiding this comment

hectorj2f Nov 8, 2022 • 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

hectorj2f commented Nov 8, 2022

haydentherapper commented Nov 8, 2022

priyawadhwa left a 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

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

Choose a reason for hiding this comment

priyawadhwa commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

znewman01 commented Nov 9, 2022

znewman01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kommendorkapten commented Nov 9, 2022

hectorj2f commented Nov 10, 2022

kommendorkapten commented Nov 10, 2022

hectorj2f commented Dec 5, 2022

hectorj2f Nov 8, 2022 •

edited

Loading