From ef999725ddba35aa38976cf9a2264ffb36e5fc9c Mon Sep 17 00:00:00 2001 From: James Fleming Date: Thu, 27 Aug 2020 16:30:12 +0000 Subject: [PATCH 1/3] fix(custom-resources): resource deletion no longer loses permissions --- ...nteg.user-pool-domain-cfdist.expected.json | 22 ++++++++- .../test/integ.imagescan.expected.json | 27 ++++++++++- .../globalaccelerator-security-group.test.ts | 1 + .../aws-custom-resource.ts | 21 +++++++-- .../integ.aws-custom-resource.expected.json | 45 ++++++++++++++++++- 5 files changed, 108 insertions(+), 8 deletions(-) diff --git a/packages/@aws-cdk/aws-cognito/test/integ.user-pool-domain-cfdist.expected.json b/packages/@aws-cdk/aws-cognito/test/integ.user-pool-domain-cfdist.expected.json index 13f653712c7cd..4c1a188e9196b 100644 --- a/packages/@aws-cdk/aws-cognito/test/integ.user-pool-domain-cfdist.expected.json +++ b/packages/@aws-cdk/aws-cognito/test/integ.user-pool-domain-cfdist.expected.json @@ -78,7 +78,10 @@ "InstallLatestAwsSdk": true }, "UpdateReplacePolicy": "Delete", - "DeletionPolicy": "Delete" + "DeletionPolicy": "Delete", + "DependsOn": [ + "UserPoolDomainCloudFrontDomainNameCustomResourcePolicy7DE54188" + ] }, "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2": { "Type": "AWS::IAM::Role", @@ -111,6 +114,23 @@ ] } }, + "UserPoolDomainCloudFrontDomainNameCustomResourcePolicy7DE54188": { + "Type": "AWS::IAM::Policy", + "Properties": { + "PolicyDocument": { + "Statement": [ + { + "Action":"cognito-idp:DescribeUserPoolDomain", + "Effect":"Allow", + "Resource": "*" + } + ], + "Version": "2012-10-17" + }, + "PolicyName": "UserPoolDomainCloudFrontDomainNameCustomResourcePolicy7DE54188", + "Roles": [{"Ref":"AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2"}] + } + }, "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleDefaultPolicyD28E1A5E": { "Type": "AWS::IAM::Policy", "Properties": { diff --git a/packages/@aws-cdk/aws-ecr/test/integ.imagescan.expected.json b/packages/@aws-cdk/aws-ecr/test/integ.imagescan.expected.json index 2c976e6327438..fbfaeb93caa5c 100644 --- a/packages/@aws-cdk/aws-ecr/test/integ.imagescan.expected.json +++ b/packages/@aws-cdk/aws-ecr/test/integ.imagescan.expected.json @@ -77,7 +77,32 @@ "InstallLatestAwsSdk": true }, "UpdateReplacePolicy": "Delete", - "DeletionPolicy": "Delete" + "DeletionPolicy": "Delete", + "DependsOn": [ + "RepoImageScanOnPushCustomResourcePolicy556E941E" + ] + }, + "RepoImageScanOnPushCustomResourcePolicy556E941E": { + "Type": "AWS::IAM::Policy", + "Properties": { + "PolicyDocument": { + "Statement": [ + { + "Action":"ecr:PutImageScanningConfiguration", + "Effect":"Allow", + "Resource": { + "Fn::GetAtt": [ + "Repo02AC86CF", + "Arn" + ] + } + } + ], + "Version": "2012-10-17" + }, + "PolicyName": "RepoImageScanOnPushCustomResourcePolicy556E941E", + "Roles": [{"Ref":"AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2"}] + } }, "RepoImageScanComplete7BC71935": { "Type": "AWS::Events::Rule", diff --git a/packages/@aws-cdk/aws-globalaccelerator/test/globalaccelerator-security-group.test.ts b/packages/@aws-cdk/aws-globalaccelerator/test/globalaccelerator-security-group.test.ts index 053e4da712caa..c7eb0d87f8fa8 100644 --- a/packages/@aws-cdk/aws-globalaccelerator/test/globalaccelerator-security-group.test.ts +++ b/packages/@aws-cdk/aws-globalaccelerator/test/globalaccelerator-security-group.test.ts @@ -58,6 +58,7 @@ test('custom resource exists', () => { InstallLatestAwsSdk: true, }, DependsOn: [ + 'GlobalAcceleratorSGCustomResourceCustomResourcePolicyF3294553', 'GroupC77FDACD', ], }, ResourcePart.CompleteDefinition)); diff --git a/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts b/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts index 37f61ccb65260..0788ad7f09202 100644 --- a/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts +++ b/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts @@ -324,24 +324,33 @@ export class AwsCustomResource extends cdk.Construct implements iam.IGrantable { }); this.grantPrincipal = provider.grantPrincipal; + // Create the policy statements for the custom resource function role, or use the user-provided ones + const statements = []; if (props.policy.statements.length !== 0) { // Use custom statements provided by the user for (const statement of props.policy.statements) { provider.addToRolePolicy(statement); + statements.push(statement); } } else { // Derive statements from AWS SDK calls for (const call of [props.onCreate, props.onUpdate, props.onDelete]) { if (call) { - provider.addToRolePolicy(new iam.PolicyStatement({ + const statement = new iam.PolicyStatement({ actions: [awsSdkToIamAction(call.service, call.action)], resources: props.policy.resources, - })); + }); + provider.addToRolePolicy(statement); + statements.push(statement); } } - } - + const policy = new iam.Policy(this, 'CustomResourcePolicy', { + statements: statements, + }); + if (provider.role !== undefined) { + policy.attachToRole(provider.role); + } const create = props.onCreate || props.onUpdate; this.customResource = new cdk.CustomResource(this, 'Resource', { resourceType: props.resourceType || 'Custom::AWS', @@ -354,6 +363,10 @@ export class AwsCustomResource extends cdk.Construct implements iam.IGrantable { installLatestAwsSdk: props.installLatestAwsSdk ?? true, }, }); + + // If the policy was deleted first, then the function might lose permissions to delete the custom resource + // This is here so that the policy doesn't get removed before onDelete is called + this.customResource.node.addDependency(policy); } /** diff --git a/packages/@aws-cdk/custom-resources/test/aws-custom-resource/integ.aws-custom-resource.expected.json b/packages/@aws-cdk/custom-resources/test/aws-custom-resource/integ.aws-custom-resource.expected.json index f8527c3bdd8eb..94bb691267f79 100644 --- a/packages/@aws-cdk/custom-resources/test/aws-custom-resource/integ.aws-custom-resource.expected.json +++ b/packages/@aws-cdk/custom-resources/test/aws-custom-resource/integ.aws-custom-resource.expected.json @@ -45,7 +45,10 @@ "InstallLatestAwsSdk": true }, "UpdateReplacePolicy": "Delete", - "DeletionPolicy": "Delete" + "DeletionPolicy": "Delete", + "DependsOn": [ + "PublishCustomResourcePolicyDF696FCA" + ] }, "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2": { "Type": "AWS::IAM::Role", @@ -191,6 +194,7 @@ "InstallLatestAwsSdk": true }, "DependsOn": [ + "ListTopicsCustomResourcePolicy31A8396A", "TopicBFC7AF6E" ], "UpdateReplacePolicy": "Delete", @@ -241,7 +245,44 @@ "InstallLatestAwsSdk": true }, "UpdateReplacePolicy": "Delete", - "DeletionPolicy": "Delete" + "DeletionPolicy": "Delete", + "DependsOn": [ + "GetParameterCustomResourcePolicyD8E5D455" + ] + }, + "PublishCustomResourcePolicyDF696FCA": { + "Type": "AWS::IAM::Policy", + "Properties": { + "PolicyDocument": { + "Statement": [{"Action":"sns:Publish","Effect":"Allow","Resource":"*"}], + "Version": "2012-10-17" + }, + "PolicyName": "PublishCustomResourcePolicyDF696FCA", + "Roles": [{"Ref":"AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2"}] + } + }, + "ListTopicsCustomResourcePolicy31A8396A": { + "Type": "AWS::IAM::Policy", + "Properties": { + "PolicyDocument": { + "Statement": [{"Action":"sns:ListTopics","Effect":"Allow","Resource":"*"}], + "Version": "2012-10-17" + }, + "PolicyName": "ListTopicsCustomResourcePolicy31A8396A", + "Roles": [{"Ref":"AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2"}] + }, + "DependsOn": ["TopicBFC7AF6E"] + }, + "GetParameterCustomResourcePolicyD8E5D455": { + "Type": "AWS::IAM::Policy", + "Properties": { + "PolicyDocument": { + "Statement": [{"Action":"ssm:GetParameter","Effect":"Allow","Resource":"*"}], + "Version": "2012-10-17" + }, + "PolicyName": "GetParameterCustomResourcePolicyD8E5D455", + "Roles": [{"Ref":"AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2"}] + } } }, "Parameters": { From 1df4e818c5d3b43927d30bc71b59d3a55dbc1082 Mon Sep 17 00:00:00 2001 From: James Fleming Date: Thu, 27 Aug 2020 21:45:05 +0000 Subject: [PATCH 2/3] Remove previous policy which accumulated all permissions --- ...nteg.user-pool-domain-cfdist.expected.json | 22 -------- .../test/integ.imagescan.expected.json | 27 ---------- .../aws-custom-resource.ts | 1 - .../aws-custom-resource.test.ts | 54 +++++++++++++++++-- .../integ.aws-custom-resource.expected.json | 32 ----------- 5 files changed, 49 insertions(+), 87 deletions(-) diff --git a/packages/@aws-cdk/aws-cognito/test/integ.user-pool-domain-cfdist.expected.json b/packages/@aws-cdk/aws-cognito/test/integ.user-pool-domain-cfdist.expected.json index 4c1a188e9196b..5f3a61539ea8a 100644 --- a/packages/@aws-cdk/aws-cognito/test/integ.user-pool-domain-cfdist.expected.json +++ b/packages/@aws-cdk/aws-cognito/test/integ.user-pool-domain-cfdist.expected.json @@ -131,27 +131,6 @@ "Roles": [{"Ref":"AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2"}] } }, - "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleDefaultPolicyD28E1A5E": { - "Type": "AWS::IAM::Policy", - "Properties": { - "PolicyDocument": { - "Statement": [ - { - "Action": "cognito-idp:DescribeUserPoolDomain", - "Effect": "Allow", - "Resource": "*" - } - ], - "Version": "2012-10-17" - }, - "PolicyName": "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleDefaultPolicyD28E1A5E", - "Roles": [ - { - "Ref": "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2" - } - ] - } - }, "AWS679f53fac002430cb0da5b7982bd22872D164C4C": { "Type": "AWS::Lambda::Function", "Properties": { @@ -204,7 +183,6 @@ "Timeout": 120 }, "DependsOn": [ - "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleDefaultPolicyD28E1A5E", "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2" ] } diff --git a/packages/@aws-cdk/aws-ecr/test/integ.imagescan.expected.json b/packages/@aws-cdk/aws-ecr/test/integ.imagescan.expected.json index fbfaeb93caa5c..76cd5933128c1 100644 --- a/packages/@aws-cdk/aws-ecr/test/integ.imagescan.expected.json +++ b/packages/@aws-cdk/aws-ecr/test/integ.imagescan.expected.json @@ -159,32 +159,6 @@ ] } }, - "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleDefaultPolicyD28E1A5E": { - "Type": "AWS::IAM::Policy", - "Properties": { - "PolicyDocument": { - "Statement": [ - { - "Action": "ecr:PutImageScanningConfiguration", - "Effect": "Allow", - "Resource": { - "Fn::GetAtt": [ - "Repo02AC86CF", - "Arn" - ] - } - } - ], - "Version": "2012-10-17" - }, - "PolicyName": "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleDefaultPolicyD28E1A5E", - "Roles": [ - { - "Ref": "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2" - } - ] - } - }, "AWS679f53fac002430cb0da5b7982bd22872D164C4C": { "Type": "AWS::Lambda::Function", "Properties": { @@ -237,7 +211,6 @@ "Timeout": 120 }, "DependsOn": [ - "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleDefaultPolicyD28E1A5E", "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2" ] } diff --git a/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts b/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts index 0788ad7f09202..3b89258e897cb 100644 --- a/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts +++ b/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts @@ -340,7 +340,6 @@ export class AwsCustomResource extends cdk.Construct implements iam.IGrantable { actions: [awsSdkToIamAction(call.service, call.action)], resources: props.policy.resources, }); - provider.addToRolePolicy(statement); statements.push(statement); } } diff --git a/packages/@aws-cdk/custom-resources/test/aws-custom-resource/aws-custom-resource.test.ts b/packages/@aws-cdk/custom-resources/test/aws-custom-resource/aws-custom-resource.test.ts index 184e72c4ed4ca..76d7ec9fa585d 100644 --- a/packages/@aws-cdk/custom-resources/test/aws-custom-resource/aws-custom-resource.test.ts +++ b/packages/@aws-cdk/custom-resources/test/aws-custom-resource/aws-custom-resource.test.ts @@ -286,11 +286,6 @@ test('implements IGrantable', () => { expect(stack).toHaveResource('AWS::IAM::Policy', { PolicyDocument: { Statement: [ - { - Action: 'service:Action', - Effect: 'Allow', - Resource: '*', - }, { Action: 'iam:PassRole', Effect: 'Allow', @@ -564,3 +559,52 @@ test('can specify function name', () => { FunctionName: 'my-cool-function', }); }); + +test('separate policies per custom resource', () => { + // GIVEN + const stack = new cdk.Stack(); + + // WHEN + new AwsCustomResource(stack, 'Custom1', { + onCreate: { + service: 'service1', + action: 'action1', + physicalResourceId: PhysicalResourceId.of('id1'), + }, + policy: AwsCustomResourcePolicy.fromSdkCalls({ resources: AwsCustomResourcePolicy.ANY_RESOURCE }), + }); + new AwsCustomResource(stack, 'Custom2', { + onCreate: { + service: 'service2', + action: 'action2', + physicalResourceId: PhysicalResourceId.of('id2'), + }, + policy: AwsCustomResourcePolicy.fromSdkCalls({ resources: AwsCustomResourcePolicy.ANY_RESOURCE }), + }); + + // THEN + expect(stack).toHaveResource('AWS::IAM::Policy', { + PolicyDocument: { + Statement: [ + { + Action: 'service1:Action1', + Effect: 'Allow', + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + }); + expect(stack).toHaveResource('AWS::IAM::Policy', { + PolicyDocument: { + Statement: [ + { + Action: 'service2:Action2', + Effect: 'Allow', + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + }); +}); diff --git a/packages/@aws-cdk/custom-resources/test/aws-custom-resource/integ.aws-custom-resource.expected.json b/packages/@aws-cdk/custom-resources/test/aws-custom-resource/integ.aws-custom-resource.expected.json index 94bb691267f79..81da053efe11d 100644 --- a/packages/@aws-cdk/custom-resources/test/aws-custom-resource/integ.aws-custom-resource.expected.json +++ b/packages/@aws-cdk/custom-resources/test/aws-custom-resource/integ.aws-custom-resource.expected.json @@ -81,37 +81,6 @@ ] } }, - "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleDefaultPolicyD28E1A5E": { - "Type": "AWS::IAM::Policy", - "Properties": { - "PolicyDocument": { - "Statement": [ - { - "Action": "sns:Publish", - "Effect": "Allow", - "Resource": "*" - }, - { - "Action": "sns:ListTopics", - "Effect": "Allow", - "Resource": "*" - }, - { - "Action": "ssm:GetParameter", - "Effect": "Allow", - "Resource": "*" - } - ], - "Version": "2012-10-17" - }, - "PolicyName": "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleDefaultPolicyD28E1A5E", - "Roles": [ - { - "Ref": "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2" - } - ] - } - }, "AWS679f53fac002430cb0da5b7982bd22872D164C4C": { "Type": "AWS::Lambda::Function", "Properties": { @@ -164,7 +133,6 @@ "Timeout": 120 }, "DependsOn": [ - "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleDefaultPolicyD28E1A5E", "AWS679f53fac002430cb0da5b7982bd2287ServiceRoleC1EA0FF2" ] }, From 3ec88f63e2d61cd3b4182a38e3edf50e0ad96fdd Mon Sep 17 00:00:00 2001 From: James Fleming Date: Fri, 28 Aug 2020 13:43:33 +0000 Subject: [PATCH 3/3] Also do for user-provided policies --- .../lib/aws-custom-resource/aws-custom-resource.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts b/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts index 3b89258e897cb..db40e3269ca17 100644 --- a/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts +++ b/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts @@ -329,7 +329,6 @@ export class AwsCustomResource extends cdk.Construct implements iam.IGrantable { if (props.policy.statements.length !== 0) { // Use custom statements provided by the user for (const statement of props.policy.statements) { - provider.addToRolePolicy(statement); statements.push(statement); } } else {