diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts index f912a4c180182..a3b42438f945c 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts @@ -69,20 +69,20 @@ export class PolicyStatement { return ret; } - private readonly _action = new Array(); - private readonly _notAction = new Array(); + private readonly _action = new OrderedSet(); + private readonly _notAction = new OrderedSet(); private readonly _principal: { [key: string]: any[] } = {}; private readonly _notPrincipal: { [key: string]: any[] } = {}; - private readonly _resource = new Array(); - private readonly _notResource = new Array(); + private readonly _resource = new OrderedSet(); + private readonly _notResource = new OrderedSet(); private readonly _condition: { [key: string]: any } = { }; private _sid?: string; private _effect: Effect; private principalConditionsJson?: string; // Hold on to those principals - private readonly _principals = new Array(); - private readonly _notPrincipals = new Array(); + private readonly _principals = new OrderedSet(); + private readonly _notPrincipals = new OrderedSet(); private _frozen = false; constructor(props: PolicyStatementProps = {}) { @@ -193,12 +193,15 @@ export class PolicyStatement { */ public addPrincipals(...principals: IPrincipal[]) { this.assertNotFrozen('addPrincipals'); - this._principals.push(...principals); - if (Object.keys(principals).length > 0 && Object.keys(this._notPrincipal).length > 0) { + if (principals.length > 0 && this._notPrincipals.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 added = this._principals.push(...principals); + for (const principal of added) { const fragment = principal.policyFragment; mergePrincipal(this._principal, fragment.principalJson); this.addPrincipalConditions(fragment.conditions); @@ -215,12 +218,15 @@ export class PolicyStatement { */ public addNotPrincipals(...notPrincipals: IPrincipal[]) { this.assertNotFrozen('addNotPrincipals'); - this._notPrincipals.push(...notPrincipals); - if (Object.keys(notPrincipals).length > 0 && Object.keys(this._principal).length > 0) { + if (notPrincipals.length > 0 && this._principals.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 added = this._notPrincipals.push(...notPrincipals); + for (const notPrincipal of added) { const fragment = notPrincipal.policyFragment; mergePrincipal(this._notPrincipal, fragment.principalJson); this.addPrincipalConditions(fragment.conditions); @@ -426,14 +432,14 @@ export class PolicyStatement { */ public toStatementJson(): any { return normalizeStatement({ - Action: this._action, - NotAction: this._notAction, + Action: this._action.direct(), + NotAction: this._notAction.direct(), Condition: this._condition, Effect: this.effect, Principal: this._principal, NotPrincipal: this._notPrincipal, - Resource: this._resource, - NotResource: this._notResource, + Resource: this._resource.direct(), + NotResource: this._notResource.direct(), Sid: this.sid, }); } @@ -503,7 +509,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 (this._principals.length === 0 && this._notPrincipals.length === 0) { errors.push('A PolicyStatement used in a resource-based policy must specify at least one IAM principal.'); } return errors; @@ -516,10 +522,10 @@ export class PolicyStatement { */ public validateForIdentityPolicy(): string[] { const errors = this.validateForAnyPolicy(); - if (Object.keys(this._principal).length > 0 || Object.keys(this._notPrincipal).length > 0) { + if (this._principals.length > 0 || this._notPrincipals.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 (this._resource.length === 0 && this._notResource.length === 0) { errors.push('A PolicyStatement used in an identity-based policy must specify at least one resource.'); } return errors; @@ -529,42 +535,42 @@ export class PolicyStatement { * The Actions added to this statement */ public get actions() { - return [...this._action]; + return this._action.copy(); } /** * The NotActions added to this statement */ public get notActions() { - return [...this._notAction]; + return this._notAction.copy(); } /** * The Principals added to this statement */ public get principals(): IPrincipal[] { - return [...this._principals]; + return this._principals.copy(); } /** * The NotPrincipals added to this statement */ public get notPrincipals(): IPrincipal[] { - return [...this._notPrincipals]; + return this._notPrincipals.copy(); } /** * The Resources added to this statement */ public get resources() { - return [...this._resource]; + return this._resource.copy(); } /** * The NotResources added to this statement */ public get notResources() { - return [...this._notResource]; + return this._notResource.copy(); } /** @@ -821,4 +827,54 @@ export function deriveEstimateSizeOptions(scope: IConstruct): EstimateSizeOption } return { actionEstimate, arnEstimate }; +} + +/** + * A class that behaves both as a set and an array + * + * Used for the elements of a PolicyStatement. In practice they behave as sets, + * but we have thousands of snapshot tests in existence that will rely on a + * particular order so we can't just change the type to `Set<>` wholesale without + * causing a lot of churn. + */ +class OrderedSet { + private readonly set = new Set(); + private readonly array = new Array(); + + /** + * Add new elements to the set + * + * @param xs the elements to be added + * + * @returns the elements actually added + */ + public push(...xs: readonly A[]): A[] { + const ret = new Array(); + for (const x of xs) { + if (this.set.has(x)) { + continue; + } + this.set.add(x); + this.array.push(x); + ret.push(x); + } + return ret; + } + + public get length() { + return this.array.length; + } + + public copy(): A[] { + return [...this.array]; + } + + /** + * Direct (read-only) access to the underlying array + * + * (Saves a copy) + */ + public direct(): readonly A[] { + return this.array; + } } \ No newline at end of file diff --git a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts index ca759d438ab89..af39183ed002b 100644 --- a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts +++ b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts @@ -347,8 +347,8 @@ describe('IAM policy document', () => { expect(stack.resolve(statement.toStatementJson())).toEqual({ Effect: 'Allow', - Action: ['a'], - Resource: ['x'], + Action: 'a', + Resource: 'x', }); }); @@ -364,8 +364,8 @@ describe('IAM policy document', () => { expect(stack.resolve(statement.toStatementJson())).toEqual({ Effect: 'Allow', - NotAction: ['a'], - NotResource: ['x'], + NotAction: 'a', + NotResource: 'x', }); }); diff --git a/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts b/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts index ec1f9f691f727..27965fbe150d1 100644 --- a/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts +++ b/packages/@aws-cdk/aws-iam/test/policy-statement.test.ts @@ -215,6 +215,22 @@ describe('IAM policy statement', () => { .toThrow(/Cannot use an IAM Group as the 'Principal' or 'NotPrincipal' in an IAM Policy/); }); + test('multiple identical entries render to a scalar (instead of a singleton list)', () => { + const stack = new Stack(); + const policyStatement = new PolicyStatement({ + actions: ['aws:Action'], + }); + + policyStatement.addResources('asdf'); + policyStatement.addResources('asdf'); + policyStatement.addResources('asdf'); + + expect(stack.resolve(policyStatement.toStatementJson())).toEqual({ + Effect: 'Allow', + Action: 'aws:Action', + Resource: 'asdf', + }); + }); test('a frozen policy statement cannot be modified any more', () => { // GIVEN diff --git a/packages/@aws-cdk/pipelines/lib/codepipeline/codepipeline.ts b/packages/@aws-cdk/pipelines/lib/codepipeline/codepipeline.ts index a764d341243a7..3eaabf40c6641 100644 --- a/packages/@aws-cdk/pipelines/lib/codepipeline/codepipeline.ts +++ b/packages/@aws-cdk/pipelines/lib/codepipeline/codepipeline.ts @@ -4,7 +4,7 @@ import * as cp from '@aws-cdk/aws-codepipeline'; import * as cpa from '@aws-cdk/aws-codepipeline-actions'; import * as ec2 from '@aws-cdk/aws-ec2'; import * as iam from '@aws-cdk/aws-iam'; -import { Aws, CfnCapabilities, Duration, Fn, Lazy, PhysicalName, Stack } from '@aws-cdk/core'; +import { Aws, CfnCapabilities, Duration, PhysicalName, Stack } from '@aws-cdk/core'; import * as cxapi from '@aws-cdk/cx-api'; import { Construct } from 'constructs'; import { AssetType, FileSet, IFileSetProducer, ManualApprovalStep, ShellStep, StackAsset, StackDeployment, Step } from '../blueprint'; @@ -12,6 +12,7 @@ import { DockerCredential, dockerCredentialsInstallCommands, DockerCredentialUsa import { GraphNodeCollection, isGraph, AGraphNode, PipelineGraph } from '../helpers-internal'; import { PipelineBase } from '../main'; import { AssetSingletonRole } from '../private/asset-singleton-role'; +import { CachedFnSub } from '../private/cached-fnsub'; import { preferredCliVersion } from '../private/cli-version'; import { appOf, assemblyBuilderOf, embeddedAsmPath, obtainScope } from '../private/construct-internals'; import { toPosixPath } from '../private/fs'; @@ -293,16 +294,12 @@ export class CodePipeline extends PipelineBase { private readonly selfMutation: boolean; private _myCxAsmRoot?: string; private readonly dockerCredentials: DockerCredential[]; + private readonly cachedFnSub = new CachedFnSub(); /** * Asset roles shared for publishing */ - private readonly assetCodeBuildRoles: Record = {}; - - /** - * Per asset type, the target role ARNs that need to be assumed - */ - private readonly assetPublishingRoles: Record> = {}; + private readonly assetCodeBuildRoles: Map = new Map(); /** * This is set to the very first artifact produced in the pipeline @@ -678,15 +675,13 @@ export class CodePipeline extends PipelineBase { throw new Error('All assets in a single publishing step must be of the same type'); } - const publishingRoles = this.assetPublishingRoles[assetType] = (this.assetPublishingRoles[assetType] ?? new Set()); - for (const asset of assets) { - if (asset.assetPublishingRoleArn) { - publishingRoles.add(asset.assetPublishingRoleArn); - } - } - const role = this.obtainAssetCodeBuildRole(assets[0].assetType); + for (const roleArn of assets.flatMap(a => a.assetPublishingRoleArn ? [a.assetPublishingRoleArn] : [])) { + // The ARNs include raw AWS pseudo parameters (e.g., ${AWS::Partition}), which need to be substituted. + role.addAssumeRole(this.cachedFnSub.fnSub(roleArn)); + }; + // The base commands that need to be run const script = new CodeBuildStep(node.id, { commands, @@ -824,9 +819,10 @@ export class CodePipeline extends PipelineBase { * Modeled after the CodePipeline role and 'CodePipelineActionRole' roles. * Generates one role per asset type to separate file and Docker/image-based permissions. */ - private obtainAssetCodeBuildRole(assetType: AssetType): iam.IRole { - if (this.assetCodeBuildRoles[assetType]) { - return this.assetCodeBuildRoles[assetType]; + private obtainAssetCodeBuildRole(assetType: AssetType): AssetSingletonRole { + const existing = this.assetCodeBuildRoles.get(assetType); + if (existing) { + return existing; } const stack = Stack.of(this); @@ -840,22 +836,15 @@ export class CodePipeline extends PipelineBase { ), }); - // Publishing role access - // The ARNs include raw AWS pseudo parameters (e.g., ${AWS::Partition}), which need to be substituted. - // Lazy-evaluated so all asset publishing roles are included. - assetRole.addToPolicy(new iam.PolicyStatement({ - actions: ['sts:AssumeRole'], - resources: Lazy.list({ produce: () => Array.from(this.assetPublishingRoles[assetType] ?? []).map(arn => Fn.sub(arn)) }), - })); - // Grant pull access for any ECR registries and secrets that exist if (assetType === AssetType.DOCKER_IMAGE) { this.dockerCredentials.forEach(reg => reg.grantRead(assetRole, DockerCredentialUsage.ASSET_PUBLISHING)); } - this.assetCodeBuildRoles[assetType] = assetRole; + this.assetCodeBuildRoles.set(assetType, assetRole); return assetRole; } + } function dockerUsageFromCodeBuild(cbt: CodeBuildProjectType): DockerCredentialUsage | undefined { diff --git a/packages/@aws-cdk/pipelines/lib/legacy/pipeline.ts b/packages/@aws-cdk/pipelines/lib/legacy/pipeline.ts index 974c8ce16ef1a..dd3ba4fadf7fb 100644 --- a/packages/@aws-cdk/pipelines/lib/legacy/pipeline.ts +++ b/packages/@aws-cdk/pipelines/lib/legacy/pipeline.ts @@ -3,12 +3,13 @@ import * as codebuild from '@aws-cdk/aws-codebuild'; import * as codepipeline from '@aws-cdk/aws-codepipeline'; import * as ec2 from '@aws-cdk/aws-ec2'; import * as iam from '@aws-cdk/aws-iam'; -import { Annotations, App, CfnOutput, Fn, Lazy, PhysicalName, Stack, Stage } from '@aws-cdk/core'; +import { Annotations, App, CfnOutput, PhysicalName, Stack, Stage } from '@aws-cdk/core'; import { Construct } from 'constructs'; import { AssetType } from '../blueprint/asset-type'; import { dockerCredentialsInstallCommands, DockerCredential, DockerCredentialUsage } from '../docker-credentials'; import { ApplicationSecurityCheck } from '../private/application-security-check'; import { AssetSingletonRole } from '../private/asset-singleton-role'; +import { CachedFnSub } from '../private/cached-fnsub'; import { preferredCliVersion } from '../private/cli-version'; import { appOf, assemblyBuilderOf } from '../private/construct-internals'; import { DeployCdkStackAction, PublishAssetsAction, UpdatePipelineAction } from './actions'; @@ -488,10 +489,10 @@ class AssetPublishing extends Construct { private readonly MAX_PUBLISHERS_PER_STAGE = 50; private readonly publishers: Record = {}; - private readonly assetRoles: Record = {}; + private readonly assetRoles: Map = new Map(); private readonly assetAttachedPolicies: Record = {}; - private readonly assetPublishingRoles: Record> = {}; private readonly myCxAsmRoot: string; + private readonly cachedFnSub = new CachedFnSub(); private readonly lastStageBeforePublishing?: codepipeline.IStage; private readonly stages: codepipeline.IStage[] = []; @@ -533,11 +534,9 @@ class AssetPublishing extends Construct { } // Late-binding here (rather than in the constructor) to prevent creating the role in cases where no asset actions are created. - if (!this.assetRoles[command.assetType]) { - this.generateAssetRole(command.assetType); - } - this.assetPublishingRoles[command.assetType] = (this.assetPublishingRoles[command.assetType] ?? new Set()).add(command.assetPublishingRoleArn); - + const assetRole = this.generateAssetRole(command.assetType); + // The ARNs include raw AWS pseudo parameters (e.g., ${AWS::Partition}), which need to be substituted. + assetRole.addAssumeRole(this.cachedFnSub.fnSub(command.assetPublishingRoleArn)); const publisherKey = this.props.singlePublisherPerType ? command.assetType.toString() : command.assetId; let action = this.publishers[publisherKey]; @@ -579,7 +578,7 @@ class AssetPublishing extends Construct { cloudAssemblyInput: this.props.cloudAssemblyInput, cdkCliVersion: this.props.cdkCliVersion, assetType: command.assetType, - role: this.assetRoles[command.assetType], + role: this.assetRoles.get(command.assetType), dependable: this.assetAttachedPolicies[command.assetType], vpc: this.props.vpc, subnetSelection: this.props.subnetSelection, @@ -600,7 +599,10 @@ class AssetPublishing extends Construct { * Generates one role per asset type to separate file and Docker/image-based permissions. */ private generateAssetRole(assetType: AssetType) { - if (this.assetRoles[assetType]) { return this.assetRoles[assetType]; } + const existing = this.assetRoles.get(assetType); + if (existing) { + return existing; + } const rolePrefix = assetType === AssetType.DOCKER_IMAGE ? 'Docker' : 'File'; const assetRole = new AssetSingletonRole(this, `${rolePrefix}Role`, { @@ -608,21 +610,13 @@ class AssetPublishing extends Construct { assumedBy: new iam.CompositePrincipal(new iam.ServicePrincipal('codebuild.amazonaws.com'), new iam.AccountPrincipal(Stack.of(this).account)), }); - // Publishing role access - // The ARNs include raw AWS pseudo parameters (e.g., ${AWS::Partition}), which need to be substituted. - // Lazy-evaluated so all asset publishing roles are included. - assetRole.addToPolicy(new iam.PolicyStatement({ - actions: ['sts:AssumeRole'], - resources: Lazy.list({ produce: () => [...this.assetPublishingRoles[assetType]].map(arn => Fn.sub(arn)) }), - })); - // Grant pull access for any ECR registries and secrets that exist if (assetType === AssetType.DOCKER_IMAGE) { this.dockerCredentials.forEach(reg => reg.grantRead(assetRole, DockerCredentialUsage.ASSET_PUBLISHING)); } - this.assetRoles[assetType] = assetRole; - return this.assetRoles[assetType]; + this.assetRoles.set(assetType, assetRole); + return assetRole; } } diff --git a/packages/@aws-cdk/pipelines/lib/private/asset-singleton-role.ts b/packages/@aws-cdk/pipelines/lib/private/asset-singleton-role.ts index 3015387ab1a5f..d43dfba61613d 100644 --- a/packages/@aws-cdk/pipelines/lib/private/asset-singleton-role.ts +++ b/packages/@aws-cdk/pipelines/lib/private/asset-singleton-role.ts @@ -11,6 +11,7 @@ import { Construct, IDependable } from 'constructs'; */ export class AssetSingletonRole extends iam.Role { private _rejectDuplicates = false; + private _assumeRoleStatement: iam.PolicyStatement | undefined; constructor(scope: Construct, id: string, props: iam.RoleProps) { super(scope, id, props); @@ -82,4 +83,36 @@ export class AssetSingletonRole extends iam.Role { return super.addToPrincipalPolicy(statement); } + + /** + * Make sure the Role has sts:AssumeRole permissions to the given ARN + * + * Will add a new PolicyStatement to the Role if necessary, otherwise add resources to the existing + * PolicyStatement. + * + * Normally this would have been many `grantAssume()` calls (which would get deduplicated by the + * policy minimization logic), but we have to account for old pipelines that don't have policy + * minimization enabled. + */ + public addAssumeRole(roleArn: string) { + if (!this._assumeRoleStatement) { + this._assumeRoleStatement = new iam.PolicyStatement({ + actions: ['sts:AssumeRole'], + }); + + this.addToPrincipalPolicy(this._assumeRoleStatement); + } + + // Chunk into multiple statements to facilitate overflowing into overflow policies. + // Ideally we would do one ARN per statement and have policy minimization do its job, but that would make + // the situation A LOT worse if minimization is not enabled (which it isn't by default). So find a middle + // ground in pre-minimization chunking: reduce overhead while still allowing splitting. + const MAX_ARNS_PER_STATEMENT = 10; + + this._assumeRoleStatement.addResources(roleArn); + if (this._assumeRoleStatement.resources.length >= MAX_ARNS_PER_STATEMENT) { + // Next call to this function will create a new statement + this._assumeRoleStatement = undefined; + } + } } diff --git a/packages/@aws-cdk/pipelines/lib/private/cached-fnsub.ts b/packages/@aws-cdk/pipelines/lib/private/cached-fnsub.ts new file mode 100644 index 0000000000000..1a7e5595140b1 --- /dev/null +++ b/packages/@aws-cdk/pipelines/lib/private/cached-fnsub.ts @@ -0,0 +1,28 @@ +import { Fn } from '@aws-cdk/core'; + +/** + * Wrap a string in `Fn.sub`, but return the same `Fn.sub` value for the same string + * + * If we don't do this, every new `Fn.sub()` creates a new `IResolvable` instance + * which will stringify to a unique string value, and we can't dedupe the stringified + * values anymore. + * + * Potentially we could/should do deduplication in the token system itself, but + * we would have to be consistent about it and do it for all tokens, which has + * an unpredictable memory impact and I'm scared of making such a sweeping + * change. Hence, a local solution to a local problem. + */ +export class CachedFnSub { + private cache = new Map(); + + public fnSub(x: string) { + const existing = this.cache.get(x); + if (existing) { + return existing; + } + + const ret = Fn.sub(x); + this.cache.set(x, ret); + return ret; + } +} \ No newline at end of file diff --git a/packages/@aws-cdk/pipelines/test/compliance/assets.test.ts b/packages/@aws-cdk/pipelines/test/compliance/assets.test.ts index 2828d494c1c3b..20471b4c1828a 100644 --- a/packages/@aws-cdk/pipelines/test/compliance/assets.test.ts +++ b/packages/@aws-cdk/pipelines/test/compliance/assets.test.ts @@ -949,7 +949,7 @@ function expectedAssetRolePolicy(assumeRolePattern: string | string[], attachedR { Action: 'sts:AssumeRole', Effect: 'Allow', - Resource: assumeRolePattern.map(arn => { return { 'Fn::Sub': arn }; }), + Resource: unsingleton(assumeRolePattern.map(arn => { return { 'Fn::Sub': arn }; })), }, { Action: ['s3:GetObject*', 's3:GetBucket*', 's3:List*'], @@ -1063,4 +1063,11 @@ function synthesize(stack: Stack) { } return root.synth({ skipValidation: true }); +} + +function unsingleton(xs: A[]): A | A[] { + if (xs.length === 1) { + return xs[0]; + } + return xs; } \ No newline at end of file