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

(aws-s3-deployment): bucket deployment fails when toolkit stack uses Customer KMS #25100

Closed
jaecktec opened this issue Apr 13, 2023 · 3 comments · Fixed by #29540
Closed

(aws-s3-deployment): bucket deployment fails when toolkit stack uses Customer KMS #25100

jaecktec opened this issue Apr 13, 2023 · 3 comments · Fixed by #29540
Labels
@aws-cdk/aws-s3-deployment bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@jaecktec
Copy link

Describe the bug

When initialising the toolkit stack with --bootstrap-kms-key-id the bucket deployment fails.

Expected Behavior

Bucket deployment copies the ressources to the desired destination

Current Behavior

The deployment fails and the logs read:

download failed: s3://cdk-<qualifier>-assets-<account>-<region>/<asset-hash>.zip to <tmp-folder> An error occurred (AccessDenied) when calling the GetObject operation: The ciphertext refers to a customer master key that does not exist, does not exist in this region, or you are not allowed to access.

Reproduction Steps

Bootstrap with KMS:

npx cdk bootstrap aws://$AWS_ACCOUNT_ID/$AWS_REGION --bootstrap-kms-key-id $KMS_KEY_ID

create a stack with a bucket deployment

import * as cdk from 'aws-cdk-lib';
import * as s3 from 'aws-cdk-lib/aws-s3';
import * as s3deploy from 'aws-cdk-lib/aws-s3-deployment';

export class MyStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    // Create an S3 bucket
    const bucket = new s3.Bucket(this, 'MyBucket', {
      bucketName: 'my-bucket-name'
    });

    // Deploy a text file to the bucket using BucketDeployment
    new s3deploy.BucketDeployment(this, 'DeployTextFile', {
      sources: [s3deploy.Source.asset('./path/to/text/file.txt')],
      destinationBucket: bucket,
      destinationKeyPrefix: 'path/to/deployed/text/file.txt'
    });
  }
}

Possible Solution

The handlerRole of the BucketDeployment construct needs to have something like this:

    Key.fromKeyArn(this, 'cdk-toolkit-encryption-key', Stack.of(this).formatArn({
      region: Aws.REGION,
      service: 'kms',
      account: Aws.ACCOUNT_ID,
      resource: 'key',
      resourceName: Fn.importValue(`CdkBootstrap-${this.node.tryGetContext('@aws-cdk/core:bootstrapQualifier')}-FileAssetKeyArn`),
    }))
      // @ts-ignore
      .grantEncryptDecrypt(bucketDeployment.handlerRole);

Additional Information/Context

Bootstrap Stack is updated to the most recent version

CDK CLI Version

2.73.0

Framework Version

No response

Node.js Version

16.20.0

OS

macos 13.2.1

Language

Typescript

Language Version

4.9.5

Other information

No response

@jaecktec jaecktec added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 13, 2023
@pahud
Copy link
Contributor

pahud commented Apr 13, 2023

Thank you for your report. Yeah I believe you will need to grant your handler role to decrypt your CMK in this case. I believe we probably can improve this user experience even better. I am leaving this issue open and welcome all community feedbacks.

@pahud pahud added p2 feature-request A feature should be added or improved. effort/medium Medium work item – several days of effort and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 13, 2023
@peterwoodworth peterwoodworth added bug This issue is a bug. p1 and removed p2 feature-request A feature should be added or improved. labels May 19, 2023
@kxue-godaddy
Copy link
Contributor

@pahud , I think we are facing the same problem and this pattern might apply to other enterprise users as well.

My company uses a slightly customized version of the CDK Bootstrap utility – we hard-code ServerSideEncryptionByDefault.KMSMasterKeyID in the CFN template of the CDKToolkit stack. Therefore, the StagingBucket is by default encrypted with a KMS key from a central management account. When using BucketDeployment, we need to give the execution role of the CustomResourceHandler Lambda permission to perform kms:Decrypt on this cross-account KMS key. We know that we can pass in BucketDeploymentProps.role, but this means we need to manually add to this role all the S3 permissions which would otherwise be automatically generated inside BucketDeployment. Currently we are using the "monkey-patching" hack below, but I don't think it's a good coding pattern because it replies on private implementation details.

const lambda = bucketDeployment.node.findChild('CustomResourceHandler') as any;
const role = lambda.role as Role;
role.addToPolicy(
  new PolicyStatement({
  actions: ['kms:Decrypt'],
  effect: Effect.ALLOW,
  resources: ['*'],

I want to propose possibly adding one (or more) of the following new features to BucketDeployment, to make the user-experience better. Could you let me know if you would accept any of them into the code base? If yes, I'd be happy to prepare a PR. I think I have the code change in mind.

  1. Expose the Lambda execution role as a public attribute of BucketDeployment. This enables users to easily call addToPolicy on the role without using the hack above. It is also a simple code change I think.
  2. I noticed that most of the source buckets seem to be the StagingBucket of the CDKToolkit stack. This stack by default has a CFN export named CdkBootstrap-hnb659fds-FileAssetKeyArn, which is the ARN of the KMS encryption key used. We can add the kms:Decrypt permission to this specific ARN inside BucketDeployment. This probably solves most of the situations (at least ours) without users touching the execution role.
  3. If there is a source coming from using s3deploy.Source.bucket(...) on an existing bucket, it seems the only way to get the encryption key ARN of the bucket is to use another Custom Resource. Using this could add all the necessary (and minimal) kms:Decrypt permissions without exposing new attributes of BucketDeployment, but I'm not sure if it's worth the effort.
  4. Maybe we give a "wholesale" kms:Decrypt permission on resources: ['*'] to the Lambda execution role by default? This doesn't change the "user interface" of BucketDeployment either. At my company all KMS keys are typically from a management account and have resource-based policies, so I'm not worried about the wildcard permission, but this is possibly not a good practice for other users.

I'd be happy to make a PR on changes that you think are acceptable. Please let me know. Thanks!

@mergify mergify bot closed this as completed in #29540 Apr 10, 2024
mergify bot pushed a commit that referenced this issue Apr 10, 2024
…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*
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-s3-deployment bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
4 participants