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

SQS: support bucket notifications #564

Merged
merged 1 commit into from
Aug 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions packages/@aws-cdk/aws-kms/lib/key.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Construct, DeletionPolicy, Output, PolicyDocument, PolicyStatement } from '@aws-cdk/cdk';
import { Construct, DeletionPolicy, Output, PolicyDocument, PolicyStatement, resolve } from '@aws-cdk/cdk';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point I would import cdk = require('@aws-cdk/cdk');.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll forgive me - I forgot to push this change and merged. We'll clean this up later.

import { EncryptionKeyAlias } from './alias';
import { cloudformation, KeyArn } from './kms.generated';

Expand Down Expand Up @@ -54,10 +54,15 @@ export abstract class EncryptionKeyRef extends Construct {

/**
* Adds a statement to the KMS key resource policy.
* @param statement The policy statement to add
* @param allowNoOp If this is set to `false` and there is no policy
* defined (i.e. external key), the operation will fail. Otherwise, it will
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the text for the @param, or your Sphinx output will be all funked-up!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What?! how is this related to my sphinx output?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my sphinx output:

   .. py:method:: addToResourcePolicy(statement, [allowNoOp])

      Adds a statement to the KMS key resource policy.


      :param statement: The policy statement to add
      :type statement: :py:class:`@aws-cdk/cdk.PolicyStatement`
      :param allowNoOp: If this is set to `false` and there is no policy defined (i.e. external key), the operation will fail. Otherwise, it will no-op.
      :type allowNoOp: boolean or undefined

Looks fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird. I had bunch of cases where this generated invalid rst. Maybe typescript changed how it parses jsdoc tags?

* no-op.
*/
public addToResourcePolicy(statement: PolicyStatement) {
public addToResourcePolicy(statement: PolicyStatement, allowNoOp = true) {
if (!this.policy) {
return;
if (allowNoOp) { return; }
throw new Error(`Unable to add statement to IAM resource policy for KMS key: ${JSON.stringify(resolve(this.keyArn))}`);
}

this.policy.addStatement(statement);
Expand Down
28 changes: 27 additions & 1 deletion packages/@aws-cdk/aws-kms/test/test.key.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { exactlyMatchTemplate, expect } from '@aws-cdk/assert';
import { App, PolicyDocument, PolicyStatement, Stack } from '@aws-cdk/cdk';
import { Test } from 'nodeunit';
import { EncryptionKey } from '../lib';
import { EncryptionKey, KeyArn } from '../lib';

export = {
'default key'(test: Test) {
Expand Down Expand Up @@ -349,5 +349,31 @@ export = {
});

test.done();
},

'addToResourcePolicy allowNoOp and there is no policy': {
'succeed if set to true (default)'(test: Test) {
const stack = new Stack();

const key = EncryptionKey.import(stack, 'Imported', { keyArn: new KeyArn('foo/bar') });

key.addToResourcePolicy(new PolicyStatement().addResource('*').addAction('*'));

test.done();
},

'fails if set to false'(test: Test) {

const stack = new Stack();

const key = EncryptionKey.import(stack, 'Imported', { keyArn: new KeyArn('foo/bar') });

test.throws(() =>
key.addToResourcePolicy(new PolicyStatement().addResource('*').addAction('*'), /* allowNoOp */ false),
'Unable to add statement to IAM resource policy for KMS key: "foo/bar"');

test.done();

}
}
};
85 changes: 77 additions & 8 deletions packages/@aws-cdk/aws-sqs/lib/queue-ref.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
import { Construct, Output, PolicyStatement, Token } from '@aws-cdk/cdk';
import kms = require('@aws-cdk/aws-kms');
import s3n = require('@aws-cdk/aws-s3-notifications');
import cdk = require('@aws-cdk/cdk');
import { QueuePolicy } from './policy';
import { QueueArn } from './sqs.generated';

/**
* Reference to a new or existing Amazon SQS queue
*/
export abstract class QueueRef extends Construct {
export abstract class QueueRef extends cdk.Construct implements s3n.IBucketNotificationDestination {
/**
* Import an existing queue
*/
public static import(parent: Construct, name: string, props: QueueRefProps) {
public static import(parent: cdk.Construct, name: string, props: QueueRefProps) {
new ImportedQueue(parent, name, props);
}

Expand All @@ -23,6 +25,11 @@ export abstract class QueueRef extends Construct {
*/
public abstract readonly queueUrl: QueueUrl;

/**
* If this queue is server-side encrypted, this is the KMS encryption key.
*/
public abstract readonly encryptionMasterKey?: kms.EncryptionKeyRef;

/**
* Controls automatic creation of policy objects.
*
Expand All @@ -32,13 +39,21 @@ export abstract class QueueRef extends Construct {

private policy?: QueuePolicy;

/**
* The set of S3 bucket IDs that are allowed to send notifications to this queue.
*/
private readonly notifyingBuckets = new Set<string>();

/**
* Export a queue
*/
public export(): QueueRefProps {
return {
queueArn: new Output(this, 'QueueArn', { value: this.queueArn }).makeImportValue(),
queueUrl: new Output(this, 'QueueUrl', { value: this.queueUrl }).makeImportValue(),
queueArn: new cdk.Output(this, 'QueueArn', { value: this.queueArn }).makeImportValue(),
queueUrl: new cdk.Output(this, 'QueueUrl', { value: this.queueUrl }).makeImportValue(),
keyArn: this.encryptionMasterKey
? new cdk.Output(this, 'KeyArn', { value: this.encryptionMasterKey.keyArn }).makeImportValue()
: undefined
};
}

Expand All @@ -49,7 +64,7 @@ export abstract class QueueRef extends Construct {
* will be automatically created upon the first call to `addToPolicy`. If
* the queue is improted (`Queue.import`), then this is a no-op.
*/
public addToResourcePolicy(statement: PolicyStatement) {
public addToResourcePolicy(statement: cdk.PolicyStatement) {
if (!this.policy && this.autoCreatePolicy) {
this.policy = new QueuePolicy(this, 'Policy', { queues: [ this ] });
}
Expand All @@ -59,14 +74,61 @@ export abstract class QueueRef extends Construct {
}
}

/**
* Allows using SQS queues as destinations for bucket notifications.
* Use `bucket.onEvent(event, queue)` to subscribe.
* @param bucketArn The ARN of the notifying bucket.
* @param bucketId A unique ID for the notifying bucket.
*/
public asBucketNotificationDestination(bucketArn: cdk.Arn, bucketId: string): s3n.BucketNotificationDestinationProps {
if (!this.notifyingBuckets.has(bucketId)) {
this.addToResourcePolicy(new cdk.PolicyStatement()
.addServicePrincipal('s3.amazonaws.com')
.addAction('sqs:SendMessage')
.addResource(this.queueArn)
.addCondition('ArnLike', { 'aws:SourceArn': bucketArn }));

// 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.
// by setting allowNoOp to false, we ensure that only custom keys that we can actually
// control access to can be used here as described in:
// https://docs.aws.amazon.com/AmazonS3/latest/dev/ways-to-add-notification-config-to-bucket.html
if (this.encryptionMasterKey) {
this.encryptionMasterKey.addToResourcePolicy(new cdk.PolicyStatement()
.addServicePrincipal('s3.amazonaws.com')
.addAction('kms:GenerateDataKey')
.addAction('kms:Decrypt')
.addResource('*'), /* allowNoOp */ false);
}

this.notifyingBuckets.add(bucketId);
}

return {
arn: this.queueArn,
type: s3n.BucketNotificationDestinationType.Queue
};
}
}

/**
* Reference to a queue
*/
export interface QueueRefProps {
/**
* The ARN of the queue.
*/
queueArn: QueueArn;

/**
* The URL of the queue.
*/
queueUrl: QueueUrl;

/**
* KMS encryption key, if this queue is server-side encrypted by a KMS key.
*/
keyArn?: kms.KeyArn;
}

/**
Expand All @@ -75,18 +137,25 @@ export interface QueueRefProps {
class ImportedQueue extends QueueRef {
public readonly queueArn: QueueArn;
public readonly queueUrl: QueueUrl;
public readonly encryptionMasterKey?: kms.EncryptionKeyRef;

protected readonly autoCreatePolicy = false;

constructor(parent: Construct, name: string, props: QueueRefProps) {
constructor(parent: cdk.Construct, name: string, props: QueueRefProps) {
super(parent, name);
this.queueArn = props.queueArn;
this.queueUrl = props.queueUrl;

if (props.keyArn) {
this.encryptionMasterKey = kms.EncryptionKey.import(this, 'Key', {
keyArn: props.keyArn
});
}
}
}

/**
* URL of a queue
*/
export class QueueUrl extends Token {
export class QueueUrl extends cdk.Token {
}
19 changes: 14 additions & 5 deletions packages/@aws-cdk/aws-sqs/lib/queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,11 @@ export class Queue extends QueueRef {
*/
public readonly queueUrl: QueueUrl;

/**
* If this queue is encrypted, this is the KMS key.
*/
public encryptionMasterKey?: kms.EncryptionKeyRef;

protected readonly autoCreatePolicy = true;

constructor(parent: cdk.Construct, name: string, props: QueueProps = {}) {
Expand Down Expand Up @@ -255,30 +260,34 @@ export class Queue extends QueueRef {
}

private determineEncryptionProps(props: QueueProps): EncryptionProps {
const encryption = props.encryption || QueueEncryption.Unencrypted;
let encryption = props.encryption || QueueEncryption.Unencrypted;

if (encryption !== QueueEncryption.Kms && props.encryptionMasterKey) {
throw new Error("Encryption key is specified, so 'encryption' must be set to 'Kms'.");
encryption = QueueEncryption.Kms; // KMS is implied by specifying an encryption key
}

if (encryption === QueueEncryption.Unencrypted) {
return {};
}

if (encryption === QueueEncryption.KmsManaged) {
this.encryptionMasterKey = kms.EncryptionKey.import(this, 'Key', {
keyArn: new kms.KeyArn('alias/aws/sqs')
});

return {
kmsMasterKeyId: 'alias/aws/sqs',
kmsDataKeyReusePeriodSeconds: props.dataKeyReuseSec
};
}

if (props.encryption === QueueEncryption.Kms) {
const encryptionKey = props.encryptionMasterKey || new kms.EncryptionKey(this, 'Key', {
if (encryption === QueueEncryption.Kms) {
this.encryptionMasterKey = props.encryptionMasterKey || new kms.EncryptionKey(this, 'Key', {
description: `Created by ${this.path}`
});

return {
kmsMasterKeyId: encryptionKey.keyArn,
kmsMasterKeyId: this.encryptionMasterKey.keyArn,
kmsDataKeyReusePeriodSeconds: props.dataKeyReuseSec
};
}
Expand Down
4 changes: 3 additions & 1 deletion packages/@aws-cdk/aws-sqs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,12 @@
"cdk-build-tools": "^0.8.1",
"cdk-integ-tools": "^0.8.1",
"cfn2ts": "^0.8.1",
"pkglint": "^0.8.1"
"pkglint": "^0.8.1",
"@aws-cdk/aws-s3": "^0.8.1"
},
"dependencies": {
"@aws-cdk/aws-kms": "^0.8.1",
"@aws-cdk/aws-s3-notifications": "^0.8.1",
"@aws-cdk/cdk": "^0.8.1"
},
"homepage": "https://github.com/awslabs/aws-cdk"
Expand Down
Loading