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

feat: allow findings to be ignored #25

Merged
merged 26 commits into from
Nov 29, 2023
Merged

feat: allow findings to be ignored #25

merged 26 commits into from
Nov 29, 2023

Conversation

jamestelfer
Copy link
Member

@jamestelfer jamestelfer commented Nov 20, 2023

This PR seeks to introduce an "ignore" file for the plugin, allowing findings for vulnerabilities to be ignored (but not removed).

Note

While this is a commit-by-commit PR, I've left some earlier commits that included a more complicated YAML document schema where ignore items supported simple string entries as well as structured ones. I removed this after feedback from @tomwwright, and kept the commits to document the decision somewhat, but also to keep this implementation archived as a possible future reference.

An ignore file is a YAML file that has the following structure:

ignores:
  - id: CVE-2023-100
  - id: CVE-2023-200
    until: 2023-12-31
    reason: allowing 2 weeks for base image to update
  - id: CVE-2023-300

This structure allows for findings to be ignored for a period of time, giving a team time to respond while allowing builds to continue.

Ignore configuration can be specified in a number of places, from least to most important:

  • /etc/ecr-scan-results-buildkite-plugin/ignore.y[a]ml
  • buildkite/ecr-scan-results-ignore.y[a]ml
  • .buildkite/ecr-scan-results-ignore.y[a]ml
  • .ecr-scan-results-ignore.y[a]ml

Configuration in the /etc/ecr-scan-results-buildkite-plugin allows for organizations to ship agents with plugin configuration that centrally manages findings that can be ignored. If a listing for a finding with the same CVE name appears in multiple files, the most local wins: central configuration can be overridden by the repository.

When a finding is ignored, it is removed from consideration for threshold checks, but it's not discarded. The annotation created by the plugin annotates the results with further information instead.

The summary counts at the top show the number of ignored findings:

image

Ignored findings are separated from the main list and shown at the bottom:

image

If a reason for ignoring a finding is provided, it's made available by expanding the Until date:

image

Fixes #24

It's either make it more sane or turn it off.
Load each then de-deplicate, allowing later definitions to override earlier
ones.
Ignore configuration is read from a series of possible locations, and
the downloaded findings are summarized based on the
supplied configuration.
@jamestelfer jamestelfer force-pushed the add-ignore-file branch 3 times, most recently from 963a2bd to 9f846ec Compare November 23, 2023 09:10
Simple version: annotates summary findings and marks findings in table
when ignored.
Using the AWS structs directly was becoming cumbersome. This allows
for a simpler template: adding ignore lists in the existing structure
was becoming too complicated.
This potentially leads to more discoverability that it is possible
to add `until` and `reason` fields.
Remove on read so expired items don't affect cascades.
An inline div surrounded by whitespace was interpreted
as a Markdown paragraph when uploaded to Buildkite.
@jamestelfer jamestelfer marked this pull request as ready for review November 23, 2023 12:44
Linux conventions are to configure commands using `/etc/` for general
configuration rather than user home directories. This lessens the chance
of accidental modification by an agent.
Copy link
Contributor

@therealvio therealvio left a comment

Choose a reason for hiding this comment

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

I have taken my first stab. The feedback I have is pretty simple, as I try to build a mental model of how it all works.

src/finding/summary.go Outdated Show resolved Hide resolved
src/findingconfig/ignores.go Show resolved Hide resolved
src/report/annotation.go Outdated Show resolved Hide resolved
src/finding/summary.go Outdated Show resolved Hide resolved
Comment on lines 67 to 70
counts := SeverityCount{}
if c, exists := s.Counts[severity]; exists {
counts = c
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: usually, this logic is inverted

Suggested change
counts := SeverityCount{}
if c, exists := s.Counts[severity]; exists {
counts = c
}
c := SeverityCount{}
if counts, exists := s.Counts[severity]; !exists {
counts = c
}

Copy link
Member Author

Choose a reason for hiding this comment

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

The variables defined as a part of the assignment in the if clause (if counts, exists := ) are only in scope in the body of the if, so inverting as suggested won't compile.

The error is counts declared and not used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed this to:

counts := s.Counts[severity]

Since the default value is returned if the key is not in the map, which is what I'm after.

src/finding/summary.go Outdated Show resolved Hide resolved
src/findingconfig/ignores.go Outdated Show resolved Hide resolved
src/report/annotation.go Outdated Show resolved Hide resolved
src/report/annotation.go Outdated Show resolved Hide resolved
jamestelfer and others added 2 commits November 27, 2023 18:55
Co-authored-by: Callum Gardner <10970827+ctgardner@users.noreply.github.com>
This is now possible due to API changes up the call chain.
The map lookup will return the empty value by default if the key does not
exist.
These were split into separate declarations to assist initial testing,
but this doesn't help readability.
Copy link
Contributor

@ctgardner ctgardner left a comment

Choose a reason for hiding this comment

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

lgtm

src/finding/summary.go Show resolved Hide resolved
@jamestelfer
Copy link
Member Author

☝️ I've really appreciated all the thoughtful review comments, they've definitely improved the implementation, and I've learned through it too 🙏

@jamestelfer jamestelfer merged commit 9b12f26 into main Nov 29, 2023
7 checks passed
@jamestelfer jamestelfer deleted the add-ignore-file branch November 29, 2023 22:33
@jamestelfer jamestelfer mentioned this pull request Mar 15, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow findings to be ignored
3 participants