(sns-subscriptions): add SourceArn condition to CMK policy when subscribing SQS to SNS

HansFalkenberg-Visma commented 2 years ago

Describe the feature

When topic.addSubscription is used with an SqsSubscription referencing a queue with SSE encryption enabled, the queue's CMK policy is extended to allow the SNS service principal for all accounts. This was implemented in https://github.com/aws/aws-cdk/pull/14960

The generated statement should include a condition to restrict usage to the specific topic, ie.

            "Condition": {
                "ArnEquals": {
                    "aws:SourceArn": "arn:aws:sns:REGION:ACCOUNT:TOPIC"
                }
            }

Use Case

To help prevent an AWS service from being used as a confused deputy in a policy where the principal is an AWS service principal, you can use the aws:SourceArn or aws:SourceAccount global condition keys.

https://docs.aws.amazon.com/kms/latest/developerguide/policy-conditions.html

This was apparently not possible before. While this is a security improvement, I do not believe it rates reporting as a security vulnerability, because

it is well known that until recently the KMS did not even support this, ref comments on https://github.com/aws/aws-cdk/pull/12146
and indeed not all documentation has gotten around to recommend it, ref step 1 at https://docs.aws.amazon.com/sns/latest/dg/sns-enable-encryption-for-topic-sqs-queue-subscriptions.html
I don't know of any practical attack (it would be some kind of a confused deputy attack, of course, but I can't currently imagine how)

Proposed Solution

The implementation itself should be trivial because the necessary code is already in https://github.com/aws/aws-cdk/blob/v2-main/packages/%40aws-cdk/aws-sns-subscriptions/lib/sqs.ts for the queue's policy.

Simply add three lines to

    if (this.queue.encryptionMasterKey) {
      this.queue.encryptionMasterKey.addToResourcePolicy(new iam.PolicyStatement({
        resources: ['*'],
        actions: ['kms:Decrypt', 'kms:GenerateDataKey'],
        principals: [snsServicePrincipal],
      }));
    }

like so

    if (this.queue.encryptionMasterKey) {
      this.queue.encryptionMasterKey.addToResourcePolicy(new iam.PolicyStatement({
        resources: ['*'],
        actions: ['kms:Decrypt', 'kms:GenerateDataKey'],
        principals: [snsServicePrincipal],
        conditions: {
          ArnEquals: { 'aws:SourceArn': topic.topicArn },
        },
      }));
    }

I don't know that a workaround is currently possible. I tried adding the more restrictive policy (with the condition) to the CMK first, but CDK will always add the more permissive policy (without the condition) with no (easily available) interfaces for removing it again.

Other Information

This would not be a breaking change to the CDK API, but the generated policy would be more restrictive. Solutions that have -- erroneously, I would say -- depended on the too permissive policy for use by SNS topics not managed by the same CDK application/stack might have issues. They have the simple fix of explicitly adding back the permissive policy statement to the CMK.

The following is the exact statement that was generated before the proposed change. After the change it would be replaced by a more restrictive statement, one per topic (with each topic's ARN as a condition). This can of course be used as is, but it might be just as easy to add statements with ARN conditions for the other topics too:

        {
            "Effect": "Allow",
            "Principal": {
                "Service": "sns.amazonaws.com"
            },
            "Action": [
                "kms:Decrypt",
                "kms:GenerateDataKey"
            ],
            "Resource": "*"
        }

Acknowledgements

[ ] I may be able to implement this feature request
[ ] This feature might incur a breaking change

CDK version used

v2.24.1

Environment details (OS name and version, etc.)

n/a

daschaa commented 2 years ago

With #20521 now implemented, can this be closed?

tanyalava commented 2 years ago

Just like snsServicePrincipal is used to access the service sns.amazonaws.com, is there a principal to access s3.amazonaws.com ?

khushail commented 4 months ago

@HansFalkenberg-Visma , since @daschaa has provided the Fix and PR is merged, could I close this issue ?

HansFalkenberg-Visma commented 4 months ago

Yes, with my cdk.json looking like this it now adds the expected condition to the key's policy:

{
  "app": "npx ts-node --prefer-ts-exts cdk/bin/aws.ts",
  "context": {
    "@aws-cdk/aws-sns-subscriptions:restrictSqsDescryption": true
  }
}

I assume the s in Descryption is a typo, but nothing to do about that now.

pahud commented 4 days ago

Hi @HansFalkenberg-Visma Is this feature request still relevant? I guess we can close it now?

github-actions[bot] commented 4 days ago

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

aws / aws-cdk