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

aws-events: EventBus and EventBusPolicy only accept a single statement #24671

Closed
ctobolski opened this issue Mar 17, 2023 · 8 comments · Fixed by #27340
Closed

aws-events: EventBus and EventBusPolicy only accept a single statement #24671

ctobolski opened this issue Mar 17, 2023 · 8 comments · Fixed by #27340
Labels
@aws-cdk/aws-events Related to CloudWatch Events bug This issue is a bug. p2

Comments

@ctobolski
Copy link

Describe the bug

The EventBus only accepts a single IAM statement in the policy. If I want to grant multiple principals different access to a bus, it should be possible with addToResourcePolicy, but that method explicitly denies additional statements. Defining an EventBusPolicy does not alleviate the issue, because EventBusPolicy also only accepts a single iam.PolicyStatement.

Expected Behavior

The documentation for EventBusPolicy and EventBus indicate that additional permissions should be added through the addToResourcePolicy method.
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_events.EventBus.html#applywbrremovalwbrpolicypolicy
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_events.EventBusPolicy.html

Policies define the operations that are allowed on this resource.

You almost never need to define this construct directly.

All AWS resources that support resource policies have a method called addToResourcePolicy(), which will automatically >create a new resource policy if one doesn't exist yet, otherwise it will add to the existing policy.

Prefer to use addToResourcePolicy() instead.

Expected behavior would be that addToResourcePolicy creates or updates the existing resource policy with additional statements.

Current Behavior

New statements are silently ignored, and the construct does not support adding additional permissions.
It looks like the behavior to only accept a single statement is codified in the addToResourcePolicy implementation. Shouldn't the policy contain a policy document rather than a single statement?

https://github.com/aws/aws-cdk/blob/main/packages/@aws-cdk/aws-events/lib/event-bus.ts#L341

It seems odd that not having a SID would cause an error to be thrown, while not adding new permissions would allow execution to continue and potentially deploy a change that doesn't have the correct policy.

Reproduction Steps

Create a bus, add multiple resource policies

import * as events from 'aws-cdk-lib/aws-events';
import * as iam from 'aws-cdk-lib/aws-iam';
import * as cdk from 'aws-cdk-lib';
import { Construct } from 'constructs';

export class BusStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props: cdk.StackProps) {
    super(scope, id, props);
    const bus = new events.EventBus(this, 'my-bus', {
      eventBusName: 'my-bus'
    });

    const externalPrincipal1 = new iam.AccountPrincipal('someaccountid');
    const addPublishForFooSource = new iam.PolicyStatement({
      effect: iam.Effect.ALLOW,
      principals: [externalPrincipal1],
      sid: `uniquesid1`,
      resources: [bus.eventBusArn],
      actions: ['events:PutEvents'],
    });
    addPublishForFooSource.addCondition('StringEquals', { 'events:source': ['foo'] });
    //succeeds
    bus.addToResourcePolicy(addPublishForFooSource);

    const externalPrincipal2 = new iam.AccountPrincipal('someotheraccountid');
    const  addPublishForBarSource = new iam.PolicyStatement({
      effect: iam.Effect.ALLOW,
      principals: [externalPrincipal2],
      sid: `uniquesid2`,
      resources: [bus.eventBusArn],
      actions: ['events:PutEvents'],
    });
    addPublishForFooSource.addCondition('StringEquals', { 'events:source': ['bar'] });
    //no error thrown, but no policy update
    bus.addToResourcePolicy(addPublishForBarSource);
  }
}

Possible Solution

Modify the EventBusPolicy to accept a PolicyDocument rather than a PolicyStatement, and update the addToResourcePolicy to append new statements to the existing policy.

Additional Information/Context

No response

CDK CLI Version

2.69

Framework Version

No response

Node.js Version

v16.17.1

OS

OSX

Language

Typescript

Language Version

No response

Other information

No response

@ctobolski ctobolski added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 17, 2023
@github-actions github-actions bot added the @aws-cdk/aws-events Related to CloudWatch Events label Mar 17, 2023
@khushail khushail self-assigned this Mar 17, 2023
@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Mar 17, 2023
@pahud pahud added the p2 label Mar 21, 2023
@khushail
Copy link
Contributor

Hi @ctobolski , thanks for reporting this issue and sharing the code snippet. I am able to reproduce this.

Currently I am marking this as P2 (which would mean it won't be worked upon immediately). We use +1s to help prioritize our work, and are happy to re-evaluate this issue based on community feedback. You can reach out to the cdk.dev community on Slack to solicit support for re-prioritization.

However if you would like to contribute, here is the contributing guide to get started.

@khushail khushail removed investigating This issue is being investigated and/or work is in progress to resolve the issue. needs-reproduction This issue needs reproduction. labels Mar 21, 2023
@khushail khushail removed their assignment Mar 21, 2023
@micro-jumbo
Copy link

👍🏼

@jsvasquez
Copy link

+1 and fixing.

I'll take care of this.

@postsa
Copy link
Contributor

postsa commented Sep 29, 2023

@ctobolski + @khushail I opened up #27340 with a fix.

It looks like the behavior to only accept a single statement is codified in the addToResourcePolicy implementation. Shouldn't the policy contain a policy document rather than a single statement?

Currently, the CFN construct for an EventBusPolicy takes only a single statement. However, updating addToResourcePolicy to accept additional statements on subsequent calls and creating new a policy for each results in the expected behavior.

@postsa
Copy link
Contributor

postsa commented Oct 3, 2023

@jsvasquez if you have a chance to review the above I would appreciate it!

@bs-u27
Copy link

bs-u27 commented Nov 9, 2023

+1

1 similar comment
@phadadi
Copy link

phadadi commented Nov 23, 2023

+1

@mergify mergify bot closed this as completed in #27340 Dec 19, 2023
mergify bot pushed a commit that referenced this issue Dec 19, 2023
…27340)

Enable the creation of multiple event bus policies on a single event bus.

Closes #24671.

The result of the Policies created by the integration test is a resource policy on the event bus that looks like 

```json
{
  "Version": "2012-10-17",
  "Statement": [{
    "Sid": "Statement2",
    "Effect": "Allow",
    "Principal": {
      "AWS": "arn:aws:iam::<account-id>:root"
    },
    "Action": "events:PutRule",
    "Resource": "arn:aws:events:us-west-2:<account-id>:event-bus/StackBusAA0A1E4B"
  }, {
    "Sid": "Statement1",
    "Effect": "Allow",
    "Principal": {
      "AWS": "arn:aws:iam::<account-id>:root"
    },
    "Action": "events:PutEvents",
    "Resource": "arn:aws:events:us-west-2:<account-id>:event-bus/StackBusAA0A1E4B"
  }]
}
```

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-events Related to CloudWatch Events bug This issue is a bug. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants