Repository: bridgecrewio/checkov
Pull request: Correct regex determining if a value if var dependant.
Fixes: #924

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


Problem statement:
BaseResourceValueCheck._is_variable_dependant incorrect
**Describe the bug**
BaseResourceValueCheck is supposed to ignore values that have unresolved data, but its pattern matching isn't looking for the correct thing, so it doesn't work. For example, these values won't cause the check to be skipped: `${var.whatever}`, `something-${local.whatever}`.

It's looking (essentially, this is simplified a bit) for `var.*` or `local.*`, which isn't correct because the parser will provide values wrapped in eval markers.

**To Reproduce**
Steps to reproduce the behavior:
1. Create a check looking for a specific value
2. Provide terraform with a value of `var.SOMETHING_THAT_WONT_RESOLVE`. (Note: the check will see `${var.SOMETHING_THAT_WONT_RESOLVE`)
3. See that the check fails rather than passing

**Expected behavior**
Check should pass.

Hints from issue discussion:

Commit message: Correct regex determining if a value if var dependant.

Fixes: #924

Patch to review:
diff --git a/checkov/terraform/checks/resource/base_resource_value_check.py b/checkov/terraform/checks/resource/base_resource_value_check.py
index d312b4e1ac..c95eb51d4f 100644
--- a/checkov/terraform/checks/resource/base_resource_value_check.py
+++ b/checkov/terraform/checks/resource/base_resource_value_check.py
@@ -6,7 +6,7 @@
 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 @@ def _filter_key_path(path):
 
     @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 --git a/tests/terraform/checks/resource/test_base_resource_value_check.py b/tests/terraform/checks/resource/test_base_resource_value_check.py
new file mode 100644
index 0000000000..68f36c7f2a
--- /dev/null
+++ b/tests/terraform/checks/resource/test_base_resource_value_check.py
@@ -0,0 +1,100 @@
+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


Code review comments:
diff_hunk: @@ -6,7 +6,7 @@
 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'\${([^{}]+?)}')
path: checkov/terraform/checks/resource/base_resource_value_check.py
text: @user1:
@author It looks like that the proposed regex would not match variable dependent expression such as 
`foo = var.bar` which is valid, so I suggest to let the curly braces be optional instead of mandatory.
Another issue is that it would match a non-dependent expression such as `foo={"bar"}`.

@author:
That's valid for Terraform, but not something the check should ever see. The HCL2 parser (`bc-python-hcl2` or `python-hcl2`) will always wrap it to `${var.bar}`.

The regex is only checking the value portion, so for the second, I don't think it applies?... unless I misunderstand you.

Assuming this terraform:
```
resource aws_s3_bucket test {
  foo = var.bar
  foo2 = { 
    "bar" = "baz"
  }
}
```

`foo` would be evaluated by the HCL parser to `["${var.bar}"]` and the value seen by the regex would be `${var.bar}`.
`foo2` would be evaluated to a map and so doesn't apply.

I could see a false hit with a TF string like `foo = "This eval has escapes: $${var.tricky}"`. But not requiring the eval brackets probably just makes that worse.

The regex actually comes from what I originally used when I redid the terraform parser. The problem is had was that it didn't real with really complex values (such as maps within the statement)... which is a consideration, I suppose. We could:

1. leave as-is which handles, I think, most of the likely cases but can miss some complex stuff.
2. change to `"${" in value`, which probably also handles most cases, frankly. It would misfire on escaped "${" values, which probably aren't that likely.
3. Use the full "var block" searching logic from the Terraform parser, which would work but seems overkill.

And of course, if I misunderstand you and missed a case, please let me know.

@user1:
@author I wasn't aware that the expression `var.foo` would be outputted from hcl2 as `${var.foo}`. If so, I tend to go with option 3 you mentioned, as expressions can be complex and we don't want to miss that.

@author:
@user1 - Seems reasonable to me. Change pushed.

@author:
@user1 - BTW, I should have mentioned I've been putting tests for how the HCL2 parser behaves into [`test_hcl2_load_assumptions.py`](https://github.com/bridgecrewio/checkov/blob/master/tests/terraform/parser/test_hcl2_load_assumptions.py). I don't have a simple case for variables in there, but you can see them wrapped in some more complex cases.