Skip to content

Commit

Permalink
fix(events-targets): EventBus IAM statements are only added for the f…
Browse files Browse the repository at this point in the history
…irst target (#20479)

If the `EventBus` constructor is called with no arguments, then attaching more than a single target to its policy will silently fail to add them. This is because of a strange edge case in the implementation that was not accounted for previously; it is possible for `props.role` to be `undefined`, yet `singletonEventRole()` is still capable of finding the desired role. `singletonEventRole()` does not add the new statements to any IAM policies that it finds, so as a result adding multiple targets does not add any of them.

Fixes #19407.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
comcalvi authored Jun 1, 2022
1 parent 264c02e commit 74318c7
Show file tree
Hide file tree
Showing 12 changed files with 138 additions and 55 deletions.
11 changes: 7 additions & 4 deletions packages/@aws-cdk/aws-events-targets/lib/api-destination.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,16 @@ export class ApiDestination implements events.IRuleTarget {
addToDeadLetterQueueResourcePolicy(_rule, this.props.deadLetterQueue);
}

const role = this.props?.eventRole ?? singletonEventRole(this.apiDestination);
role.addToPrincipalPolicy(new iam.PolicyStatement({
resources: [this.apiDestination.apiDestinationArn],
actions: ['events:InvokeApiDestination'],
}));

return {
...(this.props ? bindBaseTargetConfig(this.props) : {}),
arn: this.apiDestination.apiDestinationArn,
role: this.props?.eventRole ?? singletonEventRole(this.apiDestination, [new iam.PolicyStatement({
resources: [this.apiDestination.apiDestinationArn],
actions: ['events:InvokeApiDestination'],
})]),
role,
input: this.props.event,
targetResource: this.apiDestination,
httpParameters,
Expand Down
18 changes: 11 additions & 7 deletions packages/@aws-cdk/aws-events-targets/lib/api-gateway.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,16 +98,20 @@ export class ApiGateway implements events.IRuleTarget {
this.props?.path || '/',
this.props?.stage || this.restApi.deploymentStage.stageName,
);

const role = this.props?.eventRole || singletonEventRole(this.restApi);
role.addToPrincipalPolicy(new iam.PolicyStatement({
resources: [restApiArn],
actions: [
'execute-api:Invoke',
'execute-api:ManageConnections',
],
}));

return {
...(this.props ? bindBaseTargetConfig(this.props) : {}),
arn: restApiArn,
role: this.props?.eventRole || singletonEventRole(this.restApi, [new iam.PolicyStatement({
resources: [restApiArn],
actions: [
'execute-api:Invoke',
'execute-api:ManageConnections',
],
})]),
role,
deadLetterConfig: this.props?.deadLetterQueue && { arn: this.props.deadLetterQueue?.queueArn },
input: this.props?.postBody,
targetResource: this.restApi,
Expand Down
23 changes: 12 additions & 11 deletions packages/@aws-cdk/aws-events-targets/lib/batch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,20 +87,21 @@ export class BatchJob implements events.IRuleTarget {
addToDeadLetterQueueResourcePolicy(rule, this.props.deadLetterQueue);
}

// When scoping resource-level access for job submission, you must provide both job queue and job definition resource types.
// https://docs.aws.amazon.com/batch/latest/userguide/ExamplePolicies_BATCH.html#iam-example-restrict-job-def
const role = singletonEventRole(this.jobDefinitionScope);
role.addToPrincipalPolicy(new iam.PolicyStatement({
actions: ['batch:SubmitJob'],
resources: [
this.jobDefinitionArn,
this.jobQueueArn,
],
}));

return {
...bindBaseTargetConfig(this.props),
arn: this.jobQueueArn,
// When scoping resource-level access for job submission, you must provide both job queue and job definition resource types.
// https://docs.aws.amazon.com/batch/latest/userguide/ExamplePolicies_BATCH.html#iam-example-restrict-job-def
role: singletonEventRole(this.jobDefinitionScope, [
new iam.PolicyStatement({
actions: ['batch:SubmitJob'],
resources: [
this.jobDefinitionArn,
this.jobQueueArn,
],
}),
]),
role,
input: this.props.event,
targetResource: this.jobQueueScope,
batchParameters,
Expand Down
13 changes: 7 additions & 6 deletions packages/@aws-cdk/aws-events-targets/lib/codebuild.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,16 @@ export class CodeBuildProject implements events.IRuleTarget {
addToDeadLetterQueueResourcePolicy(_rule, this.props.deadLetterQueue);
}

const role = this.props.eventRole || singletonEventRole(this.project);
role.addToPrincipalPolicy(new iam.PolicyStatement({
actions: ['codebuild:StartBuild'],
resources: [this.project.projectArn],
}));

return {
...bindBaseTargetConfig(this.props),
arn: this.project.projectArn,
role: this.props.eventRole || singletonEventRole(this.project, [
new iam.PolicyStatement({
actions: ['codebuild:StartBuild'],
resources: [this.project.projectArn],
}),
]),
role,
input: this.props.event,
targetResource: this.project,
};
Expand Down
11 changes: 7 additions & 4 deletions packages/@aws-cdk/aws-events-targets/lib/codepipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,17 @@ export class CodePipeline implements events.IRuleTarget {
}

public bind(_rule: events.IRule, _id?: string): events.RuleTargetConfig {
const role = this.options.eventRole || singletonEventRole(this.pipeline);
role.addToPrincipalPolicy(new iam.PolicyStatement({
resources: [this.pipeline.pipelineArn],
actions: ['codepipeline:StartPipelineExecution'],
}));

return {
...bindBaseTargetConfig(this.options),
id: '',
arn: this.pipeline.pipelineArn,
role: this.options.eventRole || singletonEventRole(this.pipeline, [new iam.PolicyStatement({
resources: [this.pipeline.pipelineArn],
actions: ['codepipeline:StartPipelineExecution'],
})]),
role,
targetResource: this.pipeline,
};
}
Expand Down
9 changes: 3 additions & 6 deletions packages/@aws-cdk/aws-events-targets/lib/ecs-task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,9 @@ export class EcsTask implements events.IRuleTarget {
this.taskCount = props.taskCount ?? 1;
this.platformVersion = props.platformVersion;

if (props.role) {
const role = props.role;
this.createEventRolePolicyStatements().forEach(role.addToPrincipalPolicy.bind(role));
this.role = role;
} else {
this.role = singletonEventRole(this.taskDefinition, this.createEventRolePolicyStatements());
this.role = props.role ?? singletonEventRole(this.taskDefinition);
for (const stmt of this.createEventRolePolicyStatements()) {
this.role.addToPrincipalPolicy(stmt);
}

// Security groups are only configurable with the "awsvpc" network mode.
Expand Down
6 changes: 2 additions & 4 deletions packages/@aws-cdk/aws-events-targets/lib/event-bus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,8 @@ export class EventBus implements events.IRuleTarget {
constructor(private readonly eventBus: events.IEventBus, private readonly props: EventBusProps = {}) { }

bind(rule: events.IRule, _id?: string): events.RuleTargetConfig {
if (this.props.role) {
this.props.role.addToPrincipalPolicy(this.putEventStatement());
}
const role = this.props.role ?? singletonEventRole(rule, [this.putEventStatement()]);
const role = this.props.role ?? singletonEventRole(rule);
role.addToPrincipalPolicy(this.putEventStatement());

if (this.props.deadLetterQueue) {
addToDeadLetterQueueResourcePolicy(rule, this.props.deadLetterQueue);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,16 @@ export class KinesisFirehoseStream implements events.IRuleTarget {
* result from a Event Bridge event.
*/
public bind(_rule: events.IRule, _id?: string): events.RuleTargetConfig {
const policyStatements = [new iam.PolicyStatement({
const role = singletonEventRole(this.stream);
role.addToPrincipalPolicy(new iam.PolicyStatement({
actions: ['firehose:PutRecord', 'firehose:PutRecordBatch'],
resources: [this.stream.attrArn],
})];
}));


return {
arn: this.stream.attrArn,
role: singletonEventRole(this.stream, policyStatements),
role,
input: this.props.message,
targetResource: this.stream,
};
Expand Down
7 changes: 4 additions & 3 deletions packages/@aws-cdk/aws-events-targets/lib/kinesis-stream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,15 @@ export class KinesisStream implements events.IRuleTarget {
* result from a CloudWatch event.
*/
public bind(_rule: events.IRule, _id?: string): events.RuleTargetConfig {
const policyStatements = [new iam.PolicyStatement({
const role = singletonEventRole(this.stream);
role.addToPrincipalPolicy(new iam.PolicyStatement({
actions: ['kinesis:PutRecord', 'kinesis:PutRecords'],
resources: [this.stream.streamArn],
})];
}));

return {
arn: this.stream.streamArn,
role: singletonEventRole(this.stream, policyStatements),
role,
input: this.props.message,
targetResource: this.stream,
kinesisParameters: this.props.partitionKeyPath ? { partitionKeyPath: this.props.partitionKeyPath } : undefined,
Expand Down
5 changes: 1 addition & 4 deletions packages/@aws-cdk/aws-events-targets/lib/state-machine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,8 @@ export class SfnStateMachine implements events.IRuleTarget {
private readonly role: iam.IRole;

constructor(public readonly machine: sfn.IStateMachine, private readonly props: SfnStateMachineProps = {}) {
if (props.role) {
props.role.grant(new iam.ServicePrincipal('events.amazonaws.com'));
}
// no statements are passed because we are configuring permissions by using grant* helper below
this.role = props.role ?? singletonEventRole(machine, []);
this.role = props.role ?? singletonEventRole(machine);
machine.grantStartExecution(this.role);
}

Expand Down
4 changes: 1 addition & 3 deletions packages/@aws-cdk/aws-events-targets/lib/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export function bindBaseTargetConfig(props: TargetBaseProps) {
* events have the same target, they will share a role.
* @internal
*/
export function singletonEventRole(scope: IConstruct, policyStatements: iam.PolicyStatement[]): iam.IRole {
export function singletonEventRole(scope: IConstruct): iam.IRole {
const id = 'EventsRole';
const existing = scope.node.tryFindChild(id) as iam.IRole;
if (existing) { return existing; }
Expand All @@ -81,8 +81,6 @@ export function singletonEventRole(scope: IConstruct, policyStatements: iam.Poli
assumedBy: new iam.ServicePrincipal('events.amazonaws.com'),
});

policyStatements.forEach(role.addToPolicy.bind(role));

return role;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,3 +167,81 @@ test('with a Dead Letter Queue specified', () => {
],
});
});

test('event buses are correctly added to the rule\'s principal policy', () => {
const stack = new Stack();
const rule = new events.Rule(stack, 'Rule', {
schedule: events.Schedule.expression('rate(1 min)'),
});

const bus1 = new events.EventBus(stack, 'bus' + 1);
const bus2 = new events.EventBus(stack, 'bus' + 2);

rule.addTarget(new targets.EventBus(bus1));
rule.addTarget(new targets.EventBus(bus2));

Template.fromStack(stack).hasResourceProperties('AWS::Events::Rule', {
Targets: [
{
Arn: {
'Fn::GetAtt': [
'bus110C385DC',
'Arn',
],
},
Id: 'Target0',
RoleArn: {
'Fn::GetAtt': [
'RuleEventsRoleC51A4248',
'Arn',
],
},
},
{
Arn: {
'Fn::GetAtt': [
'bus22D01F126',
'Arn',
],
},
Id: 'Target1',
RoleArn: {
'Fn::GetAtt': [
'RuleEventsRoleC51A4248',
'Arn',
],
},
},
],
});
Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [
{
Effect: 'Allow',
Action: 'events:PutEvents',
Resource: {
'Fn::GetAtt': [
'bus110C385DC',
'Arn',
],
},
},
{
Effect: 'Allow',
Action: 'events:PutEvents',
Resource: {
'Fn::GetAtt': [
'bus22D01F126',
'Arn',
],
},
},
],
Version: '2012-10-17',
},
Roles: [{
Ref: 'RuleEventsRoleC51A4248',
}],
});
});

0 comments on commit 74318c7

Please sign in to comment.