Merge branch 'main' of https://github.com/microsoft/AzureTRE into pr/…

…marrobi/3807
microsoft · Nov 14, 2024 · 7a3188d · 7a3188d
2 parents cd8d1a8 + 158dce1
commit 7a3188d
Show file tree

Hide file tree

Showing 48 changed files with 175 additions and 216 deletions.
diff --git a/.github/actions/devcontainer_run_command/action.yml b/.github/actions/devcontainer_run_command/action.yml
@@ -186,9 +186,6 @@ runs:
           -e TF_INPUT="0" \
           -e TF_IN_AUTOMATION="1" \
           -e USE_ENV_VARS_NOT_FILES="true" \
-          -e ARM_STORAGE_USE_AZUREAD="true" \
-          -e ARM_USE_AZUREAD="true" \
-          -e ARM_USE_OIDC="true" \
           -e BUNDLE_TYPE="${{ inputs.BUNDLE_TYPE }}" \
           -e WORKSPACE_SERVICE_NAME="${{ inputs.WORKSPACE_SERVICE_NAME }}" \
           -e ARM_ENVIRONMENT="${{ env.ARM_ENVIRONMENT }}" \

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,7 @@
 FEATURES:
 
 ENHANCEMENTS:
+* Key Vaults should use RBAC instead of access policies for access control ([#4000](https://github.com/microsoft/AzureTRE/issues/4000))
 * Split log entries with [Log chunk X of Y] for better readability. ([[#3992](https://github.com/microsoft/AzureTRE/issues/3992)
 * Expose APP_SERVICE_SKU build variable to allow enablement of App Gateway WAF ([#4111](https://github.com/microsoft/AzureTRE/pull/4111))
 * Update Terraform to use Azure AD authentication rather than storage account keys ([#4103](https://github.com/microsoft/AzureTRE/issues/4103))

diff --git a/Makefile b/Makefile
@@ -1,4 +1,4 @@
-.PHONY: bootstrap-init mgmt-deploy mgmt-destroy build-api-image push-api-image deploy-tre destroy-tre letsencrypt
+.PHONY: bootstrap-init mgmt-deploy mgmt-destroy build-api-image push-api-image tre-deploy tre-destroy letsencrypt
 .DEFAULT_GOAL := help
 
 SHELL:=/bin/bash

diff --git a/core/terraform/.terraform.lock.hcl b/core/terraform/.terraform.lock.hcl
diff --git a/core/terraform/airlock/service_bus.tf b/core/terraform/airlock/service_bus.tf
@@ -3,35 +3,35 @@ resource "azurerm_servicebus_queue" "step_result" {
   name         = local.step_result_queue_name
   namespace_id = var.airlock_servicebus.id
 
-  enable_partitioning = false
+  partitioning_enabled = false
 }
 
 resource "azurerm_servicebus_queue" "status_changed" {
   name         = local.status_changed_queue_name
   namespace_id = var.airlock_servicebus.id
 
-  enable_partitioning = false
+  partitioning_enabled = false
 }
 
 resource "azurerm_servicebus_queue" "scan_result" {
   name         = local.scan_result_queue_name
   namespace_id = var.airlock_servicebus.id
 
-  enable_partitioning = false
+  partitioning_enabled = false
 }
 
 resource "azurerm_servicebus_queue" "data_deletion" {
   name         = local.data_deletion_queue_name
   namespace_id = var.airlock_servicebus.id
 
-  enable_partitioning = false
+  partitioning_enabled = false
 }
 
 resource "azurerm_servicebus_topic" "blob_created" {
   name         = local.blob_created_topic_name
   namespace_id = var.airlock_servicebus.id
 
-  enable_partitioning = false
+  partitioning_enabled = false
 }
 
 resource "azurerm_servicebus_subscription" "airlock_processor" {

diff --git a/core/terraform/api-webapp.tf b/core/terraform/api-webapp.tf
@@ -85,8 +85,8 @@ resource "azurerm_linux_web_app" "api" {
     ftps_state                                    = "Disabled"
 
     application_stack {
-      docker_image     = "${local.docker_registry_server}/${var.api_image_repository}"
-      docker_image_tag = local.version
+      docker_registry_url = "https://${local.docker_registry_server}"
+      docker_image_name   = "${var.api_image_repository}:${local.version}"
     }
 
     cors {

diff --git a/core/terraform/appgateway/certificate.tf b/core/terraform/appgateway/certificate.tf
@@ -1,15 +1,7 @@
-resource "azurerm_key_vault_access_policy" "app_gw_managed_identity" {
-  key_vault_id = var.keyvault_id
-  tenant_id    = azurerm_user_assigned_identity.agw_id.tenant_id
-  object_id    = azurerm_user_assigned_identity.agw_id.principal_id
-
-  key_permissions = [
-    "Get",
-  ]
-
-  secret_permissions = [
-    "Get",
-  ]
+resource "azurerm_role_assignment" "keyvault_appgw_role" {
+  scope                = var.keyvault_id
+  role_definition_name = "Key Vault Secrets User"
+  principal_id         = azurerm_user_assigned_identity.agw_id.principal_id // id-agw-<TRE_ID>
 }
 
 resource "azurerm_key_vault_certificate" "tlscert" {

diff --git a/core/terraform/cosmos_mongo.tf b/core/terraform/cosmos_mongo.tf
@@ -1,12 +1,12 @@
 resource "azurerm_cosmosdb_account" "mongo" {
-  name                      = "cosmos-mongo-${var.tre_id}"
-  location                  = azurerm_resource_group.core.location
-  resource_group_name       = azurerm_resource_group.core.name
-  offer_type                = "Standard"
-  kind                      = "MongoDB"
-  enable_automatic_failover = false
-  mongo_server_version      = 4.2
-  ip_range_filter           = "${local.azure_portal_cosmos_ips}${var.enable_local_debugging ? ",${local.myip}" : ""}"
+  name                       = "cosmos-mongo-${var.tre_id}"
+  location                   = azurerm_resource_group.core.location
+  resource_group_name        = azurerm_resource_group.core.name
+  offer_type                 = "Standard"
+  kind                       = "MongoDB"
+  automatic_failover_enabled = false
+  mongo_server_version       = 4.2
+  ip_range_filter            = "${local.azure_portal_cosmos_ips}${var.enable_local_debugging ? ",${local.myip}" : ""}"
 
   capabilities {
     name = "EnableServerless"
@@ -93,11 +93,11 @@ resource "azurerm_private_endpoint" "mongo" {
 
 resource "azurerm_key_vault_secret" "cosmos_mongo_connstr" {
   name         = "porter-db-connection-string"
-  value        = azurerm_cosmosdb_account.mongo.connection_strings[0]
+  value        = azurerm_cosmosdb_account.mongo.primary_mongodb_connection_string
   key_vault_id = azurerm_key_vault.kv.id
   tags         = local.tre_core_tags
   depends_on = [
-    azurerm_key_vault_access_policy.deployer
+    azurerm_role_assignment.keyvault_deployer_role
   ]
 
   lifecycle { ignore_changes = [tags] }

diff --git a/core/terraform/json-to-env.sh b/core/terraform/json-to-env.sh
@@ -29,6 +29,10 @@ jq -r '
             "path": "keyvault_uri",
             "env_var": "KEYVAULT_URI"
         },
+        {
+            "path": "keyvault_resource_id",
+            "env_var": "KEYVAULT_RESOURCE_ID"
+        },
         {
             "path": "azure_tre_fqdn",
             "env_var": "FQDN"

diff --git a/core/terraform/keyvault.tf b/core/terraform/keyvault.tf
@@ -1,34 +1,26 @@
 resource "azurerm_key_vault" "kv" {
-  name                     = "kv-${var.tre_id}"
-  tenant_id                = data.azurerm_client_config.current.tenant_id
-  location                 = azurerm_resource_group.core.location
-  resource_group_name      = azurerm_resource_group.core.name
-  sku_name                 = "standard"
-  purge_protection_enabled = var.kv_purge_protection_enabled
-  tags                     = local.tre_core_tags
+  name                      = "kv-${var.tre_id}"
+  tenant_id                 = data.azurerm_client_config.current.tenant_id
+  location                  = azurerm_resource_group.core.location
+  resource_group_name       = azurerm_resource_group.core.name
+  sku_name                  = "standard"
+  enable_rbac_authorization = true
+  purge_protection_enabled  = var.kv_purge_protection_enabled
+  tags                      = local.tre_core_tags
 
   lifecycle { ignore_changes = [access_policy, tags] }
 }
 
-resource "azurerm_key_vault_access_policy" "deployer" {
-  key_vault_id = azurerm_key_vault.kv.id
-  tenant_id    = data.azurerm_client_config.current.tenant_id
-  object_id    = data.azurerm_client_config.current.object_id
-
-  key_permissions         = ["Get", "List", "Update", "Create", "Import", "Delete", "Recover"]
-  secret_permissions      = ["Get", "List", "Set", "Delete", "Purge", "Recover"]
-  certificate_permissions = ["Get", "List", "Update", "Create", "Import", "Delete", "Purge", "Recover"]
-  storage_permissions     = ["Get", "List", "Update", "Delete"]
+resource "azurerm_role_assignment" "keyvault_deployer_role" {
+  scope                = azurerm_key_vault.kv.id
+  role_definition_name = "Key Vault Administrator"
+  principal_id         = data.azurerm_client_config.current.object_id // deployer - either CICD service principal or local user
 }
 
-resource "azurerm_key_vault_access_policy" "managed_identity" {
-  key_vault_id = azurerm_key_vault.kv.id
-  tenant_id    = azurerm_user_assigned_identity.id.tenant_id
-  object_id    = azurerm_user_assigned_identity.id.principal_id
-
-  key_permissions         = ["Get", "List", ]
-  secret_permissions      = ["Get", "List", ]
-  certificate_permissions = ["Get", "List", ]
+resource "azurerm_role_assignment" "keyvault_apiidentity_role" {
+  scope                = azurerm_key_vault.kv.id
+  role_definition_name = "Key Vault Secrets User"
+  principal_id         = azurerm_user_assigned_identity.id.principal_id // id-api-<TRE_ID>
 }
 
 data "azurerm_private_dns_zone" "vaultcore" {
@@ -68,7 +60,7 @@ resource "azurerm_key_vault_secret" "api_client_id" {
   key_vault_id = azurerm_key_vault.kv.id
   tags         = local.tre_core_tags
   depends_on = [
-    azurerm_key_vault_access_policy.deployer
+    azurerm_role_assignment.keyvault_deployer_role
   ]
 
   lifecycle { ignore_changes = [tags] }
@@ -80,7 +72,7 @@ resource "azurerm_key_vault_secret" "api_client_secret" {
   key_vault_id = azurerm_key_vault.kv.id
   tags         = local.tre_core_tags
   depends_on = [
-    azurerm_key_vault_access_policy.deployer
+    azurerm_role_assignment.keyvault_deployer_role
   ]
 
   lifecycle { ignore_changes = [tags] }
@@ -92,7 +84,7 @@ resource "azurerm_key_vault_secret" "auth_tenant_id" {
   key_vault_id = azurerm_key_vault.kv.id
   tags         = local.tre_core_tags
   depends_on = [
-    azurerm_key_vault_access_policy.deployer
+    azurerm_role_assignment.keyvault_deployer_role
   ]
 
   lifecycle { ignore_changes = [tags] }
@@ -104,7 +96,7 @@ resource "azurerm_key_vault_secret" "application_admin_client_id" {
   key_vault_id = azurerm_key_vault.kv.id
   tags         = local.tre_core_tags
   depends_on = [
-    azurerm_key_vault_access_policy.deployer
+    azurerm_role_assignment.keyvault_deployer_role
   ]
 
   lifecycle { ignore_changes = [tags] }
@@ -116,7 +108,7 @@ resource "azurerm_key_vault_secret" "application_admin_client_secret" {
   key_vault_id = azurerm_key_vault.kv.id
   tags         = local.tre_core_tags
   depends_on = [
-    azurerm_key_vault_access_policy.deployer
+    azurerm_role_assignment.keyvault_deployer_role
   ]
 
   lifecycle { ignore_changes = [tags] }

diff --git a/core/terraform/main.tf b/core/terraform/main.tf
@@ -104,7 +104,7 @@ module "appgateway" {
   depends_on = [
     module.network,
     azurerm_key_vault.kv,
-    azurerm_key_vault_access_policy.deployer,
+    azurerm_role_assignment.keyvault_deployer_role,
     azurerm_private_endpoint.api_private_endpoint
   ]
 }
@@ -175,7 +175,7 @@ module "resource_processor_vmss_porter" {
     module.network,
     module.azure_monitor,
     azurerm_key_vault.kv,
-    azurerm_key_vault_access_policy.deployer
+    azurerm_role_assignment.keyvault_deployer_role
   ]
 }
 

diff --git a/core/terraform/modules_move_definitions.tf b/core/terraform/modules_move_definitions.tf
@@ -148,16 +148,6 @@ moved {
   to   = azurerm_key_vault.kv
 }
 
-moved {
-  from = module.keyvault.azurerm_key_vault_access_policy.deployer
-  to   = azurerm_key_vault_access_policy.deployer
-}
-
-moved {
-  from = module.keyvault.azurerm_key_vault_access_policy.managed_identity
-  to   = azurerm_key_vault_access_policy.managed_identity
-}
-
 moved {
   from = module.keyvault.azurerm_private_endpoint.kvpe
   to   = azurerm_private_endpoint.kvpe

diff --git a/core/terraform/network/network.tf b/core/terraform/network/network.tf
@@ -27,7 +27,7 @@ resource "azurerm_subnet" "app_gw" {
   virtual_network_name                          = azurerm_virtual_network.core.name
   resource_group_name                           = var.resource_group_name
   address_prefixes                              = [local.app_gw_subnet_address_prefix]
-  private_endpoint_network_policies_enabled     = false
+  private_endpoint_network_policies             = "Disabled"
   private_link_service_network_policies_enabled = true
   depends_on                                    = [azurerm_subnet.azure_firewall]
 }
@@ -37,7 +37,7 @@ resource "azurerm_subnet" "web_app" {
   virtual_network_name                          = azurerm_virtual_network.core.name
   resource_group_name                           = var.resource_group_name
   address_prefixes                              = [local.web_app_subnet_address_prefix]
-  private_endpoint_network_policies_enabled     = false
+  private_endpoint_network_policies             = "Disabled"
   private_link_service_network_policies_enabled = true
   depends_on                                    = [azurerm_subnet.app_gw]
 
@@ -57,8 +57,8 @@ resource "azurerm_subnet" "shared" {
   resource_group_name  = var.resource_group_name
   address_prefixes     = [local.shared_services_subnet_address_prefix]
   # notice that private endpoints do not adhere to NSG rules
-  private_endpoint_network_policies_enabled = false
-  depends_on                                = [azurerm_subnet.web_app]
+  private_endpoint_network_policies = "Disabled"
+  depends_on                        = [azurerm_subnet.web_app]
 }
 
 resource "azurerm_subnet" "resource_processor" {
@@ -67,8 +67,8 @@ resource "azurerm_subnet" "resource_processor" {
   resource_group_name  = var.resource_group_name
   address_prefixes     = [local.resource_processor_subnet_address_prefix]
   # notice that private endpoints do not adhere to NSG rules
-  private_endpoint_network_policies_enabled = false
-  depends_on                                = [azurerm_subnet.shared]
+  private_endpoint_network_policies = "Disabled"
+  depends_on                        = [azurerm_subnet.shared]
 }
 
 resource "azurerm_subnet" "airlock_processor" {
@@ -77,8 +77,8 @@ resource "azurerm_subnet" "airlock_processor" {
   resource_group_name  = var.resource_group_name
   address_prefixes     = [local.airlock_processor_subnet_address_prefix]
   # notice that private endpoints do not adhere to NSG rules
-  private_endpoint_network_policies_enabled = false
-  depends_on                                = [azurerm_subnet.resource_processor]
+  private_endpoint_network_policies = "Disabled"
+  depends_on                        = [azurerm_subnet.resource_processor]
 
   delegation {
     name = "delegation"
@@ -100,8 +100,8 @@ resource "azurerm_subnet" "airlock_notification" {
   resource_group_name  = var.resource_group_name
   address_prefixes     = [local.airlock_notifications_subnet_address_prefix]
   # notice that private endpoints do not adhere to NSG rules
-  private_endpoint_network_policies_enabled = false
-  depends_on                                = [azurerm_subnet.airlock_processor]
+  private_endpoint_network_policies = "Disabled"
+  depends_on                        = [azurerm_subnet.airlock_processor]
 
   delegation {
     name = "delegation"
@@ -120,8 +120,8 @@ resource "azurerm_subnet" "airlock_storage" {
   resource_group_name  = var.resource_group_name
   address_prefixes     = [local.airlock_storage_subnet_address_prefix]
   # notice that private endpoints do not adhere to NSG rules
-  private_endpoint_network_policies_enabled = false
-  depends_on                                = [azurerm_subnet.airlock_notification]
+  private_endpoint_network_policies = "Disabled"
+  depends_on                        = [azurerm_subnet.airlock_notification]
 }
 
 resource "azurerm_subnet" "airlock_events" {
@@ -130,8 +130,8 @@ resource "azurerm_subnet" "airlock_events" {
   resource_group_name  = var.resource_group_name
   address_prefixes     = [local.airlock_events_subnet_address_prefix]
   # notice that private endpoints do not adhere to NSG rules
-  private_endpoint_network_policies_enabled = false
-  depends_on                                = [azurerm_subnet.airlock_storage]
+  private_endpoint_network_policies = "Disabled"
+  depends_on                        = [azurerm_subnet.airlock_storage]
 
   # Eventgrid CAN'T send messages over private endpoints, hence we need to allow service endpoints to the service bus
   # We are using service endpoints + managed identity to send these messaages

diff --git a/core/terraform/outputs.tf b/core/terraform/outputs.tf
@@ -30,6 +30,10 @@ output "keyvault_uri" {
   value = azurerm_key_vault.kv.vault_uri
 }
 
+output "keyvault_resource_id" {
+  value = azurerm_key_vault.kv.id
+}
+
 output "service_bus_resource_id" {
   value = azurerm_servicebus_namespace.sb.id
 }
@@ -59,7 +63,7 @@ output "state_store_endpoint" {
 }
 
 output "cosmosdb_mongo_endpoint" {
-  value     = azurerm_cosmosdb_account.mongo.connection_strings[0]
+  value     = azurerm_cosmosdb_account.mongo.primary_sql_connection_string
   sensitive = true
 }