Repository navigation
Conversation
…ret, 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 <vault>/secrets/<admin secret> 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 LeanerCloud#199
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
Both Azure compute paths set
ADMIN_PASSWORD_SECRETandADMIN_EMAIL, which arms the admin-password sync (buildAdminPasswordSyncCallback,internal/server/app.go). When the admin changes their password, the sync callsazsecrets.SetSecret. The only Key Vault grant either runtime identity holds is the vault-wide Key Vault Secrets User, which is read-only, so the write gets a 403. The callback logged that at WARN and carried on. The password change had already been committed to the database, so the vault copy silently drifted.On current main (
a14dc8f2) both halves still reproduce:workload_kv_secrets_useris Secrets User (modules/compute/azure/aks/main.tf). fix(iac/azure): wire AKS workload identity so pods can assume the workload UAI #996 has since wired workload identity (fix(iac/azure): AKS workload identity is not wired up, so the UAI is unassumable by any pod #198), so once AKS is deployed this write path is live.compute_key_vault_secrets_user(Secrets User) inenvironments/azure/compute.tf. The secrets module'swriteable_secrets_roleOfficer grant iscount = 0everywhere, becausecontainer_app_identity_principal_idis never passed. All three Azure environments deploy container-apps, so this half is what is actually deployed.Fix
I took the issue's option 2 in spirit: an explicit write grant scoped as narrowly as Azure allows.
modules/compute/azure/container-apps(admin_password_secret_writer) andmodules/compute/azure/aks(workload_admin_password_secret_writer) each add anazurerm_role_assignmentof Key Vault Secrets Officer:<key_vault_id>/secrets/<admin_password_secret_name>, which is that one secret, not the vault. Key Vault RBAC supports secret-level role assignments. No built-in role is set-only, so delete/backup/restore on this one secret come with it, and nothing on any other secret. The vault-wide grant stays read-only.AZURE_CLIENT_ID) or the AKS workload UAI (federated to the pod service account).admin_password_secret_name != "" && admin_email != "", the same condition that arms the Go callback. The grant exists only when something will use it.var.key_vault_idplus the secret name rather than the secret'sresource_versionless_id.scopeis ForceNew, and an apply-time-only value would replace the grant (the #1802 pattern).admin_password_secret_namegains a validation that it must be a bare Key Vault secret name (^[0-9A-Za-z-]{1,127}$), so a path or ID cannot move the scope off the single secret.Microsoft.Authorization/roleAssignments/write(ci-cd-permissions/locals_data.tf), the same permission it uses for the existing Secrets User, Crypto User and AcrPull assignments. The bootstrap does not need re-applying, and I added no fallback.internal/server/app.go):PutSecretnow logs at ERROR, naming the secret and stating it no longer matches the database password, with a hint to check the runtime identity's write access. Before, this was a WARN.Deploy pause
Azure deploys have been paused since 2026-10-20 (deploy-azure workflows disabled). The Terraform half takes effect only on the next Azure deploy after the pause lifts. The PR is otherwise merge-ready, and merging it deploys nothing. The Go logging change ships with the next image build.
Closes #199
Verification
All of the following is unit, mock and fixture evidence. Nothing was planned or applied against a real subscription, and no real credentials were used.
Go (fail-before/pass-after): new tests in
internal/server/app_coverage_test.go:TestBuildAdminPasswordSyncCallback_PutSecretFailureLogsError(ERROR names the secret and the cause, no WARN, no false success line, password not logged)TestBuildAdminPasswordSyncCallback_UserLookupFailureLogsErrorTestBuildAdminPasswordSyncCallback_QuietPathsLogNoError(success, non-admin and missing user log no ERROR)Terraform (mocked
terraform test, fail-before/pass-after): newtests/admin_password_writer.tftest.hclin both modules.<vault>/secrets/admin-passwordwith Secrets Officer. Container-apps also asserts the principal is the app's UAI and that the secret matches the container'sADMIN_PASSWORD_SECRET. AKS asserts the same for the pod spec, and that the vault-wide grant is still Secrets User at vault scope.Mutation probes, each run against both modules, were all killed: vault-wide scope, dropping the
admin_emailhalf of the count gate, removing the name validation, and Secrets User instead of Officer.Other checks:
terraform fmt -check -recursive terraform/passes.terraform validatepasses in both modules and interraform/environments/azureafterinit -backend=false.bash scripts/check-azure-kv-access-policy.shpasses (207 files), andscripts/test-azure-kv-access-policy.shgives 13/13.fix/issue-204branch, run on this change merged onto that branch, passes, and its harness gives 26/26.bash scripts/check-azure-role-parity.shpasses.actionlint .github/workflows/ci.ymlandpre-commit run zizmor --files .github/workflows/ci.ymlpass.git merge-treeagainst open chore(iac/aws): add multi_az support and run prod RDS Multi-AZ on a non-burstable class #1086, fix(iac/aws): route private egress through a persistent fck-nat ENI #1090 and fix(iac): bound every Terraform provider constraint below the next major #1203 is clean. With fix(iac): bound every Terraform provider constraint below the next major #1203's provider-constraint bump (~> 3.117) applied on top, both module tests still init with-lockfile=readonlyand pass.CI wiring: a new "Test Azure compute modules" step in the
terraform-validatejob (azure leg) runs init/fmt/test for everyterraform/modules/compute/azure/*/tests. Both modules'.terraform.lock.hclgainlinux_amd64anddarwin_amd64hashes, following the #1143 precedent, so-lockfile=readonlyinit works on the Linux runner. They previously carried a single platform'sh1:.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@b56afb2b= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-324andfix/issue-204both edit.github/workflows/ci.ymlnext to theazure-kv-access-policyjob. The tree kept #324's new "Run Function URL reader self-tests" step and #204's updated job comment; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran oned0c7174, which differs fromb56afb2bonly by one.gitallowedline on fix/issue-324:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....--build-tags integration.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout):internal/api,internal/purchase,internal/schedulerandinternal/serverall pass..gitallowedline. Re-run atb56afb2b: git-secrets, using the repo'sscripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Gaps and things to eyeball
kubelet_identityblock (Optional+Computed) plans as an empty list, soaks_acrcannot be evaluated. Anyapplyrun also fails at teardown, so the AKS runs are targeted plans, whereprincipal_idis unknown. The reference isazurerm_user_assigned_identity.workload.principal_id, the same as the read grant next to it. The container-apps test applies and asserts its principal.az role definition listresponse. As the issue notes, I could not query Azure. "Key Vault Secrets Officer" (Microsoft.KeyVault/vaults/secrets/*) and secret-level scopes are documented by Microsoft.secrets/setSecret/action. That needs a bootstrap-created role definition (roleDefinitions/write is bootstrap-only), and the runtime lookup would fail every Azure deploy until a human re-applies the bootstrap. Scoping the built-in role to one secret seemed the better trade. Happy to switch if the owner prefers the custom role.Out of scope (noted, not changed)
modules/secrets/azurestill carries the unusedcontainer_app_secrets_user/writeable_secrets_rolegrant (count = 0 in every environment). Its comment, andknown_issues/18_tf_env_azure.md, describe it as the runtime's write path. Removing or rewording it is a separate cleanup.ADMIN_PASSWORD_SECRET/ADMIN_EMAIL) was not taken, because the startup path also readsADMIN_PASSWORD_SECRET.A maintainer, please mirror the issue's triage labels on this PR:
type/bug,priority/p3,severity/low,urgency/eventually,impact/internal,effort/s,triaged.🤖 Generated with Claude Code