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

[kxue-godaddy]

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

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

[kxue-godaddy]

Exemption Request

I'm requesting an exemption from updating test and snapshot files in this PR.

The only code change in this PR is changing BucketDeployment.handlerRole from private to public. Other parts of the PR are pure documentation explaining when and how to use this newly exposed attribute to add onto the Lambda's execution role permissions.

[GavinZZ]

why is this change needed?

[kxue-godaddy]

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.

[GavinZZ]

@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?

[kxue-godaddy]

@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.

[GavinZZ]

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

[GavinZZ]

Agree that no test is needed. Just have one question on the ts-fixture file change.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[GavinZZ]

LGTM, thank you for contributing and answering my questions.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[kxue-godaddy]

Looks like "Queue: Embarked in merge queue" was not successful because I didn't allow workflow runs in my fork repo. Just updated that and I will trigger mergify again.

[kxue-godaddy]

@Mergifyio refresh

[mergify]

refresh

✅ Pull request refreshed

[kxue-godaddy]

Mergify still doesn't work. Error message is:

Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/repo-metrics-monthly.yml without workflows permission

I'm going to click the "Update branch" button on this PR and see what happens.

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 67877cb
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[kxue-godaddy]

@GavinZZ

Mergify reports an error for "Queue: Embarked in merge queue" after PR approval. I tried clicking the "Update branch" button and mergify dismissed your approval as stale. Could you approve again? The changes are the same. Sorry!

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).