Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(events-targets): encrypted queues get too wide permissions (under feature flag) #22740

Merged
merged 4 commits into from
Nov 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-events-targets/lib/log-group.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down
21 changes: 15 additions & 6 deletions packages/@aws-cdk/aws-events-targets/lib/sqs.ts
Original file line number Diff line number Diff line change
@@ -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';

/**
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-events-targets/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
{
"version": "20.0.0",
"version": "21.0.0",
"files": {
"e18a99f507ccee95a180ba3b3e0df1a514804ce9399b167dd95db86457e1e650": {
"2dc225f2fbae904e5a29adf2e7d3d70b01c10760a4fb779d1a447bda79b33e1d": {
"source": {
"path": "aws-cdk-sqs-event-target.template.json",
"packaging": "file"
},
"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}"
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@
"kms:GenerateDataKey*",
"kms:ReEncrypt*"
],
"Condition": {
"StringEquals": {
"aws:SourceAccount": {
"Ref": "AWS::AccountId"
}
}
},
"Effect": "Allow",
"Principal": {
"Service": "events.amazonaws.com"
Expand Down Expand Up @@ -98,6 +105,13 @@
"sqs:GetQueueUrl",
"sqs:SendMessage"
],
"Condition": {
"StringEquals": {
"aws:SourceAccount": {
"Ref": "AWS::AccountId"
}
}
},
"Effect": "Allow",
"Principal": {
"Service": "events.amazonaws.com"
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"20.0.0"}
{"version":"21.0.0"}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "20.0.0",
"version": "21.0.0",
"testCases": {
"integ.sqs-event-rule-target": {
"stacks": [
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "20.0.0",
"version": "21.0.0",
"artifacts": {
"Tree": {
"type": "cdk:tree",
Expand All @@ -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": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"path": "Tree",
"constructInfo": {
"fqn": "constructs.Construct",
"version": "10.1.85"
"version": "10.1.140"
}
},
"aws-cdk-sqs-event-target": {
Expand Down Expand Up @@ -58,6 +58,13 @@
"kms:GenerateDataKey*",
"kms:ReEncrypt*"
],
"Condition": {
"StringEquals": {
"aws:SourceAccount": {
"Ref": "AWS::AccountId"
}
}
},
"Effect": "Allow",
"Principal": {
"Service": "events.amazonaws.com"
Expand Down Expand Up @@ -165,6 +172,13 @@
"sqs:GetQueueUrl",
"sqs:SendMessage"
],
"Condition": {
"StringEquals": {
"aws:SourceAccount": {
"Ref": "AWS::AccountId"
}
}
},
"Effect": "Allow",
"Principal": {
"Service": "events.amazonaws.com"
Expand Down Expand Up @@ -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"
}
}
}
120 changes: 120 additions & 0 deletions packages/@aws-cdk/aws-events-targets/test/sqs/sqs.test.ts
Original file line number Diff line number Diff line change
@@ -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', () => {
Expand Down Expand Up @@ -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');
Expand Down
8 changes: 8 additions & 0 deletions packages/@aws-cdk/cx-api/lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down Expand Up @@ -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,
};

/**
Expand Down