Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix vex export returning invalid CycloneDX #3948

Merged
merged 8 commits into from
Aug 9, 2024
Merged

Fix vex export returning invalid CycloneDX #3948

merged 8 commits into from
Aug 9, 2024

Conversation

SaberStrat
Copy link

@SaberStrat SaberStrat commented Jul 8, 2024 •
edited
Loading

Description

Fixes exported VEX and BOMs with vulnerabilities not conforming to CycloneDX 1.5.

Addressed Issue

#3834

Additional Details

The general idea is to deduplicate vulnerabilities based on all Vulnerability fields.

I settled on an implementation with one HashSet that tracks all Vulnerability objects seen when converting Findings into Vulnerabilitys.

It requires an enhancement of the cyclonedx-core-java library, namely adding custom equals and hashCode methods to the Vulnerability class for fieldwise comparison: CycloneDX/cyclonedx-core-java#463.

Other possible implementations I considered:

  1. Comparison of Vulnerabilitys based on all of its fields except for affects and accumulate affects objects in one vulnerability if all other Vulnerability fields have otherwise equal values. This came from an interpretation of a remark from the original issue reporter in the issue report. But the CycloneDX schema doesn't really require an accumulation, if I'm not mistaken.
    I considered a few different implementations of this (in case an accumulation was not wrong after all).
    1. a HashMap of Vulnerabilitys to HashSets of Affects. This was the way I'd have gone with
    2. generating the affects list in its own convert method. I think this would've meant introducing two more methods. Could do this, if an accumulation of affects is a possibility after all
    3. appending to an affects list defined outside of and passed to Vulnerability's convert() calls. Was not sure about having convert() convert vulnerability objects AND modify a state of a list passed to it.

Checklist

  • I have read and understand the contributing guidelines
  • This PR fixes a defect, and I have provided tests to verify that the fix is effective
  • This PR implements an enhancement, and I have provided tests to verify that it works as intended
  • This PR introduces changes to the database model, and I have added corresponding update logic
  • This PR introduces new or alters existing behavior, and I have updated the documentation accordingly

@SaberStrat SaberStrat marked this pull request as ready for review July 9, 2024 21:46
@SaberStrat SaberStrat marked this pull request as draft July 9, 2024 21:47
@SaberStrat SaberStrat mentioned this pull request Jul 19, 2024
Closed
@SaberStrat SaberStrat marked this pull request as ready for review July 28, 2024 06:42
@SaberStrat SaberStrat requested a review from nscuro July 28, 2024 06:50
@nscuro nscuro added the defect Something isn't working label Aug 6, 2024
@nscuro nscuro added this to the 4.12 milestone Aug 6, 2024
@nscuro
Copy link
Member

nscuro commented Aug 6, 2024

@SaberStrat cyclonedx-core-java 9.0.5 was released earlier today, which includes your changes. You should be able to bump the library version now to make your implementation work.

Kirill.Sybin added 6 commits August 7, 2024 23:18
Make exported VEXs and BOMs valid CycloneDX
c8aa003 
Deduplicate found vulnerabilities and accumulate affected components
per vulnerabiltiy.

Signed-off-by: Kirill.Sybin <[email protected]>
Add test
94e9fa8 
Signed-off-by: Kirill.Sybin <[email protected]>
Use full Vulnerability object to check for dupes
84fa366 
Use all fields of Vulnerability objects, except for `affects`, for
deduplicated.
Requires CycloneDX/cyclonedx-core-java#463.

Signed-off-by: Kirill.Sybin <[email protected]>
Simplify Vulnerability deduplication logic
c1244fc 
Implement CycloneDX schema 1.5's definition of uniqueness of the
vulnerability field literally and have affects contribute to a
vulnerability's uniquenss too, and do not accumulate affects if
other all other fields of multiple vulnerability objects are the same.

Signed-off-by: Kirill.Sybin <[email protected]>
Move VEX export validity tests to VexResourceTest
6b7c18f 
As per change request, moving, reworking and enhancing the validity
test.
It is moved to VexResourceTest.
The rework consists of asserting the VEX export Response instead of the
validity of the exported BOM object, as the new way checks for the
original issue right where it is produced from the user perspective,
while the old checked where the original issue was noticed by the user
as well as being susceptible to errors in the validate method as well.
The enhancement consists of testing with vulnerabilities with differing
objects such as differing analysis of the same vuln id on different
components, thereby testing the deduplication of the entire
Vulnerability object.

Signed-off-by: Kirill.Sybin <[email protected]>
Bump org.cyclonedx.cyclonedx-core-java
3881bd7 
Bump library to use the version containing the required enchangement
CycloneDX/cyclonedx-core-java#463

Signed-off-by: Kirill.Sybin <[email protected]>
@SaberStrat SaberStrat force-pushed the fix/3834-vex-export-returns-invalid-cyclonedx branch from d988949 to 3881bd7 Compare August 7, 2024 21:29
@SaberStrat
Copy link
Author

cyclonedx-core-java bumped and rebased to master.

@nscuro
Copy link
Member

nscuro commented Aug 8, 2024

Looks like the schema validator version that comes with cyclonedx-core-java 9.0.5 had some slight changes in its error formatting:

Different value found in node "errors[0]", expected: <"$.components[0].type: does not have a value in the enumeration [application, framework, library, container, operating-system, device, firmware, file]"> but was: <"$.components[0].type: does not have a value in the enumeration ["application", "framework", "library", "container", "operating-system", "device", "firmware", "file"]">.

@SaberStrat
Copy link
Author

@nscuro I take it adding escaped quotes to the expected error messages in the test methods is not a proper solution for this?

@nscuro
Copy link
Member

nscuro commented Aug 9, 2024 •
edited
Loading

It is, actually! Nothing special to do on our side since we merely forward the error messages. But still good to have such tests to be made aware of when these things change.

Kirill.Sybin added 2 commits August 9, 2024 20:38
Update expected schema validator messages in tests
df55a0e 
Adjust expected json schema validator error messages after the library
producing them got bumped as part of the bump of
org.cyclonedx.cyclonedx-core-java.

Signed-off-by: Kirill.Sybin <[email protected]>
Update expected schema validator message in test
c7b8b4e 
Adjust another expected message missed in the previous commit.

Signed-off-by: Kirill.Sybin <[email protected]>
@SaberStrat
Copy link
Author

Rinse and repeat. This time, I ran the full test suite locally and it passed.

Should I cleanup/squash the commits down logically?

@nscuro
Copy link
Member

nscuro commented Aug 9, 2024

Should I cleanup/squash the commits down logically?

Not necessary.

@codacy-production Codacy Production
Copy link

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
✅ +0.02% (target: -1.00%) ✅ 100.00% (target: 70.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (2812b3a) 22107 16894 76.42%
Head commit (c7b8b4e) 22113 (+6) 16902 (+8) 76.43% (+0.02%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#3948) 8 8 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

nscuro
nscuro approved these changes Aug 9, 2024
View reviewed changes
Copy link
Member

@nscuro nscuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SaberStrat! I'll also backport this to v4.11.6.

@nscuro nscuro merged commit be40919 into DependencyTrack:master Aug 9, 2024
11 checks passed
@SaberStrat SaberStrat deleted the fix/3834-vex-export-returns-invalid-cyclonedx branch August 9, 2024 23:24
@nscuro nscuro mentioned this pull request Aug 10, 2024
Merged
2 tasks
@sahibamittal sahibamittal mentioned this pull request Aug 12, 2024
Open
2 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nscuro nscuro nscuro approved these changes

Assignees
No one assigned
Labels
defect Something isn't working
Projects
None yet
Milestone
4.12
Development

Successfully merging this pull request may close these issues.
2 participants
@SaberStrat @nscuro