diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index df2e3f39a..935e7f4e3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -974,6 +974,20 @@ jobs: terraform -chdir=iac/federation/azure-target/terraform fmt -check -recursive terraform -chdir=iac/federation/azure-target/terraform test + # The runtime identity's admin-password write grant must cover exactly + # the one secret the app writes, and exist only when the sync is armed + # (#199). + - name: Test Azure compute modules + if: matrix.cloud == 'azure' + run: | + for t in terraform/modules/compute/azure/*/tests; do + [ -d "$t" ] || continue + d=$(dirname "$t") + terraform -chdir="$d" init -backend=false -lockfile=readonly -input=false + terraform -chdir="$d" fmt -check -recursive + terraform -chdir="$d" test + done + - name: Test AWS compute modules if: matrix.cloud == 'aws' run: | diff --git a/internal/server/app.go b/internal/server/app.go index 42c1a1a2f..1af91125a 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -992,14 +992,25 @@ func buildAdminPasswordSyncCallback(store auth.StoreInterface, resolver secrets. return func(ctx context.Context, userID, newPassword string) { user, err := store.GetUserByID(ctx, userID) - if err != nil || user == nil || user.Email != adminEmail { + if err != nil { + // Cannot tell whether this was the admin, so the secret manager copy + // may now be stale. Say so instead of skipping silently. + logging.Errorf("Admin password sync skipped: could not load user %s: %v; if this user is the admin, secret %q may no longer match the database password", userID, err, adminPasswordSecret) + return + } + if user == nil || user.Email != adminEmail { return } if err := resolver.PutSecret(ctx, adminPasswordSecret, newPassword); err != nil { - logging.Warnf("Failed to sync admin password to secret manager: %v", err) - } else { - logging.Infof("Admin password synced to secret manager") + // The password change is already committed to the database, so the + // two copies have now diverged. ERROR, not WARN: anything recovering + // from the secret manager copy gets the old password (#199). The + // usual cause is a runtime identity without write access to this + // one secret. + logging.Errorf("Admin password sync FAILED: secret %q no longer matches the database password (check the runtime identity can write this secret): %v", adminPasswordSecret, err) + return } + logging.Infof("Admin password synced to secret manager") } } diff --git a/internal/server/app_coverage_test.go b/internal/server/app_coverage_test.go index 42cd12501..209f512a5 100644 --- a/internal/server/app_coverage_test.go +++ b/internal/server/app_coverage_test.go @@ -3,6 +3,7 @@ package server import ( "context" "errors" + "strings" "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" @@ -123,6 +124,75 @@ func TestBuildAdminPasswordSyncCallback_AdminEmailMatches(t *testing.T) { testutil.AssertEqual(t, "newpass", resolver.putValue) } +// #199: a failed write (for example a 403 because the runtime identity has +// read-only access to the vault) leaves the secret manager copy stale while +// the database already holds the new password. That must be an ERROR naming +// the secret, not a WARN that reads as harmless. +func TestBuildAdminPasswordSyncCallback_PutSecretFailureLogsError(t *testing.T) { + testutil.SetEnv(t, "ADMIN_PASSWORD_SECRET", "admin-password") + testutil.SetEnv(t, "ADMIN_EMAIL", "admin@example.com") + logs := captureLogs(t) + + store := &mockAuthStoreWithUser{ + user: &auth.User{ID: "user-123", Email: "admin@example.com"}, + } + resolver := &mockSecretResolver{putErr: errors.New("403 Forbidden: setSecret not permitted")} + + cb := buildAdminPasswordSyncCallback(store, resolver) + cb(context.Background(), "user-123", "newpass") + + testutil.AssertEqual(t, "admin-password", resolver.putKey) + errs := errorLines(logs.String()) + testutil.AssertEqual(t, 1, len(errs)) + testutil.AssertContains(t, errs[0], `"admin-password"`) + testutil.AssertContains(t, errs[0], "403 Forbidden") + testutil.AssertContains(t, errs[0], "no longer matches the database password") + testutil.AssertFalse(t, strings.Contains(logs.String(), "newpass"), "the password must never be logged") + testutil.AssertFalse(t, strings.Contains(logs.String(), "[WARN]"), "the failure must not be downgraded to WARN") + testutil.AssertFalse(t, strings.Contains(logs.String(), "synced to secret manager"), "a failed sync must not also report success") +} + +// #199: when the user lookup fails the callback cannot tell whether the admin +// changed their password, so it must say the copy may be stale instead of +// returning silently. +func TestBuildAdminPasswordSyncCallback_UserLookupFailureLogsError(t *testing.T) { + testutil.SetEnv(t, "ADMIN_PASSWORD_SECRET", "admin-password") + testutil.SetEnv(t, "ADMIN_EMAIL", "admin@example.com") + logs := captureLogs(t) + + store := &mockAuthStoreWithUser{err: errors.New("connection reset")} + resolver := &mockSecretResolver{} + + cb := buildAdminPasswordSyncCallback(store, resolver) + cb(context.Background(), "user-123", "newpass") + + testutil.AssertEqual(t, "", resolver.putKey) + errs := errorLines(logs.String()) + testutil.AssertEqual(t, 1, len(errs)) + testutil.AssertContains(t, errs[0], "user-123") + testutil.AssertContains(t, errs[0], "connection reset") + testutil.AssertContains(t, errs[0], `"admin-password"`) + testutil.AssertFalse(t, strings.Contains(logs.String(), "newpass"), "the password must never be logged") +} + +// The quiet paths stay quiet: a successful sync and a non-admin user log no +// ERROR, so the new ERROR lines keep meaning "the copies diverged". +func TestBuildAdminPasswordSyncCallback_QuietPathsLogNoError(t *testing.T) { + testutil.SetEnv(t, "ADMIN_PASSWORD_SECRET", "admin-password") + testutil.SetEnv(t, "ADMIN_EMAIL", "admin@example.com") + logs := captureLogs(t) + + admin := &mockAuthStoreWithUser{user: &auth.User{ID: "user-123", Email: "admin@example.com"}} + other := &mockAuthStoreWithUser{user: &auth.User{ID: "user-456", Email: "other@example.com"}} + + buildAdminPasswordSyncCallback(admin, &mockSecretResolver{})(context.Background(), "user-123", "newpass") + buildAdminPasswordSyncCallback(other, &mockSecretResolver{})(context.Background(), "user-456", "newpass") + buildAdminPasswordSyncCallback(&mockAuthStoreForHealth{}, &mockSecretResolver{})(context.Background(), "user-789", "newpass") + + testutil.AssertEqual(t, 0, len(errorLines(logs.String()))) + testutil.AssertContains(t, logs.String(), "Admin password synced to secret manager") +} + // ----- Close with nil DB (already covered in app_test.go; duplicate here for clarity) ----- func TestClose_NilDB_NoPanic(t *testing.T) { diff --git a/terraform/modules/compute/azure/aks/.terraform.lock.hcl b/terraform/modules/compute/azure/aks/.terraform.lock.hcl index 2ea85c8d1..4c43bed82 100644 --- a/terraform/modules/compute/azure/aks/.terraform.lock.hcl +++ b/terraform/modules/compute/azure/aks/.terraform.lock.hcl @@ -5,6 +5,8 @@ provider "registry.terraform.io/hashicorp/azurerm" { version = "3.117.1" constraints = "~> 3.0" hashes = [ + "h1:3c9iOEtBMnHrpJLlhbQ0sCZPWhE/2dvEPcL8KkXAh7w=", + "h1:MufLWHQWuFt+iX9bthLooPP3dDZloujkzaIZPW5A/p4=", "h1:j6wnjpHfBcQC4xd3ZYquaIPIIR46xJQs7rxwPdSOZos=", "zh:0c513676836e3c50d004ece7d2624a8aff6faac14b833b96feeac2e4bc2c1c12", "zh:50ea01ada95bae2f187db9e926e463f45d860767a85ebc59160414e00e76c35d", @@ -25,6 +27,8 @@ provider "registry.terraform.io/hashicorp/helm" { version = "2.17.0" constraints = "~> 2.11" hashes = [ + "h1:0LSHBFqJvHTzQesUwagpDLsrzVliY+t2c26nDJizHFM=", + "h1:K5FEjxvDnxb1JF1kG1xr8J3pNGxoaR3Z0IBG9Csm/Is=", "h1:kQMkcPVvHOguOqnxoEU2sm1ND9vCHiT8TvZ2x6v/Rsw=", "zh:06fb4e9932f0afc1904d2279e6e99353c2ddac0d765305ce90519af410706bd4", "zh:104eccfc781fc868da3c7fec4385ad14ed183eb985c96331a1a937ac79c2d1a7", @@ -45,6 +49,8 @@ provider "registry.terraform.io/hashicorp/kubernetes" { version = "2.38.0" constraints = "~> 2.23" hashes = [ + "h1:5CkveFo5ynsLdzKk+Kv+r7+U9rMrNjfZPT3a0N/fhgE=", + "h1:7nJdsd1RMPBtOjDXidB37+KSDN5VcOWkbkow69qJVGc=", "h1:soK8Lt0SZ6dB+HsypFRDzuX/npqlMU6M0fvyaR1yW0k=", "zh:0af928d776eb269b192dc0ea0f8a3f0f5ec117224cd644bdacdc682300f84ba0", "zh:1be998e67206f7cfc4ffe77c01a09ac91ce725de0abaec9030b22c0a832af44f", @@ -65,7 +71,9 @@ provider "registry.terraform.io/hashicorp/time" { version = "0.13.1" constraints = "~> 0.11" hashes = [ + "h1:+W+DMrVoVnoXo3f3M4W+OpZbkCrUn6PnqDF33D2Cuf0=", "h1:ZT5ppCNIModqk3iOkVt5my8b8yBHmDpl663JtXAIRqM=", + "h1:eSX+RgYfeumojmeJ9Y5CbnkH2NkBC+9vUEolqdQVtGw=", "zh:02cb9aab1002f0f2a94a4f85acec8893297dc75915f7404c165983f720a54b74", "zh:04429b2b31a492d19e5ecf999b116d396dac0b24bba0d0fb19ecaefe193fdb8f", "zh:26f8e51bb7c275c404ba6028c1b530312066009194db721a8427a7bc5cdbc83a", diff --git a/terraform/modules/compute/azure/aks/main.tf b/terraform/modules/compute/azure/aks/main.tf index 07b051e67..416d7b193 100644 --- a/terraform/modules/compute/azure/aks/main.tf +++ b/terraform/modules/compute/azure/aks/main.tf @@ -201,6 +201,25 @@ resource "azurerm_role_assignment" "workload_kv_secrets_user" { principal_id = azurerm_user_assigned_identity.workload.principal_id } +# Write access to the admin-password secret ONLY (#199). The deployment below +# sets ADMIN_PASSWORD_SECRET and ADMIN_EMAIL, which arms the app's +# admin-password sync (buildAdminPasswordSyncCallback in internal/server/app.go): +# on an admin password change it calls azsecrets.SetSecret, which the read-only +# grant above 403s, leaving the vault copy stale. +# +# Scoped to the one secret rather than the vault, so the vault-wide grant stays +# read-only and the extra rights of "Key Vault Secrets Officer" (no built-in +# role is set-only) reach this secret and nothing else. The scope is built from +# the vault ARM ID and the secret name, both known at plan time, because scope +# is ForceNew. The count mirrors the app-side gate: both env vars non-empty. +resource "azurerm_role_assignment" "workload_admin_password_secret_writer" { + count = var.admin_password_secret_name != "" && var.admin_email != "" ? 1 : 0 + + scope = "${var.key_vault_id}/secrets/${var.admin_password_secret_name}" + role_definition_name = "Key Vault Secrets Officer" + principal_id = azurerm_user_assigned_identity.workload.principal_id +} + # Grant AKS cluster identity access to pull images from ACR resource "azurerm_role_assignment" "aks_acr" { principal_id = azurerm_kubernetes_cluster.main.kubelet_identity[0].object_id diff --git a/terraform/modules/compute/azure/aks/tests/admin_password_writer.tftest.hcl b/terraform/modules/compute/azure/aks/tests/admin_password_writer.tftest.hcl new file mode 100644 index 000000000..fcd717de5 --- /dev/null +++ b/terraform/modules/compute/azure/aks/tests/admin_password_writer.tftest.hcl @@ -0,0 +1,151 @@ +# The AKS deployment sets ADMIN_PASSWORD_SECRET and ADMIN_EMAIL, which arms the +# app's admin-password sync (azsecrets.SetSecret, internal/server/app.go). The +# workload identity's vault-wide grant is the read-only "Key Vault Secrets +# User", so it needs a write grant on that one secret and nowhere else (#199). +# +# Every run is a targeted plan. Under mock providers the cluster's +# kubelet_identity block (Optional+Computed) plans as an empty list, so +# aks_acr's kubelet_identity[0] cannot be evaluated, and apply-time teardown +# re-plans the whole module. Targeting keeps aks_acr out of the graph. The +# grant's principal_id is therefore unknown here and not asserted; the +# container-apps module's test applies and checks the same binding pattern. +mock_provider "azurerm" {} +mock_provider "kubernetes" {} +mock_provider "helm" {} +mock_provider "time" {} + +variables { + project_name = "cudly" + environment = "dev" + resource_group_name = "cudly-test-rg" + location = "eastus2" + vnet_subnet_id = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/cudly-test-rg/providers/Microsoft.Network/virtualNetworks/vnet/subnets/aks" + image_name = "cudlytest.azurecr.io/cudly" + database_host = "cudly-test-postgres.postgres.database.azure.com" + database_name = "cudly" + database_username = "cudlyadmin" + database_password_secret_name = "database-password" + key_vault_id = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/cudly-test-rg/providers/Microsoft.KeyVault/vaults/cudly-test-kv" + admin_email = "admin@example.com" + admin_password_secret_name = "admin-password" +} + +run "sync_armed_grants_write_on_that_secret_only" { + command = plan + + plan_options { + target = [ + azurerm_role_assignment.workload_admin_password_secret_writer, + azurerm_role_assignment.workload_kv_secrets_user, + ] + } + + assert { + condition = length(azurerm_role_assignment.workload_admin_password_secret_writer) == 1 + error_message = "With ADMIN_PASSWORD_SECRET and ADMIN_EMAIL both set the sync is armed, so the workload identity needs a write grant or SetSecret 403s." + } + + assert { + condition = azurerm_role_assignment.workload_admin_password_secret_writer[0].scope == "${var.key_vault_id}/secrets/admin-password" + error_message = "The write grant must be scoped to the admin-password secret itself, not the vault or anything wider." + } + + assert { + condition = azurerm_role_assignment.workload_admin_password_secret_writer[0].role_definition_name == "Key Vault Secrets Officer" + error_message = "Key Vault Secrets Officer is the built-in role that carries secrets/setSecret/action." + } + + # Nothing broader: the vault-wide grant stays read-only. + assert { + condition = azurerm_role_assignment.workload_kv_secrets_user.scope == var.key_vault_id && azurerm_role_assignment.workload_kv_secrets_user.role_definition_name == "Key Vault Secrets User" + error_message = "The vault-wide grant must stay the read-only Key Vault Secrets User; only the single-secret grant may write." + } +} + +# The granted secret must be the one the pods are told to write. +run "granted_secret_is_the_one_the_pods_write" { + command = plan + + plan_options { + target = [ + azurerm_role_assignment.workload_admin_password_secret_writer, + azurerm_role_assignment.workload_kv_secrets_user, + kubernetes_deployment.app, + ] + } + + variables { + deploy_kubernetes_resources = true + } + + assert { + condition = anytrue([ + for e in kubernetes_deployment.app[0].spec[0].template[0].spec[0].container[0].env : + e.name == "ADMIN_PASSWORD_SECRET" && e.value == "admin-password" + ]) + error_message = "The granted secret must be the one passed to the pods as ADMIN_PASSWORD_SECRET." + } + + assert { + condition = azurerm_role_assignment.workload_admin_password_secret_writer[0].scope == "${var.key_vault_id}/secrets/admin-password" + error_message = "The single-secret scope must be known at plan time (scope is ForceNew)." + } +} + +run "no_admin_email_means_no_write_grant" { + command = plan + + plan_options { + target = [ + azurerm_role_assignment.workload_admin_password_secret_writer, + azurerm_role_assignment.workload_kv_secrets_user, + ] + } + + variables { + admin_email = "" + } + + assert { + condition = length(azurerm_role_assignment.workload_admin_password_secret_writer) == 0 + error_message = "Without ADMIN_EMAIL the sync callback never arms, so no write grant may exist." + } +} + +run "no_secret_name_means_no_write_grant" { + command = plan + + plan_options { + target = [ + azurerm_role_assignment.workload_admin_password_secret_writer, + azurerm_role_assignment.workload_kv_secrets_user, + ] + } + + variables { + admin_password_secret_name = "" + } + + assert { + condition = length(azurerm_role_assignment.workload_admin_password_secret_writer) == 0 + error_message = "Without ADMIN_PASSWORD_SECRET the sync callback never arms, so no write grant may exist." + } +} + +# A path in the name would move the grant's scope off the single secret. +run "secret_name_with_a_path_is_rejected" { + command = plan + + plan_options { + target = [ + azurerm_role_assignment.workload_admin_password_secret_writer, + azurerm_role_assignment.workload_kv_secrets_user, + ] + } + + variables { + admin_password_secret_name = "admin-password/../../secrets/other" + } + + expect_failures = [var.admin_password_secret_name] +} diff --git a/terraform/modules/compute/azure/aks/variables.tf b/terraform/modules/compute/azure/aks/variables.tf index 435e523a8..5cfef00bf 100644 --- a/terraform/modules/compute/azure/aks/variables.tf +++ b/terraform/modules/compute/azure/aks/variables.tf @@ -122,9 +122,17 @@ variable "admin_email" { } variable "admin_password_secret_name" { - description = "Key Vault secret name containing admin password" + description = "Key Vault secret name containing admin password. When set together with admin_email, the runtime identity gets write access to this one secret for the admin-password sync (#199)." type = string default = "" + + # The write grant's RBAC scope is /secrets/, so a + # value carrying a "/" (or anything else outside Key Vault's secret-name + # alphabet) could move that scope somewhere other than exactly one secret. + validation { + condition = var.admin_password_secret_name == "" || can(regex("^[0-9A-Za-z-]{1,127}$", var.admin_password_secret_name)) + error_message = "admin_password_secret_name must be empty or a bare Key Vault secret name (1-127 letters, digits and hyphens), not a path or ID." + } } variable "auto_migrate" { diff --git a/terraform/modules/compute/azure/container-apps/.terraform.lock.hcl b/terraform/modules/compute/azure/container-apps/.terraform.lock.hcl index 7a7d90232..3106ce6ef 100644 --- a/terraform/modules/compute/azure/container-apps/.terraform.lock.hcl +++ b/terraform/modules/compute/azure/container-apps/.terraform.lock.hcl @@ -5,6 +5,8 @@ provider "registry.terraform.io/hashicorp/azurerm" { version = "3.117.1" constraints = "~> 3.0" hashes = [ + "h1:3c9iOEtBMnHrpJLlhbQ0sCZPWhE/2dvEPcL8KkXAh7w=", + "h1:MufLWHQWuFt+iX9bthLooPP3dDZloujkzaIZPW5A/p4=", "h1:j6wnjpHfBcQC4xd3ZYquaIPIIR46xJQs7rxwPdSOZos=", "zh:0c513676836e3c50d004ece7d2624a8aff6faac14b833b96feeac2e4bc2c1c12", "zh:50ea01ada95bae2f187db9e926e463f45d860767a85ebc59160414e00e76c35d", diff --git a/terraform/modules/compute/azure/container-apps/main.tf b/terraform/modules/compute/azure/container-apps/main.tf index c84c35401..fa25b507c 100644 --- a/terraform/modules/compute/azure/container-apps/main.tf +++ b/terraform/modules/compute/azure/container-apps/main.tf @@ -324,6 +324,32 @@ resource "azurerm_role_assignment" "signing_key_crypto_user" { principal_id = azurerm_user_assigned_identity.container_app.principal_id } +# Write access to the admin-password secret ONLY (#199), for the admin-password +# sync: when the admin changes their password the app writes the new value back +# with azsecrets.SetSecret (buildAdminPasswordSyncCallback in +# internal/server/app.go). The vault-wide grant in +# terraform/environments/azure/compute.tf is "Key Vault Secrets User", which is +# read-only, so without this the write 403s and the vault copy goes stale. +# +# Scope is the one secret, not the vault: Key Vault RBAC accepts a role +# assignment on .../vaults//secrets/, so the delete/purge/backup +# rights that come with "Key Vault Secrets Officer" (no built-in role is +# set-only) reach this secret and nothing else. The scope is built from the +# vault ARM ID and the secret name, both known at plan time, rather than from +# the secret's resource_versionless_id: scope is ForceNew, and an +# apply-time-only value would replace the grant whenever it went unknown. +# +# The count mirrors the app-side gate exactly: the sync callback is armed only +# when ADMIN_PASSWORD_SECRET and ADMIN_EMAIL are both non-empty, so the write +# grant exists only when something will use it. +resource "azurerm_role_assignment" "admin_password_secret_writer" { + count = var.admin_password_secret_name != "" && var.admin_email != "" ? 1 : 0 + + scope = "${var.key_vault_id}/secrets/${var.admin_password_secret_name}" + role_definition_name = "Key Vault Secrets Officer" + principal_id = azurerm_user_assigned_identity.container_app.principal_id +} + # ============================================== # Diagnostic Settings # ============================================== diff --git a/terraform/modules/compute/azure/container-apps/tests/admin_password_writer.tftest.hcl b/terraform/modules/compute/azure/container-apps/tests/admin_password_writer.tftest.hcl new file mode 100644 index 000000000..3df1146ab --- /dev/null +++ b/terraform/modules/compute/azure/container-apps/tests/admin_password_writer.tftest.hcl @@ -0,0 +1,125 @@ +# The admin-password sync writes the new password back to Key Vault +# (azsecrets.SetSecret, internal/server/app.go). The vault-wide grant the +# Container App identity holds is the read-only "Key Vault Secrets User", so +# the identity needs a write grant on that one secret and nowhere else (#199). +mock_provider "azurerm" { + # Resources referenced by ID elsewhere in the module are parsed, so the mock + # needs well-formed ARM IDs. + mock_resource "azurerm_user_assigned_identity" { + defaults = { + id = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/cudly-test-rg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/cudly-test-identity" + principal_id = "22222222-2222-2222-2222-222222222222" + client_id = "33333333-3333-3333-3333-333333333333" + } + } + mock_resource "azurerm_container_app_environment" { + defaults = { + id = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/cudly-test-rg/providers/Microsoft.App/managedEnvironments/cudly-test-env" + } + } +} + +variables { + app_name = "cudly-test" + environment = "dev" + resource_group_name = "cudly-test-rg" + location = "eastus2" + image_uri = "cudlytest.azurecr.io/cudly:test" + database_host = "cudly-test-postgres.postgres.database.azure.com" + database_name = "cudly" + database_username = "cudlyadmin" + database_password_secret_name = "database-password" + subscription_id = "00000000-0000-0000-0000-000000000000" + reservation_role_definition_id = "/subscriptions/00000000-0000-0000-0000-000000000000/providers/Microsoft.Authorization/roleDefinitions/11111111-1111-1111-1111-111111111111" + key_vault_uri = "https://cudly-test-kv.vault.azure.net/" + key_vault_id = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/cudly-test-rg/providers/Microsoft.KeyVault/vaults/cudly-test-kv" + signing_key_name = "oidc-signing-key" + registry_server = "cudlytest.azurecr.io" + container_registry_id = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/cudly-test-rg/providers/Microsoft.ContainerRegistry/registries/cudlytest" + admin_email = "admin@example.com" + admin_password_secret_name = "admin-password" + enable_scheduled_tasks = false + infrastructure_subnet_id = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/cudly-test-rg/providers/Microsoft.Network/virtualNetworks/vnet/subnets/apps" + internal_load_balancer_enabled = false +} + +run "sync_armed_grants_write_on_that_secret_only" { + command = apply + + assert { + condition = length(azurerm_role_assignment.admin_password_secret_writer) == 1 + error_message = "With ADMIN_PASSWORD_SECRET and ADMIN_EMAIL both set the sync is armed, so the identity needs a write grant or SetSecret 403s." + } + + assert { + condition = azurerm_role_assignment.admin_password_secret_writer[0].scope == "${var.key_vault_id}/secrets/admin-password" + error_message = "The write grant must be scoped to the admin-password secret itself, not the vault or anything wider." + } + + assert { + condition = azurerm_role_assignment.admin_password_secret_writer[0].role_definition_name == "Key Vault Secrets Officer" + error_message = "Key Vault Secrets Officer is the built-in role that carries secrets/setSecret/action." + } + + assert { + condition = azurerm_role_assignment.admin_password_secret_writer[0].principal_id == azurerm_user_assigned_identity.container_app.principal_id + error_message = "The write grant must go to the Container App's own identity, the one the app authenticates as (AZURE_CLIENT_ID)." + } + + # The secret named in the grant must be the one the app writes. + assert { + condition = anytrue([ + for e in azurerm_container_app.main.template[0].container[0].env : + e.name == "ADMIN_PASSWORD_SECRET" && e.value == "admin-password" + ]) + error_message = "The granted secret must be the one passed to the app as ADMIN_PASSWORD_SECRET." + } + + # Nothing broader: no other grant in this module carries a secrets write role. + assert { + condition = alltrue([ + azurerm_role_assignment.signing_key_crypto_user.role_definition_name == "Key Vault Crypto User", + azurerm_role_assignment.acr_pull.role_definition_name == "AcrPull", + azurerm_role_assignment.cost_management_reader.role_definition_name == "Cost Management Reader", + azurerm_role_assignment.subscription_reader.role_definition_name == "Reader", + ]) + error_message = "Only the per-secret grant may carry Key Vault Secrets Officer." + } +} + +run "no_admin_email_means_no_write_grant" { + command = plan + + variables { + admin_email = "" + } + + assert { + condition = length(azurerm_role_assignment.admin_password_secret_writer) == 0 + error_message = "Without ADMIN_EMAIL the sync callback never arms, so no write grant may exist." + } +} + +run "no_secret_name_means_no_write_grant" { + command = plan + + variables { + admin_password_secret_name = "" + } + + assert { + condition = length(azurerm_role_assignment.admin_password_secret_writer) == 0 + error_message = "Without ADMIN_PASSWORD_SECRET the sync callback never arms, so no write grant may exist." + } +} + +# A path in the name would move the grant's scope off the single secret. +run "secret_name_with_a_path_is_rejected" { + command = plan + + variables { + admin_password_secret_name = "admin-password/../../secrets/other" + } + + expect_failures = [var.admin_password_secret_name] +} diff --git a/terraform/modules/compute/azure/container-apps/variables.tf b/terraform/modules/compute/azure/container-apps/variables.tf index c605deb77..96b533a6f 100644 --- a/terraform/modules/compute/azure/container-apps/variables.tf +++ b/terraform/modules/compute/azure/container-apps/variables.tf @@ -154,9 +154,17 @@ variable "admin_email" { } variable "admin_password_secret_name" { - description = "Key Vault secret name containing admin password" + description = "Key Vault secret name containing admin password. When set together with admin_email, the runtime identity gets write access to this one secret for the admin-password sync (#199)." type = string default = "" + + # The write grant's RBAC scope is /secrets/, so a + # value carrying a "/" (or anything else outside Key Vault's secret-name + # alphabet) could move that scope somewhere other than exactly one secret. + validation { + condition = var.admin_password_secret_name == "" || can(regex("^[0-9A-Za-z-]{1,127}$", var.admin_password_secret_name)) + error_message = "admin_password_secret_name must be empty or a bare Key Vault secret name (1-127 letters, digits and hyphens), not a path or ID." + } } variable "allowed_origins" {