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

Support cross-account customer-managed key for RDS and S3 #175

Open
abhinavg6 opened this issue Feb 5, 2024 · 2 comments
Open

Support cross-account customer-managed key for RDS and S3 #175

abhinavg6 opened this issue Feb 5, 2024 · 2 comments
Labels
enhancement New feature or request Priority-P1 High priority designation

Comments

@abhinavg6
Copy link

abhinavg6 commented Feb 5, 2024

Currently we support encryption of the RDS database and instance-level S3 bucket using the key managed by the deployment owner (W&B in case of dedicated cloud, and W&B customer in case of self-managed). We would like to ensure that it could also work with a cross-account KMS key, such that W&B could encrypt the RDS & S3 for a dedicated cloud deployment using a KMS key provided by the W&B customer from their AWS account.

With this work, we should also provide the flexibility to use different keys for the database and the bucket. We shoudn't be forced to use the same key for both (which I think is the case today).

This would need cross-account testing, and we would like to get the IAM permissions / policies that the W&B customer would need to enable on their KMS key. Additionally from a risk mitigation perspective, it would also be good to have a set of instructions for how to prevent revoke or deletion of the customer's KMS key as such an action can render the entire deployment inactive / useless.

@abhinavg6 abhinavg6 added enhancement New feature or request Priority-P1 High priority designation labels Feb 5, 2024
@zacharyblasczyk
Copy link
Contributor

Implement a variable database_kms_key_arn that is similar in implementation to bucket_kms_key_arn.

  1. We should pass the var.database_kms_key_arn to the app_eks.database_kms_key_arn and create a (node policy)[https://github.com/wandb/terraform-aws-wandb/blob/main/main.tf#L130] like we do for bucket_kms_key_arn if this is set.
  2. If var.database_kms_key_arn is set, we should be using it as a turnery on https://github.com/wandb/terraform-aws-wandb/blob/main/main.tf#L11 local.kms_key_arn. This will get passed to https://github.com/wandb/terraform-aws-wandb/blob/main/main.tf#L66

@jsbroks @nfoucha, We use local.kms_key_arn on (redis)[https://github.com/wandb/terraform-aws-wandb/blob/main/main.tf#L200C29-L200C46] and for (file_storage)[https://github.com/wandb/terraform-aws-wandb/blob/main/main.tf#L24] in addition to the (database)[https://github.com/wandb/terraform-aws-wandb/blob/main/main.tf#L62C9-L62C17]

Do we want to allow for separate KMS encryption of each of them?

There is also the issue of the performance_insights_kms_key_arn
https://github.com/wandb/terraform-aws-wandb/blob/main/main.tf#L67C3-L67C35

Please advice about how we want to default these if we revamp/reuse local.kms_key_arn.

@nfoucha
Copy link
Contributor

nfoucha commented Feb 14, 2024

Here's a quick breakdown of the desired logic for the bucket:

# This part of the logic exists in the code
if var.bucket_name is set:
    Use customer supplied bucket and kms key arn from var.bucket_kms_key_arn
# This part of the logic needs to be created
else if var.bucket_kms_key_arn is set:
    Use w&b created bucket and kms key arn from var.bucket_kms_key_arn
# This part of the logic exists in the code (file_storage module, line 16 in main.tf)
else
    Use w&b created bucket and kms key arn from w&b created key (from kms module, line 1 in main.tf)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Priority-P1 High priority designation
Projects
None yet
Development

No branches or pull requests

3 participants