-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(pipes-alpha): support for customer-managed KMS keys to encrypt pipe data #33546
Changes from all commits
66a25cc
f417ef8
ba2c374
eca5cbe
90bdaa0
9489730
181b4ab
c053604
7da16d5
140ddfd
0a4c434
8ce477f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import { IResource, Resource, Stack } from 'aws-cdk-lib'; | ||
import { IRole, Role, ServicePrincipal } from 'aws-cdk-lib/aws-iam'; | ||
import { IResource, Resource, Stack, ValidationError } from 'aws-cdk-lib'; | ||
import { ArnPrincipal, IRole, PolicyStatement, Role, ServicePrincipal } from 'aws-cdk-lib/aws-iam'; | ||
import * as kms from 'aws-cdk-lib/aws-kms'; | ||
import { CfnPipe } from 'aws-cdk-lib/aws-pipes'; | ||
import { addConstructMetadata } from 'aws-cdk-lib/core/lib/metadata-resource'; | ||
import { Construct } from 'constructs'; | ||
|
@@ -165,6 +166,13 @@ export interface PipeProps { | |
readonly tags?: { | ||
[key: string]: string; | ||
}; | ||
|
||
/** | ||
* The AWS KMS customer managed key to encrypt pipe data. | ||
* | ||
* @default undefined - AWS managed key is used | ||
*/ | ||
readonly kmsKey?: kms.IKey; | ||
} | ||
|
||
abstract class PipeBase extends Resource implements IPipe { | ||
|
@@ -279,10 +287,38 @@ export class Pipe extends PipeBase { | |
return { ...currentLogConfiguration, ...additionalLogConfiguration }; | ||
}, initialLogConfiguration); | ||
|
||
if (props.kmsKey) { | ||
if (!props.pipeName) { | ||
throw new ValidationError('`pipeName` is required when specifying a `kmsKey` prop.', this); | ||
} | ||
// Add permissions to the KMS key | ||
// see https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-encryption-pipes-cmkey.html#eb-encryption-key-policy-pipe | ||
props.kmsKey.addToResourcePolicy( | ||
new PolicyStatement({ | ||
actions: ['kms:Decrypt', 'kms:DescribeKey', 'kms:GenerateDataKey'], | ||
resources: ['*'], | ||
principals: [new ArnPrincipal(this.pipeRole.roleArn)], | ||
conditions: { | ||
'ArnLike': { | ||
'kms:EncryptionContext:aws:pipe:arn': Stack.of(this).formatArn({ | ||
service: 'pipes', | ||
resource: 'pipe', | ||
resourceName: props.pipeName, | ||
}), | ||
}, | ||
'ForAnyValue:StringEquals': { | ||
'kms:EncryptionContextKeys': [ | ||
'aws:pipe:arn', | ||
], | ||
}, | ||
}, | ||
}), | ||
); | ||
} | ||
|
||
/** | ||
* Pipe resource | ||
*/ | ||
|
||
const resource = new CfnPipe(this, 'Resource', { | ||
name: props.pipeName, | ||
description: props.description, | ||
|
@@ -295,6 +331,7 @@ export class Pipe extends PipeBase { | |
targetParameters: target.targetParameters, | ||
desiredState: props.desiredState, | ||
logConfiguration: logConfiguration, | ||
kmsKeyIdentifier: props.kmsKey?.keyArn, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a question—did it work correctly even without setting a key policy? When I previously attempted to contribute to this, I encountered an error without a key policy, but I couldn't come up with a proper way to configure it and had to give up at the time. It might have been my misunderstanding back then, or something might have changed with an update. If it worked fine, please just ignore this question. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are absolutely right.. I misunderstood that this implementation would be work fine. Thank you for your great suggestion. How about adding the restriction that makes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for confirming. So that's how it is... Personally, I've been considering the following three options:
Based on the above, I believe option 1 or option 2 would be good, and I think option 1 would be better to avoid a breaking change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hadn't thought of option 2. I think implementing option 2 with a feature flag is also a good idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've just noticed that the feature flag is not essential because this is alpha module.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also think Options2 is smarter, but since the impact is significant, I'd like to wait for the maintainer's opinion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the deep dive. I think Option 1 makes sense to me. Can you help me understand Option 2 + feature flag solution? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the clarification.
You're absolutely right! When setting a CMK for stacks under an App where the function flag is not enabled, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the quick response. Given the above discussion, I'll remove the |
||
tags: props.tags, | ||
}); | ||
|
||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed several unnecessary spaces.