From 0b429fb80e7820afb606d5424476444940166ade Mon Sep 17 00:00:00 2001 From: Ke Xue <109686543+kxue-godaddy@users.noreply.github.com> Date: Wed, 10 Apr 2024 13:13:55 -0400 Subject: [PATCH] fix(s3-deployment): `BucketDeployment` fails when bootstrap stack's `StagingBucket` is encrypted with customer managed KMS key (#29540) ### Issue #25100 Closes #25100. ### Reason for this change When the CDK bootstrap stack's `StagingBucket` is encrypted with a customer managed KMS key whose key policy does not include wildcard KMS permissions similar to those of the S3 managed KMS key, `BucketDeployment` fails because the Lambda's execution role doesn't get the `kms:Decrypt` permission necessary to download from the bucket. In addition, if one of the sources for `BucketDeployment` comes from the `Source.bucket` static method, and the bucket is an "out-of-app imported reference" created from `s3.Bucket.fromBucketName`, the bucket's `encryptionKey` attribute is `null` and the current code won't add the `kms:Decrypt` permission on the bucket's encryption key to the Lambda's execution role. If this bucket is additionally encrypted with a customer managed KMS key without sufficient resource-based policy, the deployment fails as well. ### Description of changes It's not easy to make the code "just work" in every situation, because there's no way to pinpoint the source bucket's encryption key ARN without using another custom resource, which is a heavy-lifting and it's hard to give this new Lambda a reasonable and minimal set of execution role permissions. Therefore, this PR resolves the issue by changing `BucketDeployment.handlerRole` from `private readonly` to `public readonly`, and adding documentations on how to handle errors resulting from "not authorized to perform: kms:Decrypt". The current code allows customizing `handlerRole` by passing in `BucketDeploymentProps.role`. This change makes the customization easier because users don't need to manually add the S3 permissions. The only code change is on the visibility of `BucketDeployment.handlerRole`. All other changes are documentations. I proposed 4 possible changes in my comment to Issue #25100, and only the first one (changing visibility) is pursued in this PR. The second one was abandoned because the CFN export `CdkBootstrap-hnb659fds-FileAssetKeyArn` is deprecated. ### Description of how you validated changes I wrote a CDK app which uses the `BucketDeployment` construct. After manually adding relevant KMS permissions to the Lambda execution role, I verified that the bucket deployment worked in the following two scenarios: - My personal account; bootstrap stack's `StagingBucket` encrypted with a custom KMS key which only has the default key policy. - GoDaddy corporate account; `StagingBucket` encrypted with a KMS key from an AWS Organization management account. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../aws-cdk-lib/aws-s3-deployment/README.md | 63 +++++++++++++++++-- .../lib/bucket-deployment.ts | 6 +- .../aws-s3-deployment/lib/source.ts | 25 ++++++++ .../aws_s3_deployment/default.ts-fixture | 2 + 4 files changed, 89 insertions(+), 7 deletions(-) diff --git a/packages/aws-cdk-lib/aws-s3-deployment/README.md b/packages/aws-cdk-lib/aws-s3-deployment/README.md index 1c290c61d7a68..9bc53cff46280 100644 --- a/packages/aws-cdk-lib/aws-s3-deployment/README.md +++ b/packages/aws-cdk-lib/aws-s3-deployment/README.md @@ -24,13 +24,13 @@ This is what happens under the hood: 1. When this stack is deployed (either via `cdk deploy` or via CI/CD), the contents of the local `website-dist` directory will be archived and uploaded - to an intermediary assets bucket. If there is more than one source, they will - be individually uploaded. -2. The `BucketDeployment` construct synthesizes a custom CloudFormation resource + to an intermediary assets bucket (the `StagingBucket` of the CDK bootstrap stack). + If there is more than one source, they will be individually uploaded. +2. The `BucketDeployment` construct synthesizes a Lambda-backed custom CloudFormation resource of type `Custom::CDKBucketDeployment` into the template. The source bucket/key is set to point to the assets bucket. -3. The custom resource downloads the .zip archive, extracts it and issues `aws - s3 sync --delete` against the destination bucket (in this case +3. The custom resource invokes its associated Lambda function, which downloads the .zip archive, + extracts it and issues `aws s3 sync --delete` against the destination bucket (in this case `websiteBucket`). If there is more than one source, the sources will be downloaded and merged pre-deployment at this step. @@ -67,6 +67,58 @@ const deployment = new s3deploy.BucketDeployment(this, 'DeployWebsite', { deployment.addSource(s3deploy.Source.asset('./another-asset')); ``` +For the Lambda function to download object(s) from the source bucket, besides the obvious +`s3:GetObject*` permissions, the Lambda's execution role needs the `kms:Decrypt` and `kms:DescribeKey` +permissions on the KMS key that is used to encrypt the bucket. By default, when the source bucket is +encrypted with the S3 managed key of the account, these permissions are granted by the key's +resource-based policy, so they do not need to be on the Lambda's execution role policy explicitly. +However, if the encryption key is not the s3 managed one, its resource-based policy is quite likely +to NOT grant such KMS permissions. In this situation, the Lambda execution will fail with an error +message like below: + +```txt +download failed: ... +An error occurred (AccessDenied) when calling the GetObject operation: +User: *** is not authorized to perform: kms:Decrypt on the resource associated with this ciphertext +because no identity-based policy allows the kms:Decrypt action +``` + +When this happens, users can use the public `handlerRole` property of `BucketDeployment` to manually +add the KMS permissions: + +```ts +declare const destinationBucket: s3.Bucket; + +const deployment = new s3deploy.BucketDeployment(this, 'DeployFiles', { + sources: [s3deploy.Source.asset(path.join(__dirname, 'source-files'))], + destinationBucket, +}); + +deployment.handlerRole.addToPolicy( + new iam.PolicyStatement({ + actions: ['kms:Decrypt', 'kms:DescribeKey'], + effect: iam.Effect.ALLOW, + resources: [''], + }), +); +``` + +The situation above could arise from the following scenarios: + +- User created a customer managed KMS key and passed its ID to the `cdk bootstrap` command via + the `--bootstrap-kms-key-id` CLI option. + The [default key policy](https://docs.aws.amazon.com/kms/latest/developerguide/key-policy-default.html#key-policy-default-allow-root-enable-iam) + alone is not sufficient to grant the Lambda KMS permissions. + +- A corporation uses its own custom CDK bootstrap process, which encrypts the CDK `StagingBucket` + by a KMS key from a management account of the corporation's AWS Organization. In this cross-account + access scenario, the KMS permissions must be explicitly present in the Lambda's execution role policy. + +- One of the sources for the `BucketDeployment` comes from the `Source.bucket` static method, which + points to a bucket whose encryption key is not the S3 managed one, and the resource-based policy + of the encryption key is not sufficient to grant the Lambda `kms:Decrypt` and `kms:DescribeKey` + permissions. + ## Supported sources The following source types are supported for bucket deployments: @@ -370,7 +422,6 @@ The syntax for template variables is `{{ variableName }}` in your local file. Th specify the substitutions in CDK like this: ```ts -import * as iam from 'aws-cdk-lib/aws-iam'; import * as lambda from 'aws-cdk-lib/aws-lambda'; declare const myLambdaFunction: lambda.Function; diff --git a/packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts b/packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts index 5d782dfb6bc49..fa7df6a1cbf51 100644 --- a/packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts +++ b/packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts @@ -277,7 +277,11 @@ export class BucketDeployment extends Construct { private requestDestinationArn: boolean = false; private readonly destinationBucket: s3.IBucket; private readonly sources: SourceConfig[]; - private readonly handlerRole: iam.IRole; + + /** + * Execution role of the Lambda function behind the custom CloudFormation resource of type `Custom::CDKBucketDeployment`. + */ + public readonly handlerRole: iam.IRole; constructor(scope: Construct, id: string, props: BucketDeploymentProps) { super(scope, id); diff --git a/packages/aws-cdk-lib/aws-s3-deployment/lib/source.ts b/packages/aws-cdk-lib/aws-s3-deployment/lib/source.ts index da555ff24867b..c6815194aa843 100644 --- a/packages/aws-cdk-lib/aws-s3-deployment/lib/source.ts +++ b/packages/aws-cdk-lib/aws-s3-deployment/lib/source.ts @@ -68,6 +68,31 @@ export class Source { * * Make sure you trust the producer of the archive. * + * If the `bucket` parameter is an "out-of-app" reference "imported" via static methods such + * as `s3.Bucket.fromBucketName`, be cautious about the bucket's encryption key. In general, + * CDK does not query for additional properties of imported constructs at synthesis time. + * For example, for a bucket created from `s3.Bucket.fromBucketName`, CDK does not know + * its `IBucket.encryptionKey` property, and therefore will NOT give KMS permissions to the + * Lambda execution role of the `BucketDeployment` construct. If you want the + * `kms:Decrypt` and `kms:DescribeKey` permissions on the bucket's encryption key + * to be added automatically, reference the imported bucket via `s3.Bucket.fromBucketAttributes` + * and pass in the `encryptionKey` attribute explicitly. + * + * @example + * declare const destinationBucket: s3.Bucket; + * const sourceBucket = s3.Bucket.fromBucketAttributes(this, 'SourceBucket', { + * bucketArn: 'arn:aws:s3:::my-source-bucket-name', + * encryptionKey: kms.Key.fromKeyArn( + * this, + * 'SourceBucketEncryptionKey', + * 'arn:aws:kms:us-east-1:123456789012:key/' + * ), + * }); + * const deployment = new s3deploy.BucketDeployment(this, 'DeployFiles', { + * sources: [s3deploy.Source.bucket(sourceBucket, 'source.zip')], + * destinationBucket, + * }); + * * @param bucket The S3 Bucket * @param zipObjectKey The S3 object key of the zip file with contents */ diff --git a/packages/aws-cdk-lib/rosetta/aws_s3_deployment/default.ts-fixture b/packages/aws-cdk-lib/rosetta/aws_s3_deployment/default.ts-fixture index f074d1326e124..cdf22b94430c7 100644 --- a/packages/aws-cdk-lib/rosetta/aws_s3_deployment/default.ts-fixture +++ b/packages/aws-cdk-lib/rosetta/aws_s3_deployment/default.ts-fixture @@ -4,6 +4,8 @@ import { Construct } from 'constructs'; import * as s3deploy from 'aws-cdk-lib/aws-s3-deployment'; import * as s3 from 'aws-cdk-lib/aws-s3'; import * as ec2 from'aws-cdk-lib/aws-ec2'; +import * as iam from 'aws-cdk-lib/aws-iam'; +import * as kms from 'aws-cdk-lib/aws-kms'; import * as path from 'path'; class Fixture extends Stack {