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?

DmitriyLewen

@knqyf263 left comments

DmitriyLewen · 2024-09-16T05:07:57Z

pkg/sbom/io/encode.go

+	components := lo.MapKeys(report.BOM.Components(), func(value *core.Component, _ uuid.UUID) string {
+		return value.PkgIdentifier.BOMRef
+	})


It doesn't work for SPDX.

I thought about using SPDX-ID as bom-ref (see #6907 (comment)).

also I realized that there might be issues with non-Trivy CycloneDX reports:
Bom-Ref is an optional field - https://cyclonedx.org/docs/1.6/json/#components_items_bom-ref
So there might be a case where bom-ref doesn't exist - so we won't add vulnerabilities for these components.

Bom-Ref is an optional field - https://cyclonedx.org/docs/1.6/json/#components_items_bom-ref

It's interesting. I was not aware of that. However, vulnerabilities.affects.ref must be BOM-Ref. Do you think we should generate BOM-Refs in this case?
https://cyclonedx.org/docs/1.6/json/#vulnerabilities_items_affects_items_ref

It doesn't work for SPDX.

Hmm. I didn't think SPDX supports vulnerabilities, but I just remember we added support. I'll think about it.
#7213

However, vulnerabilities.affects.ref must be BOM-Re

This is a weird case. I still don't understand why Bom-ref is not a required field.

Looks like we need to separate the possible cases:

sbom result without vulnerabilities

vuln scanner is disabled - we can skip adding bom-ref to preserve the scanned sbom file as much as possible.

Trivy found no vulnerabilities - I'm not sure what we should do. I'm more inclined to think that we still need to add bom-refs (use the same logic when vuln scanner is enabled)

sbom result with vulnerabilities - vulnerabilities.affects.ref is a required field => we need to populate this field => we need to use the same bom-ref for the linked component.

Yes, I agree BOM-Ref should be required.

Looks like we need to separate the possible cases:

To keep things simple, IMO, it is better to generate a BOM-Ref if it's missing in CycloneDX that Trivy is scanning.

since I don't have statistics on how often CycloneDX files without bom-refs are used - it's hard for me to make a choice.
I'm not against your decision. Let's do it this way and get feedback from users.

I wanted to reuse the SBOM of the input as much as possible, but I found that my implementation did not work well when using --pkg-relationships or --pkg-types with trivy sbom.

DmitriyLewen · 2024-09-16T05:15:27Z

integration/sbom_test.go

@@ -165,6 +176,7 @@ func TestSBOM(t *testing.T) {
 			// Run "trivy sbom"
 			runTest(t, osArgs, tt.golden, outputFile, types.Format(tt.args.format), runOptions{
 				override: overrideFuncs(overrideSBOMReport, overrideUID, tt.override),
+				fakeUUID: tt.fakeUUID,


It looks like we only use fakeUUID for serialNumber:

trivy/integration/testdata/fluentd-multiple-lockfiles-reused.cdx.json.golden

Line 5 in 8b33309

"serialNumber": "urn:uuid:3ff14136-e09f-4df9-80ea-000000000006",

This might be confusing (that we don't overwrite other UUID's).
Maybe we just don't check serialNumber?
@knqyf263 @afdesk wdyt?

The serial number is generated by UUID. I think it's better to confirm it is generated as expected rather than ignoring it.

pkg/sbom/core/bom.go

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

This reverts commit 67961d7.

This reverts commit 8b33309.

This reverts commit 0bb6387.

This reverts commit 418100c.

knqyf263 · 2024-09-18T10:46:23Z

I reverted my changes. I'll open another PR.

…7340) Signed-off-by: knqyf263 <[email protected]> Co-authored-by: amf <[email protected]> Co-authored-by: knqyf263 <[email protected]>

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

DmitriyLewen reviewed Sep 16, 2024

View reviewed changes

knqyf263 added 5 commits September 16, 2024 09:55

refactor: rename AddVulnerabilities to SetVulnerabilities

67961d7

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

Revert "refactor: rename AddVulnerabilities to SetVulnerabilities"

13b0ff4

This reverts commit 67961d7.

Revert "fix: linter error"

e67a91e

This reverts commit 8b33309.

Revert "test: add appropriate parsed BOMs"

28c45d8

This reverts commit 0bb6387.

Revert "refactor: reuse the parsed SBOM"

f4478c0

This reverts commit 418100c.

knqyf263 enabled auto-merge September 18, 2024 10:46

knqyf263 added this pull request to the merge queue Sep 18, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 18, 2024

knqyf263 added this pull request to the merge queue Sep 18, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 18, 2024

knqyf263 added this pull request to the merge queue Sep 18, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 18, 2024

knqyf263 added this pull request to the merge queue Sep 19, 2024

Merged via the queue into aquasecurity:main with commit 5dd94eb Sep 19, 2024
12 checks passed

aqua-bot mentioned this pull request Sep 18, 2024

release: v0.56.0 [main] #7447

Merged

This was referenced Sep 19, 2024

fix(sbom): detect main OS and ignore pkgs for other OSes #6907

Open

feat(sbom): keep internal uuid to identify components #7550

Draft

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

DmitriyLewen left a comment

DmitriyLewen Sep 16, 2024

DmitriyLewen Sep 16, 2024

knqyf263 Sep 16, 2024

knqyf263 Sep 16, 2024

DmitriyLewen Sep 16, 2024

knqyf263 Sep 16, 2024

DmitriyLewen Sep 16, 2024

knqyf263 Sep 16, 2024

DmitriyLewen Sep 16, 2024

knqyf263 Sep 18, 2024

knqyf263 commented Sep 18, 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

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

DmitriyLewen left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knqyf263 commented Sep 18, 2024

afdesk commented Aug 14, 2024 •

edited

Loading

CLAassistant commented Aug 15, 2024 •

edited

Loading

knqyf263 commented Aug 16, 2024 •

edited

Loading