Repository navigation
Deserialization silently loads broken bom #677
Description
Activity
@wkoot, do you want to give it a try fixing this?
I believe that making the
bom-refa part of__hash__and therefore the equality operator would solve this.But this boils down to an architectural choice regarding model strictness; how much deduplication should be enforced?
It is apparent that some tools (includingcyclonedx-cli) produce CycloneDX sbom json files which are lax in this regard.
By including thebom-refin the object hash strictness could be traded for practicality, saving a lot of hassle.I think there are three options:
- Throw an error when loading invalid data and hope to convince other tools to validate their output
- Make this library (and perhaps the spec? I don't know if it's in there) lax and make hash include
bom-ref - Implement a strictness setting functioning as a switch between hash calcs with or without
bom-ref
The first option might be an uphill battle, while the other two will most likely mean that other tools will not be changed.
However, perhaps it isn't very likely that other tools will be changed - that might depend on what's in the spec exactly.
Option three is probably the worst, as it would both complicate implementation but still remove incentive for other tools.It would be easier if there was a working
mergeordeduplicateimplementation that could be referred to :-)
I also found that such an operation can be rather expensive on my dataset of ~30 sbom files (totalling about 18 MB raw json). Runningcyclonedx-cli mergetakes 4-6 minutes, a correctly deduplicated merge on the same dataset takes about 40 minutes.I believe that making the bom-ref a part of
__hash__and therefore the equality operator would solve this.You could add your example to this very project's test cases and alter the code to showcase this.
Feel free to pullrequest a working example.I think there are three options:
- Throw an error when loading invalid data and hope to convince other tools to validate their output
- Make this library (and perhaps the spec? I don't know if it's in there) lax and make hash include
bom-ref - Implement a strictness setting functioning as a switch between hash calcs with or without
bom-ref
re:
- throwing an error: nope, deserialization is best-effort and tries hard to generate data structures in a very forgiving way.(maybe throwing warnings can be a thing)
- [...] lax and make hash include
bom-ref: feel free to open a pullrequest that includes tests showcasing this helps your issue. - Implement a strictness setting: feel free to open a pullrequest that includes tests showcasing this helps your issue.
It would be easier if there was a working merge or deduplicate implementation that could be referred to :-)
There is just no agreement/consensus on the merging algorithms - see CycloneDX/specification#320.
Such an implementation would not be part of the core library for a long time, until it has proven general value and overall correctness. (Many implemented their own/custom merge approach fitting their needs, utilizing this very library - you may do so, too)Reacted by wkoot- added 5 commits that reference this issue
on Sep 19, 2024 - added a commit that references this issue
on Feb 12, 2025 A preview of the fix/feature is available via https://github.com/CycloneDX/cyclonedx-python-lib/releases/tag/v9.0.1-rc.1
Verified working in
cyclonedx-python-lib==9.1.0, thanks!There is however a new error upon validating the testdata originally posted, I've updated the sample
It seems that deserialization silently loads broken bom data, omitting components due to deduplication.
For reproduction, refer to the sample testdata output as produced in CycloneDX/cyclonedx-cli#399:
{ "bomFormat": "CycloneDX", "specVersion": "1.5", "serialNumber": "urn:uuid:66fa5692-2e9d-45c5-830a-ec8ccaf7dcc9", "version": 1, "metadata": { "component": { "type": "application", "bom-ref": "root", "name": "test" } }, "components": [ { "type": "operating-system", "bom-ref": "test12", "name": "alpine" }, { "type": "container", "bom-ref": "test11", "name": "alpine" }, { "type": "operating-system", "bom-ref": "test22", "name": "alpine" }, { "type": "container", "bom-ref": "test21", "name": "alpine" } ], "dependencies": [ { "ref": "root", "dependsOn": [ "test11", "test21" ] }, { "ref": "test11", "dependsOn": [ "test12" ] }, { "ref": "test21", "dependsOn": [ "test22" ] } ] }Deserialize and validate bom:
Because
Bom.from_json()doesn't throw an error, there's no telling what data is actually lost during deserialization.