-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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(bootstrap): no longer creates KMS master key by default #10365
Conversation
The modern bootstrap stack used to unconditionally create a KMS Customer Master Key (CMK) for users. This incurs a $1/month charge for every user of the CDK for every region and account they want to deploy in, which is not acceptable if we're going to make this the default bootstrapping experience in the future. This PR switches off the creation of the CMK by default for new bootstrap stacks. Bootstrap stacks that already exist can remove the existing CMK by running: ``` cdk bootstrap --bootstrap-customer-key=false [aws://...] ``` This change is backwards compatible: updates to existing (modern) bootstrap stacks will leave the current KMS key in place. To achieve this, the new default is encoded into the CLI, not into the template. Fixes #10115.
Are there any downsides of using the default S3 key? Is there any cross-account access needed or policies that are not possible with AWS managed CMKs? |
Not for this key, no. For the keys that are created as part of the Pipeline, yes. I'm working on a separate PR there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me; two nitpicks.
/** | ||
* Magic parameter value that will cause the bootstrap-template.yml to NOT create a CMK but use the default keyo | ||
*/ | ||
const USE_AWS_MANAGED_KEY = '-'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~
Any reason not to make this a little less magical? Like
const USE_AWS_MANAGED_KEY = '-'; | |
const USE_AWS_MANAGED_KEY = 'USE_AWS_MANAGED_KEY'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess not!
const kmsKeyId = params.kmsKeyId ?? | ||
(params.createCustomerMasterKey === true ? '' : | ||
params.createCustomerMasterKey === false ? USE_AWS_MANAGED_KEY : | ||
current.parameters.FileAssetsBucketKmsKeyId !== undefined ? undefined : USE_AWS_MANAGED_KEY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~
Take or leave it, but removed a ternary, defined a new constant, and got this:
const kmsKeyId = params.kmsKeyId ?? | |
(params.createCustomerMasterKey === true ? '' : | |
params.createCustomerMasterKey === false ? USE_AWS_MANAGED_KEY : | |
current.parameters.FileAssetsBucketKmsKeyId !== undefined ? undefined : USE_AWS_MANAGED_KEY); | |
const currentKmsKeyId = current.parameters.FileAssetsBucketKmsKeyId; | |
const kmsKeyId = params.kmsKeyId ?? | |
(params.createCustomerMasterKey ? CREATE_CMK | |
: (params.createCustomerMasterKey === false || currentKmsKeyId === undefined) | |
? USE_AWS_MANAGED_KEY : undefined); |
…to huijbers/bootstrap-no-cmk
Tested to work with x-account and x-account/x-region pipeline |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
The modern bootstrap stack used to unconditionally create a KMS Customer
Master Key (CMK) for users. This incurs a $1/month charge for every user
of the CDK for every region and account they want to deploy in, which is
not acceptable if we're going to make this the default bootstrapping
experience in the future.
This PR switches off the creation of the CMK by default for new
bootstrap stacks. Bootstrap stacks that already exist can remove the
existing CMK by running:
This change is backwards compatible: updates to existing (modern)
bootstrap stacks will leave the current KMS key in place. To achieve
this, the new default is encoded into the CLI, not into the template.
Fixes #10115.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license