Skip to content

Commit a36f2f0

Browse files
otaviomacedootaviomacedo
authored
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*
1 parent ea9ab4b commit a36f2f0

File tree

12 files changed

+187
-20
lines changed

12 files changed

+187
-20
lines changed

‎packages/@aws-cdk/aws-events-targets/lib/log-group.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
import * as events from '@aws-cdk/aws-events';
2+
import { RuleTargetInputProperties, RuleTargetInput, EventField, IRule } from '@aws-cdk/aws-events';
23
import * as iam from '@aws-cdk/aws-iam';
34
import * as logs from '@aws-cdk/aws-logs';
45
import * as cdk from '@aws-cdk/core';
56
import { ArnFormat, Stack } from '@aws-cdk/core';
67
import { LogGroupResourcePolicy } from './log-group-resource-policy';
78
import { TargetBaseProps, bindBaseTargetConfig } from './util';
8-
import { RuleTargetInputProperties, RuleTargetInput, EventField, IRule } from '@aws-cdk/aws-events';
99

1010
/**
1111
* Options used when creating a target input template

‎packages/@aws-cdk/aws-events-targets/lib/sqs.ts

+15-6
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import * as events from '@aws-cdk/aws-events';
22
import * as iam from '@aws-cdk/aws-iam';
33
import * as sqs from '@aws-cdk/aws-sqs';
4+
import { Aws, FeatureFlags } from '@aws-cdk/core';
5+
import * as cxapi from '@aws-cdk/cx-api';
46
import { addToDeadLetterQueueResourcePolicy, TargetBaseProps, bindBaseTargetConfig } from './util';
57

68
/**
@@ -52,15 +54,22 @@ export class SqsQueue implements events.IRuleTarget {
5254
* @see https://docs.aws.amazon.com/eventbridge/latest/userguide/resource-based-policies-eventbridge.html#sqs-permissions
5355
*/
5456
public bind(rule: events.IRule, _id?: string): events.RuleTargetConfig {
55-
// Only add the rule as a condition if the queue is not encrypted, to avoid circular dependency. See issue #11158.
56-
const principalOpts = this.queue.encryptionMasterKey ? {} : {
57-
conditions: {
57+
const restrictToSameAccount = FeatureFlags.of(rule).isEnabled(cxapi.EVENTS_TARGET_QUEUE_SAME_ACCOUNT);
58+
59+
let conditions: any = {};
60+
if (!this.queue.encryptionMasterKey) {
61+
conditions = {
5862
ArnEquals: { 'aws:SourceArn': rule.ruleArn },
59-
},
60-
};
63+
};
64+
} else if (restrictToSameAccount) {
65+
// Aadd only the account id as a condition, to avoid circular dependency. See issue #11158.
66+
conditions = {
67+
StringEquals: { 'aws:SourceAccount': Aws.ACCOUNT_ID },
68+
};
69+
}
6170

6271
// deduplicated automatically
63-
this.queue.grantSendMessages(new iam.ServicePrincipal('events.amazonaws.com', principalOpts));
72+
this.queue.grantSendMessages(new iam.ServicePrincipal('events.amazonaws.com', { conditions }));
6473

6574
if (this.props.deadLetterQueue) {
6675
addToDeadLetterQueueResourcePolicy(rule, this.props.deadLetterQueue);

‎packages/@aws-cdk/aws-events-targets/package.json

+2
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@
9898
"@aws-cdk/aws-autoscaling": "0.0.0",
9999
"@aws-cdk/aws-codebuild": "0.0.0",
100100
"@aws-cdk/aws-codepipeline": "0.0.0",
101+
"@aws-cdk/cx-api": "0.0.0",
101102
"@aws-cdk/aws-ec2": "0.0.0",
102103
"@aws-cdk/aws-ecs": "0.0.0",
103104
"@aws-cdk/aws-events": "0.0.0",
@@ -121,6 +122,7 @@
121122
"@aws-cdk/aws-autoscaling": "0.0.0",
122123
"@aws-cdk/aws-codebuild": "0.0.0",
123124
"@aws-cdk/aws-codepipeline": "0.0.0",
125+
"@aws-cdk/cx-api": "0.0.0",
124126
"@aws-cdk/aws-ec2": "0.0.0",
125127
"@aws-cdk/aws-ecs": "0.0.0",
126128
"@aws-cdk/aws-events": "0.0.0",

‎packages/@aws-cdk/aws-events-targets/test/logs/log-group.test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,10 @@ import { Template } from '@aws-cdk/assertions';
22
import * as events from '@aws-cdk/aws-events';
33
import * as logs from '@aws-cdk/aws-logs';
44
import * as sqs from '@aws-cdk/aws-sqs';
5+
import { testDeprecated } from '@aws-cdk/cdk-build-tools';
56
import * as cdk from '@aws-cdk/core';
67
import * as targets from '../../lib';
78
import { LogGroupTargetInput } from '../../lib';
8-
import { testDeprecated } from '@aws-cdk/cdk-build-tools';
99

1010
test('use log group as an event rule target', () => {
1111
// GIVEN

‎packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.assets.json

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
{
2-
"version": "20.0.0",
2+
"version": "21.0.0",
33
"files": {
4-
"e18a99f507ccee95a180ba3b3e0df1a514804ce9399b167dd95db86457e1e650": {
4+
"2dc225f2fbae904e5a29adf2e7d3d70b01c10760a4fb779d1a447bda79b33e1d": {
55
"source": {
66
"path": "aws-cdk-sqs-event-target.template.json",
77
"packaging": "file"
88
},
99
"destinations": {
1010
"current_account-current_region": {
1111
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
12-
"objectKey": "e18a99f507ccee95a180ba3b3e0df1a514804ce9399b167dd95db86457e1e650.json",
12+
"objectKey": "2dc225f2fbae904e5a29adf2e7d3d70b01c10760a4fb779d1a447bda79b33e1d.json",
1313
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
1414
}
1515
}

‎packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.template.json

+14
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,13 @@
3535
"kms:GenerateDataKey*",
3636
"kms:ReEncrypt*"
3737
],
38+
"Condition": {
39+
"StringEquals": {
40+
"aws:SourceAccount": {
41+
"Ref": "AWS::AccountId"
42+
}
43+
}
44+
},
3845
"Effect": "Allow",
3946
"Principal": {
4047
"Service": "events.amazonaws.com"
@@ -98,6 +105,13 @@
98105
"sqs:GetQueueUrl",
99106
"sqs:SendMessage"
100107
],
108+
"Condition": {
109+
"StringEquals": {
110+
"aws:SourceAccount": {
111+
"Ref": "AWS::AccountId"
112+
}
113+
}
114+
},
101115
"Effect": "Allow",
102116
"Principal": {
103117
"Service": "events.amazonaws.com"

‎packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/cdk.out

+1-1
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
{"version":"20.0.0"}
1+
{"version":"21.0.0"}

‎packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/integ.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"version": "20.0.0",
2+
"version": "21.0.0",
33
"testCases": {
44
"integ.sqs-event-rule-target": {
55
"stacks": [

‎packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/manifest.json

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"version": "20.0.0",
2+
"version": "21.0.0",
33
"artifacts": {
44
"Tree": {
55
"type": "cdk:tree",
@@ -23,7 +23,7 @@
2323
"validateOnSynth": false,
2424
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}",
2525
"cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}",
26-
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/e18a99f507ccee95a180ba3b3e0df1a514804ce9399b167dd95db86457e1e650.json",
26+
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/2dc225f2fbae904e5a29adf2e7d3d70b01c10760a4fb779d1a447bda79b33e1d.json",
2727
"requiresBootstrapStackVersion": 6,
2828
"bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version",
2929
"additionalDependencies": [

‎packages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/tree.json

+19-5
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
"path": "Tree",
1010
"constructInfo": {
1111
"fqn": "constructs.Construct",
12-
"version": "10.1.85"
12+
"version": "10.1.140"
1313
}
1414
},
1515
"aws-cdk-sqs-event-target": {
@@ -58,6 +58,13 @@
5858
"kms:GenerateDataKey*",
5959
"kms:ReEncrypt*"
6060
],
61+
"Condition": {
62+
"StringEquals": {
63+
"aws:SourceAccount": {
64+
"Ref": "AWS::AccountId"
65+
}
66+
}
67+
},
6168
"Effect": "Allow",
6269
"Principal": {
6370
"Service": "events.amazonaws.com"
@@ -165,6 +172,13 @@
165172
"sqs:GetQueueUrl",
166173
"sqs:SendMessage"
167174
],
175+
"Condition": {
176+
"StringEquals": {
177+
"aws:SourceAccount": {
178+
"Ref": "AWS::AccountId"
179+
}
180+
}
181+
},
168182
"Effect": "Allow",
169183
"Principal": {
170184
"Service": "events.amazonaws.com"
@@ -284,14 +298,14 @@
284298
}
285299
},
286300
"constructInfo": {
287-
"fqn": "constructs.Construct",
288-
"version": "10.1.85"
301+
"fqn": "@aws-cdk/core.Stack",
302+
"version": "0.0.0"
289303
}
290304
}
291305
},
292306
"constructInfo": {
293-
"fqn": "constructs.Construct",
294-
"version": "10.1.85"
307+
"fqn": "@aws-cdk/core.App",
308+
"version": "0.0.0"
295309
}
296310
}
297311
}

‎packages/@aws-cdk/aws-events-targets/test/sqs/sqs.test.ts

+120
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import { Template } from '@aws-cdk/assertions';
22
import * as events from '@aws-cdk/aws-events';
3+
import * as kms from '@aws-cdk/aws-kms';
34
import * as sqs from '@aws-cdk/aws-sqs';
45
import { Duration, Stack } from '@aws-cdk/core';
6+
import * as cxapi from '@aws-cdk/cx-api';
57
import * as targets from '../../lib';
68

79
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
141143
});
142144
});
143145

146+
test('Encrypted queues result in a policy statement with aws:sourceAccount condition when the feature flag is on', () => {
147+
// GIVEN
148+
const stack = new Stack();
149+
stack.node.setContext(cxapi.EVENTS_TARGET_QUEUE_SAME_ACCOUNT, true);
150+
const queue = new sqs.Queue(stack, 'MyQueue', {
151+
encryptionMasterKey: kms.Key.fromKeyArn(stack, 'key', 'arn:aws:kms:us-west-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab'),
152+
});
153+
154+
const rule = new events.Rule(stack, 'MyRule', {
155+
schedule: events.Schedule.rate(Duration.hours(1)),
156+
});
157+
158+
// WHEN
159+
rule.addTarget(new targets.SqsQueue(queue));
160+
161+
// THEN
162+
Template.fromStack(stack).hasResourceProperties('AWS::SQS::QueuePolicy', {
163+
PolicyDocument: {
164+
Statement: [
165+
{
166+
Action: [
167+
'sqs:SendMessage',
168+
'sqs:GetQueueAttributes',
169+
'sqs:GetQueueUrl',
170+
],
171+
Condition: {
172+
StringEquals: {
173+
'aws:SourceAccount': { Ref: 'AWS::AccountId' },
174+
},
175+
},
176+
Effect: 'Allow',
177+
Principal: { Service: 'events.amazonaws.com' },
178+
Resource: {
179+
'Fn::GetAtt': [
180+
'MyQueueE6CA6235',
181+
'Arn',
182+
],
183+
},
184+
},
185+
],
186+
Version: '2012-10-17',
187+
},
188+
Queues: [{ Ref: 'MyQueueE6CA6235' }],
189+
});
190+
191+
Template.fromStack(stack).hasResourceProperties('AWS::Events::Rule', {
192+
ScheduleExpression: 'rate(1 hour)',
193+
State: 'ENABLED',
194+
Targets: [
195+
{
196+
Arn: {
197+
'Fn::GetAtt': [
198+
'MyQueueE6CA6235',
199+
'Arn',
200+
],
201+
},
202+
Id: 'Target0',
203+
},
204+
],
205+
});
206+
});
207+
208+
test('Encrypted queues result in a permissive policy statement when the feature flag is off', () => {
209+
// GIVEN
210+
const stack = new Stack();
211+
const queue = new sqs.Queue(stack, 'MyQueue', {
212+
encryptionMasterKey: kms.Key.fromKeyArn(stack, 'key', 'arn:aws:kms:us-west-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab'),
213+
});
214+
215+
const rule = new events.Rule(stack, 'MyRule', {
216+
schedule: events.Schedule.rate(Duration.hours(1)),
217+
});
218+
219+
// WHEN
220+
rule.addTarget(new targets.SqsQueue(queue));
221+
222+
// THEN
223+
Template.fromStack(stack).hasResourceProperties('AWS::SQS::QueuePolicy', {
224+
PolicyDocument: {
225+
Statement: [
226+
{
227+
Action: [
228+
'sqs:SendMessage',
229+
'sqs:GetQueueAttributes',
230+
'sqs:GetQueueUrl',
231+
],
232+
Effect: 'Allow',
233+
Principal: { Service: 'events.amazonaws.com' },
234+
Resource: {
235+
'Fn::GetAtt': [
236+
'MyQueueE6CA6235',
237+
'Arn',
238+
],
239+
},
240+
},
241+
],
242+
Version: '2012-10-17',
243+
},
244+
Queues: [{ Ref: 'MyQueueE6CA6235' }],
245+
});
246+
247+
Template.fromStack(stack).hasResourceProperties('AWS::Events::Rule', {
248+
ScheduleExpression: 'rate(1 hour)',
249+
State: 'ENABLED',
250+
Targets: [
251+
{
252+
Arn: {
253+
'Fn::GetAtt': [
254+
'MyQueueE6CA6235',
255+
'Arn',
256+
],
257+
},
258+
Id: 'Target0',
259+
},
260+
],
261+
});
262+
});
263+
144264
test('fail if messageGroupId is specified on non-fifo queues', () => {
145265
const stack = new Stack();
146266
const queue = new sqs.Queue(stack, 'MyQueue');

‎packages/@aws-cdk/cx-api/lib/features.ts

+8
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,13 @@ export const APIGATEWAY_DISABLE_CLOUDWATCH_ROLE = '@aws-cdk/aws-apigateway:disab
352352
*/
353353
export const ENABLE_PARTITION_LITERALS = '@aws-cdk/core:enablePartitionLiterals';
354354

355+
/**
356+
* This flag applies to SQS Queues that are used as the target of event Rules. When enabled, only principals
357+
* from the same account as the Queue can send messages. If a queue is unencrypted, this restriction will
358+
* always apply, regardless of the value of this flag.
359+
*/
360+
export const EVENTS_TARGET_QUEUE_SAME_ACCOUNT = '@aws-cdk/aws-events:eventsTargetQueueSameAccount';
361+
355362
/**
356363
* Flag values that should apply for new projects
357364
*
@@ -387,6 +394,7 @@ export const FUTURE_FLAGS: { [key: string]: boolean } = {
387394
[SNS_SUBSCRIPTIONS_SQS_DECRYPTION_POLICY]: true,
388395
[APIGATEWAY_DISABLE_CLOUDWATCH_ROLE]: true,
389396
[ENABLE_PARTITION_LITERALS]: true,
397+
[EVENTS_TARGET_QUEUE_SAME_ACCOUNT]: true,
390398
};
391399

392400
/**

0 commit comments

Comments
 (0)