checkov icon indicating copy to clipboard operation
checkov copied to clipboard

CKV_AZURE_218 failing

Open horiagunica opened this issue 1 year ago • 10 comments

Describe the issue Check CKV_AZURE_218 - "Ensure Application Gateway defines secure protocols for in transit communication" is failing in the following scenario :

Check: CKV_AZURE_218: "Ensure Application Gateway defines secure protocols for in transit communication"
        FAILED for resource: module.appgw.azurerm_application_gateway.this
        File: /modules/appgw/main.tf:48-313
        Calling File: /examples/standalone_vmseries/main.tf:307-339

Repository : https://github.com/PaloAltoNetworks/terraform-azurerm-vmseries-modules/tree/checkov-version-bump-fixes

Examples

Inside the module - I am specifically setting the default values :

variable "ssl_policy_type" {
  description = <<-EOF
  Type of an SSL policy. Possible values are `Predefined` or `Custom`.
  If the value is `Custom` the following values are mandatory: `ssl_policy_cipher_suites` and `ssl_policy_min_protocol_version`.
  EOF
  default     = "Predefined"
  type        = string
  nullable    = false
}

variable "ssl_policy_name" {
  description = <<-EOF
  Name of an SSL policy. Supported only for `ssl_policy_type` set to `Predefined`. Normally you can set it also for `Custom` policies but the name is discarded on Azure side causing an update to Application Gateway each time terraform code is run. Therefore this property is omitted in the code for `Custom` policies. 
  
  For the `Predefined` polcies, check the [Microsoft documentation](https://docs.microsoft.com/en-us/azure/application-gateway/application-gateway-ssl-policy-overview) for possible values as they tend to change over time. The default value is currently (Q1 2022) a Microsoft's default.
  EOF
  default     = "AppGwSslPolicy20220101S"
  type        = string
  nullable    = false
}

variable "ssl_policy_min_protocol_version" {
  description = <<-EOF
  Minimum version of the TLS protocol for SSL Policy. Required only for `ssl_policy_type` set to `Custom`. 

  Possible values are: `TLSv1_0`, `TLSv1_1`, `TLSv1_2` or `null` (only to be used with a `Predefined` policy).
  EOF
  default     = "TLSv1_2"
  type        = string
}

variable "ssl_policy_cipher_suites" {
  description = <<-EOF
  A list of accepted cipher suites. Required only for `ssl_policy_type` set to `Custom`. 
  For possible values see [documentation](https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/application_gateway#cipher_suites).
  EOF
  default     = ["TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"]
  type        = list(string)
}

The calling file standalone_vmseries example calls the module but in .tfvars it doesn't have a value for the map - which means the resource is not actually used there (it's present for modularity and flexibility only).

Version (please complete the following information):

  • Checkov Version 2.3.310

Additional context

We are running checkov from pre-commit inside a runner (local tests offered the same results):

- hooks:
  - args:
    - --compact
    - --quiet
    - --skip-check
    - CKV_AZURE_118,CKV_AZURE_119,CKV_AZURE_120,CKV2_AZURE_10,CKV2_AZURE_12,CKV_AZURE_35,CKV_AZURE_206,CKV_AZURE_93,CKV2_AZURE_1,CKV2_AZURE_18,CKV_AZURE_97,CKV_AZURE_59,CKV_AZURE_190,CKV2_AZURE_33,CKV_AZURE_179,CKV_AZURE_1,CKV_AZURE_49,CKV_AZURE_217
    id: checkov
    verbose: true
  repo: https://github.com/bridgecrewio/checkov.git
  rev: 2.3.310

horiagunica avatar Jul 04 '23 12:07 horiagunica

This is the checkov parser. I don't think theres any support for the try function your using for a lot of your variables, so we end up with policy_type='try(each.value.ssl_policy_type,None)' inside of a nested for_each. and the use of policy_name = var.ssl_policy_type == "Predefined" ? var.ssl_policy_name : null and that within a dynamic block.

JamesWoolfenden avatar Jul 14 '23 08:07 JamesWoolfenden

Hi @JamesWoolfenden ! Thank you for investigating this. Development of such deep parsing sounds quite complex...I'm guessing we should not expect such a feature soon ? We can just skip the check for now - I just wanted to ask before we actually do 🙇

horiagunica avatar Jul 19 '23 07:07 horiagunica

If your using TLSv1_2 or better and none of the "bad cyphers" https://github.com/bridgecrewio/checkov/blob/main/checkov/terraform/checks/resource/azure/AppGWDefinesSecureProtocols.py then you should be golden.

JamesWoolfenden avatar Jul 19 '23 13:07 JamesWoolfenden

@JamesWoolfenden Hello!

Unfortunately it doesn't work - I already set those as defaults inside the module (and not modifying them in the main calling example TF code) and the issue still triggers (copy-pasting from the initial issue description) :

variable "ssl_policy_min_protocol_version" {
  description = <<-EOF
  Minimum version of the TLS protocol for SSL Policy. Required only for `ssl_policy_type` set to `Custom`. 

  Possible values are: `TLSv1_0`, `TLSv1_1`, `TLSv1_2` or `null` (only to be used with a `Predefined` policy).
  EOF
  default     = "TLSv1_2"
  type        = string
}

variable "ssl_policy_cipher_suites" {
  description = <<-EOF
  A list of accepted cipher suites. Required only for `ssl_policy_type` set to `Custom`. 
  For possible values see [documentation](https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/application_gateway#cipher_suites).
  EOF
  default     = ["TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"]
  type        = list(string)
}

horiagunica avatar Jul 24 '23 16:07 horiagunica

@JamesWoolfenden is there any chance to fix this here ? Or shall we just suppress it until further notice ?

horiagunica avatar Oct 30 '23 09:10 horiagunica

@horiagunica id send you requests to @gruebel as im not in the engineering team.

JamesWoolfenden avatar Oct 30 '23 10:10 JamesWoolfenden

Hi @gruebel, are there any updates on this?

acelebanski avatar Apr 02 '24 16:04 acelebanski

hey @acelebanski support for try was added via #6043, so there is a chance it works now, but of course there can be other factors making it still fail.

gruebel avatar Apr 02 '24 16:04 gruebel