message: Before logic changes to K8 registry, which has some gnarly logic

diff:
@@ -16,30 +16,16 @@ class Registry(BaseCheckRegistry):
entity_type, entity_configuration = self.extract_entity_details(entity)
results = {}
checks = self.get_checks(entity_type)
- check_id_allowlist = runner_filter.checks
- check_id_denylist = runner_filter.skip_checks
for check in checks:
skip_info = {}
if skipped_checks:
if check.id in [x['id'] for x in skipped_checks]:
skip_info = [x for x in skipped_checks if x['id'] == check.id][0]
- if check_id_allowlist:
- if check.id in check_id_allowlist:
- self.logger.debug("Running check: {} on file {}".format(check.name, scanned_file))
- result = check.run(scanned_file=scanned_file, entity_configuration=entity_configuration,
- entity_name=entity_type, entity_type=entity_type, skip_info=skip_info)
- results[check] = result
- elif check_id_denylist:
- if check.id not in check_id_denylist:
- result = check.run(scanned_file=scanned_file, entity_configuration=entity_configuration,
- entity_name=entity_type, entity_type=entity_type, skip_info=skip_info)
- results[check] = result
- else:
+ if runner_filter.should_run_check(check.id):
self.logger.debug("Running check: {} on file {}".format(check.name, scanned_file))
result = check.run(scanned_file=scanned_file, entity_configuration=entity_configuration,
entity_name=entity_type, entity_type=entity_type, skip_info=skip_info)
-
results[check] = result
return results
'''
\ No newline at end of file

diff:
@@ -24,7 +24,7 @@ class Runner(BaseRunner):
files_list = []
if external_checks_dir:
for directory in external_checks_dir:
- arm_registry.load_external_checks(directory)
+ arm_registry.load_external_checks(directory, runner_filter)
if files:
for file in files:

diff:
@@ -24,7 +24,7 @@ class Runner(BaseRunner):
files_list = []
if external_checks_dir:
for directory in external_checks_dir:
- cfn_registry.load_external_checks(directory)
+ cfn_registry.load_external_checks(directory, runner_filter)
if files:
for file in files:

diff:
@@ -13,8 +13,19 @@ class BaseCheckRegistry(object):
self.logger = logging.getLogger(__name__)
self.checks = {}
self.check_id_allowlist = None
+ self.external_checks_runner_filter = None
def register(self, check):
+ # IMPLEMENTATION NOTE: Checks are registered when the script is loaded
+ # (see BaseResourceCheck.__init__() for the various frameworks). The only
+ # difficultly with this process is that external checks need to be specially
+ # identified for filter handling. That's why you'll see stateful setting of
+ # RunnerFilters during load_external_checks.
+ # Built-in checks are registered immediately at script start, before
+ # external checks.
+ if self.external_checks_runner_filter:
+ self.external_checks_runner_filter.notify_external_check(check.id)
+
for entity in check.supported_entities:
if entity not in self.checks.keys():
self.checks[entity] = []
@@ -45,22 +56,13 @@ class BaseCheckRegistry(object):
(entity_type, entity_name, entity_configuration) = self.extract_entity_details(entity)
results = {}
checks = self.get_checks(entity_type)
- check_id_allowlist = runner_filter.checks
- check_id_denylist = runner_filter.skip_checks
for check in checks:
skip_info = {}
if skipped_checks:
if check.id in [x['id'] for x in skipped_checks]:
skip_info = [x for x in skipped_checks if x['id'] == check.id][0]
- if check_id_allowlist:
- if check.id in check_id_allowlist:
- result = self.run_check(check, entity_configuration, entity_name, entity_type, scanned_file, skip_info)
- results[check] = result
- elif check_id_denylist:
- if check.id not in check_id_denylist:
- result = self.run_check(check, entity_configuration, entity_name, entity_type, scanned_file, skip_info)
- results[check] = result
- else:
+
+ if runner_filter.should_run_check(check.id):
result = self.run_check(check, entity_configuration, entity_name, entity_type, scanned_file, skip_info)
results[check] = result
return results
@@ -89,7 +91,7 @@ class BaseCheckRegistry(object):
return True
return False
- def load_external_checks(self, directory):
+ def load_external_checks(self, directory, runner_filter):
""" Browse a directory looking for .py files to import.
Log an error when the directory does not contains an __init__.py or
@@ -106,7 +108,12 @@ class BaseCheckRegistry(object):
for entry in directory_content:
if self._file_can_be_imported(entry):
check_name = entry.name.replace('.py', '')
+
+ # Filter is set while loading external checks so the filter can be informed
+ # of the checks, which need to be handled specially.
+ prior_external_checks_runner_filter = self.external_checks_runner_filter
try:
+ self.external_checks_runner_filter = runner_filter
self.logger.debug("Importing external check '{}'".format(check_name))
importlib.import_module(check_name)
except SyntaxError as e:
@@ -121,3 +128,6 @@ class BaseCheckRegistry(object):
error_column=e.args[1][2]
)
)
+ finally:
+ self.external_checks_runner_filter = prior_external_checks_runner_filter
+

diff:
@@ -22,6 +22,17 @@ class Registry(BaseCheckRegistry):
if skipped_checks:
if check.id in [x['id'] for x in skipped_checks]:
skip_info = [x for x in skipped_checks if x['id'] == check.id][0]
+
+ if self._should_run_scan(check.id, entity_configuration, check_id_allowlist, check_id_denylist):
+ self.logger.debug("Running check: {} on file {}".format(check.name, scanned_file))
+
+ result = check.run(scanned_file=scanned_file, entity_configuration=entity_configuration,
+ entity_name=entity_type, entity_type=entity_type, skip_info=skip_info)
+ results[check] = result
+ return results
+
+ @staticmethod
+ def _should_run_scan(check_id, entity_configuration, check_id_allowlist, check_id_denylist):
if check_id_allowlist:
# Allow list provides namespace-only allows, check-only allows, or both
# If namespaces not specified, all namespaces are scanned
@@ -39,7 +50,7 @@ class Registry(BaseCheckRegistry):
if "default" in allowed_namespaces:
run_check = True
else:
- if check.id in check_id_allowlist:
+ if check_id in check_id_allowlist:
if allowed_namespaces:
# Check if namespace in allowed namespaces
if "metadata" in entity_configuration and "namespace" in entity_configuration["metadata"]:
@@ -55,11 +66,7 @@ class Registry(BaseCheckRegistry):
# No namespaces to filter
run_check = True
if run_check:
- self.logger.debug("Running check: {} on file {}".format(check.name, scanned_file))
-
- result = check.run(scanned_file=scanned_file, entity_configuration=entity_configuration,
- entity_name=entity_type, entity_type=entity_type, skip_info=skip_info)
- results[check] = result
+ return True
elif check_id_denylist:
namespace_skip = False
if "metadata" in entity_configuration and "namespace" in entity_configuration["metadata"]:
@@ -71,14 +78,8 @@ class Registry(BaseCheckRegistry):
else:
if "default" in check_id_denylist:
namespace_skip = True
- if check.id not in check_id_denylist and namespace_skip == False:
- result = check.run(scanned_file=scanned_file, entity_configuration=entity_configuration,
- entity_name=entity_type, entity_type=entity_type, skip_info=skip_info)
- results[check] = result
+ if check_id not in check_id_denylist and namespace_skip == False:
+ return True
else:
- self.logger.debug("Running check: {} on file {}".format(check.name, scanned_file))
- result = check.run(scanned_file=scanned_file, entity_configuration=entity_configuration,
- entity_name=entity_type, entity_type=entity_type, skip_info=skip_info)
-
- results[check] = result
- return results
+ return True
+ return False

diff:
@@ -24,7 +24,7 @@ class Runner(BaseRunner):
files_list = []
if external_checks_dir:
for directory in external_checks_dir:
- registry.load_external_checks(directory)
+ registry.load_external_checks(directory, runner_filter)
if files:
for file in files:

diff:
@@ -2,6 +2,7 @@ class RunnerFilter(object):
framework = 'all'
checks = []
skip_checks = []
+ external_check_ids = set()
def __init__(self, framework='all', checks=None, skip_checks=None):
if checks is None:
@@ -18,3 +19,18 @@ class RunnerFilter(object):
else:
self.skip_checks = skip_checks
self.framework = framework
+
+ def notify_external_check(self, check_id):
+ self.external_check_ids.add(check_id)
+
+ def should_run_check(self, check_id):
+ if check_id in self.external_check_ids:
+ pass # enabled unless kipped
+ elif self.checks:
+ if check_id in self.checks:
+ return True
+ else:
+ return False
+ if self.skip_checks and check_id in self.skip_checks:
+ return False
+ return True

diff:
@@ -15,29 +15,15 @@ class Registry(BaseCheckRegistry):
entity_type, entity_configuration = self.extract_entity_details(entity)
results = {}
checks = self.get_checks(entity_type)
- check_id_allowlist = runner_filter.checks
- check_id_denylist = runner_filter.skip_checks
for check in checks:
skip_info = {}
if skipped_checks:
if check.id in [x['id'] for x in skipped_checks]:
skip_info = [x for x in skipped_checks if x['id'] == check.id][0]
- if check_id_allowlist:
- if check.id in check_id_allowlist:
- self.logger.debug("Running check: {} on file {}".format(check.name, scanned_file))
- result = check.run(scanned_file=scanned_file, entity_configuration=entity_configuration,
- entity_name=entity_type, entity_type=entity_type, skip_info=skip_info)
- results[check] = result
- elif check_id_denylist:
- if check.id not in check_id_denylist:
- result = check.run(scanned_file=scanned_file, entity_configuration=entity_configuration,
- entity_name=entity_type, entity_type=entity_type, skip_info=skip_info)
- results[check] = result
- else:
+ if runner_filter.should_run_check(check.id):
self.logger.debug("Running check: {} on file {}".format(check.name, scanned_file))
result = check.run(scanned_file=scanned_file, entity_configuration=entity_configuration,
entity_name=entity_type, entity_type=entity_type, skip_info=skip_info)
-
results[check] = result
return results

diff:
@@ -26,7 +26,7 @@ class Runner(BaseRunner):
files_list = []
if external_checks_dir:
for directory in external_checks_dir:
- sls_registry.load_external_checks(directory)
+ sls_registry.load_external_checks(directory, runner_filter)
if files:
for file in files:

diff:
@@ -43,7 +43,7 @@ class Runner(BaseRunner):
parsing_errors = {}
if external_checks_dir:
for directory in external_checks_dir:
- resource_registry.load_external_checks(directory)
+ resource_registry.load_external_checks(directory, runner_filter)
if root_folder:
root_folder = os.path.abspath(root_folder)
self.parser.hcl2(directory=root_folder, tf_definitions=self.tf_definitions, parsing_errors=parsing_errors)

diff:
+import os
+import unittest
+from unittest import mock
+
+from checkov.runner_filter import RunnerFilter
+
+
+class TestRunnerFilter(unittest.TestCase):
+
+ # Expected pseudo-code for when checks should run:
+ # if has_check_flag_specified():
+ # checks_to_run = checks_specifically_included
+ # else:
+ # checks_to_run = all_built_in_checks
+ # if has_checks_dir_specified():
+ # checks_to_run += checks_from_external_dir
+ # for skipped_check in skip_check_flags():
+ # checks_to_run.remove(skipped_check)
+
+ def test_should_run_default(self):
+ instance = RunnerFilter()
+ self.assertTrue(instance.should_run_check("CHECK_1"))
+
+ def test_should_run_specific_enable(self):
+ instance = RunnerFilter(checks=["CHECK_1"])
+ self.assertTrue(instance.should_run_check("CHECK_1"))
+
+ def test_should_run_omitted_specific_enable(self):
+ instance = RunnerFilter(checks=["CHECK_1"])
+ self.assertFalse(instance.should_run_check("CHECK_999"))
+
+ def test_should_run_specific_disable(self):
+ instance = RunnerFilter(skip_checks=["CHECK_1"])
+ self.assertFalse(instance.should_run_check("CHECK_1"))
+
+ def test_should_run_omitted_specific_disable(self):
+ instance = RunnerFilter(skip_checks=["CHECK_1"])
+ self.assertTrue(instance.should_run_check("CHECK_999"))
+
+ def test_should_run_external(self):
+ instance = RunnerFilter(skip_checks=["CHECK_1"])
+ instance.notify_external_check("EXT_CHECK_999")
+ self.assertTrue(instance.should_run_check("EXT_CHECK_999"))
+
+ def test_should_run_external2(self):
+ instance = RunnerFilter(checks=["CHECK_1"], skip_checks=["CHECK_2"])
+ instance.notify_external_check("EXT_CHECK_999")
+ self.assertTrue(instance.should_run_check("EXT_CHECK_999"))
+
+ def test_should_run_external3(self):
+ instance = RunnerFilter(checks=["EXT_CHECK_999"])
+ instance.notify_external_check("EXT_CHECK_999")
+ self.assertTrue(instance.should_run_check("EXT_CHECK_999"))
+
+ def test_should_run_external_disabled(self):
+ instance = RunnerFilter(skip_checks=["CHECK_1", "EXT_CHECK_999"])
+ instance.notify_external_check("EXT_CHECK_999")
+ self.assertFalse(instance.should_run_check("EXT_CHECK_999"))
+
+ def test_should_run_specific_disable_AND_enable(self):
+ instance = RunnerFilter(checks=["CHECK_1"], skip_checks=["CHECK_1"])
+ self.assertTrue(instance.should_run_check("CHECK_1"))
+
+
+if __name__ == '__main__':
+ unittest.main()

diff:
+import os
+import unittest
+from unittest import mock
+
+from checkov.kubernetes.base_registry import Registry
+from checkov.runner_filter import RunnerFilter
+
+
+class TestRunnerFilter(unittest.TestCase):
+
+ def test_run_by_id_default(self):
+ instance = Registry()
+ self.assertTrue(instance._should_run_scan("CKV_1", {}, [], []))
+
+ def test_run_by_id_specific_enable(self):
+ instance = Registry()
+ self.assertTrue(instance._should_run_scan("CKV_1", {}, ["CKV_1"], []))
+
+ def test_run_by_id_omitted_specific_enable(self):
+ instance = Registry()
+ self.assertFalse(instance._should_run_scan("CKV_999", {}, ["CKV_1"], []))
+
+ def test_run_by_id_specific_disable(self):
+ instance = Registry()
+ self.assertFalse(instance._should_run_scan("CKV_1", {}, [], ["CKV_1"]))
+
+ def test_run_by_id_omitted_specific_disable(self):
+ instance = Registry()
+ self.assertTrue(instance._should_run_scan("CKV_999", {}, [], ["CKV_1"]))
+
+ def test_run_by_id_external(self):
+ instance = Registry()
+ self.assertTrue(instance._should_run_scan("CKV_EXT_999", {}, [], ["CKV_1"]))
+
+ def test_run_by_id_external2(self):
+ instance = Registry()
+ self.assertTrue(instance._should_run_scan("CKV_EXT_999", {}, ["CKV_1"], ["CKV_2"]))
+
+ def test_run_by_id_external3(self):
+ instance = Registry()
+ self.assertTrue(instance._should_run_scan("CKV_EXT_999", {}, ["CKV_EXT_999"], []))
+
+ def test_run_by_id_external_disabled(self):
+ instance = Registry()
+ self.assertFalse(instance._should_run_scan("CKV_EXT_999", {}, [], ["CKV_1", "CKV_EXT_999"]))
+
+ def test_run_by_id_specific_disable_AND_enable(self):
+ instance = Registry()
+ self.assertTrue(instance._should_run_scan("CKV_1", {}, ["CKV_1"], ["CKV_1"]))
+
+ # Namespace filtering
+
+ def test_namespace_allow_default(self):
+ instance = Registry()
+ config = {"metadata": {"namespace": "not_matched"}}
+ # TODO: Maybe this case SHOULD run? Matching old state (false) for now. - @robeden
+ self.assertFalse(instance._should_run_scan("CKV_1", config, ["default"], []))
+
+ def test_namespace_deny_default(self):
+ instance = Registry()
+ config = {"metadata": {"namespace": "not_matched"}}
+ # TODO: Maybe this case SHOULDN'T run? Matching old state (true) for now. - @robeden
+ self.assertTrue(instance._should_run_scan("CKV_1", config, [], ["default"]))
+
+ def test_namespace_allow_specific(self):
+ instance = Registry()
+ config = {"metadata": {"namespace": "matched"}}
+ self.assertTrue(instance._should_run_scan("CKV_1", config, ["matched"], []))
+
+ def test_namespace_deny_specific(self):
+ instance = Registry()
+ config = {"metadata": {"namespace": "matched"}}
+ self.assertFalse(instance._should_run_scan("CKV_1", config, [], ["matched"]))
+
+ def test_namespace_allow_specific_other(self):
+ instance = Registry()
+ config = {"metadata": {"namespace": "not_matched"}}
+ self.assertFalse(instance._should_run_scan("CKV_1", config, ["something_else"], []))
+
+ def test_namespace_deny_specific_other(self):
+ instance = Registry()
+ config = {"metadata": {"namespace": "not_matched"}}
+ self.assertTrue(instance._should_run_scan("CKV_1", config, [], ["something_else"]))
+
+
+if __name__ == '__main__':
+ unittest.main()

diff:
@@ -2,6 +2,8 @@ import os
import unittest
from unittest.mock import patch
+from checkov.runner_filter import RunnerFilter
+
class TestRegistry(unittest.TestCase):
@@ -23,7 +25,7 @@ class TestRegistry(unittest.TestCase):
def test_registry_external_check_load(self):
current_dir = os.path.dirname(os.path.realpath(__file__))
external_dir = current_dir + "/example_external_dir/extra_checks"
- self.registry.load_external_checks(external_dir)
+ self.registry.load_external_checks(external_dir, RunnerFilter())
external_check_loaded = False
external_check = None

diff:
@@ -2,6 +2,8 @@ import os
import unittest
from unittest.mock import patch
+from checkov.runner_filter import RunnerFilter
+
class TestRegistry(unittest.TestCase):
@@ -22,7 +24,7 @@ class TestRegistry(unittest.TestCase):
def test_registry_external_check_load(self):
current_dir = os.path.dirname(os.path.realpath(__file__))
external_dir = current_dir + "/example_external_dir/extra_checks"
- self.registry.load_external_checks(external_dir)
+ self.registry.load_external_checks(external_dir, RunnerFilter())
external_check_loaded = False
external_check = None


message: Proposed fix for check for Redshift Cluster Logging

change_type: ADD

diff:
+from checkov.common.models.enums import CheckResult, CheckCategories
+from checkov.terraform.checks.resource.base_resource_value_check import BaseResourceValueCheck
+
+
+class RedshiftClusterLogging(BaseResourceValueCheck):
+ def __init__(self):
+ name = "Ensure Redshift Cluster logging is enabled"
+ id = "CKV_AWS_71"
+ supported_resource = ['aws_redshift_cluster']
+ categories = [CheckCategories.LOGGING]
+ super().__init__(name=name, id=id, categories=categories, supported_resources=supported_resource)
+
+ def get_inspected_key(self):
+ return 'logging/[0]/enable'
+
+check = RedshiftClusterLogging()

diff:
+import unittest
+
+import hcl2
+
+from checkov.terraform.checks.resource.aws.RedshiftClusterLogging import check
+from checkov.common.models.enums import CheckResult
+
+
+class TestRedshiftClusterLogging(unittest.TestCase):
+
+ def test_failure_missing_1(self):
+ hcl_res = hcl2.loads("""
+ resource "aws_redshift_cluster" "default" {
+ cluster_identifier = "tf-redshift-cluster"
+ database_name = "mydb"
+ master_username = "foo"
+ master_password = "Mustbe8characters"
+ node_type = "dc1.large"
+ cluster_type = "single-node"
+ }
+ """)
+ resource_conf = hcl_res['resource'][0]['aws_redshift_cluster']['default']
+ scan_result = check.scan_resource_conf(conf=resource_conf)
+ self.assertEqual(CheckResult.FAILED, scan_result)
+
+ def test_failure_missing_2(self):
+ hcl_res = hcl2.loads("""
+ resource "aws_redshift_cluster" "default" {
+ cluster_identifier = "tf-redshift-cluster"
+ database_name = "mydb"
+ master_username = "foo"
+ master_password = "Mustbe8characters"
+ node_type = "dc1.large"
+ cluster_type = "single-node"
+ logging {
+ somethingelse = "true"
+ }
+ }
+ """)
+ resource_conf = hcl_res['resource'][0]['aws_redshift_cluster']['default']
+ scan_result = check.scan_resource_conf(conf=resource_conf)
+ self.assertEqual(CheckResult.FAILED, scan_result)
+
+
+ def test_failure_false(self):
+ hcl_res = hcl2.loads("""
+ resource "aws_redshift_cluster" "default" {
+ cluster_identifier = "tf-redshift-cluster"
+ database_name = "mydb"
+ master_username = "foo"
+ master_password = "Mustbe8characters"
+ node_type = "dc1.large"
+ cluster_type = "single-node"
+ logging {
+ enable = 0
+ }
+ }
+ """)
+ resource_conf = hcl_res['resource'][0]['aws_redshift_cluster']['default']
+ scan_result = check.scan_resource_conf(conf=resource_conf)
+ self.assertEqual(CheckResult.FAILED, scan_result)
+
+ def test_success(self):
+ hcl_res = hcl2.loads("""
+ resource "aws_redshift_cluster" "default" {
+ cluster_identifier = "tf-redshift-cluster"
+ database_name = "mydb"
+ master_username = "foo"
+ master_password = "Mustbe8characters"
+ node_type = "dc1.large"
+ cluster_type = "single-node"
+ logging {
+ enable = 1
+ }
+ }
+ """)
+ resource_conf = hcl_res['resource'][0]['aws_redshift_cluster']['default']
+ scan_result = check.scan_resource_conf(conf=resource_conf)
+ self.assertEqual(CheckResult.PASSED, scan_result)
+
+if __name__ == '__main__':
+ unittest.main()

license: Apache License 2.0

repo: bridgecrewio/checkov

original_message: Proposed fix for #173 check for Redshift Cluster Logging (#514)


message: Correct CKV_AWS_72 to match Action not Principal

change_type: MODIFY

diff:
@@ -21,7 +21,7 @@ class SQSPolicy(BaseResourceCheck):
"""
if "policy" in conf.keys():
if is_json(conf["policy"][0]):
- if json.loads(conf["policy"][0])['Statement'][0]['Principal'] == '*':
+ if json.loads(conf["policy"][0])['Statement'][0]['Action'] == '*':
return CheckResult.FAILED
return CheckResult.PASSED

diff:
@@ -22,7 +22,7 @@ class TestSQSPolicy(unittest.TestCase):
"Sid": "First",
"Effect": "Allow",
"Principal": "*",
- "Action": "sqs:SendMessage",
+ "Action": "*",
"Resource": "${aws_sqs_queue.q.arn}",
"Condition": {
"ArnEquals": {


message: working PoC lambda

diff:
+requests
+boto3
\ No newline at end of file

diff:
+service: checkov-platform-poc
+
+provider:
+ name: aws
+ runtime: python3.8
+ stage: dev
+ version: 1
+ region: eu-west-2
+ memorySize: 128
+ custom:
+ IAM_COGNITO_ACCESS_KEY: ${file(./cognito_access_role.yml):IAM_COGNITO_ACCESS_KEY}
+ IAM_COGNITO_SECRET_KEY: ${file(./cognito_access_role.yml):IAM_COGNITO_SECRET_KEY}
+ BC_USER_POOL: ${file(./cognito_access_role.yml):BC_USER_POOL}
+ BC_USER_POOL_CLIENT_ID: ${file(./cognito_access_role.yml):BC_USER_POOL_CLIENT_ID}
+ BC_DASHBOARD_URL: ${file(./cognito_access_role.yml):BC_DASHBOARD_URL}
+ BC_API_URL: ${file(./cognito_access_role.yml):BC_API_URL}
+
+
+plugins:
+ - serverless-python-requirements
+package:
+ exclude:
+ - ./cognito_access_role.yml
+
+
+functions:
+ checkovUser:
+ timeout: 30
+ handler: signup.signup
+ environment:
+ Z_AWS_SECRET_ACCESS_KEY: ${self:provider.custom.IAM_COGNITO_SECRET_KEY}
+ Z_AWS_ACCESS_KEY_ID: ${self:provider.custom.IAM_COGNITO_ACCESS_KEY}
+ BC_API_URL: ${self:provider.custom.BC_API_URL}
+ BC_USER_POOL: ${self:provider.custom.BC_USER_POOL}
+ BC_USER_POOL_CLIENT_ID: ${self:provider.custom.BC_USER_POOL_CLIENT_ID}
+ BC_DASHBOARD_URL: ${self:provider.custom.BC_DASHBOARD_URL}
+ events:
+ - http:
+ path: checkovUser
+ method: post
+ cors:
+ origin: '*'
+ headers: # <-- Specify allowed headers
+ - Content-Type
+ - X-Amz-Date
+ - X-Api-Key
+ - X-Amz-Security-Token
+ - X-Amz-User-Agent
+ - Access-Control-Allow-Origin
+ - Access-Control-Allow-Headers
+ - Access-Control-Allow-Credentials
+ allowCredentials: false
+
+

diff:
+import os
+import json
+import datetime
+import requests
+import logging
+import boto3
+import random
+import string
+
+def getRandomString(length):
+ # Random string with the combination of lower and upper case
+ letters = string.ascii_letters
+ result_str = ''.join(random.choice(letters) for i in range(length))
+ return result_str + str(random.randint(0,9))
+
+def signup(event, context):
+ bc_api_url = os.environ["BC_API_URL"]
+ bc_user_pool = os.environ["BC_USER_POOL"]
+ bc_user_pool_client_id = os.environ["BC_USER_POOL_CLIENT_ID"]
+ bc_dashboard_url = os.environ["BC_DASHBOARD_URL"]
+
+ signupUrl = "{}/signup".format(bc_api_url)
+ postData = json.loads(event["body"])
+
+ userEmail = postData["email"]
+ orgName = postData["email"].split("@")[0]
+ print(orgName)
+ print(userEmail)
+ customerName = orgName
+
+
+ signupPayload = {
+ "customer_name": orgName,
+ "org_name": orgName,
+ "org_name_display": orgName,
+ "owner_email": userEmail,
+ "owner_first_name":"Checkov",
+ "owner_last_name":"User",
+ "owner_phone":"+441234567890"
+ }
+
+ signupHeaders = {
+ 'Accept': 'application/json, text/plain, */*',
+ 'User-Agent': 'Mozilla/5.0 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36',
+ 'Content-Type': 'application/json;charset=UTF-8'
+ }
+
+ response = requests.request("POST", signupUrl, headers=signupHeaders, json=signupPayload)
+
+ print(response.text.encode('utf8'))
+ if response.status_code == 200:
+ # Created platform user sucessfully. Now to get API Token
+ # Set the users password, disabling the change password prompt at login (better UX).
+ # Then, auth to cognito using the known creds to get the users JWT. Use this to call the intergrations/ApiToken endpoint. Return details to checkov along with user's password and JWT.
+ idpClient = boto3.client('cognito-idp', region_name='us-west-2', aws_access_key_id=os.environ["Z_AWS_ACCESS_KEY_ID"],aws_secret_access_key=os.environ["Z_AWS_SECRET_ACCESS_KEY"])
+ userPassword = getRandomString(8)
+ changePasswordResponse = idpClient.admin_set_user_password(
+ UserPoolId=bc_user_pool,
+ Username=userEmail,
+ Password=userPassword,
+ Permanent=True
+ )
+
+ userAuthResponse = idpClient.admin_initiate_auth(
+ UserPoolId=bc_user_pool,
+ ClientId=bc_user_pool_client_id,
+ AuthFlow='ADMIN_NO_SRP_AUTH',
+ AuthParameters={
+ 'USERNAME': userEmail,
+ 'PASSWORD': userPassword
+ }
+ )
+
+
+ apiTokenUrl = "{}/integrations/apiToken".format(bc_api_url)
+ apiTokenPayload = {}
+ ApiTokenHeaders = {
+ 'Accept': 'application/json',
+ 'Authorization': "{}".format(userAuthResponse["AuthenticationResult"]["IdToken"]),
+ 'User-Agent': 'Mozilla/5.0 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36'
+ }
+
+ apiTokenResponse = requests.request("GET", apiTokenUrl, headers=ApiTokenHeaders, data = apiTokenPayload)
+ print(apiTokenUrl)
+ print(ApiTokenHeaders)
+ print(apiTokenResponse.json())
+ print(userPassword)
+ print(userAuthResponse)
+ print("data:")
+ print(apiTokenResponse.json()["data"])
+ apiToken = apiTokenResponse.json()["data"]
+
+ returnUserDetails = {
+ "userEmail": userEmail,
+ "userPassword": userPassword,
+ "userJWT": userAuthResponse["AuthenticationResult"]["IdToken"],
+ "userApiToken": apiToken,
+ "dashboardURL": bc_dashboard_url
+ }
+
+ print(json.dumps(returnUserDetails))
+
+ response = {
+ "statusCode": 200,
+ 'headers': {
+ 'Content-Type': 'application/json',
+ 'Access-Control-Allow-Origin': '*',
+ 'Access-Control-Allow-Credentials': 'true',
+ 'Access-Control-Allow-Headers': '*'
+ },
+ "body": json.dumps(returnUserDetails)
+ }
+ return response
+
+ else:
+ response = {
+ "statusCode": 500,
+ 'headers': {
+ 'Content-Type': 'text/plain',
+ 'Access-Control-Allow-Origin': '*',
+ 'Access-Control-Allow-Credentials': 'true',
+ 'Access-Control-Allow-Headers': '*'
+ },
+ "body": "Failed to create platform user! Sorry!"
+ }
+ return response
\ No newline at end of file

original_message: working PoC lambda


message: working PoC Checkov integration

diff:
@@ -12,6 +12,9 @@ from termcolor import colored
from git import Repo
import re
from checkov.common.bridgecrew.platform_integration import BcPlatformIntegration
+import requests
+import time
+
class RunnerRegistry(object):
runners = []
@@ -70,9 +73,31 @@ class RunnerRegistry(object):
if reply[:1] == 'y':
print(Style.BRIGHT + colored("\nEmail Address? \n", 'blue', attrs=['bold']) + colored("Last prompt, promise, well automate the rest, and redirect you to your visualizations! ", 'yellow') + Style.RESET_ALL)
reply = str(input('E-Mail:')).lower().strip()
- #Ignoring the e-mail for now, we have a dummy account
- # TODO: Integrate with lambda for user creation
- bc_poc_api_token="be9a013d-7879-56b7-a33d-3a9d24b11c98"
+ print(Style.BRIGHT + colored("\nCreating Bridgecrew account & configuring Checkov ... \n", 'green', attrs=['bold']) + Style.RESET_ALL)
+ bc_poc_api = environ["BC_POC_API"]
+
+ pocApiPayload = {
+ "email": reply
+ }
+
+ signupHeaders = {
+ 'Accept': 'application/json',
+ 'User-Agent': 'Mozilla/5.0 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36',
+ 'Content-Type': 'application/json;charset=UTF-8'
+ }
+
+ pocApiResponse = requests.request("POST", bc_poc_api, headers=signupHeaders, json=pocApiPayload)
+
+ # DONE: Integrate with lambda for user creation
+ if pocApiResponse.status_code == 200:
+ bc_poc_api_token = pocApiResponse.json()["userApiToken"]
+ print(Style.BRIGHT + colored("Account Created! \n", 'green', attrs=['bold']) + Style.RESET_ALL)
+ print(Style.BRIGHT + colored("Using API Token: {} \n".format(bc_poc_api_token), 'green', attrs=['bold']) + Style.RESET_ALL)
+ #bc_poc_api_token="f4c08644-6a2f-514d-9ccf-a9b0fbe92e16"
+
+ print(Style.BRIGHT + colored("POC!! Sleeping for 3 mins waiting for upload-files-to-scanners-bucket to be created! \n", 'red', attrs=['bold']) + Style.RESET_ALL)
+ time.sleep(180)
+
if args.file:
#Work out git repo name for BC --repo-id from dir at path of --file (files var)
#TODO: DIR happy path first
@@ -88,7 +113,6 @@ class RunnerRegistry(object):
try:
repo = Repo(dir)
git_remote_uri = repo.remotes.origin.url
- #regex_repo_id = re.compile(r'^(https|git)(:\/\/|@)([^\/:]+)[\/:]([^\/:]+)\/(.+).git$')
git_repo_dict = re.match(r'(https|git)(:\/\/|@)([^\/:]+)[\/:]([^\/:]+)\/(.+).git', git_remote_uri).group(4,5)
repo_id = git_repo_dict[0] + "/" + git_repo_dict[1]
valid_repos +=1
@@ -98,6 +122,8 @@ class RunnerRegistry(object):
#No pushed repo (so no org/repo path)
repo_id = "bridgecrewuserrepo/" + path.basename(args.directory[0])
print(repo_id)
+
+
bc_integration = BcPlatformIntegration()
bc_integration.setup_bridgecrew_credentials(bc_api_key=bc_poc_api_token, repo_id=repo_id)
if bc_integration.is_integration_configured():
@@ -107,7 +133,10 @@ class RunnerRegistry(object):
bc_integration.persist_scan_results(scan_reports)
print(Style.BRIGHT + colored("Checkov report upload complete", 'green', attrs=['bold']) + Style.RESET_ALL)
bc_integration.commit_repository(args.branch)
+ print(Style.BRIGHT + colored("COMPLETE! Your Bridgecrew dashboard is available here: \n {} \n Username: {} \n Pasword: {}".format(pocApiResponse.json()["dashboardURL"],pocApiResponse.json()["userEmail"],pocApiResponse.json()["userPassword"]), 'blue', attrs=['bold']) + Style.RESET_ALL)
+ else:
+ print(Style.BRIGHT + colored("\nCould not create account, please try again on your next scan! \n", 'red', attrs=['bold']) + Style.RESET_ALL)
exit_code = 1 if 1 in exit_codes else 0

diff:
@@ -98,7 +98,7 @@ def signup(event, context):
"dashboardURL": bc_dashboard_url
}
- print(json.dumps(returnUserDetails))
+ print(json.dumps(returnUserDetails)):
response = {
"statusCode": 200,

original_message: working PoC Checkov integration


message: checkov naming for non-git repo's

diff:
@@ -120,7 +120,7 @@ class RunnerRegistry(object):
pass
if valid_repos == 0:
#No pushed repo (so no org/repo path)
- repo_id = "bridgecrewuserrepo/" + path.basename(args.directory[0])
+ repo_id = "checkovuserrepo/" + path.basename(args.directory[0])
print(repo_id)

original_message: checkov naming for non-git repo's


message: sample secrets file

diff:
+IAM_COGNITO_ACCESS_KEY: "" #IAM Credentials with access to admin-auth to Cognito userpool below
+IAM_COGNITO_SECRET_KEY: "" #IAM Credentials with access to admin-auth to Cognito userpool below
+BC_API_URL: "<Platform-API-base-URI>/v1/api/v1" #Platform API base URI.
+BC_USER_POOL: "" # Cognito Userpool ID
+BC_USER_POOL_CLIENT_ID: "" # Cognito Userpool App Client ID
+BC_DASHBOARD_URL: "" #Dashboard URI from the same platform instance as BC_API_URL environment

original_message: sample secrets file


message:
Add AuthenticatedRead/authenticated-read to set of "public" ACL values for s3 buckets
Fixes: 596

diff:
@@ -11,7 +11,7 @@ class S3AccessLogs(BaseResourceCheck):
super().__init__(name=name, id=id, categories=categories, supported_resources=supported_resources)
def scan_resource_conf(self, conf):
- if conf.get('Properties') and conf['Properties'].get('AccessControl') in ['PublicReadWrite', 'PublicRead']:
+ if conf.get('Properties') and conf['Properties'].get('AccessControl') in ['PublicReadWrite', 'PublicRead', 'AuthenticatedRead']:
return CheckResult.FAILED
return CheckResult.PASSED

diff:
@@ -14,7 +14,7 @@ class S3PublicACLRead(BaseResourceNegativeValueCheck):
return 'acl'
def get_forbidden_values(self):
- return ["public-read", "public-read-write", "website"]
+ return ["public-read", "public-read-write", "website", "authenticated-read"]
check = S3PublicACLRead()

diff:
@@ -3,6 +3,7 @@ import unittest
from checkov.cloudformation.checks.resource.aws.S3BlockPublicACLs import check
from checkov.cloudformation.runner import Runner
+from checkov.common.models.enums import CheckResult
from checkov.runner_filter import RunnerFilter
@@ -21,6 +22,16 @@ class TestS3BlockPublicACLs(unittest.TestCase):
self.assertEqual(summary['skipped'], 0)
self.assertEqual(summary['parsing_errors'], 0)
+ def test_failure_auth_read(self):
+ resource_conf = {
+ "Type": "AWS::S3::Bucket",
+ "Properties": {
+ "AccessControl": "AuthenticatedRead"
+ }
+ }
+ scan_result = check.scan_resource_conf(conf=resource_conf)
+ self.assertEqual(CheckResult.FAILED, scan_result)
+
if __name__ == '__main__':
unittest.main()

diff:
@@ -16,6 +16,16 @@ class TestS3PublicACL(unittest.TestCase):
scan_result = check.scan_resource_conf(conf=resource_conf)
self.assertEqual(CheckResult.FAILED, scan_result)
+ def test_failure_auth_read(self):
+ resource_conf = {"region": ["us-west-2"],
+ "bucket": ["my_bucket"],
+ "acl": ["authenticated-read"],
+ "force_destroy": [True],
+ "tags": [{"Name": "my-bucket"}],
+ }
+ scan_result = check.scan_resource_conf(conf=resource_conf)
+ self.assertEqual(CheckResult.FAILED, scan_result)
+
def test_success(self):
resource_conf = {"region": ["us-west-2"],
"bucket": ["my_bucket"],


message: Prevent S3 lockouts, tests for S3:* DENY resource policies.

diff:
+from checkov.common.models.enums import CheckResult, CheckCategories
+from checkov.terraform.checks.resource.base_resource_check import BaseResourceCheck
+import json
+
+
+class S3ProtectAgainstPolicyLockout(BaseResourceCheck):
+
+ def __init__(self):
+ name = "Ensure S3 bucket policy does not lockout all but root user. (Prevent lockouts needing root account fixes)"
+ id = "CKV_AWS_89"
+ supported_resources = ['aws_s3_bucket', 'aws_s3_bucket_policy']
+ categories = [CheckCategories.IAM]
+ super().__init__(name=name, id=id, categories=categories, supported_resources=supported_resources)
+
+ def scan_resource_conf(self, conf):
+ if 'policy' in conf.keys():
+ if isinstance(conf['policy'][0], str):
+ try:
+ policy_block = json.loads(conf['policy'][0])
+ if 'Statement' in policy_block.keys():
+ for statement in policy_block['Statement']:
+ if statement['Effect'] == 'Allow':
+ continue
+
+ principal = statement['Principal']
+
+ if principal == '*':
+ return CheckResult.FAILED
+ elif 'AWS' in statement['Principal']:
+ # Can be a string or an array of strings
+ aws = statement['Principal']['AWS']
+ if (type(aws) == str and aws == '*') or (type(aws) == list and '*' in aws):
+ return CheckResult.FAILED
+
+ action = statement['Action']
+
+ if action == '*':
+ return CheckResult.FAILED
+ elif 's3' in statement['Action']:
+ # Can be a string or an array of strings
+ s3 = statement['Action']['s3']
+ if (type(s3) == str and s3 == '*') or (type(s3) == list and '*' in s3):
+ return CheckResult.FAILED
+ except: # nosec
+ pass
+ return CheckResult.PASSED
+
+check = S3ProtectAgainstPolicyLockout()

diff:
+import unittest
+import hcl2
+
+from checkov.terraform.checks.resource.aws.S3ProtectAgainstPolicyLockout import check
+from checkov.common.models.enums import CheckResult
+
+
+class TestS3ProtectAgainstPolicyLockout(unittest.TestCase):
+
+ def test_failure(self):
+ hcl_res = hcl2.loads("""
+ resource "aws_s3_bucket" "s3" {
+ bucket = "bucket"
+
+ policy = <<POLICY
+ {
+ "Version": "2012-10-17",
+ "Statement": [
+ {
+ "Principal": {
+ "AWS": [
+ "*"
+ ]
+ },
+ "Effect": "Deny",
+ "Action": [
+ "s3:*"
+ ],
+ "Resource": [
+ "*"
+ ]
+ }
+ ]
+ }
+ POLICY
+ }
+ """)
+ resource_conf = hcl_res['resource'][0]['aws_s3_bucket']['s3']
+ scan_result = check.scan_resource_conf(conf=resource_conf)
+ self.assertEqual(CheckResult.FAILED, scan_result)
+
+ def test_failure_2(self):
+ hcl_res = hcl2.loads("""
+ resource "aws_s3_bucket_policy" "s3" {
+ bucket = "bucket"
+
+ policy = <<POLICY
+ {
+ "Version": "2012-10-17",
+ "Statement": [{
+ "Principal": {
+ "AWS": [
+ "*"
+ ]
+ },
+ "Effect": "Deny",
+ "Action": "s3:*",
+ "Resource": [
+ "*"
+ ]
+ }]
+ }
+ POLICY
+ }
+ """)
+ resource_conf = hcl_res['resource'][0]['aws_s3_bucket_policy']['s3']
+ scan_result = check.scan_resource_conf(conf=resource_conf)
+ self.assertEqual(CheckResult.FAILED, scan_result)
+
+ def test_failure_3(self):
+ hcl_res = hcl2.loads("""
+ resource "aws_s3_bucket" "s3" {
+ bucket = "bucket"
+
+ policy = <<POLICY
+ {
+ "Version": "2012-10-17",
+ "Statement": [
+ {
+ "Principal": "*",
+ "Effect": "Deny",
+ "Action": "s3:*"
+ }
+ ]
+ }
+ POLICY
+ }
+ """)
+ resource_conf = hcl_res['resource'][0]['aws_s3_bucket']['s3']
+ scan_result = check.scan_resource_conf(conf=resource_conf)
+ self.assertEqual(CheckResult.FAILED, scan_result)
+
+ def test_success_policyobj(self):
+ hcl_res = hcl2.loads("""
+ resource "aws_s3_bucket_policy" "s3" {
+ bucket = "bucket"
+
+ policy = <<POLICY
+ {
+ "Id": "Policy1597273448050",
+ "Version": "2012-10-17",
+ "Statement": [
+ {
+ "Sid": "Stmt1597273446725",
+ "Action": [
+ "s3:GetObject"
+ ],
+ "Effect": "Deny",
+ "Resource": "arn:aws:s3:::bucket/*",
+ "Principal": {
+ "AWS": "some_arn"
+ }
+ }
+ ]
+ }
+ POLICY
+ }
+ """)
+ resource_conf = hcl_res['resource'][0]['aws_s3_bucket_policy']['s3']
+ scan_result = check.scan_resource_conf(conf=resource_conf)
+ self.assertEqual(CheckResult.PASSED, scan_result)
+
+
+if __name__ == '__main__':
+ unittest.main()


message: S3 Lockout check improvements based on PR feedback

diff:
from checkov.common.models.enums import CheckResult, CheckCategories
from checkov.terraform.checks.resource.base_resource_check import BaseResourceCheck
+from checkov.common.util.type_forcers import force_list
import json
@@ -7,7 +8,7 @@ class S3ProtectAgainstPolicyLockout(BaseResourceCheck):
def __init__(self):
name = "Ensure S3 bucket policy does not lockout all but root user. (Prevent lockouts needing root account fixes)"
- id = "CKV_AWS_89"
+ id = "CKV_AWS_93"
supported_resources = ['aws_s3_bucket', 'aws_s3_bucket_policy']
categories = [CheckCategories.IAM]
super().__init__(name=name, id=id, categories=categories, supported_resources=supported_resources)
@@ -18,7 +19,18 @@ class S3ProtectAgainstPolicyLockout(BaseResourceCheck):
try:
policy_block = json.loads(conf['policy'][0])
if 'Statement' in policy_block.keys():
- for statement in policy_block['Statement']:
+ for statement in force_list(policy_block['Statement']):
+ if 'Condition' in statement.keys():
+ # https://github.com/bridgecrewio/checkov/pull/627#issuecomment-714681751
+ continue
+
+ if 'NotAction' in statement.keys():
+ continue
+
+ if 'Effect' not in statement.keys():
+ # Defaults to allow if doesn't exist.
+ continue
+
if statement['Effect'] == 'Allow':
continue

diff:
@@ -90,6 +90,65 @@ class TestS3ProtectAgainstPolicyLockout(unittest.TestCase):
scan_result = check.scan_resource_conf(conf=resource_conf)
self.assertEqual(CheckResult.FAILED, scan_result)
+ def test_skip_noeffect(self):
+ hcl_res = hcl2.loads("""
+ resource "aws_s3_bucket_policy" "s3" {
+ bucket = "bucket"
+
+ policy = <<POLICY
+ {
+ "Id": "Policy1597273448050",
+ "Version": "2012-10-17",
+ "Statement": [
+ {
+ "Sid": "Stmt1597273446725",
+ "Action": [
+ "s3:GetObject"
+ ],
+ "Resource": "arn:aws:s3:::bucket/*",
+ "Principal": {
+ "AWS": "some_arn"
+ }
+ }
+ ]
+ }
+ POLICY
+ }
+ """)
+ resource_conf = hcl_res['resource'][0]['aws_s3_bucket_policy']['s3']
+ scan_result = check.scan_resource_conf(conf=resource_conf)
+ self.assertEqual(CheckResult.PASSED, scan_result)
+
+ def test_skip_notaction(self):
+ hcl_res = hcl2.loads("""
+ resource "aws_s3_bucket_policy" "s3" {
+ bucket = "bucket"
+
+ policy = <<POLICY
+ {
+ "Id": "Policy1597273448050",
+ "Version": "2012-10-17",
+ "Statement": [
+ {
+ "Sid": "Stmt1597273446725",
+ "NotAction": [
+ "s3:GetObject"
+ ],
+ "Effect": "Deny",
+ "Resource": "arn:aws:s3:::bucket/*",
+ "Principal": {
+ "AWS": "some_arn"
+ }
+ }
+ ]
+ }
+ POLICY
+ }
+ """)
+ resource_conf = hcl_res['resource'][0]['aws_s3_bucket_policy']['s3']
+ scan_result = check.scan_resource_conf(conf=resource_conf)
+ self.assertEqual(CheckResult.PASSED, scan_result)
+
def test_success_policyobj(self):
hcl_res = hcl2.loads("""
resource "aws_s3_bucket_policy" "s3" {
@@ -120,6 +179,33 @@ class TestS3ProtectAgainstPolicyLockout(unittest.TestCase):
scan_result = check.scan_resource_conf(conf=resource_conf)
self.assertEqual(CheckResult.PASSED, scan_result)
+ def test_success_statementnotlist(self):
+ hcl_res = hcl2.loads("""
+ resource "aws_s3_bucket_policy" "s3" {
+ bucket = "bucket"
+
+ policy = <<POLICY
+ {
+ "Id": "Policy1597273448050",
+ "Version": "2012-10-17",
+ "Statement": {
+ "Sid": "Stmt1597273446725",
+ "Action": [
+ "s3:GetObject"
+ ],
+ "Effect": "Deny",
+ "Resource": "arn:aws:s3:::bucket/*",
+ "Principal": {
+ "AWS": "some_arn"
+ }
+ }
+ }
+ POLICY
+ }
+ """)
+ resource_conf = hcl_res['resource'][0]['aws_s3_bucket_policy']['s3']
+ scan_result = check.scan_resource_conf(conf=resource_conf)
+ self.assertEqual(CheckResult.PASSED, scan_result)
if __name__ == '__main__':
unittest.main()


message: S3 Lockout check improvements based on PR feedback 2/2

diff:
@@ -27,11 +27,7 @@ class S3ProtectAgainstPolicyLockout(BaseResourceCheck):
if 'NotAction' in statement.keys():
continue
- if 'Effect' not in statement.keys():
- # Defaults to allow if doesn't exist.
- continue
-
- if statement['Effect'] == 'Allow':
+ if statement.get('Effect') != 'Deny':
continue
principal = statement['Principal']


message:
Fix checks not using force_list() when looping on policy_block['statement']
statement may not always be a list.

diff:
from checkov.common.models.enums import CheckResult, CheckCategories
from checkov.terraform.checks.resource.base_resource_check import BaseResourceCheck
+from checkov.common.util.type_forcers import force_list
import json
@@ -17,7 +18,7 @@ class IAMAdminPolicyDocument(BaseResourceCheck):
try:
policy_block = json.loads(conf['policy'][0])
if 'Statement' in policy_block.keys():
- for statement in policy_block['Statement']:
+ for statement in force_list(policy_block['Statement']):
if 'Action' in statement and \
statement.get('Effect', ['Allow']) == 'Allow' and \
'*' in statement.get('Action', ['']) and \

diff:
from checkov.common.models.enums import CheckResult, CheckCategories
from checkov.terraform.checks.resource.base_resource_check import BaseResourceCheck
+from checkov.common.util.type_forcers import force_list
import json
@@ -17,7 +18,7 @@ class IAMStarActionPolicyDocument(BaseResourceCheck):
try:
policy_block = json.loads(conf['policy'][0])
if 'Statement' in policy_block.keys():
- for statement in policy_block['Statement']:
+ for statement in force_list(policy_block['Statement']):
if 'Action' in statement and \
statement.get('Effect', ['Allow']) == 'Allow' and \
'*' in statement.get('Action', ['']):

diff:
from checkov.common.models.enums import CheckResult, CheckCategories
from checkov.terraform.checks.resource.base_resource_check import BaseResourceCheck
+from checkov.common.util.type_forcers import force_list
import json
@@ -18,7 +19,7 @@ class S3AllowsAnyPrincipal(BaseResourceCheck):
try:
policy_block = json.loads(conf['policy'][0])
if 'Statement' in policy_block.keys():
- for statement in policy_block['Statement']:
+ for statement in force_list(policy_block['Statement']):
if statement['Effect'] == 'Deny':
continue
if 'Principal' not in statement:


message:
Avoid `_clean_bad_definitions` when loading json files
Fixes:

diff:
@@ -701,7 +701,7 @@ Load JSON or HCL, depending on filename.
try:
with open(file, "r") as f:
if file_name.endswith(".json"):
- return _clean_bad_definitions(json.load(f))
+ return json.load(f)
else:
return _clean_bad_definitions(hcl2.load(f))
except Exception as e:
@@ -712,7 +712,8 @@ Load JSON or HCL, depending on filename.
def _clean_bad_definitions(tf_definition_list):
return {
- block_type: list(filter(lambda definition_list: block_type == 'locals' or len(definition_list.keys()) == 1, tf_definition_list[block_type]))
+ block_type: list(filter(lambda definition_list: block_type == 'locals' or
+ len(definition_list.keys()) == 1, tf_definition_list[block_type]))
for block_type in tf_definition_list.keys()
}

diff:
+{
+ "variable": {
+ "environment": {
+ "type": "string"
+ },
+ "aws_region": {
+ "default": "us-east-1",
+ "type": "string"
+ },
+ "aws_profile": {
+ "type": "string"
+ }
+ },
+ "terraform": {
+ "required_providers": {
+ "aws": {
+ "version": "~> 2.70.0",
+ "source": "aws"
+ }
+ }
+ },
+ "provider": {
+ "aws": [
+ {
+ "profile": "${var.aws_profile}",
+ "region": "${var.aws_region}",
+ "alias": "default"
+ },
+ {
+ "profile": "external",
+ "region": "us-west-1",
+ "skip_requesting_account_id": true,
+ "alias": "external"
+ }
+ ]
+ },
+ "resource": {
+ "aws_secretsmanager_secret": {
+ "local-secret": {
+ "name": "internal-secret",
+ "provider": "aws.default"
+ },
+ "external-secret": {
+ "name": "external-secret",
+ "provider": "aws.external"
+ }
+ }
+ }
+}
\ No newline at end of file

diff:
+{
+ "cdk.tf.json": {
+ "variable": {
+ "environment": {
+ "type": "string"
+ },
+ "aws_region": {
+ "default": "us-east-1",
+ "type": "string"
+ },
+ "aws_profile": {
+ "type": "string"
+ }
+ },
+ "terraform": {
+ "required_providers": {
+ "aws": {
+ "version": "~> 2.70.0",
+ "source": "aws"
+ }
+ }
+ },
+ "provider": {
+ "aws": [
+ {
+ "profile": "${var.aws_profile}",
+ "region": "${var.aws_region}",
+ "alias": "default"
+ },
+ {
+ "profile": "external",
+ "region": "us-west-1",
+ "skip_requesting_account_id": true,
+ "alias": "external"
+ }
+ ]
+ },
+ "resource": {
+ "aws_secretsmanager_secret": {
+ "local-secret": {
+ "name": "internal-secret",
+ "provider": "aws.default"
+ },
+ "external-secret": {
+ "name": "external-secret",
+ "provider": "aws.external"
+ }
+ }
+ }
+ }
+}
\ No newline at end of file

diff:
@@ -84,6 +84,7 @@ class TestParserScenarios(unittest.TestCase):
self.go("doc_evaluations_verify")
def test_bad_tf(self):
+ # Note: this hits the _clean_bad_definitions internal function
self.go("bad_tf")
def test_null_variables_651(self):
@@ -94,6 +95,9 @@ class TestParserScenarios(unittest.TestCase):
# Run only manually, this test currently fails on multiple issues
self.go("count_eval")
+ def test_json_807(self):
+ self.go("json_807")
+
@staticmethod
def go(dir_name):
dir_path = os.path.join(os.path.dirname(os.path.realpath(__file__)),

original_message:
Avoid `_clean_bad_definitions` when loading json files (#808)

Fixes: #807


message: Test cases and top-level loop support

diff:
@@ -310,9 +310,28 @@ class Parser:
if isinstance(value, str):
altered_value = value
- had_pattern_match = False
-
+ had_var_block_match = False
+
+ # The way in which matches are processed is important as they are ordered and may contain
+ # portions of one another. For example:
+ # ${merge(local.common_tags,local.common_data_tags,{'Name': 'my-thing-${var.ENVIRONMENT}-${var.REGION}'})}
+ # In this case, we expect blocks similar to this:
+ # 1) ${var.ENVIRONMENT}
+ # 2) ${var.REGION}
+ # 3) ${merge(local.common_tags,local.common_data_tags,{'Name': 'my-thing-${var.ENVIRONMENT}-${var.REGION}'})}
+ # If either of the first two are replaced, we can still process the outer eval block
+ # if the substitutions made to the earlier vars are also made to the later. That allows
+ # knowing what the string should really look like so substitutions can be made properly.
+ # If this proves not to work well, the other option is to abort the later (because
+ # the full string isn't found in the value anymore) and come back to it on another
+ # processor loop. This works... but requires another processor loop.
+ # (If you're thinking we should make a DAG and do this properly... you're probably right.)
+ prev_matches: List[Tuple[str, str]] = [] # original value -> replaced
for match in _find_var_blocks(value):
+ # Update what's expected in the match, see comment above
+ for prev_match in prev_matches:
+ match.replace(prev_match[0], prev_match[1])
+
var_base = match.var_only
# Expressions such as (from variable definition):
@@ -321,7 +340,8 @@ class Parser:
# "type = ${string}"
if var_base in _SIMPLE_TYPES and match.full_str == value:
altered_value = var_base
- had_pattern_match = True
+ had_var_block_match = True
+ prev_matches.append((match.full_str, var_base))
else:
replaced = _handle_single_var_pattern(var_base, var_value_and_file_map,
locals_values, resource_list,
@@ -333,9 +353,10 @@ class Parser:
altered_value = replaced
else:
altered_value = altered_value.replace(match.full_str, str(replaced))
- had_pattern_match = True
+ prev_matches.append((match.full_str, replaced))
+ had_var_block_match = True
- if not had_pattern_match:
+ if not had_var_block_match:
# tomap is annoying because the curly braces in the string break the regex. Rather than
# coming up with something that's significantly more complex, we'll special case this
# check. Only do this when there wasn't a pattern match to make sure there are no
@@ -812,10 +833,15 @@ class VarBlockMatch:
full_str: str # Example: ${local.foo}
var_only: str # Example: local.fop
+ def replace(self, original: str, replaced: str):
+ self.full_str = self.full_str.replace(original, replaced)
+ self.var_only = self.var_only.replace(original, replaced)
+
def _find_var_blocks(value: str) -> List[VarBlockMatch]:
"""
- Find and return all the var blocks within a given string.
+ Find and return all the var blocks within a given string. Order is important and may contain portions of
+ one another.
"""
# Note: This used to be implemented with a regex: r'\${([^{}]+?)}')

diff:
+locals {
+ common_tags = {
+ Tag1 = "one"
+ Tag2 = "two"
+ }
+}
+
+variable "ENV" {}
+
+resource "aws_s3_bucket" "bucket" {
+ # var.ENV has no default, so need to evaluate the merge without the
+ # fully resolved statement.
+ tags = merge(local.common_tags, {Name = "my-bucket-${var.ENV}"})
+}
\ No newline at end of file

diff:
+{
+ "main.tf": {
+ "BUCKET_NAME": {
+ "var_file": "variables.tf",
+ "value": "this-is-my-default",
+ "definitions": [
+ {
+ "definition_name": "BUCKET_NAME",
+ "definition_expression": "${var.BUCKET_NAME}",
+ "definition_path": "resource/0/aws_s3_bucket/test/bucket/0"
+ }
+ ]
+ }
+ }
+}
\ No newline at end of file

diff:
+{
+ "main.tf": {
+ "resource": [
+ {
+ "aws_s3_bucket": {
+ "test": {
+ "bucket": [
+ "this-is-my-default"
+ ]
+ }
+ }
+ }
+ ]
+ },
+ "variables.tf": {
+ "variable": [
+ {
+ "BUCKET_NAME": {
+ "type": ["string"],
+ "default": ["this-is-my-default"]
+ }
+ }
+ ]
+ }
+}
\ No newline at end of file

diff:
+resource "aws_s3_bucket" "test" {
+ bucket = var.BUCKET_NAME
+}

diff:
+variable "BUCKET_NAME" {
+ type = string
+ default = "this-is-my-default"
+}
\ No newline at end of file

change_type: RENAME

diff:
@@ -55,6 +55,24 @@ class TestHCL2LoadAssumptions(unittest.TestCase):
}
self.go(tf, expect)
+ def test_merge_with_inner_var(self):
+ tf = '''
+ resource "aws_s3_bucket" "foo" {
+ tags = merge(local.common_tags, local.common_data_tags, {Name = "my-thing-${var.ENVIRONMENT}-${var.REGION}"})
+ }'''
+ expect = {
+ "resource": [
+ {
+ "aws_s3_bucket": {
+ "foo": {
+ "tags": ["${merge(local.common_tags,local.common_data_tags,{'Name': 'my-thing-${var.ENVIRONMENT}-${var.REGION}'})}"]
+ }
+ }
+ }
+ ]
+ }
+ self.go(tf, expect)
+
def test_variable_block(self):
tf = '''
variable "my_var" {

diff:
@@ -70,7 +70,16 @@ class TestParserInternals(unittest.TestCase):
"merge({\"a\": \"}, evil\"},{\"b\": \"\\\" , evil\"})")]),
("$${foo}", []), # escape interpolation
('${merge({\'a\': \'}, evil\'})}',
- [VBM('${merge({\'a\': \'}, evil\'})}', 'merge({\'a\': \'}, evil\'})')])
+ [VBM('${merge({\'a\': \'}, evil\'})}', 'merge({\'a\': \'}, evil\'})')]),
+
+ # Ordered returning of sub-vars and then outer var.
+ ("${merge(local.common_tags,local.common_data_tags,{'Name': 'my-thing-${var.ENVIRONMENT}-${var.REGION}'})}",
+ [
+ VBM("${var.ENVIRONMENT}", "var.ENVIRONMENT"),
+ VBM("${var.REGION}", "var.REGION"),
+ VBM("${merge(local.common_tags,local.common_data_tags,{'Name': 'my-thing-${var.ENVIRONMENT}-${var.REGION}'})}",
+ "merge(local.common_tags,local.common_data_tags,{'Name': 'my-thing-${var.ENVIRONMENT}-${var.REGION}'})")
+ ])
]
for case in cases:
actual = parser._find_var_blocks(case[0])

diff:
@@ -28,6 +28,9 @@ class TestParserScenarios(unittest.TestCase):
def test_variable_defaults(self):
self.go("variable_defaults")
+ def test_variable_defaults_separate_files(self):
+ self.go("variable_defaults_separate_files")
+
def test_local_block(self):
self.go("local_block")
@@ -40,6 +43,9 @@ class TestParserScenarios(unittest.TestCase):
def test_merge_function(self):
self.go("merge_function")
+ def test_merge_function_unresolved_var(self):
+ self.go("merge_function_unresolved_var")
+
def test_tobool_function(self):
self.go("tobool_function")

original_message: Test cases and top-level loop support


diff:
@@ -8,7 +8,6 @@ from typing import Mapping, Optional, Dict, Any, List, Callable, Tuple
import deep_merge
import hcl2
import jmespath
-from dataclasses import dataclass
from checkov.common.runners.base_runner import filter_ignored_directories
from checkov.common.util.consts import DEFAULT_EXTERNAL_MODULES_DIR, RESOLVED_MODULE_ENTRY_NAME
@@ -16,7 +15,7 @@ from checkov.common.util.type_forcers import convert_str_to_bool
from checkov.common.variables.context import EvaluationContext, VarReference
from checkov.terraform.module_loading.registry import ModuleLoaderRegistry
from checkov.terraform.module_loading.registry import module_loader_registry as default_ml_registry
-
+from checkov.terraform.parser_var_blocks import find_var_blocks, split_merge_args
LOGGER = logging.getLogger(__name__)
@@ -327,7 +326,7 @@ class Parser:
# processor loop. This works... but requires another processor loop.
# (If you're thinking we should make a DAG and do this properly... you're probably right.)
prev_matches: List[Tuple[str, str]] = [] # original value -> replaced
- for match in _find_var_blocks(value):
+ for match in find_var_blocks(value):
# Update what's expected in the match, see comment above
for prev_match in prev_matches:
match.replace(prev_match[0], prev_match[1])
@@ -535,10 +534,7 @@ def _handle_single_var_pattern(orig_variable: str, var_value_and_file_map: Dict[
module_data_retrieval: Callable[[str], Dict[str, Any]],
eval_map_by_var_name: Dict[str, EvaluationContext],
context, orig_variable_full, root_directory: str) -> Any:
- if "${" in orig_variable:
- return orig_variable
-
- elif orig_variable.startswith("module."):
+ if orig_variable.startswith("module."):
if not module_list:
return orig_variable
@@ -647,7 +643,7 @@ def _handle_single_var_pattern(orig_variable: str, var_value_and_file_map: Dict[
return orig_variable # no change
elif orig_variable.startswith("merge(") and orig_variable.endswith(")"):
altered_value = orig_variable[6:-1]
- args = _split_merge_args(altered_value)
+ args = split_merge_args(altered_value)
if args is None:
return orig_variable
merged_map = {}
@@ -780,160 +776,3 @@ def _remove_module_dependency_in_path(path):
if re.findall(resolved_module_pattern, path):
path = re.sub(resolved_module_pattern, '', path)
return path
-
-
-def _split_merge_args(value: str) -> Optional[List[str]]:
- """
- Split arguments of a merge function. For example, "merge(local.one, local.two)" would
- call this function with a value of "local.one, local.two" which would return
- ["local.one", "local.two"]. If the value cannot be unpacked, None will be returned.
- """
- if not value:
- return None
-
- # There are a number of splitting scenarios depending on whether variables or
- # direct maps are used:
- # merge({tag1="foo"},{tag2="bar"})
- # merge({tag1="foo"},local.some_tags)
- # merge(local.some_tags,{tag2="bar"})
- # merge(local.some_tags,local.some_other_tags)
- # Also, the number of arguments can vary, things can be nested, strings are evil...
- # See tests/terraform/test_parser_internals.py for many examples.
-
- to_return = []
- current_arg_buffer = ""
- processing_str_escape = False
- inside_collection_stack = [] # newest at position 0, contains the terminator for the collection
- for c in value:
- if c == "," and not inside_collection_stack:
- current_arg_buffer = current_arg_buffer.strip()
- # Note: can get a zero-length buffer when there's a double comman. This can
- # happen with multi-line args (see parser_internals test)
- if len(current_arg_buffer) != 0:
- to_return.append(current_arg_buffer)
- current_arg_buffer = ""
- else:
- current_arg_buffer += c
-
- processing_str_escape = _str_parser_loop_collection_helper(c,
- inside_collection_stack,
- processing_str_escape)
-
- current_arg_buffer = current_arg_buffer.strip()
- if len(current_arg_buffer) > 0:
- to_return.append(current_arg_buffer)
-
- if len(to_return) == 0:
- return None
- return to_return
-
-
-@dataclass
-class VarBlockMatch:
- full_str: str # Example: ${local.foo}
- var_only: str # Example: local.fop
-
- def replace(self, original: str, replaced: str):
- self.full_str = self.full_str.replace(original, replaced)
- self.var_only = self.var_only.replace(original, replaced)
-
-
-def _find_var_blocks(value: str) -> List[VarBlockMatch]:
- """
- Find and return all the var blocks within a given string. Order is important and may contain portions of
- one another.
- """
-
- # Note: This used to be implemented with a regex: r'\${([^{}]+?)}')
- # That found ${...} without a {} inside. However, this caused issues with things containing maps
- # which needed to be processed.
-
- to_return: List[VarBlockMatch] = []
- eval_buffer = ""
- in_eval = False
- preceding_dollar = False
- processing_str_escape = False
- inside_collection_stack: List[str] = [] # newest at position 0, contains terminator for collection
- for c in value:
- if c == "$":
- if preceding_dollar: # ignore double $
- preceding_dollar = False
- continue
- preceding_dollar = True
- elif c == "{" and preceding_dollar:
- # NOTE: An eval block can start within another eval block, in which case we drop the old
- # stuff and process this one.
- in_eval = True
- eval_buffer = "" # reset buffer
- inside_collection_stack.clear() # reset stack
- processing_str_escape = False
- preceding_dollar = False
- continue
- else:
- preceding_dollar = False
-
- if not in_eval:
- continue
-
- if c == "}" and not inside_collection_stack:
- eval_buffer = eval_buffer.strip()
- if len(eval_buffer) == 0:
- # Something went wrong because we have an empty arg. Blow out.
- return []
- to_return.append(VarBlockMatch("${" + eval_buffer + "}", eval_buffer))
- eval_buffer = ""
- in_eval = False
- continue
- else:
- eval_buffer += c
-
- processing_str_escape = _str_parser_loop_collection_helper(c, inside_collection_stack,
- processing_str_escape)
-
- return to_return
-
-
-def _str_parser_loop_collection_helper(c: str, inside_collection_stack: List[str],
- processing_str_escape: bool) -> bool:
- """
- This function handles dealing with tracking when a char-by-char state loop is inside a
- "collection" (map, array index, method args, string).
-
- :param c: Active character
- :param inside_collection_stack: Stack of terminators for collections. This will be modified by
- this function. The active terminator will be at position 0.
-
-
- :return: value to set for `processing_str_escape`
- """
- inside_a_string = False
- if inside_collection_stack:
- terminator = inside_collection_stack[0]
-
- if terminator == '"' or terminator == "'":
- if processing_str_escape:
- processing_str_escape = False
- return processing_str_escape
- elif c == "\\":
- processing_str_escape = True
- return processing_str_escape
- else:
- inside_a_string = True
-
- if c == terminator:
- del inside_collection_stack[0]
- return processing_str_escape
-
- if not inside_a_string:
- if c == '"':
- inside_collection_stack.insert(0, '"')
- elif c == "'":
- inside_collection_stack.insert(0, "'")
- elif c == "{":
- inside_collection_stack.insert(0, "}")
- elif c == "[":
- inside_collection_stack.insert(0, "]")
- elif c == "(":
- inside_collection_stack.insert(0, ")")
-
- return processing_str_escape

diff:
+from dataclasses import dataclass
+from enum import Enum, IntEnum
+from typing import List, Optional
+
+
+@dataclass
+class VarBlockMatch:
+ full_str: str # Example: ${local.foo}
+ var_only: str # Example: local.fop
+
+ def replace(self, original: str, replaced: str):
+ self.full_str = self.full_str.replace(original, replaced)
+ self.var_only = self.var_only.replace(original, replaced)
+
+
+class ParserMode(Enum):
+ # Note: values are just for debugging.
+ EVAL = "${"
+ MAP = "{"
+ STRING_SINGLE_QUOTE = "'"
+ STRING_DOUBLE_QUOTE = '"'
+ PARAMS = "("
+ ARRAY = "["
+
+ @staticmethod
+ def is_string(mode: str) -> bool:
+ return mode == ParserMode.STRING_SINGLE_QUOTE or mode == ParserMode.STRING_DOUBLE_QUOTE
+
+ def __repr__(self):
+ return str(self.value)
+
+ def __str__(self):
+ return str(self.value)
+
+
+def find_var_blocks(value: str) -> List[VarBlockMatch]:
+ """
+ Find and return all the var blocks within a given string. Order is important and may contain portions of
+ one another.
+ """
+
+ to_return: List[VarBlockMatch] = []
+
+ mode_stack: List[ParserMode] = []
+ eval_start_pos_stack: List[int] = [] # location of first char inside brackets
+ preceding_dollar = False
+ preceding_string_escape = False
+ for index, c in enumerate(value):
+ current_mode = " " if not mode_stack else mode_stack[-1]
+
+ # Print statement of power...
+ # print(f"{str(index).ljust(3, ' ')} {c} {'$' if preceding_dollar else ' '} "
+ # f"{'`' if preceding_string_escape else ' '} "
+ # f"{current_mode.ljust(2)} - {mode_stack}")
+
+ if c == "$":
+ if preceding_dollar: # ignore double $
+ preceding_dollar = False
+ continue
+
+ preceding_dollar = True
+ continue
+
+ if c == "{" and preceding_dollar:
+ mode_stack.append(ParserMode.EVAL)
+ eval_start_pos_stack.append(index + 1) # next char
+ preceding_dollar = False
+ continue
+ elif c == "\\" and ParserMode.is_string(current_mode):
+ preceding_string_escape = True
+ continue
+
+ preceding_dollar = False
+
+ if c == "}":
+ if current_mode == ParserMode.EVAL:
+ mode_stack.pop()
+ start_pos = eval_start_pos_stack.pop()
+ eval_string = value[start_pos: index]
+ to_return.append(VarBlockMatch("${" + eval_string + "}", eval_string))
+ elif current_mode == ParserMode.MAP:
+ mode_stack.pop()
+ elif c == "]" and current_mode == ParserMode.ARRAY:
+ mode_stack.pop()
+ elif c == ")" and current_mode == ParserMode.PARAMS:
+ mode_stack.pop()
+ elif c == '"':
+ if preceding_string_escape:
+ preceding_string_escape = False
+ continue
+ elif current_mode == ParserMode.STRING_DOUBLE_QUOTE:
+ mode_stack.pop()
+ else:
+ mode_stack.append(ParserMode.STRING_DOUBLE_QUOTE)
+ elif c == "'":
+ if preceding_string_escape:
+ preceding_string_escape = False
+ continue
+ elif current_mode == ParserMode.STRING_SINGLE_QUOTE:
+ mode_stack.pop()
+ else:
+ mode_stack.append(ParserMode.STRING_SINGLE_QUOTE)
+ elif c == "{":
+ # NOTE: Can't be preceded by a dollar sign (that was checked earlier)
+ if not ParserMode.is_string(current_mode):
+ mode_stack.append(ParserMode.MAP)
+ elif c == "[": # do we care?
+ if not ParserMode.is_string(current_mode):
+ mode_stack.append(ParserMode.ARRAY)
+ elif c == "(": # do we care?
+ if not ParserMode.is_string(current_mode):
+ mode_stack.append(ParserMode.PARAMS)
+
+ preceding_string_escape = False
+
+ return to_return
+
+
+def split_merge_args(value: str) -> Optional[List[str]]:
+ """
+ Split arguments of a merge function. For example, "merge(local.one, local.two)" would
+ call this function with a value of "local.one, local.two" which would return
+ ["local.one", "local.two"]. If the value cannot be unpacked, None will be returned.
+ """
+ if not value:
+ return None
+
+ # There are a number of splitting scenarios depending on whether variables or
+ # direct maps are used:
+ # merge({tag1="foo"},{tag2="bar"})
+ # merge({tag1="foo"},local.some_tags)
+ # merge(local.some_tags,{tag2="bar"})
+ # merge(local.some_tags,local.some_other_tags)
+ # Also, the number of arguments can vary, things can be nested, strings are evil...
+ # See tests/terraform/test_parser_var_blocks.py for many examples.
+
+ to_return = []
+ current_arg_buffer = ""
+ processing_str_escape = False
+ inside_collection_stack = [] # newest at position 0, contains the terminator for the collection
+ for c in value:
+ if c == "," and not inside_collection_stack:
+ current_arg_buffer = current_arg_buffer.strip()
+ # Note: can get a zero-length buffer when there's a double comman. This can
+ # happen with multi-line args (see parser_internals test)
+ if len(current_arg_buffer) != 0:
+ to_return.append(current_arg_buffer)
+ current_arg_buffer = ""
+ else:
+ current_arg_buffer += c
+
+ processing_str_escape = _str_parser_loop_collection_helper(c,
+ inside_collection_stack,
+ processing_str_escape)
+
+ current_arg_buffer = current_arg_buffer.strip()
+ if len(current_arg_buffer) > 0:
+ to_return.append(current_arg_buffer)
+
+ if len(to_return) == 0:
+ return None
+ return to_return
+
+
+
+def _str_parser_loop_collection_helper(c: str, inside_collection_stack: List[str],
+ processing_str_escape: bool) -> bool:
+ """
+ This function handles dealing with tracking when a char-by-char state loop is inside a
+ "collection" (map, array index, method args, string).
+
+ :param c: Active character
+ :param inside_collection_stack: Stack of terminators for collections. This will be modified by
+ this function. The active terminator will be at position 0.
+
+
+ :return: value to set for `processing_str_escape`
+ """
+ inside_a_string = False
+ if inside_collection_stack:
+ terminator = inside_collection_stack[0]
+
+ if terminator == '"' or terminator == "'":
+ if processing_str_escape:
+ processing_str_escape = False
+ return processing_str_escape
+ elif c == "\\":
+ processing_str_escape = True
+ return processing_str_escape
+ else:
+ inside_a_string = True
+
+ if c == terminator:
+ del inside_collection_stack[0]
+ return processing_str_escape
+
+ if not inside_a_string:
+ if c == '"':
+ inside_collection_stack.insert(0, '"')
+ elif c == "'":
+ inside_collection_stack.insert(0, "'")
+ elif c == "{":
+ inside_collection_stack.insert(0, "}")
+ elif c == "[":
+ inside_collection_stack.insert(0, "]")
+ elif c == "(":
+ inside_collection_stack.insert(0, ")")
+
+ return processing_str_escape

diff:
import unittest
-from typing import List, Optional, Tuple
-
from checkov.terraform import parser
-from checkov.terraform.parser import VarBlockMatch as VBM
class TestParserInternals(unittest.TestCase):
def test_eval_string_to_list(self):
expected = ["a", "b", "c"]
assert parser._eval_string('["a", "b", "c"]') == expected
-
- def test_split_merge_args(self):
- cases: List[Tuple[str, List[str]]] = [
- ("local.one, local.two",
- ["local.one", "local.two"]),
- ("{Tag4 = \"four\"}, {Tag5 = \"five\"}",
- ["{Tag4 = \"four\"}", "{Tag5 = \"five\"}"]),
- ("{a=\"b\"}, {a=[1,2], c=\"z\"}, {d=3}",
- ["{a=\"b\"}", "{a=[1,2], c=\"z\"}", "{d=3}"]),
- ("local.common_tags, merge({Tag4 = \"four\"}, {Tag5 = \"five\"})",
- ["local.common_tags", "merge({Tag4 = \"four\"}, {Tag5 = \"five\"})"]),
- (", ",
- None),
- ("",
- None),
- (", leading_comma",
- ["leading_comma"]),
- ("kinda_maybe_shouldnt_work_but_we_will_roll_with_it, ", # <-- trailing comma
- ["kinda_maybe_shouldnt_work_but_we_will_roll_with_it"]),
- ("local.one",
- ["local.one"]),
- ('{"a": "}, evil"}', # bracket inside string, should not be split
- ['{"a": "}, evil"}']),
- ("{'a': '}, evil'}", # bracket inside string, should not be split
- ["{'a': '}, evil'}"]), # Note: these happen with native maps (see merge tests)
- ('${merge({\'a\': \'}, evil\'})}',
- ['${merge({\'a\': \'}, evil\'})}']),
- ('local.common_tags,,{\'Tag4\': \'four\'},,{\'Tag2\': \'Dev\'},',
- ["local.common_tags", "{\'Tag4\': \'four\'}", "{\'Tag2\': \'Dev\'}"])
- ]
- for case in cases:
- actual = parser._split_merge_args(case[0])
- assert actual == case[1], f"Case \"{case[0]}\" failed. Expected: {case[1]} Actual: {actual}"
-
- def test_find_var_blocks(self):
- cases: List[Tuple[str, List[parser.VarBlockMatch]]] = [
- ("${local.one}",
- [VBM("${local.one}", "local.one")]),
- ("${merge({a=\"b\"}, {a=[1,2], c=\"z\"}, {d=3})}",
- [VBM("${merge({a=\"b\"}, {a=[1,2], c=\"z\"}, {d=3})}",
- "merge({a=\"b\"}, {a=[1,2], c=\"z\"}, {d=3})")]),
- ("\"string$ ${tomap({key=\"value\"})[key]} are fun\"",
- [VBM("${tomap({key=\"value\"})[key]}", "tomap({key=\"value\"})[key]")]),
- # This case highlights that inner evals should be returned
- ("${filemd5(\"${path.module}/templates/some-file.json\")}",
- [VBM("${path.module}", "path.module")]),
- ("${local.NAME[foo]}-${local.TAIL}${var.gratuitous_var_default}",
- [
- VBM("${local.NAME[foo]}", "local.NAME[foo]"),
- VBM("${local.TAIL}", "local.TAIL"),
- VBM("${var.gratuitous_var_default}", "var.gratuitous_var_default")
- ]),
- ("${tostring(\"annoying {\")}",
- [VBM("${tostring(\"annoying {\")}", "tostring(\"annoying {\")")]),
- ("${this-is-unterminated", []),
- ("${merge({\"a\": \"}, evil\"},{\"b\": \"\\\" , evil\"})}",
- [VBM("${merge({\"a\": \"}, evil\"},{\"b\": \"\\\" , evil\"})}",
- "merge({\"a\": \"}, evil\"},{\"b\": \"\\\" , evil\"})")]),
- ("$${foo}", []), # escape interpolation
- ('${merge({\'a\': \'}, evil\'})}',
- [VBM('${merge({\'a\': \'}, evil\'})}', 'merge({\'a\': \'}, evil\'})')]),
-
- # Ordered returning of sub-vars and then outer var.
- ("${merge(local.common_tags,local.common_data_tags,{'Name': 'my-thing-${var.ENVIRONMENT}-${var.REGION}'})}",
- [
- VBM("${var.ENVIRONMENT}", "var.ENVIRONMENT"),
- VBM("${var.REGION}", "var.REGION"),
- VBM("${merge(local.common_tags,local.common_data_tags,{'Name': 'my-thing-${var.ENVIRONMENT}-${var.REGION}'})}",
- "merge(local.common_tags,local.common_data_tags,{'Name': 'my-thing-${var.ENVIRONMENT}-${var.REGION}'})")
- ])
- ]
- for case in cases:
- actual = parser._find_var_blocks(case[0])
- assert actual == case[1], f"Case \"{case[0]}\" failed. Expected: {case[1]} Actual: {actual}"

original_message:
New and more powerful (and correct) find_var_blocks

Broke `find_var_blocks` and supporting code into its own script and reworked it to be able to handle multiple nested var blocks. This allows inner eval blocks which are not resolved (see `merge_function_unresolved_var` parser scenario).

Fixes: #816


message:
Functioning --quiet for -o json.
* Functioning --quiet for -o json. Needs tests,
looking at for callable unit test and passing in cli params
* added integration test for -o json --quiet

diff:
@@ -46,6 +46,7 @@ jobs:
pipenv run checkov -s -d cfngoat/ -o json > checkov_report_cfngoat.json
pipenv run checkov -s -d kubernetes-goat/ -o json > checkov_report_kubernetes-goat.json
pipenv run checkov -s --skip-check CKV_AWS_33,CKV_AWS_41 -d terragoat/terraform/ -o json > checkov_report_terragoat_with_skip.json
+ pipenv run checkov -s -d cfngoat/ -o json --quiet > checkov_report_cfngoat_quiet.json
- name: Run integration tests
run: |

diff:
@@ -78,6 +78,7 @@ jobs:
pipenv run checkov -s -d cfngoat/ -o json > checkov_report_cfngoat.json
pipenv run checkov -s -d kubernetes-goat/ -o json > checkov_report_kubernetes-goat.json
pipenv run checkov -s --skip-check CKV_AWS_33,CKV_AWS_41 -d terragoat/terraform/ -o json > checkov_report_terragoat_with_skip.json
+ pipenv run checkov -s -d cfngoat/ -o json --quiet > checkov_report_cfngoat_quiet.json
- name: Run integration tests
run: |

diff:
@@ -49,7 +49,16 @@ class Report:
def get_json(self):
return json.dumps(self.get_dict(), indent=4)
- def get_dict(self):
+ def get_dict(self, is_quiet=False):
+ if is_quiet:
+ return {
+ "check_type": self.check_type,
+ "results": {
+ "failed_checks": [check.__dict__ for check in self.failed_checks]
+ },
+ "summary": self.get_summary()
+ }
+ else:
return {
"check_type": self.check_type,
"results": {

diff:
@@ -48,7 +48,7 @@ class RunnerRegistry(object):
for report in scan_reports:
if not report.is_empty():
if args.output == "json":
- report_jsons.append(report.get_dict())
+ report_jsons.append(report.get_dict(is_quiet=args.quiet))
elif args.output == "junitxml":
junit_reports.append(report)
# report.print_junit_xml()

diff:
@@ -33,6 +33,13 @@ class TestCheckovJsonReport(unittest.TestCase):
self.assertEqual(data["summary"]["parsing_errors"], 0, "expecting 0 parsing errors")
self.assertGreater(data["summary"]["failed"], 1, "expecting more then 1 failed checks")
+ def validate_json_quiet(self):
+ report_path = current_dir + "/../checkov_report_cfngoat_quiet.json"
+ with open(report_path) as json_file:
+ data = json.load(json_file)
+ self.assertTrue(data["results"]["failed_checks"])
+ self.assertFalse(data["results"]["passed_checks"])
+ self.assertTrue(data["summary"])
if __name__ == '__main__':
unittest.main()

original_message:
Functioning --quiet for -o json. (#854)

* Functioning --quiet for -o json. Needs tests,
looking at #774 for callable unit test and passing in cli params

* added integration test for -o json --quiet


message:
Handle malformed input case
Fixes: 857

diff:
@@ -94,6 +94,10 @@ class BaseCheckRegistry(object):
(entity_type, entity_name, entity_configuration) = self.extract_entity_details(entity)
results = {}
+
+ if not isinstance(entity_configuration, dict):
+ return results
+
checks = self.get_checks(entity_type)
for check in checks:
skip_info = {}

diff:
@@ -10,6 +10,9 @@ provider_tag_mapping = {
def get_resource_tags(resource_type, entity_config):
+ if not isinstance(entity_config, dict):
+ return None
+
if '_' not in resource_type:
return None # probably not a resource block
provider = resource_type[:resource_type.index('_')]

diff:
+resource "aws_instance" {
+ ami = "amiid"
+ instance_type = "t3.micro"
+}

diff:
@@ -495,6 +495,15 @@ class TestRunnerValid(unittest.TestCase):
# no need to join with a '/' because the TF runner adds it to the start of the file path
self.assertEqual(record.repo_file_path, f'/{file_rel_path}')
+ def test_runner_malformed_857(self):
+ current_dir = os.path.dirname(os.path.realpath(__file__))
+
+ passing_tf_file_path = current_dir + "/resources/malformed_857/main.tf"
+
+ runner = Runner()
+ runner.run(root_folder=None, external_checks_dir=None, files=[passing_tf_file_path])
+ # If we get here all is well. :-) Failure would throw an exception.
+
def tearDown(self):
parser_registry.definitions_context = {}

original_message:
Handle malformed input case (#858)

Fixes: 857


old_path: .gitignore

new_path: .gitignore

diff:
@@ -9,7 +9,6 @@ __pycache__/
# Terraform
*.tfstate*
*.terraform*
-*.tfvars
*.tfbackend
# git

diff:
@@ -211,7 +211,7 @@ class Parser:
if hcl_tfvars: # terraform.tfvars
data = _load_or_die_quietly(hcl_tfvars, self.out_parsing_errors)
if data:
- var_value_and_file_map.update({k: (v, hcl_tfvars.path) for k, v in data.items()})
+ var_value_and_file_map.update({k: (v[0], hcl_tfvars.path) for k, v in data.items()})
if json_tfvars: # terraform.tfvars.json
data = _load_or_die_quietly(json_tfvars, self.out_parsing_errors)
if data:
@@ -730,7 +730,8 @@ Load JSON or HCL, depending on filename.
def _clean_bad_definitions(tf_definition_list):
return {
block_type: list(filter(lambda definition_list: block_type == 'locals' or
- len(definition_list.keys()) == 1, tf_definition_list[block_type]))
+ not isinstance(definition_list, dict)
+ or len(definition_list.keys()) == 1, tf_definition_list[block_type]))
for block_type in tf_definition_list.keys()
}

diff:
+VERSIONING = true
\ No newline at end of file

diff:
+variable "foo" {}
+
+resource "aws_s3_bucket" "my_bucket" {
+ bucket = var.foo
+}
\ No newline at end of file

diff:
@@ -7,6 +7,17 @@ import hcl2
# This group of tests is used to confirm assumptions about how the hcl2 library parses into json.
# We want to make sure important assumptions are caught if behavior changes.
class TestHCL2LoadAssumptions(unittest.TestCase):
+ def test_tfvars(self):
+ tf = '''
+ VERSIONING = true
+ CHECKOV = "awesome"
+ '''
+ expect = {
+ "VERSIONING": [True],
+ "CHECKOV": ["awesome"]
+ }
+ self.go(tf, expect)
+
def test_multiline_function(self):
tf = '''
locals {

diff:
@@ -105,6 +105,9 @@ class TestParserScenarios(unittest.TestCase):
def test_json_807(self):
self.go("json_807")
+ def test_tfvars(self):
+ self.go("tfvars")
+
@staticmethod
def go(dir_name):
dir_path = os.path.join(os.path.dirname(os.path.realpath(__file__)),

original_message:
Fix problems with .tfvars value referencing

Fixes: #865


diff:
@@ -209,9 +209,10 @@ class Parser:
continue
var_value_and_file_map[key[7:]] = value, f"env:{key}"
if hcl_tfvars: # terraform.tfvars
- data = _load_or_die_quietly(hcl_tfvars, self.out_parsing_errors)
+ data = _load_or_die_quietly(hcl_tfvars, self.out_parsing_errors,
+ clean_definitions=False)
if data:
- var_value_and_file_map.update({k: (v[0], hcl_tfvars.path) for k, v in data.items()})
+ var_value_and_file_map.update({k: (_safe_index(v, 0), hcl_tfvars.path) for k, v in data.items()})
if json_tfvars: # terraform.tfvars.json
data = _load_or_die_quietly(json_tfvars, self.out_parsing_errors)
if data:
@@ -706,7 +707,8 @@ def _handle_indexing(reference: str, data_source: Callable[[str], Optional[Any]]
return data_source(reference)
-def _load_or_die_quietly(file: os.PathLike, parsing_errors: Dict) -> Optional[Mapping]:
+def _load_or_die_quietly(file: os.PathLike, parsing_errors: Dict,
+ clean_definitions: bool = True) -> Optional[Mapping]:
"""
Load JSON or HCL, depending on filename.
:return: None if the file can't be loaded
@@ -720,7 +722,11 @@ Load JSON or HCL, depending on filename.
if file_name.endswith(".json"):
return json.load(f)
else:
- return _clean_bad_definitions(hcl2.load(f))
+ raw_data = hcl2.load(f)
+ if clean_definitions:
+ return _clean_bad_definitions(raw_data)
+ else:
+ return raw_data
except Exception as e:
LOGGER.debug(f'failed while parsing file {file}', exc_info=e)
parsing_errors[file_path] = e
@@ -777,3 +783,12 @@ def _remove_module_dependency_in_path(path):
if re.findall(resolved_module_pattern, path):
path = re.sub(resolved_module_pattern, '', path)
return path
+
+
+def _safe_index(sequence_hopefully, index) -> Optional[Any]:
+ try:
+ return sequence_hopefully[index]
+ except IndexError as e:
+ logging.debug(f'Failed to parse index int ({index}) out of {sequence_hopefully}')
+ logging.debug(e, stack_info=True)
+ return None

diff:
variable "foo" {}
+variable "list_data" {}
+variable "map_data" {}
resource "aws_s3_bucket" "my_bucket" {
+ // TODO: List/map isn't resolving correctly
+// bucket = "${var.foo}-${var.list_data[0]}-${var.map_data[stage]}"
bucket = var.foo
}
\ No newline at end of file

original_message:
Avoid attempting to clean data from tfvars

Fixes: #868


diff:
@@ -349,7 +349,7 @@ class Parser:
eval_map_by_var_name,
new_context, value, root_directory)
if replaced != var_base:
- if match.full_str == value:
+ if match.full_str == altered_value:
altered_value = replaced
else:
altered_value = altered_value.replace(match.full_str, str(replaced))

diff:
"Tag4": "four"
}
],
+ "local_local_manual": [
+ {
+ "Tag1": "one",
+ "Tag2": "two",
+ "Tag3": "three",
+ "Name": "Bob"
+ }
+ ],
"manual_to_local": [
{
"Tag4": "four",
"Tag2": "multiline_tag2",
"Tag4": "four"
}
+ ],
+ "static1": ["one"],
+ "static2": ["two"]
+ }
+ ],
+ "resource": [
+ {
+ "aws_something": {
+ "something": {
+ "tags": [
+ {
+ "Tag1": "one",
+ "Tag2": "two",
+ "Tag3": "three",
+ "Name": "Bob-one-two"
+ }
]
}
+ }
+ }
]
}
}
\ No newline at end of file

diff:
@@ -9,6 +9,7 @@ locals {
local_to_local = merge(local.common_tags, local.common_data_tags)
local_to_manual = merge(local.common_tags, {Tag4 = "four"})
+ local_local_manual = merge(local.common_tags, local.common_data_tags, {Name = "Bob"})
manual_to_local = merge({Tag4 = "four"}, local.common_tags)
manual_to_manual = merge({Tag4 = "four"}, {Tag5="five"})
@@ -36,4 +37,12 @@ locals {
{Tag4 = "four"},
{Tag2="multiline_tag2"}
)
+
+ static1 = "one"
+ static2 = "two"
+}
+
+resource "aws_something" "something" {
+ #
+ tags = merge(local.common_tags, local.common_data_tags, {Name = "Bob-${local.static1}-${local.static2}"})
}
\ No newline at end of file

original_message:
Prevent string conversion of dict containing string with var substitution

Fixes: #873


message:
Additional fixes to resolving indexed values, follow-up to
Relates to:

diff:
@@ -573,7 +573,9 @@ def _handle_single_var_pattern(orig_variable: str, var_value_and_file_map: Dict[
elif orig_variable.startswith("var."):
var_name = orig_variable[4:]
- var_value_and_file = _handle_indexing(var_name, lambda r: var_value_and_file_map.get(r))
+ var_value_and_file = _handle_indexing(var_name,
+ lambda r: var_value_and_file_map.get(r),
+ value_is_a_tuple=True)
if var_value_and_file is not None:
var_value, var_file = var_value_and_file
eval_context = eval_map_by_var_name.get(var_name)
@@ -688,21 +690,48 @@ def _handle_single_var_pattern(orig_variable: str, var_value_and_file_map: Dict[
return orig_variable # fall back to no change
-def _handle_indexing(reference: str, data_source: Callable[[str], Optional[Any]]) -> Optional[Any]:
+def _handle_indexing(reference: str,
+ data_source: Callable[[str], Optional[Any]],
+ value_is_a_tuple: bool = False) -> Optional[Any]:
+ """
+
+ :param reference: Full reference with the variable (ex: "my_list[0]")
+ :param data_source: Data source for retrieving the variable value. Provided the variable name
+ (ex: "my_list"), the value should be returned, if available.
+ :param value_is_a_tuple: Indicates whether the returned value will be a tuple with the value as
+ item 0 and the source location as item 1. This is returned for some data
+ sources and not for others. When true, the returned value will also be a
+ Tuple, if non-None.
+ :return:
+ """
if reference.endswith("]") and "[" in reference:
base_ref = reference[:reference.rindex("[")]
- value = data_source(base_ref)
reference_val = reference[reference.rindex("[") + 1: -1]
+
+ value = data_source(base_ref)
+ if value is None:
+ return None
+
+ if value_is_a_tuple:
+ value_tuple = value
+ value = value_tuple[0]
+
if isinstance(value, dict):
+ if value_is_a_tuple:
+ return value.get(reference_val), value_tuple[1]
+ else:
return value.get(reference_val)
elif isinstance(value, list):
try:
+ if value_is_a_tuple:
+ return value[int(reference_val)], value_tuple[1]
+ else:
return value[int(reference_val)]
except ValueError as e:
# TODO: handle count.index correctly
logging.debug(f'Failed to parse index int out of {reference_val}')
logging.debug(e, stack_info=True)
- return
+ return None
else:
return data_source(reference)

diff:
@@ -3,7 +3,5 @@ variable "list_data" {}
variable "map_data" {}
resource "aws_s3_bucket" "my_bucket" {
- // TODO: List/map isn't resolving correctly
-// bucket = "${var.foo}-${var.list_data[0]}-${var.map_data[stage]}"
- bucket = var.foo
+ bucket = "${var.foo}-${var.list_data[0]}-${var.map_data[stage]}"
}
\ No newline at end of file

original_message:
Additional fixes to resolving indexed values, follow-up to #869 (#871)

Relates to: #868, #869


message:
TF parser support for ternary conditions
* Work drop
* Support ternaries with latest parser work
* Logic correction in "false" case
Also disables some tests for some currently unhandled cases.
* Multi-line test case (HCL2 parser doesn't like)
* Merge fix

diff:
@@ -535,6 +535,11 @@ def _handle_single_var_pattern(orig_variable: str, var_value_and_file_map: Dict[
module_data_retrieval: Callable[[str], Dict[str, Any]],
eval_map_by_var_name: Dict[str, EvaluationContext],
context, orig_variable_full, root_directory: str) -> Any:
+
+ ternary_info = _is_ternary(orig_variable)
+ if ternary_info:
+ return _process_ternary(orig_variable, ternary_info[0], ternary_info[1])
+
if orig_variable.startswith("module."):
if not module_list:
return orig_variable
@@ -814,6 +819,36 @@ def _remove_module_dependency_in_path(path):
return path
+def _is_ternary(value: str) -> Optional[Tuple[int,int]]:
+ """
+ Determines whether or not the given string is *probably* a ternary operation
+ :return: If the expression does represent a possibly-processable ternary expression, a tuple
+ containing the index of the question mark and colon will be returned.
+ """
+ if not value:
+ return None
+ question_index = value.find("?")
+ if question_index < 1 or value.count("?") > 1:
+ return None
+ colon_index = value.find(":")
+ if colon_index < question_index or value.count(":") > 1:
+ return None
+ return question_index, colon_index
+
+
+def _process_ternary(value: str, question_index: int, colon_index: int) -> str:
+ condition = value[:question_index].strip()
+
+ # Fast & easy case is simple boolean
+ condition_lower = condition.lower()
+ if condition_lower == "true":
+ return _to_native_value(value[question_index + 1: colon_index].strip())
+ elif condition_lower == "false":
+ return _to_native_value(value[colon_index + 1:].strip())
+
+ # Otherwise, this isn't evaluated enough
+ return value
+
def _safe_index(sequence_hopefully, index) -> Optional[Any]:
try:
return sequence_hopefully[index]

diff:
@@ -110,6 +110,20 @@ def find_var_blocks(value: str) -> List[VarBlockMatch]:
elif c == "(": # do we care?
if not ParserMode.is_string(current_mode):
mode_stack.append(ParserMode.PARAMS)
+ elif c == "?" and current_mode == ParserMode.EVAL: # ternary
+ # If what's been processed in the ternary so far is "true" or "false" (boolean or string type)
+ # then nothing special will happen here and only the full expression will be returned.
+ # Anything else will be treated as an unresolved variable block.
+ start_pos = eval_start_pos_stack[-1] # DO NOT pop: there's no separate eval start indicator
+ eval_string = value[start_pos: index].strip()
+
+ # HACK ALERT: For the cases with the trailing quotes, see:
+ # test_hcl2_load_assumptions.py -> test_weird_ternary_string_clipping
+ if eval_string not in {"true", "false", '"true"', '"false"', 'true"', 'false"'}:
+ # REMINDER: The eval string is not wrapped in a eval markers since they didn't really
+ # appear in the original value. If they're put in, substitution doesn't
+ # work properly.
+ to_return.append(VarBlockMatch(eval_string, eval_string))
preceding_string_escape = False

diff:
+{
+ "main.tf": {
+ "locals": [
+ {
+ "a": ["a"],
+ "b": ["b"],
+ "empty": [""],
+ "bool_true": ["correct"],
+ "bool_false": ["correct"],
+ "type": ["bool"],
+ "default": [true]
+ }
+ ]
+ }
+}
\ No newline at end of file

diff:
+locals {
+ a = "a"
+ b = "b"
+ empty = ""
+
+ bool_true = true ? "correct" : "wrong"
+ bool_false = false ? "wrong" : "correct"
+
+// local_true = true
+ // TODO: HCL2 parser doesn't like the following line
+// multiline = (local.local_true) ?
+// "correct" : "wrong"
+
+ // TODO: See test_hcl2_load_assumptions.py -> test_weird_ternary_string_clipping
+ // Doesn't currently pull the ternary correctly since it's evaluated inside the string.
+// bool_string_true = "true" ? "correct" : "wrong"
+// bool_string_false = "false" ? "wrong" : "correct"
+
+ // TODO: Comparison cases...
+// compare_string_true = "a" == "a" ? "correct" : "wrong"
+// compare_string_false = "a" != "a" ? "wrong" : "correct"
+//
+// compare_num_true = 1 == 1 ? "correct" : "wrong"
+// compare_num_false = 1 != 1 ? "correct" : "wrong"
+//
+// # NOTE: I don't think evals in locals is valid in TF, but the parser will eval it
+// default_not_taken = local.a != "" ? local.a : "default value"
+// default_taken = local.empty != "" ? local.a : "default value"
+
+ type = bool
+ default = true
+}
\ No newline at end of file

diff:
+{
+ "main.tf": {
+ "variable": [
+ {
+ "metadata_http_tokens_required": {
+ "type": ["bool"],
+ "default": [true],
+ "description": ["Whether or not the metadata service requires session tokens"]
+ }
+ }
+ ],
+ "resource": [
+ {
+ "aws_instance": {
+ "foo": {
+ "ami": ["ami-005e54dee72cc1d00"],
+ "instance_type": ["t2.micro"],
+ "root_block_device": [
+ {
+ "encrypted": [true]
+ }
+ ],
+ "metadata_options": [
+ {
+ "http_tokens": ["required"]
+ }
+ ]
+ }
+ }
+ }
+ ]
+ }
+}
\ No newline at end of file

diff:
+variable "metadata_http_tokens_required" {
+ type = bool
+ default = true
+ description = "Whether or not the metadata service requires session tokens"
+}
+
+resource "aws_instance" "foo" {
+ ami = "ami-005e54dee72cc1d00" # us-west-2
+ instance_type = "t2.micro"
+
+ root_block_device {
+ encrypted = true
+ }
+
+ metadata_options {
+ http_tokens = (var.metadata_http_tokens_required) ? "required" : "optional"
+ }
+}
\ No newline at end of file

diff:
@@ -7,6 +7,27 @@ import hcl2
# This group of tests is used to confirm assumptions about how the hcl2 library parses into json.
# We want to make sure important assumptions are caught if behavior changes.
class TestHCL2LoadAssumptions(unittest.TestCase):
+ def test_ternary(self):
+ # Ternary and removal of parens are interesting things here
+ tf = '''
+ resource "aws_instance" "foo" {
+ metadata_options {
+ http_tokens = (var.metadata_http_tokens_required) ? "required" : "optional"
+ }
+ }'''
+ expect = {
+ "resource": [{
+ "aws_instance": {
+ "foo": {
+ "metadata_options": [{
+ "http_tokens": ['${var.metadata_http_tokens_required ? "required" : "optional"}']
+ }]
+ }
+ }
+ }]
+ }
+ self.go(tf, expect)
+
def test_tfvars(self):
tf = '''
VERSIONING = true
@@ -255,3 +276,20 @@ class TestHCL2LoadAssumptions(unittest.TestCase):
f"{json.dumps(expected_result, indent=2)}\n" \
f"** ACTUAL **\n" \
f"{json.dumps(actual_result, indent=2)}"
+
+ def test_math(self):
+ tf = "four = 2 + 2"
+ expect = {
+ "four": ["${2 + 2}"]
+ }
+ self.go(tf, expect)
+
+ def test_weird_ternary_string_clipping(self):
+ tf = 'bool_string_false = "false" ? "wrong" : "correct"'
+ expect = {
+ "bool_string_false": ["false\" ? \"wrong\" : \"correct"]
+ # -- --
+ # | |
+ # missing quotes on outer tokens :-(
+ }
+ self.go(tf, expect)

diff:
@@ -105,6 +105,12 @@ class TestParserScenarios(unittest.TestCase):
def test_json_807(self):
self.go("json_807")
+ def test_ternaries(self):
+ self.go("ternaries")
+
+ def test_ternary_793(self):
+ self.go("ternary_793")
+
def test_tfvars(self):
self.go("tfvars")

diff:
@@ -121,7 +121,46 @@ class TestParserInternals(unittest.TestCase):
VBM("${merge(local.common_tags,local.common_data_tags,{'Name': 'my-thing-${var.ENVIRONMENT}-${var.REGION}'})}",
"merge(local.common_tags,local.common_data_tags,{'Name': 'my-thing-${var.ENVIRONMENT}-${var.REGION}'})")
]
- )
+ ),
+
+ # Ternaries
+ (
+ '${var.metadata_http_tokens_required ? "required" : "optional"}',
+ [
+ VBM('var.metadata_http_tokens_required', 'var.metadata_http_tokens_required'),
+ VBM('${var.metadata_http_tokens_required ? "required" : "optional"}',
+ 'var.metadata_http_tokens_required ? "required" : "optional"')
+ ]
+ ),
+ (
+ '${1 + 1 == 2 ? "required" : "optional"}',
+ [
+ VBM('1 + 1 == 2', '1 + 1 == 2'),
+ VBM('${1 + 1 == 2 ? "required" : "optional"}', '1 + 1 == 2 ? "required" : "optional"')
+ ]
+ ),
+ (
+ '${true ? "required" : "optional"}',
+ [
+ VBM('${true ? "required" : "optional"}', 'true ? "required" : "optional"')
+ ]
+ ),
+ (
+ '${false ? "required" : "optional"}',
+ [
+ VBM('${false ? "required" : "optional"}', 'false ? "required" : "optional"')
+ ]
+ ),
+ # TODO: var -> comparison -> ternary
+ # (
+ # '${local.empty != "" ? local.a : "default value"}',
+ # [
+ # VBM("local.empty", "local.empty"),
+ # VBM('local.empty != ""', 'local.empty != ""'),
+ # VBM('${local.empty != "" ? local.a : "default value"}',
+ # 'local.empty != "" ? local.a : "default value"')
+ # ]
+ # )
]
for case in cases:
actual = find_var_blocks(case[0])

original_message:
TF parser support for ternary conditions (#855)

* Work drop

* Support ternaries with latest parser work

* Logic correction in "false" case

Also disables some tests for some currently unhandled cases.

* Multi-line test case (HCL2 parser doesn't like)

* Merge fix

Co-authored-by: Nimrod Kor <nimrodkor@gmail.com>


message: Ensure predictable ordering for the __resolved__ field so tests are reliable

diff:
@@ -541,6 +541,7 @@ class Parser:
del self.out_definitions[key]
if new_key not in resolved_loc_list:
resolved_loc_list.append(new_key)
+ resolved_loc_list.sort() # For testing, need predictable ordering
deep_merge.merge(all_module_definitions, module_definitions)
except Exception as e:

diff:
"ENGINE_VERSION": ["11"],
"ENCRYPTED": [true],
"__resolved__": [
- "modules/db/variables.tf[envs/myaccount/us-east-1/main.tf#0]",
- "modules/db/db.tf[envs/myaccount/us-east-1/main.tf#0]"
+ "modules/db/db.tf[envs/myaccount/us-east-1/main.tf#0]",
+ "modules/db/variables.tf[envs/myaccount/us-east-1/main.tf#0]"
]
}
}
"ENGINE_VERSION": ["11"],
"ENCRYPTED": [true],
"__resolved__": [
- "modules/db/variables.tf[envs/myotheraccount/us-east-1/main.tf#0]",
- "modules/db/db.tf[envs/myotheraccount/us-east-1/main.tf#0]"
+ "modules/db/db.tf[envs/myotheraccount/us-east-1/main.tf#0]",
+ "modules/db/variables.tf[envs/myotheraccount/us-east-1/main.tf#0]"
]
}
}


message: Remove code incorrectly inserted on merge

diff:
@@ -500,8 +500,6 @@ class Parser:
specified_vars = {k: v[0] for k, v in module_call_data.items()
if k != "source" and k != "version"}
- if not dir_filter(os.path.abspath(content.path())):
- continue
self._internal_dir_load(directory=content.path(),
module_loader_registry=module_loader_registry,
dir_filter=dir_filter, specified_vars=specified_vars,


message: Update build.yml CI to match changes in pr-check CI

diff:
@@ -13,6 +13,7 @@ jobs:
with:
python-version: 3.7
- uses: dschep/install-pipenv-action@v1
+ - uses: azure/setup-helm@v1
- name: Build & install checkov package
run: |
pipenv run pip install pytest


message: update build.yml to reflect changes to integration tests in test-pr

diff:
@@ -45,7 +45,8 @@ jobs:
run: |
pipenv run checkov -s -d terragoat/terraform/ -o json > checkov_report_terragoat.json
pipenv run checkov -s -d cfngoat/ -o json > checkov_report_cfngoat.json
- pipenv run checkov -s -d kubernetes-goat/ -o json > checkov_report_kubernetes-goat.json
+ pipenv run checkov -s -d kubernetes-goat/ --framework kubernetes -o json > checkov_report_kubernetes-goat.json
+ pipenv run checkov -s -d kubernetes-goat/ --framework helm -o json > checkov_report_kubernetes-goat-helm.json
pipenv run checkov -s --skip-check CKV_AWS_33,CKV_AWS_41 -d terragoat/terraform/ -o json > checkov_report_terragoat_with_skip.json
pipenv run checkov -s -d cfngoat/ -o json --quiet > checkov_report_cfngoat_quiet.json


message: Add debug statement when caller context cannot be found

diff:
@@ -142,6 +142,7 @@ class Runner(BaseRunner):
"module." + referrer_id,
separator=".")
except KeyError:
+ logging.debug("Unable to find caller context for: %s", abs_caller_file)
caller_context = None
if caller_context:


diff:
@@ -6,7 +6,7 @@ from checkov.common.models.enums import CheckResult
from checkov.common.models.consts import ANY_VALUE
from checkov.common.util.type_forcers import force_list
-VARIABLE_DEPENDANT_REGEX = r'(?:local|var)\.[^\s]+'
+VARIABLE_DEPENDANT_REGEX = re.compile(r'\${([^{}]+?)}')
class BaseResourceValueCheck(BaseResourceCheck):
@@ -25,7 +25,10 @@ class BaseResourceValueCheck(BaseResourceCheck):
@staticmethod
def _is_variable_dependant(value):
- if isinstance(value, str) and re.match(VARIABLE_DEPENDANT_REGEX, value):
+ if not isinstance(value, str):
+ return False
+
+ if VARIABLE_DEPENDANT_REGEX.search(value):
return True
return False

diff:
+import unittest
+
+from checkov.common.models.consts import ANY_VALUE
+from checkov.common.models.enums import CheckResult
+from checkov.terraform.checks.resource.base_resource_value_check import BaseResourceValueCheck
+from checkov.terraform.checks.resource.registry import resource_registry
+
+
+class TestAnyCheck(BaseResourceValueCheck):
+ def __init__(self):
+ super().__init__("Ensure it ain't broke", "test/TestAnyCheck", [], ["doesnt_matter"])
+
+ def get_inspected_key(self):
+ return "foo"
+
+ def get_expected_value(self):
+ return ANY_VALUE
+
+
+class TestStaticCheck(BaseResourceValueCheck):
+ def __init__(self):
+ super().__init__("Ensure it ain't broke", "test/TestStaticCheck", [], ["doesnt_matter"])
+
+ def get_inspected_key(self):
+ return "foo"
+
+ def get_expected_value(self):
+ return "bar"
+
+
+class Test(unittest.TestCase):
+ def test_string_match_any(self):
+ result = self._check(TestAnyCheck(),
+ {"foo": "bar"})
+ self.assertEqual(result, CheckResult.PASSED)
+
+ def test_string_match_static(self):
+ result = self._check(TestStaticCheck(),
+ {"foo": "bar"})
+ self.assertEqual(result, CheckResult.PASSED)
+
+ def test_string_mismatch_static(self):
+ result = self._check(TestStaticCheck(),
+ {"foo": "definitely not bar"})
+ self.assertEqual(result, CheckResult.FAILED)
+
+ def test_string_contains_var_any(self):
+ result = self._check(TestAnyCheck(),
+ {"foo": "something-${var.whatever}"})
+ self.assertEqual(result, CheckResult.PASSED)
+
+ def test_string_contains_var_static(self):
+ result = self._check(TestStaticCheck(),
+ {"foo": "something-${var.whatever}"})
+ self.assertEqual(result, CheckResult.PASSED)
+
+ def test_var_any(self):
+ result = self._check(TestAnyCheck(),
+ {"foo": "${var.whatever}"})
+ self.assertEqual(result, CheckResult.PASSED)
+
+ def test_var_static(self):
+ result = self._check(TestStaticCheck(),
+ {"foo": "${var.whatever}"})
+ self.assertEqual(result, CheckResult.PASSED)
+
+ def test_local_any(self):
+ result = self._check(TestAnyCheck(),
+ {"foo": "${local.whatever}"})
+ self.assertEqual(result, CheckResult.PASSED)
+
+ def test_local_static(self):
+ result = self._check(TestStaticCheck(),
+ {"foo": "${local.whatever}"})
+ self.assertEqual(result, CheckResult.PASSED)
+
+ def test_resource_any(self):
+ result = self._check(TestAnyCheck(),
+ {"foo": "${aws_s3_bucket.foo.bucket}"})
+ self.assertEqual(result, CheckResult.PASSED)
+
+ def test_resource_static(self):
+ result = self._check(TestStaticCheck(),
+ {"foo": "${aws_s3_bucket.foo.bucket}"})
+ self.assertEqual(result, CheckResult.PASSED)
+
+ @staticmethod
+ def _check(check, data):
+ return check.scan_resource_conf(data)
+
+ # This will install a custom check, so setUp/tearDown will ensure the check list is unchanged
+ # globally by our changes.
+ def setUp(self) -> None:
+ self.check_list_before = resource_registry.checks.copy() # copy
+ super().setUp()
+
+ def tearDown(self) -> None:
+ super().tearDown()
+ resource_registry.checks = self.check_list_before
+ self.check_list_before = None

original_message:
Correct regex determining if a value if var dependant.

Fixes: #924


message: Switch from regex to sharing logic from parser_var_blocks for var identification

diff:
@@ -5,8 +5,7 @@ from checkov.terraform.checks.resource.base_resource_check import BaseResourceCh
from checkov.common.models.enums import CheckResult
from checkov.common.models.consts import ANY_VALUE
from checkov.common.util.type_forcers import force_list
-
-VARIABLE_DEPENDANT_REGEX = re.compile(r'\${([^{}]+?)}')
+from checkov.terraform.parser_var_blocks import find_var_blocks
class BaseResourceValueCheck(BaseResourceCheck):
@@ -27,8 +26,10 @@ class BaseResourceValueCheck(BaseResourceCheck):
def _is_variable_dependant(value):
if not isinstance(value, str):
return False
+ if "${" not in value:
+ return False
- if VARIABLE_DEPENDANT_REGEX.search(value):
+ if find_var_blocks(value):
return True
return False