feat(iac): enrich cli json results for iac with successful items (IAC-2962)

[sergiu-snyk]

Pull Request Submission

Please check the boxes once done.

The pull request must:

• Reviewer Documentation
  • follow CONTRIBUTING rules
  • be accompanied by a detailed description of the changes
  • contain a risk assessment of the change (Low | Medium | High) with regards to breaking existing functionality. A change e.g. of an underlying language plugin can completely break the functionality for that language, but appearing as only a version change in the dependencies.
  • highlight breaking API if applicable
  • contain a link to the automatic tests that cover the updated functionality.
  • contain testing instructions in case that the reviewer wants to manual verify as well, to add to the manual testing done by the author.
  • link to the link to the PR for the User-facing documentation
• User facing Documentation
  • update any relevant documentation in gitbook by submitting a gitbook PR, and including the PR link here
  • ensure that the message of the final single commit is descriptive and prefixed with either feat: or fix: , others might be used in rare occasions as well, if there is no need to document the changes in the release notes. The changes or fixes should be described in detail in the commit message for the changelog & release notes.
• Testing
  • Changes, removals and additions to functionality must be covered by acceptance / integration tests or smoke tests - either already existing ones, or new ones, created by the author of the PR.

Pull Request Review

All pull requests must undergo a thorough review process before being merged.
The review process of the code PR should include code review, testing, and any necessary feedback or revisions.
Pull request reviews of functionality developed in other teams only review the given documentation and test reports.

Manual testing will not be performed by the reviewing team, and is the responsibility of the author of the PR.

For Node projects: It’s important to make sure changes in package.json are also affecting package-lock.json correctly.

If a dependency is not necessary, don’t add it.

When adding a new package as a dependency, make sure that the change is absolutely necessary. We would like to refrain from adding new dependencies when possible.
Documentation PRs in gitbook are reviewed by Snyk's content team. They will also advise on the best phrasing and structuring if needed.

Pull Request Approval

Once a pull request has been reviewed and all necessary revisions have been made, it is approved for merging into
the main codebase. The merging of the code PR is performed by the code owners, the merging of the documentation PR
by our content writers.

What does this PR do?

Adds infrastructureAsCodeSucesses to the json output. See pitch for more info.

How should this be manually tested?

npm run build:dev
node ~/snyk/cli/dist/cli iac test --org=f7e38c85-68b1-4d42-8744-2c6a0befb3f6 --json
(the org is an organisation where IAC+ is enabled)

What are the relevant tickets?

https://snyksec.atlassian.net/browse/IAC-2962
https://snyksec.atlassian.net/browse/IAC-2969

Screenshots

To keep the output reasonably small, only very important fields were included in the list of successes.

 "infrastructureAsCodeSuccesses": [
      {
        "id": "SNYK-CC-00022",
        "title": "S3 bucket is not encrypted",
        "severity": "medium",
        "type": "terraformconfig",
        "subType": "aws_s3_bucket",
        "path": [
          "resource",
          "aws_s3_bucket[test-bucket0]"
        ],
        "msg": "resource.aws_s3_bucket[test-bucket0]",
        "isIgnored": false
      }
]

For comparison, here's the output of infrastructureAsCodeIssues:

"infrastructureAsCodeIssues": [
      {
        "severity": "medium",
        "resolve": "For AWS provider < v4.0.0, add `logging` block attribute. For AWS provider >= v4.0.0, add `aws_s3_bucket_logging` resource.",
        "impact": "Enabling server access logging provides detailed records for the requests that are made to a S3 bucket. This information is useful for security and compliance auditing purposes.",
        "msg": "resource.aws_s3_bucket[test-bucket0].logging",
        "remediation": {
          "terraform": "For AWS provider < v4.0.0, add `logging` block attribute. For AWS provider >= v4.0.0, add `aws_s3_bucket_logging` resource."
        },
        "type": "terraformconfig",
        "subType": "aws_s3_bucket",
        "issue": "S3 server access logging is disabled",
        "publicId": "SNYK-CC-00021",
        "title": "S3 server access logging is disabled",
        "references": [
          "https://docs.aws.amazon.com/AmazonS3/latest/dev/ServerLogs.html"
        ],
        "id": "SNYK-CC-00021",
        "isIgnored": false,
        "iacDescription": {
          "issue": "S3 server access logging is disabled",
          "impact": "Enabling server access logging provides detailed records for the requests that are made to a S3 bucket. This information is useful for security and compliance auditing purposes.",
          "resolve": "For AWS provider < v4.0.0, add `logging` block attribute. For AWS provider >= v4.0.0, add `aws_s3_bucket_logging` resource."
        },
        "lineNumber": 1,
        "documentation": "https://security.snyk.io/rules/cloud/SNYK-CC-00021",
        "isGeneratedByCustomRule": false,
        "path": [
          "resource",
          "aws_s3_bucket[test-bucket0]",
          "logging"
        ],
        "compliance": [],
        "description": "Enabling server access logging provides detailed records for the requests that are made to a S3 bucket. This information is useful for security and compliance auditing purposes."
      }
]

Additional questions

[sergiu-snyk]

This msg field actually contains the formatted path of the resource. It was included in the infrastructureAsCodeIssues, so I kept it here as well. Lmk if you see a reason to remove it.

[andreeaneata]

Ok from IaC point of view 🚀

[PeterSchafer]

@sergiu-snyk we took a look at your PR and from what we can see the changes generally look good!

We do have a question on the changes from a platform perspective. We manually compared the output of the latest stable CLI and the version from this branch and the output of the json data increased quite a bit. In our example from 28kB to 140kB. Since we have recently seen plenty of issues with OOM errors, we were wondering if you considered this aspect and looked at different scenarios to stress test the implementation.
A quick check of the surrounding code showed for example that the data is read from a file into a string (see here). Since strings in node (v8) have a max size limit, this is a potential issue for existing users that suddenly have increased output size breaking their pipelines.

If this new data shall be available by default I would recommend to add a stress test and use a streaming json parser in readJson().

I hope this make sense. Please feel free to reach out to me.

[sergiu-snyk]

@PeterSchafer, thank you for the time taken for a thorough review. Your comment raises valid concerns. Here are my thoughts.

There are two places where we deal with JSON output, that are impacted by my change:

1. an intermediary JSON output generated by snyk-iac-test. This is where you pointed that we read it whole into a string.

2. the final JSON output of the CLI

To stress test the implementation I used a script that generates large TF files by multiplying an AWS resource X times.

const text1 = `resource "aws_s3_bucket" "test-bucket`;
const text3 = `" {
    bucket = "test-bucket`;
const text4 = `"

tags = {
  Name        = "test"
  Environment = "Dev"
  }
}`;

let text = '';
for (let i = 0; i < 8000; i++) {
    text = `${text}${text1}${i}${text3}${i}${text4}\n`
}

console.log(text);

My findings after giving it another go at testing are:

1. for the intermediary output I noticed an increase of 60% for a large TF file (X=7000 in the script). I had failed to notice this before, because I tested with smaller files where the % increase is negligible, and I inferred that it would be the same for large files as well (there's a rawResult json field that is very large and I mistakenly thought the added passedVulnerabilties would always be small in comparison, but that is not the case for large files).

What this means is that in the CLI without my changes, the JSON read would error at X=11000, while with my changes would error at X=7000. The error in question relates to a limit in JSON.parse, not to the v8 limit of reading files larger than 2GB

To address this I could do one or all of the following:

a. remove rawResults from snyk-iac-test output (based on a new flag called --excludeRawResults). rawResults is a big structure and removing it would compensate well for the addition of passedVulnerabilities.

b. remove fields from passedVulnerabilities that we do not use to build the CLI JSON output.

These two measures would bring the intermediary JSON output to what it is now or even below.

c. use a streaming JSON parser. Depending on how we implement this we may trigger changes in the way we process it downstream, as that would need to work on chunks instead of the whole. This seems a riskier and more complicated change, but would deal with the issue better.

2. for the final CLI output, for the script generated TF file, I notice only a 20-30% increase (not sure what input was used to see the increase 28kB to 140kB), which I think is the minimum we can have given the requirements which ask us to include IaC successes in the output. In order to be 100% this does not affect existing flows, we could hide this new feature under a CLI flag. But my understanding from when the pitch was discussed (I was not with the company then) is that CLI is reluctant to adding new flags, and we should do without.

[PeterSchafer]

@sergiu-snyk this is the test data we used for comparison,

snyk iac test --json > file.json

new.json is created with a CLI from this branch
old.json is created with a stable CLI

[github-actions]

	Warnings
⚠️	Since the CLI is unifying on a standard and improved tooling, we're starting to migrate old-style imports and exports to ES6 ones. A file you've modified is using either module.exports or require(). If you can, please update them to ES6 import syntax and export syntax. Files found: src/lib/iac/test/v2/output.ts

Generated by 🚫 dangerJS against 7267d36

[sergiu-snyk]

this is exacly the content of old json.ts - to be removed when moving this feature to stable

[sergiu-snyk]

this is exactly the content of old json-old.spec.ts - to be removed when moving this feature to stable

[sergiu-snyk]

@PeterSchafer @andreeaneata ready for another look.

All new functionality is behind the "preview" flag. In order to have the preview condition in as few places as possible, I duplicated json.ts (where the bulk of the new logic is) and it's associated tests json.spec.ts.
When the feature is considered stable will remove the condition and the -old files.

Regarding the intermediary output from snyk-iac-test: it is smaller now than it used to be, thanks to the removal of rawResults.

Regarding the final output size, it varies a lot. For the input set that @PeterSchafer indicated previously we still have an increase from 27 KB to 120 KB (I removed the title field from infrastructureAsCodeSuccesses to bring this down from 144 KB). But this size difference is all due to the new infrastructureAsCodeSuccesses field, there's no way around it.
When we have input files like these with nearly no vulnerabilities, then it is expected that the "successes" will matter a lot in the output size.

[sergiu-snyk]

file for which no vulnerabilities or passedVulnerabilities are found

[sergiu-snyk]

file for which no vulnerabilities are found, but we have passedVulnerabilities