fix(iac/azure): let the runtime identity write the admin-password secret, and log sync failures as errors

.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: |

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")
 	}
 }
 

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) {

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

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]
+}

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 <key_vault_id>/secrets/<this name>, 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" {