-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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: add report summary table #8177
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: knqyf263 <[email protected]>
@DmitriyLewen @nikpivkin Before further development, I'd like to hear your thoughts. |
I agree with you. I think we need to discuss this. Initially (I may be wrong) Trivy was crippled by aggregating individual packages to avoid having too many tables in a report.
It seems that having multiple types for a single file is rare, so I suggest using this logic and waiting for feedback from users.
I prefer this way. I also have one idea - some users don't need to see vulnerability/misconfiguration numbers (CVE etc). So we can hide other tables by default and show them only when using a new flag ( As a future update:
|
I like the idea, the summary table looks good. Should the results be sorted by file name or scan type? I noticed that terraform scan results contain directories that we should get rid of. |
Exactly. It will simplify our logic. However, it will probably break something. We should carefully announce it. That's why I didn't want to add the change into this PR.
I already implemented it in this PR unless I'm making a mistake. In the screenshot, |
The current summary table is ordered by scanners, vulnerabilities -> misconfigurations -> secrets -> licenses. Do you want to sort by sub-scanners, such as terraform?
Yes, I actually saw |
Great! |
Looks nice. What's the difference between a I think I understand it as
I think we should change k8s summary output to be more simpler rather than the other way around. To me the k8s summary output is a bit too much especially with so many numbers that it isn't very helpful. |
I actually considered reusing the K8s summary table, but ultimately decided to keep them separate for several reasons: First, as @simar7 pointed out, the K8s summary table contains much information. This isn't necessarily a negative aspect - it's designed this way because K8s doesn't display details in the summary table by default (requires Second, since we plan to make a separate plugin for the K8s cluster scan similar to AWS account scanning, I wanted to avoid core dependencies on K8s. If we were to share code, having the dependency direction flow from K8s to core would be preferable, not the other way around. However, given the different purposes mentioned above, whether we should standardize these tables at all requires further discussion.
Yes, that's correct. I made this distinction because we frequently receive user feedback asking to clarify whether something wasn't scanned at all or was scanned but returned zero findings. I'll add a note beneath the table to clarify this distinction. |
pkg/report/table/table.go
Outdated
@@ -64,6 +80,69 @@ func (tw Writer) Write(_ context.Context, report types.Report) error { | |||
return nil | |||
} | |||
|
|||
func (tw Writer) renderSummary(report types.Report) error { | |||
log.WithPrefix("report").Info("Report Summary table contains special symbols", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added info about -
and 0
into logs:
2025-01-17T14:37:53+06:00 INFO [report] Report Summary table contains special symbols '-'="Target didn't scanned" '0'="Target scanned, but didn't contain vulns/misconfigs/secrets/licenses"
Report Summary
┌─────────────────────────────────────────────┬──────────┬─────────────────┬─────────┐
│ Target │ Type │ Vulnerabilities │ Secrets │
├─────────────────────────────────────────────┼──────────┼─────────────────┼─────────┤
But we can add info before table. e.g.:
Report Summary
Note:
- `-`: Target didn't scanned.
- `0`: Target scanned, but didn't contain vulns/misconfigs/secrets/licenses.
┌─────────────────────────────────────────────┬──────────┬─────────────────┬─────────┐
│ Target │ Type │ Vulnerabilities │ Secrets │
├─────────────────────────────────────────────┼──────────┼─────────────────┼─────────┤
@aquasecurity/trivy wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we elaborate on why a certain file "didn't scan"? Is it because it was unable to get parsed, wasn't valid for the scan (then maybe we shouldn't even traverse it) or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can wait for user feedback:
In most cases -
will be used for the case when the file is scanned by another scanner.
e.g., we scanned Dockerfile
, and -
will be for vulnerabilities.
In case the file is invalid (e.g. wrong lock file) - the report will not include the result with this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer putting it in the footer.
Legend:
- `-`: Not scanned
- `0`: Clean (no security findings detected)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed in e11221a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the legend is still in header, not footer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. Something "stuck" me. I don't know why I added legend before summary table.
Fixed in 7f863b9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw one case - we show log after summary table:
2025-01-29T17:51:04+06:00 INFO Detected config files num=0
2025-01-29T17:51:04+06:00 DEBUG Specified ignore file does not exist file=".trivyignore"
2025-01-29T17:51:04+06:00 DEBUG [vex] VEX filtering is disabled
Report Summary
┌─────────────────┬──────┬─────────────────┬───────────────────┬─────────┬──────────┐
│ Target │ Type │ Vulnerabilities │ Misconfigurations │ Secrets │ Licenses │
├─────────────────┼──────┼─────────────────┼───────────────────┼─────────┼──────────┤
│ gson-2.11.0.jar │ jar │ 2 │ - │ - │ - │
└─────────────────┴──────┴─────────────────┴───────────────────┴─────────┴──────────┘
Legend:
- '-': Not scanned
- '0': Clean (no security findings detected)
2025-01-29T17:51:04+06:00 INFO Table result includes only package filenames. Use '--format json' option to get the full path to the package file.
Java (jar)
Total: 2 (UNKNOWN: 0, LOW: 0, MEDIUM: 1, HIGH: 1, CRITICAL: 0)
┌───────────────────────────────────────────────────────┬────────────────┬──────────┬────────┬───────────────────┬───────────────────────┬─────────────────────────────────────────────────────┐
│ Library │ Vulnerability │ Severity │ Status │ Installed Version │ Fixed Version │ Title │
├───────────────────────────────────────────────────────┼────────────────┼──────────┼────────┼───────────────────┼───────────────────────┼─────────────────────────────────────────────────────┤
│ com.fasterxml.jackson.core:jackson-databind │ CVE-2022-42003 │ HIGH │ fixed │ 2.13.4.1 │ 2.12.7.1, 2.13.4.2 │ jackson-databind: deep wrapper array nesting wrt │
│ (gson-2.11.0.jar) │ │ │ │ │ │ UNWRAP_SINGLE_VALUE_ARRAYS │
│ │ │ │ │ │ │ https://avd.aquasec.com/nvd/cve-2022-42003 │
├───────────────────────────────────────────────────────┼────────────────┼──────────┤ ├───────────────────┼───────────────────────┼─────────────────────────────────────────────────────┤
│ org.apache.logging.log4j:log4j-core (gson-2.11.0.jar) │ CVE-2021-44832 │ MEDIUM │ │ 2.17.0 │ 2.3.2, 2.12.4, 2.17.1 │ log4j-core: remote code execution via JDBC Appender │
│ │ │ │ │ │ │ https://avd.aquasec.com/nvd/cve-2021-44832 │
└───────────────────────────────────────────────────────┴────────────────┴──────────┴────────┴───────────────────┴───────────────────────┴─────────────────────────────────────────────────────┘
We might want to add a header (e.g. result tables
) before result tables (after the legend) to separate summary table from other tables.
but on the other hand I think it's redundant.
├───────────────────────┼────────────┼─────────────────┼───────────────────┼─────────┼──────────┤ | ||
│ test (alpine 3.20.3) │ alpine │ 2 │ - │ - │ - │ | ||
├───────────────────────┼────────────┼─────────────────┼───────────────────┼─────────┼──────────┤ | ||
│ Java │ jar │ 2 │ - │ - │ - │ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The summary table contains aggregated targets.
Do we want to split them by filePath
in this PR or just start working on removing the aggregation (in another PR)?
cc. @knqyf263
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't split aggregated results, but, actually yes, we can split them in the table writer. I think we should do that in this PR. We will not need the logic after removing the aggregation, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have 1 more question: Do we need to show empty table? ➜ ./trivy -q fs ./pkg/fanal/analyzer/language/nodejs/npm/testdata/sad
Report Summary
┌────────┬──────┬─────────────────┬─────────┐
│ Target │ Type │ Vulnerabilities │ Secrets │
└────────┴──────┴─────────────────┴─────────┘
Or we can hide table and write Info/warn log or something else. |
Showing only the header is probably unclear for users. I'd opt for log messages. |
} | ||
|
||
// showEmptyResultsWarning | ||
func (tw Writer) showEmptyResultsWarning() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
examples:
2025-01-29T12:43:10+06:00 WARN [report] Supported files for vuln scanner(s) not found.
2025-01-29T12:43:39+06:00 WARN [report] No results found for secret scanner(s).
2025-01-29T12:43:42+06:00 WARN [report] Supported files for vuln/misconfig scanner(s) not found. No results found for secret scanner(s).
2025-01-29T12:43:47+06:00 WARN [report] Scanners are not enabled.
for _, pkg := range target.Packages { | ||
for _, license := range pkg.Licenses { | ||
osPkgLicenses = append(osPkgLicenses, toDetectedLicense(scanner, license, pkg.Name, "")) | ||
if len(target.Packages) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is required to hide OS Package licenses from summary table.
This is necessary to avoid confusion - when we show that we were looking for licenses for OS packages, but there were no OS packages in the scanned folder.
e.g.:
➜ ./trivy -d fs ./cmd --scanners license
Report Summary
Legend:
- '-': Not scanned
- '0': Clean (no security findings detected)
┌───────────────────────┬──────┬──────────┐
│ Target │ Type │ Licenses │
├───────────────────────┼──────┼──────────┤
│ OS Packages │ - │ 0 │
└───────────────────────┴──────┴──────────┘
But on the other hand it will be a problem for scanning an image where we did not find OS packages.
because the user may think that we did not look for licenses for OS package files.
But the second case is more rare, so I chose this way.
Tell me - if you disagree or have other ideas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, after we introduce the summary table, I don't think we have to show OS packages when Trivy is unable to find OS packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it make sense.
Removed in 28ebb24
Confidence: finding.Confidence, | ||
Link: finding.Link, | ||
}) | ||
if options.LicenseFull { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar case.
We only keep file licenses and dpkg licenses (but we remove them in applier
) in this slice.
So the report can only include file licenses.
We don't need to show this result if the --liecense-full
flag is not set.
// "--so-summary" option is available only with "--format table". | ||
if noSummaryTable && format != types.FormatTable { | ||
noSummaryTable = false | ||
log.Warn(`"--no-summary-table" can be used only with "--format table".`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, suppose a flag has already been added for a long time, and we don't want to terminate a program for backward compatibility as it will change behavior. In that case, we have to make it a warning, but an error is basically better because the user may not see the warning message.
log.Warn(`"--no-summary-table" can be used only with "--format table".`) | |
return xerrors.New(`"--no-summary-table" can be used only with "--format table".`) |
pkg/report/table/table.go
Outdated
@@ -64,6 +80,69 @@ func (tw Writer) Write(_ context.Context, report types.Report) error { | |||
return nil | |||
} | |||
|
|||
func (tw Writer) renderSummary(report types.Report) error { | |||
log.WithPrefix("report").Info("Report Summary table contains special symbols", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the legend is still in header, not footer.
This PR introduces a summary table that displays all scan results, including those with zero findings, such as vulnerabilities and misconfiguration, to improve visibility and reduce user anxiety about the scanning process.
Background
Historically, Trivy followed the Unix philosophy where "silence is golden" and didn't output anything when no vulnerabilities were found. However, this led to user confusion about whether scans were working correctly. While we previously addressed this by showing results for OS packages with zero vulnerabilities, the inconsistency between OS and language-specific package outputs has created new confusion.
Proposed Changes
This PR adds a summary table at the beginning of the output that:
Example output:
Considerations
Aggregated Results for Individual Files
Result Aggregation Strategy
Default Behavior
--no-summary-table
)--show-summary-table
Implementation Notes
TODOs
--no-summary-table
--no-summary-table
with the formats other than--format table
0
and-
in the summary tableRelated issues
Checklist