Added support for sha384/sha512 hash algorithms in hashedrekords #1959
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1959 +/- ##
==========================================
+ Coverage 66.39% 66.48% +0.08%
==========================================
Files 92 92
Lines 9199 9246 +47
==========================================
+ Hits 6108 6147 +39
- Misses 2347 2355 +8
Partials 744 744
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Includes changes provided by @bobcallaway Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
ret2libc
force-pushed
the
add-sha384-512-hashedrekord
branch
from
2577342
to
f28ea3a
Compare
| case 40: | ||
| return crypto.SHA1, sha |
There was a problem hiding this comment.
I know this is probably because the function above included sha1, but I think we should remove this (and the stanza above). There's no other use of that AFAIK in sigstore, and I wouldn't want to perpetuate this.
There was a problem hiding this comment.
actually, i think it is used to search for entries from gitsign, im working to confirm
@wlynch - any thoughts?
There was a problem hiding this comment.
nm, i think we need to leave this - but it would be nice to add a test case that calls CreateFromArtifactProperties with a proposed sha1 value to ensure that it doesn't somehow end up in the log.
There was a problem hiding this comment.
Added it in 6903f5f . Please let me know if this is what you meant, otherwise I can adjust it.
There was a problem hiding this comment.
We don't use this for gitsign today - gitsign does a sha256 over the commit content + sha1 for rekor.
(though it would be a neat way to remove some of the chattiness gitsign has today with it's sha -> rekor entry lookup 🤔 )
Since it looks like this is already done + tests were added, I say keep it. We can revisit removing it later.
Make sure Rekor accepts SHA256 digest but not SHA1 Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
| re := V001Entry{} | ||
|
|
||
| // we will need artifact, public-key, signature | ||
| re.HashedRekordObj.Data = &models.HashedrekordV001SchemaData{} | ||
|
|
||
| re.HashedRekordObj.Signature = &models.HashedrekordV001SchemaSignature{} | ||
| re.HashedRekordObj.Signature.Content = strfmt.Base64(signature) | ||
|
|
||
| re.HashedRekordObj.Signature.PublicKey = &models.HashedrekordV001SchemaSignaturePublicKey{} | ||
| publicKeyBytes := [][]byte{pubBytes} | ||
|
|
||
| re.HashedRekordObj.Signature.PublicKey.Content = strfmt.Base64(publicKeyBytes[0]) | ||
| re.HashedRekordObj.Data.Hash = &models.HashedrekordV001SchemaDataHash{ | ||
| Algorithm: swag.String("sha1"), | ||
| Value: swag.String("a17c9aaa61e80a1bf71d0d850af4e5baa9800bbd"), | ||
| } | ||
|
|
There was a problem hiding this comment.
Could you switch this to use the same pattern as above where you call CreateFromArtifactProperties? That is the call path that is at risk (as the JSON Schema validation will fail the API request)
There was a problem hiding this comment.
Done in 0f1f8e3: I've moved this out of e2e since it no longer needs access to a runtime Rekor instance.
There was a problem hiding this comment.
The reason why I did not use CreateFromArtifactProperties is that the test would fail in CreateFromArtifactProperties on the client side and and not on the rekor server side. Thus I wanted to test that the server did not accept it rather than just checking the client.
There was a problem hiding this comment.
The reason why I did not use
CreateFromArtifactPropertiesis that the test would fail inCreateFromArtifactPropertieson the client side and and not on the rekor server side. Thus I wanted to test that the server did not accept it rather than just checking the client.
That makes sense, but I think @bobcallaway is pointing out that the server side is already checked by virtue of the JSON Schema Validation -- we could include both checks, though (with the non-e2e doing CreateFromArtifactProperties and the e2e doing it manually).
Signed-off-by: William Woodruff <william@trailofbits.com>
Summary
Add support for SHA384/512 in hashedrekord type. This PR has been split from #1945 and borrows code from #1958 (from @bobcallaway ).
Release Note
data.hash.algorithmhashedrekord.CreateFromArtifactPropertiesto accept a prefixed-checksum (e.g.sha512:<value>) to determine the type of hash used.util.UnprefixSHAAPI to split a string into the crypto.Hash being used and its valueDocumentation