feat: Add kms encryption to eks #12

zythosec · 2022-02-11T15:43:44Z

Adds cluster_encryption_config to EKS cluster for cluster secrets
Switches out local secret to secretsmanager secret

modules/database/main.tf

vanpelt

Looks good, it feels redundant and likely un-necessary to store the database secret in KMS. The secret is in the terraform state, you're taking that secret and storing it in KMS, then you're reading that value from the KMS to put it back into the terraform state 🤔 .

It's eventually going to end up in an environment variable that get's passed to k8s. We should look at the kubernetes module and ensure it's using k8s secrets for the MYSQL env variable. If done properly in the end we would have:

tfstate -> kms -> tfstate -> k8s secrets -> kms -> docker container env 🙃

vanpelt · 2022-02-12T00:51:01Z

main.tf

-  database_subnet_cidrs     = var.network_database_subnet_cidrs
-  create_elasticache_subnet = var.create_elasticache
-  elasticache_subnet_cidrs  = var.network_elasticache_subnet_cidrs
+  cidr                     = var.network_cidr


Why did create_elasticache_subnet go away here?

Bad rebase. Will fix.

jsbroks · 2022-02-12T21:44:31Z

variables.tf

@@ -203,6 +203,12 @@ variable "kubernetes_map_users" {
  default = []
 }

+variable "eks_kms_key_arn" {


for consistency we should probably use kubernetes_kms_key_arn

Im also confused why we need to pass one in? We already create a kms key. Lets use that, and unless a user requests for the ability to provide one, then we can add it as a variable (i.e, its easier to add options then remove options).

My thought when separating this out from the bucket KMS key was to give flexibility to the consumer of the module. If they want to pass in separate keys to encrypt the bucket vs the eks cluster secrets, they have the ability to do so, or they can pass in the same KMS key to both variables.

For now I would leave it as internal. And default to the internal key. Ethier way if the user doesn't provide a kms_key_arn we should default to the one we create

jsbroks · 2022-02-12T21:46:31Z

modules/database/main.tf

+resource "aws_secretsmanager_secret_version" "default" {
+  secret_id     = aws_secretsmanager_secret.default.id
+  secret_string = random_string.master_password.result
+}


Im not sure I see the point in this. We are already storing it in the TF state

So it can be rotated by AWS automatically and not have to be manually managed by the user. Since that involves a separate code change to be able to handle a rotating secret, I've opted to leave it out of this PR.

This is just the initial version. Eventually, the secret will not be in state because it will have been rotated by AWS.

Maybe we should leave it out of this PR until the other PR is ready. As you said its involues a seperate code change. That way they can all be reviewed together, and we limit the number of weird cases/one offs

I'll take it out then. The rotation piece that needs to be added is https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/secretsmanager_secret_rotation with the code change for future reference.

zythosec · 2022-02-14T14:42:05Z

@vanpelt

The secret is in the terraform state, you're taking that secret and storing it in KMS, then you're reading that value from the KMS to put it back into the terraform state

There is no way to pass an initial value of a secret to an aws_secretsmanager_secret, you have to create an aws_secretsmanager_secret_version. The reason for this is so we can eventually add rotation to the aws_secretsmanager_secret, which will then discard this initial value.

jsbroks · 2022-02-15T16:05:16Z

@zythosec Did you run a running to upgrade an existing deployment before these changes? I just want to make sure we arn't introducing any breaking changes? If so Im good to approve

zythosec · 2022-02-24T13:53:37Z

@jsbroks had some initial issues before I had removed all the mysql secret components. Now that only have the EKS/KMS items are in this PR, these changes apply cleanly. Steps I took:

git checkout main
terraform plan -out tfplan
terraform apply tfplan
git checkout kms-eks-secrets
terraform plan -out tfplan
terraform apply tfplan

Tested out signup/login/intro colab/reports and all look good.

## [1.1.0](v1.0.4...v1.1.0) (2022-02-24) ### Features * Add kms encryption to eks ([#12](#12)) ([71774c7](71774c7))

jsbroks · 2022-02-24T19:33:24Z

This PR is included in version 1.1.0 🎉

zythosec requested review from jsbroks, elainaRenee and vanpelt as code owners February 11, 2022 15:43

zythosec force-pushed the kms-eks-secrets branch from 26b1ea5 to 0459168 Compare February 11, 2022 15:45

zythosec requested review from elainaRenee and vanpelt and removed request for vanpelt and elainaRenee February 11, 2022 15:45

feat: Add kms encryption to eks and add mysql secret to secretsmanager

cb395a9

zythosec force-pushed the kms-eks-secrets branch from 0459168 to cb395a9 Compare February 11, 2022 15:52

github-actions bot reviewed Feb 11, 2022

View reviewed changes

modules/database/main.tf Outdated Show resolved Hide resolved

fix: Use kms_key_arn for mysql secret

c695929

zythosec changed the title ~~feat: add kms encryption to eks and add mysql secret to secretsmanager~~ feat: Add kms encryption to eks and add mysql secret to secretsmanager Feb 11, 2022

vanpelt approved these changes Feb 12, 2022

View reviewed changes

jsbroks requested changes Feb 12, 2022

View reviewed changes

Ross Nanopoulos added 4 commits February 15, 2022 10:56

Revert secrets manager secret

713b1c9

Fix rebase. Use local.kms_key_arn instead of variable

be6f0df

Fix whitespace

b377e68

Update example

f02d520

zythosec changed the title ~~feat: Add kms encryption to eks and add mysql secret to secretsmanager~~ feat: Add kms encryption to eks Feb 23, 2022

zythosec requested a review from jsbroks February 24, 2022 13:53

jsbroks approved these changes Feb 24, 2022

View reviewed changes

zythosec merged commit 71774c7 into main Feb 24, 2022

zythosec deleted the kms-eks-secrets branch February 24, 2022 19:32

jsbroks pushed a commit that referenced this pull request Feb 24, 2022

chore(release): version 1.1.0 [skip ci]

a589214

## [1.1.0](v1.0.4...v1.1.0) (2022-02-24) ### Features * Add kms encryption to eks ([#12](#12)) ([71774c7](71774c7))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add kms encryption to eks #12

feat: Add kms encryption to eks #12

zythosec commented Feb 11, 2022 •

edited

Loading

vanpelt left a comment

vanpelt Feb 12, 2022

zythosec Feb 14, 2022

jsbroks Feb 12, 2022

jsbroks Feb 12, 2022 •

edited

Loading

zythosec Feb 14, 2022

jsbroks Feb 14, 2022 •

edited

Loading

jsbroks Feb 12, 2022

zythosec Feb 14, 2022

zythosec Feb 14, 2022

jsbroks Feb 14, 2022

zythosec Feb 14, 2022

zythosec commented Feb 14, 2022

jsbroks commented Feb 15, 2022 •

edited

Loading

zythosec commented Feb 24, 2022

jsbroks commented Feb 24, 2022

feat: Add kms encryption to eks #12

feat: Add kms encryption to eks #12

Conversation

zythosec commented Feb 11, 2022 • edited Loading

vanpelt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsbroks Feb 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsbroks Feb 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zythosec commented Feb 14, 2022

jsbroks commented Feb 15, 2022 • edited Loading

zythosec commented Feb 24, 2022

jsbroks commented Feb 24, 2022

zythosec commented Feb 11, 2022 •

edited

Loading

jsbroks Feb 12, 2022 •

edited

Loading

jsbroks Feb 14, 2022 •

edited

Loading

jsbroks commented Feb 15, 2022 •

edited

Loading