From 36b9f8f37811ad6919dd511723457fca29cd398e Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 08:52:31 +0000 Subject: [PATCH 1/2] fix(iac/azure): let the runtime identity write the admin-password secret, and log sync failures as errors Both Azure runtime identities held only the vault-wide, read-only "Key Vault Secrets User", but both deployments set ADMIN_PASSWORD_SECRET and ADMIN_EMAIL, which arms the admin-password sync. Its SetSecret call 403'd, the failure was logged at WARN, and the Key Vault copy silently drifted from the database password. Container Apps, which all three Azure environments deploy, was affected as well as AKS: the secrets module's writeable_secrets_role grant is count = 0 everywhere. - modules/compute/azure/{container-apps,aks}: a "Key Vault Secrets Officer" role assignment scoped to /secrets/ only, for the identity the app runs as. It exists only when the secret name and admin email are both set, the same condition that arms the sync callback. The scope is built from plan-time-known inputs because scope is ForceNew. The vault-wide grant stays read-only. admin_password_secret_name is now validated as a bare secret name so it cannot widen the scope. - internal/server: a failed sync write, or a failed user lookup that skips the sync, now logs at ERROR and names the secret that diverged. - Mocked terraform tests for both modules, run from a new CI step. The module lock files gain linux_amd64 and darwin_amd64 hashes so -lockfile=readonly init works on CI. The deploy SP already holds roleAssignments/write, so the bootstrap does not need re-applying. Closes #199 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 14 ++ internal/server/app.go | 19 ++- internal/server/app_coverage_test.go | 70 ++++++++ .../compute/azure/aks/.terraform.lock.hcl | 8 + terraform/modules/compute/azure/aks/main.tf | 19 +++ .../tests/admin_password_writer.tftest.hcl | 151 ++++++++++++++++++ .../modules/compute/azure/aks/variables.tf | 10 +- .../azure/container-apps/.terraform.lock.hcl | 2 + .../compute/azure/container-apps/main.tf | 26 +++ .../tests/admin_password_writer.tftest.hcl | 125 +++++++++++++++ .../compute/azure/container-apps/variables.tf | 10 +- 11 files changed, 448 insertions(+), 6 deletions(-) create mode 100644 terraform/modules/compute/azure/aks/tests/admin_password_writer.tftest.hcl create mode 100644 terraform/modules/compute/azure/container-apps/tests/admin_password_writer.tftest.hcl diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d8a230650..b88666bc9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -676,6 +676,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 fd92d96bf..59f5777fa 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -987,14 +987,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" { From dac5bf2f6950f272bc23345f8dafd3d7ba13667a Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 10:33:27 +0000 Subject: [PATCH 2/2] fix(iac/azure): keep Terraform from reverting the synced admin password Review of this PR (rD1, raising cristim's earlier cold-review point): the app's admin-password sync writes the admin-password Key Vault secret at runtime, but azurerm_key_vault_secret.admin_password had no ignore_changes, so the next apply would see the synced value as drift and write the Terraform-side password back. That silently undoes the sync the owner decided to keep (D17). Add lifecycle { ignore_changes = [value] }. Trade-off, noted in the comment: changing var.admin_password no longer rotates the vault copy. Also drop the second, identical "Test Azure compute modules" CI step that the main merge brought in (#1114 added the same step), so Azure CI runs those module tests once. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 11 ----------- terraform/modules/secrets/azure/main.tf | 9 +++++++++ 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index caa0852f6..344deefc8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1017,17 +1017,6 @@ jobs: terraform -chdir="$d" test done - - 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 - # With CloudFront in front of a Fargate ALB, the ALB must admit only # CloudFront's origin-facing prefix list (#412). - name: Test AWS ALB ingress guard diff --git a/terraform/modules/secrets/azure/main.tf b/terraform/modules/secrets/azure/main.tf index 5f256ff42..b98a1ac4c 100644 --- a/terraform/modules/secrets/azure/main.tf +++ b/terraform/modules/secrets/azure/main.tf @@ -107,6 +107,15 @@ resource "azurerm_key_vault_secret" "admin_password" { environment = var.environment }) + # The app's admin-password sync (#199) writes this secret at runtime. Without + # ignore_changes, the next apply would compare against the synced value and + # silently write the Terraform-side password back, undoing the sync. Changing + # var.admin_password therefore no longer rotates the vault copy; rotate it + # through the app (or with `az keyvault secret set`) instead. + lifecycle { + ignore_changes = [value] + } + depends_on = [azurerm_role_assignment.current_user_secrets_officer] }