-
Notifications
You must be signed in to change notification settings - Fork 164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dsse type #596
dsse type #596
Conversation
c7d0209
to
92bcffe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good so far, a couple minor things.
Does it make sense to move any of the verifier
logic into sigstore/sigstore for reuse?
pkg/types/dsse/dsse.go
Outdated
|
||
in, ok := pe.(*models.Dsse) | ||
if !ok { | ||
return nil, errors.New("cannot unmarshal non-Rekord types") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, errors.New("cannot unmarshal non-Rekord types") | |
return nil, errors.New("cannot unmarshal non-DSSE types") |
pkg/types/dsse/v0.0.1/entry.go
Outdated
_, err := verifyEnvelope(allPubKeyBytes, env) | ||
if err != nil { | ||
return fmt.Errorf("could not verify envelope: %w", err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_, 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) | |
} | |
ff7ad21
to
b688645
Compare
Thanks for the PR and much needed cleanup! |
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. |
@mikhailswift I just ran into this issue recently. Do you need any help pushing this over the finish line? |
f624a37
to
2b52d1e
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
85d1a9e
to
009fb0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments.
allPubKeyBytes = append(allPubKeyBytes, props.PublicKeyBytes) | ||
} | ||
|
||
allPubKeyBytes = append(allPubKeyBytes, props.PublicKeysBytes...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be misunderstanding this, but it looks like the props.PublicKeyBytes will get appended twice. Is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
a8f7d0e
to
b6b476e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
return nil, fmt.Errorf("could not parse public key as x509: %w", err) | ||
} | ||
|
||
vfr, err := signature.LoadVerifier(key.CryptoPubKey(), crypto.SHA256) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the hash algorithm always going to be SHA256 for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
This can get closed out now! |
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? |
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