feat(cloudformation): Adding Sse property to SQS Encryption check

[bakosa]

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Description

This PR enhances the CKV_AWS_27 check to handle the SqsManagedSseEnabled property.

Note that AWS by default now enables SSE to newly created SQS queues. However, this check will still make sure queues are explicitly setting ether KMS or SSE since existing queues won't be encrypted.

I was not sure if we should use a yaml graph based check for this, but I thought re-defining an entire check would be a bit much/have unintended consequences.

Another thing to note is like any BaseResourceValueCheck this check won't really handle unknowns if the template is not valid.

Fixes # (5869)

Description

Read above

Fix

How does someone fix the issue in code and/or in runtime?
Pass KMS Key ID to encrypt via KmsMasterKeyId (which will set SSE to false) or set the SqsManagedSseEnabled to true

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my feature, policy, or fix is effective and works
• New and existing tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[bakosa]

Opened PR for this already #5870

[JamesWoolfenden]

You need to check what types those properties are, they should be a string and a boolean but they might not be. Other it looks perfect!

[bakosa]

You need to check what types those properties are, they should be a string and a boolean but they might not be. Other it looks perfect!

Thanks James, I was going to do just that except I was curious what the stance is here in general. If SqsManagedSseEnabled is not a bool and KmsMasterKeyId is not a string then the resource is not even valid. The BaseResourceValueCheck class that this check is currently inheriting from seems like it does not currently check if the KmsMasterKeyId is a string/dict/bool/int.. etc or not. I believe if it has ANY value/type except an empty string, it passes"

checkov/checkov/cloudformation/checks/resource/base_resource_value_check.py

Line 72 in 2b58230

if ANY_VALUE in expected_values and value is not None and (not isinstance(value, str) or value):

That being said, I will make the updates to check the types and return CheckResult.UNKNOWN if those are not as expected?

[JamesWoolfenden]

You need to check what types those properties are, they should be a string and a boolean but they might not be. Other it looks perfect!

Thanks James, I was going to do just that except I was curious what the stance is here in general. If SqsManagedSseEnabled is not a bool and KmsMasterKeyId is not a string then the resource is not even valid. The BaseResourceValueCheck class that this check is currently inheriting from seems like it does not currently check if the KmsMasterKeyId is a string/dict/bool/int.. etc or not. I believe if it has ANY value/type except an empty string, it passes"

checkov/checkov/cloudformation/checks/resource/base_resource_value_check.py

Line 72 in 2b58230

if ANY_VALUE in expected_values and value is not None and (not isinstance(value, str) or value):

That being said, I will make the updates to check the types and return CheckResult.UNKNOWN if those are not as expected?
Yeah id also change the base class BaseResourceCheck and if it fails to parse the fields then its CheckResult.UNKNOWN and the annotations for scan e.g. ) -> CheckResult: and init -> None:, and maybe a test case where someone passes something thats not a bool for SqsManagedSseEnabled.

[bakosa]

You need to check what types those properties are, they should be a string and a boolean but they might not be. Other it looks perfect!

Thanks James, I was going to do just that except I was curious what the stance is here in general. If SqsManagedSseEnabled is not a bool and KmsMasterKeyId is not a string then the resource is not even valid. The BaseResourceValueCheck class that this check is currently inheriting from seems like it does not currently check if the KmsMasterKeyId is a string/dict/bool/int.. etc or not. I believe if it has ANY value/type except an empty string, it passes"

checkov/checkov/cloudformation/checks/resource/base_resource_value_check.py

Line 72 in 2b58230

if ANY_VALUE in expected_values and value is not None and (not isinstance(value, str) or value):

That being said, I will make the updates to check the types and return CheckResult.UNKNOWN if those are not as expected?
Yeah id also change the base class BaseResourceCheck and if it fails to parse the fields then its CheckResult.UNKNOWN and the annotations for scan e.g. ) -> CheckResult: and init -> None:, and maybe a test case where someone passes something thats not a bool for SqsManagedSseEnabled.

The more I think about this the more I feel like checking for type is inconsistent from what is being done in the terraform check for this same check ID. If I check for whether these properties get a specific type, aren't we just validating the CFT/resource at that point? I don't feel like that makes a whole lot of sense based on the majority of the other checks in here.

[JamesWoolfenden]

if you don't check the types, this valid CFN will pass your check when it should fail:

AWSTemplateFormatVersion: "2010-09-09"
Resources:
  MySourceQueue:
    Type: AWS::SQS::Queue
    Properties:
      RedrivePolicy:
        deadLetterTargetArn: "example_arn"
        maxReceiveCount: 5
      SqsManagedSseEnabled: "false"

Also i think because the default is now to auto enable SSE your case test_SQSQueueEncryption-FAILED2.yml should now be a pass.

[Saarett]

Hi @bakosa, are you planning to finish the work on this PR? Your efforts are greatly appreciated.

[bakosa]

Hi @bakosa, are you planning to finish the work on this PR? Your efforts are greatly appreciated.

Yeah, I can take a look this week. I just opened another issue curious thought there too!

[bakosa]

Finally got back to this and pushed updates suggested by @JamesWoolfenden