From a36f2f0d3e71e0b2467812a9d93d4b0b26629a60 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Wed, 2 Nov 2022 18:35:33 +0000 Subject: [PATCH] fix(events-targets): encrypted queues get too wide permissions (under feature flag) (#22740) When the target of an `event.Rule` is an encrypted `sqs.Queue`, we give the queue a resource policy that allows the service principal 'events.amazonaws.com' to send messages from any account. This was done to avoid circular dependencies when trying to set permissions only to the rule itself. But this decision ended up making the queue vulnerable to attacks from other accounts. Change the policy, restricting to requests coming from the same account. To avoid a breaking change for users who may already be relying on the permissive policy, this feature has to be enabled by a feature flag. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../aws-events-targets/lib/log-group.ts | 2 +- .../@aws-cdk/aws-events-targets/lib/sqs.ts | 21 ++- .../@aws-cdk/aws-events-targets/package.json | 2 + .../test/logs/log-group.test.ts | 2 +- .../aws-cdk-sqs-event-target.assets.json | 6 +- .../aws-cdk-sqs-event-target.template.json | 14 ++ .../cdk.out | 2 +- .../integ.json | 2 +- .../manifest.json | 4 +- .../tree.json | 24 +++- .../aws-events-targets/test/sqs/sqs.test.ts | 120 ++++++++++++++++++ packages/@aws-cdk/cx-api/lib/features.ts | 8 ++ 12 files changed, 187 insertions(+), 20 deletions(-) diff --git a/packages/@aws-cdk/aws-events-targets/lib/log-group.ts b/packages/@aws-cdk/aws-events-targets/lib/log-group.ts index b12cfc849c69d..691bcb09f1755 100644 --- a/packages/@aws-cdk/aws-events-targets/lib/log-group.ts +++ b/packages/@aws-cdk/aws-events-targets/lib/log-group.ts @@ -1,11 +1,11 @@ import * as events from '@aws-cdk/aws-events'; +import { RuleTargetInputProperties, RuleTargetInput, EventField, IRule } from '@aws-cdk/aws-events'; import * as iam from '@aws-cdk/aws-iam'; import * as logs from '@aws-cdk/aws-logs'; import * as cdk from '@aws-cdk/core'; import { ArnFormat, Stack } from '@aws-cdk/core'; import { LogGroupResourcePolicy } from './log-group-resource-policy'; import { TargetBaseProps, bindBaseTargetConfig } from './util'; -import { RuleTargetInputProperties, RuleTargetInput, EventField, IRule } from '@aws-cdk/aws-events'; /** * Options used when creating a target input template diff --git a/packages/@aws-cdk/aws-events-targets/lib/sqs.ts b/packages/@aws-cdk/aws-events-targets/lib/sqs.ts index 465f0355516cf..7b8ce31e92a75 100644 --- a/packages/@aws-cdk/aws-events-targets/lib/sqs.ts +++ b/packages/@aws-cdk/aws-events-targets/lib/sqs.ts @@ -1,6 +1,8 @@ import * as events from '@aws-cdk/aws-events'; import * as iam from '@aws-cdk/aws-iam'; import * as sqs from '@aws-cdk/aws-sqs'; +import { Aws, FeatureFlags } from '@aws-cdk/core'; +import * as cxapi from '@aws-cdk/cx-api'; import { addToDeadLetterQueueResourcePolicy, TargetBaseProps, bindBaseTargetConfig } from './util'; /** @@ -52,15 +54,22 @@ export class SqsQueue implements events.IRuleTarget { * @see https://docs.aws.amazon.com/eventbridge/latest/userguide/resource-based-policies-eventbridge.html#sqs-permissions */ public bind(rule: events.IRule, _id?: string): events.RuleTargetConfig { - // Only add the rule as a condition if the queue is not encrypted, to avoid circular dependency. See issue #11158. - const principalOpts = this.queue.encryptionMasterKey ? {} : { - conditions: { + const restrictToSameAccount = FeatureFlags.of(rule).isEnabled(cxapi.EVENTS_TARGET_QUEUE_SAME_ACCOUNT); + + let conditions: any = {}; + if (!this.queue.encryptionMasterKey) { + conditions = { ArnEquals: { 'aws:SourceArn': rule.ruleArn }, - }, - }; + }; + } else if (restrictToSameAccount) { + // Aadd only the account id as a condition, to avoid circular dependency. See issue #11158. + conditions = { + StringEquals: { 'aws:SourceAccount': Aws.ACCOUNT_ID }, + }; + } // deduplicated automatically - this.queue.grantSendMessages(new iam.ServicePrincipal('events.amazonaws.com', principalOpts)); + this.queue.grantSendMessages(new iam.ServicePrincipal('events.amazonaws.com', { conditions })); if (this.props.deadLetterQueue) { addToDeadLetterQueueResourcePolicy(rule, this.props.deadLetterQueue); diff --git a/packages/@aws-cdk/aws-events-targets/package.json b/packages/@aws-cdk/aws-events-targets/package.json index 70c65b6b65abc..c78f874dfdf35 100644 --- a/packages/@aws-cdk/aws-events-targets/package.json +++ b/packages/@aws-cdk/aws-events-targets/package.json @@ -98,6 +98,7 @@ "@aws-cdk/aws-autoscaling": "0.0.0", "@aws-cdk/aws-codebuild": "0.0.0", "@aws-cdk/aws-codepipeline": "0.0.0", + "@aws-cdk/cx-api": "0.0.0", "@aws-cdk/aws-ec2": "0.0.0", "@aws-cdk/aws-ecs": "0.0.0", "@aws-cdk/aws-events": "0.0.0", @@ -121,6 +122,7 @@ "@aws-cdk/aws-autoscaling": "0.0.0", "@aws-cdk/aws-codebuild": "0.0.0", "@aws-cdk/aws-codepipeline": "0.0.0", + "@aws-cdk/cx-api": "0.0.0", "@aws-cdk/aws-ec2": "0.0.0", "@aws-cdk/aws-ecs": "0.0.0", "@aws-cdk/aws-events": "0.0.0", diff --git a/packages/@aws-cdk/aws-events-targets/test/logs/log-group.test.ts b/packages/@aws-cdk/aws-events-targets/test/logs/log-group.test.ts index c38d846882296..3b71fff8e2ed0 100644 --- a/packages/@aws-cdk/aws-events-targets/test/logs/log-group.test.ts +++ b/packages/@aws-cdk/aws-events-targets/test/logs/log-group.test.ts @@ -2,10 +2,10 @@ import { Template } from '@aws-cdk/assertions'; import * as events from '@aws-cdk/aws-events'; import * as logs from '@aws-cdk/aws-logs'; import * as sqs from '@aws-cdk/aws-sqs'; +import { testDeprecated } from '@aws-cdk/cdk-build-tools'; import * as cdk from '@aws-cdk/core'; import * as targets from '../../lib'; import { LogGroupTargetInput } from '../../lib'; -import { testDeprecated } from '@aws-cdk/cdk-build-tools'; test('use log group as an event rule target', () => { // GIVEN diff --git a/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.assets.json b/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.assets.json index db13f0a93c7ec..0c9220eddf43f 100644 --- a/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.assets.json +++ b/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.assets.json @@ -1,7 +1,7 @@ { - "version": "20.0.0", + "version": "21.0.0", "files": { - "e18a99f507ccee95a180ba3b3e0df1a514804ce9399b167dd95db86457e1e650": { + "2dc225f2fbae904e5a29adf2e7d3d70b01c10760a4fb779d1a447bda79b33e1d": { "source": { "path": "aws-cdk-sqs-event-target.template.json", "packaging": "file" @@ -9,7 +9,7 @@ "destinations": { "current_account-current_region": { "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}", - "objectKey": "e18a99f507ccee95a180ba3b3e0df1a514804ce9399b167dd95db86457e1e650.json", + "objectKey": "2dc225f2fbae904e5a29adf2e7d3d70b01c10760a4fb779d1a447bda79b33e1d.json", "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}" } } diff --git a/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.template.json b/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.template.json index 11bf4d3d3c49d..7bfa29e5df27d 100644 --- a/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.template.json +++ b/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.template.json @@ -35,6 +35,13 @@ "kms:GenerateDataKey*", "kms:ReEncrypt*" ], + "Condition": { + "StringEquals": { + "aws:SourceAccount": { + "Ref": "AWS::AccountId" + } + } + }, "Effect": "Allow", "Principal": { "Service": "events.amazonaws.com" @@ -98,6 +105,13 @@ "sqs:GetQueueUrl", "sqs:SendMessage" ], + "Condition": { + "StringEquals": { + "aws:SourceAccount": { + "Ref": "AWS::AccountId" + } + } + }, "Effect": "Allow", "Principal": { "Service": "events.amazonaws.com" diff --git a/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/cdk.out b/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/cdk.out index 588d7b269d34f..8ecc185e9dbee 100644 --- a/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/cdk.out +++ b/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/cdk.out @@ -1 +1 @@ -{"version":"20.0.0"} \ No newline at end of file +{"version":"21.0.0"} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/integ.json b/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/integ.json index 3ceac3ded4fcc..55138f5528af5 100644 --- a/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/integ.json +++ b/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/integ.json @@ -1,5 +1,5 @@ { - "version": "20.0.0", + "version": "21.0.0", "testCases": { "integ.sqs-event-rule-target": { "stacks": [ diff --git a/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/manifest.json b/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/manifest.json index a55d105dd020c..ff23d69bce1c7 100644 --- a/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/manifest.json +++ b/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/manifest.json @@ -1,5 +1,5 @@ { - "version": "20.0.0", + "version": "21.0.0", "artifacts": { "Tree": { "type": "cdk:tree", @@ -23,7 +23,7 @@ "validateOnSynth": false, "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}", "cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}", - "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/e18a99f507ccee95a180ba3b3e0df1a514804ce9399b167dd95db86457e1e650.json", + "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/2dc225f2fbae904e5a29adf2e7d3d70b01c10760a4fb779d1a447bda79b33e1d.json", "requiresBootstrapStackVersion": 6, "bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version", "additionalDependencies": [ diff --git a/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/tree.json b/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/tree.json index 2ef0f9efd491f..756cd84861370 100644 --- a/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/tree.json +++ b/packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/tree.json @@ -9,7 +9,7 @@ "path": "Tree", "constructInfo": { "fqn": "constructs.Construct", - "version": "10.1.85" + "version": "10.1.140" } }, "aws-cdk-sqs-event-target": { @@ -58,6 +58,13 @@ "kms:GenerateDataKey*", "kms:ReEncrypt*" ], + "Condition": { + "StringEquals": { + "aws:SourceAccount": { + "Ref": "AWS::AccountId" + } + } + }, "Effect": "Allow", "Principal": { "Service": "events.amazonaws.com" @@ -165,6 +172,13 @@ "sqs:GetQueueUrl", "sqs:SendMessage" ], + "Condition": { + "StringEquals": { + "aws:SourceAccount": { + "Ref": "AWS::AccountId" + } + } + }, "Effect": "Allow", "Principal": { "Service": "events.amazonaws.com" @@ -284,14 +298,14 @@ } }, "constructInfo": { - "fqn": "constructs.Construct", - "version": "10.1.85" + "fqn": "@aws-cdk/core.Stack", + "version": "0.0.0" } } }, "constructInfo": { - "fqn": "constructs.Construct", - "version": "10.1.85" + "fqn": "@aws-cdk/core.App", + "version": "0.0.0" } } } \ No newline at end of file diff --git a/packages/@aws-cdk/aws-events-targets/test/sqs/sqs.test.ts b/packages/@aws-cdk/aws-events-targets/test/sqs/sqs.test.ts index b46dcacbcc68f..d6846f81b2b59 100644 --- a/packages/@aws-cdk/aws-events-targets/test/sqs/sqs.test.ts +++ b/packages/@aws-cdk/aws-events-targets/test/sqs/sqs.test.ts @@ -1,7 +1,9 @@ import { Template } from '@aws-cdk/assertions'; import * as events from '@aws-cdk/aws-events'; +import * as kms from '@aws-cdk/aws-kms'; import * as sqs from '@aws-cdk/aws-sqs'; import { Duration, Stack } from '@aws-cdk/core'; +import * as cxapi from '@aws-cdk/cx-api'; import * as targets from '../../lib'; test('sqs queue as an event rule target', () => { @@ -141,6 +143,124 @@ test('multiple uses of a queue as a target results in multi policy statement bec }); }); +test('Encrypted queues result in a policy statement with aws:sourceAccount condition when the feature flag is on', () => { + // GIVEN + const stack = new Stack(); + stack.node.setContext(cxapi.EVENTS_TARGET_QUEUE_SAME_ACCOUNT, true); + const queue = new sqs.Queue(stack, 'MyQueue', { + encryptionMasterKey: kms.Key.fromKeyArn(stack, 'key', 'arn:aws:kms:us-west-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab'), + }); + + const rule = new events.Rule(stack, 'MyRule', { + schedule: events.Schedule.rate(Duration.hours(1)), + }); + + // WHEN + rule.addTarget(new targets.SqsQueue(queue)); + + // THEN + Template.fromStack(stack).hasResourceProperties('AWS::SQS::QueuePolicy', { + PolicyDocument: { + Statement: [ + { + Action: [ + 'sqs:SendMessage', + 'sqs:GetQueueAttributes', + 'sqs:GetQueueUrl', + ], + Condition: { + StringEquals: { + 'aws:SourceAccount': { Ref: 'AWS::AccountId' }, + }, + }, + Effect: 'Allow', + Principal: { Service: 'events.amazonaws.com' }, + Resource: { + 'Fn::GetAtt': [ + 'MyQueueE6CA6235', + 'Arn', + ], + }, + }, + ], + Version: '2012-10-17', + }, + Queues: [{ Ref: 'MyQueueE6CA6235' }], + }); + + Template.fromStack(stack).hasResourceProperties('AWS::Events::Rule', { + ScheduleExpression: 'rate(1 hour)', + State: 'ENABLED', + Targets: [ + { + Arn: { + 'Fn::GetAtt': [ + 'MyQueueE6CA6235', + 'Arn', + ], + }, + Id: 'Target0', + }, + ], + }); +}); + +test('Encrypted queues result in a permissive policy statement when the feature flag is off', () => { + // GIVEN + const stack = new Stack(); + const queue = new sqs.Queue(stack, 'MyQueue', { + encryptionMasterKey: kms.Key.fromKeyArn(stack, 'key', 'arn:aws:kms:us-west-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab'), + }); + + const rule = new events.Rule(stack, 'MyRule', { + schedule: events.Schedule.rate(Duration.hours(1)), + }); + + // WHEN + rule.addTarget(new targets.SqsQueue(queue)); + + // THEN + Template.fromStack(stack).hasResourceProperties('AWS::SQS::QueuePolicy', { + PolicyDocument: { + Statement: [ + { + Action: [ + 'sqs:SendMessage', + 'sqs:GetQueueAttributes', + 'sqs:GetQueueUrl', + ], + Effect: 'Allow', + Principal: { Service: 'events.amazonaws.com' }, + Resource: { + 'Fn::GetAtt': [ + 'MyQueueE6CA6235', + 'Arn', + ], + }, + }, + ], + Version: '2012-10-17', + }, + Queues: [{ Ref: 'MyQueueE6CA6235' }], + }); + + Template.fromStack(stack).hasResourceProperties('AWS::Events::Rule', { + ScheduleExpression: 'rate(1 hour)', + State: 'ENABLED', + Targets: [ + { + Arn: { + 'Fn::GetAtt': [ + 'MyQueueE6CA6235', + 'Arn', + ], + }, + Id: 'Target0', + }, + ], + }); +}); + test('fail if messageGroupId is specified on non-fifo queues', () => { const stack = new Stack(); const queue = new sqs.Queue(stack, 'MyQueue'); diff --git a/packages/@aws-cdk/cx-api/lib/features.ts b/packages/@aws-cdk/cx-api/lib/features.ts index e991c1e285b45..5f4d6980dc522 100644 --- a/packages/@aws-cdk/cx-api/lib/features.ts +++ b/packages/@aws-cdk/cx-api/lib/features.ts @@ -352,6 +352,13 @@ export const APIGATEWAY_DISABLE_CLOUDWATCH_ROLE = '@aws-cdk/aws-apigateway:disab */ export const ENABLE_PARTITION_LITERALS = '@aws-cdk/core:enablePartitionLiterals'; +/** + * This flag applies to SQS Queues that are used as the target of event Rules. When enabled, only principals + * from the same account as the Queue can send messages. If a queue is unencrypted, this restriction will + * always apply, regardless of the value of this flag. + */ +export const EVENTS_TARGET_QUEUE_SAME_ACCOUNT = '@aws-cdk/aws-events:eventsTargetQueueSameAccount'; + /** * Flag values that should apply for new projects * @@ -387,6 +394,7 @@ export const FUTURE_FLAGS: { [key: string]: boolean } = { [SNS_SUBSCRIPTIONS_SQS_DECRYPTION_POLICY]: true, [APIGATEWAY_DISABLE_CLOUDWATCH_ROLE]: true, [ENABLE_PARTITION_LITERALS]: true, + [EVENTS_TARGET_QUEUE_SAME_ACCOUNT]: true, }; /**