Skip to content

Commit

Permalink
fix(iam): duplicate PolicyStatements lead to too many overflow policies
Browse files Browse the repository at this point in the history
Historically, policy statement deduplicating was done during policy
rendering.  There could be a 100 statements in a policy, but if they
were all the same it would only render as 1 statement.

When we introduced policy splitting, those 100 equal statements would
*first* be divided into Overflow policies before being deduplicated
(potentially leading to too many policies being created, hitting limits).

Especially CDK Pipelines takes heavy advantage of the deduplicating
behavior, and adds the same policies over and over again, leading
to overflows.

When the `minimizePolicies` feature flag is turned on, duplicate
elimitation is implicitly performed (because equal statements
count as mergeable), but when the feature flag is off we
hit the bad behavior.

As a fix, do deduplication even if merging is turned off.
  • Loading branch information
rix0rrr committed Jun 16, 2022
1 parent 8931f11 commit 2997433
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 10 deletions.
18 changes: 12 additions & 6 deletions packages/@aws-cdk/aws-iam/lib/policy-document.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ export class PolicyDocument implements cdk.IResolvable {
*/
public _maybeMergeStatements(scope: IConstruct): void {
if (this.shouldMerge(scope)) {
const result = mergeStatements(scope, this.statements, false);
const result = mergeStatements(this.statements, { scope });
this.statements.splice(0, this.statements.length, ...result.mergedStatements);
}
}
Expand All @@ -213,11 +213,17 @@ export class PolicyDocument implements cdk.IResolvable {

// Maps final statements to original statements
let statementsToOriginals = new Map(this.statements.map(s => [s, [s]]));
if (this.shouldMerge(scope)) {
const result = mergeStatements(scope, this.statements, true);
this.statements.splice(0, this.statements.length, ...result.mergedStatements);
statementsToOriginals = result.originsMap;
}

// We always run 'mergeStatements' to minimize the policy before splitting.
// However, we only 'merge' when the feature flag is on. If the flag is not
// on, we only combine statements that are *exactly* the same. We must do
// this before splitting, otherwise we may end up with the statement set [X,
// X, X, X, X] being split off into [[X, X, X], [X, X]] before being reduced
// to [[X], [X]] (but should have been just [[X]]).
const doActualMerging = this.shouldMerge(scope);
const result = mergeStatements(this.statements, { scope, limitSize: true, mergeIfCombinable: doActualMerging });
this.statements.splice(0, this.statements.length, ...result.mergedStatements);
statementsToOriginals = result.originsMap;

const sizeOptions = deriveEstimateSizeOptions(scope);

Expand Down
69 changes: 65 additions & 4 deletions packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,32 @@ import { partitionPrincipals } from './comparable-principal';
*/
const MAX_MERGE_SIZE = 2000;

/**
* Options for the mergeStatement command
*/
export interface MergeStatementOptions {
/**
* Scope to derive configuration flags from
*/
readonly scope: IConstruct;

/**
* Do not merge statements if the result would be bigger than MAX_MERGE_SIZE
*
* @default false
*/
readonly limitSize?: boolean;

/**
* Merge statements if they can be combined to produce the same effects.
*
* If false, statements are only merged if they are exactly equal.
*
* @default true
*/
readonly mergeIfCombinable?: boolean;
}

/**
* Merge as many statements as possible to shrink the total policy doc, modifying the input array in place
*
Expand All @@ -26,9 +52,10 @@ const MAX_MERGE_SIZE = 2000;
* Good Enough(tm). If it merges anything, it's at least going to produce a smaller output
* than the input.
*/
export function mergeStatements(scope: IConstruct, statements: PolicyStatement[], limitSize: boolean): MergeStatementResult {
const sizeOptions = deriveEstimateSizeOptions(scope);
export function mergeStatements(statements: PolicyStatement[], options: MergeStatementOptions): MergeStatementResult {
const sizeOptions = deriveEstimateSizeOptions(options.scope);
const compStatements = statements.map(makeComparable);
const mergeFn = options?.mergeIfCombinable ?? true ? mergeIfCombinable : mergeIfEqual;

// Keep trying until nothing changes anymore
while (onePass()) { /* again */ }
Expand All @@ -50,7 +77,7 @@ export function mergeStatements(scope: IConstruct, statements: PolicyStatement[]
for (let i = 0; i < compStatements.length; i++) {
let j = i + 1;
while (j < compStatements.length) {
const merged = tryMerge(compStatements[i], compStatements[j], limitSize, sizeOptions);
const merged = mergeFn(compStatements[i], compStatements[j], !!options.limitSize, sizeOptions);

if (merged) {
compStatements[i] = merged;
Expand Down Expand Up @@ -90,7 +117,12 @@ export interface MergeStatementResult {
* - From their Action, Resource and Principal sets, 2 are subsets of each other
* (empty sets are fine).
*/
function tryMerge(a: ComparableStatement, b: ComparableStatement, limitSize: boolean, options: EstimateSizeOptions): ComparableStatement | undefined {
function mergeIfCombinable(
a: ComparableStatement,
b: ComparableStatement,
limitSize: boolean,
options: EstimateSizeOptions,
): ComparableStatement | undefined {
// Effects must be the same
if (a.statement.effect !== b.statement.effect) { return; }
// We don't merge Sids (for now)
Expand Down Expand Up @@ -128,6 +160,35 @@ function tryMerge(a: ComparableStatement, b: ComparableStatement, limitSize: boo
};
}

/**
* We merge two statements only if they are exactly the same
*/
function mergeIfEqual(a: ComparableStatement, b: ComparableStatement): ComparableStatement | undefined {
if (a.statement.effect !== b.statement.effect) { return; }
if (a.statement.sid !== b.statement.sid) { return; }
if (a.conditionString !== b.conditionString) { return; }
if (
!setEqual(a.statement.notActions, b.statement.notActions) ||
!setEqual(a.statement.notResources, b.statement.notResources) ||
!setEqualPrincipals(a.statement.notPrincipals, b.statement.notPrincipals)
) {
return;
}
if (
!setEqual(a.statement.actions, b.statement.actions) ||
!setEqual(a.statement.resources, b.statement.resources) ||
!setEqualPrincipals(a.statement.principals, b.statement.principals)
) {
return;
}

return {
originals: [...a.originals, ...b.originals],
statement: a.statement,
conditionString: a.conditionString,
};
}

/**
* Calculate and return cached string set representation of the statement elements
*
Expand Down
21 changes: 21 additions & 0 deletions packages/@aws-cdk/aws-iam/test/role.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,27 @@ describe('role with too large inline policy', () => {
});
});

test('many copies of the same statement do not result in overflow policies', () => {
const N = 100;

const app = new App();
const stack = new Stack(app, 'my-stack');
const role = new Role(stack, 'MyRole', {
assumedBy: new ServicePrincipal('service.amazonaws.com'),
});

for (let i = 0; i < N; i++) {
role.addToPrincipalPolicy(new PolicyStatement({
actions: ['aws:DoAThing'],
resources: ['arn:aws:service:us-east-1:111122223333:someResource/SomeSpecificResource'],
}));
}

// THEN
const template = Template.fromStack(stack);
template.resourceCountIs('AWS::IAM::ManagedPolicy', 0);
});

test('cross-env role ARNs include path', () => {
const app = new App();
const roleStack = new Stack(app, 'role-stack', { env: { account: '123456789012', region: 'us-east-1' } });
Expand Down

0 comments on commit 2997433

Please sign in to comment.