fix: enable include-non-failures by default in report conversion

[nikpivkin]

Description

This PR adds the include-non-failures flag to the convert command. It is enabled by default so as not to modify the converted report. The user can pass --include-non-failures=false to exclude passed results.

Related issues

• Close https://github.com/aquasecurity/trivy/issues/8670

Checklist

• [x] I've read the guidelines for contributing to this repository.
• [x] I've followed the conventions in the PR title.
• [ ] I've added tests that prove my fix is effective or that my feature works.
• [ ] I've updated the documentation with the relevant information (if needed).
• [ ] I've added usage information (if the PR introduces new options)
• [ ] I've included a "before" and "after" example to the description (if the PR is a user interface change).

[DmitriyLewen]

I'm not sure we need to enable --include-non-failures by default for the conversion mode. We need to use the same logic as for the scan command (fs/image/etc). It would be weird that we don't include non-failure checks in the table report for an fs scan, but include them when the user converts json to table format.

If I understand #8670 correctly, the problem is that template does not get checked for failures in convert mode. I think we need to solve this problem.

Correct me, if i missed something.

PS. I think that we need to add --include-non-failures flag for convert mode anyway.

[nikpivkin]

If the user performed a scan without the include-non-failures flag, then a conversion with the include-non-failures flag enabled will not add the passed results to the report, as they are not present in the original report. If we use the same logic as for scanning, the user will have to pass the flag explicitly when converting, which may seem counter-intuitive since they just want to convert the report without changes.

• If the user scanned without the --include-non-failures flag, then after conversion the report will not include the passed results as they are not in the original report.
• If the user scanned with the --include-non-failures flag, then after conversion the report will include the passed results, so the report remains unchanged.
• If the user wants to apply filtering during conversion, then he can pass --include-non-failures=false.

[DmitriyLewen]

We already use similar logic for --list-all-pkgs - https://trivy.dev/latest/docs/configuration/reporting/#converting

I consider such situations as follows: base json should include all possible information. Then for other reports the user can customize what he wants to see. I'm not insisting, this is just how I see it.

@aquasecurity/trivy wdyt?

[simar7]

If the user performed a scan without the include-non-failures flag, then a conversion with the include-non-failures flag enabled will not add the passed results to the report, as they are not present in the original report. If we use the same logic as for scanning, the user will have to pass the flag explicitly when converting, which may seem counter-intuitive since they just want to convert the report without changes.

• If the user scanned without the --include-non-failures flag, then after conversion the report will not include the passed results as they are not in the original report.
• If the user scanned with the --include-non-failures flag, then after conversion the report will include the passed results, so the report remains unchanged.
• If the user wants to apply filtering during conversion, then he can pass --include-non-failures=false.

I agree that this behaviour is correct but I am curious why we need to define this behaviour through a flag? To me the convert command should do nothing but convert one report format to another. It should preserve whatever the original report has as-is.

Today we happen to drop non failures while converting reports. This to me is a bug and lack of functionality today. The convert command should not care for how the report was generated but only should convert one format into another.

Adding options to the convert command will run us into issues like what @DmitriyLewen mentioned here https://github.com/aquasecurity/trivy/pull/8679#issuecomment-2777604713

[DmitriyLewen]

We added convert mode to avoid running Trivy multiple times .

Take a looks on this example: User needs to run Trivy 3 times:

• trivy filesystem --scanners misconfig --include-non-failures --format template --template "@contrib/junit.tpl" --output "report.junit.xml" . // to use junit files in the test environment with all checks
• trivy filesystem --scanners misconfig --format sarif --output "report.sarif.xml" . // to add only misconfigs in GitHub
• trivy filesystem --scanners misconfig --format table . // show only misconfig in CI/CD for operator

After adding convert the user must create a "base" json file:

• trivy filesystem --scanners misconfig --include-non-failures --format json --output "report.json" // To create base json file // And uses previous commands with convert mode:
• trivy convert --scanners misconfig --format template --template "@contrib/junit.tpl" --output "report.junit.xml" report.json
• trivy convert --scanners misconfig --format sarif --output "report.sarif.xml" report.json
• trivy convert --scanners misconfig --format table report.json

I think that users expect that they need to use same flags as for fs mode.

After enabling include-non-failures flag by default users should disable non-failures checks:

• trivy convert --scanners misconfig --format template --template "@contrib/junit.tpl" --output "report.junit.xml" report.json
• trivy convert --scanners misconfig --include-non-failures=false --format sarif --output "report.sarif.xml" report.json
• trivy convert --scanners misconfig --include-non-failures=false --format table report.json

I think it's not entirely obvious

[nikpivkin]

After enabling include-non-failures flag by default users should disable non-failures checks:

• trivy convert --scanners misconfig --format template --template "@contrib/junit.tpl" --output "report.junit.xml" report.json
• trivy convert --scanners misconfig --include-non-failures=false --format sarif --output "report.sarif.xml" report.json
• trivy convert --scanners misconfig --include-non-failures=false --format table report.json

In this case, users do not need to pass --include-non-failures=false since the base json does not contain successful results.

[DmitriyLewen]

In this case, users do not need to pass --include-non-failures=false since the base json does not contain successful results.

This is incorrect. Because base json should include successful checks for junit report.

[nikpivkin]

In this case, users do not need to pass --include-non-failures=false since the base json does not contain successful results.

This is incorrect. Because base json should include successful checks for junit report.

Why? In such a case, the user needs to pass --include-non-failures when scanning.

[DmitriyLewen]

Why? In such a case, the user needs to pass --include-non-failures when scanning.

Right.

@simar7 wdyt?

[nikpivkin]

• trivy convert --scanners misconfig --format template --template "@contrib/junit.tpl" --output "report.junit.xml" report.json

I didn't notice that --include-non-failures is missing here. Then I think we can require the flag to be included again if the user needs successful results when converting.

[simar7]

• trivy convert --scanners misconfig --format template --template "@contrib/junit.tpl" --output "report.junit.xml" report.json

I didn't notice that --include-non-failures is missing here. Then I think we can require the flag to be included again if the user needs successful results when converting.

I think what @DmitriyLewen mentioned is that if the base output contains non failures, we won't need to specify the --include-non-failures flag anyway. Regardless, I think the user should decide this upfront as to whether or not to include failures.

Once the base output is established, the trivy convert command simply converts stuff around. So I would even lean towards omitting the --include-non-failures from it unless there's a good reason for it to be included in the convert command.

Giving the ability to filter out (or not) non failures during the convert command stage is just adding more complexity FWIW IMO.

[DmitriyLewen]

I'm worried that we'll have 2 different approaches for --list-all-pkgs (though this is not quite the same case, since other related formats require this flag) and --include-non-failures but I'm not forcing it, and if @aquasecurity/trivy agree - let's use your approach.

[github-actions[bot]]

This PR is stale because it has been labeled with inactivity.