-
Notifications
You must be signed in to change notification settings - Fork 571
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
Remove SHA256 assumption in sign-blob/verify-blob #4050
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4050 +/- ##
==========================================
- Coverage 40.10% 36.56% -3.54%
==========================================
Files 155 210 +55
Lines 10044 13557 +3513
==========================================
+ Hits 4028 4957 +929
- Misses 5530 7975 +2445
- Partials 486 625 +139 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I'm going to add a few tests to this. |
958259e
to
753bfc0
Compare
What do you think about limiting this to code under the |
Sounds good, but we still want to modify the "sign"/"sign-blob" flow, don't we? At least to test the signing process through cosign and then the verification process in the new bundle format, correct? |
The sign-blob flow, yes, where we also use the new-bundle-format flag. We don’t have to include sign at the moment since we don’t yet support the new bundle format. |
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.
Reviewed this for the Fulcio PR. This LGTM and I realize my comment about sign-blob
isn't immediately relevant as we need to make all these changes first.
Line 240 in 8dd9e9b
Algorithm: swag.String(models.HashedrekordV001SchemaDataHashAlgorithmSha256), |
If we do the Fulcio change to accept ecdsa-p384-sha-384 and ecdsa-p521-sha-512, I'd add an end-to-end test that verifies signing and verification with these keys. You can generate a key, import it into the Cosign key format, then sign with |
71ffdac
to
6116868
Compare
94cdb48
to
1ff6040
Compare
@haydentherapper could you approve the workflows please? |
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.
Overall LGTM
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
1871f58
to
8234ba5
Compare
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Summary
See #3271 .
Release Note
signature.LoadSignerVerifierFromPrivateKey
to load default verifiers given a private key.Documentation