fix(events): now `EventBus.grantPutEventsTo` correctly handles service principals (under feature flag) #33729

scorbiere · 2025-03-10T23:55:38Z

Issue #22080

Closes #22080.

Reason for this change

When trying to grant PutEvents permissions to an AWS Service Principal using grantPutEventsTo, the method performed a no-op without any warnings or errors. This prevented users from properly granting permissions to service principals, even though this is a valid use case that can be done through the AWS Console. The change implements the correct behavior by creating an EventBusPolicy when dealing with service principals.

Description of changes

Added special handling for service principals in EventBus.grantPutEventsTo method
When granting permissions to a service principal, creates an EventBusPolicy instead of attempting to modify IAM policies
Returns iam.Grant.drop() for service principals to indicate permissions are handled via EventBusPolicy
Added test cases to verify both service principal and IAM principal scenarios

Describe any new or updated permissions being added

The change introduces the creation of EventBusPolicy resources with events:PutEvents permission when granting access to service principals. This is not a new permission, but rather a different way of granting the same permission through resource-based policies instead of identity-based policies.

Description of how you validated changes

Added new test cases that verify:

EventBusPolicy is correctly created when granting permissions to service principals
IAM policies are correctly created when granting permissions to IAM roles/users

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

…vice principals (issue aws#22080)

…principals (fixes aws#22080)

aws-cdk-automation

(This review is outdated)

codecov · 2025-03-11T00:05:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.38%. Comparing base (e307404) to head (96717bb).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #33729   +/-   ##
=======================================
  Coverage   82.38%   82.38%           
=======================================
  Files         120      120           
  Lines        6937     6937           
  Branches     1170     1170           
=======================================
  Hits         5715     5715           
  Misses       1119     1119           
  Partials      103      103

Flag	Coverage Δ
suite.unit	`82.38% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`∅ <ø> (∅)`
packages/aws-cdk-lib/core	`82.38% <ø> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

moelasmar · 2025-03-11T17:19:57Z

packages/aws-cdk-lib/aws-events/lib/event-bus.ts

@@ -177,6 +177,21 @@ abstract class EventBusBase extends Resource implements IEventBus {
  }

  public grantPutEventsTo(grantee: iam.IGrantable): iam.Grant {
+    // Special handling for service principals
+    if (grantee.grantPrincipal instanceof iam.ServicePrincipal) {


this is not a generic solution. We still have issues with other Principle types that does not implement addToPrincipalPolicy (or it can not linked to a Policy resource), like AccountPrincipal, AnyPrincipal.

EventBus resource supports the ResourcePolicy (AWS::Events::EventBusPolicy) .. so we should make the EventBus L2 to implement the interface IResourceWithPolicy, and to implements addToResourcePolicy function which looks it is already there.

Then you can change the implementation of this function from iam.Grant.addToPrincipal to be iam.Grant.addToPrincipalOrResource

one more thing, I am not sure if it is Ok to create multiple EventBusPolicy resources for the same EventBus or not .. we need to have an integ test case for that.

moelasmar

thanks Seb for raising this PR, I left some comment

…ntPutEventsTo * Use Grant.addToPrincipalOrResource for consistent cross-account and service principal handling * Add default SID generation for EventBus resource policies * Add comprehensive test coverage including: - Same account IAM principals - Cross-account service principals - Cross-account IAM principals - Cross-account organization principals - Token account scenarios - Imported event bus cases

…ncipals in EventBus.grantPutEventsTo

…us resource policies When granting permissions to service principals using grantPutEventsTo(), the operation currently fails silently because service principals require resource policies with Statement IDs. This change introduces a new feature flag '@aws-cdk/aws-events:requireEventBusPolicySid' that controls this behavior: * When enabled (recommended): - Resource policies will be created with Statement IDs for service principals - The operation will succeed as expected * When disabled (legacy): - A warning will be emitted - The grant operation will be dropped - No permissions will be added The feature flag is set to 'true' in recommended-feature-flags.json to ensure proper behavior by default in future versions.

moelasmar · 2025-03-18T17:48:33Z

packages/aws-cdk-lib/aws-events/test/event-bus.grant.test.ts

+        Template.fromStack(stack).hasResourceProperties('AWS::Events::EventBusPolicy', {
+          Statement: Match.objectLike({
+            Effect: 'Allow',
+            Action: 'events:PutEvents',
+            Principal: { Service: 'states.amazonaws.com' },
+            Resource: Match.anyValue(),
+            Condition: Match.absent(),
+          }),
+        });
+      });


to understand, what we are checking here different than the check in line 118

Right, this is confusing and this 2nd assertion doesn't add much value. I will re-write the assertions with helper functions in order to improve the readability.

moelasmar · 2025-03-18T18:10:16Z

packages/@aws-cdk-testing/framework-integ/test/aws-events/test/integ.eventbus-grants.ts

+import { STANDARD_NODEJS_RUNTIME } from '../../config';
+
+// Create a single app for both stacks
+const app = new App();


I think in this integ test case, there is no difference between with feature flag, and without. In both cases the permissions will be added to the Role .. may be this test case is designed to verify that the current customer code will stay working. so, can we have a new integ test case to verify the new behaviour of grant method, and also, to verify that we can call grant method more than one in the new approach.

moelasmar · 2025-03-18T18:12:10Z

packages/aws-cdk-lib/aws-events/test/event-bus.test.ts

-  test('Event Bus policy statements must have a sid', () => {
-    // GIVEN
-    const app = new App();
-    const stack = new Stack(app, 'Stack');
-    const bus = new EventBus(stack, 'Bus');
-
-    // THEN
-    expect(() => bus.addToResourcePolicy(new iam.PolicyStatement({
-      effect: iam.Effect.ALLOW,
-      principals: [new iam.ArnPrincipal('arn')],
-      actions: ['events:PutEvents'],
-    }))).toThrow('Event Bus policy statements must have a sid');
-  });


I think this test case is still valid, right ?

Yes, good catch.

moelasmar · 2025-03-18T18:21:42Z

packages/aws-cdk-lib/aws-events/test/event-bus.grant.test.ts

+        });
+      });
+
+      test('grantPutEventsTo creates both IAM and resource policies for cross-account service principal', () => {


I think the test title is wrong.

moelasmar · 2025-03-18T18:22:45Z

packages/aws-cdk-lib/aws-events/test/event-bus.grant.test.ts

+        expect(grant.success).toBeTruthy();
+      });
+
+      test('grantPutEventsTo creates both IAM and resource policies for cross-account role principal', () => {


this one as well, has a wrong title

moelasmar · 2025-03-18T18:23:41Z

packages/aws-cdk-lib/aws-events/test/event-bus.grant.test.ts

+        expect(grant.success).toBeTruthy();
+      });
+
+      test('grantPutEventsTo creates both IAM and resource policies for cross-account organization principal', () => {


same here as well

moelasmar · 2025-03-18T18:24:37Z

packages/aws-cdk-lib/aws-events/test/event-bus.grant.test.ts

+        Template.fromStack(stack).resourceCountIs('AWS::IAM::Policy', 0);
+
+        // Verify all required parts of the organization policy
+        Template.fromStack(stack).hasResourceProperties('AWS::Events::EventBusPolicy', {


what is the difference between this check and check in line 217

…t we can grant to a service principal.

moelasmar

Thanks Seb, I still have some concern about the new integration test case

moelasmar · 2025-03-19T14:42:40Z

packages/aws-cdk-lib/aws-events/test/event-bus.grant.test.ts

+/**
+ * Verifies that no resource policy (EventBusPolicy resource) was created.
+ */
+function assertNoResourcePolicy(stack: Stack) {
+  Template.fromStack(stack).resourceCountIs('AWS::Events::EventBusPolicy', 0);
+}
+
+/**
+ * Verifies that no identity-based policy was created.
+ */
+function assertNoIdentityPolicy(stack: Stack) {
+  Template.fromStack(stack).resourceCountIs('AWS::IAM::Policy', 0);
+}


nit comment, and you can ignore it .. I believe the assert statement itself is clear, and does not need to be wrapped in a function

Fair enough, but I prefer the readability of the method name in the test.

moelasmar · 2025-03-19T14:48:44Z

packages/aws-cdk-lib/aws-events/test/event-bus.grant.test.ts

        expect(grant.success).toBeTruthy();
      });
    });

    describe('cross-account scenarios', () => {
-      test('grantPutEventsTo creates EventBusPolicy for service principal without conditions', () => {
+      test('creates resource policy for service principal without conditions', () => {


nit: this test case is not a cross account test case, and there is no big difference vs test case creates only resource policy for service principal with source account condition from this change POV.

Right, I will remove it.

moelasmar · 2025-03-19T14:51:22Z

packages/aws-cdk-lib/aws-events/test/event-bus.grant.test.ts

      });

-      test('grantPutEventsTo creates both IAM and resource policies for cross-account service principal', () => {
+      test('creates resource policy for service principal with cross-account condition', () => {


nit: I also believe this test case is redundant, I think from this change POV, the Service principal with account condition is not different from a normal service principal

Right, I will remove it as well.

moelasmar · 2025-03-19T15:05:19Z

packages/aws-cdk-lib/aws-events/test/event-bus.grant.test.ts

+            ':',
+            { Ref: 'AWS::AccountId' },
+            ':event-bus/',
+            { Ref: 'EventBus7B8748AA' },


similar to the previous comment, I expect to see some normal string value, instead of ref intrinsic function.

moelasmar · 2025-03-19T15:10:14Z

packages/aws-cdk-lib/aws-events/test/event-bus.grant.test.ts

+        // GIVEN
+        // Create a role with a token principal
+        const role = new iam.Role(stack, 'Role', {
+          assumedBy: new iam.AccountRootPrincipal(), // This will use the stack's account token


to understand, why do we care about this token .. how may this value change the process of adding the permissions?

This case was making sense when I was trying to use the Grant.addToPrincipalOrResource(...) method in EventBusBase.grantPutEventsTo(...). Which not the strategy used anymore. I will remove this test.

moelasmar · 2025-03-19T15:17:57Z

packages/aws-cdk-lib/aws-events/test/event-bus.grant.test.ts

      });
    });

    describe('cross-stack scenarios', () => {
-      test('grantPutEventsTo works across stacks', () => {
+      test('creates identity-based policy in different stack', () => {


Can you add a test case to grant an imported role that is not managed by the CDK application. I expect in this scenario, we should see an Event Resource Policy instead of identity policy.

I will add that test case, but the template generated is:

ImportedRoleIdPolicy878A765C :: { "Properties": { "PolicyDocument": { "Statement": [ { "Action": "events:PutEvents", "Effect": "Allow", "Resource": { "Fn::GetAtt": [ "EventBus7B8748AA", "Arn" ], } } ], "Version": "2012-10-17" }, "PolicyName": "ImportedRoleIdPolicy878A765C", "Roles": [ "importedRoleName" ] }, "Type": "AWS::IAM::Policy" }

Even when I use iam.Role.fromRoleArn(...). The code in Grant.addToPrincipal(...) call directly ImportedRole.addToPrincipalPolicy(...)

when you call fromRolexxx function, set mutable flag to false .. https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_iam.FromRoleArnOptions.html#mutable

This means that CDK could not attach policies to this Role.

moelasmar · 2025-03-19T15:47:28Z

...-cdk-testing/framework-integ/test/aws-events/test/integ.eventbus-service-principal-grants.ts

@@ -0,0 +1,142 @@
+import * as cdk from 'aws-cdk-lib';


this test case is also miss leading, if you checked the generated template, you will find the following:

TargetEventBuscdkEventsServicePrincipalGrant11A8AC78 Event resource policy that grant events.amazonaws.com service to put event on eventBus TargetEventBus43DE6DE2 (this is the point we need to verify)

But also, you will find ForwardingRuleEventsRoleDefaultPolicy5A4528E2 identity policy that grant role ForwardingRuleEventsRole9F12172A to put events on EventBus TargetEventBus43DE6DE2, and this role is attached to EventRule ForwardingRule6039FF0D which is linked to EventBus SourceEventBusA326BD45

So, I am not sure which policy is applied. .. can you try to change this logic, to add a lambda function that try to put some event to an event bus, and do not grant the function itself, but grant the Lambda Service, and see if this will work or not

Good catch, it seems that the EventBus class implementation (link) of the method bind automatically tries to add the put event policy on its associated role (type iam.IRole), which is not compatible with the service principal we are using in this integ test.

Since I can't think of a realistic use case where we would only use a service principal role, I will remove this integ test.

…icy case. Add an integ test which uses 2 accounts to force the resource policy scenario.

aws-cdk-automation · 2025-03-21T01:18:03Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 96717bb
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify · 2025-03-21T05:03:40Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2025-03-21T05:03:57Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

scorbiere added 2 commits March 10, 2025 14:23

test(events): add failing test for EventBus.grantPutEventsTo with ser…

bf31013

…vice principals (issue aws#22080)

fix(events): EventBus.grantPutEventsTo now correctly handles service …

50bd792

…principals (fixes aws#22080)

aws-cdk-automation requested a review from a team March 10, 2025 23:55

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Mar 10, 2025

mergify bot added the contribution/core This is a PR that came from AWS. label Mar 10, 2025

aws-cdk-automation previously requested changes Mar 10, 2025

View reviewed changes

scorbiere changed the title ~~fix(events): EventBus.grantPutEventsTo now correctly handles service principals~~ fix(events): now EventBus.grantPutEventsTo correctly handles service principals Mar 10, 2025

moelasmar self-assigned this Mar 11, 2025

moelasmar reviewed Mar 11, 2025

View reviewed changes

moelasmar requested changes Mar 11, 2025

View reviewed changes

scorbiere added 4 commits March 13, 2025 14:27

Merge branch 'main' into issue_22080

e6ab1d2

fix(events): implement consistent permission handling for service pri…

70772a4

…ncipals in EventBus.grantPutEventsTo

Merge branch 'main' into issue_22080

5541e98

scorbiere added the pr/do-not-merge This PR should not be merged at this time. label Mar 14, 2025

scorbiere changed the title ~~fix(events): now EventBus.grantPutEventsTo correctly handles service principals~~ fix(aws-events): now EventBus.grantPutEventsTo correctly handles service principals (under feature flag) Mar 14, 2025

scorbiere added 3 commits March 14, 2025 14:45

Update the snapshot

3da7c75

Fix merge conflict

2848d53

scorbiere removed the pr/do-not-merge This PR should not be merged at this time. label Mar 14, 2025

scorbiere added 5 commits March 14, 2025 16:42

Merge branch 'main' into issue_22080

af3f621

Merge branch 'main' into issue_22080

ae9cee6

fix unit tests

c5740b3

Merge branch 'main' into issue_22080

df93608

Fix comment example (rosetta)

a5c8b30

moelasmar reviewed Mar 18, 2025

View reviewed changes

Re-organized and fix unit tests. Add a new integ test to validate tha…

19e5f48

…t we can grant to a service principal.

Remove unused import

303ff44

moelasmar requested changes Mar 19, 2025

View reviewed changes

scorbiere added 2 commits March 19, 2025 21:51

remove integ.eventbus-service-principal-grants.ts and update unit tests

6900b61

Added unit test to check imported role can trigger resource based pol…

8040d6b

…icy case. Add an integ test which uses 2 accounts to force the resource policy scenario.

moelasmar approved these changes Mar 20, 2025

View reviewed changes

Merge branch 'main' into issue_22080

96717bb

scorbiere changed the title ~~fix(aws-events): now EventBus.grantPutEventsTo correctly handles service principals (under feature flag)~~ fix(events): now EventBus.grantPutEventsTo correctly handles service principals (under feature flag) Mar 21, 2025

mergify bot merged commit 38d82c4 into aws:main Mar 21, 2025
27 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(events): now `EventBus.grantPutEventsTo` correctly handles service principals (under feature flag) #33729

fix(events): now `EventBus.grantPutEventsTo` correctly handles service principals (under feature flag) #33729

scorbiere commented Mar 10, 2025

aws-cdk-automation left a comment •

edited

Loading

codecov bot commented Mar 11, 2025 •

edited

Loading

moelasmar Mar 11, 2025

moelasmar Mar 11, 2025 •

edited

Loading

moelasmar left a comment

moelasmar Mar 18, 2025

scorbiere Mar 18, 2025

moelasmar Mar 18, 2025

moelasmar Mar 18, 2025

scorbiere Mar 18, 2025

moelasmar Mar 18, 2025

moelasmar Mar 18, 2025

moelasmar Mar 18, 2025

moelasmar Mar 18, 2025

moelasmar left a comment

moelasmar Mar 19, 2025

scorbiere Mar 20, 2025

moelasmar Mar 19, 2025

scorbiere Mar 19, 2025

moelasmar Mar 19, 2025

scorbiere Mar 20, 2025

moelasmar Mar 19, 2025

moelasmar Mar 19, 2025

scorbiere Mar 20, 2025

moelasmar Mar 19, 2025

scorbiere Mar 20, 2025

moelasmar Mar 20, 2025

moelasmar Mar 19, 2025

scorbiere Mar 20, 2025 •

edited

Loading

aws-cdk-automation commented Mar 21, 2025

mergify bot commented Mar 21, 2025

github-actions bot commented Mar 21, 2025

fix(events): now EventBus.grantPutEventsTo correctly handles service principals (under feature flag) #33729

fix(events): now EventBus.grantPutEventsTo correctly handles service principals (under feature flag) #33729

Conversation

scorbiere commented Mar 10, 2025

Issue #22080

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

aws-cdk-automation left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Mar 11, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

moelasmar Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

moelasmar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moelasmar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scorbiere Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation commented Mar 21, 2025

AWS CodeBuild CI Report

mergify bot commented Mar 21, 2025

github-actions bot commented Mar 21, 2025

fix(events): now `EventBus.grantPutEventsTo` correctly handles service principals (under feature flag) #33729

fix(events): now `EventBus.grantPutEventsTo` correctly handles service principals (under feature flag) #33729

aws-cdk-automation left a comment •

edited

Loading

codecov bot commented Mar 11, 2025 •

edited

Loading

moelasmar Mar 11, 2025 •

edited

Loading

scorbiere Mar 20, 2025 •

edited

Loading