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

Added SPDX format support in sbom scripts [CLOUDDST-24250] #163

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Added SPDX format support in sbom scripts [CLOUDDST-24250] #163

wants to merge 12 commits into from

Conversation

midnightercz
Copy link

@midnightercz midnightercz commented Sep 17, 2024 •
edited
Loading

Added support of SPDX format for helper scripts manipulating SBOMs [CLOUDDST-24250]
Related to ADR 0039-spdx-support

chmeliik
chmeliik reviewed Sep 19, 2024
View reviewed changes
Copy link
Contributor

@chmeliik chmeliik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the contribution!

Could you give the PR a more thorough description and link the relevant issues, docs etc.? There's a lot of guesswork for us here.

(Reviewed only base_images_sbom_script.py so far)

sbom-utility-scripts/scripts/base-images-sbom-script/app/base_images_sbom_script.py Outdated Show resolved Hide resolved
sbom-utility-scripts/scripts/base-images-sbom-script/app/base_images_sbom_script.py Outdated Show resolved Hide resolved
sbom-utility-scripts/scripts/base-images-sbom-script/app/base_images_sbom_script.py Outdated Show resolved Hide resolved
sbom-utility-scripts/scripts/base-images-sbom-script/app/base_images_sbom_script.py Outdated Show resolved Hide resolved
sbom-utility-scripts/scripts/base-images-sbom-script/app/base_images_sbom_script.py Outdated Show resolved Hide resolved
@chmeliik chmeliik mentioned this pull request Sep 27, 2024
Merged
@midnightercz midnightercz requested a review from chmeliik October 1, 2024 09:33
@mkosiarc
Copy link
Contributor

mkosiarc commented Oct 8, 2024

If the newly added scripts are to be used in the buildah task in konflux, they need to be added to the Dockerfile as well - https://github.com/konflux-ci/build-tasks-dockerfiles/blob/main/sbom-utility-scripts/Dockerfile

This means it will be part of the image that is used in the individual steps that are working with sbom in the buildah task, e.g. https://github.com/konflux-ci/build-tasks-dockerfiles/blob/main/sbom-utility-scripts/Dockerfile

@mkosiarc
Copy link
Contributor

mkosiarc commented Oct 8, 2024

Is this work tracked internally somewhere?

@mkosiarc
Copy link
Contributor

mkosiarc commented Oct 8, 2024

Seems that the CI did not run automatically because you are not a maintainer. I had to approve it and it seems to run now. Sorry about that.

The new testcase is failing:

        else:
>           root_element1, map1, inverse_map1 = map_relationships(sbom['relationships'])
E           KeyError: 'relationships'
base_images_sbom_script.py:165: KeyError
=========================== short test summary info ============================
FAILED test_base_images_sbom_script.py::test_main_input_sbom_spdx_minimal - K...

You can also test this locally with tox -e test

@midnightercz
Copy link
Author

Is this work tracked internally somewhere?

It's tracked in CLOUDDST-24250, I created the issue after I started working on this, so that's why it's not in the commit mesage

mkosiarc
mkosiarc reviewed Oct 10, 2024
View reviewed changes
sbom-utility-scripts/scripts/base-images-sbom-script/app/base_images_sbom_script.py Outdated
if contains and inverse_map1.get(r) == root_element1:
middle_element1 = r
# if not middle_element1:
# middle_element1 = root_element1
Copy link
Contributor

Choose a reason for hiding this comment

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

this is probably left by mistake?

@mkosiarc
Copy link
Contributor

@chmeliik does @midnightercz need to be added to the rhtap-build namespace, so that the pipeline can be triggered from this PR?

@chmeliik
Copy link
Contributor

chmeliik commented Oct 10, 2024 •
edited
Loading

just needs /ok-to-test no?

Huh, maybe not 😕

@chmeliik
Copy link
Contributor

/retest

@chmeliik
Copy link
Contributor

/ok-to-test

@mkosiarc
Copy link
Contributor

Is this work tracked internally somewhere?

It's tracked in CLOUDDST-24250, I created the issue after I started working on this, so that's why it's not in the commit mesage

You could always edit the commit message and make life easier for everybody in the future, but now it is at least mentioned in the pull request.

@mkosiarc
Copy link
Contributor

/ok-to-test

@mmorhun
Copy link
Collaborator

mmorhun commented Oct 11, 2024

/ok-to-test

@midnightercz midnightercz changed the title Added SPDX format support in sbom scripts Added SPDX format support in sbom scripts [CLOUDDST-24250] Oct 15, 2024
tkdchen
tkdchen reviewed Oct 16, 2024
View reviewed changes
Copy link
Contributor

@tkdchen tkdchen left a comment

Choose a reason for hiding this comment

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

Please describe the problem, which this PR addresses, in detail in the commit message of 1b01170. The information may be about what is missing by current implementation, what's the purpose of adding SPDX format support, etc.

@midnightercz
Copy link
Author

Please describe the problem, which this PR addresses, in detail in the commit message of 1b01170. The information may be about what is missing by current implementation, what's the purpose of adding SPDX format support, etc.

I added information about ADR describing the problem

This was referenced Nov 1, 2024
Open
Open
@zregvart
Copy link
Member

What can be done to get this merged? The EC team is awaiting this work to be done before proceeding with the full SPDX support.

@chmeliik
Copy link
Contributor

What can be done to get this merged? The EC team is awaiting this work to be done before proceeding with the full SPDX support.

Currently the SPDX feature is in design phase konflux-ci/architecture#213

@midnightercz
Copy link
Author

@chmeliik @zregvart I added changes from ADR in last commit + some explanatory comments. I also added test to compare merged cyclonedx and spdx to ensure they are equal (in sense of packages and purls).
Also, question: Is merge_syft_sboms_spdx.py used anywhere? It looks like it's not used in build-definitions repo. Is it possible it's deprecated script?

@zregvart
Copy link
Member

Also, question: Is merge_syft_sboms_spdx.py used anywhere? It looks like it's not used in build-definitions repo. Is it possible it's deprecated script?

I've found no use of it anywhere

chmeliik
chmeliik reviewed Dec 2, 2024
View reviewed changes
sbom-utility-scripts/scripts/merge-cachi2-sboms-script/merge_cachi2_sboms.py
or set(keys) & set(cachi2_indexed_packages.keys())
)

return package_is_duplicated
Copy link
Contributor

Choose a reason for hiding this comment

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

The whole package de-duplication approach for SPDX (the entire diff up to this function) is a bit overcomplicated and adds lots of code duplication.

Both syft and cachi2 never generate more than one purl for a package, and the purl format doesn't change between CDX and SPDX. So the deduplication logic should be identical between CDX and SPDX. The only difference should be whether we get the purl from .purl or from .externalReferences with .referenceType == "purl"

Copy link
Author

Choose a reason for hiding this comment

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

code duplication is intentional. Once we remove cyclonedx support, we can simply remove all non spdx related functions. If we enforce to support only one purl per package then logic could be simplified

Copy link
Contributor

Choose a reason for hiding this comment

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

If we enforce to support only one purl per package then logic could be simplified

yes please 🙏

sbom-utility-scripts/scripts/merge-cachi2-sboms-script/merge_cachi2_sboms.py
Comment on lines +476 to +465
filtered_packages = []
for p in syft_sbom.get("packages", []):
if is_duplicate_package(p):
if get_package_key(p) in cachi2_packages_map:
cpackage = cachi2_packages_map[get_package_key(p)]
cpackage["externalRefs"] = sorted(
merge_external_refs(cpackage.get("externalRefs", []), p.get("externalRefs", [])),
key=lambda x: (
x["referenceCategory"],
x["referenceType"],
x["referenceLocator"],
),
)
cpackage["annotations"] = merge_annotations(cpackage.get("annotations", []), p.get("annotations", []))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we merging the duplicated packages together for SPDX when we don't do it for CycloneDX? For CycloneDX, we simply drop the duplicated packages

Copy link
Author

Choose a reason for hiding this comment

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

for cyclonedx if the component is duplicated it's based on purl. That means the component is duplicate for sure. With SPDX considering possibility of multiple purls, is_duplicate_package returns True even on partial match of purls. Therefore considered removal of duplicated packages as possible information loss and that's why attributes of the duplicate package are merged with existing one instead of discarding whole package

sbom-utility-scripts/scripts/base-images-sbom-script/app/base_images_sbom_script.py Outdated Show resolved Hide resolved
sbom-utility-scripts/scripts/merge-cachi2-sboms-script/merge_cachi2_sboms.py Outdated Show resolved Hide resolved
sbom-utility-scripts/scripts/merge-cachi2-sboms-script/merge_cachi2_sboms.py Outdated Show resolved Hide resolved
sbom-utility-scripts/scripts/merge-cachi2-sboms-script/merge_cachi2_sboms.py
Comment on lines +462 to +439
if relation["relatedSpdxElement"] in package_ids:
relationships.append(_relation)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we only look at relatedSpdxElement for document 1 when we look at both spdxElementId and relatedSpdxElement for document 2?

Copy link
Author

Choose a reason for hiding this comment

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

Because in the first doc spdxElementId can be root element and in that case even relationships pointing to removed packages would be included

sbom-utility-scripts/scripts/merge-cachi2-sboms-script/merge_cachi2_sboms.py Outdated Show resolved Hide resolved
@midnightercz
Added SPDX format support in sbom scripts
0863133 
Signed-off-by: Jindrich Luza <[email protected]>
@midnightercz
Added test files
277939a 
Signed-off-by: Jindrich Luza <[email protected]>
@midnightercz
Fixed docs
53d2227 
Signed-off-by: Jindrich Luza <[email protected]>
@midnightercz
- Added create_purl_sbom_spdx.py
435923f 
- Added merge_syft_sboms_spdx.py
@midnightercz
- Fixed datetime used
ae37225 
- Added format explanatory comments
@midnightercz
Fixed BUILD_TOOL_OF relationship generation
8f316eb 
Signed-off-by: Jindrich Luza <[email protected]>
@midnightercz
Added SPDXID calculation comment
f3a9410 
Signed-off-by: Jindrich Luza <[email protected]>
@midnightercz
Added more explanatory comments and map relationships documentation
81d7cf7 
Signed-off-by: Jindrich Luza <[email protected]>
@midnightercz
Removed stray code and comments
4fd6e23 
Signed-off-by: Jindrich Luza <[email protected]>
@midnightercz
- Added :jsonencoded suffix to coverted spdx properties
a6ccb65 
- Formated test spdx files
- Improved merging spdx files
- Added test to compared merged cdx and spdx outputs
@midnightercz
- Added format autodetection
66625d7 
- Renamed middle element to root package
- Added mandatory attributes to spdx elements

Signed-off-by: Jindrich Luza <[email protected]>
@midnightercz
- Simplified root package discovery
14b867d 
- spdx_find_doc_and_root_package wraps spdx related code
- fixed isodate generator
- fixed annotator string
- build deps are generated as SPDXRef-Image- instead of SPDXRef-container-
- added test_main_input_sbom_spdx_with_packages

Signed-off-by: Jindrich Luza <[email protected]>
@chmeliik chmeliik mentioned this pull request Dec 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkosiarc mkosiarc mkosiarc left review comments

@tkdchen tkdchen Awaiting requested review from tkdchen

@chmeliik chmeliik Awaiting requested review from chmeliik

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@midnightercz @mkosiarc @chmeliik @mmorhun @zregvart @tkdchen