Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Craig Furman committed Jul 27, 2021
1 parent c5e7007 commit b4e6c18
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 34 deletions.
13 changes: 7 additions & 6 deletions 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,
Expand All @@ -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();
Expand Down
43 changes: 18 additions & 25 deletions src/cli/commands/test/iac-local-execution/results-formatter.ts
Expand Up @@ -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();
}
Expand All @@ -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 = [
Expand All @@ -61,7 +68,11 @@ function formatScanResult(
(flag) => options[flag],
);
const lineNumber: number = shouldExtractLineNumber
? extractLineNumber(scanResult, policy)
? extractLineNumber(
scanResult.fileContent,
scanResult.fileType,
cloudConfigPath,
)
: -1;

return {
Expand Down Expand Up @@ -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,
Expand Down
38 changes: 35 additions & 3 deletions test/jest/unit/iac-unit-tests/results-formatter.fixtures.ts
Expand Up @@ -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 = {
Expand All @@ -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, '.');

Expand All @@ -63,6 +68,16 @@ export function generateScanResults(): Array<IacFileScanResult> {
filePath: relativeFilePath,
fileType: 'yaml',
},
{
violatedPolicies: [{ ...yetAnotherPolicyStub }],
jsonContent: { dontCare: null },
docId: 1,
projectType: IacProjectType.K8S,
engineType: EngineType.Kubernetes,
fileContent: 'dont-care',
filePath: relativeFilePath,
fileType: 'yaml',
},
];
}

Expand All @@ -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,
Expand All @@ -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',
},
];
}
Expand Down

0 comments on commit b4e6c18

Please sign in to comment.