Repository: bridgecrewio/checkov
Pull request: Fix NSGRulePortAccessRestricted crashing on a terraform dynamic block
Fixes #488

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


Problem statement:
Checkov crashes when evaluating a Terraform dynamic block in NSGRulePortAccessRestricted.py
**Describe the bug**
When checking azure_security_group_rule, azurerm_network_security_rule or azurerm_network_security_group Terraform resource types, NSGRulePortAccessRestricted.py throws a "TypeError: string indices must be integers" error whenever there's a dynamic block.

**To Reproduce**
Steps to reproduce the behavior:
1. Create a resource in terraform, containing a dynamic security rule -
```
resource "azurerm_network_security_group" "snet_nsgs" {
  count               = "${length(local.subnets)}"
  name                = "${local.root}-snet-${lookup(local.subnets[count.index], "name")}-nsg"
  location            = "${azurerm_resource_group.net_rg.location}"
  resource_group_name = "${azurerm_resource_group.net_rg.name}"
  tags                = "${local.tags}"


  dynamic "security_rule" {
    for_each = [for s in local.subnets[count.index].nsg_rules : {
      name                       = s.name
      priority                   = s.priority
      direction                  = s.direction
      access                     = s.access
      protocol                   = s.protocol
      source_port_range          = s.source_port_range
      destination_port_range     = s.destination_port_range
      source_address_prefix      = s.source_address_prefix
      destination_address_prefix = s.destination_address_prefix
      description                = s.description
    }]
    content {
      name                       = security_rule.value.name
      priority                   = security_rule.value.priority
      direction                  = security_rule.value.direction
      access                     = security_rule.value.access
      protocol                   = security_rule.value.protocol
      source_port_range          = security_rule.value.source_port_range
      destination_port_range     = security_rule.value.destination_port_range
      source_address_prefix      = security_rule.value.source_address_prefix
      destination_address_prefix = security_rule.value.destination_address_prefix
      description                = security_rule.value.description
    }
  }
}
```
2. Run checkov
3. Error!

**Expected behavior**
As checkov cannot evaluate the dynamic block, I expect the check to be skipped without throwing an error.

**Desktop (please complete the following information):**
 - OS: Ubuntu
 - Checkov Version 1.0.479

Hints from issue discussion:

Commit message: One more try...

Looking at the rest of the code, seems CheckResult.UNKNOWN is probably more suitable.

Patch to review:
diff --git a/checkov/terraform/checks/resource/azure/NSGRulePortAccessRestricted.py b/checkov/terraform/checks/resource/azure/NSGRulePortAccessRestricted.py
index 6b09a00414..60843f35fa 100644
--- a/checkov/terraform/checks/resource/azure/NSGRulePortAccessRestricted.py
+++ b/checkov/terraform/checks/resource/azure/NSGRulePortAccessRestricted.py
@@ -26,6 +26,9 @@ def is_port_in_range(self, conf):
         return False
 
     def scan_resource_conf(self, conf):
+        if any("dynamic" in s for s in conf):
+            return CheckResult.UNKNOWN
+            
         rule_confs = [conf]
         if 'security_rule' in conf:
             rule_confs = conf['security_rule']


Code review comments:
diff_hunk: @@ -26,6 +26,9 @@ def is_port_in_range(self, conf):
         return False
 
     def scan_resource_conf(self, conf):
+        if any("dynamic" in s for s in conf):
path: checkov/terraform/checks/resource/azure/NSGRulePortAccessRestricted.py
text: @user1:
This test is really wide. Why not just `if "dynamic" in conf`?

Currently dynamic can only be used for `security_rule` so this will not skip other cases. What do you think about an additional check that dynamic is used for `security_rule`?

@user2:
Hey @author !

Thanks for the contribution. However, this change will mean if the resource has **any** dynamic blocks in it, it will return as `UNKNOWN`. IMO it would be better to check the `rule_conf` - verify it is a dict before checking fields inside `rule_conf`. WDYT?