dsse type #596

mikhailswift · 2022-01-13T23:31:45Z

Summary

Adds a DSSE type that validated each signature on the envelope. If the payload is an in-toto statement all in-toto subjects will be indexed. The hash of the payload, PAE encoded payload, and public keys are also indexed.

I've tested this locally and it works but I need to write tests and add it to the e2e test script yet. I'm opening this draft for discussion and input.

Ticket Link

Fixes #582

Release Note

bobcallaway

looking good so far, a couple minor things.

Does it make sense to move any of the verifier logic into sigstore/sigstore for reuse?

bobcallaway · 2022-01-14T13:59:26Z

pkg/types/dsse/dsse.go

+
+	in, ok := pe.(*models.Dsse)
+	if !ok {
+		return nil, errors.New("cannot unmarshal non-Rekord types")


Suggested change

return nil, errors.New("cannot unmarshal non-Rekord types")

return nil, errors.New("cannot unmarshal non-DSSE types")

bobcallaway · 2022-01-14T14:02:49Z

pkg/types/dsse/v0.0.1/entry.go

+	_, err := verifyEnvelope(allPubKeyBytes, env)
+	if err != nil {
+		return fmt.Errorf("could not verify envelope: %w", err)
+	}
+


Suggested change

_, err := verifyEnvelope(allPubKeyBytes, env)

if err != nil {

return fmt.Errorf("could not verify envelope: %w", err)

}

if _, err := verifyEnvelope(allPubKeyBytes, env); err != nil {

return fmt.Errorf("could not verify envelope: %w", err)

}

dlorenc · 2022-01-16T01:59:20Z

Thanks for the PR and much needed cleanup!

mikhailswift · 2022-01-18T16:00:24Z

Sorry for the delay in getting this finished up -- lost power over the weekend. I'm going to try and get this cleaned up and a full PR up today.

pxp928 · 2022-05-18T20:44:57Z

@mikhailswift I just ran into this issue recently. Do you need any help pushing this over the finish line?

codecov-commenter · 2022-05-20T19:04:07Z

Codecov Report

Merging #596 (85d1a9e) into main (547eb3c) will increase coverage by 0.00%.
The diff coverage is 37.03%.

@@           Coverage Diff           @@
##             main     #596   +/-   ##
=======================================
  Coverage   46.56%   46.57%           
=======================================
  Files          60       61    +1     
  Lines        5094     5121   +27     
=======================================
+ Hits         2372     2385   +13     
- Misses       2447     2460   +13     
- Partials      275      276    +1

Impacted Files	Coverage Δ
cmd/rekor-cli/app/root.go	`20.68% <ø> (ø)`
pkg/types/entries.go	`6.25% <ø> (ø)`
pkg/types/dsse/dsse.go	`37.03% <37.03%> (ø)`
pkg/types/alpine/v0.0.1/entry.go	`56.25% <0.00%> (+1.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 547eb3c...85d1a9e. Read the comment docs.

dlorenc

A few comments.

pkg/types/dsse/v0.0.1/dsse_v0_0_1_schema.json

pkg/types/dsse/v0.0.1/entry.go

dlorenc · 2022-06-05T23:08:41Z

pkg/types/dsse/v0.0.1/entry.go

+		allPubKeyBytes = append(allPubKeyBytes, props.PublicKeyBytes)
+	}
+
+	allPubKeyBytes = append(allPubKeyBytes, props.PublicKeysBytes...)


I might be misunderstanding this, but it looks like the props.PublicKeyBytes will get appended twice. Is that intentional?

PublicKeyBytes vs PublicKeysBytes

Part of the discussion on the issue was how to handle passing multiple public keys for cases where we would want to validate multiple signatures on the envelope. To handle this a new prop called PublicKeysBytes was added to handle that case. It's not a very nice solution though, so definitely open to alternatives here.

pkg/types/dsse/v0.0.1/entry.go

bobcallaway

A few comments/questions inline; I think we should also update pkg/types/README.md with some info about this type and how it explicitly differs from intoto.

pkg/types/dsse/v0.0.1/dsse_v0_0_1_schema.json

pkg/types/dsse/v0.0.1/entry.go

pkg/types/dsse/dsse.go

pkg/types/dsse/v0.0.1/entry.go

bobcallaway · 2022-06-06T19:12:48Z

pkg/types/dsse/v0.0.1/entry.go

+			return nil, fmt.Errorf("could not parse public key as x509: %w", err)
+		}
+
+		vfr, err := signature.LoadVerifier(key.CryptoPubKey(), crypto.SHA256)


is the hash algorithm always going to be SHA256 for this?

While not part of the DSSE spec it seems that all current users of DSSE default to using SHA256 during signing.

Adds a DSSE envelope type to rekor. If the DSSE envelope's payload is an in-toto statement the in-toto subjects will be used as indices for the envelope's rekord. If the envelope's payload is within the server's configured attestation size the payload will be stored as an attestation. Signed-off-by: Mikhail Swift <mikhail@testifysec.com>

dlorenc · 2022-08-29T18:56:40Z

This can get closed out now!

jmeth · 2022-09-19T14:29:46Z

Sorry to comment on a closed PR but it seems like this was closed but never merged in to main. Was that the intention here?

pxp928 · 2022-09-19T16:03:53Z

Sorry to comment on a closed PR but it seems like this was closed but never merged in to main. Was that the intention here?

@jmeth All the changes here were incorporated in another PR #973

mikhailswift force-pushed the dsse-type branch 2 times, most recently from c7d0209 to 92bcffe Compare January 13, 2022 23:35

bobcallaway reviewed Jan 14, 2022

View reviewed changes

mikhailswift force-pushed the dsse-type branch 4 times, most recently from ff7ad21 to b688645 Compare January 16, 2022 01:16

mikhailswift force-pushed the dsse-type branch 2 times, most recently from f624a37 to 2b52d1e Compare May 20, 2022 19:00

mikhailswift force-pushed the dsse-type branch 2 times, most recently from 85d1a9e to 009fb0b Compare May 23, 2022 17:36

mikhailswift marked this pull request as ready for review May 23, 2022 17:36

mikhailswift requested a review from a team as a code owner May 23, 2022 17:36

mikhailswift changed the title ~~[DRAFT] dsse type~~ dsse type May 23, 2022

dlorenc reviewed Jun 5, 2022

View reviewed changes

mikhailswift force-pushed the dsse-type branch 3 times, most recently from a8f7d0e to b6b476e Compare June 6, 2022 18:33

bobcallaway reviewed Jun 6, 2022

View reviewed changes

mikhailswift force-pushed the dsse-type branch from b6b476e to 8ae8c48 Compare June 6, 2022 21:20

mikhailswift force-pushed the dsse-type branch from 8ae8c48 to ddb8814 Compare June 6, 2022 21:23

dlorenc closed this Aug 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dsse type #596

dsse type #596

mikhailswift commented Jan 13, 2022

bobcallaway left a comment

bobcallaway Jan 14, 2022

bobcallaway Jan 14, 2022

dlorenc commented Jan 16, 2022

mikhailswift commented Jan 18, 2022

pxp928 commented May 18, 2022

codecov-commenter commented May 20, 2022 •

edited

dlorenc left a comment

dlorenc Jun 5, 2022

mikhailswift Jun 6, 2022

bobcallaway left a comment

bobcallaway Jun 6, 2022

mikhailswift Jun 6, 2022

dlorenc commented Aug 29, 2022

jmeth commented Sep 19, 2022

pxp928 commented Sep 19, 2022

	return nil, errors.New("cannot unmarshal non-Rekord types")
	return nil, errors.New("cannot unmarshal non-DSSE types")

dsse type #596

dsse type #596

Conversation

mikhailswift commented Jan 13, 2022

Summary

Ticket Link

Release Note

bobcallaway left a comment

Choose a reason for hiding this comment

bobcallaway Jan 14, 2022

Choose a reason for hiding this comment

bobcallaway Jan 14, 2022

Choose a reason for hiding this comment

dlorenc commented Jan 16, 2022

mikhailswift commented Jan 18, 2022

pxp928 commented May 18, 2022

codecov-commenter commented May 20, 2022 • edited

Codecov Report

dlorenc left a comment

Choose a reason for hiding this comment

dlorenc Jun 5, 2022

Choose a reason for hiding this comment

mikhailswift Jun 6, 2022

Choose a reason for hiding this comment

bobcallaway left a comment

Choose a reason for hiding this comment

bobcallaway Jun 6, 2022

Choose a reason for hiding this comment

mikhailswift Jun 6, 2022

Choose a reason for hiding this comment

dlorenc commented Aug 29, 2022

jmeth commented Sep 19, 2022

pxp928 commented Sep 19, 2022

codecov-commenter commented May 20, 2022 •

edited