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

fix(iam): IAM Policies are too large to deploy #19114

Merged
merged 37 commits into from Mar 18, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
f8208e9
fix(iam): IAM Policies are too large to deploy
rix0rrr Feb 23, 2022
777c24d
Add unit tests
rix0rrr Feb 24, 2022
e9b6cdc
Only merge objects for principals
rix0rrr Feb 25, 2022
68d3940
Add test for condition
rix0rrr Feb 25, 2022
243f9c0
Update snapshots
rix0rrr Feb 25, 2022
1f40baa
Also merge Deny statements, add Alloy model
rix0rrr Feb 26, 2022
ef260c2
Revert "Update snapshots"
rix0rrr Feb 26, 2022
8b896d5
Add reference to Alloy model in source
rix0rrr Feb 26, 2022
281df8b
Correct docstring
rix0rrr Feb 26, 2022
a00582a
Don't unnecessarily update expectations
rix0rrr Feb 26, 2022
aeb902f
Update packages/@aws-cdk/aws-iam/lib/policy-document.ts
rix0rrr Feb 26, 2022
adf50cb
Add review comments
rix0rrr Feb 26, 2022
ccb192b
Redid merging logic
rix0rrr Feb 27, 2022
79f17d1
Update Alloy comment
rix0rrr Feb 27, 2022
09cdce1
Not subsets, equal sets!
rix0rrr Feb 27, 2022
02274c3
Update snapshots
rix0rrr Mar 2, 2022
ef354b4
Update model some more
rix0rrr Mar 4, 2022
00d0266
Merge branch 'huijbers/minimize-policies' of github.com:aws/aws-cdk i…
rix0rrr Mar 4, 2022
cc54755
Review comments
rix0rrr Mar 8, 2022
e4c3a2a
Update packages/@aws-cdk/aws-iam/lib/private/postprocess-policy-docum…
rix0rrr Mar 9, 2022
8fbee24
Remove unused helpers
rix0rrr Mar 9, 2022
625b2a7
Merge remote-tracking branch 'origin/master' into huijbers/minimize-p…
rix0rrr Mar 9, 2022
8875096
Update packages/@aws-cdk/aws-iam/lib/private/postprocess-policy-docum…
rix0rrr Mar 9, 2022
3e570f0
Do better comparisons
rix0rrr Mar 10, 2022
fe4be84
Merge branch 'huijbers/minimize-policies' of github.com:aws/aws-cdk i…
rix0rrr Mar 10, 2022
812d4fb
Some implementation details
rix0rrr Mar 10, 2022
fac4952
Update snapshots
rix0rrr Mar 10, 2022
bdb113f
More snapshot updates
rix0rrr Mar 10, 2022
1f0b3c4
Merge branch 'master' into huijbers/minimize-policies
rix0rrr Mar 10, 2022
4988a90
Silly copy/paste bug 😓
rix0rrr Mar 14, 2022
2d2f16b
Reformat, find another weakening
rix0rrr Mar 14, 2022
e1f701c
Merge remote-tracking branch 'origin/master' into huijbers/minimize-p…
rix0rrr Mar 14, 2022
8eb3cfa
Also handle untyped principals, review comments
rix0rrr Mar 17, 2022
80d7a07
Merge branch 'huijbers/minimize-policies' of github.com:aws/aws-cdk i…
rix0rrr Mar 17, 2022
793da77
Merge branch 'master' into huijbers/minimize-policies
rix0rrr Mar 17, 2022
4fb02ef
Update new integ
rix0rrr Mar 18, 2022
37eaa56
Merge branch 'master' into huijbers/minimize-policies
mergify[bot] Mar 18, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 35 additions & 3 deletions packages/@aws-cdk/aws-iam/lib/policy-document.ts
@@ -1,5 +1,7 @@
import * as cdk from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { PolicyStatement } from './policy-statement';
import { mergeStatements } from './private/merge-statements';

/**
* Properties for a new PolicyDocument
Expand All @@ -18,6 +20,23 @@ export interface PolicyDocumentProps {
* @default - No statements
*/
readonly statements?: PolicyStatement[];

/**
* Try to minimize the policy by merging statements
*
* To avoid overrunning the maximum policy size, combine statements if they produce
* the same result. Merging happens according to the following rules:
*
* - The Effect of both statements is 'Allow' ('Deny' statements cannot be merged)
* - Neither of the statements have a 'SId'
* - Combine Principals if the rest of the statement is exactly the same.
* - Combine Resources if the rest of the statement is exactly the same.
* - Combine Actions if the rest of the statement is exactly the same.
* - We will never combine NotPrincipals, NotResources or NotActions.
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
*
* @default false, unless the feature flag `@aws-cdk/aws-iam:minimizePolicies` is set
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly minimize?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this property? Is the feature flag not enough?

I would get rid of it for now. We can always come back later, and add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this better on principle. The boolean toggles the feature, and the default value for the boolean is taken from the flag for cases where you don't control the Policy initialization.

}

/**
Expand All @@ -43,16 +62,21 @@ export class PolicyDocument implements cdk.IResolvable {
public readonly creationStack: string[];
private readonly statements = new Array<PolicyStatement>();
private readonly autoAssignSids: boolean;
private readonly minimize?: boolean;

constructor(props: PolicyDocumentProps = {}) {
this.creationStack = cdk.captureStackTrace();
this.autoAssignSids = !!props.assignSids;
this.minimize = props.minimize;

this.addStatements(...props.statements || []);
}

public resolve(context: cdk.IResolveContext): any {
context.registerPostProcessor(new RemoveDuplicateStatements(this.autoAssignSids));
context.registerPostProcessor(new ReducePolicyDocument(
this.autoAssignSids,
this.minimize ?? cdk.FeatureFlags.of(context.scope).isEnabled(cxapi.IAM_MINIMIZE_POLICIES) ?? false,
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
));
return this.render();
}

Expand Down Expand Up @@ -157,15 +181,23 @@ export class PolicyDocument implements cdk.IResolvable {
/**
* Removes duplicate statements and assign Sids if necessary
*/
class RemoveDuplicateStatements implements cdk.IPostProcessor {
constructor(private readonly autoAssignSids: boolean) {
class ReducePolicyDocument implements cdk.IPostProcessor {
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
constructor(private readonly autoAssignSids: boolean, private readonly minimize: boolean) {
}

public postProcess(input: any, _context: cdk.IResolveContext): any {
if (!input || !input.Statement) {
return input;
}

if (this.minimize) {
input.Statement = mergeStatements(input.Statement);
}

// Also remove full-on duplicates (this will not be necessary if
// we minimized, but it might still dedupe statements we didn't
// minimize like 'Deny' statements, and definitely is still necessary
// if we didn't minimize)
const jsonStatements = new Set<string>();
const uniqueStatements: any[] = [];

Expand Down
176 changes: 176 additions & 0 deletions packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts
@@ -0,0 +1,176 @@
const LENGTH_CACHE_SYM = Symbol();
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved

type IamValue = unknown | unknown[];

/**
* Only the parts of the policy schema we're interested in
*/
interface StatementSchema {
Chriscbr marked this conversation as resolved.
Show resolved Hide resolved
readonly Sid?: string;
readonly Effect?: string;
readonly Principal?: IamValue | Record<string, IamValue>;
readonly Resource?: IamValue;
readonly Action?: IamValue;
}

/**
* Merge as many statements as possible to shrink the total policy doc, modifying the input array in place
*/
export function mergeStatements(statements: StatementSchema[]): StatementSchema[] {
let merges = findMerges(statements);
while (merges.length > 0) {
merges.sort((a, b) => a.sizeDelta - b.sizeDelta); // Most negative number first
if (merges[0].sizeDelta >= 0) { break; } // Nothing more to be gained

// Apply all merges, order biggest to smallest gain
while (merges.length > 0 && merges[0].sizeDelta < 0) { // Only while we're not adding size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need the sizeDelta check twice? Wouldn't we just not enter the while here at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but we may go around the while more than once as well.

const merge = merges.shift()!;
statements[merge.i0] = merge.combined;
statements.splice(merge.i1, 1);

// This invalidates all merges involving i0 or i1, and adjusts all indices > i1
const invalidIndices = [merge.i0, merge.i1];
let j = 0;
while (j < merges.length) {
if (invalidIndices.includes(merges[j].i0) || invalidIndices.includes(merges[j].i1)) {
merges.splice(j, 1);
} else {
if (merges[j].i0 > merge.i1) { merges[j].i0 -= 1; }
if (merges[j].i1 > merge.i1) { merges[j].i1 -= 1; }
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
j++;
}
}
}

merges = findMerges(statements);
}
return statements;
}

function findMerges(statements: StatementSchema[]): StatementMerge[] {
const ret = new Array<StatementMerge>();
for (let i0 = 0; i0 < statements.length; i0++) {
for (let i1 = i0 + 1; i1 < statements.length; i1++) {
tryMerge(statements, i0, i1, ret);
}
}
return ret;
}

function tryMerge(statements: StatementSchema[], i0: number, i1: number, into: StatementMerge[]) {
const a = statements[i0];
const b = statements[i1];
if (a.Effect !== 'Allow' || b.Effect !== 'Allow') { return; }
if (a.Sid || b.Sid) { return; }
Chriscbr marked this conversation as resolved.
Show resolved Hide resolved

const beforeLen = jsonLength(a) + jsonLength(b);

tryMerging('Resource');
tryMerging('Action');
tryMerging('Principal');
Chriscbr marked this conversation as resolved.
Show resolved Hide resolved

function tryMerging<A extends keyof StatementSchema>(key: A) {
if (!deepEqual(a, b, [key])) { return; }

const combined = {
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
...a,
[key]: mergeValues(a[key], b[key]),
};

into.push({
i0,
i1,
combined,
sizeDelta: jsonLength(combined) - beforeLen,
});
}
}

/**
* Return the length of a JSON representation of the given object, cached on the object
*/
function jsonLength<A extends StatementSchema>(x: A): number {
if ((x as any)[LENGTH_CACHE_SYM]) {
return (x as any)[LENGTH_CACHE_SYM];
}

const length = JSON.stringify(x).length;
Object.defineProperty(x, LENGTH_CACHE_SYM, {
value: length,
enumerable: false,
});
return length;
}

function deepEqual(a: any, b: any, ignoreKeys: string[]): boolean {
// Short-circuit same object identity as well
if (a === b) { return true; }
Chriscbr marked this conversation as resolved.
Show resolved Hide resolved

if (Array.isArray(a) || Array.isArray(b)) {
if (!Array.isArray(a) || !Array.isArray(b) || a.length !== b.length) { return false; }
return a.every((x, i) => deepEqual(x, b[i], []));
}

if (typeof a === 'object' || typeof b === 'object') {
if (typeof a !== 'object' || typeof b !== 'object') { return false; }

const keysA = new Set(Object.keys(a));
const keysB = new Set(Object.keys(b));
for (const k of ignoreKeys) {
keysA.delete(k);
keysB.delete(k);
}

for (const k of keysA) {
if (!deepEqual(a[k], b[k], [])) { return false; }
keysB.delete(k);
}

return keysB.size === 0;
}

return false;
}

function mergeValues(a: IamValue, b: IamValue): any {
if (!a && !b) { return a; }
if (Array.isArray(a) && Array.isArray(b)) { return normalizedArray(...a, ...b); }
if (Array.isArray(a) && typeof b === 'string') { return normalizedArray(...a, b); }
if (Array.isArray(b) && typeof a === 'string') { return normalizedArray(a, ...b); }
if (typeof a === 'string' && typeof b === 'string') { return normalizedArray(a, b); }
if (typeof a === 'object' && typeof b === 'object' && b != null) {
const ret: any = { ...a };
for (const [k, v] of Object.entries(b)) {
ret[k] = ret[k] ? mergeValues(ret[k], v) : v;
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
}
return ret;
}

throw new Error(`Don't know how to merge ${JSON.stringify(a)} and ${JSON.stringify(b)}`);
}

function normalizedArray(...xs: unknown[]) {
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
return Array.from(new Set(xs)).sort();
}
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved

interface StatementMerge {
/**
* Index of statement 0
*/
i0: number;

/**
* Index of statement 1
*/
i1: number;

/**
* The result of combining them
*/
readonly combined: StatementSchema;

/**
* How many bytes the new schema is bigger than the old one
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly sizeDelta: number;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to say, I don't understand the algorithm well enough to comment on its correctness 😛. Maybe more comments would help, but I'm not sure 😛.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not that complicated. I'll add comments.

1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-iam/package.json
Expand Up @@ -92,6 +92,7 @@
},
"dependencies": {
"@aws-cdk/core": "0.0.0",
"@aws-cdk/cx-api": "0.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also be added to peerDependencies?

Copy link
Contributor Author

@rix0rrr rix0rrr Feb 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't mean anything to do that.

"@aws-cdk/region-info": "0.0.0",
"constructs": "^3.3.69"
},
Expand Down