feat: SARIF format support for IaC and containers #1384

orkamara · 2020-09-02T15:28:24Z

Ready for review
Follows CONTRIBUTING rules
Reviewed by Snyk internal team

What does this PR do?

Introduces SARIF support for both containers and IaC.
Support for --sarif flag to output SARIF to stdout.
Support for --sarif-file-output to save SARIF results to specified file.

How should this be manually tested?

snyk container test alpine --file=Dockerfile --sarif --sarif-output-file=test-container-sarif.json
snyk iac test file.yaml --sarif --sarif-file-output=test-iac-sarif.json

rontalx

Nice work on this PR! 💪 🚀
Left a few comments

src/cli/commands/test/iac-output.ts

src/cli/commands/test/index.ts

JackuB · 2020-09-10T12:27:37Z

test/acceptance/cli-args.test.ts

+
+test('flags not allowed with --sarif', (t) => {
+  t.plan(1);
+  exec(`node ${main} test --sarif --json`, (err, stdout) => {


Quick note here: we have a --json flag set in our CLI Docker containers. Would that be an issue for your use case?

Shouldn't be, the only problem is if you try to use both --sarif and --json (can't output to stdout two different formats)

I meant that you won't be able to run Snyk CLI Docker image to get a SARIF output

JackuB · 2020-09-10T12:30:26Z

package.json

@@ -61,6 +61,7 @@
    "@snyk/lodash": "^4.17.15-patch",
    "@snyk/ruby-semver": "2.2.0",
    "@snyk/snyk-cocoapods-plugin": "2.5.0",
+    "@types/sarif": "^2.1.2",


We are usually keeping @types in devDependencies

aviadhahami · 2020-09-14T07:55:05Z

src/cli/commands/test/iac-output.ts

+}
+
+function uppercaseFirstLetter(str: string): string {
+  return str.charAt(0).toUpperCase() + str.slice(1);


I'd recommend using https://lodash.com/docs/4.17.15#upperFirst

Suggested change

return str.charAt(0).toUpperCase() + str.slice(1);

import upperFirst from `lodash/upperFirst`

...

return upperFirst(str);

removed that function and replaced

aviadhahami · 2020-09-14T07:57:09Z

src/cli/commands/test/iac-output.ts

+      tool.driver.rules?.push({
+        id: iacIssue.id,
+        shortDescription: {
+          text: `${uppercaseFirstLetter(iacIssue.severity)} - ${


upperFirst

aviadhahami · 2020-09-14T07:57:41Z

src/cli/commands/test/iac-output.ts

+      if (pushedIds.includes(iacIssue.id)) {
+        return;
+      }
+      tool.driver.rules?.push({


rules is defined on top, no need for ?

aviadhahami · 2020-09-14T08:04:28Z

src/cli/commands/test/iac-output.ts

+  const pushedIds: string[] = [];
+  iacTestResponse.result.cloudConfigResults.forEach(
+    (iacIssue: AnnotatedIacIssue) => {
+      if (pushedIds.includes(iacIssue.id)) {


Non blocking
I'd suggest swapping the array w/ an object;
This will be lighter on the computational side for when searching previously seen ids

const pushedIds = {}; ... if( pushedIds[iacIssue.id]){ ... } pushedIds[iacIssue.id] = true;

aviadhahami · 2020-09-14T08:07:08Z

src/cli/commands/test/iac-output.ts

+): Sarif.Result[] {
+  const results: Sarif.Result[] = [];
+
+  iacTestResponse.result.cloudConfigResults.forEach(


I'd use map here such as:

return iacTestResponse .result .cloudConfigResults.map(iacIssue => {...})

aviadhahami · 2020-09-14T08:15:29Z

src/cli/commands/test/index.ts

+    : stringifiedJsonData;
+
+  return {
+    dataToSend,


maybe sarifData?

Also - wouldn't you like to know which handler was called? iac/containers? (returning a type can be useful here)

aviadhahami · 2020-09-14T08:16:35Z

src/cli/commands/test/sarif-output.ts

+  };
+
+  testResult.forEach((testResult) => {
+    sarifRes.runs.push({


another place to use map (if desired) :)

src/cli/commands/test/sarif-output.ts

aviadhahami · 2020-09-14T08:17:13Z

src/cli/commands/test/sarif-output.ts

+    }
+    const level = vuln.severity == 'high' ? 'error' : 'warning';
+    const cve = vuln['identifiers']['CVE'][0];
+    tool.driver.rules?.push({


rules defined on top

For some reason my local linter doesn't like that, but trying

This definitely not accepted by linter 🤷

aviadhahami · 2020-09-14T08:18:32Z

src/cli/commands/test/sarif-output.ts

+  return sarifRes;
+}
+
+export function getTool(testResult): Sarif.Tool {


this is almost a clone of the IaC to SARIF logic. Maybe can be merged?

The actual mapping between the snyk test results and sarif is quite different depending on whether it's container or IaC. That's why we did this separately originally. If you think it's necessary, can we do the rest of the changes and leave refactoring for after this PR is merged? We're on a schedule with a planned 3rd party release.

src/cli/commands/test/sarif-output.ts

src/cli/commands/test/iac-output.ts

rontalx · 2020-09-15T14:00:58Z

src/cli/commands/test/index.ts


-  if (options.json) {
+  const {
+    stdout: dataToSend,


minor:
In this line we are renaming the returned stdout property to dataToSend.

but here:
https://github.com/snyk/snyk/pull/1384/files#diff-84cda4aeafd8ad5236629ac4ec27445cR781
We are renaming dataToSend to the stdout property.

Is this intentional?

Went with stdout as per a previous request to make the type itself a bit more understandable. In the original context where it's decided what info to actually show to the user, it seemed personally that dataToSend was more understandable, so left it as it was before. No strong feelings if there are other opinions though.

@rontalx I lack to see where we return dataToSend?

rontalx · 2020-09-15T14:05:17Z

src/lib/snyk-test/iac-test-result.ts

@@ -13,6 +13,7 @@ export interface AnnotatedIacIssue {
  // Legacy fields from Registry, unused.
  name?: string;
  from?: string[];
+  lineNumber?: number;


minor:
can you please move this to be above the comment?
// Legacy fields from Registry, unused.

rontalx · 2020-09-15T14:10:08Z

test/acceptance/cli-test/cli-test.iac-k8s.spec.ts

+    '`iac test multi-file.yaml --json - no issues`': (params, utils) => async (
+      t,
+    ) => {
+      utils.chdirWorkspaces();
+      let testableObject;
+      try {
+        await params.cli.test('iac-kubernetes/multi-file.yaml', {
+          iac: true,
+          json: true,
+        });
+        t.fail('should have thrown');
+      } catch (error) {
+        testableObject = error;
+      }
+      const res: any = JSON.parse(testableObject.message);
+      iacTestJsonAssertions(t, res, null, false);
+    },


minor:
is there any reason behind moving this test?

No idea, @orkamara added most of the tests there, might have an answer :)

aviadhahami · 2020-09-15T14:44:38Z

src/cli/commands/test/iac-output.ts

@@ -7,6 +7,9 @@ import {
 import { getSeverityValue } from './formatters';
 import { printPath } from './formatters/remediation-based-format-issues';
 import { titleCaseText } from './formatters/legacy-format-issue';
+import * as Sarif from 'sarif';


imo should be sarif

aviadhahami · 2020-09-15T14:49:06Z

src/cli/commands/test/iac-output.ts

@@ -7,6 +7,9 @@ import {
 import { getSeverityValue } from './formatters';
 import { printPath } from './formatters/remediation-based-format-issues';
 import { titleCaseText } from './formatters/legacy-format-issue';
+import * as Sarif from 'sarif';
+import { SEVERITY } from '../../../lib/snyk-test/legacy';
+import { upperFirst } from 'lodash';


Suggested change

import { upperFirst } from 'lodash';

import upperFirst from 'lodash/upperFirst';

prefer module import rather than plucking (smaller bundle size)

aviadhahami · 2020-09-15T15:03:01Z

src/cli/commands/test/index.ts


-  if (options.json) {
+  const {
+    stdout: dataToSend,


@rontalx I lack to see where we return dataToSend?

github-actions · 2020-09-15T15:26:03Z

Expected release notes (by @RotemS)

features:
SARIF format support for IaC and containers (02e9bf8)

I hereby acknowledge these release notes are 🥙 AWESOME 🥙

aviadhahami

Drive blessing

snyksec · 2020-09-15T16:00:24Z

🎉 This PR is included in version 1.398.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

orkamara assigned RotemS Sep 2, 2020

RotemS force-pushed the feat/sarif-support branch 3 times, most recently from ac37eec to 03694e7 Compare September 2, 2020 15:58

orkamara force-pushed the feat/sarif-support branch 7 times, most recently from 821a5f3 to 2fa7a29 Compare September 3, 2020 13:33

RotemS force-pushed the feat/sarif-support branch 2 times, most recently from cd2618b to 650a062 Compare September 9, 2020 11:58

RotemS marked this pull request as ready for review September 9, 2020 13:21

RotemS requested a review from a team as a code owner September 9, 2020 13:21

RotemS requested a review from a team September 9, 2020 13:21

RotemS requested a review from a team as a code owner September 9, 2020 13:21

ghost requested review from aviadhahami and ekbsnyk September 9, 2020 13:22

rontalx reviewed Sep 9, 2020

View reviewed changes

almog27 self-requested a review September 9, 2020 15:06

RotemS force-pushed the feat/sarif-support branch 2 times, most recently from a9d1958 to ef2dc07 Compare September 10, 2020 11:53

RotemS requested a review from rontalx September 10, 2020 11:53

RotemS force-pushed the feat/sarif-support branch from ef2dc07 to 5f44a7c Compare September 10, 2020 12:03

JackuB reviewed Sep 10, 2020

View reviewed changes

RotemS force-pushed the feat/sarif-support branch from 5f44a7c to d686529 Compare September 10, 2020 14:26

aviadhahami reviewed Sep 14, 2020

View reviewed changes

rontalx reviewed Sep 14, 2020

View reviewed changes

src/cli/commands/test/sarif-output.ts Outdated Show resolved Hide resolved

RotemS force-pushed the feat/sarif-support branch 8 times, most recently from b674aca to c14f685 Compare September 15, 2020 13:15

rontalx reviewed Sep 15, 2020

View reviewed changes

src/cli/commands/test/iac-output.ts Show resolved Hide resolved

rontalx reviewed Sep 15, 2020

View reviewed changes

aviadhahami reviewed Sep 15, 2020

View reviewed changes

feat: SARIF format support for IaC and containers

02e9bf8

RotemS force-pushed the feat/sarif-support branch from c14f685 to 02e9bf8 Compare September 15, 2020 15:25

aviadhahami approved these changes Sep 15, 2020

View reviewed changes

rontalx approved these changes Sep 15, 2020

View reviewed changes

RotemS merged commit c3212af into master Sep 15, 2020

RotemS deleted the feat/sarif-support branch September 15, 2020 15:55

snyksec added the released label Sep 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: SARIF format support for IaC and containers #1384

feat: SARIF format support for IaC and containers #1384

orkamara commented Sep 2, 2020 •

edited by aviadhahami

rontalx left a comment

JackuB Sep 10, 2020

RotemS Sep 10, 2020

JackuB Sep 10, 2020

JackuB Sep 10, 2020 •

edited

aviadhahami Sep 14, 2020

RotemS Sep 15, 2020

aviadhahami Sep 14, 2020

aviadhahami Sep 14, 2020

aviadhahami Sep 14, 2020

aviadhahami Sep 14, 2020

aviadhahami Sep 14, 2020

aviadhahami Sep 14, 2020

aviadhahami Sep 14, 2020

RotemS Sep 15, 2020

RotemS Sep 15, 2020

aviadhahami Sep 14, 2020

RotemS Sep 15, 2020

rontalx Sep 15, 2020 •

edited

RotemS Sep 15, 2020

aviadhahami Sep 15, 2020

rontalx Sep 15, 2020

rontalx Sep 15, 2020

RotemS Sep 15, 2020

aviadhahami Sep 15, 2020

aviadhahami Sep 15, 2020

aviadhahami Sep 15, 2020

github-actions bot commented Sep 15, 2020 •

edited by RotemS

aviadhahami left a comment

snyksec commented Sep 15, 2020

-  return str.charAt(0).toUpperCase() + str.slice(1);
+  import upperFirst from `lodash/upperFirst`
+  ...
+  return upperFirst(str);

	import { upperFirst } from 'lodash';
	import upperFirst from 'lodash/upperFirst';

feat: SARIF format support for IaC and containers #1384

feat: SARIF format support for IaC and containers #1384

Conversation

orkamara commented Sep 2, 2020 • edited by aviadhahami

What does this PR do?

How should this be manually tested?

rontalx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JackuB Sep 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rontalx Sep 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 15, 2020 • edited by RotemS

aviadhahami left a comment

Choose a reason for hiding this comment

snyksec commented Sep 15, 2020

orkamara commented Sep 2, 2020 •

edited by aviadhahami

JackuB Sep 10, 2020 •

edited

rontalx Sep 15, 2020 •

edited

github-actions bot commented Sep 15, 2020 •

edited by RotemS