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: Filter vuln host list cves and show assessment by severity #375

Merged
merged 15 commits into from
Apr 9, 2021

Conversation

dmurray-lacework
Copy link
Collaborator

@dmurray-lacework dmurray-lacework commented Apr 6, 2021

Filter vuln host commands by severity:

  • list cves
  • show assessment

Usage :
lacework vuln host list-cves --severity high
lacework vuln host list-cves --active --severity high --fixable
lacework vuln host show-assessment 101 --severity high
lacework vuln host show-assessment 101 --packages --severity high

Output:

      CVE ID        SEVERITY   SCORE       PACKAGE         CURRENT VERSION         FIX VERSION        OS VERSION    HOSTS   PKG STATUS   VULN STATUS
------------------+----------+-------+-----------------+----------------------+--------------------+--------------+-------+------------+--------------
...

 10 of 100 cve(s) showing

Signed-off-by: Darren Murray [email protected]

@afiune afiune requested a review from a team April 6, 2021 22:52
@afiune afiune assigned afiune and dmurray-lacework and unassigned afiune Apr 6, 2021
cli/cmd/event.go Outdated Show resolved Hide resolved
cli/cmd/vuln_host.go Outdated Show resolved Hide resolved
cli/cmd/vuln_host.go Outdated Show resolved Hide resolved
cli/cmd/vuln_host_test.go Outdated Show resolved Hide resolved
cli/cmd/vuln_host_test.go Outdated Show resolved Hide resolved
cli/cmd/vuln_host_test.go Show resolved Hide resolved
@dmurray-lacework dmurray-lacework force-pushed the dmurray-lacework/vuln-host-severity-flag branch from 5dd3545 to b90ac84 Compare April 7, 2021 15:00
Signed-off-by: Darren Murray <[email protected]>
Signed-off-by: Darren Murray <[email protected]>
Copy link
Contributor

@afiune afiune left a comment

Choose a reason for hiding this comment

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

When developing these new flags we need to make sure we give proper messages to our users if not, they will be confused.

For example, if I run this command with the severity flag set to critical:

$ lacework vuln host list-cves --severity critical
There are no vulnerabilities in your environment.

That message is misleading 👆🏽 because there are indeed vulnerabilities, but there are NO vulnerabilities with a critical severity, the message should be instead:

There are no critical vulnerabilities in your environment.

This is the pattern we followed for the other flags --active and --fixable,
here an example:

$ lacework vuln host list-cves --active --fixable
There are no fixable vulnerabilities of packages actively running in your environment.

@afiune
Copy link
Contributor

afiune commented Apr 7, 2021

This makes me think that the other --severity flags we added to the container vulnerability space are not following this same standard ... 🤔 It would be nice to double check.

}

// order by severity
sort.Slice(out, func(i, j int) bool {
return severityOrder(out[i][1]) < severityOrder(out[j][1])
})

return out
if len(filteredPackages) > 0 {
filteredOutput := fmt.Sprintf("%v of %v package(s) showing \n", len(out), len(aggregatedPackages)+len(filteredPackages))
Copy link
Contributor

@afiune afiune Apr 7, 2021

Choose a reason for hiding this comment

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

nit: Everytime I read this footer I feel it looks way better if we type it like:

showing X of Y package(s)

But maybe this is just me.....
tenor-83302290

Copy link
Collaborator Author

@dmurray-lacework dmurray-lacework Apr 8, 2021

Choose a reason for hiding this comment

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

I quite like this way, with the most important details first. But open to be persuaded otherwise.

showing 1 of 100 package(s)
vs
1 of 100 package(s) showing

Copy link
Contributor

Choose a reason for hiding this comment

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

all good, this is just my bad English 😂

tenor-168867785

@dmurray-lacework
Copy link
Collaborator Author

dmurray-lacework commented Apr 8, 2021

This makes me think that the other --severity flags we added to the container vulnerability space are not following this same standard ... 🤔 It would be nice to double check.

@afiune Slightly different behaviour in vuln ctr code path as we check the result of the vuln assessment when displaying the 0 vulns output instead checking the length of the output rows. I've putting in a fix for the current vuln host scenario. I will address the inconsistency in the 2 code paths in our refactor ticket.

cli/cmd/vuln_host.go Outdated Show resolved Hide resolved
cli/cmd/vuln_host.go Outdated Show resolved Hide resolved
cli/cmd/vuln_host.go Outdated Show resolved Hide resolved
Copy link
Contributor

@afiune afiune left a comment

Choose a reason for hiding this comment

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

tenor-94268517

Signed-off-by: Darren Murray <[email protected]>
@dmurray-lacework dmurray-lacework merged commit 7e9313e into main Apr 9, 2021
@dmurray-lacework dmurray-lacework deleted the dmurray-lacework/vuln-host-severity-flag branch April 9, 2021 15:07
This was referenced Apr 9, 2021
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.

2 participants