feat: Add findings details to console output

[bryankaraffa]

This adds additional output to the console so it's easier to identify what needs to be fixed from GitHub Action logs

[alexjurkiewicz]

Thanks for the contribution! Can you paste some example output?

[bryankaraffa]

A scan for this image was already requested, the scan's status is IN_PROGRESS
Polling ECR for image scan findings...
Polling ECR for image scan findings...
Findings:
[{"name":"CVE-2020-28928","uri":"https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-28928","severity":"LOW","attributes":[{"key":"package_version","value":"1.2.2-r3"},{"key":"package_name","value":"musl"},{"key":"CVSS2_VECTOR","value":"AV:L/AC:L/Au:N/C:N/I:N/A:P"},{"key":"CVSS2_SCORE","value":"2.1"}]}]
Vulnerabilities found:
  0 Critical 
  0 High 
  0 Medium 
  1 Low 
  0 Informational 
  0 Undefined 
=================
  1 Total 

[alexjurkiewicz]

Is the purpose of this output to be easy to copy/paste the JSON output into another tool? Or to be easy to read in the github actions console output?

If the latter, it looks like we should apply a little extra formatting so the format is easier to read. What do you think?

[pzi]

Definitely agree with @alexjurkiewicz, if a user would like to just read and get an understanding of what is going on rather than just copy & pasting it, then I think applying some formatting would be nice.

However, having said that, this is the formatted version of the above:

[
  {
    "name": "CVE-2020-28928",
    "uri": "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-28928",
    "severity": "LOW",
    "attributes": [
      {
        "key": "package_version",
        "value": "1.2.2-r3"
      },
      {
        "key": "package_name",
        "value": "musl"
      },
      {
        "key": "CVSS2_VECTOR",
        "value": "AV:L/AC:L/Au:N/C:N/I:N/A:P"
      },
      {
        "key": "CVSS2_SCORE",
        "value": "2.1"
      }
    ]
  }
]

And that is the output for 1 CVE only... imagine 10x or 100x the vulnerabilities; could get quite noisy and unwieldy.

There is also a second question to be asked, would you want/need to see all the attributes or only some? How much value does this provide?

For this reason, I'd recommend grouping it no matter which version we end up with:
https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#grouping-log-lines

[pzi]

There is another thought of adding a flag for the extra output which would be "off" by default so the current output stays the same? Thoughts?

[alexjurkiewicz]

I think we can leave the output enabled by default. People will only read the logs when they need this info, so noisy output every run seems fine to me.

You have a good point about the verboseness of "dumb prettifying". I think you are right, we need custom formatting logic here. How about:

Findings:
  1. CVE-2020-28928 (LOW) package_version=1.2.2-r3 package_name=musl CVSS2_VECTOR=AV:L/AC:L/Au:N/C:N/I:N/A:P CVSS2_SCORE=2.1
  2. ...

To me, this seems like a reasonable compromise between readability and code complexity. What do you (both) think?

[pzi]

Great idea, that seems much simpler and turns 100 CVEs into 100 lines rather than >20x that.

Thoughts on grouping, still?

echo "::group::Findings"
echo  "1. CVE-2020-28928 (LOW) package_version=1.2.2-r3 package_name=musl CVSS2_VECTOR=AV:L/AC:L/Au:N/C:N/I:N/A:P CVSS2_SCORE=2.1"
echo  "2. ..."
echo "::endgroup::"

[alexjurkiewicz]

@bryankaraffa can you make these changes? 🙏

[bryankaraffa]

Formatted output in bd1ce75 and here's example now:

::debug::Entering main
::debug::Repository:bk-micros/calc, Tag:latest, Ignore list:
::debug::Checking for existing findings
A scan for this image was already requested, the scan's status is COMPLETE
::set-output name=findings_details::[object Object],[object Object]
::set-output name=critical::0
::set-output name=high::1
::set-output name=medium::0
::set-output name=low::1
::set-output name=informational::0
::set-output name=undefined::0
::set-output name=ignored::0
::set-output name=total::2
::group::Findings
1. CVE-2016-4074 (HIGH) package_version=1.6-r1 package_name=jq CVSS2_VECTOR=AV:N/AC:L/Au:N/C:N/I:N/A:C CVSS2_SCORE=7.8
2. CVE-2020-28928 (LOW) package_version=1.2.2-r3 package_name=musl CVSS2_VECTOR=AV:L/AC:L/Au:N/C:N/I:N/A:P CVSS2_SCORE=2.1
::endgroup::
Vulnerabilities summary:
  0 Critical 
  1 High 
  0 Medium 
  1 Low 
  0 Informational 
  0 Undefined 
=================
  2 Total 
::error::Detected 1 vulnerabilities with severity >= high (the currently configured fail_threshold).

[alexjurkiewicz]

Thank you! I've made a couple of style updates, @pzi, mind sanity checking?

[pzi]

Feel free to ignore the nitpick about the casing, that was just a consistency thing.
However, if there are no findings (either because there are none or the ones that were found are ignored, we would be showing an empty group.

Which actually raises another question, how does the ignore list come into play here? Should we show which ones are ignored or still list them but flag them as ignored?

[alexjurkiewicz]

Which actually raises another question, how does the ignore list come into play here? Should we show which ones are ignored or still list them but flag them as ignored?

Good catch. It seems better to only print non-ignored findings. However, the refactoring required for this seems complex, so I'm inclined to ignore this for now, in the interests of getting the PR merged.

[pzi]

Which actually raises another question, how does the ignore list come into play here? Should we show which ones are ignored or still list them but flag them as ignored?

Good catch. It seems better to only print non-ignored findings. However, the refactoring required for this seems complex, so I'm inclined to ignore this for now, in the interests of getting the PR merged.

Works for me. Then it's just the variable naming and showing No findings in the group when length === 0 to keep it simple:

# pseudo code
if findings.length > 0
  findings.each => console.log(...)
else
  console.log('No findings')

[pzi]

Thanks for making these changes @bryankaraffa 👍🏼 LGTM and you are right, we can move forward with what's there and hide "empty Findings" group later if required. You ok with this @alexjurkiewicz?

[alexjurkiewicz]

Sorry for the delay. I really like this improvement! It's simple code and high value output. Thank you for the contribution 🤲

[alexjurkiewicz]

Unfortunately I can't create a new release from mobile, so I'll check this out on Monday. Must also remember to update the readme tag ..

[alexjurkiewicz]

Tagged as v1.7.0 <3