fix(agentcore): custom execution role policy for runtime lacks proper permissions

[go-to-k]

Issue # (if applicable)

Closes #35852 .

Reason for this change

ECR permissions are attached even when the role is a custom role or an imported role. (https://github.com/aws/aws-cdk/blob/v2.221.0/packages/%40aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime-artifact.ts#L65)

However, the other required permissions are only granted to a policy for an auto-generated role. (https://github.com/aws/aws-cdk/blob/v2.221.0/packages/%40aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime.ts#L252-L259)

In constructs of other common modules, permissions are attached even when a custom role is passed.

• https://github.com/aws/aws-cdk/blob/v2.221.0/packages/aws-cdk-lib/aws-codepipeline/lib/pipeline.ts#L693
• https://github.com/aws/aws-cdk/blob/v2.221.0/packages/aws-cdk-lib/aws-lambda/lib/function.ts#L1468
• https://github.com/aws/aws-cdk/blob/v2.221.0/packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts#L1161

So this PR adds the permissions to the custom role.

FYI: If you avoid to add the permissions to the custom role, you can use withoutPolicyUpdates() method for Role.

Description of changes

Add the permissions to the custom role.

Describe any new or updated permissions being added

Description of how you validated changes

Both unit tests and an integ test.

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[dineshSajwan]

Thank you for the PR @go-to-k . Looks all good to me.

[kumvprat]

@go-to-k
I wanted to clarify on this statement

ECR permissions are attached even when the role is a custom role or an imported role.

In case of an imported/referenced role(i.e the one not created by the cdk app where runtime is being created), these operation will not have any effect as the referenced copy of a role is merely a read-only version of it.

My suggestion would be to use the result of the addtoPrinicpalPolicy method calls : AddToPrincipalPolicyResult and check if statementAdded is true.

[go-to-k]

@kumvprat

In case of an imported/referenced role(i.e the one not created by the cdk app where runtime is being created), these operation will not have any effect as the referenced copy of a role is merely a read-only version of it.

No, that's not the case for IAM roles. Even for imported IAM roles, a new policy resource (AWS::IAM::Policy) is created using that role's name. (The policy creation cannot be prevented unless the mutate option in the fromRoleArn method is set to false.)

const imported = iam.Role.fromRoleArn(this, 'ImportedRole', 'arn:aws:iam::123456789012:role/ExistingExecutionRole');

imported.addToPrincipalPolicy(new iam.PolicyStatement({
  actions: ['s3:GetObject'],
  resources: ['arn:aws:s3:::my-bucket/my-object'],
}));

 "Resources": {
  "ImportedRolePolicyawscdkbedrockagentcoreruntimewithimportedroleImportedRole261507D78EB91FCC": {
   "Type": "AWS::IAM::Policy",
   "Properties": {
    "PolicyDocument": {
     "Statement": [
      {
       "Action": "s3:GetObject",
       "Effect": "Allow",
       "Resource": "arn:aws:s3:::my-bucket/my-object"
      }
     ],
     "Version": "2012-10-17"
    },
    "PolicyName": "PolicyawscdkbedrockagentcoreruntimewithimportedroleImportedRole261507D7",
    "Roles": [
     "ExistingExecutionRole"
    ]
   }
  }
 },

[go-to-k]

I'll check the rest of the comments later :)

[kumvprat]

@go-to-k Thanks for the explanation : #35849 (comment)

I see that there are a couple of configurable params while importing a role, (options)

In that case the suggestion here can help us inform the users earlier(during synth/compile) if the policy additions were not successful. Since the user can reference an non-mutable role, in which case we should inform the user that the execution policy changes did not take affect. Might be a moot point since user advanced enough to disable this should be advanced enough to know the behaviour

fromRoleArn, fromRoleName and fromRoleLookup : All referenced behaviour of roles point to the default being mutable

My suggestion would be to use the result of the addtoPrinicpalPolicy method calls : AddToPrincipalPolicyResult and check if statementAdded is true.

I am not sure of all the patterns to reference roles and adding this check would be our way to ensure the behaviour is visible to the user (atleast via logs)

Edited the comment

[go-to-k]

@kumvprat

to: #35849

In that case the suggestion here can help us inform the users earlier(during synth/compile) if the policy additions were not successful. Since the user can reference an non-mutable role, in which case we should inform the user that the execution policy changes did not take affect. Might be a moot point since user advanced enough to disable this should be advanced enough to know the behaviour

I am not sure of all the patterns to reference roles and adding this check would be our way to ensure the behaviour is visible to the user (atleast via logs)

The user has explicitly specified mutate: false because they don't want to attach policies to an imported role. Given that this is their intent, is there really a need to deliberately notify the user that "the policy was not attached"?

While it's useful to provide warnings when something doesn't happen as expected, I don't think there's a need to output Annotations when things are working exactly as intended. If such a notification is necessary, shouldn't it be provided by the Role construct itself rather than in the Agentcore Runtime? (It also doesn't seem like this kind of thing is commonly done in constructs of other modules)

My suggestion would be to use the result of the addtoPrinicpalPolicy method calls : AddToPrincipalPolicyResult and check if statementAdded is true.

Also, even if the role is immutable and the addGrantsToResources is false (as default), statementAdded for the ImmutableRole will be returned as true.

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-iam/lib/private/immutable-role.ts#L88-L89

[kumvprat]

@go-to-k

For comment : #35849 (comment)

While it's useful to provide warnings when something doesn't happen as expected, I don't think there's a need to output Annotations when things are working exactly as intended. If such a notification is necessary, shouldn't it be provided by the Role construct itself rather than in the Agentcore Runtime? (It also doesn't seem like this kind of thing is commonly done in constructs of other modules)

I agree. My point was to output a log line(not an Annotation based warning) for user to inform them regardless of whether the permission are added to the user's custom role or not.

When a user deploys the AgentCoreRuntime resources via CDK they will see the role changes during cdk deploy and these log lines can help them identify why a specific permission was added to their custom role.

Example how console.log is used in the library

Also, even if the role is immutable and the addGrantsToResources is false (as default), statementAdded for the ImmutableRole will be returned as true.

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-iam/lib/private/immutable-role.ts#L88-L89

In a big enough code-base a user doing incremental changes might not have context around how a specific role is generated(created within the app or reference via the fromRoleArn and similar methods) and the log lines are supposed to provide enough visibility to the user

[go-to-k]

@kumvprat

Example how console.log is used in the library

Isn't that code used in the provider-framework's custom resource Lambda, rather than in the CDK App (constructs) itself?

not an Annotation based warning

My understanding is that all constructs run within the CDK App, and the CDK App runs as a subprocess of the CDK CLI. Therefore, I think, console.log statements are not handled by the CDK CLI and won't be output to the user's prompt. This means that if we want to output something to the user running the CDK, we need to use the Annotations.addInfo(v2)|Warning(v2)|Error() methods (in my understanding).

Also, even if the role is immutable and the addGrantsToResources is false (as default), statementAdded for the ImmutableRole will be returned as true.

As seen here, statementAdded returns true just like it would for a role actually created in the stack, even if no permissions are actually added to the imported immutable role. Because of this, isn't it difficult to make a judgment based solely on this return value?

Also, this just relates to IAM role permissions. If permissions are added to an IAM role, a list of the policies being granted will be output to the user's terminal based on broadening, which is the default value for the cdk deploy --require-approval option. Isn't this sufficient?

ref: Approve security-related changes in https://docs.aws.amazon.com/cdk/v2/guide/cli.html

[kumvprat]

@go-to-k
#35849 (comment)
Missed this in the review. CLI would be responsible to outputting log statements anyways. Thanks for pointing this out

My understanding is that all constructs run within the CDK App, and the CDK App runs as a subprocess of the CDK CLI. Therefore, I think, console.log statements are not handled by the CDK CLI and won't be output to the user's prompt. This means that if we want to output something to the user running the CDK, we need to use the Annotations.addInfo(v2)|Warning(v2)|Error() methods (in my understanding).

For below, yes my idea was to give users visibility on why a certain role/permission addition they see during cdk deploy exists. But the point above makes it hard to print logs to console anyways.

Also, this just relates to IAM role permissions. If permissions are added to an IAM role, a list of the policies being granted will be output to the user's terminal based on broadening, which is the default value for the cdk deploy --require-approval option. Isn't this sufficient?

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[mergify]

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

rebase conflict between base and head.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

[go-to-k]

@kumvprat

Thanks for your approval and the all comments！

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[kumvprat]

@Mergifyio requeue

[mergify]

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

[mergify]

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

rebase conflict between base and head.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[mergify]

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

rebase conflict between base and head.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.