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

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

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

Conversation

DmitriyLewen
Copy link
Contributor

@DmitriyLewen DmitriyLewen commented Jun 11, 2024

Description

Trivy doesn't currently support multiple OSes.
To avoid flaky packages - we need to sort found OS and select only one of them (packages for other OS should be ignored).

Sorting order:

  1. Take the OS with the largest number of packages.
  2. If OSes contain same number of packages, we compare them by SPDXID/BOMRef.

example flaky pkgs (nginx-helm.json file(see #5812 (comment)):

➜  trivy sbom nginx-helm.json --quiet --format json --list-all-pkgs | jq '.Results[0].Packages | length'
128
➜  trivy sbom nginx-helm.json --quiet --format json --list-all-pkgs | jq '.Results[0].Packages | length'
159

New Field

This PR adds new SPDXID field for Package.Identifier.
We populate this field from SPDX files, similar to BOMRef for CycloneDX files.

Related issues

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

@DmitriyLewen DmitriyLewen marked this pull request as ready for review June 13, 2024 08:56
@DmitriyLewen DmitriyLewen requested a review from knqyf263 as a code owner June 13, 2024 08:56
Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

I think they are not orphan packages. They belong to another OS. Since we don't support multiple OSes, we should ignore them. What do you think?

@DmitriyLewen
Copy link
Contributor Author

hmm... you're probably right, but then we must maintain order in order to exclude uncertainty (I mean that every time we scan a file with multiple OSes, we should take only the first one found)

@knqyf263
Copy link
Collaborator

knqyf263 commented Jun 20, 2024

then we must maintain order in order to exclude uncertainty

Yes, we keep OSes as an array instead of picking up the first one, sort them somehow and pick up one of them deterministically.

trivy/pkg/sbom/io/decode.go

Lines 121 to 130 in f7720f5

if m.osID != uuid.Nil {
onceMultiOSWarn()
continue
}
m.osID = id
sbom.Metadata.OS = &ftypes.OS{
Family: ftypes.OSType(c.Name),
Name: c.Version,
}
continue

@DmitriyLewen
Copy link
Contributor Author

pick up one of them deterministically.

We can take OS with the most related packages.
wdyt?

@knqyf263
Copy link
Collaborator

We can take OS with the most related packages.

Sounds good. But we must consider cases where several OSes have the same number of packages.

@DmitriyLewen
Copy link
Contributor Author

But we must consider cases where several OSes have the same number of packages.

Name fields can be same. So it looks like we need to use SPDXID to sort these OSes (and BomRef for CycloneDX).

@DmitriyLewen DmitriyLewen marked this pull request as draft June 25, 2024 11:48
@DmitriyLewen DmitriyLewen changed the title fix(sbom): fix flaky pkgs when sbom file contains orphan pkgs fix(sbom): detect main OS and remove pkgs for other OSes Jun 26, 2024
@DmitriyLewen DmitriyLewen changed the title fix(sbom): detect main OS and remove pkgs for other OSes fix(sbom): detect main OS and ignore pkgs for other OSes Jun 26, 2024
@@ -57,6 +57,7 @@ type PkgIdentifier struct {
UID string `json:",omitempty"` // Calculated by the package struct
PURL *packageurl.PackageURL `json:"-"`
BOMRef string `json:",omitempty"` // For CycloneDX
SPDXID string `json:",omitempty"` // For SPDX
Copy link
Contributor Author

@DmitriyLewen DmitriyLewen Jun 26, 2024

Choose a reason for hiding this comment

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

@knqyf263 wdyt about using BOMRef for SPDXID?
Name (I mean BOMRef) can be confusing (because BOMRef is field from CycloneDX), but using two fields for similar fields is also wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it again.
Using two same fields doesn't make sense and may lead to bugs.
So i used PkgIdentifier.BOMRef for SPDXID in 964f16a.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about the case where the input is CycloneDX, and the output is SPDX? I'm concerned BOM-Ref is mistakenly used as SPDX-ID, as we are currently trying to reuse the same ID after #7340.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right. After #7340 it can be problem.
I thought about adding SPDXID field (68abc78)
But it looks as bad way.

What if we will use struct with type + ref/ID. e.g.:

	type BomID struct {
		ID   string // UUID/purl/SPDXID
		Type BomType // cyclonedx/spdx
	}

If BomID.Type == outputType => save ID, otherwise - create new ID.

@@ -66,6 +66,7 @@ func TestUnmarshaler_Unmarshal(t *testing.T) {
},
},
},
BOMRef: "Package-b7ebaf0233f1ef7b",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

github.com/spdx/tools-golang returns Package SPDX identifier without SPDXRef- prefix.
On the one hand, docs say: "SPDXRef-"[idstring] where [idstring] is a unique string containing letters, numbers, ., and/or -. (https://spdx.github.io/spdx-spec/v2.3/package-information/#72-package-spdx-identifier-field). So id is text after SPDXRef-.
On the other hand, BOMRef will not match string from SPDXID field.

@DmitriyLewen DmitriyLewen marked this pull request as ready for review July 4, 2024 05:48
Copy link

github-actions bot commented Sep 3, 2024

This PR is stale because it has been labeled with inactivity.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Sep 3, 2024
@DmitriyLewen DmitriyLewen removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Sep 3, 2024
Copy link

This PR is stale because it has been labeled with inactivity.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Nov 19, 2024
@github-actions github-actions bot closed this Dec 9, 2024
@DmitriyLewen DmitriyLewen reopened this Dec 9, 2024
@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Dec 10, 2024
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.

2 participants