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 --ignore-states flag for ignoring findings with specific fix states #1473

Merged
merged 4 commits into from
Oct 17, 2023

Conversation

jhebden-gl
Copy link
Contributor

Similar to the --only-fixed and --only-unfixed flags, this PR adds --ignore-wontfix to specifically ignore findings where the software vendor (i.e. Red Hat or Debian) has stated a vulnerability will not be fixed.

Typically findings of this nature represent vulnerabilities which have been assessed & analysed by the software vendor as either not requiring a fix due to the way the software has been built or packaged by the vendor, or where the vulnerability is not exploitable in the default configuration. In the case of some images (i.e. Red Hat's ubi8/ruby31 image) this can represent hundreds of findings for an image which do not represent exploitable vulnerabilities. More importantly, users may want to specifically filter these out as they will not be fixed, and therefore are not actionable.

This differs slightly from the --ignore-unfixed flag and the filtering in place, as the --ignore-unfixed ignores both findings which will not be fixed, but also findings which have not been fixed yet. This distinction is important when reporting findings as there is benefit in a container scanner alerting users to vulnerabilities they will potentially have to report on or track through to fix availability, but for most teams there is typically no benefit to reporting findings which are not exploitable or impactful as assessed by the vendor, and especially as they are not actionable given no fix is going to be made available.

I opted to add --ignore-wontfix as this wording seems clearer about the intent of the flag, and did not add a generic flag to ignore arbitrary advisory statuses as this seemed like the smallest possible change.

I have also moved some of the ignore/filtering flags in cmd/grype/cli/legacy/root.go into a separate setIgnoreFlags function which is called by setRootFlags to keep the linter happy with the size of the function.

I've confirmed this works as intended by scanning the registry.access.redhat.com/ubi8/ruby-31 image. Scanning with and without the --ignore-wontfix flag produces a delta of 533 wontfix findings, which are correctly removed from the output of the grype scan with --ignore-wontfix.

@tgerla
Copy link
Contributor

tgerla commented Sep 7, 2023

Hey @jhebden-gl, thank you very much for this patch! We're talking this over and we might want to go in a slightly different direction. What do you think if instead of adding --ignore-wontfix, we moved the ignore functionality to a --show option that takes a list:

--show fixed,wontfix,notfixed

This might help us shrink the list of different flags out there, especially if/when we add more states.

Would you be able to join one of our community meetings to discuss? Might be nice to talk it over live. (https://github.com/anchore/grype#join-our-community-meetings)

@jhebden-gl
Copy link
Contributor Author

Hey @tgerla! Thanks for taking a look and for the feedback.

We're talking this over and we might want to go in a slightly different direction. What do you think if instead of adding --ignore-wontfix, we moved the ignore functionality to a --show option that takes a list:

--show fixed,wontfix,notfixed

I think this is a great idea, and totally hear you on cluttering up the number of options. I think this approach is also less opinionated, so users can do their own filtering. I'm happy to rework the MR to support that, if that's the direction y'all would like to take.

Would you be able to join one of our community meetings to discuss? Might be nice to talk it over live. (https://github.com/anchore/grype#join-our-community-meetings)

I'd be totally happy to jump on a community meeting and talk over some options. I'm based in NSW, Australia, though - so the time I'm seeing for the next meeting is at around 2AM local for me, which might make it a bit tricky for me to attend. I'm also happy to chat about this further on this MR, or via another async method if that works.

@willmurphyscode
Copy link
Contributor

@jhebden-gl Thanks very much - we'd really appreciate it if you could update this change to add a --show wontfix,fixed,notfixed,suppressed. I think for now, it make sense to just add this, and let the existing --only-fixed keep working. Please let us know if you have questions or if we can help at all!

@jhebden-gl jhebden-gl changed the title Add --ignore-wontfix flag for ignoring findings which will not be fixed Add --ignore-states flag for ignoring findings with specific fix states Sep 28, 2023
@jhebden-gl
Copy link
Contributor Author

@jhebden-gl Thanks very much - we'd really appreciate it if you could update this change to add a --show wontfix,fixed,notfixed,suppressed. I think for now, it make sense to just add this, and let the existing --only-fixed keep working. Please let us know if you have questions or if we can help at all!

I've updated the branch & PR to extend this to enable filtering on specific states, provided as a list.

If it's OK, I opted to use --ignore-states rather than --showas this allowed this to be implemented more elegantly with fewer lines of code. This also felt more specific and better matches the way the filtering is done within Grype's vulnerability matching code.

I've tested a few different combinations against a known image with lots of fixed & wont-fix findings and this also does not interfere with the existing --only-fixed and --only-unfixed options & can also be used in conjunction with them. So, if you're OK with --ignore-states from a user experience perspective, this PR should meet the requirements we've discussed.

I've also updated the README and added a little helper to get the known fix states, so this could be shown in the --help output, as is done with severity.

Let me know if you'd like anything changed or if you have any other feedback, and thanks for the review & feedback so far.

(@tgerla, @willmurphyscode)

Copy link
Contributor

@willmurphyscode willmurphyscode left a comment

Choose a reason for hiding this comment

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

This @jhebden-gl thanks very much for the contribution! I've made a couple of comments. If you'd like, we can take it from here, and make those changes on top of your branch, or you can update the PR. Let me know which way you'd like to proceed, and thanks again!

case grypeDb.UnknownFixState, grypeDb.FixedState, grypeDb.NotFixedState, grypeDb.WontFixState:
opts.Ignore = append(opts.Ignore, match.IgnoreRule{FixState: ignoreState})
default:
log.Warnf("ignoring unknown fix state %s for --ignore-states", ignoreState)
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 this should return an error instead of logging a warning; the CLI should fail and explain why if the user invokes it with invalid config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've addressed this on the latest commit, and I've also moved this (and the other ignore state handling) up so they happen prior to downloading the DB and running the scan, to enable a fast-feedback loop if a user supplies an invalid fix state (rather than waiting for the DB update & scan to run before they are made aware of the their typo 😄)

@@ -103,6 +104,11 @@ func (o *Grype) AddFlags(flags clio.FlagSet) {
"ignore matches for vulnerabilities that are fixed",
)

flags.StringArrayVarP(&o.IgnoreStates,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if it could be invoked with a comma-separate list, like --ignore-states not-fixed,wont-fix, but right now I seem to have to pass --ignore-states not-fixed --ignore-states wont-fix. --catalogers is handled this way in Syft, code at https://github.com/anchore/syft/blob/f6c805797715b3befbf65c291f466f6fbd94fc0b/cmd/syft/cli/options/catalog.go#L100-L104.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've addressed this on the latest commit by moving this parameter to a string and adding a helper for splitting strings on commas and filtering empty entries. I've also added a test for the helper. With these changes in place, the parameter now processes a comma-separated list of states.

Signed-off-by: James Hebden <jhebden@gitlab.com>
@jhebden-gl
Copy link
Contributor Author

This @jhebden-gl thanks very much for the contribution! I've made a couple of comments. If you'd like, we can take it from here, and make those changes on top of your branch, or you can update the PR. Let me know which way you'd like to proceed, and thanks again!

Thanks for the review & feedback! I've addressed the latest comments and tested them locally. Let me know if you have any further feedback.

…nore states comma-separated

Signed-off-by: James Hebden <jhebden@gitlab.com>
@willmurphyscode
Copy link
Contributor

Thanks @jhebden-gl! I'm taking a look today.

Copy link
Contributor

@wagoodman wagoodman left a comment

Choose a reason for hiding this comment

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

nice job 🙌

@willmurphyscode willmurphyscode merged commit 30f05c3 into anchore:main Oct 17, 2023
9 checks passed
@jhebden-gl jhebden-gl deleted the add-ignore-wontfix branch October 17, 2023 22:54
spiffcs added a commit that referenced this pull request Oct 19, 2023
* main: (137 commits)
  chore(deps): bump actions/checkout from 4.1.0 to 4.1.1 (#1564)
  Add --ignore-states flag for ignoring findings with specific fix states (#1473)
  feat: update go-sarif library to use latest release (#1563)
  bump clio to get stderr reporting fix (#1561)
  chore(deps): bump github.com/gabriel-vasile/mimetype from 1.4.2 to 1.4.3 (#1558)
  chore(deps): bump github.com/charmbracelet/lipgloss from 0.9.0 to 0.9.1 (#1557)
  Add checksum signing (#1535)
  chore(deps): bump golang.org/x/net from 0.16.0 to 0.17.0 (#1554)
  feat: disable CPE-based matching for GHSA ecosystems by default (#1412)
  chore(deps): bump github.com/google/go-cmp from 0.5.9 to 0.6.0 (#1552)
  chore(deps): update Syft to v0.93.0 (#1550)
  chore(deps): bump gorm.io/gorm from 1.25.4 to 1.25.5 (#1547)
  chore(deps): bump github.com/charmbracelet/lipgloss from 0.8.0 to 0.9.0 (#1548)
  chore(deps): bump github.com/hashicorp/go-getter from 1.7.2 to 1.7.3 (#1549)
  chore(deps): bump ossf/scorecard-action from 2.2.0 to 2.3.0 (#1544)
  fix: empty descriptor name and version (#1542)
  chore: removes unnecessary conditional (#1539)
  chore(deps): bump github.com/gkampitakis/go-snaps from 0.4.10 to 0.4.11 (#1533)
  chore(deps): update Syft to v0.92.0 (#1527)
  chore(deps): update bootstrap tools to latest versions (#1524)
  ...
@wagoodman wagoodman added the enhancement New feature or request label Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants