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): (Adding multiple AccountPrincipal to a role) #27017

Closed
RonLek opened this issue Sep 5, 2023 · 5 comments
Closed

(IAM): (Adding multiple AccountPrincipal to a role) #27017

RonLek opened this issue Sep 5, 2023 · 5 comments
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@RonLek
Copy link

RonLek commented Sep 5, 2023

Describe the bug

Through the console, I can add multiple principals by specifying the account ids as a list.

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": ["account_id1", "account_id2"]
            },
            "Action": "sts:AssumeRole"
        }
    ]
}

How do I achieve this via the CDK? I tried using CompositePrincipal but later realized it only accepts ServicePrincipal as parameters.

Expected Behavior

Expected cdk synth to produce output like when modifying through console above. If CompositePrincipal does not take in AccountPrincipal it should error out during synth.

Current Behavior

When specifying account Id within CompositePrincipal I get the following on cdk synth without any errors.

"Role1": {
      "Type": "AWS::IAM::Role",
      "Properties": {
        "AssumeRolePolicyDocument": {
          "Statement": [
            {
              "Action": "sts:AssumeRole",
              "Effect": "Allow",
              "Principal": {
                "AWS": {
                  "Fn::Join": [
                    "",
                    [
                      "arn:",
                      {
                        "Ref": "AWS::Partition"
                      },
                      ":iam::account_id_1:root"
                    ]
                  ]
                }
              }
            },
            {
              "Action": "sts:AssumeRole",
              "Effect": "Allow",
              "Principal": {
                "AWS": {
                  "Fn::Join": [
                    "",
                    [
                      "arn:",
                      {
                        "Ref": "AWS::Partition"
                      },
                      ":iam::account_id_2:root"
                    ]
                  ]
                }
              }
            }
          ],
          "Version": "2012-10-17"
        },
        "Description": "IAM role for outside AWS accounts to call send_ses_email lambda",
        "Policies": [
         {inline_policies}
        ],
        "RoleName": "Role1"
      },
    },

Reproduction Steps

Use AccountPrincipal as argument to CompositePrincipal.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.87.0

Framework Version

No response

Node.js Version

v16.3.0

OS

Amazon Linux

Language

Typescript

Language Version

No response

Other information

No response

@RonLek RonLek added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 5, 2023
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Sep 5, 2023
@peterwoodworth
Copy link
Contributor

peterwoodworth commented Sep 5, 2023

Aren't these statements identical in functionality? I'm not sure looking at the policies why the second one is problematic

@peterwoodworth peterwoodworth added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Sep 5, 2023
@RonLek
Copy link
Author

RonLek commented Sep 5, 2023

My bad, you're correct. On a closer look the join statement does produce the same principal. The CompositePrincipal doc only lists ServicePrincipal which made me think you could not use AccountPrincipal with it. Thanks.

@RonLek RonLek closed this as completed Sep 5, 2023
@github-actions
Copy link

github-actions bot commented Sep 5, 2023

⚠️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.

@abhi7cr
Copy link

abhi7cr commented Dec 12, 2023

Although these are identical behaviors at runtime, we could run into character limits (2048) more easily with cdk v2. We ran across this issue when migrating from v1 to v2 since we had a large list of account ids that were previously under 1 statement, but now separated into their own. Is there any way we can override this behavior ?

@abhi7cr
Copy link

abhi7cr commented Dec 13, 2023

For future reference, I was able to get around with a custom principal:

import { ArnPrincipal, Conditions, PrincipalBase, PrincipalPolicyFragment } from 'aws-cdk-lib/aws-iam';

export default class CustomCompositePrincipal extends PrincipalBase {
  /**
   *
   */
  constructor(principals: ArnPrincipal[], conditions: Conditions = {}) {
    super();
    this.policyFragment = {
      principalJson: {
        AWS: principals.map((p) => p.arn),
      },
      conditions,
    };
  }

  policyFragment: PrincipalPolicyFragment;

  // eslint-disable-next-line
  dedupeString(): string | undefined {
    return undefined;
  }
}

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. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

No branches or pull requests

3 participants