sec(iac/azure): pull and push ACR with Entra identities, disable the admin account - #2079
sec(iac/azure): pull and push ACR with Entra identities, disable the admin account#2079cristim wants to merge 1 commit into
Conversation
…admin account The Container App pulled its image with the registry's admin username and password, which put a push-capable credential in Terraform state, in the app's secrets, and in every deploy log, while the AcrPull grant to the app's managed identity governed nothing. Switch the registry block to the user-assigned identity, move the AcrPull assignment into the container-apps module (moved block, no recreate) so the app can depend on it, log the CI/local image push in with az acr login as the caller's Entra identity, and set admin_enabled = false. Closes #1970 Refs #1623 Co-Authored-By: claude-flow <ruv@ruv.net> Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC
|
Warning Review limit reached
On-demand reviews are free for the next 12 days. After that, they cost $0.25 per reviewed file. Or wait 56 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Comment |
|
Status: all 27 checks green, but CodeRabbit did not review this PR. The CodeRabbit check reports success while its comment above says "Review limit reached". No files were analysed. Recording that here so the green check is not later read as a clean review. The next included review unlocks in under an hour and can be requested with an on-demand run if wanted. Reviewed instead by an independent adversarial pass, which approved the diff and corrected the operator runbook. Its substantive correction, now reflected in the PR body: rotating the registry admin passwords before deploying leaves the running app holding an invalidated password in its stored secret, so pulls fail for the whole deploy rather than for the brief window inside a single apply. The order in the body is deploy first, then rotate. This PR is deliberately not auto-merged. It alters live infrastructure on the next apply, and the rotation is a manual step that has to follow the deploy in the right order. It needs a human to run the runbook. |
What
Closes #1970.
The Azure Container Registry had its admin account enabled, and both sides of the pipeline used it. CI authenticated with the static admin password to push images, and the Container App pulled with the same password stored as a
registry-passwordsecret. The deploy workflow also passed it throughnonsensitive()at the login step, so the password was printed in plaintext into every Azure deploy log.Both halves now use Entra identities. CI pushes through its existing OIDC login, the app pulls with the user-assigned managed identity it already has attached, the stored secret is gone, and the admin account is disabled.
Both halves were proven, not assumed
An independent reviewer read the locked provider's source rather than trusting the plan.
Pull. The new
registry { identity = ... }block references the same user-assigned identity the app already carries. That is the form the provider expects:registry.identitytakes the identity's resource id, and validation requires exactly one of an identity or a username and password pair. Registry and secret changes are folded into a single update call, so there is no window where the registry references a deleted secret.Push. The deploy role's definition carries a
Microsoft.ContainerRegistry/registries/*action with no exclusions. Azure RBAC wildcards match across segments, so it covers both control-plane actions behind AcrPush and the read that resolves the login server. The workflow performs its OIDC login in the same job before Terraform runs, so the registry login inherits those credentials. The rollback workflow has the same login before its apply and is covered too.The
movedblock. The reviewer rebuilt its shape independently against three states: existing state moves with zero changes, fresh state produces a single add, and the block does not error when the module's count is zero. The role assignment keeps the same principal, role and scope, so the moved instance has no attribute diff.Two operational ceilings, neither blocking
The OIDC token cannot silently refresh, so a run where more than roughly an hour elapses between the Azure login and the image push fails at the push. The steps in between include an up-to-15-minute resource-group deletion wait, so normal runs sit well inside the limit. The old static login had no such ceiling.
The first plan after deploy may show a cosmetic no-op diff on the registry identity attribute if the service echoes different casing than was submitted. That is a display difference, not a pull failure.
Once per registry, before deploying: confirm
az acr show -n $ACR --query roleAssignmentModereturns legacy permissions or nothing. If a registry was switched to ABAC repository permissions, the AcrPull and AcrPush roles stop granting data-plane access and both halves fail. The provider version in use does not expose this attribute, so it cannot be checked in code.Operator runbook
Deploy first, rotate second. This is the reverse of what the original plan specified, and the reason matters. Rotating the admin passwords before deploying invalidates the password the running app still holds in its stored secret, so pulls fail for the entire deploy, roughly 10 to 30 minutes, not the brief window inside a single apply. On production, which runs two replicas, any restart or scale-out in that window cannot pull. For dev there is a second problem: any other push to main between the rotation and this merge runs the old code, which refreshes the renewed password and prints it again in a fresh log.
Per environment:
az acr show -n "$ACR" --query adminUserEnabledreturns false.If a rotation is missed and admin is already disabled, renewal is refused, but the same one-liner above recovers it. Do not leave admin enabled afterwards. Terraform would disable it on the next apply, but the old password stays live until then.
On a brand-new environment, the first revision can fail its pull with a 401 before the role assignment propagates. Re-run the apply. Do not add credentials to work around it. The
depends_onedge orders creation but cannot wait for RBAC propagation, which is why the existing three environments are unaffected: their assignment has existed for months and is only being moved.Leftovers
A repository-wide search for admin and registry credential references returns only the AWS build path, which uses a different registry and is unchanged, the unused Azure registry module, which already defaults admin off, and the application's own bootstrap admin login secret, which is unrelated to the registry. No workflow, document or variables file referenced the ACR admin account.
Verification
terraform fmt -check -recursive,validatein both environment and moduleterraform planandapplyneed Azure credentials and are the operator's steps above.🤖 Generated with claude-flow
https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC