feat(IAM): Optimize IAM policy statements for size #916

RomainMuller · 2018-10-12T12:39:06Z

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.

Also, fixes #924

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.

rix0rrr · 2018-10-12T13:01:02Z

I wonder if it might make sense to add property-based tests, using some framework like fast-check on the routines that perform these transformations.

RomainMuller · 2018-10-12T13:29:50Z

I can definitely give it a shot!

packages/@aws-cdk/aws-iam/lib/optimize-statements.ts

packages/@aws-cdk/cdk/lib/cloudformation/fn.ts

packages/@aws-cdk/aws-cloudformation/lib/pipeline-actions.ts

packages/@aws-cdk/aws-iam/lib/role.ts

RomainMuller · 2018-10-15T15:38:18Z

Another commit will come out with the requested changes, as soon as I'm done verifying they didn't break anything (this takes a while)

… indeed fixed

RomainMuller · 2018-10-16T10:47:18Z

Changes are in, and build is un-broken. Phew.

This reverts commit 5996442. Fixes #957 Reverts #916

) This reverts commit 5996442. Fixes #957 Reverts #916

This restores the pre-concatenation of `Fn::Join` fragments that was introduced in #916 and reverted in #958 due to an other feature part of the same commit. The goal is to pre-concatenate the string literals present in the concatenated values, and flatten `Fn::Join` nested under another `Fn::Join` using the same delimiter, so as to produce more readable CloudFormation templates. Additionally, if the `Fn::Join` pre-concatenation results in a single string being left, then the `Fn::Join` intrinsic is erased and replaced with a string literal.

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.

__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.

__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.

piotr-practi · 2021-05-18T09:06:01Z

@RomainMuller I guess this was reverted and never merged back, was it?

RomainMuller requested review from rix0rrr and eladb October 12, 2018 12:39

rix0rrr reviewed Oct 12, 2018

View reviewed changes

eladb reviewed Oct 14, 2018

View reviewed changes

packages/@aws-cdk/aws-cloudformation/lib/pipeline-actions.ts Show resolved Hide resolved

packages/@aws-cdk/aws-iam/lib/role.ts Outdated Show resolved Hide resolved

Add property tests for Fn::Join compaction & IAM policy optimizer

d68f8ce

RomainMuller added 3 commits October 15, 2018 17:55

Requested changes

769c28b

Correct expectations from broken test, and add test to verify #924 is…

6bc44a0

… indeed fixed

Fix some more broken stuff

f521c4e

rix0rrr approved these changes Oct 16, 2018

View reviewed changes

RomainMuller merged commit 5996442 into master Oct 16, 2018

RomainMuller deleted the rmuller/compact-fnjoin-and-iam branch October 16, 2018 14:39

eladb mentioned this pull request Oct 18, 2018

IAM policy optimizer is too eager may cause a security issue #957

Closed

eladb pushed a commit that referenced this pull request Oct 18, 2018

Revert "feat(IAM): Optimize IAM policy statements for size (#916)"

e4f4679

This reverts commit 5996442. Fixes #957 Reverts #916

eladb mentioned this pull request Oct 18, 2018

Revert "feat(IAM): Optimize IAM policy statements for size (#916)" #958

Merged

eladb pushed a commit that referenced this pull request Oct 18, 2018

Revert "feat(IAM): Optimize IAM policy statements for size (#916)" (#958

5bb7c93

) This reverts commit 5996442. Fixes #957 Reverts #916

RomainMuller mentioned this pull request Oct 18, 2018

feat(core): Pre-concatenate Fn::Join #967

Merged

RomainMuller mentioned this pull request Oct 22, 2018

fix(iam): Merge multiple principals correctly #983

Merged

rix0rrr mentioned this pull request Oct 26, 2018

v0.14.0 #1021

Merged

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

njlynch mentioned this pull request Sep 3, 2021

fix(codepipeline): large cross-region pipelines exceed IAM policy size limit #16350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(IAM): Optimize IAM policy statements for size #916

feat(IAM): Optimize IAM policy statements for size #916

RomainMuller commented Oct 12, 2018 •

edited

Loading

rix0rrr commented Oct 12, 2018

RomainMuller commented Oct 12, 2018

RomainMuller commented Oct 15, 2018

RomainMuller commented Oct 16, 2018

piotr-practi commented May 18, 2021

feat(IAM): Optimize IAM policy statements for size #916

feat(IAM): Optimize IAM policy statements for size #916

Conversation

RomainMuller commented Oct 12, 2018 • edited Loading

rix0rrr commented Oct 12, 2018

RomainMuller commented Oct 12, 2018

RomainMuller commented Oct 15, 2018

RomainMuller commented Oct 16, 2018

piotr-practi commented May 18, 2021

RomainMuller commented Oct 12, 2018 •

edited

Loading