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

Add initial support for embedded CycloneDX VEX documents #678

Merged
merged 1 commit into from Apr 28, 2022

Conversation

samj1912
Copy link
Contributor

@samj1912 samj1912 commented Mar 17, 2022

Signed-off-by: Sambhav Kothari skothari44@bloomberg.net

Fixes #591

This adds some initial support for CycloneDX VEX documents. There are a couple of things we will need to think through -

  • Currently this PR produces an embedded VEX document - i.e. we duplicate partial information from the components in the VEX document, add a bom-ref for each and then have the vulnerabilities link to the bom-refs in the same BOM. (We should also verify if my usage of links for components within the same bom is accurate or not, I could not find any example of embedded VEX files at https://github.com/CycloneDX/bom-examples/tree/master/VEX but it is supported according to the docs)
  • We need to figure out a way to inject bom-refs in syft for components. Currently I use the PURL when available as a bom-ref and if it is not I use $type-$name-$version as the bom ref.
  • We need to figure out a way to pass bom-refs from input sboms to grype so that it can reference it in the vex. In case an input is provided that does not contain a cyclonedx sbom and a non embedded vex is selected, we would need to output both a cyclonedx SBOM and a VEX from grype (this is one possible solution but we can explore other alternatives)
  • Currently grype converts input formats into its internal schema which leads to some loss in information that is format specific (for eg. bom refs in cyclonedx's case) - we need to figure out some way to preserve such info if a bom is given as an input.
  • VEX had a number of breaking changes from the previous vulnerability extension schema in cyclonedx. As a result, I have kept the previous extension output for b/w compat. See Added vulnerabilities as part of core spec CycloneDX/specification#91 for a full list of changes.
  • Because of the lack of support for bom refs in spdx, we would need to figure out a way to generate a non-embedded vex for spdx sbom inputs or we may choose to error out entirely if the input is an spdx bom and the output is a non-embedded cyclonedx vex document until we figure out a way to have bom-refs for spdx documents.

Marking it as a draft while we resolve the above questions.

cc: @luhring , @wagoodman , @spiffcs , @kzantow , @stevespringett, @coderpatros, @nscuro

@samj1912
Copy link
Contributor Author

samj1912 commented Mar 31, 2022

The first cut of this VEX PR is done.

I have opted to go with an embedded VEX for now so that it is portable across input SBOMs, we can add support for external VEX once the upstream syft PR lands and we have more discussions on how we want to pass format specific data around in grype. (in this case the cdx bom's uuid)

I have also opted to generate a minimal list of components for the embedded VEX which use the same bom ref generation process as the sibling syft PR anchore/syft#914

Finally from conversations with the cyclonedx core team, I have verified that in case of embedded vex documents, the target ref needs to be a bom ref and not an external link. I have put up a PR at CycloneDX/specification#136 to document the same in the upstream spec.

Finally I have added an initial vuln. analysis state of "in_triage" which we can expand upon in the future with something like grype --interactive

// TODO: In the future we may want to dedupe by PURL and combine components with
// the same PURL while preserving their unique metadata.
if parsedPURL, err := packageurl.FromString(p.PURL); err == nil {
parsedPURL.Qualifiers = append(parsedPURL.Qualifiers, packageurl.Qualifier{Key: "syft-id", Value: string(p.ID)})
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no longer a guarantee that the input SBOM will be from syft, so having a syft-id key here could be misleading in some cases. (also, if we make a change here, we should probably head back to syft and match these approaches up... maybe simply id would suffice?)

Comment on lines +14 to +15
embeddedVEXJSON format = "embedded-cyclonedx-vex-json"
embeddedVEXXML format = "embedded-cyclonedx-vex-xml"
Copy link
Contributor

Choose a reason for hiding this comment

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

these are accurate, though pretty verbose. I'll give a little more thought to something that is shorter / friendlier to input.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have any suggestions for how these could be shorter. @wagoodman did you still have thoughts?

@hectorj2f
Copy link

👋🏻 @samj1912, Do you need a hand with the changes ? We'd be glad to help :)

@samj1912
Copy link
Contributor Author

👋🏻 @samj1912, Do you need a hand with the changes ? We'd be glad to help :)

I think the PR is functionally complete. Waiting for @wagoodman's reply on better output format names. As for the syft-id, id comment, will address that soon but I think we should use something more specific than id. Maybe package-id? Thoughts?

@samj1912 samj1912 requested a review from wagoodman April 19, 2022 11:59
@hectorj2f
Copy link

@samj1912 that is great, I'll give it a try. It sounds good to me to use package ids.

Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
@samj1912
Copy link
Contributor Author

Updated this PR to reflect the comments around syft-id v/s package-id.

Copy link
Contributor

@spiffcs spiffcs left a comment

Choose a reason for hiding this comment

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

Approved! Thanks for the contribution @samj1912. I tagged @wagoodman on his comment regarding verbosity of options, but after giving this a run locally and looking at the code I don't see any issues.

Comment on lines +14 to +15
embeddedVEXJSON format = "embedded-cyclonedx-vex-json"
embeddedVEXXML format = "embedded-cyclonedx-vex-xml"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have any suggestions for how these could be shorter. @wagoodman did you still have thoughts?

@spiffcs spiffcs self-requested a review April 28, 2022 16:23
@spiffcs spiffcs changed the title Add initial support for embedded CycloneDX VEX documents add initial support for embedded CycloneDX VEX documents Apr 28, 2022
@spiffcs spiffcs merged commit 9f70cdb into anchore:main Apr 28, 2022
@hectorj2f
Copy link

@samj1912 Thanks 👏🏻

@samj1912 samj1912 deleted the vex branch April 28, 2022 17:34
@samj1912 samj1912 changed the title add initial support for embedded CycloneDX VEX documents Qdd initial support for embedded CycloneDX VEX documents Apr 29, 2022
@samj1912 samj1912 changed the title Qdd initial support for embedded CycloneDX VEX documents Add initial support for embedded CycloneDX VEX documents Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add support for cyclonedx 1.4 and VEX
4 participants