Skip to content

Commit

Permalink
fix(events-targets): policy restricts access to the same account as t…
Browse files Browse the repository at this point in the history
…he 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*
  • Loading branch information
otaviomacedo authored Nov 4, 2022
1 parent ec32b5b commit 0083256
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 31 deletions.
6 changes: 3 additions & 3 deletions packages/@aws-cdk/aws-events-targets/lib/sqs.ts
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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 },
};
}

Expand Down
52 changes: 25 additions & 27 deletions packages/@aws-cdk/aws-events-targets/test/sqs/sqs.test.ts
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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',
Expand All @@ -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',
Expand All @@ -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', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/cx-api/lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down

0 comments on commit 0083256

Please sign in to comment.