checkov icon indicating copy to clipboard operation
checkov copied to clipboard

CKV_AZURE_109 - ensure key vault allows firewall rules settings - FAILS in module when using dynamic rules

Open Marcus-James-Adams opened this issue 2 years ago • 5 comments

Describe the issue CKV_AZURE_109 - ensure key vault allows firewall rules settings - FAILS when using dynamic network_acls Examples Calling module in terraform

module "key_vault" {
  source                          = "../../../../../terraform-modules/azurerm/key-vault"
  vault_name                      = "vmi"
  name                            = var.service
  location                        = var.location
  org_short                       = var.org_short
  env_short                       = var.env_short
  loc_short                       = var.loc_short
  enabled_for_deployment          = true
  enabled_for_disk_encryption     = true
  enabled_for_template_deployment = true
  sku_name                        = "standard"
  network_acls = [
    {
      "bypass"                     = "AzureServices"
      "default_action"             = "Deny"
      "ip_rules"                   = flatten([formatlist("%s/32", var.nw-staff-ip-address-list), formatlist("%s/32", var.nw-other-allowed-ip-address-list)])
      "virtual_network_subnet_ids" = ""
    },
  ]
  default_tags = local.default_tags
}

This is the relevant module code

resource "azurerm_key_vault" "main" {
  name                            = "${var.org_short}-${var.env_short}-${var.loc_short}-${var.name}-${var.vault_name}-kv"
  location                        = var.location
  resource_group_name             = azurerm_resource_group.main.name
  tenant_id                       = data.azurerm_client_config.main.tenant_id
  sku_name                        = var.sku_name
  purge_protection_enabled        = true
  enabled_for_deployment          = var.enabled_for_deployment
  enabled_for_disk_encryption     = var.enabled_for_disk_encryption
  enabled_for_template_deployment = var.enabled_for_template_deployment

  dynamic "network_acls" {
    for_each = var.network_acls
    content {
      bypass                     = network_acls.value.bypass
      default_action             = network_acls.value.default_action
      ip_rules                   = network_acls.value.ip_rules == "" ? [] : flatten([network_acls.value.ip_rules])
      virtual_network_subnet_ids = network_acls.value.virtual_network_subnet_ids == "" ? [] : flatten([network_acls.value.virtual_network_subnet_ids])
    }
  }

  tags = var.default_tags
}

Version (please complete the following information):

  • Checkov Version 2.0.1004

Additional context Add any other context about the problem here.

Marcus-James-Adams avatar Apr 06 '22 11:04 Marcus-James-Adams

@Marcus-James-Adams The problem is that the check code is not handling the for-each. The logic in KeyVaultEnablesFirewallRulesSettings.py is looking for a key network_acls/0/default_action with a value of Deny, however, due to the for_each the actual value is network_acls.value.default_action, so the test fails. Can't seem to find where the for_each is handled in the parser, but it won't work for your sample.

MooreDerek avatar Apr 13 '22 10:04 MooreDerek

facing the same issue

guidooliveira avatar Jul 27 '22 21:07 guidooliveira

I am facing similar issue but without dynamic blocks. Here's my code:

locals {
  purge_protection_enabled = var.testing ? false : var.purge_protection_enabled

  firewall_policy = {
    bypass                     = var.nacl_bypass_azureservices ? "AzureServices" : "None"
    default_action             = var.nacl_default_action_allow || var.testing ? "Allow" : "Deny"
    virtual_network_subnet_ids = var.nacl_subnet_ids

    ip_rules = distinct(concat(
      [
        # default ranges
      ], var.nacl_ip_rules
    ))
  }

  tenant_id       = data.azurerm_subscription.current.tenant_id
  subscription_id = data.azurerm_subscription.current.id
}

# Create Key Vault
resource "azurerm_key_vault" "this" {
  name                            = var.name
  location                        = var.resource_group.location
  resource_group_name             = var.resource_group.name
  sku_name                        = "standard"
  enabled_for_disk_encryption     = var.enabled_for_disk_encryption
  enabled_for_deployment          = var.enabled_for_deployment
  enabled_for_template_deployment = var.enabled_for_template_deployment
  tenant_id                       = local.tenant_id
  soft_delete_retention_days      = 30
  purge_protection_enabled        = local.purge_protection_enabled
  enable_rbac_authorization       = true # ADR:<TBD>

  network_acls {
    bypass                     = local.firewall_policy.bypass
    default_action             = local.firewall_policy.default_action
    ip_rules                   = local.firewall_policy.ip_rules
    virtual_network_subnet_ids = local.firewall_policy.virtual_network_subnet_ids
  }

  tags = var.tags
}

When I run terraform plan, there are no changes detected, Key Vault is created with Deny as default action and permits AzureServices (this is inline with CKV_AZURE_109). However, Checkov fails:

<testcase name="[NONE][CKV_AZURE_109] Ensure that key vault allows firewall rules settings" classname="/.external_modules/---/azure-migration/modules/key-vault/HEAD/main.tf.azurerm_key_vault.this" file="/.external_modules/---/azure-migration/modules/key-vault/HEAD/main.tf">
			<failure type="failure" message="Ensure that key vault allows firewall rules settings">
Resource: azurerm_key_vault.this
File: /.external_modules/---/azure-migration/modules/key-vault/HEAD/main.tf: 53-74
Guideline: https://docs.bridgecrew.io/docs/ensure-that-key-vault-allows-firewall-rules-settings
		53 | resource &quot;azurerm_key_vault&quot; &quot;this&quot; {
		54 |   name                            = var.name
		55 |   location                        = var.resource_group.location
		56 |   resource_group_name             = var.resource_group.name
		57 |   sku_name                        = &quot;standard&quot;
		58 |   enabled_for_disk_encryption     = var.enabled_for_disk_encryption
		59 |   enabled_for_deployment          = var.enabled_for_deployment
		60 |   enabled_for_template_deployment = var.enabled_for_template_deployment
		61 |   tenant_id                       = local.tenant_id
		62 |   soft_delete_retention_days      = 30
		63 |   purge_protection_enabled        = local.purge_protection_enabled
		64 |   enable_rbac_authorization       = true # ADR:&lt;TBD&gt;
		65 | 
		66 |   network_acls {
		67 |     bypass                     = local.firewall_policy.bypass
		68 |     default_action             = local.firewall_policy.default_action
		69 |     ip_rules                   = local.firewall_policy.ip_rules
		70 |     virtual_network_subnet_ids = local.firewall_policy.virtual_network_subnet_ids
		71 |   }
		72 | 
		73 |   tags = var.tags
		74 | }
</failure>
		</testcase>

I'd like to highlight the fact that terraform does not detect ANY changes to the code or state. It does look like Checkov doesn't like dynamic logic hidden within locals block, and as such it can't detect that criteria has been in fact met by the code and combination of variables. Specifically this line, I believe, confuses Checkov:

default_action = var.nacl_default_action_allow || var.testing ? "Allow" : "Deny"

Is this expected? Do you want me to raise separate issue for this?

Yakuza-UA avatar Aug 10 '22 09:08 Yakuza-UA

My issues does seem like a separate use case. I've just tested it by removing logical OR (|| var.testing) and it has passed the check. IMHO, looks like Checkov is unable to evaluate logical expression to understand what will be the final value fed into resource. Is this a known limitation?

Yakuza-UA avatar Aug 10 '22 10:08 Yakuza-UA

Same issue here,

Even if I hard code Deny and AzureServices in the module it fails.

Check: CKV_AZURE_109: "Ensure that key vault allows firewall rules settings" FAILED for resource: module.kv_lab_hub.azurerm_key_vault.this File: /../../../../TerraformResources/keyvault/main.tf:1-25 Calling File: /keyvault.tf:1-29 Guide: https://docs.bridgecrew.io/docs/ensure-that-key-vault-allows-firewall-rules-settings

image

dbiegunski avatar Nov 20 '22 17:11 dbiegunski

Thanks for contributing to Checkov! We've automatically marked this issue as stale to keep our issues list tidy, because it has not had any activity for 6 months. It will be closed in 14 days if no further activity occurs. Commenting on this issue will remove the stale tag. If you want to talk through the issue or help us understand the priority and context, feel free to add a comment or join us in the Checkov slack channel at https://slack.bridgecrew.io Thanks!

stale[bot] avatar May 21 '23 08:05 stale[bot]

Closing issue due to inactivity. If you feel this is in error, please re-open, or reach out to the community via slack: https://slack.bridgecrew.io Thanks!

stale[bot] avatar Jun 08 '23 17:06 stale[bot]