Skip to content

Commit

Permalink
fix(s3-notifications): notifications allowed with imported kms keys (#…
Browse files Browse the repository at this point in the history
…18989)

----
fixes: #18988

If you add an sqs queue as notification target to an s3 bucket, and this sqs queue is encrypted with an imported kms IKey, the stack won't synthesize. Instead of failing, it should warn the user, that it can not ensure the correct kms key policy permissions.

This fix will solve this.

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
markussiebert committed Feb 22, 2022
1 parent 6ec1005 commit 7441418
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 3 deletions.
12 changes: 12 additions & 0 deletions packages/@aws-cdk/aws-s3-notifications/README.md
Expand Up @@ -26,6 +26,18 @@ const topic = new sns.Topic(this, 'Topic');
bucket.addEventNotification(s3.EventType.OBJECT_CREATED_PUT, new s3n.SnsDestination(topic));
```

The following example shows how to send a notification to an SQS queue
when an object is created in an S3 bucket:

```ts
import * as sqs from '@aws-cdk/aws-sqs';

const bucket = new s3.Bucket(this, 'Bucket');
const queue = new sqs.Queue(this, 'Queue');

bucket.addEventNotification(s3.EventType.OBJECT_CREATED_PUT, new s3n.SqsDestination(queue));
```

The following example shows how to send a notification to a Lambda function when an object is created in an S3 bucket:

```ts
Expand Down
12 changes: 10 additions & 2 deletions packages/@aws-cdk/aws-s3-notifications/lib/sqs.ts
@@ -1,6 +1,10 @@
import * as iam from '@aws-cdk/aws-iam';
import * as s3 from '@aws-cdk/aws-s3';
import * as sqs from '@aws-cdk/aws-sqs';
import { Annotations } from '@aws-cdk/core';

// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
// eslint-disable-next-line no-duplicate-imports, import/order
import { Construct } from '@aws-cdk/core';

/**
Expand All @@ -24,11 +28,15 @@ export class SqsDestination implements s3.IBucketNotificationDestination {
// if this queue is encrypted, we need to allow S3 to read messages since that's how
// it verifies that the notification destination configuration is valid.
if (this.queue.encryptionMasterKey) {
this.queue.encryptionMasterKey.addToResourcePolicy(new iam.PolicyStatement({
const statement = new iam.PolicyStatement({
principals: [new iam.ServicePrincipal('s3.amazonaws.com')],
actions: ['kms:GenerateDataKey*', 'kms:Decrypt'],
resources: ['*'],
}), /* allowNoOp */ false);
});
const addResult = this.queue.encryptionMasterKey.addToResourcePolicy(statement, /* allowNoOp */ true);
if (!addResult.statementAdded) {
Annotations.of(this.queue.encryptionMasterKey).addWarning(`Can not change key policy of imported kms key. Ensure that your key policy contains the following permissions: \n${JSON.stringify(statement.toJSON(), null, 2)}`);
}
}

return {
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-s3-notifications/package.json
Expand Up @@ -80,6 +80,7 @@
},
"dependencies": {
"@aws-cdk/aws-iam": "0.0.0",
"@aws-cdk/aws-kms": "0.0.0",
"@aws-cdk/aws-lambda": "0.0.0",
"@aws-cdk/aws-s3": "0.0.0",
"@aws-cdk/aws-sns": "0.0.0",
Expand All @@ -90,6 +91,7 @@
"homepage": "https://github.com/aws/aws-cdk",
"peerDependencies": {
"@aws-cdk/aws-iam": "0.0.0",
"@aws-cdk/aws-kms": "0.0.0",
"@aws-cdk/aws-lambda": "0.0.0",
"@aws-cdk/aws-s3": "0.0.0",
"@aws-cdk/aws-sns": "0.0.0",
Expand Down
27 changes: 26 additions & 1 deletion packages/@aws-cdk/aws-s3-notifications/test/queue.test.ts
@@ -1,4 +1,5 @@
import { Match, Template } from '@aws-cdk/assertions';
import { Match, Template, Annotations } from '@aws-cdk/assertions';
import * as kms from '@aws-cdk/aws-kms';
import * as s3 from '@aws-cdk/aws-s3';
import * as sqs from '@aws-cdk/aws-sqs';
import { Stack } from '@aws-cdk/core';
Expand Down Expand Up @@ -96,3 +97,27 @@ test('if the queue is encrypted with a custom kms key, the key resource policy i
},
});
});

test('if the queue is encrypted with a imported kms key, printout warning', () => {
const stack = new Stack();
const bucket = new s3.Bucket(stack, 'Bucket');
const key = kms.Key.fromKeyArn(stack, 'ImportedKey', 'arn:aws:kms:us-west-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab');
const queue = new sqs.Queue(stack, 'Queue', {
encryption: sqs.QueueEncryption.KMS,
encryptionMasterKey: key,
});

bucket.addObjectCreatedNotification(new notif.SqsDestination(queue));

Annotations.fromStack(stack).hasWarning('/Default/ImportedKey', `Can not change key policy of imported kms key. Ensure that your key policy contains the following permissions: \n${JSON.stringify({
Action: [
'kms:GenerateDataKey*',
'kms:Decrypt',
],
Effect: 'Allow',
Principal: {
Service: 's3.amazonaws.com',
},
Resource: '*',
}, null, 2)}`);
});

0 comments on commit 7441418

Please sign in to comment.