From b4e6c18496841d61c2740f539609ac6b799bcd7b Mon Sep 17 00:00:00 2001 From: Craig Furman Date: Tue, 27 Jul 2021 15:03:24 +0100 Subject: [PATCH] fix: IaC multi-doc yaml indexing In the IaC local execution data flow, the stage in results-formatter.ts takes a list of IacFileScanResult as input, and returns a list of FormattedResult. One key difference between these two objects is that the former list contains a distinct object per document in a multi-doc yaml file, but for the latter we are grouping these so that there is a distinct object per file. FormattedResults each contain a list of vulnerabilities, so we end up with vulnerabilities from different YAML documents (in the same file) sharing a list. Vulnerabilities with the same "resource path" (Kubernetes example: `spec.containers[web].securityContext.privileged`), but in distinct YAML documents in the same file, should be differentiated by the "cloudConfigPath" (analogous to the "path" of libraries used to address dependecies in the open-source flows), and also by the line number. Both of these distinguishing fields were actually being clobbered by their values from the first document in a file: doc ID was always 0, and the line numbers referred to identical resources in the first doc rather than subsequent docs. This needs to be resolved in order to deliver IaC CLI issue-ignore support in a sane way, and so is tangentially related to https://snyksec.atlassian.net/browse/CC-990. This will also help any tooling that infers information from line numbers, which could help us build things like editor plugins and language servers in the future. --- .../extract-line-number.ts | 13 +++--- .../iac-local-execution/results-formatter.ts | 43 ++++++++----------- .../results-formatter.fixtures.ts | 38 ++++++++++++++-- 3 files changed, 60 insertions(+), 34 deletions(-) diff --git a/src/cli/commands/test/iac-local-execution/extract-line-number.ts b/src/cli/commands/test/iac-local-execution/extract-line-number.ts index c273815ea07..dd4e75feaa5 100644 --- a/src/cli/commands/test/iac-local-execution/extract-line-number.ts +++ b/src/cli/commands/test/iac-local-execution/extract-line-number.ts @@ -1,4 +1,4 @@ -import { IaCErrorCodes, IacFileScanResult, PolicyMetadata } from './types'; +import { IaCErrorCodes } from './types'; import { CustomError } from '../../../../lib/errors'; import { CloudConfigFileTypes, @@ -25,14 +25,15 @@ function getFileTypeForLineNumber(fileType: string): CloudConfigFileTypes { } export function extractLineNumber( - scanResult: IacFileScanResult, - policy: PolicyMetadata, + fileContent: string, + fileType: string, + cloudConfigPath: string[], ): number { try { return issuesToLineNumbers( - scanResult.fileContent, - getFileTypeForLineNumber(scanResult.fileType), - policy.msg.split('.'), // parser defaults to docId:0 and checks for the rest of the path + fileContent, + getFileTypeForLineNumber(fileType), + cloudConfigPath, ); } catch { const err = new FailedToExtractLineNumberError(); diff --git a/src/cli/commands/test/iac-local-execution/results-formatter.ts b/src/cli/commands/test/iac-local-execution/results-formatter.ts index 0aa33be5dd6..92dc80cc1bc 100644 --- a/src/cli/commands/test/iac-local-execution/results-formatter.ts +++ b/src/cli/commands/test/iac-local-execution/results-formatter.ts @@ -23,11 +23,18 @@ export function formatScanResults( meta: TestMeta, ): FormattedResult[] { try { - // Relevant only for multi-doc yaml files - const scannedResultsGroupedByDocId = groupMultiDocResults(scanResults); - return scannedResultsGroupedByDocId.map((iacScanResult) => - formatScanResult(iacScanResult, meta, options), - ); + const groupedByFile = scanResults.reduce((memo, scanResult) => { + const res = formatScanResult(scanResult, meta, options); + if (memo[scanResult.filePath]) { + memo[scanResult.filePath].result.cloudConfigResults.push( + ...res.result.cloudConfigResults, + ); + } else { + memo[scanResult.filePath] = res; + } + return memo; + }, {} as { [key: string]: FormattedResult }); + return Object.values(groupedByFile); } catch (e) { throw new FailedToFormatResults(); } @@ -48,7 +55,7 @@ function formatScanResult( const formattedIssues = scanResult.violatedPolicies.map((policy) => { const cloudConfigPath = scanResult.docId !== undefined - ? [`[DocId:${scanResult.docId}]`].concat(policy.msg.split('.')) + ? [`[DocId: ${scanResult.docId}]`].concat(policy.msg.split('.')) : policy.msg.split('.'); const flagsRequiringLineNumber = [ @@ -61,7 +68,11 @@ function formatScanResult( (flag) => options[flag], ); const lineNumber: number = shouldExtractLineNumber - ? extractLineNumber(scanResult, policy) + ? extractLineNumber( + scanResult.fileContent, + scanResult.fileType, + cloudConfigPath, + ) : -1; return { @@ -145,24 +156,6 @@ function computePaths( }; } -function groupMultiDocResults( - scanResults: IacFileScanResult[], -): IacFileScanResult[] { - const groupedData = scanResults.reduce((memo, result) => { - if (memo[result.filePath]) { - memo[result.filePath].violatedPolicies = memo[ - result.filePath - ].violatedPolicies.concat(result.violatedPolicies); - } else { - memo[result.filePath] = result; - } - - return memo; - }, {} as IacFileScanResult); - - return Object.values(groupedData); -} - export function filterPoliciesBySeverity( violatedPolicies: PolicyMetadata[], severityThreshold?: SEVERITY, diff --git a/test/jest/unit/iac-unit-tests/results-formatter.fixtures.ts b/test/jest/unit/iac-unit-tests/results-formatter.fixtures.ts index 5a63518437f..b8156d3284f 100644 --- a/test/jest/unit/iac-unit-tests/results-formatter.fixtures.ts +++ b/test/jest/unit/iac-unit-tests/results-formatter.fixtures.ts @@ -28,7 +28,6 @@ export const policyStub: PolicyMetadata = { subType: 'Deployment', title: 'Container is running in privileged mode', type: 'k8s', - documentation: 'https://snyk.io/security-rules/SNYK-CC-K8S-2', }; const anotherPolicyStub: PolicyMetadata = { @@ -38,6 +37,12 @@ const anotherPolicyStub: PolicyMetadata = { publicId: 'SNYK-CC-K8S-2', }; +const yetAnotherPolicyStub: PolicyMetadata = { + ...anotherPolicyStub, + id: '3', + publicId: 'SNYK-CC-K8S-3', +}; + const relativeFilePath = 'dont-care.yaml'; const absoluteFilePath = path.resolve(relativeFilePath, '.'); @@ -63,6 +68,16 @@ export function generateScanResults(): Array { filePath: relativeFilePath, fileType: 'yaml', }, + { + violatedPolicies: [{ ...yetAnotherPolicyStub }], + jsonContent: { dontCare: null }, + docId: 1, + projectType: IacProjectType.K8S, + engineType: EngineType.Kubernetes, + fileContent: 'dont-care', + filePath: relativeFilePath, + fileType: 'yaml', + }, ]; } @@ -80,7 +95,7 @@ export function generateCloudConfigResults( ...anotherPolicyStub, id: anotherPolicyStub.publicId, name: anotherPolicyStub.title, - cloudConfigPath: ['[DocId:0]'].concat(anotherPolicyStub.msg.split('.')), + cloudConfigPath: ['[DocId: 0]'].concat(anotherPolicyStub.msg.split('.')), isIgnored: false, iacDescription: { issue: anotherPolicyStub.issue, @@ -89,7 +104,24 @@ export function generateCloudConfigResults( }, severity: anotherPolicyStub.severity, lineNumber: withLineNumber ? 3 : -1, - documentation: anotherPolicyStub.documentation, + documentation: 'https://snyk.io/security-rules/SNYK-CC-K8S-2', + }, + { + ...yetAnotherPolicyStub, + id: yetAnotherPolicyStub.publicId, + name: yetAnotherPolicyStub.title, + cloudConfigPath: ['[DocId: 1]'].concat( + yetAnotherPolicyStub.msg.split('.'), + ), + isIgnored: false, + iacDescription: { + issue: yetAnotherPolicyStub.issue, + impact: yetAnotherPolicyStub.impact, + resolve: yetAnotherPolicyStub.resolve, + }, + severity: yetAnotherPolicyStub.severity, + lineNumber: withLineNumber ? 3 : -1, + documentation: 'https://snyk.io/security-rules/SNYK-CC-K8S-3', }, ]; }