Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positves CKV2_AWS_67 #6294

Open
aaleksandrov opened this issue May 10, 2024 · 6 comments · May be fixed by #6434
Open

False positves CKV2_AWS_67 #6294

aaleksandrov opened this issue May 10, 2024 · 6 comments · May be fixed by #6434
Assignees
Labels
checks Check additions or changes

Comments

@aaleksandrov
Copy link

Describe the issue
CKV2_AWS_67 generates false positives when using default AWS managed encryption key (AES256)

Examples

Check: CKV2_AWS_67: "Ensure AWS S3 bucket encrypted with Customer Managed Key (CMK) has regular rotation"

	FAILED for resource: module.batch-stream-ingestion.aws_s3_bucket_server_side_encryption_configuration.sse_config

	File: /src/modules/batch-stream-ingestion/main.tf:80-88


resource "aws_s3_bucket_server_side_encryption_configuration" "sse_config" {
   bucket = aws_s3_bucket.firehose_s3_ingestion_result_bucket.bucket
     rule {
       apply_server_side_encryption_by_default {
         sse_algorithm = "AES256"
        }
     }
}

Version (please complete the following information):

  • Checkov Version: latest from master
@aaleksandrov aaleksandrov added the checks Check additions or changes label May 10, 2024
@stepintooracledba
Copy link

stepintooracledba commented May 10, 2024

This also fails on a S3 bucket where Customer Managed Key is used. CMK already has rotation enabled. False Alerts on Default and CMK Keys..

@rafaljanicki
Copy link
Contributor

And it also fails on keys that are stored in a different account

sean-nixon pushed a commit to sean-nixon/aws-cudos-framework-deployment that referenced this issue May 13, 2024
Works around apparent bug in checkov bridgecrewio/checkov#6294

The KMS key rotation is not configured on the aws_s3_bucket_server_side_encryption_configuration
resource so it does not make sense to check for it there. Key rotation is outside the scope
of this module.
iakov-aws pushed a commit to aws-samples/aws-cudos-framework-deployment that referenced this issue May 13, 2024
Works around apparent bug in checkov bridgecrewio/checkov#6294

The KMS key rotation is not configured on the aws_s3_bucket_server_side_encryption_configuration
resource so it does not make sense to check for it there. Key rotation is outside the scope
of this module.

Co-authored-by: Sean Nixon <smnixon@amazon.com>
@ubbeK
Copy link

ubbeK commented May 15, 2024

Looks like this PR is causing this: #6239

@tsmithv11
Copy link
Collaborator

tsmithv11 commented Jun 7, 2024

@aaleksandrov this check explicitly looks for CMKs not AWS managed keys which are considered less secure, so that is not a false positive. We have others like this like CKV_AWS_181

@stepintooracledba according to the docs, rotation is off by default: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/kms_key#enable_key_rotation. Can you provide documentation or a counter example? If so, we'll get it fixed.

@rafaljanicki sounds like you may be right. Do you have a counter example in TF you can share? We'll get this policy updated based on that.

@tsmithv11 tsmithv11 self-assigned this Jun 7, 2024
@aaleksandrov
Copy link
Author

this check explicitly looks for CMKs not AWS managed keys which are considered less secure, so that is not a false positive. We have others like this like CKV_AWS_181

@tsmithv11 It's quite confusing that this check basically includes 2 checks. If it's desired behavior can you at least change the message? To "Ensure AWS S3 bucket encrypted with Customer Managed Key (CMK) and it has regular rotation"

@tsmithv11
Copy link
Collaborator

@aaleksandrov no problem. I opened #6434 with this adjustment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
checks Check additions or changes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants