feat(sbom): add Parent Contains Relationship for Orphan OS Packages#9007
feat(sbom): add Parent Contains Relationship for Orphan OS Packages#9007masahiro331 wants to merge 17 commits intoaquasecurity:mainfrom
Conversation
e252811 to
a3eaaa9
Compare
|
@DmitriyLewen |
| @@ -8639,6 +8639,7 @@ | |||
| "pkg:deb/debian/base-passwd@3.5.46?arch=amd64&distro=debian-10.2", | |||
| "pkg:deb/debian/bash@5.0-4?arch=amd64&distro=debian-10.2", | |||
| "pkg:deb/debian/bsdutils@2.33.1-0.1?arch=amd64&distro=debian-10.2&epoch=1", | |||
| "pkg:deb/debian/ca-certificates@20190110?arch=all&distro=debian-10.2", | |||
There was a problem hiding this comment.
reason why pkg:deb/debian/ca-certificates@20190110?arch=all&distro=debian-10.2 is not related with an OS component:
{
"ref": "pkg:deb/debian/ruby2.5@2.5.5-3%2Bdeb10u1?arch=amd64&distro=debian-10.2",
"dependsOn": [
"pkg:deb/debian/libc6@2.28-10?arch=amd64&distro=debian-10.2",
"pkg:deb/debian/libgmp10@6.1.2%2Bdfsg-4?arch=amd64&distro=debian-10.2&epoch=2",
"pkg:deb/debian/libruby2.5@2.5.5-3%2Bdeb10u1?arch=amd64&distro=debian-10.2",
"pkg:deb/debian/rubygems-integration@1.11?arch=all&distro=debian-10.2"
]
},
{
"ref": "pkg:deb/debian/ruby@2.5.1?arch=amd64&distro=debian-10.2&epoch=1",
"dependsOn": [
"pkg:deb/debian/ruby2.5@2.5.5-3%2Bdeb10u1?arch=amd64&distro=debian-10.2"
]
},
{
"ref": "pkg:deb/debian/rubygems-integration@1.11?arch=all&distro=debian-10.2",
"dependsOn": [
"pkg:deb/debian/ca-certificates@20190110?arch=all&distro=debian-10.2"
]
},|
@masahiro331 |
|
Thank you. |
DmitriyLewen
left a comment
There was a problem hiding this comment.
LGTM.
@knqyf263 you also worked on belongToParent function.
Can you take a look? Perhaps we missed something.
|
Actually, we may want to revert the logc to add Then, it would be simple. diff --git a/pkg/sbom/io/encode.go b/pkg/sbom/io/encode.go
index 8f395c150..9eb5faf55 100644
--- a/pkg/sbom/io/encode.go
+++ b/pkg/sbom/io/encode.go
@@ -251,37 +251,6 @@ func (e *Encoder) encodePackages(parent *core.Component, result types.Result) {
e.bom.AddRelationship(c, nil, "")
}
}
-
- // Add relationships between the parent and the orphaned packages
- // For OS packages, packages with circular dependencies are not added as relationships to the parent component in belongToParent.
- // To resolve this, we add relationships between these circularly dependent packages and the parent component.
- // cf. https://github.com/aquasecurity/trivy/issues/9011
- if result.Class == types.ClassOSPkg {
- e.addOrphanedPackagesRelationships(parent, components)
- }
-}
-
-// addOrphanedPackagesRelationships adds relationships between the parent and the orphaned packages
-func (e *Encoder) addOrphanedPackagesRelationships(parent *core.Component, components map[string]*core.Component) {
- pkgs := lo.MapKeys(components, func(c *core.Component, _ string) uuid.UUID {
- return c.ID()
- })
- orphans := e.traverseRelationship(parent.ID(), pkgs)
- for _, c := range orphans {
- e.bom.AddRelationship(parent, c, core.RelationshipContains)
- }
-}
-
-func (e *Encoder) traverseRelationship(id uuid.UUID, components map[uuid.UUID]*core.Component) map[uuid.UUID]*core.Component {
- for _, rel := range e.bom.Relationships()[id] {
- _, ok := components[rel.Dependency]
- if !ok {
- continue
- }
- delete(components, rel.Dependency)
- components = e.traverseRelationship(rel.Dependency, components)
- }
- return components
}
// existedPkgIdentifier tries to look for package identifier (BOM-ref, PURL) by component name and component type
@@ -457,7 +426,7 @@ func (*Encoder) belongToParent(pkg ftypes.Package, parents map[string]ftypes.Pac
// Case 4: Relationship: unknown, DependsOn: known (e.g., GoBinaries, OS packages)
// - Packages with parents: false. These packages are included in the packages from `parents` (e.g. GoBinaries deps and root package).
// - Packages without parents: true. These packages are included in the parent (e.g. OS packages without parents).
- if pkg.Relationship == ftypes.RelationshipDirect {
+ if pkg.Relationship == ftypes.RelationshipDirect || pkg.Relationship == ftypes.RelationshipUnknown {
return !hasRoot
}
diff --git a/pkg/sbom/io/encode_test.go b/pkg/sbom/io/encode_test.go
index ef4bc74d4..b6bebfaf4 100644
--- a/pkg/sbom/io/encode_test.go
+++ b/pkg/sbom/io/encode_test.go
@@ -520,6 +520,10 @@ func TestEncoder_Encode(t *testing.T) {
},
},
uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000002"): {
+ {
+ Dependency: uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000003"),
+ Type: core.RelationshipContains,
+ },
{
Dependency: uuid.MustParse("3ff14136-e09f-4df9-80ea-000000000004"),
Type: core.RelationshipContains, |
|
RelationshipUnknown is in such a wonderful value. This is the best. |
|
You can see this PR to know what needs to be reverted. |
|
Thank you. |
…manager - Implement functionality to merge multiple PackageInfo with the same package manager (rpm, deb) - Add merge process in Decode function - Add logic to merge PackageInfo using package manager type as key
…package manager" This reverts commit 34e2935a2d6f86c917deb42aa3c37f445d815b2f.
- add lint to issue into comment - use lo.MapKeys - change place in code for traverseRelationship - add wantVulns to fix unit test
39bff3f to
5b8e9e4
Compare
|
https://proxy.golang.org seems unstable now. |
5b8e9e4 to
d7dae5e
Compare
| "pkg:npm/asap@2.0.6", | ||
| "pkg:npm/jquery@3.3.9", | ||
| "pkg:npm/js-tokens@4.0.0", | ||
| "pkg:npm/loose-envify@1.4.0", | ||
| "pkg:npm/object-assign@4.1.1", | ||
| "pkg:npm/promise@8.0.3", | ||
| "pkg:npm/prop-types@15.7.2", | ||
| "pkg:npm/react-is@16.8.6", | ||
| "pkg:npm/react@16.8.6", | ||
| "pkg:npm/redux@4.0.1" | ||
| "pkg:npm/redux@4.0.1", | ||
| "pkg:npm/scheduler@0.13.6", | ||
| "pkg:npm/symbol-observable@1.2.0" |
There was a problem hiding this comment.
The npm modules' relationships are unknown.
So, all packages have parent relationships.
There was a problem hiding this comment.
@knqyf263
The npm package lock v1 does not have a dependency identifier(relationship.Unknown).
I think it would be difficult to solve all problems with a common process.
Do you have any good ideas?
There was a problem hiding this comment.
I think it would be a good idea to fix the implementation on the npm parser side, but what do you think?
There was a problem hiding this comment.
Hi @masahiro331
Sorry for the wait.
We already used this logic (unknown relationships are always related to parents) and users didn't complain.
We just wanted to improve our logic, but as you can see, we didn't cover all cases.
About npm v1. The latest version v6 (since v7 lockfile uses v2 scheme) was released in 2022 - https://docs.npmjs.com/cli/v6/using-npm/changelog
So I think we can leave it as is
I think most users have already migrated to newer versions
d7dae5e to
157f28d
Compare
| @@ -557,6 +557,7 @@ func TestMarshaler_MarshalReport(t *testing.T) { | |||
| { | |||
| Ref: "3ff14136-e09f-4df9-80ea-000000000004", | |||
| Dependencies: &[]string{ | |||
| "3ff14136-e09f-4df9-80ea-000000000005", | |||
There was a problem hiding this comment.
Investigate if this change is correct.
There was a problem hiding this comment.
|
Looks like it's not work... |
26ad5cf to
38b46e5
Compare
This reverts commit f8ef0b56fff2df6ea1750037074f371ccb4cb586.
38b46e5 to
58ae63c
Compare
|
@knqyf263 I think it would be a good idea to fix the implementation on the npm parser side, but what do you think? |
|
Hi @masahiro331 |
| if pkg.Relationship == ftypes.RelationshipUnknown && len(parents[pkg.ID]) != 0 { | ||
| return !hasRoot | ||
| } | ||
| if pkg.Relationship == ftypes.RelationshipDirect || pkg.Relationship == ftypes.RelationshipUnknown { |
There was a problem hiding this comment.
| if pkg.Relationship == ftypes.RelationshipUnknown && len(parents[pkg.ID]) != 0 { | |
| return !hasRoot | |
| } | |
| if pkg.Relationship == ftypes.RelationshipDirect || pkg.Relationship == ftypes.RelationshipUnknown { | |
| if pkg.Relationship == ftypes.RelationshipDirect || pkg.Relationship == ftypes.RelationshipUnknown { |
It can be simpler, or am i missing any case?
There was a problem hiding this comment.
I updated logic - 3b5a83e
tell me if this logic doesn’t cover some cases
|
This PR is stale because it has been labeled with inactivity. |
|
This PR is stale because it has been labeled with inactivity. |
DmitriyLewen
left a comment
There was a problem hiding this comment.
I think we can go back to the previous approach: link unknown components to their parent when there’s no root component. (#9007 (comment))
@knqyf263 if you don’t have any objections, I’ll merge this PR.
|
This PR is stale because it has been labeled with inactivity. |
Description
Add Parent Contains Relationship for Orphan OS Packages
Problem
During the encoding phase, some OS packages were correctly identified and added to the BOM as components, but they were not linked to their parent OS component. These "orphan" packages resulted in an incomplete dependency graph in the final SBOM.
Solution
A new recursive function, traverseRelationship, has been added to encode.go. This function traverses the dependency graph to identify all components that are already linked to a parent.
Any OS packages that are not part of this graph are considered orphans.
The encodePackages function now explicitly adds a Contains relationship from the parent component (e.g., the OS component) to each of these identified orphan packages.
Related
Checklist