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

(aws-iam): changes in #17689 increase assume role policy size #18564

Closed
benmunro opened this issue Jan 20, 2022 · 2 comments · Fixed by #19114
Closed

(aws-iam): changes in #17689 increase assume role policy size #18564

benmunro opened this issue Jan 20, 2022 · 2 comments · Fixed by #19114
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@benmunro
Copy link
Contributor

What is the problem?

After updating to CDK verison 1.138.0 from 1.112.0 my CloudFormation deployments started failed with the following error

Cannot exceed quota for ACLSizePerRole: 2048 (Service: AmazonIdentityManagement; Status Code: 409; Error Code: LimitExceeded; Request ID: 45c28053-a294-426e-a4a1-5d1370c10de5; Proxy: null)

This is because the formatting of the role policy changed to have a statement per principal allowing the sts:AssumeRole action rather than a single statement for all the principals. My role allows ~25 accounts to assume it which generates a policy over the limit in the new CDK version. This diff of a test case from that commit mirrors what I am seeing 9f22b2f#diff-a9e05944220b717b56d514486d7213bd99085c533f08d22b0d0606220bd74567.

I'm raising this as a bug since it caused my previously working stack to fail to deploy after the update. For now I've worked around this with a custom iam.IPrincipal implementation which returns a iam.PrincipalPolicyFragment containing all of my principals.

Reproduction Steps

Create a stack with the following

import * as cdk from "monocdk";
import * as iam from "monocdk/aws-iam";

class TestStack extends cdk.Stack {
  constructor(scope: cdk.App) {
    super(scope, "Stack");
    new iam.Role(this, "CrossAccountSharingRole", {
      assumedBy: new iam.CompositePrincipal(
        new iam.AccountPrincipal("123"),
        new iam.AccountPrincipal("456"),
      ),
      roleName: "CloudWatch-CrossAccountSharingRole",
      managedPolicies: [
        iam.ManagedPolicy.fromAwsManagedPolicyName("CloudWatchReadOnlyAccess"),
      ],
    });
  }
}

What did you expect to happen?

An AssumeRolePolicyDocument with many principals

Resources:
  CrossAccountSharingRoleFB26871E:
    Type: AWS::IAM::Role
    Properties:
      AssumeRolePolicyDocument:
        Statement:
          - Action: sts:AssumeRole
            Effect: Allow
            Principal:
              AWS:
                - Fn::Join:
                    - ""
                    - - "arn:"
                      - Ref: AWS::Partition
                      - :iam::123:root
                - Fn::Join:
                    - ""
                    - - "arn:"
                      - Ref: AWS::Partition
                      - :iam::456:root
        Version: "2012-10-17"
      ManagedPolicyArns:
        - Fn::Join:
            - ""
            - - "arn:"
              - Ref: AWS::Partition
              - :iam::aws:policy/CloudWatchReadOnlyAccess
      RoleName: CloudWatch-CrossAccountSharingRole

What actually happened?

Many AssumeRolePolicyDocuments with a single principal in each

Resources:
  CrossAccountSharingRoleFB26871E:
    Type: AWS::IAM::Role
    Properties:
      AssumeRolePolicyDocument:
        Statement:
          - Action: sts:AssumeRole
            Effect: Allow
            Principal:
              AWS:
                - Fn::Join:
                    - ""
                    - - "arn:"
                      - Ref: AWS::Partition
                      - :iam::123:root
          - Action: sts:AssumeRole
            Effect: Allow
            Principal:
              AWS:
                - Fn::Join:
                    - ""
                    - - "arn:"
                      - Ref: AWS::Partition
                      - :iam::456:root
        Version: "2012-10-17"
      ManagedPolicyArns:
        - Fn::Join:
            - ""
            - - "arn:"
              - Ref: AWS::Partition
              - :iam::aws:policy/CloudWatchReadOnlyAccess
      RoleName: CloudWatch-CrossAccountSharingRole

CDK CLI Version

1.138.0 (build 6dbfe8f)

Framework Version

No response

Node.js Version

v14.18.3

OS

AL2

Language

Typescript

Language Version

No response

Other information

No response

@benmunro benmunro added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 20, 2022
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Jan 20, 2022
@NGL321 NGL321 added p1 and removed needs-triage This issue or PR still needs to be triaged. labels Jan 26, 2022
@holomekc
Copy link

At least in java we could overcome this via:

public static PrincipalBase createSingleStatementIamPrincipal(final List<ArnPrincipal> users) {
    // return new CompositePrincipal(users.toArray(new PrincipalBase[0]))
    return new PrincipalBase() {
        @Override
        public @NotNull PrincipalPolicyFragment getPolicyFragment() {
            final HashMap<String, List<String>> map = new HashMap<>();
            map.put("AWS", users.stream().map(ArnPrincipal::getArn).collect(Collectors.toList()));
            return new PrincipalPolicyFragment(map);
        }
    };
}

Not sure if this helps you.

Would be great to have more control over what is generated by CompositePrincipal.

@rix0rrr rix0rrr added the effort/medium Medium work item – several days of effort label Feb 8, 2022
@rix0rrr rix0rrr removed their assignment Feb 8, 2022
@mergify mergify bot closed this as completed in #19114 Mar 18, 2022
mergify bot pushed a commit that referenced this issue Mar 18, 2022
The policies we generate sometimes have a lot of duplication between
statements. This duplication can lead to the policy going over the size
limit an IAM policy (either 2k, 6k or 10k bytes, depending on the resource
type).

This change combines multiple statements together, as long as it
doesn't change the meaning of the final policy.

Because doing so for all existing stacks will probably provoke minor
heart attacks in operators everywhere, the new behavior is gated
behind a feature flag. It can be retroactively switched on by
people currently being bit by the size issues:

```
@aws-cdk/aws-iam:minimizePolicies
```

We will merge 2 statements if their effects are the same, and they are otherwise exactly the same apart from their `Action`, `Resource` or `Principal` declarations. We will not merge `NotXxx` statements, because doing so will change the meaning of the statement (`not A or not B ≠ not (A or B)`). There may be multiple possible merges that apply and we are not guaranteed to find the smallest merging, nor do we take effort to find all possible merges and do simplifications like `*`-subsumption. This is a starting point that should help out in the common case.

Fixes #18774, fixes #16350, fixes #18457, fixes #18564, fixes #19276.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

iliapolo pushed a commit that referenced this issue Mar 20, 2022
The policies we generate sometimes have a lot of duplication between
statements. This duplication can lead to the policy going over the size
limit an IAM policy (either 2k, 6k or 10k bytes, depending on the resource
type).

This change combines multiple statements together, as long as it
doesn't change the meaning of the final policy.

Because doing so for all existing stacks will probably provoke minor
heart attacks in operators everywhere, the new behavior is gated
behind a feature flag. It can be retroactively switched on by
people currently being bit by the size issues:

```
@aws-cdk/aws-iam:minimizePolicies
```

We will merge 2 statements if their effects are the same, and they are otherwise exactly the same apart from their `Action`, `Resource` or `Principal` declarations. We will not merge `NotXxx` statements, because doing so will change the meaning of the statement (`not A or not B ≠ not (A or B)`). There may be multiple possible merges that apply and we are not guaranteed to find the smallest merging, nor do we take effort to find all possible merges and do simplifications like `*`-subsumption. This is a starting point that should help out in the common case.

Fixes #18774, fixes #16350, fixes #18457, fixes #18564, fixes #19276.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants