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

Jump to bottom

Included a Handler for CycloneDX Version Ranges #1789

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

Included a Handler for CycloneDX Version Ranges #1789

wants to merge 7 commits into from

Conversation

nathannaveen
Copy link
Contributor

Description of the PR

  • Includes a hander for the CycloneDX version ranges.
  • Includes tests for the version range parser
  • Partially fixes [bug] CycloneDX ingestion failing #1148
  • Passes down the graphql client to CycloneDX from the top (Also passing it down to all the other parsers). This is so that we can call model.Packages.

PR Checklist

  • All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
  • All new changes are covered by tests
  • If GraphQL schema is changed, make generate has been run
  • If OpenAPI spec is changed, make generate has been run
  • If collectsub protobuf has been changed, make proto has been run
  • All CI checks are passing (tests and formatting)
  • All dependent PRs have already been merged

@nathannaveen nathannaveen force-pushed the nathan/versionRangesForCycloneDX branch 2 times, most recently from bd89b6c to 0d00394 Compare March 26, 2024 14:40
pxp928
pxp928 reviewed Mar 26, 2024
View reviewed changes
Copy link
Collaborator

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

@nathannaveen looks like one of the tests is failing: https://github.com/guacsec/guac/actions/runs/8437894287/job/23108722974?pr=1789#step:7:156

@nathannaveen nathannaveen force-pushed the nathan/versionRangesForCycloneDX branch from 23e39ec to f234758 Compare March 27, 2024 13:28
@nathannaveen
Copy link
Contributor Author

@pxp928 Thanks, I fixed the test!

pxp928
pxp928 reviewed Apr 18, 2024
View reviewed changes
Copy link
Collaborator

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

Couple of changes/questions. Were you able to test this with any examples?

pkg/ingestor/parser/cyclonedx/parser_cyclonedx.go Outdated Show resolved Hide resolved
pkg/ingestor/parser/cyclonedx/parser_cyclonedx.go Outdated Show resolved Hide resolved
pkg/ingestor/parser/cyclonedx/parser_cyclonedx.go Outdated Show resolved Hide resolved
@nathannaveen nathannaveen force-pushed the nathan/versionRangesForCycloneDX branch 3 times, most recently from a56894a to a06a21f Compare April 19, 2024 16:22
@nathannaveen nathannaveen requested a review from pxp928 April 19, 2024 16:24
@nathannaveen
Included a Version Range Parser for CycloneDX
2210535 
* Included a version range parser for CycloneDX

Signed-off-by: nathannaveen <[email protected]>
@nathannaveen
Included Tests
7f78bf1 
Signed-off-by: nathannaveen <[email protected]>
@nathannaveen
Passing the graphql client from the top
41c4f93 
Signed-off-by: nathannaveen <[email protected]>
@nathannaveen
Fixed Tests
a03b41a 
Signed-off-by: nathannaveen <[email protected]>
@nathannaveen
Update based on code review
ed05894 
Signed-off-by: nathannaveen <[email protected]>
@nathannaveen
Update based on code review
15ebaae 
Signed-off-by: nathannaveen <[email protected]>
@nathannaveen nathannaveen force-pushed the nathan/versionRangesForCycloneDX branch from 5a0516e to b5bec6a Compare May 10, 2024 23:59
@nathannaveen
Updated based on code review
c35c59f 
Signed-off-by: nathannaveen <[email protected]>
@nathannaveen nathannaveen force-pushed the nathan/versionRangesForCycloneDX branch from b5bec6a to c35c59f Compare May 10, 2024 23:59
pxp928
pxp928 reviewed Jun 12, 2024
View reviewed changes
internal/testing/cmd/ingest/cmd/root.go
@@ -85,7 +87,9 @@ var rootCmd = &cobra.Command{
Use: "ingest",
Short: "example ingestor for ingesting a set of example documents and populating a graph for GUAC",
Run: func(cmd *cobra.Command, args []string) {
ingestExample(cmd, args)
httpClient := http.Client{}
Copy link
Collaborator

@pxp928 pxp928 Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we will need to refactor this. We don't want to be querying graphQL on ingestion. Will need to re-work this after some thought.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will make it an issue to discuss during the next maintainer meeting.

lumjjb
lumjjb reviewed Jul 1, 2024
View reviewed changes
pkg/ingestor/parser/cyclonedx/parser_cyclonedx.go
@@ -448,33 +461,186 @@ func (c *cyclonedxParser) getAffectedPackages(ctx context.Context, vulnInput *mo

var viList []assembler.VexIngest
for _, affect := range *affectsObj.Range {
// TODO: Handle package range versions (see - https://github.com/CycloneDX/bom-examples/blob/master/VEX/CISA-Use-Cases/Case-8/vex.json#L42)
Copy link
Contributor

Choose a reason for hiding this comment

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

@SantiagoTorres @arorasoham9 something to check against the identifier model of how to express such version ranges for CVEs and VEX.

@stale Stale
Copy link

stale bot commented Aug 31, 2024

This pull request has been automatically marked as stale because it has not had recent activity (60 days of inactivity).
It will be closed in 30 days if no further activity occurs.
Thank you for your contribution!

@stale stale bot added the wontfix This will not be worked on label Aug 31, 2024
@pxp928
Copy link
Collaborator

pxp928 commented Sep 2, 2024

Ping to keep this open. This needs to be discussed during the maintainer meeting to determine a path fwd.

@stale stale bot removed the wontfix This will not be worked on label Sep 2, 2024
@pxp928
Copy link
Collaborator

pxp928 commented Oct 10, 2024

ping for stale bot

@stale Stale
Copy link

stale bot commented Dec 10, 2024

This pull request has been automatically marked as stale because it has not had recent activity (60 days of inactivity).
It will be closed in 30 days if no further activity occurs.
Thank you for your contribution!

@stale stale bot added the wontfix This will not be worked on label Dec 10, 2024
@funnelfiasco
Copy link
Contributor

@nathannaveen are you blocked on Maintainer discussion here? If so, I'll put it on Monday's agenda so we can move this forward.

@stale stale bot removed the wontfix This will not be worked on label Dec 10, 2024
@nathannaveen
Copy link
Contributor Author

@funnelfiasco, I am blocked on the maintainer discussion. Thank you for suggesting putting it on the agenda, it would be great if we can get this merged in!

@pxp928
Copy link
Collaborator

pxp928 commented Dec 16, 2024

@nathannaveen based on discussion at the maintainer call the high level plan to resolve this PR is:

- Store certifyvex at package with empty string
- Store the version range as part of statement
- Remove graphql
- At query time, use the version range to validate with the purls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pxp928 pxp928 pxp928 left review comments

@lumjjb lumjjb lumjjb left review comments

@mihaimaruseac mihaimaruseac Awaiting requested review from mihaimaruseac mihaimaruseac is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[bug] CycloneDX ingestion failing
4 participants
@nathannaveen @pxp928 @funnelfiasco @lumjjb