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

AccountPrinicpal does not support multiple accounts #924

Closed
ccurrie-amzn opened this issue Oct 12, 2018 · 4 comments · Fixed by #916 or #1021
Closed

AccountPrinicpal does not support multiple accounts #924

ccurrie-amzn opened this issue Oct 12, 2018 · 4 comments · Fixed by #916 or #1021
Labels
bug This issue is a bug.

Comments

@ccurrie-amzn
Copy link
Contributor

ccurrie-amzn commented Oct 12, 2018

If a single role is used to grant access to multiple accounts, the principal does not output correct cloudformation:

new AccountPrincipal(["123","456"])   

generates:

AWS:
    'Fn::Join':
        - ''
        -
            - 'arn:'
            -
                Ref: 'AWS::Partition'
            - ':iam::123,456:root'

It should instead generate:

AWS:
    -
        'Fn::Join':
            - ''
            -
                - 'arn:'
                -
                    Ref: 'AWS::Partition'
                - ':iam::123:root'
    -
        'Fn::Join':
            - ''
            -
                - 'arn:'
                -
                    Ref: 'AWS::Partition'
                - ':iam::456:root'

The current workaround is to build the role using property overrides.

@eladb eladb added the bug This issue is a bug. label Oct 14, 2018
RomainMuller added a commit that referenced this issue Oct 16, 2018
* feat(IAM): Optimize IAM policy statements for size

Attempts to reduce policy sizes by performing a number of optimizations:
- `FnConcat` pre-concatenates string literals that are not tokens, to reduce rendered size
- Redundant patterns in `Action` and `Resource` policy elements get de-duplicated (aka:
  `["iam:GetObject", "iam:Get*"]` becomes `["iam:Get*"]`, since this implies `"iam:GetObject"`)
- Redundant statements get de-duplicated (if a statement offers strictly broader permissions
  than another one, then the later will be removed).
- Statements that differ only in `Resource` content get merged (adding entries to the `Resource` element).

IAM policy statements that have an `Sid` are not merged with anything (this could lead to the `Sid`
mis-representing the statement's intention). They are also not de-duplicated (unless the other statement
has the exact same `Sid`, which is likely symptom of a programming error).

This causes widespread changes to the IAM policy documents (and some occurrences of `Fn::Join` intrinsic
all over the place, so this commit is... massive). Generally speaking, this appears to make for smaller
templates, which at least is not a bad thing.

Fixes #924
RomainMuller added a commit that referenced this issue Oct 22, 2018
When adidng multiple principals, an array of principals was created, but
IAM documents expect an object with different keys, for which values may
be arrays. This corrects the merging process so it produces valid
statements.

This restores the fix for #924 that was introduced in #916 and reverted
in #958 due to an other feature part of the same commit.
RomainMuller added a commit that referenced this issue Oct 23, 2018
When adidng multiple principals, an array of principals was created, but
IAM documents expect an object with different keys, for which values may
be arrays. This corrects the merging process so it produces valid
statements.

This restores the fix for #924 that was introduced in #916 and reverted
in #958 due to an other feature part of the same commit.
rix0rrr pushed a commit that referenced this issue Oct 26, 2018
__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade
your installation the CDK Toolkit to the matching version:

```shell
$ npm i -g aws-cdk
$ cdk --version
0.14.0 (build ...)
```

Bug Fixes
=========

* remove CloudFormation property renames ([#973](#973)) ([3f86603](3f86603)), closes [#852](#852)
* **aws-ec2:** fix retention of all egress traffic rule ([#998](#998)) ([b9d5b43](b9d5b43)), closes [#987](#987)
* **aws-s3-deployment:** avoid deletion during update using physical ids ([#1006](#1006)) ([bca99c6](bca99c6)), closes [#981](#981) [#981](#981)
* **cloudformation-diff:** ignore changes to DependsOn ([#1005](#1005)) ([3605f9c](3605f9c)), closes [#274](#274)
* **cloudformation-diff:** track replacements ([#1003](#1003)) ([a83ac5f](a83ac5f)), closes [#1001](#1001)
* **docs:** fix EC2 readme for "natgatway" configuration ([#994](#994)) ([0b1e7cc](0b1e7cc))
* **docs:** updates to contribution guide ([#997](#997)) ([b42e742](b42e742))
* **iam:** Merge multiple principals correctly ([#983](#983)) ([3fc5c8c](3fc5c8c)), closes [#924](#924) [#916](#916) [#958](#958)

Features
=========

* add construct library for Application AutoScaling ([#933](#933)) ([7861c6f](7861c6f)), closes [#856](#856) [#861](#861) [#640](#640) [#644](#644)
* add HostedZone context provider ([#823](#823)) ([1626c37](1626c37))
* **assert:** haveResource lists failing properties ([#1016](#1016)) ([7f6f3fd](7f6f3fd))
* **aws-cdk:** add CDK app version negotiation ([#988](#988)) ([db4e718](db4e718)), closes [#891](#891)
* **aws-codebuild:** Introduce a CodePipeline test Action. ([#873](#873)) ([770f9aa](770f9aa))
* **aws-sqs:** Add grantXxx() methods ([#1004](#1004)) ([8c90350](8c90350))
* **core:** Pre-concatenate Fn::Join ([#967](#967)) ([33c32a8](33c32a8)), closes [#916](#916) [#958](#958)

BREAKING CHANGES
=========

* DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling.
* CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names.
* You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.
@rix0rrr rix0rrr mentioned this issue Oct 26, 2018
rix0rrr added a commit that referenced this issue Oct 26, 2018
__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade
your installation the CDK Toolkit to the matching version:

```shell
$ npm i -g aws-cdk
$ cdk --version
0.14.0 (build ...)
```

Bug Fixes
=========

* remove CloudFormation property renames ([#973](#973)) ([3f86603](3f86603)), closes [#852](#852)
* **aws-ec2:** fix retention of all egress traffic rule ([#998](#998)) ([b9d5b43](b9d5b43)), closes [#987](#987)
* **aws-s3-deployment:** avoid deletion during update using physical ids ([#1006](#1006)) ([bca99c6](bca99c6)), closes [#981](#981) [#981](#981)
* **cloudformation-diff:** ignore changes to DependsOn ([#1005](#1005)) ([3605f9c](3605f9c)), closes [#274](#274)
* **cloudformation-diff:** track replacements ([#1003](#1003)) ([a83ac5f](a83ac5f)), closes [#1001](#1001)
* **docs:** fix EC2 readme for "natgatway" configuration ([#994](#994)) ([0b1e7cc](0b1e7cc))
* **docs:** updates to contribution guide ([#997](#997)) ([b42e742](b42e742))
* **iam:** Merge multiple principals correctly ([#983](#983)) ([3fc5c8c](3fc5c8c)), closes [#924](#924) [#916](#916) [#958](#958)

Features
=========

* add construct library for Application AutoScaling ([#933](#933)) ([7861c6f](7861c6f)), closes [#856](#856) [#861](#861) [#640](#640) [#644](#644)
* add HostedZone context provider ([#823](#823)) ([1626c37](1626c37))
* **assert:** haveResource lists failing properties ([#1016](#1016)) ([7f6f3fd](7f6f3fd))
* **aws-cdk:** add CDK app version negotiation ([#988](#988)) ([db4e718](db4e718)), closes [#891](#891)
* **aws-codebuild:** Introduce a CodePipeline test Action. ([#873](#873)) ([770f9aa](770f9aa))
* **aws-sqs:** Add grantXxx() methods ([#1004](#1004)) ([8c90350](8c90350))
* **core:** Pre-concatenate Fn::Join ([#967](#967)) ([33c32a8](33c32a8)), closes [#916](#916) [#958](#958)

BREAKING CHANGES
=========

* DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling.
* CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names.
* You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.
jonparker pushed a commit to jonparker/aws-cdk that referenced this issue Oct 29, 2018
__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade
your installation the CDK Toolkit to the matching version:

```shell
$ npm i -g aws-cdk
$ cdk --version
0.14.0 (build ...)
```

Bug Fixes
=========

* remove CloudFormation property renames ([aws#973](aws#973)) ([3f86603](aws@3f86603)), closes [aws#852](aws#852)
* **aws-ec2:** fix retention of all egress traffic rule ([aws#998](aws#998)) ([b9d5b43](aws@b9d5b43)), closes [aws#987](aws#987)
* **aws-s3-deployment:** avoid deletion during update using physical ids ([aws#1006](aws#1006)) ([bca99c6](aws@bca99c6)), closes [aws#981](aws#981) [aws#981](aws#981)
* **cloudformation-diff:** ignore changes to DependsOn ([aws#1005](aws#1005)) ([3605f9c](aws@3605f9c)), closes [aws#274](aws#274)
* **cloudformation-diff:** track replacements ([aws#1003](aws#1003)) ([a83ac5f](aws@a83ac5f)), closes [aws#1001](aws#1001)
* **docs:** fix EC2 readme for "natgatway" configuration ([aws#994](aws#994)) ([0b1e7cc](aws@0b1e7cc))
* **docs:** updates to contribution guide ([aws#997](aws#997)) ([b42e742](aws@b42e742))
* **iam:** Merge multiple principals correctly ([aws#983](aws#983)) ([3fc5c8c](aws@3fc5c8c)), closes [aws#924](aws#924) [aws#916](aws#916) [aws#958](aws#958)

Features
=========

* add construct library for Application AutoScaling ([aws#933](aws#933)) ([7861c6f](aws@7861c6f)), closes [aws#856](aws#856) [aws#861](aws#861) [aws#640](aws#640) [aws#644](aws#644)
* add HostedZone context provider ([aws#823](aws#823)) ([1626c37](aws@1626c37))
* **assert:** haveResource lists failing properties ([aws#1016](aws#1016)) ([7f6f3fd](aws@7f6f3fd))
* **aws-cdk:** add CDK app version negotiation ([aws#988](aws#988)) ([db4e718](aws@db4e718)), closes [aws#891](aws#891)
* **aws-codebuild:** Introduce a CodePipeline test Action. ([aws#873](aws#873)) ([770f9aa](aws@770f9aa))
* **aws-sqs:** Add grantXxx() methods ([aws#1004](aws#1004)) ([8c90350](aws@8c90350))
* **core:** Pre-concatenate Fn::Join ([aws#967](aws#967)) ([33c32a8](aws@33c32a8)), closes [aws#916](aws#916) [aws#958](aws#958)

BREAKING CHANGES
=========

* DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling.
* CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names.
* You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.
@symroe
Copy link

symroe commented Feb 24, 2022

I wonder if this has been regressed since this issue was closed?

Today, with aws-cdk.aws-iam==1.145.0 (using Python 3.8):

iam.Role(
    self,
    "role-name",
    assumed_by=iam.AccountPrincipal(["123","456"]),
)

Produces

AWS:
  Fn::Join:
    - ""
    - - "arn:"
      - Ref: AWS::Partition
      - :iam::123,456:root

I would expect to be able to pass a list of account IDs, as per the initial bug report and fix.

(My use case is to take a list from SSM and pass it in, but it looks like there might be other issues there)

@thieslei
Copy link

Hi Folks.

Any updates about it?

My case is very similar to @symroe, I'm receiving a list of accountIDs and a need to pass it to a ECR policy as multiple Principals.

I'm using Python 3.9 and CDK 2.8.

@thieslei
Copy link

Problem Solved.

We used the append function and save the results into a variable.

PrincipalList = []
for account in AccountList:
    PrincipalList.append(iam.AccountPrincipal(account))

Then we used the variable to forward the principals list to the PolicyStatement.

I hope it helps 👍 😁 .

@aperuru
Copy link

aperuru commented Oct 19, 2023

I ran into a similar problem, but I was able to solve this using spread operator.
The only difference is that I intended to add multiple account principals to the trusted relationship along with the service principal so that the permissions are not wide open.

    const accountsToAllowAssumedBy = ['000000000000', '111111111111'];

    const role = new iam.Role(this, 'Role', {
      roleName: 'AssumeRoleTest',
      assumedBy: new iam.CompositePrincipal(
        new iam.ServicePrincipal('ecs-tasks.amazonaws.com'),
        ...accountsToAllowAssumedBy.map((account) => new iam.AccountPrincipal(account)), //Spread Operator (...)
      ),
    });

Output:

"Role1ABCC5F0": {
   "Type": "AWS::IAM::Role",
   "Properties": {
    "RoleName": "AssumeRoleTest",
    "AssumeRolePolicyDocument": {
     "Statement": [
      {
       "Action": "sts:AssumeRole",
       "Effect": "Allow",
       "Principal": {
        "AWS": [
         "arn:aws:iam::000000000000:root",
         "arn:aws:iam::111111111111:root"
        ],
        "Service": "ecs-tasks.amazonaws.com"
       }
      }
     ],
    },
   },
  },

In your case, you can just do this

    const accountsToAllowAssumedBy = ['000000000000', '111111111111'];

    const role = new iam.Role(this, 'Role', {
      roleName: 'AssumeRoleTest',
      assumedBy: new iam.CompositePrincipal(
        ...accountsToAllowAssumedBy.map((account) => new iam.AccountPrincipal(account)), //Spread Operator (...)
      ),
    });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants