-
Notifications
You must be signed in to change notification settings - Fork 630
Fix signing and verification with ed25519 keys with bundles and Rekor #4414
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4414 +/- ##
==========================================
- Coverage 40.10% 34.24% -5.87%
==========================================
Files 155 218 +63
Lines 10044 15638 +5594
==========================================
+ Hits 4028 5355 +1327
- Misses 5530 9591 +4061
- Partials 486 692 +206 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f9897c5
to
1de6ec9
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.
LGTM!
I'm so confused 😅 so help me check my understanding. Fulcio does not support ed25519ph, but Rekor does. And if someone is using a key they manage (instead of Fulcio), we want to support ed25519ph and use that with Rekor as well. In #4386 we removed some ed25519ph support, but did so in a way that broke "pure" ed25519 for self-managed keys. So we still aren't supporting ed25519ph with Fulcio, but we are fixing ed25519 for self-managed keys, and you can also use a self-managed ed25519ph key with Rekor. Did I get that right? |
Thanks y'all for the comments. I was a little preemptive putting this up, I need to fix e2e tests (they're failing because when the tlog isn't used, we use ed25519 - the prehash variant is only for hashedrekord). @steiza, correct. This is to support self-managed ed25519 keys. A summary of what's supported and what's not: Supported:
Maybe supported:
Not supported:
|
09f731d
to
92392bd
Compare
With the recent changes we made to use sigstore-go rather than Cosign for signing and verification, ed25519 managed key support broke, because we were incorrectly specifying ed25519ph for dsse Rekor entries and not specifying ed25519ph for hashedrekord entries. This PR correctly sets load options for when signing and verifying a blob (using the prehash variant) and when signing/verifying attestations (using the pure variant). This also fixes a bug where the SignerVerifier Keypair didn't handle crypto.Hash(0) for ed25519, which specifies no hash when signing. This has been tested with sign/verify, sign-blob/verify-blob, attest/verify-attestation, and attest-blob/verify-blob-attestation. Signed-off-by: Hayden <[email protected]>
92392bd
to
bc9c6d2
Compare
With the recent changes we made to use sigstore-go rather than Cosign for signing and verification, ed25519 managed key support broke, because we were incorrectly specifying ed25519ph for dsse Rekor entries and not specifying ed25519ph for hashedrekord entries. This PR correctly sets load options for when signing and verifying a blob (using the prehash variant) and when signing/verifying attestations (using the pure variant). This also fixes a bug where the SignerVerifier Keypair didn't handle crypto.Hash(0) for ed25519, which specifies no hash when signing.
This has been tested with sign/verify, sign-blob/verify-blob, attest/verify-attestation, and attest-blob/verify-blob-attestation.
Summary
Release Note
Documentation