Skip to content

Commit

Permalink
fix(iam): policies aren't minimized as far as possible (#19764)
Browse files Browse the repository at this point in the history
IAM Policies were being correctly minimized; however, the minimization
was being performed in one pass across all statements.

It can be that after one pass, statements have ended up in forms that
allow for more merging. Example:

```
[{ A1, R1 }, { A2, R1 }, { A1, R2 }, { A2, R2 }]
// -> (pass one, combine actions)
[{ [A1, A2], R1}, { [A1, A2], R2 }]
// -> (pass two, combine resources)
[{ [A1, A2], [R1, R2] }]
```

Change to perform minimization passes until nothing changes anymore.

Fixes #19751.


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr committed Apr 5, 2022
1 parent 5ce0ad3 commit 876ed8a
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 16 deletions.
39 changes: 23 additions & 16 deletions packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,26 +18,33 @@ import { StatementSchema, normalizeStatement, IamValue } from './postprocess-pol
export function mergeStatements(statements: StatementSchema[]): StatementSchema[] {
const compStatements = statements.map(makeComparable);

let i = 0;
while (i < compStatements.length) {
let didMerge = false;

for (let j = i + 1; j < compStatements.length; j++) {
const merged = tryMerge(compStatements[i], compStatements[j]);
if (merged) {
compStatements[i] = merged;
compStatements.splice(j, 1);
didMerge = true;
break;
// Keep trying until nothing changes anymore
while (onePass()) { /* again */ }
return compStatements.map(renderComparable);

// Do one optimization pass, return 'true' if we merged anything
function onePass() {
let ret = false;
let i = 0;
while (i < compStatements.length) {
let didMerge = false;

for (let j = i + 1; j < compStatements.length; j++) {
const merged = tryMerge(compStatements[i], compStatements[j]);
if (merged) {
compStatements[i] = merged;
compStatements.splice(j, 1);
ret = didMerge = true;
break;
}
}
}

if (!didMerge) {
i++;
if (!didMerge) {
i++;
}
}
return ret;
}

return compStatements.map(renderComparable);
}

/**
Expand Down
30 changes: 30 additions & 0 deletions packages/@aws-cdk/aws-iam/test/merge-statements.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,36 @@ test('fail merging typed and untyped principals', () => {
]);
});

test('keep merging even if it requires multiple passes', () => {
// [A, R1], [B, R1], [A, R2], [B, R2]
// -> [{A, B}, R1], [{A, B], R2]
// -> [{A, B}, {R1, R2}]
assertMerged([
new iam.PolicyStatement({
actions: ['service:A'],
resources: ['R1'],
}),
new iam.PolicyStatement({
actions: ['service:B'],
resources: ['R1'],
}),
new iam.PolicyStatement({
actions: ['service:A'],
resources: ['R2'],
}),
new iam.PolicyStatement({
actions: ['service:B'],
resources: ['R2'],
}),
], [
{
Effect: 'Allow',
Action: ['service:A', 'service:B'],
Resource: ['R1', 'R2'],
},
]);
});

function assertNoMerge(statements: iam.PolicyStatement[]) {
const app = new App();
const stack = new Stack(app, 'Stack');
Expand Down

0 comments on commit 876ed8a

Please sign in to comment.