Checkov report parsing enhanced #12398
Conversation
Let's debate if this version is the best to use.
|
This pull request contains a low-confidence potential information disclosure risk in the Checkov parser where adding a description field might inadvertently expose sensitive implementation details from an untrusted source. 💭 Unconfirmed Findings (1)
All finding details can be found in the DryRun Security Dashboard. |
|
i'm not expecting this PR to be merged immediately, in fact it shouldn't: the unit tests for Checkov need to be updated as the scan files being used are now quite outdated. |
shodanwashere
changed the title
Warning
Note BeautifulSoup is being used specifically to obtain snippets of the documentation for different fields by parsing the plain HTML content.
Caution I included the latest version of this library on this PR, but I do agree, I should have checked to see if Beautiful Soup has any vulnerabilities that could open up Defect Dojo specifically with Checkov reports. |
|
Nice PR! My notes:
But, not sure if the maintainers want (or if if should be in general) to have this kind of logic in a parser... needs to be discussed. Could also be done before importing to DD. |
Was taking a quick gander and maybe, we could have it |
|
@shodanwashere Thanks a lot for the PR, really happy with contributions. After consideration the team have decided not to accept the PR as-is. Experience has learned us that reaching out to external systems to enhance findings during import has certain risks and maintenance costs that we're unable to support currently. We have a GitHub repository with community contributions: https://github.com/DefectDojo/Community-Contribs where useful scripts are shared with the community. We would welcome a Another approach could be to suggest to Prismacloud to add more text to the report instead of just a link to their website. The benefit of the link is that the website will always have the latest and greatest information. In issue #12384 it was suggested to include the related benchmark items from the report to the finding in Defect Dojo. That would be a useful and welcome addition to have a PR for. |
|
just pushed a number of commits based on the obtained feedback. I'll be maintaining the benchmark references in the findings as mitigation info and will also be future proofing to include the description if it's in the original finding. I'm currently in conversations with PrismaCloud Support to understand why Checkov is omitting information for findings, so we'll be able to make this PR more robust in the future. |
There was a problem hiding this comment.
Thank you @shodanwashere . Could you look at the test/linting results?
Specifically, this fixes Q000, F821 and F541 identified previously on lines 126, 128 and 130
Description
Implements the suggestions on this issue: #12384 and then some.
To clarify, findings imported from Checkov reports get additional info injected straight from the Palo Alto guidelines documentation.
Findings now come with a more descriptive description (hehe) on their Description field (hehe), and additional fix suggestions and benchmark guidelines on the Mitigation field.
Test results
My theory is these tests fail because they use scan files generated from back when Checkov was still owned by Bridgecrew, whose endpoints have since gone offline. Now findings use documentation hosted by Palo Alto Networks.
Checklist
This checklist is for your information.
dev.dev.bugfixbranch.