feat: updates kms comp with embedded policy creation #523
Conversation
![bridgecrew[bot]](https://avatars.githubusercontent.com/in/52968?s=60&v=4){kind=small}
| @@ -1,15 +1,80 @@ | |||
| locals { | |||
| account_id = data.aws_caller_identity.current.account_id | |||
| account_principal = "arn:aws:iam::${local.account_id}:root" | |||
There was a problem hiding this comment.
Let's use the data source here for govcloud
| account_principal = "arn:aws:iam::${local.account_id}:root" | |
| account_principal = "arn:${data.aws_partition.current.partition}:iam::${local.account_id}:root" |
https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/partition
|
|
||
| principals { | ||
| type = "AWS" | ||
| identifiers = local.administration_principals |
There was a problem hiding this comment.
| identifiers = local.administration_principals | |
| identifiers = [local.account_principal] |
| administration_principals = [ | ||
| local.account_principal | ||
| ] | ||
|
|
There was a problem hiding this comment.
This local doesn't seem very useful
| administration_principals = [ | |
| local.account_principal | |
| ] |
| ))) | ||
| } | ||
|
|
||
| data "aws_caller_identity" "current" {} |
There was a problem hiding this comment.
| data "aws_caller_identity" "current" {} | |
| data "aws_caller_identity" "current" {} | |
| data "aws_partition" "current" {} |
| variable "description" { | ||
| type = string | ||
| description = "The description for the KMS Key." | ||
| default = null |
There was a problem hiding this comment.
If the description isn't set to the original value of Parameter Store KMS master key, won't that cause people's kms keys to be recreated?
There was a problem hiding this comment.
I'm not sure honestly as it has been a long time, but updated regardless 😅
| default = "Parameter Store KMS master key" | ||
| description = "The description of the key as viewed in AWS console" | ||
| default = "ENCRYPT_DECRYPT" | ||
| description = "Specifies the intended use of the key. Valid values: `ENCRYPT_DECRYPT` or `SIGN_VERIFY`." |
There was a problem hiding this comment.
Should we add a validation rule here or is this already done by the upstream module?
| type = string | ||
| default = "" | ||
| description = "The display name of the alias. The name must start with the word `alias` followed by a forward slash. If not specified, the alias name will be auto-generated." | ||
| default = "SYMMETRIC_DEFAULT" |
There was a problem hiding this comment.
Is this a breaking change or is this the normal default and we're just being explicit?
There was a problem hiding this comment.
This is the downstream child module default. I believe all is good on this one, not a breaking change. I've added validation.
| "kms:*", | ||
| ] | ||
|
|
||
| resources = ["*"] |
There was a problem hiding this comment.
It would be good to allow overriding the resources here so users of this component can give granular permissions to kms. These would also allay warnings by bridgecrew shown above.
There was a problem hiding this comment.
resources in this case is just the key that is being attached to, so I don't believe it is ever needed to be overwritten. If it is not supposed, AWS states that the policy will not work. In AWS docs, this is always * in their examples. See docs here.
| identifiers = local.principals | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of hard coding this policy here, can we utilize this module and expose the inputs as variables so we can be as flexible as possible from the YAML?
https://github.com/cloudposse/terraform-aws-iam-policy
e.g.
components:
terraform:
kms:
vars:
policy:
iam_policy_statements:
KeyAdministration:
effect: "Allow"
actions:
- "kms:*"
resources:
- "*"
principals:
type: "AWS"
# This may be something we can contribute to the upstream module to dynamically fill these %s in
# or it can be added by the HCL for this component
identifiers:
- "arn:%s:iam::%s:root"
KeyUsage:
effect: "Allow"
actions:
- "kms:Encrypt"
- "kms:Decrypt"
- "kms:ReEncrypt*"
- "kms:GenerateDataKey*"
- "kms:DescribeKey"
resources:
- "*"
principals:
type: "AWS"
# Same here, these can be provided by the HCL unless this `identifiers` key is provided
identifiers:
- "..."There was a problem hiding this comment.
While this is totally valid, it's non-trivial and others haven't done this work / needed this yet, so to move this PR along I'm going to consider this out of scope. If we decide we need this to merge, I can make the changes necessary 👍
|
@nitrocode thanks for the review! Sorry it took me so long to circle back to it 😄 I couldn't complete this PR since I can't push to the branch I started here due to lack of permissions to push to cc @goruha |
what
why
aws-team-roleroles into the KMS policy, so we do something like "Admins in the dev account have access to use this Key"references