-
Notifications
You must be signed in to change notification settings - Fork 514
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
cyclonedx: add artifact relationships as dependencies #706
cyclonedx: add artifact relationships as dependencies #706
Conversation
679f191
to
b833b52
Compare
I am checking the issues in the tests 🔍 ... |
a750bc4
to
6ad17fc
Compare
@@ -57,18 +65,50 @@ func toComponent(p pkg.Package) cyclonedx.Component { | |||
} | |||
} | |||
|
|||
func lookupRelationship(ty artifact.RelationshipType) bool { | |||
switch ty { | |||
case artifact.OwnershipByFileOverlapRelationship: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is great! It naturally builds on the new relationship features we've been adding over the last few weeks. I think the only issue with this PR is the relationship type in this switch case.
I'm still learning about CycloneDX spec details; does CycloneDX have any specific definitions on what a dependency is? That is, is a CycloneDX dependency a package build dependency? dev dependency? Must it be a package-to-package dependency? Or are package-to-file / file-to-package dependencies allowed?
Good news is that I think the right relationship types can start being added in #572 . This issue is all about extending the package catalogers with the ability to raise up relationships that they find in their ecosystems. For example, the DPKG status file has fields for package dependencies (http://www.fifi.org/doc/libapt-pkg-doc/dpkg-tech.html/ch1.html) which means that the pkg.DpkgMetadata
can now be extended to extract out this information (the Depends
field at least) and have that cataloger raise up relationships between any two packages that were both found to be installed. (#572 is means to cover all ecosystems, not just DPKG, but figured I'd be more specific here)
The major addition in this issue is that one or more TBD relationship types will be added to accommodate describing these dependencies here:
https://github.com/anchore/syft/blob/main/syft/artifact/relationship.go#L3-L12
The relationships that are there now don't seem to match with what a CycloneDX dependency is (I think), but the new relationship types being added will.
I think that means that the switch case here will be replaced with one or more new relationship types that more closely describe "package-to-package dependencies". I don't think it makes sense to merge this PR with the OwnershipByFileOverlapRelationship
relationship described (unless I'm wrong about the intention of the CycloneDX dependency definition here) but with a different switch case in the near future it makes a ton of sense 👍 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wagoodman CycloneDx dependencies describe how components depend on other components through a dependency graph that represents direct and transitive relationships. Generally I found syft code already does that for SPDX with the mentioned relationship type.
The relationships that are there now don't seem to match with what a CycloneDX dependency is (I think), but the new relationship types being added will.
Could you share which you think those new relationship types are ? I found that OwnershipByFileOverlapRelationship
was more general-purpose type while ContainsRelationship
is focused to SPDX files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wagoodman Could you share some pointers where I could start adding support for these new types ? I am very concerned of using a CycloneDX file without having the minimum set of properties defined by the NTIA https://www.ntia.doc.gov/files/ntia/publications/sbom_minimum_elements_report.pdf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I've added some comments on #572 (comment) that could help here.
TL;DR we'll need to add at least the following SPDX relationships (from https://spdx.github.io/spdx-spec/relationships-between-SPDX-elements/):
RUNTIME_DEPENDENCY_OF
DEV_DEPENDENCY_OF
BUILD_DEPENDENCY_OF
DEPENDENCY_OF
They would be added here: https://github.com/anchore/syft/blob/v0.34.0/syft/artifact/relationship.go#L3
RuntimeDependencyOfRelationship RelationshipType = "runtime-dependency-of"
DevDependencyOfRelationship RelationshipType = "dev-dependency-of"
BuildDependencyOfRelationship RelationshipType = "build-dependency-of"
DependencyOfRelationship RelationshipType = "dependency-of"
Once added, these should all map to the case statement in your PR that checks if the given relationship should translate to a CycloneDX dependency.
Issue #572 is all about beginning to leverage these new relationships, as of now we don't have any catalogers that surface these dependencies as relationships.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wagoodman Okay, I'll update the PR with these changes. Thanks.
6ad17fc
to
94775ad
Compare
@hectorj2f Nice addition. I don't have more to add barring the relationship comments @wagoodman already made. I noticed:
was showing up in our static analysis. Would you be able to push a small change that resolves this gocritic error at your earliest convenience? I'm assuming the I'll also give 👀 to the rest of the PR. |
@spiffcs Yes, I'll add that change too. 👍🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comments. Thanks so much for the PR @hectorj2f! I didn't have too much to add since @wagoodman had such a good review for the core parts of the relationships.
@hectorj2f Let me know if you need some extra commits from me on this PR to help. If not no worries, but didn't want this to get stale since there is really good work here. Always willing to jump in and clean stuff up if you need! |
Signed-off-by: hectorj2f <hectorf@vmware.com>
94775ad
to
dd7644d
Compare
@spiffcs I won't have time to work on this until next Thursday :/. Feel free to add commits to this PR, I'll sync back with you next week if you want. |
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Thanks for the commits @spiffcs 👏🏻 |
@hectorj2f I'm going to close this PR in favor of #768. Sorry I could not keep it all on the same PR in this case. Let me know if you have any questions about this. |
I realized thad artifact relationships were added to SPDX but they weren't added to the dependencies in CycloneDX format. Syft should include those to the cyclonedx format.
related to: #572