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.

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

refactor(sbom): add intermediate representation for BOM #6240

Merged
merged 14 commits into from
Mar 12, 2024

Conversation

knqyf263
Copy link
Collaborator

@knqyf263 knqyf263 commented Mar 1, 2024

Overview

This PR introduces refactoring of the SBOM implementation, which is complicated now. The primary focus is to abstract the intermediate representation of BOMs, transition from a CycloneDX-specific model to a more generic structure, and pave the way for supporting additional formats like SPDX with increased efficiency and reduced code duplication.

Key Changes

  • Introduction of core.BOM: To address the limitations of the tightly coupled implementation with CycloneDX, we've introduced a new intermediate representation, core.BOM. This abstraction not only facilitates a cleaner and more generic implementation but also simplifies future extensions to support other standards.
  • Decoupling and Reducing Code Duplication: Previously, decoding directly from CycloneDX and SPDX to types.SBOM led to redundant code and disparities in feature support between the two formats. By converting to core.BOM first, we streamline the decoding process, ensuring that improvements and fixes benefit both formats equally.
  • Preparation for SPDX Support: While this PR lays the groundwork for converting BOM representations to and from SPDX, the actual implementation of SPDX conversion will be addressed in a subsequent PR, furthering our goal of supporting multiple SBOM standards efficiently.
  • Unique Identifier Handling: Recognizing the issue of non-unique BOM-Refs in CycloneDX, we have implemented a strategy to include file paths in PURL qualifiers. This PR changes the way of handling colliding BOM-Refs by preparing to switch to UUIDs in cases where PURL collisions occur within an SBOM.

Next Steps

This PR focuses on introducing a more loosely coupled intermediate representation and incorporating reverse conversion capabilities. The conversion to SPDX, alongside additional enhancements and optimizations, will be covered in upcoming pull requests.

Design

Current

image

Goal

image

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

Signed-off-by: knqyf263 <knqyf263@gmail.com>
@knqyf263 knqyf263 self-assigned this Mar 1, 2024
Signed-off-by: knqyf263 <knqyf263@gmail.com>
Signed-off-by: knqyf263 <knqyf263@gmail.com>
Signed-off-by: knqyf263 <knqyf263@gmail.com>
Signed-off-by: knqyf263 <knqyf263@gmail.com>
Signed-off-by: knqyf263 <knqyf263@gmail.com>
Signed-off-by: knqyf263 <knqyf263@gmail.com>
@knqyf263 knqyf263 marked this pull request as ready for review March 2, 2024 14:24
Copy link
Contributor

@chen-keinan chen-keinan left a comment

Choose a reason for hiding this comment

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

@knqyf263 lgtm , I have added some nit comments

@@ -379,3 +381,7 @@ func (t *Package) UnmarshalJSONWithMetadata(node jfather.Node) error {
t.EndLine = node.Range().End.Line
return nil
}

func packageID(name, version string) string {
return dependency.ID(ftypes.Npm, name, version)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:
keep function call consist, in some cases dependency.ID called directly in some cases it has a wrapper packageID in another case there is a wrapper ID

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it is called once, I'd call dependency.ID directly. If it is called several times. I defined packageID. As you said, ID and packageID should be aligned. Fixed in 0e025f9. Thanks!

Copy link
Contributor

@DmitriyLewen DmitriyLewen Mar 11, 2024

Choose a reason for hiding this comment

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

Can you help me understand why we can't just always use dependency.ID?


If it is called once, I'd call dependency.ID directly. If it is called several times. I defined packageID

Add packageID:

  • pkg/dependency/parser/java/pom/parse.go

Use dependency.ID:

  • pkg/dependency/parser/swift/swift/parse.go

Copy link
Contributor

Choose a reason for hiding this comment

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

@knqyf263 take a look comment above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. But it is not that strict policy. It won't end the world even if we don't follow it.
1057d92
5bfb04c

pkg/purl/purl.go Outdated Show resolved Hide resolved
pkg/sbom/io/encode_test.go Show resolved Hide resolved
pkg/sbom/io/decode.go Outdated Show resolved Hide resolved
pkg/sbom/io/decode.go Outdated Show resolved Hide resolved
pkg/sbom/io/decode.go Outdated Show resolved Hide resolved
@knqyf263 knqyf263 mentioned this pull request Mar 4, 2024
6 tasks
Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

Looks good. Left 2 comments.

pkg/dependency/id.go Outdated Show resolved Hide resolved
pkg/fanal/artifact/image/remote_sbom_test.go Show resolved Hide resolved
knqyf263 and others added 4 commits March 8, 2024 14:24
Co-authored-by: DmitriyLewen <91113035+DmitriyLewen@users.noreply.github.com>
Signed-off-by: knqyf263 <knqyf263@gmail.com>
Signed-off-by: knqyf263 <knqyf263@gmail.com>
Signed-off-by: knqyf263 <knqyf263@gmail.com>
@codefromthecrypt
Copy link
Contributor

@knqyf263 what do you use to draw that diagram? looks neat.

@knqyf263
Copy link
Collaborator Author

@codefromthecrypt This one https://excalidraw.com/

Signed-off-by: knqyf263 <knqyf263@gmail.com>
@knqyf263
Copy link
Collaborator Author

@DmitriyLewen @chen-keinan I've resolved all comments. Can you please take another look?

Signed-off-by: knqyf263 <knqyf263@gmail.com>
Signed-off-by: knqyf263 <knqyf263@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants