fix(sbom): export bom-ref when converting a package to a component #7340

afdesk · 2024-08-14T11:01:06Z

Description

Reproduction Steps

$ wget https://pastebin.com/raw/iD0PiatU
$ trivy sbom --format cyclonedx --scanners vuln iD0PiatU

Before:

      "affects": [
        {
          "ref": "ca36a16f-8acd-4d6a-b9d9-6e9e265bc0d8",
          "versions": [
            {
              "version": "227",
              "status": "affected"
            }
          ]

After:

"affects": [
        {
          "ref": "pkg:pypi/pywin32@227",
          "versions": [
            {
              "version": "227",
              "status": "affected"
            }
          ]
        }
      ]

Related issues

Close Unmatched Vulnerabilities.affects.ref when scanning CycloneDX sbom with duplicate Purls #7337

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).

knqyf263 · 2024-08-15T08:50:25Z

pkg/sbom/cyclonedx/marshal_test.go

@@ -1607,7 +1607,7 @@ func TestMarshaler_MarshalReport(t *testing.T) {
 						Updated:   "2022-12-20T10:15:00+00:00",
 						Affects: &[]cdx.Affects{
 							{
-								Ref: "pkg:maven/com.fasterxml.jackson.core/[email protected]",
+								Ref: "pkg:maven/com.fasterxml.jackson.core/[email protected]?file_path=jackson-databind-2.13.4.1.jar",


We don't include file paths in PURL anymore. If it remains somewhere, it should be deleted.

ok, I'm removing it

CLAassistant · 2024-08-15T11:34:18Z

All committers have signed the CLA.

CLAassistant · 2024-08-15T11:34:19Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ afdesk
❌ amf

amf seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

knqyf263 · 2024-08-16T05:03:12Z

We should add a new test to ensure original BOM-Refs are kept as-is. In the existing test, the input is SBOM, but the output is Trivy JSON.

trivy/integration/sbom_test.go

Lines 51 to 59 in 6a72dd4

    
           { 
        
           	name: "fluentd-multiple-lockfiles cyclonedx", 
        
           	args: args{ 
        
           		input:        "testdata/fixtures/sbom/fluentd-multiple-lockfiles-cyclonedx.json", 
        
           		format:       "json", 
        
           		artifactType: "cyclonedx", 
        
           	}, 
        
           	golden: "testdata/fluentd-multiple-lockfiles.json.golden", 
        
           },

afdesk · 2024-08-18T04:12:20Z

We should add a new test to ensure original BOM-Refs are kept as-is.

The testcase is added now.

knqyf263 · 2024-08-21T03:10:06Z

integration/sbom_test.go

+		{
+			name: "scan SBOM into SBOM",
+			args: args{
+				input:        "testdata/fixtures/sbom/pywin32-cyclonedx.json",


Can we re-use the existing json, fluentd-multiple-lockfiles-cyclonedx.json? You can change this file to use UUID in BOM-Ref.

the testcase is updated fluentd-multiple-lockfiles-cyclonedx.json.
I wanted to use fluentd-multiple-lockfiles.cdx.json.golden also, but this cdx result is a large, because it's result for fluentd-multiple-lockfiles.tar.gz. fluentd-multiple-lockfiles-cyclonedx.json is shorter.
so I've created a new golden file.

DmitriyLewen

@afdesk I may have found another issue (maybe for another PR):
File iD0PiatU contains 2 same packages with different BOMRefs.
Should we show 2 BOMrefs in affects array:
something like that:

"affects": [
        {
          "ref": "pkg:pypi/pywin32@227",
          "versions": [
            {
              "version": "227",
              "status": "affected"
            }
          ]
        }
        {
          "ref": "de3ojve0eoj0j0je",
          "versions": [
            {
              "version": "227",
              "status": "affected"
            }
          ]
        }
      ]

DmitriyLewen · 2024-08-23T07:00:09Z

integration/testdata/fluentd-multiple-lockfiles-short.cdx.json.golden

+      "bom-ref": "3ff14136-e09f-4df9-80ea-000000000006",
+      "type": "operating-system",
+      "name": "debian",
+      "version": "10.2",


input and output bomrefs for OS are not equial:

trivy/integration/testdata/fixtures/sbom/fluentd-multiple-lockfiles-cyclonedx.json

Lines 107 to 110 in bfdf5cf

"bom-ref": "353f2470-9c8b-4647-9d0d-96d893838dc8",

"type": "operating-system",

"name": "debian",

"version": "10.2",

afdesk · 2024-08-29T05:19:54Z

Now Trivy builds new SBOM components from the result:

trivy/pkg/sbom/io/encode.go

Line 260 in 98e136e

    
           func (e *Encoder) resultComponent(root *core.Component, r types.Result, osFound *ftypes.OS) *core.Component {

to keep existing BOM-refs we should look for this one (if report.BOM != nil) by name and save it.
it matters for non-root and non-child components.

if we don't want to use this logic I can see 2 options:

we don't keep intermediate BOM-refs. because these ones are generated UUIDs.
Trivy still updates them for empty BOM-refs.
we don't encode a result into a new BOM report, and keep existing BOM.
just will add vuln fieldsL:
SBOM -> Trivy -> the same SBOM+ vulnerability list.

@knqyf263 @DmitriyLewen wdyt? thanks

DmitriyLewen · 2024-08-29T06:53:06Z

to keep existing BOM-refs we should look for this one (if report.BOM != nil) by name and save it.
it matters for non-root and non-child components.

I think we can try to use this way.
But we need to save operating-system here:

trivy/pkg/sbom/io/decode.go

Lines 120 to 136 in 98e136e

    
           case core.TypeOS: 
        
           	if m.osID != uuid.Nil { 
        
           		onceMultiOSWarn() 
        
           		continue 
        
           	} 
        
           	m.osID = id 
        
           	sbom.Metadata.OS = &ftypes.OS{ 
        
           		Family: ftypes.OSType(c.Name), 
        
           		Name:   c.Version, 
        
           	} 
        
           	continue 
        
           case core.TypeApplication: 
        
           	if app := m.decodeApplication(c); app.Type != "" { 
        
           		m.apps[id] = app 
        
           		continue 
        
           	} 
        
           }

we don't keep intermediate BOM-refs.

Do you mean BOMrefs for #2?

trivy/pkg/sbom/io/encode.go

Lines 160 to 173 in 98e136e

    
           //   Container component (alpine:3.15) --------------------- #1 
        
           //     -> Operating System Component (Alpine Linux 3.15) --- #2 
        
           //       -> Library component (bash-4.12) ------------------ #3 
        
           //       -> Library component (vim-8.2)   ------------------ #3 
        
           //       -> etc. 
        
           // 
        
           // Else if a package is language-specific package associated with a lock file, 
        
           // it will be a dependency of "Application" component. 
        
           // e.g. 
        
           //   Container component (alpine:3.15) ------------------------ #1 
        
           //     -> Application component (/app/package-lock.json) ------ #2 
        
           //       -> Library component (npm package, express-4.17.3) --- #3 
        
           //       -> Library component (npm package, lodash-4.17.21) --- #3 
        
           //       -> etc.

we don't encode a result into a new BOM report, and keep existing BOM.
just will add vuln fieldsL:
SBOM -> Trivy -> the same SBOM+ vulnerability list.

I am not sure about this.
This will be work with Trivy reports, but we may see problem with SBOM files from other sources.

afdesk · 2024-08-29T06:58:20Z

Do you mean BOMrefs for #2?

yes, for Application component (/app/package-lock.json)

afdesk · 2024-08-30T08:41:20Z

@DmitriyLewen could you take a look? thanks

DmitriyLewen · 2024-08-30T09:09:45Z

pkg/sbom/cyclonedx/unmarshal.go

 	if c.PackageURL != "" {
-		purl, err = packageurl.FromString(c.PackageURL)
+		purl = &packageurl.PackageURL{}


Looks like we can remove this

it seems that no, we have to allocate memory

DmitriyLewen · 2024-08-30T09:14:55Z

pkg/sbom/io/encode.go

@@ -283,6 +288,14 @@ func (e *Encoder) resultComponent(root *core.Component, r types.Result, osFound
 		component.Type = core.TypeApplication
 	}

+	// try to look for BOM-ref for this component
+	for _, c := range e.components {
+		if c.Name == component.Name && c.Type == component.Type {


IIRC we correctly store BomRefs for the Root component and packages.
Do we need this check for all types? maybe we can do this check only for Application and OperatingSystem types?

DmitriyLewen

LGTM

@knqyf263 take a look, when you have time

Signed-off-by: knqyf263 <[email protected]>

knqyf263 · 2024-09-13T09:43:02Z

I have refactored. @DmitriyLewen @afdesk Can you please take a look?

afdesk added 2 commits August 14, 2024 16:58

fix(sbom): export bom-ref

73a0018

fix tests

f22f6b4

afdesk changed the title ~~fix(sbom): export bom-ref~~ fix(sbom): export bom-ref when converting a package to a component Aug 14, 2024

afdesk marked this pull request as ready for review August 14, 2024 22:05

afdesk requested a review from knqyf263 as a code owner August 14, 2024 22:05

knqyf263 reviewed Aug 15, 2024

View reviewed changes

remove file path

0eebb2a

afdesk force-pushed the fix/sbom-comps branch from 17c76c8 to 0eebb2a Compare August 15, 2024 11:45

afdesk requested a review from knqyf263 August 15, 2024 12:10

add test case

46e10ac

knqyf263 reviewed Aug 21, 2024

View reviewed changes

fix: update test data

5cb120b

afdesk requested a review from DmitriyLewen as a code owner August 21, 2024 18:27

afdesk requested a review from knqyf263 August 22, 2024 04:51

DmitriyLewen reviewed Aug 23, 2024

View reviewed changes

afdesk marked this pull request as draft August 23, 2024 07:42

test: keep the same uid

4e42d28

afdesk added 2 commits August 30, 2024 13:51

fix: keep existing BOM-ref

61486f3

add: a check

6067d5f

afdesk marked this pull request as ready for review August 30, 2024 08:40

afdesk requested a review from DmitriyLewen August 30, 2024 08:40

DmitriyLewen reviewed Aug 30, 2024

View reviewed changes

refactor: look for SBOM ref only for Os and App layers

0a5090f

afdesk requested a review from DmitriyLewen August 30, 2024 10:30

refactor

eaaf98b

DmitriyLewen approved these changes Aug 30, 2024

View reviewed changes

knqyf263 added 3 commits September 13, 2024 12:00

refactor: reuse the parsed SBOM

418100c

Signed-off-by: knqyf263 <[email protected]>

test: add appropriate parsed BOMs

0bb6387

Signed-off-by: knqyf263 <[email protected]>

fix: linter error

8b33309

Signed-off-by: knqyf263 <[email protected]>

knqyf263 requested a review from DmitriyLewen September 13, 2024 09:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sbom): export bom-ref when converting a package to a component #7340

fix(sbom): export bom-ref when converting a package to a component #7340

afdesk commented Aug 14, 2024 •

edited

Loading

knqyf263 Aug 15, 2024

afdesk Aug 15, 2024

CLAassistant commented Aug 15, 2024 •

edited

Loading

CLAassistant commented Aug 15, 2024

knqyf263 commented Aug 16, 2024 •

edited

Loading

afdesk commented Aug 18, 2024

knqyf263 Aug 21, 2024

afdesk Aug 22, 2024

DmitriyLewen left a comment

DmitriyLewen Aug 23, 2024

afdesk commented Aug 29, 2024

DmitriyLewen commented Aug 29, 2024

afdesk commented Aug 29, 2024

afdesk commented Aug 30, 2024

DmitriyLewen Aug 30, 2024

afdesk Aug 30, 2024

afdesk Aug 30, 2024

DmitriyLewen Aug 30, 2024

afdesk Aug 30, 2024

DmitriyLewen left a comment

knqyf263 commented Sep 13, 2024

	"bom-ref": "353f2470-9c8b-4647-9d0d-96d893838dc8",
	"type": "operating-system",
	"name": "debian",
	"version": "10.2",

fix(sbom): export bom-ref when converting a package to a component #7340

Are you sure you want to change the base?

fix(sbom): export bom-ref when converting a package to a component #7340

Conversation

afdesk commented Aug 14, 2024 • edited Loading

Description

Related issues

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Aug 15, 2024 • edited Loading

CLAassistant commented Aug 15, 2024

knqyf263 commented Aug 16, 2024 • edited Loading

afdesk commented Aug 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitriyLewen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afdesk commented Aug 29, 2024

DmitriyLewen commented Aug 29, 2024

afdesk commented Aug 29, 2024

afdesk commented Aug 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitriyLewen left a comment

Choose a reason for hiding this comment

knqyf263 commented Sep 13, 2024

afdesk commented Aug 14, 2024 •

edited

Loading

CLAassistant commented Aug 15, 2024 •

edited

Loading

knqyf263 commented Aug 16, 2024 •

edited

Loading