From 0083256d2329e6195c96a45589079f678b67a184 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Fri, 4 Nov 2022 14:25:06 +0000 Subject: [PATCH] fix(events-targets): policy restricts access to the same account as the Queue, not the Rule (#22766) When restricting access to encrypted queues, we should use the account of the Rule, instead of the account of the Queue itself. ---- ### All Submissions: * [ ] 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-cdk/aws-events-targets/lib/sqs.ts | 6 +-- .../aws-events-targets/test/sqs/sqs.test.ts | 52 +++++++++---------- packages/@aws-cdk/cx-api/lib/features.ts | 2 +- 3 files changed, 29 insertions(+), 31 deletions(-) diff --git a/packages/@aws-cdk/aws-events-targets/lib/sqs.ts b/packages/@aws-cdk/aws-events-targets/lib/sqs.ts index 7b8ce31e92a75..680fe381ec9aa 100644 --- a/packages/@aws-cdk/aws-events-targets/lib/sqs.ts +++ b/packages/@aws-cdk/aws-events-targets/lib/sqs.ts @@ -1,7 +1,7 @@ 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 { FeatureFlags } from '@aws-cdk/core'; import * as cxapi from '@aws-cdk/cx-api'; import { addToDeadLetterQueueResourcePolicy, TargetBaseProps, bindBaseTargetConfig } from './util'; @@ -62,9 +62,9 @@ export class SqsQueue implements events.IRuleTarget { ArnEquals: { 'aws:SourceArn': rule.ruleArn }, }; } else if (restrictToSameAccount) { - // Aadd only the account id as a condition, to avoid circular dependency. See issue #11158. + // Add only the account id as a condition, to avoid circular dependency. See issue #11158. conditions = { - StringEquals: { 'aws:SourceAccount': Aws.ACCOUNT_ID }, + StringEquals: { 'aws:SourceAccount': rule.env.account }, }; } 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 d6846f81b2b59..e0e848e182d36 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,8 +1,8 @@ -import { Template } from '@aws-cdk/assertions'; +import { Match, 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 { App, Duration, Stack } from '@aws-cdk/core'; import * as cxapi from '@aws-cdk/cx-api'; import * as targets from '../../lib'; @@ -144,24 +144,38 @@ 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', () => { + const app = new App(); // 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 ruleStack = new Stack(app, 'ruleStack', { + env: { + account: '111111111111', + region: 'us-east-1', + }, }); + ruleStack.node.setContext(cxapi.EVENTS_TARGET_QUEUE_SAME_ACCOUNT, true); - const rule = new events.Rule(stack, 'MyRule', { + const rule = new events.Rule(ruleStack, 'MyRule', { schedule: events.Schedule.rate(Duration.hours(1)), }); + const queueStack = new Stack(app, 'queueStack', { + env: { + account: '222222222222', + region: 'us-east-1', + }, + }); + const queue = new sqs.Queue(queueStack, 'MyQueue', { + encryptionMasterKey: kms.Key.fromKeyArn(queueStack, 'key', 'arn:aws:kms:us-west-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab'), + }); + + // WHEN rule.addTarget(new targets.SqsQueue(queue)); // THEN - Template.fromStack(stack).hasResourceProperties('AWS::SQS::QueuePolicy', { + Template.fromStack(queueStack).hasResourceProperties('AWS::SQS::QueuePolicy', { PolicyDocument: { - Statement: [ + Statement: Match.arrayWith([ { Action: [ 'sqs:SendMessage', @@ -170,7 +184,7 @@ test('Encrypted queues result in a policy statement with aws:sourceAccount condi ], Condition: { StringEquals: { - 'aws:SourceAccount': { Ref: 'AWS::AccountId' }, + 'aws:SourceAccount': '111111111111', }, }, Effect: 'Allow', @@ -182,27 +196,11 @@ test('Encrypted queues result in a policy statement with aws:sourceAccount condi ], }, }, - ], + ]), 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', () => { diff --git a/packages/@aws-cdk/cx-api/lib/features.ts b/packages/@aws-cdk/cx-api/lib/features.ts index 5f4d6980dc522..a17dfa5f3212b 100644 --- a/packages/@aws-cdk/cx-api/lib/features.ts +++ b/packages/@aws-cdk/cx-api/lib/features.ts @@ -354,7 +354,7 @@ 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 + * from the same account as the Rule 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';