-
Notifications
You must be signed in to change notification settings - Fork 751
Feat: enable kms centralized key usage tracking #1402
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: enable kms centralized key usage tracking #1402
Conversation
/gcbrun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/gcbrun |
3 similar comments
/gcbrun |
/gcbrun |
/gcbrun |
1-org/envs/shared/variables.tf
Outdated
variable "enable_kms_key_usage_tracking" { | ||
description = "Enable KMS centralized key usage tracking system." | ||
type = bool | ||
default = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend setting the default to true
because this is a recommended feature, and it doesn't have side-effects like extra cost that customers might want to avoid.
1-org/envs/shared/iam.tf
Outdated
@@ -154,3 +166,10 @@ resource "google_project_iam_member" "cai_monitoring_builder" { | |||
role = each.key | |||
member = "serviceAccount:${google_service_account.cai_monitoring_builder[0].email}" | |||
} | |||
|
|||
resource "google_organization_iam_member" "kms_protected_resources_viewer" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need an extra group for this, this can be removed.
Typically, the persona at a customer with the KMS Admin roles is the persona who wants to view KMS resources across the org. I can't think of a common scenario where a customer would want separate groups for kms_admin and kms_protected_resources_viewer
1-org/envs/shared/variables.tf
Outdated
scc_admin = optional(string, null) | ||
global_secrets_admin = optional(string, null) | ||
kms_admin = optional(string, null) | ||
kms_protected_resources_viewer = optional(string, null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as the IAM binding, I don't think we need an extra group for kms_protected_resources_viewer. This can be removed.
1-org/README.md
Outdated
@@ -82,6 +82,8 @@ to Bigquery and Pub/Sub. This will result in additional charges for those copies | |||
|
|||
- To use the **hub-and-spoke** architecture described in the **Networking** section of the [Google Cloud security foundations guide](https://cloud.google.com/architecture/security-foundations/networking#hub-and-spoke), set the `enable_hub_and_spoke` variable to `true`. | |||
|
|||
- As a KMS Administrator, you can [view all the keys in your organization from a centralized location](https://cloud.google.com/kms/docs/view-key-usage) by setting the `enable_kms_key_usage_tracking` variable to true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we will set enable_kms_key_usage
tracking to true, I recommend to also remove the extra directions from the readme. This helps keep the instructions more simple.
The documentation on what the variable does is already auto-generated in the readme.md under envs/shared, so we don't need redundant text here to describe each variable.
Hi @eeaton thank you for your review. I've made the necessary changes based on your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, LGTM
/gcbrun |
1 similar comment
/gcbrun |
/gcbrun |
/gcbrun |
/gcbrun |
/gcbrun |
1 similar comment
/gcbrun |
This PR enables the centralized View Key Usage KMS feature.