(aws-events-targets): Missing permissions on the DLQ's CMK

arnulfojr commented 2 years ago

Describe the bug

When adding a CMK-SSE enabled SQS Queue as a DLQ to a Rule, the abstraction adds the required permission to the SQS Queue resource policy for EventBridge to sendMessages but does not consider that the DLQ can be encrypted and therefore not applying the required KMS permissions to the associated KMS Key.

The required permission are documented in the official documentation: https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-key-management.html#sqs-what-permissions-for-sse

For example, adding a Lambda as a Target: https://github.com/aws/aws-cdk/blob/9cee4d00539bed61872126a07bdc14aedba041d8/packages/%40aws-cdk/aws-events-targets/lib/lambda.ts#L35-L37

would call the addToDeadLetterQueueResourcePolicy function: https://github.com/aws/aws-cdk/blob/9cee4d00539bed61872126a07bdc14aedba041d8/packages/%40aws-cdk/aws-events-targets/lib/util.ts#L116-L138

This is not exclusive to Lambda as a target (as it affects the Rule's DLQ rather than the target's DLQ), based on the quick look up this affects the following targets that have a DLQ that's SSE-enabled.

Lambda
Api Destinations
Api Gateway
Bath
CodeBuild
EventBus
SQS
State Machines

Expected Behavior

The Target abstraction automatically adds the required permissions on the DLQ (SQS queue) and in the CMK Key as documented in the AWS documentation - https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-key-management.html#sqs-what-permissions-for-sse

Current Behavior

Grants events.amazon.com permission to SendMessage in the SQS queue but does not grant events.amazon.com the required KMS permissions.

Reproduction Steps

export class SomeConstruct extends cdk.Construct {
  public readonly rule: events.Rule;
  public readonly dlq: sqs.IQueue;
  public readonly dlqKey: kms.IKey;

  constructor(scope: cdk.Stack, id: string, props: SomeConstructProps) {
    super(scope, id);

    this.dlqKey = new kms.Key(this, "RuleDLQKey", {
      enableKeyRotation: true,
      description: "SSE Key for the Rule DLQ",
    });

    this.dlq = new sqs.Queue(this, "RuleDLQ", {
      encryption: sqs.QueueEncryption.KMS,
      encryptionMasterKey: this.dlqKey,
      retentionPeriod: cdk.Duration.days(7),
    });

    this.rule = new events.Rule(this, "ARule", {
      description: "Forwards CW Alarm events to a Lambda",
      enabled: true,
      eventBus: props.eventBus,
    });

    this.rule.addEventPattern({ /* some pattern */  });

    this.rule.addTarget(
      new eventTargets.LambdaFunction(props.someFunction, {
        retryAttempts: 185,
        maxEventAge: cdk.Duration.hours(6),
        deadLetterQueue: this.dlq,
      })
    );
  }
}

Possible Solution

A: Use the Grant API to grant sendMessage permissions to the SQS Queue: https://github.com/aws/aws-cdk/blob/9cee4d00539bed61872126a07bdc14aedba041d8/packages/%40aws-cdk/aws-sqs/lib/queue-base.ts#L202,L212

The downfall is that it adds other operations.

B: In addToDeadLetterQueueResourcePolicy add then required check

if (queue.encryptionMasterKey) {
   queue.encryptionMasterKey.grant(new iam.ServicePrincipal("events.amazon.com"), "kms:Decrypt", "kms:GenerateDataKey")
}

or similar

Additional Information/Context

No response

CDK CLI Version

1.155.0

Framework Version

No response

Node.js Version

14

OS

Linux

Language

Typescript, Python, .NET, Java, Go

Language Version

No response

Other information

No response

rix0rrr commented 2 years ago

How confident are you that the permissions to write to the DLQ need to be granted to events.amazonaws.com (as opposed to sqs.amazonaws.com, which to my mental model is the service that's writing to the DLQ).

All in all, I wonder if we're not better off using kms:ViaService in all keys that are used for SSE.

           - Action:
              - kms:Decrypt
              - kms:DescribeKey
              - kms:Encrypt
              - kms:ReEncrypt*
              - kms:GenerateDataKey*
            Effect: Allow
            Principal:
              AWS: "*"
            Resource: "*"
            Condition:
              StringEquals:
                kms:ViaService: sqs.amazonaws.com

arnulfojr commented 2 years ago

I'm very certain that the service sends the events to the configured SQS queue. In my code sample above, EventBridge will do the SendMessage (notice how the sourceArn is set to the EventBridge Rule) These docs explain it https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-rule-dlq.html#eb-dlq-perms

Yes the kms:ViaService context key should do the trick as well. I think that could do the implementation slightly easier as we would just need to pass the Service Principal to grant access to.

aws / aws-cdk