Add Violations support for JAS scanners #241
Conversation
…dded the new parsed violations results to ApplicabilityScanManager.
…ew parsed violations results to IacScanManager.
…new parsed violations results to SastScanManager.
…e new parsed violations results to SecretScanManager.
…ded relevant logs
…methods for hte new struct
eranturgeman
had a problem deploying
to
frogbot
GitHub Actions
Failure
…o jas-violations-support
eranturgeman
had a problem deploying
to
frogbot
GitHub Actions
Failure
…canner cannot incur violations
…y commented at this point)
…o delete comments in results.go)
…rseCommandResults related to jas violations
…esults) + changed the names for Sca parsing methods
…all relevant references
| // Parse parameters for the SCA result | ||
| type scaParseParams struct { | ||
| CmdType utils.CommandType | ||
| IssueId, Summary, MarkdownDescription, CveScore, ImpactedPackagesName, ImpactedPackagesVersion string |
There was a problem hiding this comment.
| IssueId, Summary, MarkdownDescription, CveScore, ImpactedPackagesName, ImpactedPackagesVersion string | |
| IssueId string | |
| Summary string | |
| MarkdownDescription string | |
| CveScore string | |
| ImpactedPackagesName string | |
| ImpactedPackagesVersion string |
| scaCurrentRun *sarif.Run | ||
| currentTarget results.ScanTarget | ||
| parsedScaKeys *datastructures.Set[string] | ||
| currentTarget *currentTargetState |
There was a problem hiding this comment.
i'd change the name here to currentTargetState so we will not have a mixup with currentTarget inside currentTargetState
| if err = sc.parseScaViolations(target, scaResponse, applicableScan...); err != nil { | ||
| return | ||
| } | ||
| return |
There was a problem hiding this comment.
Question: I remomber we said that we can have both vulnerabilities and violations together. should we take out this return or is it not possible in here?
| assert.Equal(t, "[CVE-2022-1234] loadsh 1.4.1", getScaVulnerabilitySarifHeadline("loadsh", "1.4.1", "CVE-2022-1234")) | ||
| assert.NotEqual(t, "[CVE-2022-1234] comp 1.4.1", getScaVulnerabilitySarifHeadline("comp", "1.2.1", "CVE-2022-1234")) | ||
| assert.Equal(t, "[CVE-2022-1234] loadsh 1.4.1", getScaVulnerabilitySarifHeadline("loadsh", "1.4.1", "CVE-2022-1234", "")) | ||
| assert.NotEqual(t, "[CVE-2022-1234] comp 1.4.1", getScaVulnerabilitySarifHeadline("comp", "1.2.1", "CVE-2022-1234", "")) |
There was a problem hiding this comment.
Also add a testcase for a watch?
| sjc.current.Statuses.ApplicabilityStatusCode = &applicableScan[i].StatusCode | ||
| } | ||
| } | ||
| if violations { |
There was a problem hiding this comment.
can we have both violations and vulnerabilities (includeAllVulnerabilities)
| return func(vulnerability services.Vulnerability, cves []formats.CveRow, applicabilityStatus jasutils.ApplicabilityStatus, severity severityutils.Severity, impactedPackagesName, impactedPackagesVersion, impactedPackagesType string, fixedVersion []string, directComponents []formats.ComponentRow, impactPaths [][]formats.ComponentRow) error { | ||
| tech := target.Technology | ||
| if tech == "" { | ||
| tech = techutils.Technology(impactedPackagesType) |
There was a problem hiding this comment.
is impactedPackagesType always have a value?
| func (sc *CmdResultsSummaryConverter) ParseViolations(target results.ScanTarget, scaResponse services.ScanResponse, applicabilityRuns ...*sarif.Run) (err error) { | ||
| func (sc *CmdResultsSummaryConverter) ParseScaIssues(target results.ScanTarget, violations bool, scaResponse results.ScanResult[services.ScanResponse], applicableScan ...results.ScanResult[[]*sarif.Run]) (err error) { | ||
| if violations { | ||
| return sc.parseScaViolations(target, scaResponse, applicableScan...) |
There was a problem hiding this comment.
same as before - can we have both vuln and violations?
| @@ -221,77 +249,124 @@ func getCveIds(cves []formats.CveRow, issueId string) []string { | |||
| return ids | |||
| } | |||
|
|
|||
| func (sc *CmdResultsSummaryConverter) ParseLicenses(target results.ScanTarget, licenses []services.License) (err error) { | |||
| func (sc *CmdResultsSummaryConverter) ParseLicenses(target results.ScanTarget, scaResponse results.ScanResult[services.ScanResponse]) (err error) { | |||
There was a problem hiding this comment.
| func (sc *CmdResultsSummaryConverter) ParseLicenses(target results.ScanTarget, scaResponse results.ScanResult[services.ScanResponse]) (err error) { | |
| func (sc *CmdResultsSummaryConverter) ParseLicenses(_ results.ScanTarget, _ results.ScanResult[services.ScanResponse]) (err error) { |
| } | ||
|
|
||
| func (sc *CmdResultsSummaryConverter) getJasHandler(scanType jasutils.JasScanType) results.ParseJasFunc { | ||
| func (sc *CmdResultsSummaryConverter) getJasHandler(scanType jasutils.JasScanType, violations bool) results.ParseJasFunc { |
There was a problem hiding this comment.
I'd add a description about what exactly this handler is doing. it seems that it counts and returns a map with counts of SOMETHING but it is unclear what
| return func(run *sarif.Run, rule *sarif.ReportingDescriptor, severity severityutils.Severity, result *sarif.Result, location *sarif.Location) (err error) { | ||
| if location == nil { | ||
| // Only count the issue if it has a location | ||
| return | ||
| } | ||
| // Get the scanType count | ||
| // Get the scanType count and status | ||
| resultStatus := formats.NoStatus |
There was a problem hiding this comment.
what scanType != Secrets, resultsStatus == NoStatus == ""
it means we enter the map an empty key.
This can be easily overrided by mistake, and anyway it might be best to set NoStatus with some actual non-empty value for debugs and better comprehansion later on
| } | ||
| if count == nil { | ||
| return | ||
| } | ||
| // PrepareJasIssues calls the handler for each issue (location) | ||
| // Calls the handler for each issue (location) |
There was a problem hiding this comment.
I dont understand what exactly we are calling here. we are only setting the map with values and returning this handler function
| // Set git repo key and check if it has any watches defined in the platform. | ||
| context.GitRepoHttpsCloneUrl = gitRepoHttpsCloneUrl | ||
| if len(context.PlatformWatches.GitRepositoryWatches) == 0 && len(watches) == 0 && projectKey == "" { | ||
| log.Debug(fmt.Sprintf("Git repo key was provided (%s) but no watches were defined in the platform. ignoring git repo key...", context.GitRepoHttpsCloneUrl)) |
There was a problem hiding this comment.
I think the log should be something like:
"No watches were found in the platform for the given git repo key (%s), and no watches were given by the user (using watches or project flags). Calculating vulnerabilities..."
| return | ||
| } | ||
|
|
||
| // If the user requested to include vulnerabilities, or if the user didn't provide any watches, project key, artifactory repo path or git repo key, we should include vulnerabilities. |
There was a problem hiding this comment.
I think the comment here should be:
"If the user requested to include vulnerabilities, or if the user didn't provide any watches, project key, artifactory repo path or no watches configured on the given git repo key, we should include vulnerabilities"
| type ResultContext struct { | ||
| // If watches are provided, the scan will be performed only with the provided watches. | ||
| Watches []string `json:"watches,omitempty"` | ||
| // (Resource) If repo_path is provided, the scan will be performed on the repository's project watches. |
There was a problem hiding this comment.
The comment should be:
(Resource) If repo_path is provided, the scan will be performed on the repository's watches.
| } | ||
| } | ||
|
|
||
| func CreateTestGitRepoWatch(t *testing.T, policyName, watchName string, gitResources ...string) (string, func()) { |
There was a problem hiding this comment.
I think the name of the function should be different since you can create a watch with git repository or with build type.
Maby we should call it CreateWatchForTests?
attiasas
added
the
safe to test
github-actions
bot
removed
the
safe to test
devbranch.go vet ./....go fmt ./....Depends on:
Add Support for JAS violations
1. Allow generating JAS violations in the scan by providing
watch,GitRepositoryKey, orprojectattributes.GitRepositoryKey: the http/https git clone url of the repository the code related to. (The actual expected value is converted by removing the protocol)Xray(SCA scan) and to theanalyzerManager(JAS scans)2. Generating violations in all output formats (adding attributes to tables/structs where needed)
3. Violations should also be generated for binary scans that support JAS (passing the
project keyto theanalyzerManager)4. In Frogbot,
GitRepositoryKeyis always passed to the command, this means the user will start to see only violations and not vulnerabilities as the default behavior without supplying any arguments, to avoid breaking:GitRepositoryKeywe will fetch the active watches on this resource, if non exists we will show vulnerabilities (only if the user did not request any other params likewatches\project key\repo path...)Additional changes
resultsContextstruct to replacecommonGraphScanParamsto hold and handle all the attributes that can be used to generate results (watches, project key, git repo ....)ScanResult[T interface{}].StatusCode(not performed = nil,completed = 0,failed != 0)convertor_test.gojf scan/jf docker scan) when--repo-pathis presentJFROG_SECURITY_CLI_TESTS_JFROG_PLATFORM_PROJECT_KEY