fix: use --role-arn service role for companion stack create/update - #9267
fix: use --role-arn service role for companion stack create/update#9267Sanjays2402 wants to merge 6 commits into
Conversation
The companion stack created by sam deploy --resolve-image-repos ignored the --role-arn service role: create_stack/update_stack were called without RoleARN, so ECR repo creation fell back to the caller's permissions and could fail with ecr:CreateRepository AccessDenied. Thread role_arn through CompanionStackManager and sync_ecr_stack, and pass RoleARN to CloudFormation create_stack/update_stack when set. Fixes aws#5051
github-actions
Bot
added
area/deploy
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
| if resolve_image_repos: | ||
| image_repositories = sync_ecr_stack( | ||
| template_file, stack_name, region, s3_bucket, s3_prefix, image_repositories | ||
| template_file, stack_name, region, s3_bucket, s3_prefix, image_repositories, role_arn=role_arn |
There was a problem hiding this comment.
[BUG] The fix only covers the non-guided branch, but --role-arn applies to guided deploys too — it is passed to DeployContext unconditionally at command.py:416, and it is also persisted into samconfig.toml. That means sam deploy --guided --role-arn <role> --resolve-image-repos still reproduces the exact failure from #5051: the main stack deploys under the service role, while the companion stack is created with the caller's credentials and fails on ecr:CreateRepository.
Both companion-stack entry points in the guided flow are affected:
samcli/commands/deploy/guided_context.py:191—sync_ecr_stack(...)when--resolve-image-reposis setsamcli/commands/deploy/guided_context.py:362—CompanionStackManager(stack_name, region, s3_bucket, s3_prefix)inprompt_image_repository, which callsmanager.sync_repos()at line 390 and therefore reachesupdate_companion_stack()/create_stack
Threading role_arn through GuidedContext would close the gap, e.g. accept it in GuidedContext.__init__, pass role_arn=role_arn from the guided branch of do_cli, and forward it at both call sites:
# guided_context.py, in guided_prompts
image_repositories = (
sync_ecr_stack(
self.template_file,
stack_name,
region,
managed_s3_bucket,
self.s3_prefix,
self.image_repositories,
role_arn=self.role_arn,
)
if self.resolve_image_repositories
else self.prompt_image_repository(...)
)If leaving guided out is deliberate, it is worth stating why in the PR description, since the reported symptom is identical in that path.
There was a problem hiding this comment.
Good catch — you're right, the guided path had the same bug. GuidedContext now accepts role_arn, do_cli passes --role-arn through on the guided branch, and it's forwarded at both companion-stack call sites (sync_ecr_stack in guided_prompts and the CompanionStackManager in prompt_image_repository). Added guided tests asserting the role reaches both. Verified: 24/24 guided-context tests and all companion-stack manager tests pass.
GuidedContext now accepts role_arn and forwards it at both guided companion-stack call sites (sync_ecr_stack in guided_prompts and CompanionStackManager in prompt_image_repository); do_cli passes --role-arn through on the guided path. Previously sam deploy --guided --role-arn --resolve-image-repos created the companion stack with caller credentials. Add guided role_arn tests.
| "Capabilities": ["CAPABILITY_AUTO_EXPAND"], | ||
| } | ||
| if self._role_arn: | ||
| stack_kwargs["RoleARN"] = self._role_arn |
There was a problem hiding this comment.
[BUG] The service role is now wired into create_stack/update_stack, but sync_repos() also reaches ECR directly, and that path still runs under the caller's credentials — so the failure mode from #5051 remains reachable.
def sync_repos(self) -> None:
has_repo = bool(self.get_repository_mapping())
if self.does_companion_stack_exist():
self.delete_unreferenced_repos() # <-- direct ECR call, caller creds
...def delete_unreferenced_repos(self) -> None:
repos = self.get_unreferenced_repos()
for repo in repos:
try:
self._ecr_client.delete_repository(repositoryName=repo.physical_id, force=True)
except self._ecr_client.exceptions.RepositoryNotFoundException:
LOG.debug(...)self._ecr_client is built from ambient credentials in __init__; a CloudFormation RoleARN only affects calls CloudFormation makes on your behalf, not SDK calls SAM CLI makes itself. For the user profile described in the issue (caller has no ECR permissions, the --role-arn role does), removing an image function from the template and re-deploying with --resolve-image-repos --role-arn will raise an AccessDenied ClientError on ecr:DeleteRepository. That error is not caught here (only RepositoryNotFoundException is), so the deploy aborts with a raw botocore error before the companion stack update runs.
Two options, either is fine:
- Assume
role_arnvia STS and buildself._ecr_clientfrom those credentials whenrole_arnis set, so all companion-stack side effects use one identity. - If keeping caller credentials for ECR is intentional, catch
ClientErrorwithAccessDeniedExceptionhere and raise an actionable message explaining thatecr:DeleteRepositoryis required on the calling identity even when--role-arnis supplied.
Worth covering with a unit test for the unreferenced-repo path, since the current tests exercise only create/update.
There was a problem hiding this comment.
Good catch — when role_arn is set, the manager now assumes it via STS and builds self._ecr_client from the assumed-role credentials, so delete_unreferenced_repos and all other direct ECR calls run under one identity. An assume-role failure raises an actionable AWSServiceClientError. Added tests covering the assumed-role delete path and the failure case.
| self._account_id = boto3.client("sts").get_caller_identity().get("Account") | ||
| sts_client = boto3.client("sts") | ||
| self._account_id = sts_client.get_caller_identity().get("Account") | ||
| if role_arn: |
There was a problem hiding this comment.
[BUG] The constructor now calls sts:AssumeRole on the --role-arn value unconditionally whenever it is set. That is very likely to break the exact command this PR is trying to fix.
--role-arn is a CloudFormation service role: the only principal that needs to be in its trust policy is cloudformation.amazonaws.com. The deploying user/CI principal generally has no sts:AssumeRole permission on it, and the role's trust policy generally does not trust that principal. In that (common) configuration, _ecr_client_for_role raises AWSServiceClientError and sam deploy --role-arn <role> --resolve-image-repos fails hard at manager construction — before any stack or ECR work happens. Users who previously succeeded (main stack under the service role, ECR calls under their own credentials, which they were permitted to make) would now be blocked. This also contradicts the stated side effects in the PR description, which only mentions passing RoleARN to create_stack/update_stack.
The assume also happens eagerly even when no direct ECR call will ever be made (the only use of _ecr_client is delete_unreferenced_repos, which is a no-op when there are no stale repos), so the failure is triggered on paths that need no extra permissions at all.
Suggested direction: keep the RoleARN wiring on the CloudFormation calls (that part correctly fixes #5051), and make the assume-role behavior non-fatal and lazy — only attempt it when a direct ECR call is actually about to be made, and fall back to the caller's credentials with a LOG.debug/warning if the role cannot be assumed:
def getecr_client(self):
"""Return the assumed-role ECR client when available, else the caller's client."""
if not self._role_arn:
return self._ecr_client
if self._role_ecr_client is None:
try:
self._role_ecr_client = self._ecr_client_for_role(boto3.client("sts"), self._role_arn)
except AWSServiceClientError as ex:
LOG.debug("Unable to assume %s for ECR operations, using caller credentials: %s", self._role_arn, ex)
self._role_ecr_client = self._ecr_client
return self._role_ecr_clientIf assuming the service role is genuinely intended as a hard requirement, that is a user-visible behavior change that needs to be called out in the PR description and documentation, not just an internal detail.
There was a problem hiding this comment.
Good call — that was a real regression I introduced. The eager assume is gone from the constructor; the role is now assumed lazily only when a direct ECR call is actually about to happen, and an assume failure just falls back to caller credentials with a debug log instead of aborting the deploy. When no --role-arn is set, nothing changed. CloudFormation create/update/delete keep their RoleARN wiring. Reworked the tests: assume is attempted lazily and exactly once, the failure case now asserts construction succeeds and ECR deletes run under caller creds, plus a case that no STS call happens when there are no stale repos.
| "Capabilities": ["CAPABILITY_AUTO_EXPAND"], | ||
| } | ||
| if self._role_arn: | ||
| stack_kwargs["RoleARN"] = self._role_arn |
There was a problem hiding this comment.
[BUG] The service role is applied to create_stack/update_stack but not to _delete_companion_stack, which is reachable from sync_repos() whenever the last image function is removed:
if self.does_companion_stack_exist():
self.delete_unreferenced_repos()
if has_repo:
self.update_companion_stack()
else:
self._delete_companion_stack() # no RoleARNDeleteStack accepts RoleARN. When omitted, CloudFormation reuses the role previously associated with the stack — but any companion stack created before this fix (or created without --role-arn) has no associated role, so CloudFormation falls back to a session derived from the caller's credentials and the stack delete can fail with the same AccessDenied class of error as #5051, this time on repository deletion. Passing the role keeps the whole companion-stack lifecycle on one identity:
def deletecompanion_stack(self) -> None:
stack_name = self._companion_stack.stack_name
waiter = self._cfn_client.get_waiter("stack_delete_complete")
delete_kwargs: Dict[str, Any] = {"StackName": stack_name}
if self._role_arn:
delete_kwargs["RoleARN"] = self._role_arn
self._cfn_client.delete_stack(**delete_kwargs)
waiter.wait(StackName=stack_name, WaiterConfig=self._delete_stack_waiter_config)There was a problem hiding this comment.
Fixed — _delete_companion_stack now passes RoleARN on delete_stack when the role is set, so the whole companion-stack lifecycle (create/update/delete) runs under one identity. Also covered by a new test asserting delete_stack gets the RoleARN kwarg; the no-role case still deletes with just StackName.
| """ | ||
| try: | ||
| assumed_role = sts_client.assume_role(RoleArn=role_arn, RoleSessionName="sam-cli-companion-stack") | ||
| except ClientError as ex: |
There was a problem hiding this comment.
[ERROR_HANDLING] The _get_ecr_client docstring states that "any failure falls back to the caller's credentials rather than aborting the deploy", but only ClientError is translated into AWSServiceClientError here — and _get_ecr_client catches only AWSServiceClientError. Any other assume_role failure escapes the fallback and aborts the deploy from a path that is explicitly designed never to abort.
This is reachable without an unusual setup: --role-arn is an unvalidated free-form string (role_arn_click_option in samcli/commands/_utils/options.py declares no type or callback), and botocore client-side validates RoleArn before the call. A typo'd or truncated value raises ParamValidationError, not ClientError. Because sync_repos() runs delete_unreferenced_repos() before update_companion_stack(), the user gets a raw botocore traceback instead of the clean CloudFormation validation error they would have gotten previously. Endpoint/connection errors from STS have the same effect.
Widening the caught type keeps the documented behavior intact:
from botocore.exceptions import BotoCoreError, ClientError
try:
assumed_role = sts_client.assume_role(RoleArn=role_arn, RoleSessionName="sam-cli-companion-stack")
except (ClientError, BotoCoreError) as ex:
raise AWSServiceClientError(
f"Error assuming the provided role {role_arn} for companion stack ECR operations: {ex}"
) from exBotoCoreError is the base for ParamValidationError, NoCredentialsError, and the endpoint/connection errors, so the fallback then covers every realistic STS failure.
There was a problem hiding this comment.
Good point — a malformed role ARN fails with ParamValidationError before any ClientError is raised, so the fallback would never trigger there. _ecr_client_for_role now catches BotoCoreError as well, and I added a test with a malformed ARN covering the fallback.
| If repo does not exist, this will simply skip it. | ||
| """ | ||
| repos = self.get_unreferenced_repos() | ||
| ecr_client = self._get_ecr_client() |
There was a problem hiding this comment.
[GENERAL] _get_ecr_client() is called before checking whether there is anything to delete, so the sts:AssumeRole attempt happens even when repos is empty. That contradicts the laziness the method's own docstring promises ("only when a direct ECR call is actually about to be made").
The empty case is the steady state, not an edge case: sync_repos() calls delete_unreferenced_repos() on every deploy where the companion stack already exists, and there are usually no stale repos. Combined with the fact that a CloudFormation service role's trust policy generally does not include the deploying principal — as the docstring itself notes — this means every routine sam deploy --role-arn ... --resolve-image-repos emits a failed AssumeRole to CloudTrail for no reason, which is the kind of event security tooling alerts on.
Guarding on the repo list resolves it:
repos = self.get_unreferenced_repos()
if not repos:
return
ecr_client = self._get_ecr_client()
for repo in repos:
...Note on prior review threads: the four earlier findings (guided-path plumbing, RoleARN on _delete_companion_stack, the unconditional constructor-time assume_role, and the direct-ECR identity mismatch) are all resolved in this diff and were not re-raised.
There was a problem hiding this comment.
Fixed — delete_unreferenced_repos now returns early when there are no candidate repos, so AssumeRole is never called in that case. Added a test asserting assume_role isn't invoked when nothing is stale.
…and skip AssumeRole when there are no stale repos
| raise AWSServiceClientError( | ||
| f"Error assuming the provided role {role_arn} for companion stack ECR operations: {ex}" | ||
| ) from ex | ||
| credentials = assumed_role["Credentials"] |
There was a problem hiding this comment.
[ERROR_HANDLING] The try block only wraps the assume_role call, so the "always falls back" contract documented in _get_ecr_client is still incomplete — this is the unresolved remainder of the earlier error-handling finding. Broadening the caught types to (ClientError, BotoCoreError) fixed the exception-breadth half, but the two statements that consume the response are outside the protected region:
except (ClientError, BotoCoreError) as ex:
raise AWSServiceClientError(...) from ex
credentials = assumed_role["Credentials"] # outside try
return boto3.client( # outside try
"ecr",
config=self._boto_config,
aws_access_key_id=credentials["AccessKeyId"],
...
)_get_ecr_client catches only AWSServiceClientError, so anything raised by these two statements (a KeyError on a response shape that lacks Credentials/AccessKeyId, or a botocore error from client construction) propagates out of delete_unreferenced_repos → sync_repos and aborts the deploy from a code path whose whole purpose is to never abort. That is the same class of failure the fallback was added to prevent, and the fix is to move both statements inside the protected block:
try:
assumed_role = sts_client.assume_role(RoleArn=role_arn, RoleSessionName="sam-cli-companion-stack")
credentials = assumed_role["Credentials"]
return boto3.client(
"ecr",
config=self._boto_config,
aws_access_key_id=credentials["AccessKeyId"],
aws_secret_access_[REDACTED]
aws_session_[REDACTED]
)
except (ClientError, BotoCoreError, KeyError) as ex:
raise AWSServiceClientError(
f"Error assuming the provided role {role_arn} for companion stack ECR operations: {ex}"
) from exThis also makes the Raises section of the docstring accurate for the whole method rather than just its first line.
There was a problem hiding this comment.
Fixed — the try block now also covers credential extraction and ECR client construction, so any failure there (malformed AssumeRole response, client construction errors) raises AWSServiceClientError and falls back to the caller-credential client.
| self._ecr_client.delete_repository(repositoryName=repo.physical_id, force=True) | ||
| except self._ecr_client.exceptions.RepositoryNotFoundException: | ||
| ecr_client.delete_repository(repositoryName=repo.physical_id, force=True) | ||
| except ecr_client.exceptions.RepositoryNotFoundException: |
There was a problem hiding this comment.
[BUG] Routing delete_repository through the assumed service role changes the identity of this call for users whose role is assumable, but the except clause still only handles RepositoryNotFoundException. A CloudFormation service role is commonly scoped to what CFN needs to create/update the stack; if it can be assumed but lacks ecr:DeleteRepository, the resulting AccessDenied ClientError is unhandled and aborts sam deploy before packaging — a case that worked before this PR, because the delete previously always ran under the caller's (typically broader) credentials.
Note this is reachable on ordinary deploys, not just teardown: sync_repos() calls delete_unreferenced_repos() whenever the companion stack exists, so removing or renaming a single image function is enough.
Since the design already treats the assumed role as best-effort, the permission failure deserves the same treatment — fall back to the caller's client rather than failing the deploy:
for repo in repos:
try:
ecr_client.delete_repository(repositoryName=repo.physical_id, force=True)
except ecr_client.exceptions.RepositoryNotFoundException:
LOG.debug("Image repo [%s] not found in companion stack. Skipping deletion.", repo.physical_id)
except ClientError as ex:
if ecr_client is self._ecr_client or ex.response.get("Error", {}).get("Code") != "AccessDeniedException":
raise
LOG.debug(
"Assumed role not authorized to delete image repo [%s]; retrying with caller credentials.",
repo.physical_id,
)
self._ecr_client.delete_repository(repositoryName=repo.physical_id, force=True)Re-raising for any other error code keeps genuine failures visible instead of silently swallowing them.
There was a problem hiding this comment.
Fixed — if the assumed-role delete hits AccessDeniedException, we now retry the delete with the caller-credential client instead of aborting the deploy. Other errors still propagate.
| "Assumed role not authorized to delete image repo [%s]; retrying with caller credentials.", | ||
| repo.physical_id, | ||
| ) | ||
| self._ecr_client.delete_repository(repositoryName=repo.physical_id, force=True) |
There was a problem hiding this comment.
[ERROR_HANDLING] The caller-credentials retry is not covered by the RepositoryNotFoundException handler that guards the primary call, so this method's documented contract — "If repo does not exist, this will simply skip it" — no longer holds on the fallback path.
ECR authorizes a request before checking whether the resource exists, so a service role that is assumable but lacks ecr:DeleteRepository returns AccessDeniedException even for a repository that was already deleted out of band (manually, or by a previous partially-failed deploy). The fallback then calls self._ecr_client.delete_repository, which raises RepositoryNotFoundException with nothing catching it — aborting the deploy from the exact code path that was added to keep the deploy alive.
Secondary point: after the fallback fires for the first repo, the loop keeps using the assumed-role client for the remaining repos, so every subsequent repo pays another AccessDenied round trip before falling back again.
Both are fixed by reusing the existing not-found handling and demoting the client once:
except ClientError as ex:
error_code = ex.response.get("Error", {}).get("Code")
if ecr_client is self._ecr_client or error_code != "AccessDeniedException":
raise
LOG.debug(
"Assumed role not authorized to delete image repo [%s]; retrying with caller credentials.",
repo.physical_id,
)
# Stop using the assumed-role client for the remaining repos.
self._role_ecr_client = ecr_client = self._ecr_client
try:
self._ecr_client.delete_repository(repositoryName=repo.physical_id, force=True)
except self._ecr_client.exceptions.RepositoryNotFoundException:
LOG.debug("Image repo [%s] not found in companion stack. Skipping deletion.", repo.physical_id)Rebinding ecr_client is safe here because the except ecr_client.exceptions.… clause is resolved at exception time, so later iterations correctly reference the caller's client.
Notes on prior findings: the guided-path wiring (command.py, guided_context.py), the lazy assume with caller-credential fallback, the RoleARN on delete_stack, the broadened (ClientError, BotoCoreError, KeyError) catch with response consumption inside the try, and the early return that avoids sts:AssumeRole when no repos are stale are all present in this head and are no longer flagged. I verified no other companion-stack entry point needs the role: sam package --resolve-image-repos has no --role-arn option, sam sync sets support_resolve_image_repos=False, and the PackageContext created by sam deploy leaves resolve_image_repos at its False default.
1 participant
Which issue(s) does this change fix?
#5051
Why is this change necessary?
When deploying an image-based function with
sam deploy --role-arn <role> --resolve-image-repos, the companion stack that creates the ECR repositories is created/updated without the service role. CloudFormation then falls back to the caller's credentials, which can fail withecr:CreateRepositoryAccessDenied even though the role passed via--role-arnhas the required permissions.How does it address the issue?
CompanionStackManageraccepts an optionalrole_arnand passesRoleARNtocreate_stack/update_stackwhen set.sync_ecr_stackforwards the new optional parameter.sam deploy(non-guided) passes its--role-arnvalue through tosync_ecr_stack.What side effects does this change have?
None when
--role-arnis not provided (theRoleARNkwarg is omitted, keeping existing behavior). When provided, the companion stack is created/updated under the same service role as the main stack.Mandatory Checklist
make prpasses — ran the affected unit tests, plus ruff, black, and mypy on the changed files (all clean); did not run the full suitemake update-reproducible-reqsif dependencies were changed — dependencies unchangedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.