From a9114c57855a5a8faf8e64de12fe6514f886e803 Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Mon, 20 Aug 2018 17:19:59 +0300 Subject: [PATCH] feat(aws-s3): additional grant* methods for buckets (#591) Add: - `grantPut` => s3:PutObject*, s3:Abort* (on objects) - `grantDelete` => s3:DeleteObject* (on objects) - `grantWrite` => put + delete (on bucket + objects) OTHER CHANGES - Fix 'npm run integ' in the 'aws-cdk-codepipeline' package, which would fail because a Topic with an email subscription could not be deleted. --- ...g.cfn-template-from-repo.lit.expected.json | 10 +-- .../test/integ.lambda-pipeline.expected.json | 2 +- .../test/integ.pipeline-cfn.expected.json | 6 +- ...g.pipeline-code-commit-build.expected.json | 4 +- .../integ.pipeline-code-commit.expected.json | 2 +- .../test/integ.pipeline-events.expected.json | 14 +-- .../test/integ.pipeline-events.ts | 1 - .../aws-lambda/test/inline.expected.json | 2 +- .../test/integ.inline.expected.json | 2 +- packages/@aws-cdk/aws-s3/lib/bucket.ts | 86 +++++++++++++++---- packages/@aws-cdk/aws-s3/lib/perms.ts | 12 ++- .../aws-s3/test/integ.bucket.expected.json | 2 +- packages/@aws-cdk/aws-s3/test/test.bucket.ts | 24 +++++- 13 files changed, 118 insertions(+), 49 deletions(-) diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.cfn-template-from-repo.lit.expected.json b/packages/@aws-cdk/aws-codepipeline/test/integ.cfn-template-from-repo.lit.expected.json index 91fde8381e96d..cac1870ff16d0 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.cfn-template-from-repo.lit.expected.json +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.cfn-template-from-repo.lit.expected.json @@ -31,8 +31,8 @@ "s3:GetObject*", "s3:GetBucket*", "s3:List*", - "s3:PutObject*", "s3:DeleteObject*", + "s3:PutObject*", "s3:Abort*" ], "Effect": "Allow", @@ -144,6 +144,7 @@ "Version": "1" }, "Configuration": { + "StackName": "OurStack", "ActionMode": "CHANGE_SET_REPLACE", "ChangeSetName": "StagedChangeSet", "TemplatePath": "SourceArtifact::template.yaml", @@ -153,8 +154,7 @@ "PipelineDeployPrepareChangesRoleD28C853C", "Arn" ] - }, - "StackName": "OurStack" + } }, "InputArtifacts": [ { @@ -185,9 +185,9 @@ "Version": "1" }, "Configuration": { + "StackName": "OurStack", "ActionMode": "CHANGE_SET_EXECUTE", - "ChangeSetName": "StagedChangeSet", - "StackName": "OurStack" + "ChangeSetName": "StagedChangeSet" }, "InputArtifacts": [], "Name": "ExecuteChanges", diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.lambda-pipeline.expected.json b/packages/@aws-cdk/aws-codepipeline/test/integ.lambda-pipeline.expected.json index 92fce302dca7f..b18461fbf3d30 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.lambda-pipeline.expected.json +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.lambda-pipeline.expected.json @@ -31,8 +31,8 @@ "s3:GetObject*", "s3:GetBucket*", "s3:List*", - "s3:PutObject*", "s3:DeleteObject*", + "s3:PutObject*", "s3:Abort*" ], "Effect": "Allow", diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-cfn.expected.json b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-cfn.expected.json index f9ad5997041fd..8067191fc54e1 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-cfn.expected.json +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-cfn.expected.json @@ -31,8 +31,8 @@ "s3:GetObject*", "s3:GetBucket*", "s3:List*", - "s3:PutObject*", "s3:DeleteObject*", + "s3:PutObject*", "s3:Abort*" ], "Effect": "Allow", @@ -156,6 +156,7 @@ "Version": "1" }, "Configuration": { + "StackName": "IntegTest-TestActionStack", "ActionMode": "CHANGE_SET_REPLACE", "ChangeSetName": "ChangeSetIntegTest", "TemplatePath": "SourceArtifact::test.yaml", @@ -164,8 +165,7 @@ "CfnChangeSetRole6F05F6FC", "Arn" ] - }, - "StackName": "IntegTest-TestActionStack" + } }, "InputArtifacts": [ { diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit-build.expected.json b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit-build.expected.json index c2d5d6a2ceb96..c929fc78a3225 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit-build.expected.json +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit-build.expected.json @@ -38,8 +38,8 @@ "s3:GetObject*", "s3:GetBucket*", "s3:List*", - "s3:PutObject*", "s3:DeleteObject*", + "s3:PutObject*", "s3:Abort*" ], "Effect": "Allow", @@ -291,8 +291,8 @@ "s3:GetObject*", "s3:GetBucket*", "s3:List*", - "s3:PutObject*", "s3:DeleteObject*", + "s3:PutObject*", "s3:Abort*" ], "Effect": "Allow", diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit.expected.json b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit.expected.json index 3c8affcb60d40..36b68b9f5b721 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit.expected.json +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit.expected.json @@ -38,8 +38,8 @@ "s3:GetObject*", "s3:GetBucket*", "s3:List*", - "s3:PutObject*", "s3:DeleteObject*", + "s3:PutObject*", "s3:Abort*" ], "Effect": "Allow", diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-events.expected.json b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-events.expected.json index 13462c828f0a8..3bd0090bbd708 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-events.expected.json +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-events.expected.json @@ -31,8 +31,8 @@ "s3:GetObject*", "s3:GetBucket*", "s3:List*", - "s3:PutObject*", "s3:DeleteObject*", + "s3:PutObject*", "s3:Abort*" ], "Effect": "Allow", @@ -464,8 +464,8 @@ "s3:GetObject*", "s3:GetBucket*", "s3:List*", - "s3:PutObject*", "s3:DeleteObject*", + "s3:PutObject*", "s3:Abort*" ], "Effect": "Allow", @@ -530,16 +530,6 @@ "MyTopic86869434": { "Type": "AWS::SNS::Topic" }, - "MyTopicbenisraeBF3B2D80": { - "Type": "AWS::SNS::Subscription", - "Properties": { - "Endpoint": "benisrae@amazon.com", - "Protocol": "email", - "TopicArn": { - "Ref": "MyTopic86869434" - } - } - }, "MyTopicPolicy12A5EC17": { "Type": "AWS::SNS::TopicPolicy", "Properties": { diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-events.ts b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-events.ts index e19276f6fcfd1..4c5fb3fbfaf19 100644 --- a/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-events.ts +++ b/packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-events.ts @@ -31,7 +31,6 @@ new codebuild.PipelineBuildAction(stack, 'CodeBuildAction', { }); const topic = new sns.Topic(stack, 'MyTopic'); -topic.subscribeEmail('benisrae', 'benisrae@amazon.com'); pipeline.onStateChange('OnPipelineStateChange').addTarget(topic, { textTemplate: 'Pipeline changed state to ', diff --git a/packages/@aws-cdk/aws-lambda/test/inline.expected.json b/packages/@aws-cdk/aws-lambda/test/inline.expected.json index 88c79a6c72492..fd1c0abac77c2 100644 --- a/packages/@aws-cdk/aws-lambda/test/inline.expected.json +++ b/packages/@aws-cdk/aws-lambda/test/inline.expected.json @@ -34,8 +34,8 @@ "s3:GetObject*", "s3:GetBucket*", "s3:List*", - "s3:PutObject*", "s3:DeleteObject*", + "s3:PutObject*", "s3:Abort*" ], "Effect": "Allow", diff --git a/packages/@aws-cdk/aws-lambda/test/integ.inline.expected.json b/packages/@aws-cdk/aws-lambda/test/integ.inline.expected.json index 93f3430530ed6..3018cc0994958 100644 --- a/packages/@aws-cdk/aws-lambda/test/integ.inline.expected.json +++ b/packages/@aws-cdk/aws-lambda/test/integ.inline.expected.json @@ -54,8 +54,8 @@ "s3:GetObject*", "s3:GetBucket*", "s3:List*", - "s3:PutObject*", "s3:DeleteObject*", + "s3:PutObject*", "s3:Abort*" ], "Effect": "Allow", diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 9ecb1da168a64..4f7a62e481c16 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -161,17 +161,59 @@ export abstract class BucketRef extends cdk.Construct { } /** - * Temporary API for granting read permissions for this bucket and it's - * contents to an IAM principal (Role/Group/User). + * Grant read permissions for this bucket and it's contents to an IAM + * principal (Role/Group/User). + * + * If encryption is used, permission to use the key to decrypt the contents + * of the bucket will also be granted to the same principal. * - * If an encryption key is used, permission to ues the key to decrypt the - * contents of the bucket will also be granted. + * @param identity The principal + * @param objectsKeyPattern Restrict the permission to a certain key pattern (default '*') */ public grantRead(identity?: iam.IPrincipal, objectsKeyPattern: any = '*') { - if (!identity) { - return; - } - this.grant(identity, objectsKeyPattern, perms.BUCKET_READ_ACTIONS, perms.KEY_READ_ACTIONS); + this.grant(identity, perms.BUCKET_READ_ACTIONS, perms.KEY_READ_ACTIONS, + this.bucketArn, + this.arnForObjects(objectsKeyPattern)); + } + + /** + * Grant write permissions to this bucket to an IAM principal. + * + * If encryption is used, permission to use the key to encrypt the contents + * of written files will also be granted to the same principal. + * + * @param identity The principal + * @param objectsKeyPattern Restrict the permission to a certain key pattern (default '*') + */ + public grantWrite(identity?: iam.IPrincipal, objectsKeyPattern: any = '*') { + this.grant(identity, perms.BUCKET_WRITE_ACTIONS, perms.KEY_WRITE_ACTIONS, + this.bucketArn, + this.arnForObjects(objectsKeyPattern)); + } + + /** + * Grants s3:PutObject* and s3:Abort* permissions for this bucket to an IAM principal. + * + * If encryption is used, permission to use the key to encrypt the contents + * of written files will also be granted to the same principal. + * @param identity The principal + * @param objectsKeyPattern Restrict the permission to a certain key pattern (default '*') + */ + public grantPut(identity?: iam.IPrincipal, objectsKeyPattern: any = '*') { + this.grant(identity, perms.BUCKET_PUT_ACTIONS, perms.KEY_WRITE_ACTIONS, + this.arnForObjects(objectsKeyPattern)); + } + + /** + * Grants s3:DeleteObject* permission to an IAM pricipal for objects + * in this bucket. + * + * @param identity The principal + * @param objectsKeyPattern Restrict the permission to a certain key pattern (default '*') + */ + public grantDelete(identity?: iam.IPrincipal, objectsKeyPattern: any = '*') { + this.grant(identity, perms.BUCKET_DELETE_ACTIONS, [], + this.arnForObjects(objectsKeyPattern)); } /** @@ -180,21 +222,31 @@ export abstract class BucketRef extends cdk.Construct { * * If an encryption key is used, permission to use the key for * encrypt/decrypt will also be granted. + * + * @param identity The principal + * @param objectsKeyPattern Restrict the permission to a certain key pattern (default '*') */ public grantReadWrite(identity?: iam.IPrincipal, objectsKeyPattern: any = '*') { - if (!identity) { - return; - } const bucketActions = perms.BUCKET_READ_ACTIONS.concat(perms.BUCKET_WRITE_ACTIONS); const keyActions = perms.KEY_READ_ACTIONS.concat(perms.KEY_WRITE_ACTIONS); - this.grant(identity, objectsKeyPattern, bucketActions, keyActions); - } - private grant(identity: iam.IPrincipal, objectsKeyPattern: any, bucketActions: string[], keyActions: string[]) { - const resources = [ + this.grant(identity, + bucketActions, + keyActions, this.bucketArn, - this.arnForObjects(objectsKeyPattern) - ]; + this.arnForObjects(objectsKeyPattern)); + } + + private grant(identity: iam.IPrincipal | undefined, + bucketActions: string[], + keyActions: string[], + resource: cdk.Arn, ...otherResources: cdk.Arn[]) { + + if (!identity) { + return; + } + + const resources = [ resource, ...otherResources ]; identity.addToPolicy(new cdk.PolicyStatement() .addResources(...resources) diff --git a/packages/@aws-cdk/aws-s3/lib/perms.ts b/packages/@aws-cdk/aws-s3/lib/perms.ts index d67360bc547a9..bb8bbcefbbfff 100644 --- a/packages/@aws-cdk/aws-s3/lib/perms.ts +++ b/packages/@aws-cdk/aws-s3/lib/perms.ts @@ -4,12 +4,20 @@ export const BUCKET_READ_ACTIONS = [ 's3:List*', ]; -export const BUCKET_WRITE_ACTIONS = [ +export const BUCKET_PUT_ACTIONS = [ 's3:PutObject*', - 's3:DeleteObject*', 's3:Abort*' ]; +export const BUCKET_DELETE_ACTIONS = [ + 's3:DeleteObject*' +]; + +export const BUCKET_WRITE_ACTIONS = [ + ...BUCKET_DELETE_ACTIONS, + ...BUCKET_PUT_ACTIONS +]; + export const KEY_READ_ACTIONS = [ 'kms:Decrypt', 'kms:DescribeKey', diff --git a/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json b/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json index ce11fc9d144e1..5f0af8b0f8c9c 100644 --- a/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json +++ b/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json @@ -114,8 +114,8 @@ "s3:GetObject*", "s3:GetBucket*", "s3:List*", - "s3:PutObject*", "s3:DeleteObject*", + "s3:PutObject*", "s3:Abort*" ], "Effect": "Allow", diff --git a/packages/@aws-cdk/aws-s3/test/test.bucket.ts b/packages/@aws-cdk/aws-s3/test/test.bucket.ts index ee2b199034c73..8c86ff387c6ca 100644 --- a/packages/@aws-cdk/aws-s3/test/test.bucket.ts +++ b/packages/@aws-cdk/aws-s3/test/test.bucket.ts @@ -609,8 +609,8 @@ export = { "s3:GetObject*", "s3:GetBucket*", "s3:List*", - "s3:PutObject*", "s3:DeleteObject*", + "s3:PutObject*", "s3:Abort*" ], "Effect": "Allow", @@ -763,8 +763,8 @@ export = { "s3:GetObject*", "s3:GetBucket*", "s3:List*", - "s3:PutObject*", "s3:DeleteObject*", + "s3:PutObject*", "s3:Abort*" ], "Effect": "Allow", @@ -826,6 +826,26 @@ export = { }, }, + 'more grants'(test: Test) { + const stack = new cdk.Stack(); + const bucket = new s3.Bucket(stack, 'MyBucket', { encryption: s3.BucketEncryption.Kms }); + const putter = new iam.User(stack, 'Putter'); + const writer = new iam.User(stack, 'Writer'); + const deleter = new iam.User(stack, 'Deleter'); + + bucket.grantPut(putter); + bucket.grantWrite(writer); + bucket.grantDelete(deleter); + + const resources = stack.toCloudFormation().Resources; + const actions = (id: string) => resources[id].Properties.PolicyDocument.Statement[0].Action; + + test.deepEqual(actions('WriterDefaultPolicyDC585BCE'), [ 's3:DeleteObject*', 's3:PutObject*', 's3:Abort*' ]); + test.deepEqual(actions('PutterDefaultPolicyAB138DD3'), [ 's3:PutObject*', 's3:Abort*' ]); + test.deepEqual(actions('DeleterDefaultPolicyCD33B8A0'), 's3:DeleteObject*'); + test.done(); + }, + 'cross-stack permissions'(test: Test) { const stackA = new cdk.Stack(); const bucketFromStackA = new s3.Bucket(stackA, 'MyBucket');