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

feat(eks): envelope encryption for secrets #9438

Merged
merged 3 commits into from
Aug 21, 2020

Conversation

davidsung
Copy link
Contributor

@davidsung davidsung commented Aug 4, 2020

feat(eks): envelope encryption for secrets

This PR adds envelope encryption support for Amazon EKS. Added a new key secretsEncryptionKey in ClusterProps for users to specify their own KMS CMK upon cluster creation:

new eks.Cluster(this, 'Cluster', {
  version: eks.KubernetesVersion.V1_16,
  secretsEncryptionKey,
});

Closes: #9140


Considerations

  1. Confirmed Secrets Encryption is enabled in the provisioned Amazon EKS (both standard resource AWS::EKS::Cluster and custom resource Custom::AWSCDK-EKS-Cluster) after running an integration test from scratch.
  2. By inspecting the CloudTrail logs after the integration test, confirmed the exact KMS IAM permission required for the cluster creation role as ['kms:Encrypt', 'kms:Decrypt', 'kms:DescribeKey', 'kms:CreateGrant']. Note: The encryption provider is using its own way to generate data encryption key, not using KMS GenerateDataKey, and hence IAM permissionkms:GenerateDataKey* is not required.

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

@mergify
Copy link
Contributor

mergify bot commented Aug 4, 2020

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@davidsung davidsung changed the title feat(eks): add envelope encryption support feat(eks): Add Envelope Encryption Support Aug 4, 2020
@davidsung davidsung changed the title feat(eks): Add Envelope Encryption Support feat(eks): add envelope encryption support Aug 4, 2020
@davidsung davidsung changed the title feat(eks): add envelope encryption support feat(eks): add envelope encryption feature upon cluster creation Aug 4, 2020
@iliapolo
Copy link
Contributor

iliapolo commented Aug 5, 2020

@davidsung this is ready for review yes?

@davidsung
Copy link
Contributor Author

@davidsung this is ready for review yes?

yes @iliapolo thanks!

@davidsung
Copy link
Contributor Author

@iliapolo As @eladb already started working on this #9472, feel free to close this one whenever you like.

eladb
eladb previously requested changes Aug 6, 2020
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Ha ha... Just submitted a PR for this yesterday as well... I'll close it in favor of this one.

One thing to note: I don't believe EKS supports updating the encryption configuration, so we need to fail in an update is attempted. See this.

Additionally, please add unit tests to the cluster provider - see this as a reference: https://github.com/aws/aws-cdk/pull/9472/files#diff-1cc6461fbdf09ac0273cadbbe91bd2fc

@eladb eladb assigned iliapolo and unassigned eladb Aug 6, 2020
@eladb
Copy link
Contributor

eladb commented Aug 6, 2020

@iliapolo As @eladb already started working on this #9472, feel free to close this one whenever you like.

@davidsung you be great if you want to follow up on this and complete it.

@eladb eladb changed the title feat(eks): add envelope encryption feature upon cluster creation feat(eks): envelope encryption for secrets Aug 6, 2020
@davidsung
Copy link
Contributor Author

Ha ha... Just submitted a PR for this yesterday as well... I'll close it in favor of this one.

One thing to note: I don't believe EKS supports updating the encryption configuration, so we need to fail in an update is attempted. See this.

Additionally, please add unit tests to the cluster provider - see this as a reference: https://github.com/aws/aws-cdk/pull/9472/files#diff-1cc6461fbdf09ac0273cadbbe91bd2fc

Exactly! I will handle the update event like what you did with the next PR. Thanks!

@mergify mergify bot dismissed eladb’s stale review August 6, 2020 18:36

Pull request has been modified.

@davidsung
Copy link
Contributor Author

@iliapolo I have followed all the suggestions from @eladb and submitted again. It's ready for review. Thanks!

@davidsung
Copy link
Contributor Author

@iliapolo As @eladb already started working on this #9472, feel free to close this one whenever you like.

@davidsung you be great if you want to follow up on this and complete it.

@eladb i have made the changes according to your advise. kindly review! Thanks!

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Waiting for AWS SDK to be updated in lambda

eladb
eladb previously approved these changes Aug 17, 2020
@iliapolo
Copy link
Contributor

Hi @davidsung - FYI The PR is approved but there are some merge conflicts that needs resolving.

@davidsung
Copy link
Contributor Author

Hi @davidsung - FYI The PR is approved but there are some merge conflicts that needs resolving.

I will rebase this upstream master branch on the forked repo and resolve the conflicts there. I hope this can be done within the next 2 days.

@davidsung davidsung force-pushed the eks-add-encryption-key branch from 0b6d0ee to fc7b307 Compare August 21, 2020 19:24
@mergify
Copy link
Contributor

mergify bot commented Aug 21, 2020

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@mergify mergify bot dismissed eladb’s stale review August 21, 2020 19:24

Pull request has been modified.

add envelope encryption support upon eks cluster creation
add update event handling for secretEncryptionKey mutation attempt
add test case for checking update event handling
update readme

Closes aws#9140

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@davidsung davidsung force-pushed the eks-add-encryption-key branch from fc7b307 to c764b9c Compare August 21, 2020 19:32
@davidsung
Copy link
Contributor Author

Hi @davidsung - FYI The PR is approved but there are some merge conflicts that needs resolving.

I will rebase this upstream master branch on the forked repo and resolve the conflicts there. I hope this can be done within the next 2 days.

Hi @iliapolo, I've resolved the conflicts. This PR is now ready for your review. Thanks!

@mergify
Copy link
Contributor

mergify bot commented Aug 21, 2020

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 69b9adc
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Aug 21, 2020

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

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.

[aws-eks] expose EncryptionConfig in eks.Cluster Construct
4 participants