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
6.76k stars 1.08k forks source link

Wrong report while skipping checks #6014

Open tberreis opened 5 months ago

tberreis commented 5 months ago

Describe the issue As documented here we are able to skip checks by using the parameter '--skip-check SKIP_CHECK'. This works as expected but it seems these skips are not included in the overall report.

The corresponding line of code when building the report here seems to be never reached in case the check is skipped.

Examples terraform file to be checked

resource "azurerm_storage_account" "this" {
  name                            = "test"
  resource_group_name             = "test"
  location                        = "westeurope"
  min_tls_version                 = "TLS1_2"
  allow_nested_items_to_be_public = false

  network_rules {
    default_action = "Deny"
  }
}

Output when checking everything

[root /run/shm]# checkov -f main.tf --compact --quiet
terraform scan results:

Passed checks: 7, Failed checks: 8, Skipped checks: 0

Check: CKV_AZURE_33: "Ensure Storage logging is enabled for Queue service for read, write and delete requests"
        FAILED for resource: azurerm_storage_account.this
        File: /main.tf:1-11
        Guide: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/azure-policies/azure-logging-policies/enable-requests-on-storage-logging-for-queue-service
Check: CKV_AZURE_59: "Ensure that Storage accounts disallow public access"
        FAILED for resource: azurerm_storage_account.this
        File: /main.tf:1-11
        Guide: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/azure-policies/azure-networking-policies/ensure-that-storage-accounts-disallow-public-access
Check: CKV_AZURE_206: "Ensure that Storage Accounts use replication"
        FAILED for resource: azurerm_storage_account.this
        File: /main.tf:1-11
        Guide: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/azure-policies/azure-general-policies/azr-general-206
Check: CKV2_AZURE_40: "Ensure storage account is not configured with Shared Key authorization"
        FAILED for resource: azurerm_storage_account.this
        File: /main.tf:1-11
        Guide: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/azure-policies/azure-iam-policies/bc-azure-2-40
Check: CKV2_AZURE_41: "Ensure storage account is configured with SAS expiration policy"
        FAILED for resource: azurerm_storage_account.this
        File: /main.tf:1-11
        Guide: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/azure-policies/azure-iam-policies/bc-azure-2-41
Check: CKV2_AZURE_33: "Ensure storage account is configured with private endpoint"
        FAILED for resource: azurerm_storage_account.this
        File: /main.tf:1-11
        Guide: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/azure-policies/azure-general-policies/bc-azure-2-33
Check: CKV2_AZURE_38: "Ensure soft-delete is enabled on Azure storage account"
        FAILED for resource: azurerm_storage_account.this
        File: /main.tf:1-11
        Guide: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/azure-policies/azure-general-policies/bc-azure-2-38
Check: CKV2_AZURE_1: "Ensure storage for critical data are encrypted with Customer Managed Key"
        FAILED for resource: azurerm_storage_account.this
        File: /main.tf:1-11
        Guide: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/azure-policies/azure-general-policies/ensure-storage-for-critical-data-are-encrypted-with-customer-managed-key

Output when skipping all checks matching 'CKV2*'

[root /run/shm]# checkov -f main.tf --skip-check 'CKV2*' --compact --quiet
terraform scan results:

Passed checks: 6, Failed checks: 3, Skipped checks: 0

Check: CKV_AZURE_33: "Ensure Storage logging is enabled for Queue service for read, write and delete requests"
        FAILED for resource: azurerm_storage_account.this
        File: /main.tf:1-11
        Guide: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/azure-policies/azure-logging-policies/enable-requests-on-storage-logging-for-queue-service
Check: CKV_AZURE_59: "Ensure that Storage accounts disallow public access"
        FAILED for resource: azurerm_storage_account.this
        File: /main.tf:1-11
        Guide: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/azure-policies/azure-networking-policies/ensure-that-storage-accounts-disallow-public-access
Check: CKV_AZURE_206: "Ensure that Storage Accounts use replication"
        FAILED for resource: azurerm_storage_account.this
        File: /main.tf:1-11
        Guide: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/azure-policies/azure-general-policies/azr-general-206

The scan result for both examples shows 0 skipped checks which is misleading at least in my opinion: Passed checks: 7, Failed checks: 8, Skipped checks: 0 Passed checks: 6, Failed checks: 3, Skipped checks: 0

Adding a single line of logging to the function add_record like logging.info(record.check_result), I can see only these checks which succeeded or failed:

[root /run/shm]# LOG_LEVEL=info checkov -f main.tf --skip-check 'CKV2*' --compact --quiet
2024-02-06 18:05:33,872 [MainThread  ] [INFO ]  Checking necessary system dependancies for helm checks.
2024-02-06 18:05:33,916 [MainThread  ] [INFO ]  Found working version of helm dependancies: v3.14.0
2024-02-06 18:05:33,916 [MainThread  ] [INFO ]  Checking necessary system dependancies for kustomize checks.
2024-02-06 18:05:33,954 [MainThread  ] [INFO ]  Found working version of kustomize dependancy kubectl: 1.29
2024-02-06 18:05:34,842 [MainThread  ] [INFO ]  Scanning root folder and producing fresh tf_definitions and context
2024-02-06 18:05:34,844 [MainThread  ] [INFO ]  Custom detector found at /usr/local/lib/python3.9/site-packages/checkov/secrets/plugins/custom_regex_detector.py. Loading...
2024-02-06 18:05:34,852 [MainThread  ] [INFO ]  Creating vertices
2024-02-06 18:05:34,852 [MainThread  ] [INFO ]  [TerraformLocalGraph] created 1 vertices
2024-02-06 18:05:34,852 [MainThread  ] [INFO ]  Creating edges
2024-02-06 18:05:34,852 [MainThread  ] [INFO ]  [TerraformLocalGraph] created 0 edges
2024-02-06 18:05:34,852 [MainThread  ] [INFO ]  Rendering variables, graph has 1 vertices and 0 edges
2024-02-06 18:05:34,853 [MainThread  ] [INFO ]  Building cross variable edges
2024-02-06 18:05:34,853 [MainThread  ] [INFO ]  Found 0 cross variable edges
2024-02-06 18:05:34,858 [MainThread  ] [INFO ]  {'result': <CheckResult.PASSED: 'PASSED'>, 'evaluated_keys': ['min_tls_version']}
2024-02-06 18:05:34,858 [MainThread  ] [INFO ]  {'result': <CheckResult.PASSED: 'PASSED'>, 'evaluated_keys': ['network_rules/[0]/default_action']}
2024-02-06 18:05:34,858 [MainThread  ] [INFO ]  {'result': <CheckResult.FAILED: 'FAILED'>, 'evaluated_keys': ['account_kind']}
2024-02-06 18:05:34,858 [MainThread  ] [INFO ]  {'result': <CheckResult.FAILED: 'FAILED'>, 'evaluated_keys': ['public_network_access_enabled']}
2024-02-06 18:05:34,858 [MainThread  ] [INFO ]  {'result': <CheckResult.PASSED: 'PASSED'>, 'evaluated_keys': ['allow_nested_items_to_be_public']}
2024-02-06 18:05:34,858 [MainThread  ] [INFO ]  {'result': <CheckResult.FAILED: 'FAILED'>, 'evaluated_keys': ['account_replication_type']}
2024-02-06 18:05:34,859 [MainThread  ] [INFO ]  {'result': <CheckResult.PASSED: 'PASSED'>, 'evaluated_keys': ['name']}
2024-02-06 18:05:34,859 [MainThread  ] [INFO ]  {'result': <CheckResult.PASSED: 'PASSED'>, 'evaluated_keys': ['network_rules']}
2024-02-06 18:05:34,859 [MainThread  ] [INFO ]  {'result': <CheckResult.PASSED: 'PASSED'>, 'evaluated_keys': ['enable_https_traffic_only']}
2024-02-06 18:05:34,871 [MainThread  ] [INFO ]  file main.tf results len 0
2024-02-06 18:05:34,872 [MainThread  ] [INFO ]  Cleanup the whole temp directory: /tmp/tmpvftakfz6

Version (please complete the following information):

Additional context -/-

gruebel commented 5 months ago

The flag name was always misleading, it is more like an ignore-check flag. It will never be checked and therefore you won't find it in a report.

tberreis commented 5 months ago

Hi @gruebel, Thank you for your reply. Like documented in Reviewing Scan Results the counter increases when adding a # checkov:skip to the code directly and I don't understand, why this is handled differently in the code than adding the same exclude as a parameter. In my opinion the meaning is exactly the same: Suppress any warning for that specific rule.

This should be reworked or at least documented. Maybe it's a good idea to rename the parameter and deprecate the old one?

gruebel commented 5 months ago

it is documented https://github.com/bridgecrewio/checkov/blob/main/docs/2.Basics/Suppressing%20and%20Skipping%20Policies.md#specifying-or-skipping-checks-for-the-entire-run

You can also fine-tune which checks run or do not run for the overall scan using the --check and --skip-check flags. You can use these flags to specify check IDs (or wildcards) and / or check severities (if using the platform integration). Any skipped check will simply not run at all and will not appear in the output.

tberreis commented 5 months ago

it is documented https://github.com/bridgecrewio/checkov/blob/main/docs/2.Basics/Suppressing%20and%20Skipping%20Policies.md#specifying-or-skipping-checks-for-the-entire-run

You can also fine-tune which checks run or do not run for the overall scan using the --check and --skip-check flags. You can use these flags to specify check IDs (or wildcards) and / or check severities (if using the platform integration). Any skipped check will simply not run at all and will not appear in the output.

Thank you for pointing me in the right direction. With this discussion in mind it makes sense for me now but as you already mentioned, the implementation of the parameter is a bit misleading.

Would it be possible to use this issue as a feature request to either rename the parameter or to make it possible to include the skipped checks in the overall report? Could be an optional configuration flag or the like to not break the current usage. Or should I better create a new issue for the desired behavior while being as descriptive as possible?

In my use case I have lots of CI/CD pipelines which are consuming these skips via a Gitlab CI/CD variable and it would be great for the end users to see, that and how many rules have been skipped.

Saarett commented 5 months ago

Tagging @tsmithv11 for visibility