Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(s3-deployment): BucketDeployment fails when bootstrap stack's StagingBucket is encrypted with customer managed KMS key #29540

Merged
merged 2 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 57 additions & 6 deletions packages/aws-cdk-lib/aws-s3-deployment/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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: ['<encryption key ARN>'],
}),
);
```

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:
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
25 changes: 25 additions & 0 deletions packages/aws-cdk-lib/aws-s3-deployment/lib/source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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/<key-id>'
* ),
* });
* 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
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this change needed?

Copy link
Contributor Author

@kxue-godaddy kxue-godaddy Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing! @GavinZZ

I'm following the guidelines on writing docs in the Rosetta section. Without adding these lines, the new code snippets I added won't compile (e.g. Line 98 of packages/aws-cdk-lib/aws-s3-deployment/README.md, which uses iam.PolicyStatement). The section also recommends "Utilize the default.ts-fixture that already exists rather than writing new .ts-fixture files", so I'm updating the default ts-fixture file.

It seems that the CI (when I was making the PR) doesn't check if Rosetta compiles, but since this PR is mainly doc changes, I want to make sure the updates can be reflected on the CDK doc site. <-- By this I mean not all committed docs compile when I ran yarn rosetta:extract --strict locally. I only made sure all aws-s3-deployment docs compile but didn't touch other subpackages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kxue-godaddy thanks for getting back to me so quickly. I agree that the import statement for iam is necessary since it's used in the README file, but I don't think I see any usage of kms (other than the IAM actions). Would you please try removing the kms import statement and re-ran the command?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GavinZZ

I removed the kms line and reran the command. It shows the following error:

aws-cdk-lib.aws_s3_deployment.Source#bucket-example.ts:24:18 - error TS2304: Cannot find name 'kms'.

24   encryptionKey: kms.Key.fromKeyArn(

The kms import line is needed by packages/aws-cdk-lib/aws-s3-deployment/lib/source.ts, where I added an example for Source.bucket via the @example tag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nice, thank you. Didn't notice that there's a kms usage here.

import * as path from 'path';

class Fixture extends Stack {
Expand Down