Skip to content

Commit

Permalink
fix(s3-deployment): BucketDeployment fails when bootstrap stack's `…
Browse files Browse the repository at this point in the history
…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*
  • Loading branch information
kxue-godaddy authored Apr 10, 2024
1 parent a7b4e29 commit 0b429fb
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 7 deletions.
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';
import * as path from 'path';

class Fixture extends Stack {
Expand Down

0 comments on commit 0b429fb

Please sign in to comment.