Skip to content

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

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.

No activity

Activity on this issue will appear here.

Activity

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

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