From f8413cc98efac409afbebb86802fa3eca3853f5e Mon Sep 17 00:00:00 2001 From: Ludovico Magnocavallo Date: Sun, 11 Apr 2021 14:48:16 +0200 Subject: [PATCH] Add support for group-based IAM to resource management modules (#229) * group_iam support for organization * group_iam support for folder * fix typo in variable description * add group_iam to project module * update project module README --- modules/folder/README.md | 13 +- modules/folder/main.tf | 15 +- modules/folder/variables.tf | 128 +++++++++--------- modules/organization/README.md | 23 +++- modules/organization/main.tf | 15 +- modules/organization/variables.tf | 18 ++- modules/project/README.md | 3 +- modules/project/main.tf | 19 ++- modules/project/variables.tf | 10 +- tests/modules/organization/fixture/main.tf | 1 + .../modules/organization/fixture/variables.tf | 5 + tests/modules/organization/test_plan.py | 52 +++++-- 12 files changed, 206 insertions(+), 96 deletions(-) diff --git a/modules/folder/README.md b/modules/folder/README.md index d08bfac0..1a7e7055 100644 --- a/modules/folder/README.md +++ b/modules/folder/README.md @@ -1,6 +1,6 @@ # Google Cloud Folder Module -This module allows the creation and management of folders together with their individual IAM bindings and organization policies. +This module allows the creation and management of folders, including support for IAM bindings, organization policies, and hierarchical firewall rules. ## Examples @@ -11,11 +11,14 @@ module "folder" { source = "./modules/folder" parent = "organizations/1234567890" name = "Folder name" + group_iam = { + "cloud-owners@example.org" = ["roles/owner", "roles/projectCreator"] + } iam = { - "roles/owner" = ["group:users@example.com"] + "roles/owner" = ["user:one@example.com"] } } -# tftest:modules=1:resources=2 +# tftest:modules=1:resources=3 ``` ### Organization policies @@ -158,7 +161,6 @@ module "folder2" { # tftest:modules=2:resources=6 ``` - ## Variables @@ -168,7 +170,8 @@ module "folder2" { | *firewall_policies* | Hierarchical firewall policies to *create* in this folder. | map(map(object({...}))) | | {} | | *firewall_policy_attachments* | List of hierarchical firewall policy IDs to *attach* to this folder. | map(string) | | {} | | *folder_create* | Create folder. When set to false, uses id to reference an existing folder. | bool | | true | -| *iam* | IAM bindings in {ROLE => [MEMBERS]} format. | map(set(string)) | | {} | +| *group_iam* | Authoritative IAM binding for organization groups, in {GROUP_EMAIL => [ROLES]} format. Group emails need to be static. Can be used in combination with the `iam` variable. | map(list(string)) | | {} | +| *iam* | IAM bindings in {ROLE => [MEMBERS]} format. | map(list(string)) | | {} | | *id* | Folder ID in case you use folder_create=false | string | | null | | *logging_exclusions* | Logging exclusions for this folder in the form {NAME -> FILTER}. | map(string) | | {} | | *logging_sinks* | Logging sinks to create for this folder. | map(object({...})) | | {} | diff --git a/modules/folder/main.tf b/modules/folder/main.tf index 6816adf0..cab6b7f0 100644 --- a/modules/folder/main.tf +++ b/modules/folder/main.tf @@ -21,6 +21,19 @@ locals { merge(rule, { policy = policy, name = rule_name }) ] ]) + group_iam_roles = distinct(flatten(values(var.group_iam))) + group_iam = { + for r in local.group_iam_roles : r => [ + for k, v in var.group_iam : "group:${k}" if try(index(v, r), null) != null + ] + } + iam = { + for role in distinct(concat(keys(var.iam), keys(local.group_iam))) : + role => concat( + try(var.iam[role], []), + try(local.group_iam[role], []) + ) + } rules_map = { for rule in local.extended_rules : "${rule.policy}-${rule.name}" => rule @@ -59,7 +72,7 @@ resource "google_folder" "folder" { } resource "google_folder_iam_binding" "authoritative" { - for_each = var.iam + for_each = local.iam folder = local.folder.name role = each.key members = each.value diff --git a/modules/folder/variables.tf b/modules/folder/variables.tf index 7726029b..1b9aed86 100644 --- a/modules/folder/variables.tf +++ b/modules/folder/variables.tf @@ -14,9 +14,75 @@ * limitations under the License. */ +variable "contacts" { + description = "List of essential contacts for this resource. Must be in the form EMAIL -> [NOTIFICATION_TYPES]. Valid notification types are ALL, SUSPENSION, SECURITY, TECHNICAL, BILLING, LEGAL, PRODUCT_UPDATES" + type = map(list(string)) + default = {} +} + +variable "firewall_policies" { + description = "Hierarchical firewall policies to *create* in this folder." + type = map(map(object({ + description = string + direction = string + action = string + priority = number + ranges = list(string) + ports = map(list(string)) + target_service_accounts = list(string) + target_resources = list(string) + logging = bool + }))) + default = {} +} + +variable "firewall_policy_attachments" { + description = "List of hierarchical firewall policy IDs to *attach* to this folder." + type = map(string) + default = {} +} + +variable "folder_create" { + description = "Create folder. When set to false, uses id to reference an existing folder." + type = bool + default = true +} + +variable "group_iam" { + description = "Authoritative IAM binding for organization groups, in {GROUP_EMAIL => [ROLES]} format. Group emails need to be static. Can be used in combination with the `iam` variable." + type = map(list(string)) + default = {} +} + variable "iam" { description = "IAM bindings in {ROLE => [MEMBERS]} format." - type = map(set(string)) + type = map(list(string)) + default = {} +} + +variable "id" { + description = "Folder ID in case you use folder_create=false" + type = string + default = null +} + +variable "logging_sinks" { + description = "Logging sinks to create for this folder." + type = map(object({ + destination = string + type = string + filter = string + iam = bool + include_children = bool + # TODO exclusions also support description and disabled + exclusions = map(string) + })) + default = {} +} + +variable "logging_exclusions" { + description = "Logging exclusions for this folder in the form {NAME -> FILTER}." + type = map(string) default = {} } @@ -52,63 +118,3 @@ variable "policy_list" { })) default = {} } - -variable "firewall_policies" { - description = "Hierarchical firewall policies to *create* in this folder." - type = map(map(object({ - description = string - direction = string - action = string - priority = number - ranges = list(string) - ports = map(list(string)) - target_service_accounts = list(string) - target_resources = list(string) - logging = bool - }))) - default = {} -} - -variable "firewall_policy_attachments" { - description = "List of hierarchical firewall policy IDs to *attach* to this folder." - type = map(string) - default = {} -} - -variable "logging_sinks" { - description = "Logging sinks to create for this folder." - type = map(object({ - destination = string - type = string - filter = string - iam = bool - include_children = bool - # TODO exclusions also support description and disabled - exclusions = map(string) - })) - default = {} -} - -variable "logging_exclusions" { - description = "Logging exclusions for this folder in the form {NAME -> FILTER}." - type = map(string) - default = {} -} - -variable "folder_create" { - description = "Create folder. When set to false, uses id to reference an existing folder." - type = bool - default = true -} - -variable "id" { - description = "Folder ID in case you use folder_create=false" - type = string - default = null -} - -variable "contacts" { - description = "List of essential contacts for this resource. Must be in the form EMAIL -> [NOTIFICATION_TYPES]. Valid notification types are ALL, SUSPENSION, SECURITY, TECHNICAL, BILLING, LEGAL, PRODUCT_UPDATES" - type = map(list(string)) - default = {} -} diff --git a/modules/organization/README.md b/modules/organization/README.md index acc3f18f..c912c517 100644 --- a/modules/organization/README.md +++ b/modules/organization/README.md @@ -13,7 +13,12 @@ This module allows managing several organization properties: module "org" { source = "./modules/organization" organization_id = "organizations/1234567890" - iam = { "roles/projectCreator" = ["group:cloud-admins@example.org"] } + group_iam = { + "cloud-owners@example.org" = ["roles/owner", "roles/projectCreator"] + } + iam = { + "roles/projectCreator" = ["group:cloud-admins@example.org"] + } policy_boolean = { "constraints/compute.disableGuestAttributesAccess" = true "constraints/compute.skipDefaultNetworkCreation" = true @@ -27,10 +32,21 @@ module "org" { } } } -# tftest:modules=1:resources=4 +# tftest:modules=1:resources=5 ``` +## IAM + +There are several mutually exclusive ways of managing IAM in this module + +- non-authoritative via the `iam_additive` and `iam_additive_members` variables, where bindings created outside this module will coexist with those managed here +- authoritative via the `group_iam` and `iam` variables, where bindings created outside this module (eg in the console) will be removed at each `terraform apply` cycle if the same role is also managed here +- authoritative policy via the `iam_bindings_authoritative` variable, where any binding created outside this module (eg in the console) will be removed at each `terraform apply` cycle regardless of the role + +Some care must be takend with the `groups_iam` variable (and in some situations with the additive variables) to ensure that variable keys are static values, so that Terraform is able to compute the dependency graph. + ## Hierarchical firewall rules + ```hcl module "org" { source = "./modules/organization" @@ -60,6 +76,7 @@ module "org" { ``` ## Logging Sinks + ```hcl module "gcs" { source = "./modules/gcs" @@ -134,7 +151,6 @@ module "org" { # tftest:modules=5:resources=11 ``` - ## Variables @@ -145,6 +161,7 @@ module "org" { | *custom_roles* | Map of role name => list of permissions to create in this project. | map(list(string)) | | {} | | *firewall_policies* | Hierarchical firewall policies to *create* in the organization. | map(map(object({...}))) | | {} | | *firewall_policy_attachments* | List of hierarchical firewall policy IDs to *attach* to the organization | map(string) | | {} | +| *group_iam* | Authoritative IAM binding for organization groups, in {GROUP_EMAIL => [ROLES]} format. Group emails need to be static. Can be used in combination with the `iam` variable. | map(list(string)) | | {} | | *iam* | IAM bindings, in {ROLE => [MEMBERS]} format. | map(list(string)) | | {} | | *iam_additive* | Non authoritative IAM bindings, in {ROLE => [MEMBERS]} format. | map(list(string)) | | {} | | *iam_additive_members* | IAM additive bindings in {MEMBERS => [ROLE]} format. This might break if members are dynamic values. | map(list(string)) | | {} | diff --git a/modules/organization/main.tf b/modules/organization/main.tf index 9a16903c..22867ca3 100644 --- a/modules/organization/main.tf +++ b/modules/organization/main.tf @@ -16,6 +16,19 @@ locals { organization_id_numeric = split("/", var.organization_id)[1] + group_iam_roles = distinct(flatten(values(var.group_iam))) + group_iam = { + for r in local.group_iam_roles : r => [ + for k, v in var.group_iam : "group:${k}" if try(index(v, r), null) != null + ] + } + iam = { + for role in distinct(concat(keys(var.iam), keys(local.group_iam))) : + role => concat( + try(var.iam[role], []), + try(local.group_iam[role], []) + ) + } iam_additive_pairs = flatten([ for role, members in var.iam_additive : [ for member in members : { role = role, member = member } @@ -67,7 +80,7 @@ resource "google_organization_iam_custom_role" "roles" { } resource "google_organization_iam_binding" "authoritative" { - for_each = var.iam + for_each = local.iam org_id = local.organization_id_numeric role = each.key members = each.value diff --git a/modules/organization/variables.tf b/modules/organization/variables.tf index 1db13579..6cd42c8d 100644 --- a/modules/organization/variables.tf +++ b/modules/organization/variables.tf @@ -20,6 +20,12 @@ variable "custom_roles" { default = {} } +variable "group_iam" { + description = "Authoritative IAM binding for organization groups, in {GROUP_EMAIL => [ROLES]} format. Group emails need to be static. Can be used in combination with the `iam` variable." + type = map(list(string)) + default = {} +} + variable "iam" { description = "IAM bindings, in {ROLE => [MEMBERS]} format." type = map(list(string)) @@ -49,12 +55,6 @@ variable "iam_audit_config" { # } } -variable "iam_bindings_authoritative" { - description = "IAM authoritative bindings, in {ROLE => [MEMBERS]} format. Roles and members not explicitly listed will be cleared. Bindings should also be authoritative when using authoritative audit config. Use with caution." - type = map(list(string)) - default = null -} - variable "iam_audit_config_authoritative" { description = "IAM Authoritative service audit logging configuration. Service as key, map of log permission (eg DATA_READ) and excluded members as value for each service. Audit config should also be authoritative when using authoritative bindings. Use with caution." type = map(map(list(string))) @@ -66,6 +66,12 @@ variable "iam_audit_config_authoritative" { # } } +variable "iam_bindings_authoritative" { + description = "IAM authoritative bindings, in {ROLE => [MEMBERS]} format. Roles and members not explicitly listed will be cleared. Bindings should also be authoritative when using authoritative audit config. Use with caution." + type = map(list(string)) + default = null +} + variable "organization_id" { description = "Organization id in organizations/nnnnnn format." type = string diff --git a/modules/project/README.md b/modules/project/README.md index 2d99f977..8000082e 100644 --- a/modules/project/README.md +++ b/modules/project/README.md @@ -160,7 +160,8 @@ module "project-host" { | *billing_account* | Billing account id. | string | | null | | *contacts* | List of essential contacts for this resource. Must be in the form EMAIL -> [NOTIFICATION_TYPES]. Valid notification types are ALL, SUSPENSION, SECURITY, TECHNICAL, BILLING, LEGAL, PRODUCT_UPDATES | map(list(string)) | | {} | | *custom_roles* | Map of role name => list of permissions to create in this project. | map(list(string)) | | {} | -| *iam* | IAM bindings in {ROLE => [MEMBERS]} format. | map(set(string)) | | {} | +| *group_iam* | Authoritative IAM binding for organization groups, in {GROUP_EMAIL => [ROLES]} format. Group emails need to be static. Can be used in combination with the `iam` variable. | map(list(string)) | | {} | +| *iam* | IAM bindings in {ROLE => [MEMBERS]} format. | map(list(string)) | | {} | | *iam_additive* | IAM additive bindings in {ROLE => [MEMBERS]} format. | map(list(string)) | | {} | | *iam_additive_members* | IAM additive bindings in {MEMBERS => [ROLE]} format. This might break if members are dynamic values. | map(list(string)) | | {} | | *labels* | Resource labels. | map(string) | | {} | diff --git a/modules/project/main.tf b/modules/project/main.tf index dd9b0058..4f07a595 100644 --- a/modules/project/main.tf +++ b/modules/project/main.tf @@ -15,6 +15,19 @@ */ locals { + group_iam_roles = distinct(flatten(values(var.group_iam))) + group_iam = { + for r in local.group_iam_roles : r => [ + for k, v in var.group_iam : "group:${k}" if try(index(v, r), null) != null + ] + } + iam = { + for role in distinct(concat(keys(var.iam), keys(local.group_iam))) : + role => concat( + try(var.iam[role], []), + try(local.group_iam[role], []) + ) + } iam_additive_pairs = flatten([ for role, members in var.iam_additive : [ for member in members : { role = role, member = member } @@ -110,7 +123,7 @@ resource "google_project_service" "project_services" { # - additive (non-authoritative) roles might fail due to dynamic values resource "google_project_iam_binding" "authoritative" { - for_each = var.iam + for_each = local.iam project = local.project.project_id role = each.key members = each.value @@ -329,7 +342,7 @@ resource "google_essential_contacts_contact" "contact" { resource "google_access_context_manager_service_perimeter_resource" "service-perimeter-resource-standard" { count = var.service_perimeter_standard != null ? 1 : 0 - # If used, remember to uncomment 'lifecycle' block in the + # If used, remember to uncomment 'lifecycle' block in the # modules/vpc-sc/google_access_context_manager_service_perimeter resource. perimeter_name = var.service_perimeter_standard resource = "projects/${local.project.number}" @@ -338,7 +351,7 @@ resource "google_access_context_manager_service_perimeter_resource" "service-per resource "google_access_context_manager_service_perimeter_resource" "service-perimeter-resource-bridges" { for_each = toset(var.service_perimeter_bridges != null ? var.service_perimeter_bridges : []) - # If used, remember to uncomment 'lifecycle' block in the + # If used, remember to uncomment 'lifecycle' block in the # modules/vpc-sc/google_access_context_manager_service_perimeter resource. perimeter_name = each.value resource = "projects/${local.project.number}" diff --git a/modules/project/variables.tf b/modules/project/variables.tf index 58adb9f3..fa4c84da 100644 --- a/modules/project/variables.tf +++ b/modules/project/variables.tf @@ -32,9 +32,15 @@ variable "custom_roles" { default = {} } +variable "group_iam" { + description = "Authoritative IAM binding for organization groups, in {GROUP_EMAIL => [ROLES]} format. Group emails need to be static. Can be used in combination with the `iam` variable." + type = map(list(string)) + default = {} +} + variable "iam" { description = "IAM bindings in {ROLE => [MEMBERS]} format." - type = map(set(string)) + type = map(list(string)) default = {} } @@ -196,7 +202,7 @@ variable "contacts" { variable "service_perimeter_standard" { description = "Name of VPC-SC Standard perimeter to add project into. Specify the name in the form of 'accessPolicies/ACCESS_POLICY_NAME/servicePerimeters/PERIMETER_NAME'." type = string - default = null + default = null } diff --git a/tests/modules/organization/fixture/main.tf b/tests/modules/organization/fixture/main.tf index f155b2c5..b17a6535 100644 --- a/tests/modules/organization/fixture/main.tf +++ b/tests/modules/organization/fixture/main.tf @@ -18,6 +18,7 @@ module "test" { source = "../../../../modules/organization" organization_id = "organizations/1234567890" custom_roles = var.custom_roles + group_iam = var.group_iam iam = var.iam iam_additive = var.iam_additive iam_additive_members = var.iam_additive_members diff --git a/tests/modules/organization/fixture/variables.tf b/tests/modules/organization/fixture/variables.tf index 43a86639..eb4e807c 100644 --- a/tests/modules/organization/fixture/variables.tf +++ b/tests/modules/organization/fixture/variables.tf @@ -19,6 +19,11 @@ variable "custom_roles" { default = {} } +variable "group_iam" { + type = map(list(string)) + default = {} +} + variable "iam" { type = map(list(string)) default = {} diff --git a/tests/modules/organization/test_plan.py b/tests/modules/organization/test_plan.py index b2fcfe4d..168b836a 100644 --- a/tests/modules/organization/test_plan.py +++ b/tests/modules/organization/test_plan.py @@ -30,6 +30,32 @@ def test_audit_config(plan_runner): assert log_types == set(['DATA_READ', 'DATA_WRITE']) +def test_iam(plan_runner): + "Test IAM." + group_iam = ( + '{' + '"owners@example.org" = ["roles/owner", "roles/resourcemanager.folderAdmin"],' + '"viewers@example.org" = ["roles/viewer"]' + '}' + ) + iam = ( + '{' + '"roles/owner" = ["user:one@example.org", "user:two@example.org"],' + '"roles/browser" = ["domain:example.org"]' + '}' + ) + _, resources = plan_runner(FIXTURES_DIR, group_iam=group_iam, iam=iam) + roles = sorted([(r['values']['role'], sorted(r['values']['members'])) + for r in resources if r['type'] == 'google_organization_iam_binding']) + assert roles == [ + ('roles/browser', ['domain:example.org']), + ('roles/owner', ['group:owners@example.org', 'user:one@example.org', + 'user:two@example.org']), + ('roles/resourcemanager.folderAdmin', ['group:owners@example.org']), + ('roles/viewer', ['group:viewers@example.org']), + ] + + def test_iam_additive_members(plan_runner): "Test IAM additive members." iam = ( @@ -126,7 +152,7 @@ def test_firweall_policy(plan_runner): assert len(resources) == 4 policies = [r for r in resources - if r['type'] == 'google_compute_organization_security_policy'] + if r['type'] == 'google_compute_organization_security_policy'] assert len(policies) == 1 rules = [r for r in resources @@ -146,16 +172,16 @@ def test_firweall_policy(plan_runner): rule_values.append((name, index, action, direction, priority, config)) assert sorted(rule_values) == sorted([ - ('rule', 'policy1-allow-ingress', 'allow', 'INGRESS', 100,[ - { - 'dest_ip_ranges': None, - 'layer4_config': [{'ip_protocol': 'tcp', 'ports': ['22']}], - 'src_ip_ranges': ['10.0.0.0/8'] - }]), - ('rule', 'policy1-deny-egress', 'deny', 'EGRESS', 200, [ - { - 'dest_ip_ranges': ['192.168.0.0/24'], - 'layer4_config': [{'ip_protocol': 'tcp', 'ports': ['443']}], - 'src_ip_ranges': None - }]) + ('rule', 'policy1-allow-ingress', 'allow', 'INGRESS', 100, [ + { + 'dest_ip_ranges': None, + 'layer4_config': [{'ip_protocol': 'tcp', 'ports': ['22']}], + 'src_ip_ranges': ['10.0.0.0/8'] + }]), + ('rule', 'policy1-deny-egress', 'deny', 'EGRESS', 200, [ + { + 'dest_ip_ranges': ['192.168.0.0/24'], + 'layer4_config': [{'ip_protocol': 'tcp', 'ports': ['443']}], + 'src_ip_ranges': None + }]) ])