bridgecrewio / checkov

Prevent cloud misconfigurations and find vulnerabilities during build-time in infrastructure as code, container images and open source packages with Checkov by Bridgecrew.
https://www.checkov.io/
Apache License 2.0
7.08k stars 1.11k forks source link

CKV2_IBM_1 is failing if parsing value from a variable #5824

Open ocofaigh opened 10 months ago

ocofaigh commented 10 months ago

Describe the issue Looking at the logic here, the check for CKV2_IBM_1 is only checking if the value of type is set to private. Our module actually gets the value of this from a variable, and so the check fails with this:

Check: CKV2_IBM_1: "Ensure load balancer for VPC is private (disable public access)"
    FAILED for resource: module.slz_vsi.ibm_is_lb.lb
    File: /load_balancer.tf:12-22

        12 | resource "ibm_is_lb" "lb" {
        13 |   for_each        = local.load_balancer_map
        14 |   name            = "${var.prefix}-${each.value.name}-lb"
        15 |   subnets         = var.subnets[*].id
        16 |   type            = each.value.type
        17 |   profile         = each.value.profile
        18 |   security_groups = each.value.security_group == null ? null : [ibm_is_security_group.security_group[each.value.security_group.name].id]
        19 |   resource_group  = var.resource_group_id
        20 |   tags            = var.tags
        21 |   access_tags     = var.access_tags
        22 | }

I don't think the logic should be failing here since the value is going to be passed by the consumer. Perhaps the logic should be updated to ensure type is not set to public?

Examples https://github.com/terraform-ibm-modules/terraform-ibm-landing-zone-vsi/blob/main/load_balancer.tf

Version (please complete the following information):

Additional context Add any other context about the problem here.

Saarett commented 10 months ago

@ocofaigh Thanks for reaching out 🙂 @praveen-panw Can I have your thoughts here?

gruebel commented 10 months ago

The default of the type field public, that's why you have to set it especially to private. The reason why it flags for your case is the usage of for_each. We try our best to evaluate it correctly, but this is not always possible. So, this is not a specific issue with the check itself, but the incorrect evaluation of the for_each field.

Saarett commented 10 months ago

@gruebel Thanks for the input, @gruebel ! We will prioritize this task internally.

ocofaigh commented 10 months ago

This seems to be a general pattern issue for other rules too. For example, I'm seeing the following failures because values are being passed by variables:

Check: CKV2_IBM_5: "Ensure Service ID creation is restricted in account settings"
    FAILED for resource: module.iam_account_settings.ibm_iam_account_settings.iam_account_settings
    File: /main.tf:18-36

        18 | resource "ibm_iam_account_settings" "iam_account_settings" {
        19 |   if_match                  = "*"
        20 |   allowed_ip_addresses      = local.iam_allowed_ip_addresses
        21 |   max_sessions_per_identity = var.max_sessions_per_identity
        22 |   mfa                       = var.mfa
        23 |   dynamic "user_mfa" {
        24 |     for_each = local.user_mfa_list
        25 |     content {
        26 |       iam_id = user_mfa.value.iam_id
        27 |       mfa    = user_mfa.value.mfa
        28 |     }
        29 |   }
        30 |   restrict_create_service_id                 = var.serviceid_creation
        31 |   restrict_create_platform_apikey            = var.api_creation
        32 |   session_expiration_in_seconds              = var.active_session_timeout
        33 |   session_invalidation_in_seconds            = var.inactive_session_timeout
        34 |   system_access_token_expiration_in_seconds  = var.access_token_expiration
        35 |   system_refresh_token_expiration_in_seconds = var.refresh_token_expiration
        36 | }

Check: CKV2_IBM_3: "Ensure API key creation is restricted in account settings"
    FAILED for resource: module.iam_account_settings.ibm_iam_account_settings.iam_account_settings
    File: /main.tf:18-36

        18 | resource "ibm_iam_account_settings" "iam_account_settings" {
        19 |   if_match                  = "*"
        20 |   allowed_ip_addresses      = local.iam_allowed_ip_addresses
        21 |   max_sessions_per_identity = var.max_sessions_per_identity
        22 |   mfa                       = var.mfa
        23 |   dynamic "user_mfa" {
        24 |     for_each = local.user_mfa_list
        25 |     content {
        26 |       iam_id = user_mfa.value.iam_id
        27 |       mfa    = user_mfa.value.mfa
        28 |     }
        29 |   }
        30 |   restrict_create_service_id                 = var.serviceid_creation
        31 |   restrict_create_platform_apikey            = var.api_creation
        32 |   session_expiration_in_seconds              = var.active_session_timeout
        33 |   session_invalidation_in_seconds            = var.inactive_session_timeout
        34 |   system_access_token_expiration_in_seconds  = var.access_token_expiration
        35 |   system_refresh_token_expiration_in_seconds = var.refresh_token_expiration
        36 | }
SteveVaknin commented 8 months ago

Hi @ocofaigh I'm looking into it.

1 - I see that you have an example file with the setting type = "public . tests/terraform/graph/variable_rendering/resources/steve_test/examples/complete/main.tf It might cause the dynamic variable rendering evaluation and fails the test. I'm having some issue with reproducing it in my environment, can you please give that a try?

2- Looks like the second example you shared is not a part of the same repository. Can you please share that one as well?

ocofaigh commented 8 months ago

@SteveVaknin

  1. I created a PR to update the example to use private but it didn't help:
    
    Passed checks: 0, Failed checks: 2, Skipped checks: 0, Parsing errors: 1

Check: CKV2_IBM_1: "Ensure load balancer for VPC is private (disable public access)" FAILED for resource: module.slz_vsi.ibm_is_lb.lb File: /load_balancer.tf:12-22

    12 | resource "ibm_is_lb" "lb" {
    13 |   for_each        = local.load_balancer_map
    14 |   name            = "${var.prefix}-${each.value.name}-lb"
    15 |   subnets         = var.subnets[*].id
    16 |   type            = each.value.type
    17 |   profile         = each.value.profile
    18 |   security_groups = each.value.security_group == null ? null : [ibm_is_security_group.security_group[each.value.security_group.name].id]
    19 |   resource_group  = var.resource_group_id
    20 |   tags            = var.tags
    21 |   access_tags     = var.access_tags
    22 | }

Check: CKV2_IBM_1: "Ensure load balancer for VPC is private (disable public access)" FAILED for resource: module.slz_vsi.module.fscloud_vsi.ibm_is_lb.lb File: /load_balancer.tf:12-22

    12 | resource "ibm_is_lb" "lb" {
    13 |   for_each        = local.load_balancer_map
    14 |   name            = "${var.prefix}-${each.value.name}-lb"
    15 |   subnets         = var.subnets[*].id
    16 |   type            = each.value.type
    17 |   profile         = each.value.profile
    18 |   security_groups = each.value.security_group == null ? null : [ibm_is_security_group.security_group[each.value.security_group.name].id]
    19 |   resource_group  = var.resource_group_id
    20 |   tags            = var.tags
    21 |   access_tags     = var.access_tags
    22 | }


2. The second example is from here -> https://github.com/terraform-ibm-modules/terraform-ibm-iam-account-settings/blob/fa6f70939ac801cd9d0e0c83be47bb3d49fb66c8/main.tf#L30-L31
SteveVaknin commented 6 months ago

Hi @ocofaigh Checked further, It looks like the issue is rendering an empty for_each that is set as the default.
Tried to modify the variable rendering processing but it still reproduces, i'll sync with the team and hope to find a better resolution.

stale[bot] commented 2 weeks ago

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 codifiedsecurity.slack.com Thanks!

ocofaigh commented 20 hours ago

@SteveVaknin Is it something you still plan to look in to?