aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.42k stars 3.81k forks source link

IAM: Role Trust Relationships cannot be extended with multiple principals #27006

Open ttais2017 opened 11 months ago

ttais2017 commented 11 months ago

Describe the bug

The Trust Relationships of a Role cannot be extended with multiple principals. As example I included this configuration (which is possible from AWS-console)

image

Expected Behavior

The Trust Relationships can contain multiple principals

Current Behavior

While Deployment I'm getting this errors:

" A PolicyStatement used in an identity-based policy cannot specify any IAM principals" and "A PolicyStatement used in an identity-based policy must specify at least one resource"

Reproduction Steps

see this code as example:

image

I tried also with this:

image

but I got this errors:

image

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

aws-cli/2.13.12 Python/3.11.4 Windows/10 exe/AMD64 prompt/off

Framework Version

No response

Node.js Version

9.5.1

OS

Windows

Language

Java

Language Version

Java 17

Other information

No response

wz2b commented 11 months ago

I have this same exact problem. I'm trying to create a function that will be called by cloudfront. According to this document you need to allow the role to be assumed by two principles. Currently, the assumedBy doesn't accept an array.

const fnRole = new iam.Role(this, "redirect-function-role-id", {
    roleName: "redirect-function-role",
    assumedBy: new iam.ServicePrincipal('edgelambda.amazonaws.com')
})

As a workaround, I thought I could just add another principal to the trust policy like this:

 fnRole.grantAssumeRole(new iam.ServicePrincipal('lambda.amazonaws.com'))

but the messed up thing is that just silently did nothing - no error, it just didn't work. The output of the two things above generated this:

  redirectfunctionroleid9F482E14:
    Type: AWS::IAM::Role
    Properties:
      AssumeRolePolicyDocument:
        Statement:
          - Action: sts:AssumeRole
            Effect: Allow
            Principal:
              Service: edgelambda.amazonaws.com
        Version: "2012-10-17"
      RoleName: redirect-function-role

I think that is related to https://github.com/aws/aws-cdk/issues/24507 - as mentioned in that ticket, this does work:

  role.assumeRolePolicy?.addStatements(new iam.PolicyStatement({
    actions: ['sts:AssumeRole'],
    principals: [new iam.ServicePrincipal('lambda.amazonaws.com')],
  }))

I kind of consider that to be a workaround in the sense that I think the .grantAssumeRole() should do the same thing - maybe the developers disagree. I think what'd be an even better approach would be to make assumedBy take either string or string[]

peterwoodworth commented 11 months ago

grantAssumeRole doesn't add to the Role's trust policy - it grants the other principal permissions in their policy statement. Does addToPrincipalPolicy not work? https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_iam.Role.html#addwbrtowbrprincipalwbrpolicystatement

ttais2017 commented 11 months ago

I tried this: image

but I get deploy errors: image

ttais2017 commented 11 months ago

I dont still understand why this method does not work. @peterwoodworth: "grantAssumeRole doesn't add to the Role's trust polic" ? image

wz2b commented 11 months ago

I dont still understand why this method does not work. @peterwoodworth

I don't, either. The name pretty strongly implies that you're granting some principal authority to assume the role.

peterwoodworth commented 11 months ago

From our README, try CompositePrincipal. This works for me

    const role = new iam.Role(this, 'Role', {
      assumedBy: new iam.CompositePrincipal(
        new iam.ServicePrincipal('lambda.amazonaws.com'),
        new iam.ServicePrincipal('apigateway.amazonaws.com'),
      ),
      managedPolicies: [
        iam.ManagedPolicy.fromAwsManagedPolicyName('AmazonSagemakerFullAccess'),
        iam.ManagedPolicy.fromAwsManagedPolicyName('AmazonS3FullAccess')
      ],
      inlinePolicies: {
        policy: new iam.PolicyDocument({
          statements: [
            new iam.PolicyStatement({
              actions: ['lakeformation:*'],
              resources: ['*'],
              effect: iam.Effect.ALLOW,
            })
          ]
        })
      }
    })

Regarding the confusion over grantAssumeRole, it states:

Grant permissions to the given principal to assume this role.

I'm not really sure how this can be made more clear in the docstring. It's stating that it's granting permission to the principal, it doesn't state anything about adding to its own trust policy. If you have any suggestions on how to make this more clear, let me know.

Aside from the docstring, we should at the least document a use case of grantAssumeRole(), addToPrincipalPolicy(), and assumeRolePolicy

wz2b commented 11 months ago

I'm not really sure how this can be made more clear in the docstring.

I'm not sure, either. I have a feeling I'm missing something fundamental. As an experiment I tried doing this:

const fnRole = new iam.Role(this, "redirect-function-role-id", {
    roleName: "redirect-function-role",
    assumedBy: new iam.ServicePrincipal('edgelambda.amazonaws.com')
})

then did a cdk synth > file1.json. Then I added this:

 fnRole.grantAssumeRole(new iam.ServicePrincipal('lambda.amazonaws.com'))

then did cdk synth > file2.json. Then I diffed them, and they were the same:

  craproleid58ADBD94:
    Type: AWS::IAM::Role
    Properties:
      AssumeRolePolicyDocument:
        Statement:
          - Action: sts:AssumeRole
            Effect: Allow
            Principal:
              Service: edgelambda.amazonaws.com
        Version: "2012-10-17"
      RoleName: redirect-function-role

What I expected it to do was to add a second principal under the AssumeRolePolicyDocument. It didn't change the role at all, unless it somehow changed it outside of the role definition in the template.

ttais2017 commented 11 months ago

@peterwoodworth : Tnx for your suggestion. I tested that and it works also for me.

w.r.t. the Documentation, I thinks the confusion is related to the name of action "sts:AssumeRole" and then think the developer, the function grantAssumeRole should also be used for that. My suggestion add into the docu as "note" or "warning" . For adding trusted relationships pls use "CompositePrincipal" while creating the role" or something like that.

wz2b commented 11 months ago

I should note that if you use the other method, on the role, assumeRolePolicy?.addStatements that you have full access to the trust policy. This would let you do more than just adding a Composite Principal. There probably aren't a whole lot of reasons you would want to do this, but if you wanted to do something like add a condition to the trust policy that's how you could do it (in addition to adding multiple principals):

            "Condition": {
                "StringEqualsIfExists": {
                    "aws:SourceAccount": "<account-id>"
                }
            }

You might want to do that if you have weird subaccount or cross-account requirements shrug.

Your way is less typing.

aperuru commented 9 months ago

I ran into a similar problem, but I was able to solve this with fewer lines of code using spread operator. The only difference is that I intended to add multiple account principals to the trusted relationship along with the service principal so that the permissions are not wide open.

    const accountsToAllowAssumedBy = ['000000000000', '111111111111'];

    const role = new iam.Role(this, 'Role', {
      roleName: 'AssumeRoleTest',
      assumedBy: new iam.CompositePrincipal(
        new iam.ServicePrincipal('ecs-tasks.amazonaws.com'),
        ...accountsToAllowAssumedBy.map((account) => new iam.AccountPrincipal(account)), //Spread Operator (...)
      ),
    });

Output:

"Role1ABCC5F0": {
   "Type": "AWS::IAM::Role",
   "Properties": {
    "RoleName": "AssumeRoleTest",
    "AssumeRolePolicyDocument": {
     "Statement": [
      {
       "Action": "sts:AssumeRole",
       "Effect": "Allow",
       "Principal": {
        "AWS": [
         "arn:aws:iam::000000000000:root",
         "arn:aws:iam::111111111111:root"
        ],
        "Service": "ecs-tasks.amazonaws.com"
       }
      }
     ],
    },
   },
  },