kms: allow user to pass separate kms keys for s3 and db#165
kms: allow user to pass separate kms keys for s3 and db#165Jeff McCune (jeffmccune) wants to merge 3 commits intobraintrustdata:mainfrom
Conversation
Without this patch the user may only pass one kms arn for use for all resources managed by Braintrust. This is problem for our enviornment where two KMS keys are provided which can only be used for the respective S3 and RDS services. As a result, we must plumb through both kms arns to the respective S3 and RDS modules.
|
Mike Deeks (@mdeeks) Note you'll need to cherry pick this then tag a v3.1.x release unless v4 supports tofu now. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
|
|
||
| deployment_name = var.deployment_name | ||
| kms_key_arn = local.kms_key_arn | ||
| kms_key_arn = local.kms_key_arn_s3 |
There was a problem hiding this comment.
Switching storage to local.kms_key_arn_s3 encrypts the buckets with a potentially different key, but the IAM policies in modules/services-common (e.g., iam-api.tf lines 114-122 and iam-brainstore.tf lines 152-159) still grant KMS permissions only to local.kms_key_arn. When a user supplies distinct S3 and base keys—the main new scenario—those service roles will lack kms:Encrypt/Decrypt on the bucket’s key, so reads/writes against the encrypted buckets will fail with AccessDenied. Please include the S3-specific key in those IAM policies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good to know.
Without this patch we get the following failure because the cmk we use
with kms for rds may not also be used for ssm.
```
│ Error: creating Secrets Manager Secret (a209178-prod-brain/DatabaseSecret-20251204183547822800000008): operation error Secrets Manager: CreateSecret, https response error StatusCode: 400, RequestID: 8081dad5-b5c5-4a3e-b701-f4e67aba1125, api error AccessDeniedException: Access to KMS is not allowed
│
│ with module.braintrust-data-plane.module.database.aws_secretsmanager_secret.database_secret,
│ on .terraform/modules/braintrust-data-plane/modules/database/main.tf line 153, in resource "aws_secretsmanager_secret" "database_secret":
│ 153: resource "aws_secretsmanager_secret" "database_secret" {
```
This patch resolves the error by using kms_key_arn_db only for the
aws_db_instance resource. The kms_key_arn is used for the
aws_secretmanager_secret.database_secret resource. The kms_key_arn
corresponds to the vendor managed KMS resource managed by the Braintrust
terraform module
Without this patch the kms_key_arn_s3 top level variable is not completly wired up as per braintrustdata#165 (comment) This patch wires up the IAM policies when kms_key_arn_s3 is injected to the root module.
Jeff McCune (@jeffmccune) v4.x.x should support OpenTofu. All it does is increase the AWS provider to v6. Were you seeing any errors? |
2 participants
Without this patch the user may only pass one kms arn for use for all resources managed by Braintrust. This is problem for our enviornment where two KMS keys are provided which can only be used for the respective S3 and RDS services.
As a result, we must plumb through both kms arns to the respective S3 and RDS modules.