Skip to content

Commit

Permalink
fix(s3): grantDelete with KMS SSE (aws#7528)
Browse files Browse the repository at this point in the history
Added empty array check for keyActions. This will make sure that `grantDelete` will not create malformed policy when used with `KMS` key.

Added a new integ test to check CloudFormation will not error out during the deployment.

Fixes aws#4380
  • Loading branch information
shreyasdamle authored and karupanerura committed May 7, 2020
1 parent 6c56bd8 commit 413c448
Show file tree
Hide file tree
Showing 4 changed files with 182 additions and 1 deletion.
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-s3/lib/bucket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ abstract class BucketBase extends Resource implements IBucket {
});
}

if (this.encryptionKey) {
if (this.encryptionKey && keyActions && keyActions.length !== 0) {
this.encryptionKey.grant(grantee, ...keyActions);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
{
"Resources": {
"MyKey6AB29FA6": {
"Type": "AWS::KMS::Key",
"Properties": {
"KeyPolicy": {
"Statement": [
{
"Action": [
"kms:Create*",
"kms:Describe*",
"kms:Enable*",
"kms:List*",
"kms:Put*",
"kms:Update*",
"kms:Revoke*",
"kms:Disable*",
"kms:Get*",
"kms:Delete*",
"kms:ScheduleKeyDeletion",
"kms:CancelKeyDeletion",
"kms:GenerateDataKey",
"kms:TagResource",
"kms:UntagResource"
],
"Effect": "Allow",
"Principal": {
"AWS": {
"Fn::Join": [
"",
[
"arn:",
{
"Ref": "AWS::Partition"
},
":iam::",
{
"Ref": "AWS::AccountId"
},
":root"
]
]
}
},
"Resource": "*"
}
],
"Version": "2012-10-17"
}
},
"UpdateReplacePolicy": "Retain",
"DeletionPolicy": "Retain"
},
"Deleter1FEDC09A": {
"Type": "AWS::IAM::User"
},
"DeleterDefaultPolicyCD33B8A0": {
"Type": "AWS::IAM::Policy",
"Properties": {
"PolicyDocument": {
"Statement": [
{
"Action": "s3:DeleteObject*",
"Effect": "Allow",
"Resource": {
"Fn::Join": [
"",
[
{
"Fn::GetAtt": [
"MyBucketF68F3FF0",
"Arn"
]
},
"/*"
]
]
}
}
],
"Version": "2012-10-17"
},
"PolicyName": "DeleterDefaultPolicyCD33B8A0",
"Users": [
{
"Ref": "Deleter1FEDC09A"
}
]
}
},
"MyBucketF68F3FF0": {
"Type": "AWS::S3::Bucket",
"Properties": {
"BucketEncryption": {
"ServerSideEncryptionConfiguration": [
{
"ServerSideEncryptionByDefault": {
"KMSMasterKeyID": {
"Fn::GetAtt": [
"MyKey6AB29FA6",
"Arn"
]
},
"SSEAlgorithm": "aws:kms"
}
}
]
},
"BucketName": "my-bucket-physical-name-grant-delete"
},
"UpdateReplacePolicy": "Retain",
"DeletionPolicy": "Retain"
}
}
}
22 changes: 22 additions & 0 deletions packages/@aws-cdk/aws-s3/test/integ.bucket-grantdelete-kms.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#!/usr/bin/env node
import * as iam from '@aws-cdk/aws-iam';
import * as kms from '@aws-cdk/aws-kms';
import * as cdk from '@aws-cdk/core';
import * as s3 from '../lib';

const app = new cdk.App();

const stack = new cdk.Stack(app, 'aws-cdk-s3');

const key = new kms.Key(stack, 'MyKey');
const deleter = new iam.User(stack, 'Deleter');
const bucket = new s3.Bucket(stack, 'MyBucket', {
bucketName: 'my-bucket-physical-name-grant-delete',
encryptionKey: key,
encryption: s3.BucketEncryption.KMS,
});

// when
bucket.grantDelete(deleter);

app.synth();
44 changes: 44 additions & 0 deletions packages/@aws-cdk/aws-s3/test/test.bucket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1053,6 +1053,50 @@ export = {
test.done();
},

'grantDelete, with a KMS Key'(test: Test) {
// given
const stack = new cdk.Stack();
const key = new kms.Key(stack, 'MyKey');
const deleter = new iam.User(stack, 'Deleter');
const bucket = new s3.Bucket(stack, 'MyBucket', {
bucketName: 'my-bucket-physical-name',
encryptionKey: key,
encryption: s3.BucketEncryption.KMS,
});

// when
bucket.grantDelete(deleter);

// then
expect(stack).to(haveResourceLike('AWS::IAM::Policy', {
'PolicyDocument': {
'Statement': [
{
'Action': 's3:DeleteObject*',
'Effect': 'Allow',
'Resource': {
'Fn::Join': [
'',
[
{
'Fn::GetAtt': [
'MyBucketF68F3FF0',
'Arn',
],
},
'/*',
],
],
},
},
],
'Version': '2012-10-17',
},
}));

test.done();
},

'cross-stack permissions': {
'in the same account and region'(test: Test) {
const app = new cdk.App();
Expand Down

0 comments on commit 413c448

Please sign in to comment.