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

(iam): composite principal not synthesizing correctly #18774

Closed
juweeks opened this issue Feb 1, 2022 · 9 comments · Fixed by #19114
Closed

(iam): composite principal not synthesizing correctly #18774

juweeks opened this issue Feb 1, 2022 · 9 comments · Fixed by #19114
Assignees
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

@juweeks
Copy link

juweeks commented Feb 1, 2022

What is the problem?

behavior as of v2.4 (or maybe v2.5)- instead of composite principals synthesizing to a list, a separate statement is made for each. this can result in overflow of quota for the max number of trust principals policy length with enough principals.

Reproduction Steps

iam.Role(
      self,
      id="role_id",
      assumed_by=iam.CompositePrincipal(
          iam.ArnPrincipal #1,
          iam.ArnPrincipal #2,
          iam.ArnPrincipal #3,
      ),
      max_session_duration=1,
      role_name="MyCompositeTrust"
)

What did you expect to happen?

trust policy look like this:

{
    "Action": "sts:AssumeRole",
    "Effect": "Allow",
    "Principal": {"AWS": [iam.ArnPrincipal #1, iam.ArnPrincipal #2, iam.ArnPrincipal #3]}
}

What actually happened?

trust policy looks like this:

{
    "Action": "sts:AssumeRole",
    "Effect": "Allow",
    "Principal": {"AWS": iam.ArnPrincipal #1}
},
{
    "Action": "sts:AssumeRole",
    "Effect": "Allow",
    "Principal": {"AWS": iam.ArnPrincipal #2}
},
{
    "Action": "sts:AssumeRole",
    "Effect": "Allow",
    "Principal": {"AWS": iam.ArnPrincipal #3}
}

CDK CLI Version

2.10.0 (build e5b301f)

Framework Version

No response

Node.js Version

v14.17.1

OS

Mac 12.0.1

Language

Python

Language Version

Python 3.9.9

Other information

since there is a fairly low quota on the max statements in a trust policy, this breaks very easily with a decent amount of trust principals.

@juweeks juweeks added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 1, 2022
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Feb 1, 2022
@ryparker ryparker added the p2 label Feb 1, 2022
@ryparker
Copy link
Contributor

ryparker commented Feb 1, 2022

Hey @juweeks 👋🏻 Thanks for reporting this.

We're aware of the shortcomings in policy generation and are working hard to resolve this.

Related:
#18167
#18293
#16350
#18457

@ryparker ryparker removed the needs-triage This issue or PR still needs to be triaged. label Feb 1, 2022
@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 8, 2022

Can you please clarify: are you running into a maximum NUMBER of trust principals, or are you running into a POLICY LENGTH limit?

@rix0rrr rix0rrr added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. effort/medium Medium work item – several days of effort p1 and removed p2 labels Feb 8, 2022
@juweeks
Copy link
Author

juweeks commented Feb 8, 2022

Well, I THOUGHT/SWORE it was max number of trust principals. But when I replicate the error, it shows a policy length limit issue.

Cannot exceed quota for ACLSizePerRole: 2048 (Service: AmazonIdentityManagement; Status Code: 409; Error Code: LimitExceeded; Request ID: xxxx; Proxy: null)

And since I can't find any limits anywhere regarding number of trust principals, let's go with POLICY LENGTH limit...

Also just noticed that there is an adjustable service quota for role trust policy length. Still no substitute for properly synthesizing an array of principals, but a temp fix nonetheless.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Feb 8, 2022
@rix0rrr rix0rrr removed their assignment Feb 9, 2022
@iainbowman
Copy link

We are seeing the same behaviour with aws-cdk v1.144.0 and Python 3.7.9

@ytsipun
Copy link

ytsipun commented Feb 18, 2022

+1
I think our problem is related.
After updating CDK from 1.123 to 1.144.0 we're losing a principal as the diffs show.
Code:

    new cloudfront.experimental.EdgeFunction(
...
        role: new iam.Role(this, 'DatadomeLambdaEdgeRole', {
          assumedBy: new iam.CompositePrincipal(
            new iam.ServicePrincipal('lambda.amazonaws.com'),
            new iam.ServicePrincipal('edgelambda.amazonaws.com'),
          ),

Diffs:

│   │ Resource                               │ Effect │ Action         │ Principal                                                    │ Condition │
├───┼────────────────────────────────────────┼────────┼────────────────┼──────────────────────────────────────────────────────────────┼───────────┤
│ - │ ${Datadome/DatadomeLambdaEdgeRole.Arn} │ Allow  │ sts:AssumeRole │ Service:edgelambda.amazonaws.com                             │           │
│   │                                        │        │                │ Service:lambda.amazonaws.com                                 │           │
├───┼────────────────────────────────────────┼────────┼────────────────┼──────────────────────────────────────────────────────────────┼───────────┤
│ + │ ${Datadome/DatadomeLambdaEdgeRole.Arn} │ Allow  │ sts:AssumeRole │ Service:lambda.amazonaws.com                                 │          

[~] AWS::IAM::Role Datadome/DatadomeLambdaEdgeRole DatadomeDatadomeLambdaEdgeRole711ABAD7 
 └─ [~] AssumeRolePolicyDocument
     └─ [~] .Statement:
         └─ @@ -3,10 +3,7 @@
            [ ]   "Action": "sts:AssumeRole",
            [ ]   "Effect": "Allow",
            [ ]   "Principal": {
            [-]     "Service": [
            [-]       "lambda.amazonaws.com",
            [-]       "edgelambda.amazonaws.com"
            [-]     ]
            [+]     "Service": "lambda.amazonaws.com"
            [ ]   }
            [ ] },
            [ ] {

@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 23, 2022

@ytsipun from the opening brace at the bottom I notice your diff isn't complete. Is there a chance that the next part of the diff adds a new statement with the edgelambda.amazonaws.com that's being removed from the statement you are showing?

@ytsipun
Copy link

ytsipun commented Feb 23, 2022

@rix0rrr, seems like we've missed it - it was added separately.

    "DatadomeDatadomeLambdaEdgeRole711ABAD7": {
      "Type": "AWS::IAM::Role",
      "Properties": {
        "AssumeRolePolicyDocument": {
          "Statement": [
            {
              "Action": "sts:AssumeRole",
              "Effect": "Allow",
              "Principal": {
                "Service": "lambda.amazonaws.com"
              }
            },
            {
              "Action": "sts:AssumeRole",
              "Effect": "Allow",
              "Principal": {
                "Service": "edgelambda.amazonaws.com"
              }
            }
          ],
          "Version": "2012-10-17"
        },

So yeah it's definitely related to this issue.

@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 23, 2022

This is indeed a change that's coming from a code change we recently released. It will not cause any difference in behavior though, the two policy document forms are equivalent. Your principal is not lost, it's just somewhere else.

rix0rrr added a commit that referenced this issue Feb 23, 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
```

Fixes #18774, fixes #16350, fixes #18457.
@rix0rrr rix0rrr self-assigned this Mar 14, 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.

5 participants