Add support for Rekor V2 for signing and verification #481

cmurphy · 2025-05-16T23:02:09Z

Add support for handling and uploading a Rekor v2 entry. Replace
reliance on rekor v1 with the TransparencyLogEntry from protobuf-specs
as much as possible to reduce divergence in how different entries are
handled. Add new functions where needed to avoid breaking backwards
compatibility with existing functions.

Partial sigstore/rekor-tiles#349

Summary

Release Note

Documentation

haydentherapper · 2025-05-22T01:47:41Z

pkg/bundle/bundle.go

@@ -320,6 +381,30 @@ func (b *Bundle) TlogEntries() ([]*tlog.Entry, error) {
 	return tlogEntries, nil
 }

+func (b *BundleForRekorV2) TlogV2Entries() ([]tlog.LogEntry, error) {


What's the reason for a second TlogEntries function?

Can we reuse the existing and when constructing the bundle, have a validation step that the expected fields are unset?

The original one uses the rekor v1 generated models to parse and marshal the entry object. It's completely incompatible with the rekor v2 protobuf types.

haydentherapper · 2025-05-22T01:50:45Z

pkg/verify/tlog.go

+	logEntriesVerified := 0
+
+	for _, entry := range entries {
+		// TODO: validate hashedrekord/dsse obj?


If we have the artifact, we should verify that the artifact hash matches the hashedrekord hash/dsse payload hash. This was something missing from the current validation function since we can't verify dsse envelope hashes, but this is now resolved since we've dropped the envelope hash.

Maybe I'm missing it but I don't think we have access to the artifact here. We just have the bundle.

haydentherapper · 2025-05-22T02:24:08Z

pkg/tlog/entry.go


 	return entry, nil
 }

+func ValidateLogEntryBody(entry LogEntry) error {
+	if e, ok := entry.(*rekorV1Entry); ok {


Ideally we don't have to depend on Rekor v1. What do you think about creating a small Rekor entry verification library that doesn't depend on Rekor v1's generated types, unmarshals canonicalized bodies, and handles validation? We could replace calls to UnmarshalEntry.

I'm thinking something like:

type RekorEntry interface { Kind() string Version() string Validate() error // validates expected fields are set Digest() []byte // return payload hash for DSSE/intoto Verifiers() []Verifier // from rekor-tiles Signature() []byte }

And then have implementations of the interface for a minimal set of Rekor entries, HashedRekordV001, HashedRekordV002, DSSEV001, DSSEV002 and IntotoV002.

Then have one function like UnmarshalRekorEntry that parses a canonicalized body into an RekorEntry given its kind and version, and gracefully handles unknown kinds/versions. (Or we can recreate the Rekor v1 types and typeimpls, though I think it's a little overkill and some switch statements would be cleaner)

We probably would need to do some copying from Rekor v1, though maybe there isn't actually too much to copy, I think we just need each entry defined as a struct with json tags to handle unmarshalling the canonicalized body.

Where this lives, here or in sigstore/sigstore?

I think this library would be quite useful across Go clients.

To the extent possible I've changed this to use the TLE instead of the rekor v1 OpenAPI models so that the divergence is minimized. I do not think a separate library would be appropriate. Sigstore-go is the only thing that would be using it and cosign eventually should be consuming sigstore-go.

haydentherapper · 2025-05-22T02:29:50Z

pkg/verify/tlog.go

+		if !bytes.Equal(entry.Signature(), entitySignature) {
+			return fmt.Errorf("transparency log signature does not match")
+		}
+		protoPubKey := entry.PublicKey()


IIRC this is confusingly named (to mirror rekor v1), this is a key or certificate. It'd be nice to rename this or change this to return a verifier, and conditionally compare either a certificate or key to what's in the bundle.

This cannot be changed, it's a public method.

haydentherapper · 2025-05-22T02:30:37Z

examples/sigstore-go-signing/main.go

 			}
 			opts.TransparencyLogs = append(opts.TransparencyLogs, sign.NewRekor(rekorOpts))
 		}
 	}

+	opts.UseRekorV2 = rekorVersion == uint32(2)


We also need to enforce that if Rekor v2 is used to sign, there is at least one timestamp authority.

sigstore-go verification policies are composable, it's up to the caller to decide whether to check for a timestamp or not. The bundle signer policies already enforce that there is a timestamp somewhere, and if the entry comes from rekor v2 there will be no integrated timestamp, so it will fail unless there is a TSA timestamp.

cmurphy · 2025-06-04T23:38:19Z

Refactored to reduce the amount of v1/v2 divergence to a point I'm happier with. Still need to address time, DSSE, and the verify example.

cmurphy · 2025-06-17T22:40:39Z

e2e tests waiting on sigstore/scaffolding#1606 and #489

haydentherapper

Great work on this!

haydentherapper · 2025-06-18T16:21:35Z

pkg/sign/signer.go

+		// Rekor v2 requires a timestamp authority, it will not provide integrated timestamps.
+		// Verification will fail if a timestamp authority is not provided for Rekor v2.
+		if len(opts.TimestampAuthorities) == 0 {
+			verifierOptions = append(verifierOptions, verify.WithIntegratedTimestamps(len(opts.TransparencyLogs)))


Is it possible to use the Rekor API version to return an error here rather than fail verification later? This is functional so if not, that's fine, but this will surface an error that won't make the issue obvious to the caller.

I tried but I could not find a good way to do that, there is no way to pass through the rekor API version without changing the method signature or a type definition. It could be deduced by looking at the KindVersion the tlog entry and checking if it is a hashedrekord v002 or dsse v002 type, but I don't think that's a foolproof indicator (what if we add more types to Rekor v2? what if we add those v002 types to Rekor v1? the type of entry is not technically related to whether a RFC3161 timestamp is required). I actually like the way this worked out because it's clear what the reality is, that if you're using Rekor v1 you need an integrated timestamp and if you're using Rekor v2 you need an external timestamp, and it will be clear in the verification result whether you did it correctly.

haydentherapper · 2025-06-18T18:13:05Z

pkg/sign/transparency.go

+	return nil
+}
+
+func (r *Rekor) getRekorV2TLE(ctx context.Context, pubKeyPEM []byte, b *protobundle.Bundle) (*protorekor.TransparencyLogEntry, error) {


nit, I'd rather use something like keyOrCertPEM in all the places that we pass either a key or certificate. The current name is because Rekor v1 tacked on certificates later on into the key field. I'd say the variable name change would help clarify that it's not just a key.

Can also refactor this in another PR.

haydentherapper · 2025-06-18T18:26:19Z

pkg/tlog/entry.go

+		}
+		entry.tle.InclusionProof = &v1.InclusionProof{
+			LogIndex: logIndex,
+			RootHash: []byte(*inclusionProof.RootHash),


The root hash and inclusion proof hashes are hex-encoded (parsing example), unless these are decoded elsewhere earlier in the stack, so we'll need to hex-decode for the TLE struct.

haydentherapper · 2025-06-18T18:31:49Z

pkg/tlog/entry.go

+			Hashes:     tle.InclusionProof.Hashes,
+			Checkpoint: tle.InclusionProof.Checkpoint,
+		}
+		rootHash := make([]byte, hex.EncodedLen(len(tle.InclusionProof.RootHash)))


Same here, we don't need to hex-encode.

I was facing problems in the unit tests where different tests expected different TLE objects to have the root hash hex encoded or not. This configuration was the only way I could get it to work. I will try to track down exactly why this was happening, but I think the fact that the unit and e2e tests are passing indicate that it does have to be hex encoded.

That might be a bug in the unit tests parsing the fields of the TLE. I know we also had a bug in rekor-tiles, but that's been fixed (sigstore/rekor-tiles#295). Hex encodings should only be needed when working with the OpenAPI generated objects.

haydentherapper · 2025-06-18T18:35:08Z

pkg/tlog/entry.go

 	return nil
 }

+func validateHashedRekordV002Entry(hr *rekortilespb.HashedRekordLogEntryV002) error {


For the future, we should make https://github.com/sigstore/rekor-tiles/blob/28f3c79d9855192fde1e896c27c4b7b225c7436f/pkg/types/hashedrekord/hashedrekord.go#L68 public and reuse this

We can't use that here because that validates a HashedRekordRequestV002, here we have a HashedRekordLogEntryV002.

Ah yea. So the opposite, we should move this and create a entry verifier library in rekor-tiles. Linking to sigstore/rekor-tiles#234 to figure this out later.

haydentherapper · 2025-06-18T18:41:02Z

pkg/tlog/entry.go

 }

 func (entry *Entry) Body() any {
-	return entry.logEntryAnon.Body
+	return base64.StdEncoding.EncodeToString(entry.tle.CanonicalizedBody)


We should revisit this method. It returning any makes it a bit hard to use in practice. Should this return a byte array that clients have to parse? Since it's always JSON, should this method unmarshal the body into a struct with version, kind, and spec?

Looks like Body() isn't called outside of tests, so I'd be fine with a breaking API change for this method.

Looks like Body() isn't called outside of tests, so I'd be fine with a breaking API change for this method.

I disagree with this. The definition of an exported method is that it could be called by anything outside the code base. I was very careful not to break any APIs in any of these changes. I'd be fine with adding a new BodyJSON method and marking this one as deprecated (in a separate PR), but not with changing this one.

Given the newness of the library, I don't feel as strongly around breaking changes, but I get what you're saying. We have a number of functions we've already marked as deprecated that we'll clean up in a later version, so let's do the same here, deprecate Body() and create a new function. Up to you for what this should return - I would find it most useful to return an unmarshaled struct with kind, version, and spec, or the marshaled JSON (and entry.logEntryAnon.Body can either be a JSON byte array, or a base64-encoded string of the JSON body (code), so we need to handle that)

Can we do that in a separate PR? It's not really related to rekor v2 IMO. Then we can debate there what the right thing to return is.

haydentherapper · 2025-06-18T18:43:36Z

pkg/tlog/entry.go

+				Checkpoint: swag.String(entry.tle.GetInclusionProof().GetCheckpoint().GetEnvelope()),
+				Hashes:     hashes,
+				LogIndex:   swag.Int64(entry.tle.GetInclusionProof().GetLogIndex()),
+				RootHash:   swag.String(string(entry.tle.GetInclusionProof().GetRootHash())),


This will need to be hex-encoded...sigh, these hex encodings are fun. Glad to not be working around OpenAPI v2 limitations now (iirc, there was something with byte arrays being unsupported?).

pkg/tlog/entry.go

pkg/verify/tlog.go

haydentherapper · 2025-06-18T18:58:50Z

pkg/verify/tlog.go

+
+var treeIDSuffixRegex = regexp.MustCompile(".* - [0-9]+$")
+
+func hasSTH(entry *tlog.Entry) bool {


Can we name this to be about rekor v1? In either v1 or v2, there should be a checkpoint/signed tree head.

kommendorkapten · 2025-06-24T12:58:55Z

Good work!

haydentherapper

Looks really great!

haydentherapper · 2025-06-24T23:27:57Z

pkg/tlog/entry.go

-			InclusionProof: inclusionProof,
+		hashes := make([][]byte, len(inclusionProof.Hashes))
+		for i, s := range inclusionProof.Hashes {
+			hashes[i] = []byte(s)


Do we need to hex.DecodeString here as well? https://github.com/sigstore/rekor/blob/c0f3b8c639cb52bb2fa760465ad7d49e2ff19570/pkg/api/entries.go#L96

Can we also see if we have test coverage for verifying proofs from both Rekor v1 and v2?

Can we also see if we have test coverage for verifying proofs from both Rekor v1 and v2?

This is mostly covered by the e2e tests which exercise both v1 and v2. They didn't catch this bug because this is now a deprecated function. I added a new unit test which tests this path and catches this bug.

haydentherapper

Great work! Just needs a go mod tidy

Add support for handling and uploading a Rekor v2 entry. Replace reliance on rekor v1 with the TransparencyLogEntry from protobuf-specs as much as possible to reduce divergence in how different entries are handled. Add new functions where needed to avoid breaking backwards compatibility with existing functions. Signed-off-by: Colleen Murphy <[email protected]>

haydentherapper

🎉

cmurphy force-pushed the rekor-v2 branch 2 times, most recently from 17737b9 to 2146549 Compare May 19, 2025 21:10

haydentherapper reviewed May 22, 2025

View reviewed changes

cmurphy force-pushed the rekor-v2 branch 2 times, most recently from 0912cb6 to dabc2ef Compare June 4, 2025 23:37

cmurphy force-pushed the rekor-v2 branch 5 times, most recently from 03cf220 to caddd08 Compare June 10, 2025 17:36

cmurphy force-pushed the rekor-v2 branch 3 times, most recently from 2798478 to e426064 Compare June 17, 2025 22:40

cmurphy force-pushed the rekor-v2 branch from e426064 to e5eeb17 Compare June 18, 2025 17:16

cmurphy changed the title ~~DRAFT: Add support for Rekor V2 for verification~~ Add support for Rekor V2 for signing and verification Jun 18, 2025

cmurphy marked this pull request as ready for review June 18, 2025 17:20

cmurphy requested a review from a team as a code owner June 18, 2025 17:20

haydentherapper reviewed Jun 18, 2025

View reviewed changes

cmurphy force-pushed the rekor-v2 branch from e5eeb17 to f48c459 Compare June 18, 2025 21:06

kommendorkapten previously approved these changes Jun 24, 2025

View reviewed changes

haydentherapper reviewed Jun 24, 2025

View reviewed changes

cmurphy dismissed kommendorkapten’s stale review via 2e358bb June 26, 2025 22:45

cmurphy force-pushed the rekor-v2 branch from f48c459 to 2e358bb Compare June 26, 2025 22:45

haydentherapper previously approved these changes Jun 27, 2025

View reviewed changes

cmurphy dismissed haydentherapper’s stale review via 7d9041a June 27, 2025 17:09

cmurphy force-pushed the rekor-v2 branch from 2e358bb to 7d9041a Compare June 27, 2025 17:09

haydentherapper approved these changes Jun 27, 2025

View reviewed changes

haydentherapper merged commit 2aab0d7 into sigstore:main Jun 27, 2025
12 checks passed


		var treeIDSuffixRegex = regexp.MustCompile(".* - [0-9]+$")

		func hasSTH(entry *tlog.Entry) bool {

Add support for Rekor V2 for signing and verification #481

Add support for Rekor V2 for signing and verification #481

Uh oh!

Conversation

cmurphy commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Release Note

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmurphy commented Jun 4, 2025

Uh oh!

cmurphy commented Jun 17, 2025

Uh oh!

haydentherapper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmurphy Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kommendorkapten commented Jun 24, 2025

Uh oh!

haydentherapper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmurphy commented May 16, 2025 •

edited

Loading

cmurphy Jun 18, 2025 •

edited

Loading