checkov icon indicating copy to clipboard operation
checkov copied to clipboard

CKV2_AWS_32 Incomplete Documentation, Not As Described

Open jamiepedwards opened this issue 2 years ago • 6 comments

Describe the issue CKV2_AWS_32 says it checks to see if a strict response headers policy (e.g. aws_cloudfront_response_headers_policy resource) is attached to the Cloudfront distribution. The documentation suggests this can be remediated by simply specifying the response_headers_policy_id attribute, but this is not enough. To be able to get this check to pass, I had to actually read the source code for the check.

It's not just checking that a policy is attached, it's also reviewing the attached policy to confirm that each of the attributes in the policy are configured to very specific undocumented values with little flexibility. While I'm sure that the values are well researched and driven by best practices, I don't feel that this implementation is effective as it pretty much guarantees that in almost all cases people will disable it.

In addition to being very difficult to meet the requirements to get this check to pass, it's also very brittle - if I define the aws_cloudfront_response_headers_policy in a different module or build the contents dynamically, even if they are correct, it's impossible to get the check to pass and I still end up adding an annotation anyway.

Lastly, the example given at https://docs.bridgecrew.io/docs/bc_aws_networking_65 does not even pass validation as response_headers_policy_id does not belong under origin, the correct place is inside a cache behaviour block.

Examples This code will fail the check:

resource "aws_cloudfront_distribution" "example" {
  aliases = [local.cloudfront_hostname]

  default_cache_behavior {
    allowed_methods  = ["GET", "HEAD", "OPTIONS", "PUT", "POST", "PATCH", "DELETE"]
    cached_methods   = ["GET", "HEAD"]
    target_origin_id = local.origin_id

    forwarded_values {
      query_string = false

      cookies {
        forward = "none"
      }
    }

    max_ttl = 60

    viewer_protocol_policy = "redirect-to-https"
  }

  enabled = true

  origin {
    domain_name = local.alb_dns_name
    origin_id   = local.origin_id
    custom_origin_config {
      http_port              = 80
      https_port             = 443
      origin_protocol_policy = "http-only"
      origin_ssl_protocols   = ["TLSv1.2"]
    }
  }

  restrictions {
    geo_restriction {
      restriction_type = "none"
    }
  }

  viewer_certificate {
    cloudfront_default_certificate = "true"
  }
}

This code (which is written as per the documentation at https://docs.bridgecrew.io/docs/bc_aws_networking_65 suggests) will also fail the check:

resource "aws_cloudfront_distribution" "example" {
  aliases = [local.cloudfront_hostname]

  default_cache_behavior {
    allowed_methods  = ["GET", "HEAD", "OPTIONS", "PUT", "POST", "PATCH", "DELETE"]
    cached_methods   = ["GET", "HEAD"]
    target_origin_id = local.origin_id
    cache_policy_id            = "ea271dfd-0d02-4c7c-a9b2-6b2bad9a679b"
    origin_request_policy_id   = "fd271dfc-0d02-4c7c-a9b2-6b2bad9a679b"
    response_headers_policy_id = "ea271dfc-0d02-4c7c-a9b2-6b2bad9a679b"
    viewer_protocol_policy     = "redirect-to-https"

    forwarded_values {
      query_string = false

      cookies {
        forward = "none"
      }
    }

    max_ttl = 60

    viewer_protocol_policy = "redirect-to-https"
  }

  enabled = true

  origin {
    domain_name = local.alb_dns_name
    origin_id   = local.origin_id
    custom_origin_config {
      http_port              = 80
      https_port             = 443
      origin_protocol_policy = "http-only"
      origin_ssl_protocols   = ["TLSv1.2"]
    }
  }

  restrictions {
    geo_restriction {
      restriction_type = "none"
    }
  }

  viewer_certificate {
    cloudfront_default_certificate = "true"
  }
}

To get the above to pass the check, you need to also define (and reference) a aws_cloudfront_response_headers_policy that matches the below policy exactly, with the exception of the aws_cloudfront_response_headers_policy.content_security_policy.content_security_policy attribute, which is flexible but must at least contain the value default-src 'none';

resource "aws_cloudfront_response_headers_policy" "example" {
  name = "mypolicy"

  security_headers_config {
    frame_options {
      frame_option = "DENY"
      override     = true
    }
    content_type_options {
      override = true
    }
    content_security_policy {
      content_security_policy = "default-src 'none'; <more CSP directives here>"
      override                = true
    }
    referrer_policy {
      override        = true
      referrer_policy = "same-origin"
    }
    strict_transport_security {
      override                   = true
      access_control_max_age_sec = 31536000
      include_subdomains         = true
      preload                    = true
    }
    xss_protection {
      override   = true
      mode_block = true
      protection = true
    }
  }
}

Version (please complete the following information):

  • Checkov Version 2.0.982

Additional context Suggested resolution: Don't include the deep checks for aws_cloudfront_response_headers_policy in this check, it might be a better idea to define those deep checks in a separate check that specifically examines aws_cloudfront_response_headers_policy resources. This would be more flexible and allow that resource to potentially be defined in a different place and still be subject to the same level of scrutiny, and encourage people to at least adopt the new aws_cloudfront_response_headers_policy method, they can check to see that a policy is at least defined, even if they don't agree with the rigid requirements to meet the best practices.

jamiepedwards avatar Apr 26 '22 20:04 jamiepedwards

@gruebel tagging you since you were the one who added this check (https://github.com/bridgecrewio/checkov/pull/1935), wondering if you can provide input. I find this check is useful, it just needs a bit more polish.

jamiepedwards avatar Apr 26 '22 20:04 jamiepedwards

We are hitting this too. We use a var to pass in the policy and define it elsewhere. We thought we could just update the default to use one of AWS managed policies but go as described above, hopefully this can be addressed!

lorelei-rupp-imprivata avatar May 19 '22 19:05 lorelei-rupp-imprivata

Sorry, I thought I actually answered here already 😄 @jamiepedwards yeah it makes sense to split the check, I also thought about splitting it in the mentioned connection check and then for each header one dedicated check, WDYT? @lorelei-rupp-imprivata also would like to hear your opinion 🙂

gruebel avatar May 20 '22 08:05 gruebel

Sorry, I thought I actually answered here already 😄 @jamiepedwards yeah it makes sense to split the check, I also thought about splitting it in the mentioned connection check and then for each header one dedicated check, WDYT? @lorelei-rupp-imprivata also would like to hear your opinion 🙂

Yeah I like your idea of having it be a check on its own so we could have our other module scanned, but also like the idea to ensure cloudfronts are setting it too, but thats kinda hard if you define that in another module, it would have to rely on a default set, which I am ok with too

lorelei-rupp-imprivata avatar May 20 '22 13:05 lorelei-rupp-imprivata

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 Nov 17 '22 22:11 stale[bot]

I agree with the suggested solution. The check shouldn't be doing the extra deep check. Instead of a seperate check on the header policy, so it can reside in another module or somewhere else entirely

lorelei-rupp-imprivata avatar Nov 18 '22 01:11 lorelei-rupp-imprivata

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 18 '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]