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

Searching for hasSBOMs via Artifacts in Vuln cli #1965

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

Conversation

nathannaveen
Copy link
Contributor

Description of the PR

  • This adds a search for hasSBOM nodes via Artifact nodes in the vulnerability cli.
  • Fixes Attach HasSboms to Artifacts instead of Packages when possible #1736.
    • Note that this only addresses the first part: "update the Vuln CLI to take in hash to search for hasSBOM”.
    • The second part: “Update the patch planning CLI to use hasSBOM dependencies and not direct isDependency when calculating patch plan”. Can’t be done because patch planning is meant to find dependents, and hasSBOM nodes don’t search via dependents, only via dependencies.

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 GraphQL schema is changed, GraphQL client updates/additions have been made
  • 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

Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
@pxp928 pxp928 added the needs-review Needs writer LGTM label Jun 25, 2024
@lumjjb lumjjb self-requested a review July 1, 2024 15:46
Comment on lines +138 to +174
pkgInput, err := helpers.PurlToPkg(opts.searchString)
if err != nil {
logger.Fatalf("failed to parse PURL: %v", err)
}

pkgQualifierFilter := []model.PackageQualifierSpec{}
for _, qualifier := range pkgInput.Qualifiers {
// to prevent https://github.com/golang/go/discussions/56010
qualifier := qualifier
pkgQualifierFilter = append(pkgQualifierFilter, model.PackageQualifierSpec{
Key: qualifier.Key,
Value: &qualifier.Value,
})
}

pkgFilter := &model.PkgSpec{
Type: &pkgInput.Type,
Namespace: pkgInput.Namespace,
Name: &pkgInput.Name,
Version: pkgInput.Version,
Subpath: pkgInput.Subpath,
Qualifiers: pkgQualifierFilter,
}

o, err := model.Occurrences(ctx, gqlclient, model.IsOccurrenceSpec{
Subject: &model.PackageOrSourceSpec{
Package: pkgFilter,
},
})
if err != nil {
logger.Fatalf("error querying for occurrences: %v", err)
}

depVulnPath, depVulnTableRows, err = searchForSBOMViaArtifact(ctx, gqlclient, o.IsOccurrence[0].Artifact.Id, opts.depth, false)
if err != nil {
logger.Fatalf("error searching for SBOMs via artifact: %v", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

the function is getting a bit long - i think we can refactor some of these inner conditionals into functions or at least with some helpers around going from search string to an occurrence.

path = append(path, depVulnPath...)
tableRows = append(tableRows, depVulnTableRows...)
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there an opts for artifact here? it would be good if its not default artifact if opts.Purl isnt set, so that the user is intentional.

return nil, nil, fmt.Errorf("failed getting hasSBOM via URI: %s with error: %w", now, err)
}
// if the initial depth, check if it's a purl or an SBOM URI. Otherwise, always search by pkgID
if nowNode.depth == 0 && primaryCall {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you elaborate a bit more on what primaryCall is? Given this is in a loop rather than recursing, would primaryCall be static throughout the function?

if nowNode.depth == 0 && primaryCall {
pkgResponse, err := getPkgResponseFromPurl(ctx, gqlclient, now)
if err != nil {
return nil, nil, fmt.Errorf("getPkgResponseFromPurl - error: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

%w for wraps (a couple instances)

return path, tableRows, nil
}

func searchForSBOMViaArtifact(ctx context.Context, gqlclient graphql.Client, searchString string, maxLength int, primaryCall bool) ([]string, []table.Row, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have the search functions in a central place so that we don't duplicate too many DFS / BFSes with a lot of boilerplate code - in addition we want to use the same type of search (in this case by SBOM) for everything, and having a common function will be helpful! Can you help take a look at https://github.com/guacsec/guac/blob/0675b6779487e206f30d64ce234bb42c762502a3/pkg/guacanalytics/patchPlanning.go and see if we can implement it in that function?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review Needs writer LGTM size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attach HasSboms to Artifacts instead of Packages when possible
3 participants