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

fix(sbom): use root package for unknown dependencies (if exists) #8104

Merged
merged 4 commits into from
Dec 23, 2024
Merged

fix(sbom): use root package for unknown dependencies (if exists) #8104

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
16daa4b
test(bom): add gobinary
DmitriyLewen Dec 16, 2024
f4692e4
fix(bom): check root parent
DmitriyLewen Dec 16, 2024
17c3735
refactor: update belongToParent logic
DmitriyLewen Dec 20, 2024
fbe7fd5
test: update tests
DmitriyLewen Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions pkg/sbom/io/encode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,20 +417,33 @@ func (*Encoder) belongToParent(pkg ftypes.Package, parents map[string]ftypes.Pac
// All packages are included in the parent
// Case 3: Relationship: known , DependsOn: unknown (e.g., go.mod without $GOPATH)
// All packages are included in the parent
// Case 4: Relationship: unknown, DependsOn: known (e.g., OS packages)
// All packages are included in the parent even if they have parents
// Case 4: Relationship: unknown, DependsOn: known (e.g., GoBinaries, OS packages)
Copy link
Collaborator

@knqyf263 knqyf263 Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Given that RelationshipUnknown has been introduced now, I'm wondering if this behavior is good in the first place.
In OS packages, packages with no parent are sure to be direct dependencies, but other packages are unknown, so it may be better not to add them as direct dependencies. Then, our logic is simple: no parent packages belong to a non-package parent (OS, Application component, etc.).

This will not be a problem when applying VEX, etc., since packages with at least one parent can always be present in the dependency tree.

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, I found compositions. We may want to fill in this field to represent incomplete relationships.

Copy link
Contributor Author

@DmitriyLewen DmitriyLewen Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

hm... you solution looks logical.
if Relationship is unknown and packages has parents - most likely it is one of two options:

  • this is direct package (like in out logic for dep tree)
  • this is orphan package and it makes sense to add it for parent (OS/Application) component.

Also, after changes, dependencies graph looks logical:
before:

{
	Ref: "3ff14136-e09f-4df9-80ea-000000000002", // OS component
	Dependencies: &[]string{
		"pkg:rpm/centos/[email protected]?arch=aarch64&distro=centos-8.3.2011&epoch=1",
		"pkg:rpm/centos/[email protected]?arch=aarch64&distro=centos-8.3.2011",
	},
},
{
	Ref: "pkg:rpm/centos/[email protected]?arch=aarch64&distro=centos-8.3.2011&epoch=1",
	Dependencies: &[]string{
		"pkg:rpm/centos/[email protected]?arch=aarch64&distro=centos-8.3.2011",
	},
},

after:

{
	Ref: "3ff14136-e09f-4df9-80ea-000000000002",
	Dependencies: &[]string{
		"pkg:rpm/centos/[email protected]?arch=aarch64&distro=centos-8.3.2011&epoch=1",
	},
},
{
	Ref: "pkg:rpm/centos/[email protected]?arch=aarch64&distro=centos-8.3.2011&epoch=1",
	Dependencies: &[]string{
		"pkg:rpm/centos/[email protected]?arch=aarch64&distro=centos-8.3.2011",
	},
},

I updated logic in 17c3735 (tests - fbe7fd5)

BTW, I found compositions. We may want to fill in this field to represent incomplete relationships.

I'm not sure about that.
This may complicate the logic of understanding and processing cyclonedx files.
Also, we need to use different logic for CycloneDX and SPDX files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I agree, but it's recommended here.

It is RECOMMENDED to leverage compositions to indicate unknown dependency graphs.

And I didn't find any other ways to represent the dependency graph is incomplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #8157

// - There is root parent: false. Packages are included in the root package (e.g. GoBinaries).
// - There is no root parent: true. All packages are included in the parent even if they have parents (e.g. OS packages).
switch {
// Case 1, 2 and 3
case len(parents[pkg.ID]) == 0:
return true
// Case 4
case pkg.Relationship == ftypes.RelationshipUnknown:
case pkg.Relationship == ftypes.RelationshipUnknown && !hasParentWithRootRelationship(pkg.ID, parents):
return true
default:
return false
}
}

// hasParentWithRootRelationship indicates that the parents contain the root package.
// Defining this is necessary to avoid including packages in the parent package instead of the root package.
// cf. https://github.com/aquasecurity/trivy/issues/8102
func hasParentWithRootRelationship(id string, parents map[string]ftypes.Packages) bool {
for _, parent := range parents[id] {
if parent.Relationship == ftypes.RelationshipRoot {
return true
}
}
return false
}

func filterProperties(props []core.Property) []core.Property {
return lo.Filter(props, func(property core.Property, _ int) bool {
return !(property.Value == "" || (property.Name == core.PropertySrcEpoch && property.Value == "0"))
Expand Down
171 changes: 171 additions & 0 deletions pkg/sbom/io/encode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,61 @@ func TestEncoder_Encode(t *testing.T) {
},
},
},
{
Target: "trivy",
Type: ftypes.GoBinary,
Class: types.ClassLangPkg,
Packages: []ftypes.Package{
{
ID: "github.com/aquasecurity/[email protected]",
Name: "github.com/aquasecurity/trivy",
Version: "v0.57.1",
Identifier: ftypes.PkgIdentifier{
UID: "106fee7e57f0b952",
PURL: &packageurl.PackageURL{
Type: packageurl.TypeGolang,
Namespace: "github.com/aquasecurity",
Name: "trivy",
Version: "v0.57.1",
},
},
Relationship: ftypes.RelationshipRoot,
DependsOn: []string{
"github.com/aquasecurity/[email protected]",
"[email protected]",
},
},
{
ID: "[email protected]",
Name: "stdlib",
Version: "v1.22.9",
Identifier: ftypes.PkgIdentifier{
UID: "62e7c8aaebd94b1e",
PURL: &packageurl.PackageURL{
Type: packageurl.TypeGolang,
Name: "stdlib",
Version: "v1.22.9",
},
},
Relationship: ftypes.RelationshipDirect,
},
{
ID: "github.com/aquasecurity/[email protected]",
Name: "github.com/aquasecurity/go-version",
Version: "v0.0.0-20240603093900-cf8a8d29271d",
Identifier: ftypes.PkgIdentifier{
UID: "350aed171d8ebed5",
PURL: &packageurl.PackageURL{
Type: packageurl.TypeGolang,
Namespace: "github.com/aquasecurity",
Name: "go-version",
Version: "v0.0.0-20240603093900-cf8a8d29271d",
},
},
Relationship: ftypes.RelationshipUnknown,
},
},
},
},
},
wantComponents: map[uuid.UUID]*core.Component{
Expand Down Expand Up @@ -351,6 +406,100 @@ func TestEncoder_Encode(t *testing.T) {
BOMRef: "3ff14136-e09f-4df9-80ea-000000000006",
},
},
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000007"): {
Type: core.TypeApplication,
Name: "trivy",
Properties: []core.Property{
{
Name: core.PropertyClass,
Value: "lang-pkgs",
},
{
Name: core.PropertyType,
Value: "gobinary",
},
},
PkgIdentifier: ftypes.PkgIdentifier{
BOMRef: "3ff14136-e09f-4df9-80ea-000000000007",
},
},
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000008"): {
Type: core.TypeLibrary,
Name: "github.com/aquasecurity/trivy",
Version: "v0.57.1",
SrcFile: "trivy",
Properties: []core.Property{
{
Name: core.PropertyPkgID,
Value: "github.com/aquasecurity/[email protected]",
},
{
Name: core.PropertyPkgType,
Value: "gobinary",
},
},
PkgIdentifier: ftypes.PkgIdentifier{
UID: "106fee7e57f0b952",
PURL: &packageurl.PackageURL{
Type: packageurl.TypeGolang,
Namespace: "github.com/aquasecurity",
Name: "trivy",
Version: "v0.57.1",
},
BOMRef: "pkg:golang/github.com/aquasecurity/[email protected]",
},
},
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000009"): {
Type: core.TypeLibrary,
Name: "stdlib",
Version: "v1.22.9",
SrcFile: "trivy",
Properties: []core.Property{
{
Name: core.PropertyPkgID,
Value: "[email protected]",
},
{
Name: core.PropertyPkgType,
Value: "gobinary",
},
},
PkgIdentifier: ftypes.PkgIdentifier{
UID: "62e7c8aaebd94b1e",
PURL: &packageurl.PackageURL{
Type: packageurl.TypeGolang,
Name: "stdlib",
Version: "v1.22.9",
},
BOMRef: "pkg:golang/[email protected]",
},
},
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000010"): {
Type: core.TypeLibrary,
Name: "github.com/aquasecurity/go-version",
Version: "v0.0.0-20240603093900-cf8a8d29271d",
SrcFile: "trivy",
Properties: []core.Property{
{
Name: core.PropertyPkgID,
Value: "github.com/aquasecurity/[email protected]",
},
{
Name: core.PropertyPkgType,
Value: "gobinary",
},
},
PkgIdentifier: ftypes.PkgIdentifier{
UID: "350aed171d8ebed5",
PURL: &packageurl.PackageURL{
Type: packageurl.TypeGolang,
Namespace: "github.com/aquasecurity",
Name: "go-version",
Version: "v0.0.0-20240603093900-cf8a8d29271d",
},
BOMRef: "pkg:golang/github.com/aquasecurity/[email protected]",
},
},
},
wantRels: map[uuid.UUID][]core.Relationship{
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000001"): {
Expand All @@ -366,6 +515,10 @@ func TestEncoder_Encode(t *testing.T) {
Dependency: uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000006"),
Type: core.RelationshipContains,
},
{
Dependency: uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000007"),
Type: core.RelationshipContains,
},
},
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000002"): {
{
Expand All @@ -386,6 +539,24 @@ func TestEncoder_Encode(t *testing.T) {
},
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000005"): nil,
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000006"): nil,
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000007"): {
{
Dependency: uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000008"),
Type: core.RelationshipContains,
},
},
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000008"): {
{
Dependency: uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000010"),
Type: core.RelationshipDependsOn,
},
{
Dependency: uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000009"),
Type: core.RelationshipDependsOn,
},
},
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000009"): nil,
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000010"): nil,
},
wantVulns: map[uuid.UUID][]core.Vulnerability{
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000004"): {
Expand Down
Loading