fix(codepipeline): CloudFormation deployment role always gets pipelin…

…e bucket and key permissions (#5190) Previously, we only explicitly granted the CloudFormation CodePipeline action deployment role access to the pipeline bucket (and, by extension, its KMS key) when the action was deploying into a different account. However, that meant in the single account case, if the pipeline had a key defined, the role would never be added to the key's policy, and any deployment requiring access to the artifacts bucket (like a Lambda function) would fail. This fixes the bug by always granting the deployment role permissions to the pipeline bucket (and thus the key as well). Fixes #5183
aws · Nov 26, 2019 · d5c0f3e · d5c0f3e
1 parent 209f909
commit d5c0f3e
Show file tree

Hide file tree

Showing 9 changed files with 426 additions and 9 deletions.
diff --git a/packages/@aws-cdk/app-delivery/test/integ.cicd.expected.json b/packages/@aws-cdk/app-delivery/test/integ.cicd.expected.json
@@ -419,6 +419,52 @@
           "Version": "2012-10-17"
         }
       }
+    },
+    "CodePipelineDeployChangeSetRoleDefaultPolicy289820BE": {
+      "Type": "AWS::IAM::Policy",
+      "Properties": {
+        "PolicyDocument": {
+          "Statement": [
+            {
+              "Action": [
+                "s3:GetObject*",
+                "s3:GetBucket*",
+                "s3:List*"
+              ],
+              "Effect": "Allow",
+              "Resource": [
+                {
+                  "Fn::GetAtt": [
+                    "ArtifactBucket7410C9EF",
+                    "Arn"
+                  ]
+                },
+                {
+                  "Fn::Join": [
+                    "",
+                    [
+                      {
+                        "Fn::GetAtt": [
+                          "ArtifactBucket7410C9EF",
+                          "Arn"
+                        ]
+                      },
+                      "/*"
+                    ]
+                  ]
+                }
+              ]
+            }
+          ],
+          "Version": "2012-10-17"
+        },
+        "PolicyName": "CodePipelineDeployChangeSetRoleDefaultPolicy289820BE",
+        "Roles": [
+          {
+            "Ref": "CodePipelineDeployChangeSetRoleF9F2B343"
+          }
+        ]
+      }
     }
   }
 }
diff --git a/packages/@aws-cdk/app-delivery/test/test.pipeline-deploy-stack-action.ts b/packages/@aws-cdk/app-delivery/test/test.pipeline-deploy-stack-action.ts
@@ -198,6 +198,49 @@ export = nodeunit.testCase({
       PolicyDocument: {
         Version: '2012-10-17',
         Statement: [
+          {
+            Action: [
+              "s3:GetObject*",
+              "s3:GetBucket*",
+              "s3:List*",
+            ],
+            Effect: "Allow",
+            Resource: [
+              {
+                "Fn::GetAtt": [
+                  "CodePipelineArtifactsBucketF1E925CF",
+                  "Arn",
+                ],
+              },
+              {
+                "Fn::Join": [
+                  "",
+                  [
+                    {
+                      "Fn::GetAtt": [
+                        "CodePipelineArtifactsBucketF1E925CF",
+                        "Arn",
+                      ],
+                    },
+                    "/*",
+                  ],
+                ],
+              },
+            ],
+          },
+          {
+            Action: [
+              "kms:Decrypt",
+              "kms:DescribeKey",
+            ],
+            Effect: "Allow",
+            Resource: {
+              "Fn::GetAtt": [
+                "CodePipelineArtifactsBucketEncryptionKey85407CB4",
+                "Arn",
+              ],
+            },
+          },
           {
             Action: '*',
             Effect: 'Allow',
@@ -272,6 +315,49 @@ export = nodeunit.testCase({
       PolicyDocument: {
         Version: '2012-10-17',
         Statement: [
+          {
+            Action: [
+              "s3:GetObject*",
+              "s3:GetBucket*",
+              "s3:List*",
+            ],
+            Effect: "Allow",
+            Resource: [
+              {
+                "Fn::GetAtt": [
+                  "CodePipelineArtifactsBucketF1E925CF",
+                  "Arn",
+                ],
+              },
+              {
+                "Fn::Join": [
+                  "",
+                  [
+                    {
+                      "Fn::GetAtt": [
+                        "CodePipelineArtifactsBucketF1E925CF",
+                        "Arn",
+                      ],
+                    },
+                    "/*"
+                  ],
+                ],
+              },
+            ],
+          },
+          {
+            Action: [
+              "kms:Decrypt",
+              "kms:DescribeKey",
+            ],
+            Effect: "Allow",
+            Resource: {
+              "Fn::GetAtt": [
+                "CodePipelineArtifactsBucketEncryptionKey85407CB4",
+                "Arn",
+              ],
+            },
+          },
           {
             Action: [
               'ec2:AuthorizeSecurityGroupEgress',

diff --git a/packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts b/packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts
@@ -278,18 +278,19 @@ abstract class CloudFormationDeployAction extends CloudFormationAction {
           assumedBy: new iam.ServicePrincipal('cloudformation.amazonaws.com'),
           roleName: cdk.PhysicalName.GENERATE_IF_NEEDED,
         });
-
-        // the deployment role might need read access to the pipeline's bucket
-        // (for example, if it's deploying a Lambda function),
-        // and since this is cross-account, even admin permissions won't be enough -
-        // the pipeline's bucket must trust this role
-        options.bucket.grantRead(this._deploymentRole);
       } else {
         this._deploymentRole = new iam.Role(scope, 'Role', {
           assumedBy: new iam.ServicePrincipal('cloudformation.amazonaws.com')
         });
       }
 
+      // the deployment role might need read access to the pipeline's bucket
+      // (for example, if it's deploying a Lambda function),
+      // and even if it has admin permissions, it won't be enough,
+      // as it needs to be added to the key's resource policy
+      // (and the bucket's, if the access is cross-account)
+      options.bucket.grantRead(this._deploymentRole);
+
       if (this.props2.adminPermissions) {
         this._deploymentRole.addToPolicy(new iam.PolicyStatement({
           actions: ['*'],

diff --git a/...-cdk/aws-codepipeline-actions/test/cloudformation/test.cloudformation-pipeline-actions.ts b/...-cdk/aws-codepipeline-actions/test/cloudformation/test.cloudformation-pipeline-actions.ts
@@ -1,4 +1,4 @@
-import { expect, haveResource, haveResourceLike } from '@aws-cdk/assert';
+import { expect, haveResourceLike } from '@aws-cdk/assert';
 import { CloudFormationCapabilities } from '@aws-cdk/aws-cloudformation';
 import codebuild = require('@aws-cdk/aws-codebuild');
 import codecommit = require('@aws-cdk/aws-codecommit');
@@ -200,7 +200,7 @@ export = {
 
   },
 
-  'fullPermissions leads to admin role and full IAM capabilities'(test: Test) {
+  'fullPermissions leads to admin role and full IAM capabilities with pipeline bucket+key read permissions'(test: Test) {
   // GIVEN
   const stack = new TestFixture();
 
@@ -238,10 +238,25 @@ export = {
   }));
 
   // THEN: Role is created with full permissions
-  expect(stack).to(haveResource('AWS::IAM::Policy', {
+  expect(stack).to(haveResourceLike('AWS::IAM::Policy', {
     PolicyDocument: {
       Version: '2012-10-17',
       Statement: [
+        {
+          "Action": [
+            "s3:GetObject*",
+            "s3:GetBucket*",
+            "s3:List*",
+          ],
+          "Effect": "Allow",
+        },
+        {
+          "Action": [
+            "kms:Decrypt",
+            "kms:DescribeKey",
+          ],
+          "Effect": "Allow",
+        },
         {
           Action: "*",
           Effect: 'Allow',

diff --git a/...ges/@aws-cdk/aws-codepipeline-actions/test/integ.cfn-template-from-repo.lit.expected.json b/...ges/@aws-cdk/aws-codepipeline-actions/test/integ.cfn-template-from-repo.lit.expected.json
@@ -86,6 +86,22 @@
               },
               "Resource": "*"
             },
+            {
+              "Action": [
+                "kms:Decrypt",
+                "kms:DescribeKey"
+              ],
+              "Effect": "Allow",
+              "Principal": {
+                "AWS": {
+                  "Fn::GetAtt": [
+                    "PipelineDeployPrepareChangesRoleD28C853C",
+                    "Arn"
+                  ]
+                }
+              },
+              "Resource": "*"
+            },
             {
               "Action": [
                 "kms:Decrypt",
@@ -681,6 +697,49 @@
       "Properties": {
         "PolicyDocument": {
           "Statement": [
+            {
+              "Action": [
+                "s3:GetObject*",
+                "s3:GetBucket*",
+                "s3:List*"
+              ],
+              "Effect": "Allow",
+              "Resource": [
+                {
+                  "Fn::GetAtt": [
+                    "PipelineArtifactsBucket22248F97",
+                    "Arn"
+                  ]
+                },
+                {
+                  "Fn::Join": [
+                    "",
+                    [
+                      {
+                        "Fn::GetAtt": [
+                          "PipelineArtifactsBucket22248F97",
+                          "Arn"
+                        ]
+                      },
+                      "/*"
+                    ]
+                  ]
+                }
+              ]
+            },
+            {
+              "Action": [
+                "kms:Decrypt",
+                "kms:DescribeKey"
+              ],
+              "Effect": "Allow",
+              "Resource": {
+                "Fn::GetAtt": [
+                  "PipelineArtifactsBucketEncryptionKey01D58D69",
+                  "Arn"
+                ]
+              }
+            },
             {
               "Action": "*",
               "Effect": "Allow",

diff --git a/...ws-codepipeline-actions/test/integ.lambda-deployed-through-codepipeline.lit.expected.json b/...ws-codepipeline-actions/test/integ.lambda-deployed-through-codepipeline.lit.expected.json
@@ -173,6 +173,22 @@
               },
               "Resource": "*"
             },
+            {
+              "Action": [
+                "kms:Decrypt",
+                "kms:DescribeKey"
+              ],
+              "Effect": "Allow",
+              "Principal": {
+                "AWS": {
+                  "Fn::GetAtt": [
+                    "PipelineDeployLambdaCFNDeployRole89CA1043",
+                    "Arn"
+                  ]
+                }
+              },
+              "Resource": "*"
+            },
             {
               "Action": [
                 "kms:Decrypt",
@@ -1137,6 +1153,49 @@
       "Properties": {
         "PolicyDocument": {
           "Statement": [
+            {
+              "Action": [
+                "s3:GetObject*",
+                "s3:GetBucket*",
+                "s3:List*"
+              ],
+              "Effect": "Allow",
+              "Resource": [
+                {
+                  "Fn::GetAtt": [
+                    "PipelineArtifactsBucket22248F97",
+                    "Arn"
+                  ]
+                },
+                {
+                  "Fn::Join": [
+                    "",
+                    [
+                      {
+                        "Fn::GetAtt": [
+                          "PipelineArtifactsBucket22248F97",
+                          "Arn"
+                        ]
+                      },
+                      "/*"
+                    ]
+                  ]
+                }
+              ]
+            },
+            {
+              "Action": [
+                "kms:Decrypt",
+                "kms:DescribeKey"
+              ],
+              "Effect": "Allow",
+              "Resource": {
+                "Fn::GetAtt": [
+                  "PipelineArtifactsBucketEncryptionKey01D58D69",
+                  "Arn"
+                ]
+              }
+            },
             {
               "Action": "*",
               "Effect": "Allow",