Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to build multi-principal Policy with Role #1201

Closed
ijcd opened this issue Nov 17, 2018 · 14 comments · Fixed by #1377
Closed

Unable to build multi-principal Policy with Role #1201

ijcd opened this issue Nov 17, 2018 · 14 comments · Fixed by #1377
Assignees
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. feature-request A feature should be added or improved.

Comments

@ijcd
Copy link

ijcd commented Nov 17, 2018

I am trying to create a role with the following policy document:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "AWS": "${aws_iam_role.eks_nodes.arn}",
        "Service": "ec2.amazonaws.com"
      },
      "Action": "sts:AssumeRole"
    }
  ]
}

The problem, however, is that the Role construct only takes a single entity for assumedBy.

const defaultPodRole = new iam.Role(this, "default-pod-role", {
        roleName: "default",
        path: "/pods/",
        assumedBy: new iam.ServicePrincipal("ec2.amazonaws.com"),
      })

I tried pulling out the assumeRolePolicy but its statements member is private, and that would involve digging around in the statements array anyway.

export declare class PolicyDocument extends Token {
    private readonly baseDocument?;
    private statements;

This workaround produces a role/policy/trust setup that collapses to the same interpretation, but the resulting document is not the same.

    const role = new iam.Role(this, 'Role', {
      assumedBy: new iam.ServicePrincipal('ec2.amazonaws.com'),
    });
    if ( role.assumeRolePolicy ) {
      role.assumeRolePolicy.addStatement(new iam.PolicyStatement().
        addAccountRootPrincipal().
        addAction('sts:AssumeRole'));
    }

The resulting document is this:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "Service": "ec2.amazonaws.com"
      },
      "Action": "sts:AssumeRole"
    },
    {
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::123456123456:root"
      },
      "Action": "sts:AssumeRole"
    }
  ]
}
@rix0rrr rix0rrr added feature-request A feature should be added or improved. @aws-cdk/aws-iam Related to AWS Identity and Access Management labels Nov 19, 2018
@eladb
Copy link
Contributor

eladb commented Nov 19, 2018

This is indeed a limitation. Technically we can provide an implementation of PolicyPrincipal that will allow combining multiple principals.

@ijcd can you share a bit more details about your use case? Why do you need this?

@eladb
Copy link
Contributor

eladb commented Nov 29, 2018

Thanks @eamonnfaherty, exactly what I was after.

@deen13
Copy link

deen13 commented Dec 3, 2018

Is there any progress in this issue? I do also need this feature to connect my API Gateway directly to an DynmoDB Table. 😥

@eladb eladb added the bug This issue is a bug. label Dec 17, 2018
@eladb eladb self-assigned this Dec 17, 2018
@eladb
Copy link
Contributor

eladb commented Dec 17, 2018

@deen13 PR pending

@eamonnfaherty
Copy link

To be honest for the example I specified I would like to see that working via a construct.

Generally, allowing a role to be assumed by two services is a sign that the author is breaking least privileged principle.

@eladb
Copy link
Contributor

eladb commented Dec 17, 2018

@deen13 can you provide some reference to your use case please?

@eladb
Copy link
Contributor

eladb commented Dec 17, 2018

https://github.com/eamonnfaherty Nevertheless, we rather not be opinionated in the low level IAM library. Higher level AWS constructs synthesize policies based on least privilege but if users want to define less restrictive policies for some reason they should be able to do that with the CDK.

@eamonnfaherty
Copy link

Fair enough.

eladb pushed a commit that referenced this issue Dec 18, 2018
Relax constraint on IAM policy statement principals such
that multiple principal types can be used in a statement.

Also, the `CompositePrincipal` class can be use to construct
`PolicyPrincipal`s that consist of multiple principal types (without
conditions)

Backfill missing addXxxPrincipal methods.

Deprecate (soft) `Anyone` in favor of `AnyPrincipal`.

Fixes #1201
@alastairmccormack
Copy link

alastairmccormack commented Oct 9, 2019

According to #1377, the way to do this is by using a CompositePrincipal object. (Surely changing Role.assumed_by to accept a list or adding a new method to Role would've been more obvious).

E.g. In Python:

pipeline_role = aws_iam.Role(
    scope=self, id=f'pipeline-role',
    role_name='pipeline',
    assumed_by=aws_iam.CompositePrincipal(
        aws_iam.ServicePrincipal('datapipeline.amazonaws.com'),
        aws_iam.ServicePrincipal('elasticmapreduce.amazonaws.com')
    )
)

@0xjjoyy
Copy link

0xjjoyy commented Dec 17, 2019

@eladb Use case system manager automation document.

Here the example provides ssm and ec2 service principals.

It's also in the Systems Manager user guide.

I also was only able to resolve this by searching and finding this Github issue.

It would be easier if this was better documented in the docs itself for assumed_by in Role.

@ahammond
Copy link
Contributor

Another example:

    const dmsAccessForEndpointRole = new iam.Role(this, 'DmsAccessForEndpointRole', {
      roleName: 'dms-access-for-endpoint',
      assumedBy: dmsServicePrincipal,
    });
    dmsAccessForEndpointRole.assumeRolePolicy?.addStatements(
      new iam.PolicyStatement({
        effect: iam.Effect.ALLOW,
        principals: [new iam.ServicePrincipal('redshift.amazonaws.com')],
        actions: ['sts:AssumeRole'],
      }),
    );

@ajhool
Copy link

ajhool commented Jan 28, 2021

Node example for a lambda@edge

    const someLambdaRole = new Role(this, 'someLambdaRole', {
      assumedBy: new CompositePrincipal(new ServicePrincipal("lambda"), new ServicePrincipal("edgelambda"))
    })

@iwt-dennyschaefer
Copy link

Hello 👋

Disclaimer: AWS CDK v1 👈 Would have to check the back then used version ✏️ .

I stumbled over the following problem during working on one of our projects.

During the CloudFormation deployment following error occurred.

Cannot exceed quota for ACLSizePerRole: 2048
(Service: AmazonIdentityManagement; Status Code: 409; Error Code: ...

This error states that the trust relationship policy is too big
(>= 2048 characters)

This was definitely unexpected, with locking on the synthesized code and the already deployed IAM policy it became clear that the AWS CDK produced trust relationship policy is valid but highly inefficient because it generated for each user a new policy statement!

Example Code:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::232324654350:root"
      },
      "Action": "sts:AssumeRole"
    },
    {
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::123456123456:root"
      },
      "Action": "sts:AssumeRole"
    }
  ]
}

Instead of:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::232324654350:root"
        "AWS": "arn:aws:iam::123456123456:root"
      },
      "Action": "sts:AssumeRole"
    }
}

Of course, the AWS CDK code produces way more characters for producing the same result! There is no (at least I did not found any other) way to solve this issue with AWS CDK "tools". The only valid method to create an assumeBy policy statement with more than one principle is to use the iam.CompositePrincipal method which causes the above-mentioned behavior.

Cfn resources to the rescue, in the end, the only way I found was to generate the needed PoliyStatement manual and inject it directly into the CloudFormation code of the IAM role, this will override all previously defined policies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants