Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IaC local execution experimental release #1 #1601

Merged
merged 1 commit into from
Feb 3, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ src/cli/commands/test/formatters/format-reachability.ts @snyk/flow
help/ @snyk/hammer
* @snyk/hammer @snyk/boost
src/cli/commands/test/iac-output.ts @snyk/cloudconfig
src/cli/commands/test/iac-local-execution/ @snyk/cloudconfig
src/lib/cloud-config-projects.ts @snyk/cloudconfig
src/lib/iac/ @snyk/cloudconfig
src/lib/snyk-test/iac-test-result.ts @snyk/cloudconfig
Expand All @@ -12,6 +13,7 @@ src/lib/snyk-test/run-iac-test.ts @snyk/cloudconfig
test/acceptance/cli-test/iac/ @snyk/cloudconfig
test/fixtures/iac/ @snyk/cloudconfig
test/smoke/spec/iac/ @snyk/cloudconfig
test/smoke/.iac-data/ @snyk/cloudconfig
src/lib/errors/invalid-iac-file.ts @snyk/cloudconfig
src/lib/errors/unsupported-options-iac-error.ts @snyk/cloudconfig
help/commands-docs/iac-examples.md @snyk/cloudconfig
Expand Down
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,5 @@ report.[0-9]*.[0-9]*.[0-9]*.[0-9]*.json
/test/acceptance/workspaces/**/target/
test/acceptance/workspaces/**/.gradle
test/**/.gradle
.iac-data
!test/smoke/.iac-data
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
"author": "snyk.io",
"license": "Apache-2.0",
"dependencies": {
"@open-policy-agent/opa-wasm": "git+https://github.com/open-policy-agent/npm-opa-wasm.git#f4a21fe6f4d70706f85106dc6ea867983747e040",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we binding to a git commit and not the npm distro?
https://www.npmjs.com/package/@open-policy-agent/opa-wasm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intentionally.
the package is not mature, and the latest tagged release is missing functionality that we are dependent on.
therefore, until they do a new release, we're targeting a specific commit-hash.
see:
open-policy-agent/npm-opa-wasm#31

"@snyk/cli-interface": "2.11.0",
"@snyk/dep-graph": "1.21.0",
"@snyk/gemfile": "1.2.0",
Expand Down
86 changes: 86 additions & 0 deletions src/cli/commands/test/iac-local-execution/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import * as fs from 'fs';
import * as YAML from 'js-yaml';
import { isLocalFolder } from '../../../../lib/detect';
import { getFileType } from '../../../../lib/iac/iac-parser';
import * as util from 'util';
import { IacFileTypes } from '../../../../lib/iac/constants';
import { IacFileScanResult, IacFileMetadata, IacFileData } from './types';
import { buildPolicyEngine } from './policy-engine';
import { formatResults } from './results-formatter';

const readFileContentsAsync = util.promisify(fs.readFile);
const REQUIRED_K8S_FIELDS = ['apiVersion', 'kind', 'metadata'];

// this method executes the local processing engine and then formats the results to adapt with the CLI output.
// the current version is dependent on files to be present locally which are not part of the source code.
// without these files this method would fail.
// if you're interested in trying out the experimental local execution model for IaC scanning, please reach-out.
export async function test(pathToScan: string, options) {
// TODO: add support for proper typing of old TestResult interface.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a future todo?

Copy link
Contributor Author

@rontalx rontalx Feb 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, as it is a typing rabbit-hole, and I prefer not having the old types in the CLI dictate how we build the new experience.
+I want to focus on the bare-minimum of fields for the new experience data-structures, and not jam tons of fields just because the main CLI flow requires them.
so this will be typed, but soon :)

const results = await localProcessing(pathToScan);
const formattedResults = formatResults(results, options);
const singleFileFormattedResult = formattedResults[0];

return singleFileFormattedResult as any;
}

async function localProcessing(
pathToScan: string,
): Promise<IacFileScanResult[]> {
const policyEngine = await buildPolicyEngine();
rontalx marked this conversation as resolved.
Show resolved Hide resolved
const filePathsToScan = await getFilePathsToScan(pathToScan);
const fileDataToScan = await parseFileContentsForPolicyEngine(
filePathsToScan,
);
const scanResults = await policyEngine.scanFiles(fileDataToScan);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe worth align this with the evaluate of CCPE (I saw that there is an internal evaluate function there - but maybe there it should be evaluateData or something similar)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will consider that for next release.
although I don't want to have our old types & terminology restrict us here, as it's an opportunity to rethink naming as well.

return scanResults;
}

async function getFilePathsToScan(pathToScan): Promise<IacFileMetadata[]> {
if (isLocalFolder(pathToScan)) {
throw new Error(
'IaC Experimental version does not support directory scan yet.',
);
}
if (getFileType(pathToScan) === 'tf') {
throw new Error(
'IaC Experimental version does not support Terraform scan yet.',
);
}
aron marked this conversation as resolved.
Show resolved Hide resolved

return [
{ filePath: pathToScan, fileType: getFileType(pathToScan) as IacFileTypes },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid this cast? Or document why it is needed.

];
}

async function parseFileContentsForPolicyEngine(
filesMetadata: IacFileMetadata[],
): Promise<IacFileData[]> {
const parsedFileData: Array<IacFileData> = [];
for (const fileMetadata of filesMetadata) {
const fileContent = await readFileContentsAsync(
fileMetadata.filePath,
'utf-8',
);
const yamlDocuments = YAML.safeLoadAll(fileContent);

yamlDocuments.forEach((parsedYamlDocument, docId) => {
if (
REQUIRED_K8S_FIELDS.every((requiredField) =>
parsedYamlDocument.hasOwnProperty(requiredField),
)
) {
parsedFileData.push({
...fileMetadata,
fileContent: fileContent,
jsonContent: parsedYamlDocument,
docId,
});
} else {
throw new Error('Invalid K8s File!');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we include the missing fields here to help with debugging.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we throw in one of the iterations, are we ok w/ throwing away all the process?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aviadatsnyk
for now yes, as it supports a single file scan.
when we add directory support (upcoming follow-up work), probably partial results are the right approach, which is also how it works now if I remember correctly..
Anyway, ones of our goal along this migration to the local-exec model, is to align with the experience we currently have today for the iac remote-processing cli flow.

}
});
}

return parsedFileData;
}
63 changes: 63 additions & 0 deletions src/cli/commands/test/iac-local-execution/policy-engine.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import {
OpaWasmInstance,
IacFileData,
IacFileScanResult,
PolicyMetadata,
} from './types';
import { loadPolicy } from '@open-policy-agent/opa-wasm';
import * as fs from 'fs';
import * as path from 'path';

const LOCAL_POLICY_ENGINE_DIR = `.iac-data`;
const LOCAL_POLICY_ENGINE_WASM_PATH = `${LOCAL_POLICY_ENGINE_DIR}${path.sep}policy.wasm`;
const LOCAL_POLICY_ENGINE_DATA_PATH = `${LOCAL_POLICY_ENGINE_DIR}${path.sep}data.json`;

export async function buildPolicyEngine(): Promise<PolicyEngine> {
const policyEngineCoreDataPath = `${process.cwd()}/${LOCAL_POLICY_ENGINE_WASM_PATH}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path.sep :)

const policyEngineMetaDataPath = `${process.cwd()}/${LOCAL_POLICY_ENGINE_DATA_PATH}`;
try {
const wasmFile = fs.readFileSync(policyEngineCoreDataPath);
const policyMetaData = fs.readFileSync(policyEngineMetaDataPath);
const policyMetadataAsJson: Record<string, any> = JSON.parse(
policyMetaData.toString(),
);

const opaWasmInstance: OpaWasmInstance = await loadPolicy(
Buffer.from(wasmFile),
);
opaWasmInstance.setData(policyMetadataAsJson);

return new PolicyEngine(opaWasmInstance);
} catch (err) {
throw new Error(
`Failed to build policy engine from path: ${LOCAL_POLICY_ENGINE_DIR}: \n err: ${err.message}`,
);
aron marked this conversation as resolved.
Show resolved Hide resolved
}
}

class PolicyEngine {
constructor(private opaWasmInstance: OpaWasmInstance) {
this.opaWasmInstance = opaWasmInstance;
}

private evaluate(data: Record<string, any>): PolicyMetadata[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if it throws?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return this.opaWasmInstance.evaluate(data)[0].result;
}

public async scanFiles(
aron marked this conversation as resolved.
Show resolved Hide resolved
filesToScan: IacFileData[],
): Promise<IacFileScanResult[]> {
try {
return filesToScan.map((iacFile: IacFileData) => {
const violatedPolicies = this.evaluate(iacFile.jsonContent);
return {
...iacFile,
violatedPolicies,
};
});
} catch (err) {
// TODO: to distinguish between different failure reasons
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to implement that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not for now.
this is an internal dog-fooding version which already includes much more features than it should have had.

throw new Error(`Failed to run policy engine: ${err}`);
}
}
}
121 changes: 121 additions & 0 deletions src/cli/commands/test/iac-local-execution/results-formatter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
import { IacFileScanResult, PolicyMetadata } from './types';
import { SEVERITY } from '../../../../lib/snyk-test/common';
// import {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please clean

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// issuesToLineNumbers,
// CloudConfigFileTypes,
// } from '@snyk/cloud-config-parser';

const SEVERITIES = [SEVERITY.LOW, SEVERITY.MEDIUM, SEVERITY.HIGH];

export function formatResults(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should return a TestResult[] type (defined in src/lib/snyk-test/legacy.ts).

iacLocalExecutionResults: Array<IacFileScanResult>,
options: { severityThreshold?: SEVERITY },
) {
const iacLocalExecutionGroupedResults = groupMultiDocResults(
iacLocalExecutionResults,
);
return iacLocalExecutionGroupedResults.map((iacScanResult) =>
iacLocalFileScanToFormattedResult(iacScanResult, options.severityThreshold),
);
}

//
// function getFileTypeForLineNumber(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹💨

Copy link
Contributor Author

@rontalx rontalx Feb 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally I 100% agree.
but keeping these intentionally, as this is logic that will be entered in one of the next follow-up PR's and is commented out due to a blocker.
wrote a comment on it here:
https://github.com/snyk/snyk/pull/1601/files#diff-2e4fd6421898cc5c5947d03530196bee66469cd25276fcdfcc0ba4381268b2c9R44

otherwise it will just get re-implemented soon.

// fileType: string,
// ): CloudConfigFileTypes {
// switch (fileType) {
// case 'yaml':
// case 'yml':
// return CloudConfigFileTypes.YAML;
// case 'json':
// return CloudConfigFileTypes.JSON;
// default:
// return CloudConfigFileTypes.YAML;
// }
// }

function iacLocalFileScanToFormattedResult(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should return a TestResult type (defined in src/lib/snyk-test/legacy.ts).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely correct, but for now adding a TODO on these as it's a typing rabbit-hole due to lack of accurate typing down the road in the rest of the CLI..

iacFileScanResult: IacFileScanResult,
severityThreshold?: SEVERITY,
) {
const formattedIssues = iacFileScanResult.violatedPolicies.map((policy) => {
// TODO: make sure we handle this issue with annotations:
// https://github.com/snyk/registry/pull/17277
const cloudConfigPath = [`[DocId:${iacFileScanResult.docId}]`].concat(
policy.msg.split('.'),
);
const lineNumber = -1;
// TODO: once package becomes public, restore the commented out code for having the issue-to-line-number functionality
// try {
// lineNumber = issuesToLineNumbers(
// iacFileScanResult.fileContent,
// getFileTypeForLineNumber(iacFileScanResult.fileType),
// cloudConfigPath,
// );
// } catch (err) {
// //
// }

return {
...policy,
id: policy.publicId,
from: [],
name: policy.title,
cloudConfigPath,
isIgnored: false,
iacDescription: {
issue: policy.issue,
impact: policy.impact,
resolve: policy.resolve,
},
severity: policy.severity,
lineNumber: lineNumber,
};
});
return {
result: {
cloudConfigResults: filterPoliciesBySeverity(
formattedIssues,
severityThreshold,
),
},
isPrivate: true,
packageManager: 'k8sconfig',
targetFile: iacFileScanResult.filePath,
};
}

function groupMultiDocResults(
scanResults: Array<IacFileScanResult>,
): Array<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);
}

function filterPoliciesBySeverity(
violatedPolicies: PolicyMetadata[],
severityThreshold?: SEVERITY,
): PolicyMetadata[] {
if (!severityThreshold || severityThreshold === SEVERITY.LOW) {
return violatedPolicies;
}

const severitiesToInclude = SEVERITIES.slice(
SEVERITIES.indexOf(severityThreshold),
);

return violatedPolicies.filter((policy) =>
severitiesToInclude.includes(policy.severity),
);
}
33 changes: 33 additions & 0 deletions src/cli/commands/test/iac-local-execution/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { SEVERITY } from '../../../../lib/snyk-test/common';
import { IacFileInDirectory } from '../../../../lib/types';

export type IacFileMetadata = IacFileInDirectory;
export interface IacFileData extends IacFileMetadata {
jsonContent: Record<string, any>;
fileContent: string;
docId?: number;
}
export interface IacFileScanResult extends IacFileData {
violatedPolicies: PolicyMetadata[];
}

export interface OpaWasmInstance {
evaluate: (data: Record<string, any>) => { results: PolicyMetadata[] };
setData: (data: Record<string, any>) => void;
}

export interface PolicyMetadata {
id: string;
publicId: string;
type: string;
subType: string;
title: string;
description: string;
severity: SEVERITY;
msg: string;
policyEngineType: 'opa';
issue: string;
impact: string;
resolve: string;
references: string[];
}
10 changes: 9 additions & 1 deletion src/cli/commands/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ import {
getDisplayedOutput,
} from './formatters/format-test-results';

import * as iacLocalExecution from './iac-local-execution';

const debug = Debug('snyk-test');
const SEPARATOR = '\n-------------------------------------------------------\n';

Expand Down Expand Up @@ -133,7 +135,13 @@ async function test(...args: MethodArgs): Promise<TestCommandResult> {
let res: (TestResult | TestResult[]) | Error;

try {
res = await snyk.test(path, testOpts);
if (options.iac && options.experimental) {
rontalx marked this conversation as resolved.
Show resolved Hide resolved
// this path is an experimental feature feature for IaC which does issue scanning locally without sending files to our Backend servers.
// once ready for GA, it is aimed to deprecate our remote-processing model, so IaC file scanning in the CLI is done locally.
res = await iacLocalExecution.test(path, options);
} else {
res = await snyk.test(path, testOpts);
}
if (testOpts.iacDirFiles) {
options.iacDirFiles = testOpts.iacDirFiles;
}
Expand Down
2 changes: 1 addition & 1 deletion src/lib/snyk-test/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export function assembleQueryString(options) {
return Object.keys(qs).length !== 0 ? qs : null;
}

enum SEVERITY {
export enum SEVERITY {
rontalx marked this conversation as resolved.
Show resolved Hide resolved
LOW = 'low',
MEDIUM = 'medium',
HIGH = 'high',
Expand Down
1 change: 1 addition & 0 deletions test/smoke/.iac-data/data.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{".circleci":{"jobs":{"run_tests":{"docker":[{"image":"circleci/golang:1.14"}],"steps":["checkout",{"restore_cache":{"keys":["go-mod-v4-{{ checksum \"go.sum\" }}"]}},{"run":"go build"},{"save_cache":{"key":"go-mod-v4-{{ checksum \"go.sum\" }}","paths":["/go/pkg/mod"]}},{"run":{"command":"./cloud-config-opa-policies test .\n","name":"Run Tests"}}]}},"version":2.1,"workflows":{"build_and_push":{"jobs":["run_tests"]},"version":2}},"ecosystems":{"kubernetes":{"SNYK_CC_K8S_1":{"description":"","id":"1","impact":"Compromised container could potentially modify the underlying host’s kernel by loading unauthorized modules (i.e. drivers).","issue":"Container is running in privileged mode","policyEngineType":"opa","publicId":"SNYK-CC-K8S-1","references":["CIS Kubernetes Benchmark 1.6.0 - 5.2.1 Minimize the admission of privileged containers","https://kubernetes.io/docs/concepts/policy/pod-security-policy/#privileged","https://kubernetes.io/blog/2016/08/security-best-practices-kubernetes-deployment/"],"resolve":"Remove `securityContext.privileged` attribute, or set value to `false`","severity":"high","subType":"Deployment","title":"Container is running in privileged mode","type":"k8s"}}}}
Binary file added test/smoke/.iac-data/policy.wasm
Binary file not shown.
Loading