Skip to content

fix(ci): rollback verifies the image against the assumed role's registry, not AWS_ACCOUNT_ID's #142

Open

Description

@cristim

Found during the independent review of PR LeanerCloud/cloud-commitments-cli#1641. Filed rather than fixed there to keep that p0 to one concern — but note the fix is one line, so it is cheap to pick up.

What

.github/workflows/rollback.yml derives the image to deploy and the image it verifies from two different sources, which are only equal by convention:

  • :111 builds the URI Terraform deploys from the AWS_ACCOUNT_ID repository variable:

    IMAGE_URI="${AWS_ACCOUNT_ID}.dkr.ecr.${AWS_REGION}.amazonaws.com/${ECR_REPOSITORY}:${IMAGE_TAG}"
  • :210-214 verifies existence with no --registry-id, so the call resolves against the account of the assumed role (vars.AWS_ROLE_TO_ASSUME, :193 / :281):

    aws ecr describe-images \
      --repository-name "$ECR_REPOSITORY" \
      --image-ids "imageTag=$IMAGE_TAG" \
      --region "$AWS_REGION"

If those two admin-set values ever point at different accounts — a cross-account setup, a stale variable after an account migration, a copy-paste into the wrong environment — the workflow cheerfully reports "✅ Image exists in ECR" having checked a registry that is not the one Terraform then pulls from. The rollback proceeds and fails later at deploy, or worse, succeeds against an unintended image.

This is a misconfiguration hazard rather than an attack path: both values are repository variables only an admin can set. But it is a check that silently asserts the wrong thing, which is the failure mode the "no silent fallbacks" rule exists to prevent — a verification step that can pass while the thing it claims to verify is false is worse than no verification.

Shape is pre-existing (the deleted verify-image job had the same divergence); PR LeanerCloud/cloud-commitments-cli#1641 inlined it into the gated rollback jobs without changing it.

Fix direction

Either:

(a) Pin the lookup to the same account the URI was built from — one line, preferred:

aws ecr describe-images \
  --registry-id "$AWS_ACCOUNT_ID" \
  --repository-name "$ECR_REPOSITORY" \
  ...

(requires threading AWS_ACCOUNT_ID into the verify step's env:, which currently carries only IMAGE_TAG, ECR_REPOSITORY and AWS_REGION.)

(b) Assert they match, and fail loud if not — derive the authenticated account and compare, so a divergence is reported rather than papered over:

ACTUAL=$(aws sts get-caller-identity --query Account --output text)
if [ "$ACTUAL" != "$AWS_ACCOUNT_ID" ]; then
  echo "::error::AWS_ACCOUNT_ID ($AWS_ACCOUNT_ID) does not match the assumed role's account ($ACTUAL)"
  exit 1
fi

(b) is the stronger option because it also catches the case where AWS_ACCOUNT_ID is stale relative to AWS_ROLE_TO_ASSUME for the deploy, not just the verify. (a) only makes the verify self-consistent.

Applies to both rollback-aws-lambda and rollback-aws-fargate, which carry identical copies of the verify step.

Related: LeanerCloud/cloud-commitments-cli#1542 / PR LeanerCloud/cloud-commitments-cli#1641, LeanerCloud/cloud-commitments-cli#1648.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions