From f8208e90c69425a3957a341f73af273ba7cd1a92 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 23 Feb 2022 16:32:08 +0100 Subject: [PATCH] fix(iam): IAM Policies are too large to deploy The policies we generate sometimes have a lot of duplication between statements. This duplication can lead to the policy going over the size limit an IAM policy (either 2k, 6k or 10k bytes, depending on the resource type). This change combines multiple statements together, as long as it doesn't change the meaning of the final policy. Because doing so for all existing stacks will probably provoke minor heart attacks in operators everywhere, the new behavior is gated behind a feature flag. It can be retroactively switched on by people currently being bit by the size issues: ``` @aws-cdk/aws-iam:minimizePolicies ``` Fixes #18774, fixes #16350, fixes #18457. --- .../@aws-cdk/aws-iam/lib/policy-document.ts | 40 +++- .../aws-iam/lib/private/merge-statements.ts | 176 ++++++++++++++++++ packages/@aws-cdk/aws-iam/package.json | 1 + packages/@aws-cdk/core/lib/feature-flags.ts | 6 +- packages/@aws-cdk/cx-api/lib/features.ts | 10 + 5 files changed, 227 insertions(+), 6 deletions(-) create mode 100644 packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts diff --git a/packages/@aws-cdk/aws-iam/lib/policy-document.ts b/packages/@aws-cdk/aws-iam/lib/policy-document.ts index da43cce541158..561972b66e217 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-document.ts +++ b/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 @@ -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. + * + * @default false, unless the feature flag `@aws-cdk/aws-iam:minimizePolicies` is set + */ + readonly minimize?: boolean; } /** @@ -43,16 +62,21 @@ export class PolicyDocument implements cdk.IResolvable { public readonly creationStack: string[]; private readonly statements = new Array(); 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, + )); return this.render(); } @@ -157,8 +181,8 @@ 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 { + constructor(private readonly autoAssignSids: boolean, private readonly minimize: boolean) { } public postProcess(input: any, _context: cdk.IResolveContext): any { @@ -166,6 +190,16 @@ class RemoveDuplicateStatements implements cdk.IPostProcessor { return input; } + if (this.minimize) { + // New behavior: merge statements, this will naturally + // get rid of duplicates. + return { + ...input, + Statement: mergeStatements(input.Statement), + }; + } + + // Old behavior: just remove full-on duplicates const jsonStatements = new Set(); const uniqueStatements: any[] = []; diff --git a/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts b/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts new file mode 100644 index 0000000000000..7453651355174 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts @@ -0,0 +1,176 @@ +const LENGTH_CACHE_SYM = Symbol(); + +type IamValue = unknown | unknown[]; + +/** + * Only the parts of the policy schema we're interested in + */ +interface StatementSchema { + readonly Sid?: string; + readonly Effect?: string; + readonly Principal?: IamValue | Record; + 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 + 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; } + j++; + } + } + } + + merges = findMerges(statements); + } + return statements; +} + +function findMerges(statements: StatementSchema[]): StatementMerge[] { + const ret = new Array(); + 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; } + + const beforeLen = jsonLength(a) + jsonLength(b); + + tryMerging('Resource'); + tryMerging('Action'); + tryMerging('Principal'); + + function tryMerging(key: A) { + if (!deepEqual(a, b, [key])) { return; } + + const combined = { + ...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(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; } + + 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; + } + return ret; + } + + throw new Error(`Don't know how to merge ${JSON.stringify(a)} and ${JSON.stringify(b)}`); +} + +function normalizedArray(...xs: unknown[]) { + return Array.from(new Set(xs)).sort(); +} + +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 + */ + readonly sizeDelta: number; +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-iam/package.json b/packages/@aws-cdk/aws-iam/package.json index 7172a6e453245..96ed3c82b8fc4 100644 --- a/packages/@aws-cdk/aws-iam/package.json +++ b/packages/@aws-cdk/aws-iam/package.json @@ -92,6 +92,7 @@ }, "dependencies": { "@aws-cdk/core": "0.0.0", + "@aws-cdk/cx-api": "0.0.0", "@aws-cdk/region-info": "0.0.0", "constructs": "^3.3.69" }, diff --git a/packages/@aws-cdk/core/lib/feature-flags.ts b/packages/@aws-cdk/core/lib/feature-flags.ts index 926a60168732f..44fd5e138bdc7 100644 --- a/packages/@aws-cdk/core/lib/feature-flags.ts +++ b/packages/@aws-cdk/core/lib/feature-flags.ts @@ -1,5 +1,5 @@ import * as cxapi from '@aws-cdk/cx-api'; -import { Construct } from '../lib/construct-compat'; +import { IConstruct } from '../lib/construct-compat'; /** * Features that are implemented behind a flag in order to preserve backwards @@ -12,11 +12,11 @@ export class FeatureFlags { /** * Inspect feature flags on the construct node's context. */ - public static of(scope: Construct) { + public static of(scope: IConstruct) { return new FeatureFlags(scope); } - private constructor(private readonly construct: Construct) {} + private constructor(private readonly construct: IConstruct) {} /** * Check whether a feature flag is enabled. If configured, the flag is present in diff --git a/packages/@aws-cdk/cx-api/lib/features.ts b/packages/@aws-cdk/cx-api/lib/features.ts index 9278717ef1c15..9b5dc4e872892 100644 --- a/packages/@aws-cdk/cx-api/lib/features.ts +++ b/packages/@aws-cdk/cx-api/lib/features.ts @@ -215,6 +215,15 @@ export const ECS_SERVICE_EXTENSIONS_ENABLE_DEFAULT_LOG_DRIVER = '@aws-cdk-contai */ export const EC2_UNIQUE_IMDSV2_LAUNCH_TEMPLATE_NAME = '@aws-cdk/aws-ec2:uniqueImdsv2TemplateName'; +/** + * Minimize IAM policies by combining Principals, Actions and Resources of two + * Statements in the policies, as long as it doesn't change the meaning of the + * policy. + * + * [PERMANENT] + */ +export const IAM_MINIMIZE_POLICIES = '@aws-cdk/aws-iam:minimizePolicies'; + /** * Flag values that should apply for new projects * @@ -240,6 +249,7 @@ export const FUTURE_FLAGS: { [key: string]: boolean } = { [CLOUDFRONT_DEFAULT_SECURITY_POLICY_TLS_V1_2_2021]: true, [ECS_SERVICE_EXTENSIONS_ENABLE_DEFAULT_LOG_DRIVER]: true, [EC2_UNIQUE_IMDSV2_LAUNCH_TEMPLATE_NAME]: true, + [IAM_MINIMIZE_POLICIES]: true, }; /**