diff --git a/packages/@aws-cdk/aws-iam/lib/policy-document.ts b/packages/@aws-cdk/aws-iam/lib/policy-document.ts index 9d73acb4693ac..b46286be72f7c 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-document.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy-document.ts @@ -1,6 +1,8 @@ import * as cdk from '@aws-cdk/core'; import * as cxapi from '@aws-cdk/cx-api'; +import { IConstruct } from 'constructs'; import { PolicyStatement } from './policy-statement'; +import { mergeStatements } from './private/merge-statements'; import { PostProcessPolicyDocument } from './private/postprocess-policy-document'; /** @@ -64,6 +66,7 @@ export class PolicyDocument implements cdk.IResolvable { private readonly statements = new Array(); private readonly autoAssignSids: boolean; private readonly minimize?: boolean; + private _minimized = false; constructor(props: PolicyDocumentProps = {}) { this.creationStack = cdk.captureStackTrace(); @@ -74,10 +77,8 @@ export class PolicyDocument implements cdk.IResolvable { } public resolve(context: cdk.IResolveContext): any { - context.registerPostProcessor(new PostProcessPolicyDocument( - this.autoAssignSids, - this.minimize ?? cdk.FeatureFlags.of(context.scope).isEnabled(cxapi.IAM_MINIMIZE_POLICIES) ?? false, - )); + this._maybeMergeStatements(context.scope); + context.registerPostProcessor(new PostProcessPolicyDocument(this.autoAssignSids)); return this.render(); } @@ -165,6 +166,98 @@ export class PolicyDocument implements cdk.IResolvable { return errors; } + /** + * Perform statement merging (if enabled and not done yet) + * + * Returns a mapping of merged statements to original statements on the first invocation, + * `undefined` on subsequent invocations. + * + * @internal + */ + public _maybeMergeStatements(scope: IConstruct): Map | undefined { + const minimize = this.minimize ?? cdk.FeatureFlags.of(scope).isEnabled(cxapi.IAM_MINIMIZE_POLICIES) ?? false; + if (minimize) { + if (this._minimized) { + return undefined; + } + const result = mergeStatements(this.statements); + this.statements.splice(0, this.statements.length, ...result.mergedStatements); + this._minimized = true; + return result.originsMap; + } + return new Map(this.statements.map(s => [s, [s]])); + } + + /** + * Split the statements of the PolicyDocument into multiple groups, limited by their size + * + * Returns the policy documents created to hold statements that were split off. + * + * @internal + */ + public _splitDocument(selfMaximumSize: number, splitMaximumSize: number): PolicyDocument[] { + const self = this; + const newDocs: PolicyDocument[] = []; + + // Cache statement sizes to avoid recomputing them based on the fields + const statementSizes = new Map(this.statements.map(s => [s, s._estimateSize()])); + + // Keep some size counters so we can avoid recomputing them based on the statements in each + let selfSize = 0; + const polSizes = new Map(); + // Getter with a default to save some syntactic noise + const polSize = (x: PolicyDocument) => polSizes.get(x) ?? 0; + + let i = 0; + while (i < this.statements.length) { + const statement = this.statements[i]; + + const statementSize = statementSizes.get(statement) ?? 0; + if (selfSize + statementSize < selfMaximumSize) { + // Fits in self + selfSize += statementSize; + i++; + continue; + } + + // Split off to new PolicyDocument. Find the PolicyDocument we can add this to, + // or add a fresh one. + const addToDoc = findDocWithSpace(statementSize); + addToDoc.addStatements(statement); + polSizes.set(addToDoc, polSize(addToDoc) + statementSize); + this.statements.splice(i, 1); + } + + return newDocs; + + function findDocWithSpace(size: number) { + let j = 0; + while (j < newDocs.length && polSize(newDocs[j]) + size > splitMaximumSize) { + j++; + } + if (j < newDocs.length) { + return newDocs[j]; + } + + const newDoc = new PolicyDocument({ + assignSids: self.autoAssignSids, + // Minimizing has already been done + minimize: false, + }); + newDocs.push(newDoc); + return newDoc; + } + } + + /** + * The statements in this doc + * + * @internal + */ + public _statements() { + return [...this.statements]; + } + private render(): any { if (this.isEmpty) { return undefined; diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts index 80ff191613e0e..210bcab227aae 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts @@ -5,7 +5,7 @@ import { FederatedPrincipal, IPrincipal, PrincipalBase, PrincipalPolicyFragment, ServicePrincipal, ServicePrincipalOpts, } from './principals'; import { normalizeStatement } from './private/postprocess-policy-document'; -import { LITERAL_STRING_KEY, mergePrincipal } from './util'; +import { LITERAL_STRING_KEY, mergePrincipal, sum } from './util'; const ensureArrayOrUndefined = (field: any) => { if (field === undefined) { @@ -56,22 +56,24 @@ export class PolicyStatement { * Statement ID for this statement */ public sid?: string; + /** * Whether to allow or deny the actions in this statement */ public effect: Effect; - private readonly action = new Array(); - private readonly notAction = new Array(); - private readonly principal: { [key: string]: any[] } = {}; - private readonly notPrincipal: { [key: string]: any[] } = {}; - private readonly resource = new Array(); - private readonly notResource = new Array(); - private readonly condition: { [key: string]: any } = { }; + private readonly _action = new Array(); + private readonly _notAction = new Array(); + private readonly _principal: { [key: string]: any[] } = {}; + private readonly _notPrincipal: { [key: string]: any[] } = {}; + private readonly _resource = new Array(); + private readonly _notResource = new Array(); + private readonly _condition: { [key: string]: any } = { }; private principalConditionsJson?: string; // Hold on to those principals private readonly _principals = new Array(); + private readonly _notPrincipals = new Array(); constructor(props: PolicyStatementProps = {}) { // Validate actions @@ -108,10 +110,10 @@ export class PolicyStatement { * @param actions actions that will be allowed. */ public addActions(...actions: string[]) { - if (actions.length > 0 && this.notAction.length > 0) { + if (actions.length > 0 && this._notAction.length > 0) { throw new Error('Cannot add \'Actions\' to policy statement if \'NotActions\' have been added'); } - this.action.push(...actions); + this._action.push(...actions); } /** @@ -123,10 +125,10 @@ export class PolicyStatement { * @param notActions actions that will be denied. All other actions will be permitted. */ public addNotActions(...notActions: string[]) { - if (notActions.length > 0 && this.action.length > 0) { + if (notActions.length > 0 && this._action.length > 0) { throw new Error('Cannot add \'NotActions\' to policy statement if \'Actions\' have been added'); } - this.notAction.push(...notActions); + this._notAction.push(...notActions); } // @@ -137,7 +139,7 @@ export class PolicyStatement { * Indicates if this permission has a "Principal" section. */ public get hasPrincipal() { - return Object.keys(this.principal).length > 0 || Object.keys(this.notPrincipal).length > 0; + return this._principals.length + this._notPrincipals.length > 0; } /** @@ -149,26 +151,17 @@ export class PolicyStatement { */ public addPrincipals(...principals: IPrincipal[]) { this._principals.push(...principals); - if (Object.keys(principals).length > 0 && Object.keys(this.notPrincipal).length > 0) { + if (Object.keys(principals).length > 0 && Object.keys(this._notPrincipal).length > 0) { throw new Error('Cannot add \'Principals\' to policy statement if \'NotPrincipals\' have been added'); } for (const principal of principals) { this.validatePolicyPrincipal(principal); const fragment = principal.policyFragment; - mergePrincipal(this.principal, fragment.principalJson); + mergePrincipal(this._principal, fragment.principalJson); this.addPrincipalConditions(fragment.conditions); } } - /** - * Expose principals to allow their ARNs to be replaced by account ID strings - * in policy statements for resources policies that don't allow full account ARNs, - * such as AWS::Logs::ResourcePolicy. - */ - public get principals(): IPrincipal[] { - return [...this._principals]; - } - /** * Specify principals that is not allowed or denied access to the "NotPrincipal" section of * a policy statement. @@ -178,13 +171,14 @@ export class PolicyStatement { * @param notPrincipals IAM principals that will be denied access */ public addNotPrincipals(...notPrincipals: IPrincipal[]) { - if (Object.keys(notPrincipals).length > 0 && Object.keys(this.principal).length > 0) { + this._notPrincipals.push(...notPrincipals); + if (Object.keys(notPrincipals).length > 0 && Object.keys(this._principal).length > 0) { throw new Error('Cannot add \'NotPrincipals\' to policy statement if \'Principals\' have been added'); } for (const notPrincipal of notPrincipals) { this.validatePolicyPrincipal(notPrincipal); const fragment = notPrincipal.policyFragment; - mergePrincipal(this.notPrincipal, fragment.principalJson); + mergePrincipal(this._notPrincipal, fragment.principalJson); this.addPrincipalConditions(fragment.conditions); } } @@ -269,10 +263,10 @@ export class PolicyStatement { * @param arns Amazon Resource Names (ARNs) of the resources that this policy statement applies to */ public addResources(...arns: string[]) { - if (arns.length > 0 && this.notResource.length > 0) { + if (arns.length > 0 && this._notResource.length > 0) { throw new Error('Cannot add \'Resources\' to policy statement if \'NotResources\' have been added'); } - this.resource.push(...arns); + this._resource.push(...arns); } /** @@ -284,10 +278,10 @@ export class PolicyStatement { * @param arns Amazon Resource Names (ARNs) of the resources that this policy statement does not apply to */ public addNotResources(...arns: string[]) { - if (arns.length > 0 && this.resource.length > 0) { + if (arns.length > 0 && this._resource.length > 0) { throw new Error('Cannot add \'NotResources\' to policy statement if \'Resources\' have been added'); } - this.notResource.push(...arns); + this._notResource.push(...arns); } /** @@ -301,7 +295,7 @@ export class PolicyStatement { * Indicates if this permission has at least one resource associated with it. */ public get hasResource() { - return this.resource && this.resource.length > 0; + return this._resource && this._resource.length > 0; } // @@ -312,8 +306,8 @@ export class PolicyStatement { * Add a condition to the Policy */ public addCondition(key: string, value: Condition) { - const existingValue = this.condition[key]; - this.condition[key] = existingValue ? { ...existingValue, ...value } : value; + const existingValue = this._condition[key]; + this._condition[key] = existingValue ? { ...existingValue, ...value } : value; } /** @@ -340,14 +334,16 @@ export class PolicyStatement { return new PolicyStatement({ sid: overrides.sid ?? this.sid, effect: overrides.effect ?? this.effect, - actions: overrides.actions ?? this.action, - notActions: overrides.notActions ?? this.notAction, + actions: overrides.actions ?? this.actions, + notActions: overrides.notActions ?? this.notActions, - principals: overrides.principals, - notPrincipals: overrides.notPrincipals, + principals: overrides.principals ?? this.principals, + notPrincipals: overrides.notPrincipals ?? this.notPrincipals, - resources: overrides.resources ?? this.resource, - notResources: overrides.notResources ?? this.notResource, + resources: overrides.resources ?? this.resources, + notResources: overrides.notResources ?? this.notResources, + + conditions: this.conditions, }); } @@ -358,14 +354,14 @@ export class PolicyStatement { */ public toStatementJson(): any { return normalizeStatement({ - Action: this.action, - NotAction: this.notAction, - Condition: this.condition, + Action: this._action, + NotAction: this._notAction, + Condition: this._condition, Effect: this.effect, - Principal: this.principal, - NotPrincipal: this.notPrincipal, - Resource: this.resource, - NotResource: this.notResource, + Principal: this._principal, + NotPrincipal: this._notPrincipal, + Resource: this._resource, + NotResource: this._notResource, Sid: this.sid, }); } @@ -420,7 +416,7 @@ export class PolicyStatement { */ public validateForAnyPolicy(): string[] { const errors = new Array(); - if (this.action.length === 0 && this.notAction.length === 0) { + if (this._action.length === 0 && this._notAction.length === 0) { errors.push('A PolicyStatement must specify at least one \'action\' or \'notAction\'.'); } return errors; @@ -431,7 +427,7 @@ export class PolicyStatement { */ public validateForResourcePolicy(): string[] { const errors = this.validateForAnyPolicy(); - if (Object.keys(this.principal).length === 0 && Object.keys(this.notPrincipal).length === 0) { + if (Object.keys(this._principal).length === 0 && Object.keys(this._notPrincipal).length === 0) { errors.push('A PolicyStatement used in a resource-based policy must specify at least one IAM principal.'); } return errors; @@ -442,14 +438,98 @@ export class PolicyStatement { */ public validateForIdentityPolicy(): string[] { const errors = this.validateForAnyPolicy(); - if (Object.keys(this.principal).length > 0 || Object.keys(this.notPrincipal).length > 0) { + if (Object.keys(this._principal).length > 0 || Object.keys(this._notPrincipal).length > 0) { errors.push('A PolicyStatement used in an identity-based policy cannot specify any IAM principals.'); } - if (Object.keys(this.resource).length === 0 && Object.keys(this.notResource).length === 0) { + if (Object.keys(this._resource).length === 0 && Object.keys(this._notResource).length === 0) { errors.push('A PolicyStatement used in an identity-based policy must specify at least one resource.'); } return errors; } + + /** + * The Actions added to this statement + */ + public get actions() { + return [...this._action]; + } + + /** + * The NotActions added to this statement + */ + public get notActions() { + return [...this._notAction]; + } + + /** + * The Principals added to this statement + */ + public get principals(): IPrincipal[] { + return [...this._principals]; + } + + /** + * The NotPrincipals added to this statement + */ + public get notPrincipals(): IPrincipal[] { + return [...this._notPrincipals]; + } + + /** + * The Resources added to this statement + */ + public get resources() { + return [...this._resource]; + } + + /** + * The NotResources added to this statement + */ + public get notResources() { + return [...this._notResource]; + } + + /** + * The conditions added to this statement + */ + public get conditions(): any { + return { ...this._condition }; + } + + /** + * Estimate the size of this policy statement + * + * By necessity, this will not be accurate. We'll do our best to overestimate + * so we won't have nasty surprises. + * + * @internal + */ + public _estimateSize(): number { + let ret = 0; + + const actionEstimate = 20; + const arnEstimate = 120; // A safe (over)estimate on how long ARNs typically are + + ret += `"Effect": "${this.effect}",`.length; + + count('Action', this.actions, actionEstimate); + count('NotAction', this.notActions, actionEstimate); + count('Resource', this.resources, arnEstimate); + count('NotResource', this.notResources, arnEstimate); + + ret += this.principals.length * arnEstimate; + ret += this.notPrincipals.length * arnEstimate; + + ret += JSON.stringify(this.conditions).length; + return ret; + + function count(key: string, values: string[], tokenSize: number) { + if (values.length > 0) { + ret += key.length + 5 /* quotes, colon, brackets */ + + sum(values.map(v => (cdk.Token.isUnresolved(v) ? tokenSize : v.length) + 3 /* quotes, separator */)); + } + } + } } /** diff --git a/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts b/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts index c79ecd6a8a814..17a33f5c10788 100644 --- a/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts +++ b/packages/@aws-cdk/aws-iam/lib/private/merge-statements.ts @@ -4,8 +4,18 @@ // implemented here. +import { PolicyStatement } from '../policy-statement'; +import { IPrincipal } from '../principals'; import { LITERAL_STRING_KEY } from '../util'; -import { StatementSchema, normalizeStatement, IamValue } from './postprocess-policy-document'; + + +/* + * Don't produce any merged statements larger than this. + * + * They will become impossible to divide across managed policies if we do, + * and this is the maximum size for User policies. + */ +const MAX_MERGE_SIZE = 2000; /** * Merge as many statements as possible to shrink the total policy doc, modifying the input array in place @@ -15,12 +25,21 @@ import { StatementSchema, normalizeStatement, IamValue } from './postprocess-pol * Good Enough(tm). If it merges anything, it's at least going to produce a smaller output * than the input. */ -export function mergeStatements(statements: StatementSchema[]): StatementSchema[] { +export function mergeStatements(statements: PolicyStatement[]): MergeStatementResult { const compStatements = statements.map(makeComparable); // Keep trying until nothing changes anymore while (onePass()) { /* again */ } - return compStatements.map(renderComparable); + + const mergedStatements = new Array(); + const originsMap = new Map(); + for (const comp of compStatements) { + const statement = renderComparable(comp); + mergedStatements.push(statement); + originsMap.set(statement, comp.originals); + } + + return { mergedStatements, originsMap }; // Do one optimization pass, return 'true' if we merged anything function onePass() { @@ -47,6 +66,18 @@ export function mergeStatements(statements: StatementSchema[]): StatementSchema[ } } +export interface MergeStatementResult { + /** + * The list of maximally merged statements + */ + readonly mergedStatements: PolicyStatement[]; + + /** + * Mapping of old to new statements + */ + readonly originsMap: Map; +} + /** * Given two statements, return their merging (if possible) * @@ -61,32 +92,40 @@ export function mergeStatements(statements: StatementSchema[]): StatementSchema[ */ function tryMerge(a: ComparableStatement, b: ComparableStatement): ComparableStatement | undefined { // Effects must be the same - if (a.effect !== b.effect) { return; } + if (a.statement.effect !== b.statement.effect) { return; } // We don't merge Sids (for now) - if (a.sid || b.sid) { return; } + if (a.statement.sid || b.statement.sid) { return; } + // Not if the combination grows too large + if (a.sizeEstimate + b.sizeEstimate > MAX_MERGE_SIZE) { return; } if (a.conditionString !== b.conditionString) { return; } - if (!setEqual(a.notAction, b.notAction) || !setEqual(a.notResource, b.notResource) || !setEqual(a.notPrincipal, b.notPrincipal)) { return; } + if ( + !setEqual(a.statement.notActions, b.statement.notActions) || + !setEqual(a.statement.notResources, b.statement.notResources) || + !setEqual(a.notPrincipalStrings, b.notPrincipalStrings) + ) { + return; + } // We can merge these statements if 2 out of the 3 sets of Action, Resource, Principal // are the same. - const setsEqual = (setEqual(a.action, b.action) ? 1 : 0) + - (setEqual(a.resource, b.resource) ? 1 : 0) + - (setEqual(a.principal, b.principal) ? 1 : 0); + const setsEqual = (setEqual(a.statement.actions, b.statement.actions) ? 1 : 0) + + (setEqual(a.statement.resources, b.statement.resources) ? 1 : 0) + + (setEqual(a.principalStrings, b.principalStrings) ? 1 : 0); if (setsEqual < 2 || unmergeablePrincipals(a, b)) { return; } return { - effect: a.effect, + originals: [...a.originals, ...b.originals], + statement: a.statement.copy({ + actions: setMerge(a.statement.actions, b.statement.actions), + resources: setMerge(a.statement.resources, b.statement.resources), + principals: setMerge(a.statement.principals, b.statement.principals), + }), + principalStrings: setMerge(a.principalStrings, b.principalStrings), + notPrincipalStrings: a.notPrincipalStrings, conditionString: a.conditionString, - conditionValue: b.conditionValue, - notAction: a.notAction, - notPrincipal: a.notPrincipal, - notResource: a.notResource, - - action: setMerge(a.action, b.action), - resource: setMerge(a.resource, b.resource), - principal: setMerge(a.principal, b.principal), + sizeEstimate: a.sizeEstimate + b.sizeEstimate, }; } @@ -95,42 +134,18 @@ function tryMerge(a: ComparableStatement, b: ComparableStatement): ComparableSta * * This is to be able to do comparisons on these sets quickly. */ -function makeComparable(s: StatementSchema): ComparableStatement { +function makeComparable(s: PolicyStatement): ComparableStatement { return { - effect: s.Effect, - sid: s.Sid, - action: iamSet(s.Action), - notAction: iamSet(s.NotAction), - resource: iamSet(s.Resource), - notResource: iamSet(s.NotResource), - principal: principalIamSet(s.Principal), - notPrincipal: principalIamSet(s.NotPrincipal), - conditionString: JSON.stringify(s.Condition), - conditionValue: s.Condition, + originals: [s], + statement: s, + conditionString: JSON.stringify(s.conditions), + principalStrings: s.principals.map(renderPrincipal), + notPrincipalStrings: s.notPrincipals.map(renderPrincipal), + sizeEstimate: s._estimateSize(), }; - function forceArray(x: A | Array): Array { - return Array.isArray(x) ? x : [x]; - } - - function iamSet(x: IamValue | undefined): IamValueSet { - if (x == undefined) { return {}; } - return mkdict(forceArray(x).map(e => [JSON.stringify(e), e])); - } - - function principalIamSet(x: IamValue | Record | undefined): IamValueSet { - if (x === undefined) { return {}; } - - if (Array.isArray(x) || typeof x === 'string') { - x = { [LITERAL_STRING_KEY]: x }; - } - - if (typeof x === 'object' && x !== null) { - // Turn { AWS: [a, b], Service: [c] } into [{ AWS: a }, { AWS: b }, { Service: c }] - const individualPrincipals = Object.entries(x).flatMap(([principalType, value]) => forceArray(value).map(v => ({ [principalType]: v }))); - return iamSet(individualPrincipals); - } - return {}; + function renderPrincipal(x: IPrincipal) { + return JSON.stringify(x.policyFragment.principalJson); } } @@ -144,106 +159,41 @@ function makeComparable(s: StatementSchema): ComparableStatement { * therefore be preserved. */ function unmergeablePrincipals(a: ComparableStatement, b: ComparableStatement) { - const aHasLiteral = Object.values(a.principal).some(v => LITERAL_STRING_KEY in v); - const bHasLiteral = Object.values(b.principal).some(v => LITERAL_STRING_KEY in v); + const aHasLiteral = a.statement.principals.some(v => LITERAL_STRING_KEY in v.policyFragment.principalJson); + const bHasLiteral = b.statement.principals.some(v => LITERAL_STRING_KEY in v.policyFragment.principalJson); return aHasLiteral !== bHasLiteral; } /** - * Turn a ComparableStatement back into a StatementSchema + * Turn a ComparableStatement back into a Statement */ -function renderComparable(s: ComparableStatement): StatementSchema { - return normalizeStatement({ - Effect: s.effect, - Sid: s.sid, - Condition: s.conditionValue, - Action: renderSet(s.action), - NotAction: renderSet(s.notAction), - Resource: renderSet(s.resource), - NotResource: renderSet(s.notResource), - Principal: renderPrincipalSet(s.principal), - NotPrincipal: renderPrincipalSet(s.notPrincipal), - }); - - function renderSet(x: IamValueSet): IamValue | undefined { - // Return as sorted array so that we normalize - const keys = Object.keys(x).sort(); - return keys.length > 0 ? keys.map(key => x[key]) : undefined; - } - - function renderPrincipalSet(x: IamValueSet): Record { - const keys = Object.keys(x).sort(); - // The first level will be an object - const ret: Record = {}; - for (const key of keys) { - const principal = x[key]; - if (principal == null || typeof principal !== 'object') { - throw new Error(`Principal should be an object with a principal type, got: ${principal}`); - } - const principalKeys = Object.keys(principal); - if (principalKeys.length !== 1) { - throw new Error(`Principal should be an object with 1 key, found keys: ${principalKeys}`); - } - const pk = principalKeys[0]; - if (!ret[pk]) { - ret[pk] = []; - } - (ret[pk] as IamValue[]).push(principal[pk]); - } - return ret; - } +function renderComparable(s: ComparableStatement): PolicyStatement { + return s.statement; } /** * An analyzed version of a statement that makes it easier to do comparisons and merging on - * - * We will stringify parts of the statement: comparisons are done on the strings, the original - * values are retained so we can stitch them back together into a real policy. */ interface ComparableStatement { - readonly effect?: string; - readonly sid?: string; - - readonly principal: IamValueSet; - readonly notPrincipal: IamValueSet; - readonly action: IamValueSet; - readonly notAction: IamValueSet; - readonly resource: IamValueSet; - readonly notResource: IamValueSet; - + readonly statement: PolicyStatement; + readonly originals: PolicyStatement[]; + readonly principalStrings: string[]; + readonly notPrincipalStrings: string[]; readonly conditionString: string; - readonly conditionValue: any; + readonly sizeEstimate: number; } -/** - * A collection of comparable IAM values - * - * Each value is indexed by its stringified value, mapping to its original value. - * This allows us to compare values quickly and easily (even if they are complex), - * while also being able to deduplicate the originals. - */ -type IamValueSet = Record; - /** * Whether the given sets are equal */ -function setEqual(a: IamValueSet, b: IamValueSet) { - const keysA = Object.keys(a); - const keysB = Object.keys(b); - return keysA.length === keysB.length && keysA.every(k => k in b); +function setEqual(a: A[], b: A[]) { + const bSet = new Set(b); + return a.length === b.length && a.every(k => bSet.has(k)); } /** * Merge two IAM value sets */ -function setMerge(x: IamValueSet, y: IamValueSet): IamValueSet { - return { ...x, ...y }; -} - -function mkdict(xs: Array<[string, A]>): Record { - const ret: Record = {}; - for (const x of xs) { - ret[x[0]] = x[1]; - } - return ret; -} +function setMerge(x: A[], y: A[]): A[] { + return Array.from(new Set([...x, ...y])).sort(); +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-iam/lib/private/postprocess-policy-document.ts b/packages/@aws-cdk/aws-iam/lib/private/postprocess-policy-document.ts index f54873aa7340c..05193e8467051 100644 --- a/packages/@aws-cdk/aws-iam/lib/private/postprocess-policy-document.ts +++ b/packages/@aws-cdk/aws-iam/lib/private/postprocess-policy-document.ts @@ -1,18 +1,17 @@ import * as cdk from '@aws-cdk/core'; import { LITERAL_STRING_KEY } from '../util'; -import { mergeStatements } from './merge-statements'; /** * A Token postprocesser for policy documents * - * Removes duplicate statements, merges statements, and assign Sids if necessary + * Removes duplicate statements, and assign Sids if necessary * * Because policy documents can contain all kinds of crazy things, * we do all the necessary work here after the document has been mostly resolved * into a predictable CloudFormation form. */ export class PostProcessPolicyDocument implements cdk.IPostProcessor { - constructor(private readonly autoAssignSids: boolean, private readonly minimize: boolean) { + constructor(private readonly autoAssignSids: boolean) { } public postProcess(input: any, _context: cdk.IResolveContext): any { @@ -20,10 +19,6 @@ export class PostProcessPolicyDocument implements cdk.IPostProcessor { 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 diff --git a/packages/@aws-cdk/aws-iam/lib/role.ts b/packages/@aws-cdk/aws-iam/lib/role.ts index 32c8ae6a140e7..4a84fb8600696 100644 --- a/packages/@aws-cdk/aws-iam/lib/role.ts +++ b/packages/@aws-cdk/aws-iam/lib/role.ts @@ -1,9 +1,9 @@ -import { ArnFormat, Duration, Resource, Stack, Token, TokenComparison } from '@aws-cdk/core'; +import { ArnFormat, IConstruct, Duration, Resource, Stack, Token, TokenComparison, Aspects, ConcreteDependable, Annotations } from '@aws-cdk/core'; import { Construct, Node } from 'constructs'; import { Grant } from './grant'; import { CfnRole } from './iam.generated'; import { IIdentity } from './identity-base'; -import { IManagedPolicy } from './managed-policy'; +import { IManagedPolicy, ManagedPolicy } from './managed-policy'; import { Policy } from './policy'; import { PolicyDocument } from './policy-document'; import { PolicyStatement } from './policy-statement'; @@ -13,6 +13,9 @@ import { ImmutableRole } from './private/immutable-role'; import { MutatingPolicyDocumentAdapter } from './private/policydoc-adapter'; import { AttachedPolicies, UniqueStringSet } from './util'; +const MAX_INLINE_SIZE = 10000; +const MAX_MANAGEDPOL_SIZE = 6000; + /** * Properties for defining an IAM Role */ @@ -338,7 +341,9 @@ export class Role extends Resource implements IRole { private readonly managedPolicies: IManagedPolicy[] = []; private readonly attachedPolicies = new AttachedPolicies(); private readonly inlinePolicies: { [name: string]: PolicyDocument }; + private readonly dependables = new Map(); private immutableRole?: IRole; + private _didSplit = false; constructor(scope: Construct, id: string, props: RoleProps) { super(scope, id, { @@ -394,6 +399,14 @@ export class Role extends Resource implements IRole { } return result; } + + Aspects.of(this).add({ + visit: (c) => { + if (c === this) { + this.splitLargePolicy(); + } + }, + }); } /** @@ -407,7 +420,13 @@ export class Role extends Resource implements IRole { this.attachInlinePolicy(this.defaultPolicy); } this.defaultPolicy.addStatements(statement); - return { statementAdded: true, policyDependable: this.defaultPolicy }; + + // We might split this statement off into a different policy, so we'll need to + // late-bind the dependable. + const policyDependable = new ConcreteDependable(); + this.dependables.set(statement, policyDependable); + + return { statementAdded: true, policyDependable }; } public addToPolicy(statement: PolicyStatement): boolean { @@ -484,6 +503,59 @@ export class Role extends Resource implements IRole { } return errors; } + + /** + * Split large inline policies into managed policies + * + * This gets around the 10k bytes limit on role policies. + */ + private splitLargePolicy() { + if (!this.defaultPolicy || this._didSplit) { + return; + } + this._didSplit = true; + + const self = this; + const splitDocument = this.defaultPolicy.document; + + const mergeMap = splitDocument._maybeMergeStatements(this); + if (mergeMap === undefined) { + throw new Error('Unexpected operation order: splitLargePolicy() called on already-merged policy document'); + } + const splitOffDocs = splitDocument._splitDocument(MAX_INLINE_SIZE, MAX_MANAGEDPOL_SIZE); + + const mpCount = this.managedPolicies.length + splitOffDocs.length; + if (mpCount > 20) { + Annotations.of(this).addWarning(`Policy too large: ${mpCount} exceeds the maximum of 20 managed policies attached to a Role`); + } else if (mpCount > 10) { + Annotations.of(this).addWarning(`Policy large: ${mpCount} exceeds 10 managed policies attached to a Role, this requires a quota increase`); + } + + // Create the managed policies and fix up the dependencies + markDeclaringConstruct(splitDocument, this.defaultPolicy); + + let i = 1; + for (const splitDoc of splitOffDocs) { + const mp = new ManagedPolicy(this, `OverflowPolicy${i++}`, { + description: `Part of the policies for ${this.node.path}`, + document: splitDoc, + roles: [this], + }); + markDeclaringConstruct(splitDoc, mp); + } + + /** + * Update the Dependables for the statements in the given PolicyDocument to point to the actual declaring construct + */ + function markDeclaringConstruct(doc: PolicyDocument, declaringConstruct: IConstruct) { + for (const statement of doc._statements()) { + const originalStatements = mergeMap!.get(statement) ?? []; + for (const original of originalStatements) { + self.dependables.get(original)?.add(declaringConstruct); + } + } + } + } } /** diff --git a/packages/@aws-cdk/aws-iam/lib/util.ts b/packages/@aws-cdk/aws-iam/lib/util.ts index 831f625a1fdcf..ae4825ffa0ee0 100644 --- a/packages/@aws-cdk/aws-iam/lib/util.ts +++ b/packages/@aws-cdk/aws-iam/lib/util.ts @@ -74,12 +74,15 @@ export class AttachedPolicies { */ export function mergePrincipal(target: { [key: string]: string[] }, source: { [key: string]: string[] }) { // If one represents a literal string, the other one must be empty - if ((LITERAL_STRING_KEY in source && !isEmptyObject(target)) || - (LITERAL_STRING_KEY in target && !isEmptyObject(source))) { + const sourceKeys = Object.keys(source); + const targetKeys = Object.keys(target); + + if ((LITERAL_STRING_KEY in source && targetKeys.some(k => k !== LITERAL_STRING_KEY)) || + (LITERAL_STRING_KEY in target && sourceKeys.some(k => k !== LITERAL_STRING_KEY))) { throw new Error(`Cannot merge principals ${JSON.stringify(target)} and ${JSON.stringify(source)}; if one uses a literal principal string the other one must be empty`); } - for (const key of Object.keys(source)) { + for (const key of sourceKeys) { target[key] = target[key] ?? []; let value = source[key]; @@ -135,6 +138,6 @@ export class UniqueStringSet implements IResolvable, IPostProcessor { } } -function isEmptyObject(x: { [key: string]: any }): boolean { - return Object.keys(x).length === 0; -} +export function sum(xs: number[]) { + return xs.reduce((a, b) => a + b, 0); +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-iam/test/role.test.ts b/packages/@aws-cdk/aws-iam/test/role.test.ts index bdd7a90c5ff85..fd0b024d60c22 100644 --- a/packages/@aws-cdk/aws-iam/test/role.test.ts +++ b/packages/@aws-cdk/aws-iam/test/role.test.ts @@ -1,6 +1,6 @@ -import { Template } from '@aws-cdk/assertions'; +import { Template, Match } from '@aws-cdk/assertions'; import { testDeprecated } from '@aws-cdk/cdk-build-tools'; -import { Duration, Stack, App } from '@aws-cdk/core'; +import { Duration, Stack, App, CfnResource } from '@aws-cdk/core'; import { AnyPrincipal, ArnPrincipal, CompositePrincipal, FederatedPrincipal, ManagedPolicy, PolicyStatement, Role, ServicePrincipal, User, Policy, PolicyDocument } from '../lib'; describe('IAM role', () => { @@ -593,4 +593,64 @@ test('managed policy ARNs are deduplicated', () => { }, ], }); +}); + +describe('role with too large inline policy', () => { + const N = 100; + + let app: App; + let stack: Stack; + let role: Role; + beforeEach(() => { + app = new App(); + stack = new Stack(app, 'my-stack'); + 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${i}`], + })); + } + }); + + test('excess gets split off into ManagedPolicies', () => { + // THEN + const template = Template.fromStack(stack); + template.hasResourceProperties('AWS::IAM::ManagedPolicy', { + PolicyDocument: { + Statement: Match.arrayWith([ + Match.objectLike({ + Resource: `arn:aws:service:us-east-1:111122223333:someResource/SomeSpecificResource${N - 1}`, + }), + ]), + }, + Roles: [{ Ref: 'MyRoleF48FFE04' }], + }); + }); + + test('Dependables track the final declaring construct', () => { + // WHEN + const result = role.addToPrincipalPolicy(new PolicyStatement({ + actions: ['aws:DoAThing'], + resources: [`arn:aws:service:us-east-1:111122223333:someResource/SomeSpecificResource${N}`], + })); + + const res = new CfnResource(stack, 'Depender', { + type: 'AWS::Some::Resource', + }); + + expect(result.policyDependable).toBeTruthy(); + res.node.addDependency(result.policyDependable!); + + // THEN + const template = Template.fromStack(stack); + template.hasResource('AWS::Some::Resource', { + DependsOn: [ + 'MyRoleOverflowPolicy13EF5596A', + ], + }); + }); }); \ No newline at end of file diff --git a/packages/@aws-cdk/cloud-assembly-schema/lib/assets/aws-destination.ts b/packages/@aws-cdk/cloud-assembly-schema/lib/assets/aws-destination.ts index 7dd2d9eacd8f9..f419fde03c56d 100644 --- a/packages/@aws-cdk/cloud-assembly-schema/lib/assets/aws-destination.ts +++ b/packages/@aws-cdk/cloud-assembly-schema/lib/assets/aws-destination.ts @@ -12,9 +12,6 @@ export interface AwsDestination { /** * The role that needs to be assumed while publishing this asset * - * The `assumeRole` fields is the preferred way of passing this information, - * but this needs to be supported for backwards compatibility. - * * @default - No role will be assumed */ readonly assumeRoleArn?: string; @@ -22,19 +19,7 @@ export interface AwsDestination { /** * The ExternalId that needs to be supplied while assuming this role * - * The `assumeRole` fields is the preferred way of passing this information, - * but this needs to be supported for backwards compatibility. - * * @default - No ExternalId will be supplied */ readonly assumeRoleExternalId?: string; - - /** - * Tags associated with the given role - * - * This information may be used to create IAM policies targeting this role. - * - * @default - No tags - */ - readonly assumeRoleTags?: Record; } \ No newline at end of file diff --git a/packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/artifact-schema.ts b/packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/artifact-schema.ts index fbc59dac37fab..4d98b3a29bb32 100644 --- a/packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/artifact-schema.ts +++ b/packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/artifact-schema.ts @@ -30,15 +30,6 @@ export interface BootstrapRole { * @default - Discover SSM parameter by reading stack */ readonly bootstrapStackVersionSsmParameter?: string; - - /** - * Tags associated with the given role - * - * This information may be used to create IAM policies targeting this role. - * - * @default - No tags - */ - readonly tags?: Record; } /** @@ -80,9 +71,6 @@ export interface AwsCloudFormationStackProperties { /** * The role that needs to be assumed to deploy the stack * - * The `assumeRole` field is the preferred way to pass this information, but this field - * needs to be supported for backwards compatibility. - * * @default - No role is assumed (current credentials are used) */ readonly assumeRoleArn?: string; @@ -90,22 +78,10 @@ export interface AwsCloudFormationStackProperties { /** * External ID to use when assuming role for cloudformation deployments * - * The `assumeRole` field is the preferred way to pass this information, but this field - * needs to be supported for backwards compatibility. - * * @default - No external ID */ readonly assumeRoleExternalId?: string; - /** - * Tags associated with the given role - * - * This information may be used to create IAM policies targeting this role. - * - * @default - No tags - */ - readonly assumeRoleTags?: Record; - /** * The role that is passed to CloudFormation to execute the change set * diff --git a/packages/@aws-cdk/cloud-assembly-schema/schema/assets.schema.json b/packages/@aws-cdk/cloud-assembly-schema/schema/assets.schema.json index 1574ad14aa121..40134a4e554a5 100644 --- a/packages/@aws-cdk/cloud-assembly-schema/schema/assets.schema.json +++ b/packages/@aws-cdk/cloud-assembly-schema/schema/assets.schema.json @@ -91,16 +91,12 @@ "type": "string" }, "assumeRoleArn": { - "description": "The role that needs to be assumed while publishing this asset\n\nThe `assumeRole` fields is the preferred way of passing this information,\nbut this needs to be supported for backwards compatibility. (Default - No role will be assumed)", + "description": "The role that needs to be assumed while publishing this asset (Default - No role will be assumed)", "type": "string" }, "assumeRoleExternalId": { - "description": "The ExternalId that needs to be supplied while assuming this role\n\nThe `assumeRole` fields is the preferred way of passing this information,\nbut this needs to be supported for backwards compatibility. (Default - No ExternalId will be supplied)", + "description": "The ExternalId that needs to be supplied while assuming this role (Default - No ExternalId will be supplied)", "type": "string" - }, - "assumeRoleTags": { - "description": "Tags associated with the given role\n\nThis information may be used to create IAM policies targeting this role. (Default - No tags)", - "$ref": "#/definitions/Record" } }, "required": [ @@ -108,9 +104,6 @@ "objectKey" ] }, - "Record": { - "type": "object" - }, "DockerImageAsset": { "description": "A file asset", "type": "object", @@ -185,16 +178,12 @@ "type": "string" }, "assumeRoleArn": { - "description": "The role that needs to be assumed while publishing this asset\n\nThe `assumeRole` fields is the preferred way of passing this information,\nbut this needs to be supported for backwards compatibility. (Default - No role will be assumed)", + "description": "The role that needs to be assumed while publishing this asset (Default - No role will be assumed)", "type": "string" }, "assumeRoleExternalId": { - "description": "The ExternalId that needs to be supplied while assuming this role\n\nThe `assumeRole` fields is the preferred way of passing this information,\nbut this needs to be supported for backwards compatibility. (Default - No ExternalId will be supplied)", + "description": "The ExternalId that needs to be supplied while assuming this role (Default - No ExternalId will be supplied)", "type": "string" - }, - "assumeRoleTags": { - "description": "Tags associated with the given role\n\nThis information may be used to create IAM policies targeting this role. (Default - No tags)", - "$ref": "#/definitions/Record" } }, "required": [ diff --git a/packages/@aws-cdk/cloud-assembly-schema/schema/cloud-assembly.schema.json b/packages/@aws-cdk/cloud-assembly-schema/schema/cloud-assembly.schema.json index 9a79797737a4b..19ab465985d24 100644 --- a/packages/@aws-cdk/cloud-assembly-schema/schema/cloud-assembly.schema.json +++ b/packages/@aws-cdk/cloud-assembly-schema/schema/cloud-assembly.schema.json @@ -300,17 +300,13 @@ "type": "boolean" }, "assumeRoleArn": { - "description": "The role that needs to be assumed to deploy the stack\n\nThe `assumeRole` field is the preferred way to pass this information, but this field\nneeds to be supported for backwards compatibility. (Default - No role is assumed (current credentials are used))", + "description": "The role that needs to be assumed to deploy the stack (Default - No role is assumed (current credentials are used))", "type": "string" }, "assumeRoleExternalId": { - "description": "External ID to use when assuming role for cloudformation deployments\n\nThe `assumeRole` field is the preferred way to pass this information, but this field\nneeds to be supported for backwards compatibility. (Default - No external ID)", + "description": "External ID to use when assuming role for cloudformation deployments (Default - No external ID)", "type": "string" }, - "assumeRoleTags": { - "description": "Tags associated with the given role\n\nThis information may be used to create IAM policies targeting this role. (Default - No tags)", - "$ref": "#/definitions/Record" - }, "cloudFormationExecutionRoleArn": { "description": "The role that is passed to CloudFormation to execute the change set (Default - No role is passed (currently assumed role/credentials are used))", "type": "string" @@ -340,9 +336,6 @@ "templateFile" ] }, - "Record": { - "type": "object" - }, "BootstrapRole": { "description": "Information needed to access an IAM role created\nas part of the bootstrap process", "type": "object", @@ -362,10 +355,6 @@ "bootstrapStackVersionSsmParameter": { "description": "Name of SSM parameter with bootstrap stack version (Default - Discover SSM parameter by reading stack)", "type": "string" - }, - "tags": { - "description": "Tags associated with the given role\n\nThis information may be used to create IAM policies targeting this role. (Default - No tags)", - "$ref": "#/definitions/Record" } }, "required": [ diff --git a/packages/@aws-cdk/core/lib/stack-synthesizers/_asset-manifest-builder.ts b/packages/@aws-cdk/core/lib/stack-synthesizers/_asset-manifest-builder.ts index 60a4784e92785..b1e121399cf4a 100644 --- a/packages/@aws-cdk/core/lib/stack-synthesizers/_asset-manifest-builder.ts +++ b/packages/@aws-cdk/core/lib/stack-synthesizers/_asset-manifest-builder.ts @@ -47,7 +47,6 @@ export class AssetManifestBuilder { region: resolvedOr(stack.region, undefined), assumeRoleArn: role?.assumeRoleArn, assumeRoleExternalId: role?.assumeRoleExternalId, - assumeRoleTags: role?.tags, }, }, }; @@ -102,7 +101,6 @@ export class AssetManifestBuilder { region: resolvedOr(stack.region, undefined), assumeRoleArn: role?.assumeRoleArn, assumeRoleExternalId: role?.assumeRoleExternalId, - assumeRoleTags: role?.tags, }, }, }; @@ -162,7 +160,6 @@ export class AssetManifestBuilder { export interface RoleOptions { readonly assumeRoleArn?: string; readonly assumeRoleExternalId?: string; - readonly tags?: Record; } function validateFileAssetSource(asset: FileAssetSource) { diff --git a/packages/@aws-cdk/core/lib/stack-synthesizers/default-synthesizer.ts b/packages/@aws-cdk/core/lib/stack-synthesizers/default-synthesizer.ts index 9a5b9906843c5..8f8e79ac467b9 100644 --- a/packages/@aws-cdk/core/lib/stack-synthesizers/default-synthesizer.ts +++ b/packages/@aws-cdk/core/lib/stack-synthesizers/default-synthesizer.ts @@ -75,15 +75,6 @@ export interface DefaultStackSynthesizerProps { */ readonly fileAssetPublishingExternalId?: string; - /** - * Tags associated with the file asset publishing role - * - * This information may be used to create IAM policies targeting this role. - * - * @default DefaultStackSynthesizer.DEFAULT_FILE_ASSET_PUBLISHING_ROLE_TAGS - */ - readonly fileAssetPublishingRoleTags?: Record; - /** * The role to use to publish image assets to the ECR repository in this environment * @@ -97,15 +88,6 @@ export interface DefaultStackSynthesizerProps { */ readonly imageAssetPublishingRoleArn?: string; - /** - * Tags associated with the image asset publishing role - * - * This information may be used to create IAM policies targeting this role. - * - * @default DefaultStackSynthesizer.DEFAULT_IMAGE_ASSET_PUBLISHING_ROLE_TAGS - */ - readonly imageAssetPublishingRoleTags?: Record; - /** * The role to use to look up values from the target AWS account during synthesis * @@ -120,15 +102,6 @@ export interface DefaultStackSynthesizerProps { */ readonly lookupRoleExternalId?: string; - /** - * Tags associated with the lookup role - * - * This information may be used to create IAM policies targeting this role. - * - * @default DefaultStackSynthesizer.DEFAULT_LOOKUP_ROLE_TAGS - */ - readonly lookupRoleTags?: Record; - /** * Use the bootstrapped lookup role for (read-only) stack operations * @@ -168,15 +141,6 @@ export interface DefaultStackSynthesizerProps { */ readonly deployRoleArn?: string; - /** - * Tags associated with the deploy role - * - * This information may be used to create IAM policies targeting this role. - * - * @default DefaultStackSynthesizer.DEFAULT_DEPLOY_ROLE_TAGS - */ - readonly deployRoleTags?: Record; - /** * The role CloudFormation will assume when deploying the Stack * @@ -282,36 +246,16 @@ export class DefaultStackSynthesizer extends StackSynthesizer { */ public static readonly DEFAULT_DEPLOY_ROLE_ARN = 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-${Qualifier}-deploy-role-${AWS::AccountId}-${AWS::Region}'; - /** - * Default deploy role tags - */ - public static readonly DEFAULT_DEPLOY_ROLE_TAGS: Record = { 'aws-cdk:bootstrap-role': 'deploy' }; - - /** - * Default lookup role tags - */ - public static readonly DEFAULT_LOOKUP_ROLE_TAGS: Record = { 'aws-cdk:bootstrap-role': 'lookup' }; - /** * Default asset publishing role ARN for file (S3) assets. */ public static readonly DEFAULT_FILE_ASSET_PUBLISHING_ROLE_ARN = 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-${Qualifier}-file-publishing-role-${AWS::AccountId}-${AWS::Region}'; - /** - * Default asset publishing role tags for file (S3) assets. - */ - public static readonly DEFAULT_FILE_ASSET_PUBLISHING_ROLE_TAGS: Record = { 'aws-cdk:bootstrap-role': 'file-publishing' }; - /** * Default asset publishing role ARN for image (ECR) assets. */ public static readonly DEFAULT_IMAGE_ASSET_PUBLISHING_ROLE_ARN = 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-${Qualifier}-image-publishing-role-${AWS::AccountId}-${AWS::Region}'; - /** - * Default asset publishing role tags for image (ECR) assets. - */ - public static readonly DEFAULT_IMAGE_ASSET_PUBLISHING_ROLE_TAGS: Record = { 'aws-cdk:bootstrap-role': 'image-publishing' }; - /** * Default lookup role ARN for missing values. */ @@ -352,11 +296,7 @@ export class DefaultStackSynthesizer extends StackSynthesizer { private _deployRoleArn?: string; private _cloudFormationExecutionRoleArn?: string; private fileAssetPublishingRoleArn?: string; - private fileAssetPublishingRoleTags?: Record; private imageAssetPublishingRoleArn?: string; - private imageAssetPublishingRoleTags?: Record; - private deployRoleTags?: Record; - private lookupRoleTags?: Record; private lookupRoleArn?: string; private useLookupRoleForStackOperations: boolean; private qualifier?: string; @@ -412,10 +352,6 @@ export class DefaultStackSynthesizer extends StackSynthesizer { this.bucketPrefix = spec.specialize(this.props.bucketPrefix ?? DefaultStackSynthesizer.DEFAULT_FILE_ASSET_PREFIX); this.dockerTagPrefix = spec.specialize(this.props.dockerTagPrefix ?? DefaultStackSynthesizer.DEFAULT_DOCKER_ASSET_PREFIX); this.bootstrapStackVersionSsmParameter = spec.qualifierOnly(this.props.bootstrapStackVersionSsmParameter ?? DefaultStackSynthesizer.DEFAULT_BOOTSTRAP_STACK_VERSION_SSM_PARAMETER); - this.fileAssetPublishingRoleTags = this.props.fileAssetPublishingRoleTags ?? DefaultStackSynthesizer.DEFAULT_FILE_ASSET_PUBLISHING_ROLE_TAGS; - this.imageAssetPublishingRoleTags = this.props.imageAssetPublishingRoleTags ?? DefaultStackSynthesizer.DEFAULT_IMAGE_ASSET_PUBLISHING_ROLE_TAGS; - this.deployRoleTags = this.props.deployRoleTags ?? DefaultStackSynthesizer.DEFAULT_DEPLOY_ROLE_TAGS; - this.lookupRoleTags = this.props.lookupRoleTags ?? DefaultStackSynthesizer.DEFAULT_LOOKUP_ROLE_TAGS; /* eslint-enable max-len */ } @@ -427,7 +363,6 @@ export class DefaultStackSynthesizer extends StackSynthesizer { return this.assetManifest.addFileAssetDefault(asset, this.stack, this.bucketName, this.bucketPrefix, { assumeRoleArn: this.fileAssetPublishingRoleArn, assumeRoleExternalId: this.props.fileAssetPublishingExternalId, - tags: this.fileAssetPublishingRoleTags, }); } @@ -439,7 +374,6 @@ export class DefaultStackSynthesizer extends StackSynthesizer { return this.assetManifest.addDockerImageAssetDefault(asset, this.stack, this.repositoryName, this.dockerTagPrefix, { assumeRoleArn: this.imageAssetPublishingRoleArn, assumeRoleExternalId: this.props.imageAssetPublishingExternalId, - tags: this.imageAssetPublishingRoleTags, }); } @@ -475,7 +409,6 @@ export class DefaultStackSynthesizer extends StackSynthesizer { this.emitStackArtifact(this.stack, session, { assumeRoleExternalId: this.props.deployRoleExternalId, assumeRoleArn: this._deployRoleArn, - assumeRoleTags: this.deployRoleTags, cloudFormationExecutionRoleArn: this._cloudFormationExecutionRoleArn, stackTemplateAssetObjectUrl: templateAsset.s3ObjectUrlWithPlaceholders, requiresBootstrapStackVersion: MIN_BOOTSTRAP_STACK_VERSION, @@ -486,7 +419,6 @@ export class DefaultStackSynthesizer extends StackSynthesizer { assumeRoleExternalId: this.props.lookupRoleExternalId, requiresBootstrapStackVersion: MIN_LOOKUP_ROLE_BOOTSTRAP_STACK_VERSION, bootstrapStackVersionSsmParameter: this.bootstrapStackVersionSsmParameter, - tags: this.lookupRoleTags, } : undefined, }); } diff --git a/packages/@aws-cdk/core/lib/stack-synthesizers/stack-synthesizer.ts b/packages/@aws-cdk/core/lib/stack-synthesizers/stack-synthesizer.ts index dc1d14b7eb792..75e4b46bf015f 100644 --- a/packages/@aws-cdk/core/lib/stack-synthesizers/stack-synthesizer.ts +++ b/packages/@aws-cdk/core/lib/stack-synthesizers/stack-synthesizer.ts @@ -93,13 +93,6 @@ export interface SynthesizeStackArtifactOptions { */ readonly assumeRoleExternalId?: string; - /** - * The tags associated with the assumeRoleArn - * - * @default - No tags - */ - readonly assumeRoleTags?: Record; - /** * The role that is passed to CloudFormation to execute the change set * diff --git a/packages/@aws-cdk/core/lib/stack.ts b/packages/@aws-cdk/core/lib/stack.ts index 91410813fa176..4e6287f72fc2b 100644 --- a/packages/@aws-cdk/core/lib/stack.ts +++ b/packages/@aws-cdk/core/lib/stack.ts @@ -791,7 +791,8 @@ export class Stack extends CoreConstruct implements ITaggable { const numberOfResources = Object.keys(resources).length; if (numberOfResources > this.maxResources) { - throw new Error(`Number of resources in stack '${this.node.path}': ${numberOfResources} is greater than allowed maximum of ${this.maxResources}`); + const counts = Object.entries(count(Object.values(resources).map((r: any) => `${r?.Type}`))).map(([type, c]) => `${type} (${c})`).join(', '); + throw new Error(`Number of resources in stack '${this.node.path}': ${numberOfResources} is greater than allowed maximum of ${this.maxResources}: ${counts}`); } else if (numberOfResources >= (this.maxResources * 0.8)) { Annotations.of(this).addInfo(`Number of resources: ${numberOfResources} is approaching allowed maximum of ${this.maxResources}`); } @@ -1357,6 +1358,18 @@ export interface ExportValueOptions { readonly name?: string; } +function count(xs: string[]): Record { + const ret: Record = {}; + for (const x of xs) { + if (x in ret) { + ret[x] += 1; + } else { + ret[x] = 1; + } + } + return ret; +} + // These imports have to be at the end to prevent circular imports import { CfnOutput } from './cfn-output'; import { addDependency } from './deps'; diff --git a/packages/@aws-cdk/core/test/stack-synthesis/new-style-synthesis.test.ts b/packages/@aws-cdk/core/test/stack-synthesis/new-style-synthesis.test.ts index 2e18424696f5f..4ced5a1e9afeb 100644 --- a/packages/@aws-cdk/core/test/stack-synthesis/new-style-synthesis.test.ts +++ b/packages/@aws-cdk/core/test/stack-synthesis/new-style-synthesis.test.ts @@ -49,11 +49,11 @@ describe('new style synthesis', () => { expect(firstFile).toEqual({ source: { path: 'Stack.template.json', packaging: 'file' }, destinations: { - 'current_account-current_region': expect.objectContaining({ + 'current_account-current_region': { bucketName: 'cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}', objectKey: templateObjectKey, assumeRoleArn: 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}', - }), + }, }, }); @@ -261,9 +261,6 @@ describe('new style synthesis', () => { objectKey: 'file-asset-hash.js', assumeRoleArn: 'file:role:arn', assumeRoleExternalId: 'file-external-id', - assumeRoleTags: { - 'aws-cdk:bootstrap-role': 'file-publishing', - }, }); expect(manifest.dockerImages?.['docker-asset-hash']?.destinations?.['current_account-current_region']).toEqual({ @@ -271,10 +268,9 @@ describe('new style synthesis', () => { imageTag: 'docker-asset-hash', assumeRoleArn: 'image:role:arn', assumeRoleExternalId: 'image-external-id', - assumeRoleTags: { - 'aws-cdk:bootstrap-role': 'image-publishing', - }, }); + + }); test('customize deploy role externalId', () => { @@ -327,47 +323,18 @@ describe('new style synthesis', () => { const manifest = readAssetManifest(getAssetManifest(asm)); // THEN - expect(manifest.files?.['file-asset-hash-with-prefix']?.destinations?.['current_account-current_region']).toEqual(expect.objectContaining({ + expect(manifest.files?.['file-asset-hash-with-prefix']?.destinations?.['current_account-current_region']).toEqual({ bucketName: 'file-asset-bucket', objectKey: '000000000000/file-asset-hash-with-prefix.js', assumeRoleArn: 'file:role:arn', assumeRoleExternalId: 'file-external-id', - })); + }); const templateHash = last(stackArtifact.stackTemplateAssetObjectUrl?.split('/')); expect(stackArtifact.stackTemplateAssetObjectUrl).toEqual(`s3://file-asset-bucket/000000000000/${templateHash}`); - }); - test('tags are present for all roles by default', () => { - // GIVEN - const myapp = new App(); - - // WHEN - const mystack = new Stack(myapp, 'mystack', { - synthesizer: new DefaultStackSynthesizer(), - }); - mystack.synthesizer.addFileAsset({ - fileName: __filename, - packaging: FileAssetPackaging.FILE, - sourceHash: 'asdf', - }); - - mystack.synthesizer.addDockerImageAsset({ - directoryName: 'some-folder', - sourceHash: 'xyz', - }); - - // THEN - const asm = myapp.synth(); - const assets = readAssetManifest(getAssetManifest(asm)); - - const stackArtifact = asm.getStackByName(mystack.stackName); - expect(stackArtifact.assumeRoleTags).toEqual({ 'aws-cdk:bootstrap-role': 'deploy' }); - expect(stackArtifact.lookupRole?.tags).toEqual({ 'aws-cdk:bootstrap-role': 'lookup' }); - expect(assets.files?.asdf?.destinations?.['current_account-current_region'].assumeRoleTags).toEqual({ 'aws-cdk:bootstrap-role': 'file-publishing' }); - expect(assets.dockerImages?.xyz?.destinations?.['current_account-current_region'].assumeRoleTags).toEqual({ 'aws-cdk:bootstrap-role': 'image-publishing' }); }); test('synthesis with dockerPrefix', () => { diff --git a/packages/@aws-cdk/cx-api/lib/artifacts/cloudformation-artifact.ts b/packages/@aws-cdk/cx-api/lib/artifacts/cloudformation-artifact.ts index ff9b2c3cb6197..f601131d9f532 100644 --- a/packages/@aws-cdk/cx-api/lib/artifacts/cloudformation-artifact.ts +++ b/packages/@aws-cdk/cx-api/lib/artifacts/cloudformation-artifact.ts @@ -62,13 +62,6 @@ export class CloudFormationStackArtifact extends CloudArtifact { */ public readonly assumeRoleArn?: string; - /** - * Tags associated with the deployment role - * - * @default - No tags are known - */ - public readonly assumeRoleTags?: Record; - /** * External ID to use when assuming role for cloudformation deployments * @@ -144,7 +137,6 @@ export class CloudFormationStackArtifact extends CloudArtifact { this.tags = properties.tags ?? this.tagsFromMetadata(); this.assumeRoleArn = properties.assumeRoleArn; this.assumeRoleExternalId = properties.assumeRoleExternalId; - this.assumeRoleTags = properties.assumeRoleTags; this.cloudFormationExecutionRoleArn = properties.cloudFormationExecutionRoleArn; this.stackTemplateAssetObjectUrl = properties.stackTemplateAssetObjectUrl; this.requiresBootstrapStackVersion = properties.requiresBootstrapStackVersion; diff --git a/packages/@aws-cdk/pipelines/test/codepipeline/codepipeline.test.ts b/packages/@aws-cdk/pipelines/test/codepipeline/codepipeline.test.ts index 5c039b912025a..60ba70efaf15a 100644 --- a/packages/@aws-cdk/pipelines/test/codepipeline/codepipeline.test.ts +++ b/packages/@aws-cdk/pipelines/test/codepipeline/codepipeline.test.ts @@ -1,4 +1,4 @@ -import { Template } from '@aws-cdk/assertions'; +import { Template, Annotations, Match } from '@aws-cdk/assertions'; import * as ccommit from '@aws-cdk/aws-codecommit'; import * as sqs from '@aws-cdk/aws-sqs'; import * as cdk from '@aws-cdk/core'; @@ -112,15 +112,30 @@ test('Policy sizes do not exceed the maximum size', () => { } } + // Validate sizes - for (const [logId, poldocs] of Object.entries(rolePolicies)) { - // Not entirely accurate, because our "Ref"s and "Fn::GetAtt"s actually need to be evaluated - // to ARNs... but it gives a good indication. - const totalJson = JSON.stringify(poldocs); - if (totalJson.length > 10000) { - throw new Error(`Policy for Role ${logId} is too large (${totalJson.length} bytes): ${JSON.stringify(poldocs, undefined, 2)}`); + // + // Not entirely accurate, because our "Ref"s and "Fn::GetAtt"s actually need to be evaluated + // to ARNs... but it gives an order-of-magnitude indication. + // 10% of margin for CFN intrinsics like { Fn::Join } and { Ref: 'AWS::Partition' } which don't contribute to + // the ACTUAL size, but do contribute to the measured size here. + const cfnOverheadMargin = 1.10; + + for (const [logId, poldoc] of Object.entries(rolePolicies)) { + const totalJson = JSON.stringify(poldoc); + if (totalJson.length > 10000 * cfnOverheadMargin) { + throw new Error(`Policy for Role ${logId} is too large (${totalJson.length} bytes): ${JSON.stringify(poldoc, undefined, 2)}`); } } + + for (const [logId, poldoc] of Object.entries(template.findResources('AWS::IAM::ManagedPolicy'))) { + const totalJson = JSON.stringify(poldoc); + if (totalJson.length > 6000 * cfnOverheadMargin) { + throw new Error(`Managed Policy ${logId} is too large (${totalJson.length} bytes): ${JSON.stringify(poldoc, undefined, 2)}`); + } + } + + Annotations.fromStack(pipelineStack).hasNoWarning('*', Match.anyValue()); }); interface ReuseCodePipelineStackProps extends cdk.StackProps {