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*
aws · Nov 2, 2022 · a36f2f0 · a36f2f0
1 parent ea9ab4b
commit a36f2f0
Show file tree

Hide file tree

Showing 12 changed files with 187 additions and 20 deletions.
diff --git a/packages/@aws-cdk/aws-events-targets/lib/log-group.ts b/packages/@aws-cdk/aws-events-targets/lib/log-group.ts
@@ -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

diff --git a/packages/@aws-cdk/aws-events-targets/lib/sqs.ts b/packages/@aws-cdk/aws-events-targets/lib/sqs.ts
@@ -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';
 
 /**
@@ -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);

diff --git a/packages/@aws-cdk/aws-events-targets/package.json b/packages/@aws-cdk/aws-events-targets/package.json
@@ -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",
@@ -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",

diff --git a/packages/@aws-cdk/aws-events-targets/test/logs/log-group.test.ts b/packages/@aws-cdk/aws-events-targets/test/logs/log-group.test.ts
@@ -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

diff --git a/...ets/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.assets.json b/...ets/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.assets.json
@@ -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}"
         }
       }

diff --git a/...s/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.template.json b/...s/test/sqs/integ.sqs-event-rule-target.js.snapshot/aws-cdk-sqs-event-target.template.json
@@ -35,6 +35,13 @@
         "kms:GenerateDataKey*",
         "kms:ReEncrypt*"
        ],
+       "Condition": {
+        "StringEquals": {
+         "aws:SourceAccount": {
+          "Ref": "AWS::AccountId"
+         }
+        }
+       },
        "Effect": "Allow",
        "Principal": {
         "Service": "events.amazonaws.com"
@@ -98,6 +105,13 @@
         "sqs:GetQueueUrl",
         "sqs:SendMessage"
        ],
+       "Condition": {
+        "StringEquals": {
+         "aws:SourceAccount": {
+          "Ref": "AWS::AccountId"
+         }
+        }
+       },
        "Effect": "Allow",
        "Principal": {
         "Service": "events.amazonaws.com"

diff --git a/...ages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/cdk.out b/...ages/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/cdk.out
@@ -1 +1 @@
-{"version":"20.0.0"}
+{"version":"21.0.0"}
diff --git a/...s/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/integ.json b/...s/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/integ.json
@@ -1,5 +1,5 @@
 {
-  "version": "20.0.0",
+  "version": "21.0.0",
   "testCases": {
     "integ.sqs-event-rule-target": {
       "stacks": [

diff --git a/...aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/manifest.json b/...aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/manifest.json
@@ -1,5 +1,5 @@
 {
-  "version": "20.0.0",
+  "version": "21.0.0",
   "artifacts": {
     "Tree": {
       "type": "cdk:tree",
@@ -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": [

diff --git a/...es/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/tree.json b/...es/@aws-cdk/aws-events-targets/test/sqs/integ.sqs-event-rule-target.js.snapshot/tree.json
@@ -9,7 +9,7 @@
         "path": "Tree",
         "constructInfo": {
           "fqn": "constructs.Construct",
-          "version": "10.1.85"
+          "version": "10.1.140"
         }
       },
       "aws-cdk-sqs-event-target": {
@@ -58,6 +58,13 @@
                             "kms:GenerateDataKey*",
                             "kms:ReEncrypt*"
                           ],
+                          "Condition": {
+                            "StringEquals": {
+                              "aws:SourceAccount": {
+                                "Ref": "AWS::AccountId"
+                              }
+                            }
+                          },
                           "Effect": "Allow",
                           "Principal": {
                             "Service": "events.amazonaws.com"
@@ -165,6 +172,13 @@
                                 "sqs:GetQueueUrl",
                                 "sqs:SendMessage"
                               ],
+                              "Condition": {
+                                "StringEquals": {
+                                  "aws:SourceAccount": {
+                                    "Ref": "AWS::AccountId"
+                                  }
+                                }
+                              },
                               "Effect": "Allow",
                               "Principal": {
                                 "Service": "events.amazonaws.com"
@@ -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"
     }
   }
 }
diff --git a/packages/@aws-cdk/aws-events-targets/test/sqs/sqs.test.ts b/packages/@aws-cdk/aws-events-targets/test/sqs/sqs.test.ts
@@ -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', () => {
@@ -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');

diff --git a/packages/@aws-cdk/cx-api/lib/features.ts b/packages/@aws-cdk/cx-api/lib/features.ts
@@ -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
  *
@@ -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,
 };
 
 /**