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: create dedicated kms for Aurora addon #2207

Merged
merged 4 commits into from
Apr 23, 2021

Conversation

Lou1415926
Copy link
Contributor

Previously the Aurora addons use the default KMS key for app by importing the exported KMS resource. However, it's not able toImportValue if the service uses a different account. This PR fixes this issue by creating a dedicated KMS resource for each Aurora addon.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Lou1415926 Lou1415926 requested a review from a team as a code owner April 22, 2021 23:04
@Lou1415926 Lou1415926 requested a review from efekarakus April 22, 2021 23:04
Principal:
AWS: !Sub 'arn:aws:iam::${AWS::AccountId}:root'
Action: 'kms:*'
Resource: '*'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we limit these resources to just the Aurora DB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This statement is to prevent the key from being unmanageable in the future (doc). KMS guards against this sort of unmanageable situation and the resource will fail to create.

@@ -31,6 +31,34 @@ Mappings:
{{end -}}
{{end}}
Resources:
{{logicalIDSafe .ClusterName}}AuroraKMSCMK:
Type: 'AWS::KMS::Key'
DeletionPolicy: Retain
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this deletion policy? 🙏

Resource: '*'
Condition:
StringEquals:
'kms:CallerAccount': !Ref 'AWS::AccountId'
Copy link
Contributor

Choose a reason for hiding this comment

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

nice conditionals!

Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@iamhopaul123 iamhopaul123 added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Apr 23, 2021
@mergify mergify bot merged commit e4c3c1d into aws:mainline Apr 23, 2021
thrau pushed a commit to localstack/copilot-cli-local that referenced this pull request Dec 9, 2022
Previously the Aurora addons use the default KMS key for app by importing the exported KMS resource. However, it's not able to`ImportValue` if the service uses a different account. This PR fixes this issue by creating a dedicated KMS resource for each Aurora addon.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants