Check: CKV_AWS_35: "Ensure CloudTrail logs are encrypted at rest using KMS CMKs" for terraform plan if kms_key_id is a reference to kms resource

[ismailyenigul]

Describe the bug
I used this example https://github.com/cloudposse/terraform-aws-cloudtrail/tree/master/examples/complete to test cloudtrail kms encryption check.
Experiencing same issue reported at #799 scanning terraform files is working fine. It reports SUCCESS for KMS CMKs check.
but terraform plan scanning fails

Check: CKV_AWS_35: "Ensure CloudTrail logs are encrypted at rest using KMS CMKs"
	FAILED for resource: aws_cloudtrail.default
	File: /tf-formatted.json:132-150
	Guide: https://docs.bridgecrew.io/docs/logging_7

		133 |               "values": {
		134 |                 "cloud_watch_logs_group_arn": "",
		135 |                 "cloud_watch_logs_role_arn": "",
		136 |                 "enable_log_file_validation": true,
		137 |                 "enable_logging": true,
		138 |                 "event_selector": [],
		139 |                 "include_global_service_events": false,
		140 |                 "insight_selector": [],
		141 |                 "is_multi_region_trail": false,
		142 |                 "is_organization_trail": false,
		143 |                 "name": "eg-test-cloudtrail-test",
		144 |                 "s3_key_prefix": null,
		145 |                 "sns_topic_name": null,
		146 |                 "tags": {
		147 |                   "Name": "eg-test-cloudtrail-test",
		148 |                   "Namespace": "eg",
		149 |                   "Stage": "test"
		150 |                 }

Desktop (please complete the following information):

• OS: MacOS
• Checkov Version 1.0.711

Additional context
Attached failed terraform plan in json
kms-key-reference.txt

[njgibbon]

Hello @ismailyenigul ,

kms_key_arn is not part of the aws_cloudtrail terraform API. It's kms_key_id.

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudtrail#kms_key_id

Please could you test ensuring you are using the right name. If you are still seeing a failure please come back and post the entire aws_cloudtrail resource here so we can analyse it.

This could be because of interpolation of outputs. It could be because outputs are not yet known in the plan. It could be to do with your module usage or something else. But, it may just be that you are using the wrong name.

[ismailyenigul]

Hi @njgibbon
it is my bad. I typed wrong key name in the issue description. yes it is kms_key_id and it is correctly in the resource.
please check kms-key-reference.txt json plan file in this issue attachment https://github.com/bridgecrewio/checkov/files/5845422/kms-key-reference.txt

 "after_unknown": {
          "arn": true,
          "event_selector": [],
          "home_region": true,
          "id": true,
          "insight_selector": [],
          "kms_key_id": true,
          "s3_bucket_name": true,
          "tags": {}
        }
      }
      ```

[njgibbon]

@ismailyenigul ah okey fair enough. Thanks for updating the description. In that case we will have to look further into the issue 🙂

[njgibbon]

@ismailyenigul Also, thanks for the plan output. Really useful! I'm not properly looking in to it now but I have just come across this recently raised related issue which could help understand what's happening here hashicorp/terraform-provider-aws#11823

[schosterbarak]

@ismailyenigul so we need to add the "after_unknown" properties into the resource analysis. is that correct?

[ismailyenigul]

Hi @schosterbarak
yes if you see a value :) and if there is no another way to handle this reference?
I just re-read the codes, it seems that if we follow the references, that seems also works fine.

"kms_key_id": {
                    "references": [
                      "var.kms_key_arn"
                    ]
                  },

and

"kms_key_arn": {
              "references": [
                "aws_kms_key.key[\"default\"]"
              ]
            },

but it seeems checking in after_unknown block is the easiest

        "after_unknown": {
          "arn": true,
          "event_selector": [],
          "home_region": true,
          "id": true,
          "insight_selector": [],
          "kms_key_id": true,
          "s3_bucket_name": true,
          "tags": {}
        }

[schosterbarak]

i'll research more on the best way to handle this.

[njgibbon]

Just done some analysis on this:

When we parse through a plan we are looking at the planned_values part. And this captures almost everything we want to analyse.

https://github.com/bridgecrewio/checkov/blob/master/checkov/terraform/plan_parser.py#L98

See https://www.terraform.io/docs/internals/json-format.html#plan-representation

  // "planned_values" is a description of what is known so far of the outcome in
  // the standard value representation, with any as-yet-unknown values omitted.
  "planned_values": <values-representation>,

Note the "with any as-yet-unknown values omitted.". Which captures the case being reported here and the linked hashicorp issue where they are using a kms key that has just been created in the same run.

In this case, we appear to need look under resource_changes (as things stand with the tf json plan output right now).

Then we can identify the thing we want to scan by the type field and then look at change.after_unknown where we can see what we want to see.

I have linked the whole body of this item so you can clearly see what's going on.

    {
      "address": "module.cloudtrail.aws_cloudtrail.default[0]",
      "module_address": "module.cloudtrail",
      "mode": "managed",
      "type": "aws_cloudtrail",
      "name": "default",
      "index": 0,
      "provider_name": "aws",
      "change": {
        "actions": [
          "create"
        ],
        "before": null,
        "after": {
          "cloud_watch_logs_group_arn": "",
          "cloud_watch_logs_role_arn": "",
          "enable_log_file_validation": true,
          "enable_logging": true,
          "event_selector": [],
          "include_global_service_events": false,
          "insight_selector": [],
          "is_multi_region_trail": false,
          "is_organization_trail": false,
          "name": "eg-test-cloudtrail-test",
          "s3_key_prefix": null,
          "sns_topic_name": null,
          "tags": {
            "Name": "eg-test-cloudtrail-test",
            "Namespace": "eg",
            "Stage": "test"
          }
        },
        "after_unknown": {
          "arn": true,
          "event_selector": [],
          "home_region": true,
          "id": true,
          "insight_selector": [],
          "kms_key_id": true,
          "s3_bucket_name": true,
          "tags": {}
        }
      }
    }

If we check here we can find what we want. We do not want false negatives or duplicate scans so we will need logic that takes this in to account and adds the after_unknown values in with the other values for any resource.

Importantly. the resource_changes bit only records things that are changing and not configuration which stays the same. And so we do still need to look in planned_values to ensure that resources which aren't changing are still properly regression scanned.

[schosterbarak]

thanks for the analysis @njgibbon . I miss something "s3_bucket_name" appears in "after_unknown" as boolean value, i was expecting a string. is there further anlsysis to be done to get the string / object value of it?

[ismailyenigul]

@schosterbarak I think all values in after_unknown are boolean :)

[schosterbarak]

so what would the policy check in that case?

[ismailyenigul]

If "kms_key_id" true or not
I will check kms_key_id in after_unknown if it is not configured.

[schosterbarak]

does it work the same for object elements? like server_side_encryption_configuration in s3?

[njgibbon]

@schosterbarak - Yeah, I think some more understanding may be needed. It looks as if the Booleans just mean that "yes, this will have a value", but it doesn't tell us about what the value will be yet.

In the case of a kms_key_id for cloudtrail this would pass the check because we just want any value because any valid value indicates that a key is being used for encryption.

But you could imagine other scenrios where it would cause the wrong result.

So, the whole thing needs to be thought about and understood a bit more before jumping into a change.

[ismailyenigul]

First I have to check s3 case.
Here is findings.

1. kms_key_id set directly (without any reference to output of a resource).
   like

kms_key_id                   = "arn:aws:kms:us-east-2:123456789012:key/12345678-1234-1234-1234-12345678901"

So if it is in planned_values no need to make another check. Nothing about kms_key_id in `after_unknow

   "planned_values": {
    "outputs": {
...
  "kms_key_id": "arn:aws:kms:us-east-2:123456789012:key/12345678-1234-1234-1234-12345678901",

2. kms_key_id is output of a resource.

 kms_key_id = aws_kms_key.key["default"].arn 

nothing in planned but "kms_key_id": true in after_unknown

3. no kms_key_id specified
   kms_key_id value is "" in planned_values

"kms_key_id": "",

Because default value is "" at
https://github.com/cloudposse/terraform-aws-cloudtrail/blob/master/variables.tf#L60

4. no kms_key_id passed to resource "aws_cloudtrail"

I got "kms_key_id": null in planned_values and nothing in after_unknown

[schosterbarak]

and what about AES encryption (which is an object and not a string) ?

[ismailyenigul]

About s3_bucket_name and AES

 "after_unknown": {
 323           "arn": true,
 324           "event_selector": [],
 325           "home_region": true,
 326           "id": true,
 327           "insight_selector": [],
 328           "kms_key_id": true,
 329           "s3_bucket_name": true,
 330           "tags": {}
 331         }

Again it is reference to output of another module
s3_bucket_name = module.cloudtrail_s3_bucket.bucket_id

and sse is set in s3_bucket module nothing in after_unknown

   "object_lock_configuration": [],
 385           "policy": "{\n  \"Version\": \"2012-10-17\",\n  \"Statement\": [\n    {\n      \"Sid\": \"AWSCloudTrailAclCheck\",\n      \"Effect\": \"Allow\",\n      \"Action\": \"s3:GetBucketAcl\",\n      \"Resource\": \"arn:aws:s3:::eg
     -test-cloudtrail-test\",\n      \"Principal\": {\n        \"Service\": \"cloudtrail.amazonaws.com\"\n      }\n    },\n    {\n      \"Sid\": \"AWSCloudTrailWrite\",\n      \"Effect\": \"Allow\",\n      \"Action\": \"s3:PutObject\",\n 
          \"Resource\": \"arn:aws:s3:::eg-test-cloudtrail-test/*\",\n      \"Principal\": {\n        \"Service\": [\n          \"config.amazonaws.com\",\n          \"cloudtrail.amazonaws.com\"\n        ]\n      },\n      \"Condition\": {\
     n        \"StringEquals\": {\n          \"s3:x-amz-acl\": \"bucket-owner-full-control\"\n        }\n      }\n    }\n  ]\n}",
 386           "replication_configuration": [],
 387           "server_side_encryption_configuration": [
 388             {
 389               "rule": [
 390                 {
 391                   "apply_server_side_encryption_by_default": [
 392                     {
 393                       "kms_master_key_id": "",
 394                       "sse_algorithm": "AES256"
 395                     }
 396                   ]

btw, kms_master_key_arn is set "" by default at
https://github.com/cloudposse/terraform-aws-cloudtrail-s3-bucket/blob/master/variables.tf#L95

[metahertz]

Seems #799 is related with the after_unknown references for kms_master_key_id

[stale]

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]

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!