Skip to content

sec(iac/azure): pull and push ACR with Entra identities, disable the admin account - #2079

Open
cristim wants to merge 1 commit into
mainfrom
fix/1970-acr-admin-credentials
Open

sec(iac/azure): pull and push ACR with Entra identities, disable the admin account#2079
cristim wants to merge 1 commit into
mainfrom
fix/1970-acr-admin-credentials

Conversation

@cristim

@cristim cristim commented Sep 8, 2026

Copy link
Copy Markdown
Member

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-password secret. The deploy workflow also passed it through nonsensitive() 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.identity takes 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 moved block. 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 roleAssignmentMode returns 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:

  1. Deploy the new code. Merge for dev; dispatch the workflow for staging and production.
  2. Confirm the app is on the identity and the newest revision is healthy:
az containerapp show -g "$RG" -n "$APP" --query 'properties.configuration.registries'
az containerapp revision list -g "$RG" -n "$APP" \
  --query '[].{name:name,state:properties.provisioningState,healthy:properties.healthState}'
  1. Rotate in one line. Admin is enabled for seconds, and nothing uses it any more:
az acr update -n "$ACR" --admin-enabled true && \
  az acr credential renew -n "$ACR" --password-name password && \
  az acr credential renew -n "$ACR" --password-name password2 && \
  az acr update -n "$ACR" --admin-enabled false
  1. Verify az acr show -n "$ACR" --query adminUserEnabled returns 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_on edge 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

Check Result
terraform fmt -check -recursive, validate in both environment and module exit 0
tflint, all module types exit 0, no output
trivy config, high and critical exit 0
the four Azure Key Vault and role parity guard scripts exit 0; 13 and 78 assertions passed

terraform plan and apply need Azure credentials and are the operator's steps above.


🤖 Generated with claude-flow

https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC

…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
@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

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.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: ff73af5b-ad00-419b-bf95-b86b20d04799

📥 Commits

Reviewing files that changed from the base of the PR and between eac9a62 and 778978b.

📒 Files selected for processing (5)
  • terraform/environments/azure/build.tf
  • terraform/environments/azure/compute.tf
  • terraform/environments/azure/registry.tf
  • terraform/modules/compute/azure/container-apps/main.tf
  • terraform/modules/compute/azure/container-apps/variables.tf

Comment @coderabbitai help to get the list of available commands.

@cristim cristim added priority/p1 Next up; this sprint severity/high Significant harm urgency/this-sprint Within the current sprint impact/internal Team-internal only effort/s Hours type/security Security finding triaged Item has been triaged labels Sep 8, 2026
@cristim

cristim commented Sep 8, 2026

Copy link
Copy Markdown
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/internal Team-internal only priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/security Security finding urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sec(iac/azure): ACR admin credentials are used for pulls, leaving the AcrPull grant dead

1 participant