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 trust policy role) #30185

Closed
awsrookie18 opened this issue May 13, 2024 · 8 comments
Closed

(IAM): (Adding multiple AccountPrincipal to a trust policy role) #30185

awsrookie18 opened this issue May 13, 2024 · 8 comments
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@awsrookie18
Copy link

Describe the bug

We are having issues with policy length of 4kb limit and earlier we were creating single object for each aws acount.
{ "Version": "2012-10-17", "Statement": [ { "Effect": "Allow", "Principal": { "AWS": "arn:aws:iam::<account2>:root" }, "Action": "sts:AssumeRole" }, { "Effect": "Allow", "Principal": { "AWS": "arn:aws:iam::<account1>:root" }, "Action": "sts:AssumeRole" } ] }

Because of the limit issue we now want to go ahead with array of account id's in a single object
{ "Version": "2012-10-17", "Statement": [ { "Effect": "Allow", "Principal": { "AWS": ["<accountid1>", "<accountid2>"] }, "Action": "sts:AssumeRole" } ] }

But the CDK construct expects only a string and not an array. But it is possible to do it via AWS console.

Tried using the latest AWS CDK version but no help.
Does anybody have any solution?

Expected Behavior

Through CDK we want to achieve multiple aws accounts in a single array.
{ "Version": "2012-10-17", "Statement": [ { "Effect": "Allow", "Principal": { "AWS": ["<accountid1>", "<accountid2>"] }, "Action": "sts:AssumeRole" } ] }

Current Behavior

It creates separate objects for separate accountid's. This is creating problem with policy length.

Reproduction Steps

Tried multiple ways but no luck
const compositePrincipal = new cdk.aws_iam.CompositePrincipal( ...accountsToAllowAssumedBy.map((account) => new cdk.aws_iam.AccountPrincipal(account)) //Spread Operator (...) );

const policyStatement = new cdk.aws_iam.PolicyStatement({actions: ["sts:AssumeRole"], effect: Effect.ALLOW }) policyStatement.addAwsAccountPrincipal('246597006913'); policyStatement.addAwsAccountPrincipal('344777163811');

Currently no approaches are working.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.141.0

Framework Version

No response

Node.js Version

18

OS

windows

Language

TypeScript

Language Version

No response

Other information

No response

@awsrookie18 awsrookie18 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 13, 2024
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label May 13, 2024
@khushail khushail added the investigating This issue is being investigated and/or work is in progress to resolve the issue. label May 13, 2024
@khushail khushail self-assigned this May 13, 2024
@khushail khushail removed the needs-triage This issue or PR still needs to be triaged. label May 13, 2024
@awsrookie18
Copy link
Author

awsrookie18 commented May 13, 2024

@khushail I think found one of the solution to do this but it is working half of what i am expecting

`const role = new iam.Role(this, 'role', {
assumedBy: new iam.AccountPrincipal(this.account),
});

role.assumeRolePolicy?.addStatements(new iam.PolicyStatement({
actions: ['sts:AssumeRole'],
principals: [
new iam.AccountPrincipal('123456789'),
new iam.ServicePrincipal('beep-boop.amazonaws.com')
],
}));`

or this code

`const policyStatement = new cdk.aws_iam.PolicyStatement({ actions: ['sts:AssumeRole'], effect: Effect.ALLOW });

props.accountIds.forEach((accountId: string) => {
  policyStatement.addAwsAccountPrincipal(accountId);
});`

This kind of solve the problem but here again the JSON will be
"AWS": [ "arn:aws:iam::53674143:root", "arn:aws:iam::25682590:root", "arn:aws:iam::67690013:root", "arn:aws:iam::50406086:root", "arn:aws:iam::05439250:root", "arn:aws:iam::20363038:root",

                The arn is repeated here. Is there anyway to update the above method to only have aws accountid?
                What i am looking for output is 

"AWS": [ "53674143", "25682590", "67690013"]
Through CDK.

@khushail
Copy link
Contributor

khushail commented May 13, 2024

Hi @awsrookie18 , thanks for reaching out.

I am not sure that removing complete arn and just keeping the account id would be good idea. AFAIK, Complete arn would help aws in identifying the principal arn however keeping only the value might not be correct approach for implementation.

There are multiple ways of adding principals. This one mentioned here is also executable and works fine and produces the same synthesized template as given below . I tried another simpler way of adding multiple principals (as you also shared) with using Composite principal and it worked fine as its supposed to do. Sharing the code and generated template -

code -

    const role02 = new iam.Role(this, 'MyRole02', {
      assumedBy: new iam.CompositePrincipal(
        new iam.AccountPrincipal('123456789012'),
        new iam.AccountPrincipal('234567890123'),
        new iam.ServicePrincipal('ec2.amazonaws.com'),
        new iam.ServicePrincipal('lambda.amazonaws.com'),
      )
    });

Template -

{
 "Resources": {
  "MyRole02FD11F62E": {
   "Type": "AWS::IAM::Role",
   "Properties": {
    "AssumeRolePolicyDocument": {
     "Statement": [
      {
       "Action": "sts:AssumeRole",
       "Effect": "Allow",
       "Principal": {
        "AWS": [
         {
          "Fn::Join": [
           "",
           [
            "arn:",
            {
             "Ref": "AWS::Partition"
            },
            ":iam::123456789012:root"
           ]
          ]
         },
         {
          "Fn::Join": [
           "",
           [
            "arn:",
            {
             "Ref": "AWS::Partition"
            },
            ":iam::234567890123:root"
           ]
          ]
         }
        ],
        "Service": [
         "ec2.amazonaws.com",
         "lambda.amazonaws.com"
        ]
       }
      }
     ],
     "Version": "2012-10-17"
    }
   },

Here is CDK doc on how multiple principals can be added using CompositePrincipal.

Hope that helps in clarification!

@khushail khushail added guidance Question that needs advice or information. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels May 13, 2024
@awsrookie18
Copy link
Author

Hi @khushail Yes this does get the job done. But we have many accountid's to be provisioned and we are looking for cutting down the length as we have already crossed 4KB max length.
Here the AWS docs itself speaks that providing just AWS account ID is enough.
https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html#Principal_specifying

"Principal" : {
"AWS": [
"123456789012",
"555555555555"
]
}

So for that reason i was looking for only accountid to be part of this principal and makes things easier for us.

@khushail
Copy link
Contributor

Hi @khushail Yes this does get the job done. But we have many accountid's to be provisioned and we are looking for cutting down the length as we have already crossed 4KB max length. Here the AWS docs itself speaks that providing just AWS account ID is enough. https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html#Principal_specifying

"Principal" : { "AWS": [ "123456789012", "555555555555" ] }

So for that reason i was looking for only accountid to be part of this principal and makes things easier for us.

Oh, I see that. Thanks for sharing that article. Let me do investigate this one and get back to you.

@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels May 13, 2024
@awsrookie18
Copy link
Author

Not sure about the link you shared. This created a single policy statement for each account. What i am looking for is single policy statement with multiple AWS account as an array.
Which i am able to achieve here.

Now only thing that i am confused is https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html#Principal_specifying this and achieving this with CDK.

Thank you for taking the time to investigate this. 👍

@khushail
Copy link
Contributor

@awsrookie18 , I don't see any way of getting the result in form of array as mentioned here. This might be a good feature request so marking it as one.

Contribution from the community are welcome!!

@khushail khushail added p2 feature-request A feature should be added or improved. effort/medium Medium work item – several days of effort and removed bug This issue is a bug. labels May 13, 2024
@khushail khushail removed their assignment May 13, 2024
@khushail khushail removed guidance Question that needs advice or information. investigating This issue is being investigated and/or work is in progress to resolve the issue. labels May 13, 2024
@pahud
Copy link
Contributor

pahud commented May 13, 2024

@awsrookie18

You can implement a custom class like this:

export class CustomPrincipal extends iam.PrincipalBase {
  constructor(public readonly arn: string[]) {
    super();
  }

  public get policyFragment(): iam.PrincipalPolicyFragment {
    return new iam.PrincipalPolicyFragment({ AWS: this.arn });
  }
  public dedupeString(): string | undefined {
    return undefined
  }
}


export class DummyStack extends Stack {
  constructor(scope: Construct, id: string, props: StackProps) {
    super(scope, id, props);

    const trustedAccounts = ['111111111111', '222222222222'];

    new iam.Role(this, 'Role', {
      assumedBy: new CustomPrincipal(trustedAccounts)
    });
  }
}

You get this on synth:

Resources:
  Role1ABCC5F0:
    Type: AWS::IAM::Role
    Properties:
      AssumeRolePolicyDocument:
        Statement:
          - Action: sts:AssumeRole
            Effect: Allow
            Principal:
              AWS:
                - "111111111111"
                - "222222222222"
        Version: "2012-10-17"

Let me know if it works for you.

@pahud pahud added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 14, 2024
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label May 16, 2024
@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label May 21, 2024
@github-actions github-actions bot added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label May 21, 2024
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 closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 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