-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat(spdx): add SHA-512 hash algorithm support to SPDX serializer #9130
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
|
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 think we also need to update unmarshaling. #9126 will probably help your understanding.
pkg/sbom/spdx/marshal_test.go
Outdated
@@ -37,6 +38,38 @@ func annotation(t *testing.T, comment string) spdx.Annotation { | |||
} | |||
} | |||
|
|||
func Test_spdxChecksums_SHA512(t *testing.T) { |
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.
This test doesn't appear to make sense. You can refer to other tests.
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.
you are right, i will remove the additional function
c14561b
to
ca2c4df
Compare
- Added SHA512 case to marshal.go - Added new test case for SHA512 digest in marshal_test.go - Fixed typo in SHA512 algorithm switch
ca2c4df
to
204a126
Compare
Hi @knqyf263 , I’ve addressed the following: Fixed the digest.SHA512 typo (was mistakenly written as spdx.SHA512) Removed the incorrect test case and added the correct one for SHA512 support in SPDX however, I looked into unmarshal.go as suggested, but I couldn’t find explicit handling for SHA1 or SHA256 there. |
@DicksenT Could you fix the lint error? |
on it |
@knqyf263 I've fixed the lint issues. Let me know if there's anything else I should address. Thanks again! |
Do you mind signing our CLA? |
signed, but somehow there 2 cla in here, one is signed, another one still pending despite i already agreed |
No worries. It's correctly recognized that you signed. |
pkg/sbom/spdx/marshal_test.go
Outdated
@@ -1448,6 +1448,125 @@ func TestMarshaler_Marshal(t *testing.T) { | |||
}, | |||
}, | |||
}, | |||
{ | |||
name: "happy path for SHA512 digest", |
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.
Adding a test case specifically for SHA-512 may be excessive. Can we merge this case into the existing case somehow?
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 think it's possible, but maybe i need to tweak marshal.go abit(?), or should i create the test in testdata/happy just like how it done in cycloneDx implementation?
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.
but maybe i need to tweak marshal.go a bit(?)
I don't think so. You can just modify the existing case. For example:
trivy/pkg/sbom/spdx/marshal_test.go
Line 441 in b42e7fc
Digest: "md5:483792b8b5f9eb8be7dc4407733118d0", |
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’ve added a sha512: Digest to the input report and also updated the PackageChecksums in the expected wantSBOM(both in "happy path for local container scan" test case), but the test fails, is there anything i've missed in the test case implementation?
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.
Why is it failing? What does it say?
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.
hello, is there any update on the issue?
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 don't have time to debug it right now.
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.
understandable, should we keep using spesific sha512 test case or you want me keep trying to merge the testcase?
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.
@DmitriyLewen Do you have time to take a look?
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.
simplified the test example for sha512 - 6ee1bc9
Hello @DicksenT trivy/pkg/sbom/spdx/unmarshal.go Lines 186 to 196 in 72ea4b0
You need to add testcase (as in https://github.com/aquasecurity/trivy/pull/9126/files#diff-b17fc40bdfbe923285c5951b6ff7243b43dddedfa22eb0a5967d695268096a87). |
Description
This PR adds SHA-512 hash algorithm support to the SPDX SBOM serializer.
Specifically, it updates
spdxChecksums()
inmarshal.go
to handle thedigest.SHA512
case, so Trivy can correctly include SHA-512 checksums in the SPDX output (used in tools likenpm sbom
). This complements #9126, which already added digest and CycloneDX support.A unit test was also added to ensure SHA-512 is handled correctly in SPDX.
Related issues
Related PRs
Checklist