From 87945523df100e3fb99086d7afc37838a207ee4e Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 27 Aug 2018 10:07:44 +0200 Subject: [PATCH 1/2] fix: restrict allowable types in IAM libraries A number methods were allowing 'any's in places where they easily lead to passing the wrong object. - `role.attachManagedPolicy` - Various methods on `PolicyStatement`. By restrictinig the types to what we actually expect (or `string`s) these mistakes will be harder to make. Fixes #622, doesn't completely resolve but helps with #621. --- packages/@aws-cdk/aws-iam/lib/role.ts | 8 ++++---- .../cdk/lib/cloudformation/permission.ts | 19 ++++++++++--------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/packages/@aws-cdk/aws-iam/lib/role.ts b/packages/@aws-cdk/aws-iam/lib/role.ts index 260f99f02bb60..f9650a926dac9 100644 --- a/packages/@aws-cdk/aws-iam/lib/role.ts +++ b/packages/@aws-cdk/aws-iam/lib/role.ts @@ -1,4 +1,4 @@ -import { ArnPrincipal, Construct, IDependable, PolicyDocument, PolicyPrincipal, PolicyStatement, Token } from '@aws-cdk/cdk'; +import { Arn, ArnPrincipal, Construct, IDependable, PolicyDocument, PolicyPrincipal, PolicyStatement, Token } from '@aws-cdk/cdk'; import { cloudformation, RoleArn } from './iam.generated'; import { IIdentityResource, IPrincipal, Policy } from './policy'; import { AttachedPolicies, undefinedIfEmpty } from './util'; @@ -15,7 +15,7 @@ export interface RoleProps { /** * A list of ARNs for managed policies associated with this role. - * You can add managed policies later using `addManagedPolicy(arn)`. + * You can add managed policies later using `attachManagedPolicy(arn)`. * @default No managed policies. */ managedPolicyArns?: any[]; @@ -97,7 +97,7 @@ export class Role extends Construct implements IIdentityResource, IPrincipal, ID public readonly dependencyElements: IDependable[]; private defaultPolicy?: Policy; - private readonly managedPolicies: string[]; + private readonly managedPolicies: Arn[]; private readonly attachedPolicies = new AttachedPolicies(); constructor(parent: Construct, name: string, props: RoleProps) { @@ -140,7 +140,7 @@ export class Role extends Construct implements IIdentityResource, IPrincipal, ID * Attaches a managed policy to this role. * @param arn The ARN of the managed policy to attach. */ - public attachManagedPolicy(arn: any) { + public attachManagedPolicy(arn: Arn) { this.managedPolicies.push(arn); } diff --git a/packages/@aws-cdk/cdk/lib/cloudformation/permission.ts b/packages/@aws-cdk/cdk/lib/cloudformation/permission.ts index a831140670dcd..6a40c260ad84f 100644 --- a/packages/@aws-cdk/cdk/lib/cloudformation/permission.ts +++ b/packages/@aws-cdk/cdk/lib/cloudformation/permission.ts @@ -1,4 +1,5 @@ import { Token } from '../core/tokens'; +import { Arn } from './arn'; import { FnConcat } from './fn'; import { AwsAccountId, AwsPartition } from './pseudo'; @@ -73,7 +74,7 @@ export class PrincipalPolicyFragment { } export class ArnPrincipal extends PolicyPrincipal { - constructor(public readonly arn: any) { + constructor(public readonly arn: Arn) { super(); } @@ -92,7 +93,7 @@ export class AccountPrincipal extends ArnPrincipal { * An IAM principal that represents an AWS service (i.e. sqs.amazonaws.com). */ export class ServicePrincipal extends PolicyPrincipal { - constructor(public readonly service: any) { + constructor(public readonly service: string) { super(); } @@ -209,15 +210,15 @@ export class PolicyStatement extends Token { return this; } - public addAwsPrincipal(arn: any): PolicyStatement { + public addAwsPrincipal(arn: Arn): PolicyStatement { return this.addPrincipal(new ArnPrincipal(arn)); } - public addAwsAccountPrincipal(accountId: any): PolicyStatement { + public addAwsAccountPrincipal(accountId: string): PolicyStatement { return this.addPrincipal(new AccountPrincipal(accountId)); } - public addServicePrincipal(service: any): PolicyStatement { + public addServicePrincipal(service: string): PolicyStatement { return this.addPrincipal(new ServicePrincipal(service)); } @@ -233,7 +234,7 @@ export class PolicyStatement extends Token { // Resources // - public addResource(resource: any): PolicyStatement { + public addResource(resource: string): PolicyStatement { this.resource.push(resource); return this; } @@ -245,7 +246,7 @@ export class PolicyStatement extends Token { return this.addResource('*'); } - public addResources(...resources: any[]): PolicyStatement { + public addResources(...resources: string[]): PolicyStatement { resources.forEach(r => this.addResource(r)); return this; } @@ -264,7 +265,7 @@ export class PolicyStatement extends Token { return this.resource && this.resource.length === 1 && this.resource[0] === '*'; } - public describe(sid: any): PolicyStatement { + public describe(sid: string): PolicyStatement { this.sid = sid; return this; } @@ -320,7 +321,7 @@ export class PolicyStatement extends Token { return this.addCondition(key, value); } - public limitToAccount(accountId: any): PolicyStatement { + public limitToAccount(accountId: string): PolicyStatement { return this.addCondition('StringEquals', new Token(() => { return { 'sts:ExternalId': accountId }; })); From 01819334d27d9fe0638f2800b1966d85a7e13332 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 27 Aug 2018 11:39:10 +0200 Subject: [PATCH 2/2] BREAKING CHANGE: all places where we call addResource('*') need to be fixed --- .../test/test.auto-scaling-group.ts | 2 +- packages/@aws-cdk/aws-iam/lib/group.ts | 4 +- packages/@aws-cdk/aws-iam/lib/policy.ts | 4 +- packages/@aws-cdk/aws-iam/lib/user.ts | 6 +-- .../@aws-cdk/aws-iam/test/integ.policy.ts | 6 +-- packages/@aws-cdk/aws-iam/test/integ.role.ts | 6 +-- packages/@aws-cdk/aws-iam/test/test.policy.ts | 18 +++---- packages/@aws-cdk/aws-iam/test/test.role.ts | 6 +-- packages/@aws-cdk/aws-kinesis/lib/stream.ts | 2 +- packages/@aws-cdk/aws-kms/lib/key.ts | 2 +- packages/@aws-cdk/aws-kms/test/integ.key.ts | 2 +- packages/@aws-cdk/aws-kms/test/test.key.ts | 16 +++--- .../@aws-cdk/aws-lambda/lib/lambda-ref.ts | 2 +- .../aws-lambda/lib/pipeline-action.ts | 4 +- .../@aws-cdk/aws-lambda/test/integ.lambda.ts | 2 +- .../@aws-cdk/aws-lambda/test/test.lambda.ts | 4 +- packages/@aws-cdk/aws-s3/lib/bucket.ts | 2 +- .../notifications-resource-handler.ts | 4 +- packages/@aws-cdk/aws-s3/test/test.bucket.ts | 6 +-- packages/@aws-cdk/aws-sns/test/test.sns.ts | 5 +- packages/@aws-cdk/aws-sqs/lib/queue-ref.ts | 2 +- packages/@aws-cdk/aws-sqs/test/test.sqs.ts | 4 +- .../cdk/lib/cloudformation/permission.ts | 15 ++---- .../cdk/test/cloudformation/test.perms.ts | 49 ++++++------------- 24 files changed, 76 insertions(+), 97 deletions(-) diff --git a/packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts b/packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts index 5deccbfbe8d5b..7302befe23212 100644 --- a/packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts +++ b/packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts @@ -125,7 +125,7 @@ export = { fleet.addToRolePolicy(new cdk.PolicyStatement() .addAction('*') - .addResource('*')); + .addAllResources()); expect(stack).toMatch({ "Resources": { diff --git a/packages/@aws-cdk/aws-iam/lib/group.ts b/packages/@aws-cdk/aws-iam/lib/group.ts index b9b37f5f997cf..0f9e830a33ec5 100644 --- a/packages/@aws-cdk/aws-iam/lib/group.ts +++ b/packages/@aws-cdk/aws-iam/lib/group.ts @@ -1,4 +1,4 @@ -import { ArnPrincipal, Construct, PolicyPrincipal, PolicyStatement, Token } from '@aws-cdk/cdk'; +import { Arn, ArnPrincipal, Construct, PolicyPrincipal, PolicyStatement, Token } from '@aws-cdk/cdk'; import { cloudformation, GroupArn } from './iam.generated'; import { IIdentityResource, IPrincipal, Policy } from './policy'; import { User } from './user'; @@ -73,7 +73,7 @@ export class Group extends Construct implements IIdentityResource, IPrincipal { * Attaches a managed policy to this group. * @param arn The ARN of the managed policy to attach. */ - public attachManagedPolicy(arn: any) { + public attachManagedPolicy(arn: Arn) { this.managedPolicies.push(arn); } diff --git a/packages/@aws-cdk/aws-iam/lib/policy.ts b/packages/@aws-cdk/aws-iam/lib/policy.ts index 5de4d57665517..7dfe6f0c92e07 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy.ts @@ -1,4 +1,4 @@ -import { Construct, IDependable, PolicyDocument, PolicyPrincipal, PolicyStatement, Token } from '@aws-cdk/cdk'; +import { Arn, Construct, IDependable, PolicyDocument, PolicyPrincipal, PolicyStatement, Token } from '@aws-cdk/cdk'; import { Group } from './group'; import { cloudformation } from './iam.generated'; import { Role } from './role'; @@ -31,7 +31,7 @@ export interface IPrincipal { * Attaches a managed policy to this principal. * @param arn The ARN of the managed policy */ - attachManagedPolicy(arn: any): void; + attachManagedPolicy(arn: Arn): void; } /** diff --git a/packages/@aws-cdk/aws-iam/lib/user.ts b/packages/@aws-cdk/aws-iam/lib/user.ts index 933b321858ab9..1289eb63ade4a 100644 --- a/packages/@aws-cdk/aws-iam/lib/user.ts +++ b/packages/@aws-cdk/aws-iam/lib/user.ts @@ -1,4 +1,4 @@ -import { ArnPrincipal, Construct, PolicyPrincipal, PolicyStatement, Token } from '@aws-cdk/cdk'; +import { Arn, ArnPrincipal, Construct, PolicyPrincipal, PolicyStatement, Token } from '@aws-cdk/cdk'; import { Group } from './group'; import { cloudformation, UserArn } from './iam.generated'; import { IIdentityResource, IPrincipal, Policy } from './policy'; @@ -79,7 +79,7 @@ export class User extends Construct implements IIdentityResource, IPrincipal { public readonly principal: PolicyPrincipal; private readonly groups = new Array(); - private readonly managedPolicies = new Array(); + private readonly managedPolicies = new Array(); private readonly attachedPolicies = new AttachedPolicies(); private defaultPolicy?: Policy; @@ -114,7 +114,7 @@ export class User extends Construct implements IIdentityResource, IPrincipal { * Attaches a managed policy to the user. * @param arn The ARN of the managed policy to attach. */ - public attachManagedPolicy(arn: any) { + public attachManagedPolicy(arn: Arn) { this.managedPolicies.push(arn); } diff --git a/packages/@aws-cdk/aws-iam/test/integ.policy.ts b/packages/@aws-cdk/aws-iam/test/integ.policy.ts index 446c5394484b4..e54c1b4bcb770 100644 --- a/packages/@aws-cdk/aws-iam/test/integ.policy.ts +++ b/packages/@aws-cdk/aws-iam/test/integ.policy.ts @@ -1,4 +1,4 @@ -import { App, PolicyStatement, Stack } from "@aws-cdk/cdk"; +import { App, Arn, PolicyStatement, Stack } from "@aws-cdk/cdk"; import { Policy } from "../lib"; import { User } from "../lib/user"; @@ -9,11 +9,11 @@ const stack = new Stack(app, 'aws-cdk-iam-policy'); const user = new User(stack, 'MyUser'); const policy = new Policy(stack, 'HelloPolicy', { policyName: 'Default' }); -policy.addStatement(new PolicyStatement().addResource('*').addAction('sqs:SendMessage')); +policy.addStatement(new PolicyStatement().addResource(new Arn('*')).addAction('sqs:SendMessage')); policy.attachToUser(user); const policy2 = new Policy(stack, 'GoodbyePolicy'); -policy2.addStatement(new PolicyStatement().addResource('*').addAction('lambda:InvokeFunction')); +policy2.addStatement(new PolicyStatement().addResource(new Arn('*')).addAction('lambda:InvokeFunction')); policy2.attachToUser(user); process.stdout.write(app.run()); diff --git a/packages/@aws-cdk/aws-iam/test/integ.role.ts b/packages/@aws-cdk/aws-iam/test/integ.role.ts index 244ebb12d5fa2..56a5d1a358244 100644 --- a/packages/@aws-cdk/aws-iam/test/integ.role.ts +++ b/packages/@aws-cdk/aws-iam/test/integ.role.ts @@ -1,4 +1,4 @@ -import { App, PolicyStatement, ServicePrincipal, Stack } from "@aws-cdk/cdk"; +import { App, Arn, PolicyStatement, ServicePrincipal, Stack } from "@aws-cdk/cdk"; import { Policy, Role } from "../lib"; const app = new App(process.argv); @@ -9,10 +9,10 @@ const role = new Role(stack, 'TestRole', { assumedBy: new ServicePrincipal('sqs.amazonaws.com') }); -role.addToPolicy(new PolicyStatement().addResource('*').addAction('sqs:SendMessage')); +role.addToPolicy(new PolicyStatement().addResource(new Arn('*')).addAction('sqs:SendMessage')); const policy = new Policy(stack, 'HelloPolicy', { policyName: 'Default' }); -policy.addStatement(new PolicyStatement().addAction('ec2:*').addResource('*')); +policy.addStatement(new PolicyStatement().addAction('ec2:*').addResource(new Arn('*'))); policy.attachToRole(role); process.stdout.write(app.run()); diff --git a/packages/@aws-cdk/aws-iam/test/test.policy.ts b/packages/@aws-cdk/aws-iam/test/test.policy.ts index e1eb8ee37c87e..4bf419d44cb80 100644 --- a/packages/@aws-cdk/aws-iam/test/test.policy.ts +++ b/packages/@aws-cdk/aws-iam/test/test.policy.ts @@ -1,4 +1,4 @@ -import { App, PolicyStatement, ServicePrincipal, Stack } from '@aws-cdk/cdk'; +import { App, Arn, PolicyStatement, ServicePrincipal, Stack } from '@aws-cdk/cdk'; import { Test } from 'nodeunit'; import { Role } from '../lib'; import { Group } from '../lib/group'; @@ -21,8 +21,8 @@ export = { const stack = new Stack(app, 'MyStack'); const policy = new Policy(stack, 'MyPolicy', { policyName: 'MyPolicyName' }); - policy.addStatement(new PolicyStatement().addResource('*').addAction('sqs:SendMessage')); - policy.addStatement(new PolicyStatement().addResource('arn').addAction('sns:Subscribe')); + policy.addStatement(new PolicyStatement().addResource(new Arn('*')).addAction('sqs:SendMessage')); + policy.addStatement(new PolicyStatement().addResource(new Arn('arn')).addAction('sns:Subscribe')); const group = new Group(stack, 'MyGroup'); group.attachInlinePolicy(policy); @@ -47,8 +47,8 @@ export = { const stack = new Stack(app, 'MyStack'); const policy = new Policy(stack, 'MyPolicy'); - policy.addStatement(new PolicyStatement().addResource('*').addAction('sqs:SendMessage')); - policy.addStatement(new PolicyStatement().addResource('arn').addAction('sns:Subscribe')); + policy.addStatement(new PolicyStatement().addResource(new Arn('*')).addAction('sqs:SendMessage')); + policy.addStatement(new PolicyStatement().addResource(new Arn('arn')).addAction('sns:Subscribe')); const user = new User(stack, 'MyUser'); user.attachInlinePolicy(policy); @@ -84,7 +84,7 @@ export = { users: [ user1 ], groups: [ group1 ], roles: [ role1 ], - statements: [ new PolicyStatement().addResource('*').addAction('dynamodb:PutItem') ], + statements: [ new PolicyStatement().addResource(new Arn('*')).addAction('dynamodb:PutItem') ], }); test.deepEqual(app.synthesizeStack(stack.name).template, { Resources: @@ -118,7 +118,7 @@ export = { const app = new App(); const stack = new Stack(app, 'MyStack'); const p = new Policy(stack, 'MyPolicy'); - p.addStatement(new PolicyStatement().addAction('*').addResource('*')); + p.addStatement(new PolicyStatement().addAction('*').addResource(new Arn('*'))); const user = new User(stack, 'MyUser'); p.attachToUser(user); @@ -150,7 +150,7 @@ export = { p.attachToUser(new User(stack, 'User2')); p.attachToGroup(new Group(stack, 'Group1')); p.attachToRole(new Role(stack, 'Role1', { assumedBy: new ServicePrincipal('lambda.amazonaws.com') })); - p.addStatement(new PolicyStatement().addResource('*').addAction('dynamodb:GetItem')); + p.addStatement(new PolicyStatement().addResource(new Arn('*')).addAction('dynamodb:GetItem')); test.deepEqual(app.synthesizeStack(stack.name).template, { Resources: { MyTestPolicy316BDB50: @@ -192,7 +192,7 @@ export = { group.attachInlinePolicy(policy); role.attachInlinePolicy(policy); - policy.addStatement(new PolicyStatement().addResource('*').addAction('*')); + policy.addStatement(new PolicyStatement().addResource(new Arn('*')).addAction('*')); test.deepEqual(app.synthesizeStack(stack.name).template, { Resources: { MyPolicy39D66CF6: diff --git a/packages/@aws-cdk/aws-iam/test/test.role.ts b/packages/@aws-cdk/aws-iam/test/test.role.ts index 98876bdf8d2a2..cb5c88a58335a 100644 --- a/packages/@aws-cdk/aws-iam/test/test.role.ts +++ b/packages/@aws-cdk/aws-iam/test/test.role.ts @@ -1,5 +1,5 @@ import { expect, haveResource } from '@aws-cdk/assert'; -import { FederatedPrincipal, PolicyStatement, Resource, ServicePrincipal, Stack } from '@aws-cdk/cdk'; +import { Arn, FederatedPrincipal, PolicyStatement, Resource, ServicePrincipal, Stack } from '@aws-cdk/cdk'; import { Test } from 'nodeunit'; import { Role } from '../lib'; @@ -33,7 +33,7 @@ export = { test.ok(!('MyRoleDefaultPolicyA36BE1DD' in stack.toCloudFormation().Resources), 'initially created without a policy'); - role.addToPolicy(new PolicyStatement().addResource('myresource').addAction('myaction')); + role.addToPolicy(new PolicyStatement().addResource(new Arn('myresource')).addAction('myaction')); test.ok(stack.toCloudFormation().Resources.MyRoleDefaultPolicyA36BE1DD, 'policy resource created'); expect(stack).toMatch({ Resources: @@ -66,7 +66,7 @@ export = { managedPolicyArns: [ 'managed1', 'managed2' ] }); - role.attachManagedPolicy('managed3'); + role.attachManagedPolicy(new Arn('managed3')); expect(stack).toMatch({ Resources: { MyRoleF48FFE04: { Type: 'AWS::IAM::Role', diff --git a/packages/@aws-cdk/aws-kinesis/lib/stream.ts b/packages/@aws-cdk/aws-kinesis/lib/stream.ts index 0ca4dbe1c6c7b..e23b04122101d 100644 --- a/packages/@aws-cdk/aws-kinesis/lib/stream.ts +++ b/packages/@aws-cdk/aws-kinesis/lib/stream.ts @@ -175,7 +175,7 @@ export abstract class StreamRef extends cdk.Construct implements logs.ILogSubscr if (!this.cloudWatchLogsRole) { // Create a role to be assumed by CWL that can write to this stream and pass itself. this.cloudWatchLogsRole = new iam.Role(this, 'CloudWatchLogsCanPutRecords', { - assumedBy: new cdk.ServicePrincipal(new cdk.FnConcat('logs.', new cdk.AwsRegion(), '.amazonaws.com')), + assumedBy: new cdk.ServicePrincipal(new cdk.FnConcat('logs.', new cdk.AwsRegion(), '.amazonaws.com').toString()), }); this.cloudWatchLogsRole.addToPolicy(new cdk.PolicyStatement().addAction('kinesis:PutRecord').addResource(this.streamArn)); this.cloudWatchLogsRole.addToPolicy(new cdk.PolicyStatement().addAction('iam:PassRole').addResource(this.cloudWatchLogsRole.roleArn)); diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts index cf98224910cca..b84a3a2e813fc 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -159,7 +159,7 @@ export class EncryptionKey extends EncryptionKeyRef { ]; this.addToResourcePolicy(new PolicyStatement() - .addResource('*') + .addAllResources() .addActions(...actions) .addAccountRootPrincipal()); } diff --git a/packages/@aws-cdk/aws-kms/test/integ.key.ts b/packages/@aws-cdk/aws-kms/test/integ.key.ts index ec3da4c9c3755..169cd428be370 100644 --- a/packages/@aws-cdk/aws-kms/test/integ.key.ts +++ b/packages/@aws-cdk/aws-kms/test/integ.key.ts @@ -8,7 +8,7 @@ const stack = new Stack(app, `aws-cdk-kms-1`); const key = new EncryptionKey(stack, 'MyKey'); key.addToResourcePolicy(new PolicyStatement() - .addResource('*') + .addAllResources() .addAction('kms:encrypt') .addAwsPrincipal(new AwsAccountId())); diff --git a/packages/@aws-cdk/aws-kms/test/test.key.ts b/packages/@aws-cdk/aws-kms/test/test.key.ts index e66b20b2e2240..08ee93a880b18 100644 --- a/packages/@aws-cdk/aws-kms/test/test.key.ts +++ b/packages/@aws-cdk/aws-kms/test/test.key.ts @@ -1,5 +1,5 @@ import { exactlyMatchTemplate, expect } from '@aws-cdk/assert'; -import { App, PolicyDocument, PolicyStatement, Stack } from '@aws-cdk/cdk'; +import { App, Arn, PolicyDocument, PolicyStatement, Stack } from '@aws-cdk/cdk'; import { Test } from 'nodeunit'; import { EncryptionKey, KeyArn } from '../lib'; @@ -69,8 +69,8 @@ export = { const stack = new Stack(app, 'Test'); const key = new EncryptionKey(stack, 'MyKey'); - const p = new PolicyStatement().addResource('*').addAction('kms:encrypt'); - p.addAwsPrincipal('arn'); + const p = new PolicyStatement().addAllResources().addAction('kms:encrypt'); + p.addAwsPrincipal(new Arn('arn')); key.addToResourcePolicy(p); expect(app.synthesizeStack(stack.name)).to(exactlyMatchTemplate({ @@ -144,8 +144,8 @@ export = { enableKeyRotation: true, enabled: false }); - const p = new PolicyStatement().addResource('*').addAction('kms:encrypt'); - p.addAwsPrincipal('arn'); + const p = new PolicyStatement().addAllResources().addAction('kms:encrypt'); + p.addAwsPrincipal(new Arn('arn')); key.addToResourcePolicy(p); expect(app.synthesizeStack(stack.name)).to(exactlyMatchTemplate({ @@ -297,7 +297,7 @@ export = { 'import/export can be used to bring in an existing key'(test: Test) { const stack1 = new Stack(); const policy = new PolicyDocument(); - policy.addStatement(new PolicyStatement().addResource('*')); + policy.addStatement(new PolicyStatement().addAllResources()); const myKey = new EncryptionKey(stack1, 'MyKey', { policy }); const exportedKeyRef = myKey.export(); @@ -357,7 +357,7 @@ export = { const key = EncryptionKey.import(stack, 'Imported', { keyArn: new KeyArn('foo/bar') }); - key.addToResourcePolicy(new PolicyStatement().addResource('*').addAction('*')); + key.addToResourcePolicy(new PolicyStatement().addAllResources().addAction('*')); test.done(); }, @@ -369,7 +369,7 @@ export = { const key = EncryptionKey.import(stack, 'Imported', { keyArn: new KeyArn('foo/bar') }); test.throws(() => - key.addToResourcePolicy(new PolicyStatement().addResource('*').addAction('*'), /* allowNoOp */ false), + key.addToResourcePolicy(new PolicyStatement().addAllResources().addAction('*'), /* allowNoOp */ false), 'Unable to add statement to IAM resource policy for KMS key: "foo/bar"'); test.done(); diff --git a/packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts b/packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts index 505562b050785..c6c14859f4a60 100644 --- a/packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts +++ b/packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts @@ -247,7 +247,7 @@ export abstract class FunctionRef extends cdk.Construct // // (Wildcards in principals are unfortunately not supported. this.addPermission('InvokedByCloudWatchLogs', { - principal: new cdk.ServicePrincipal(new cdk.FnConcat('logs.', new cdk.AwsRegion(), '.amazonaws.com')), + principal: new cdk.ServicePrincipal(new cdk.FnConcat('logs.', new cdk.AwsRegion(), '.amazonaws.com').toString()), sourceArn: arn }); this.logSubscriptionDestinationPolicyAddedFor.push(arn); diff --git a/packages/@aws-cdk/aws-lambda/lib/pipeline-action.ts b/packages/@aws-cdk/aws-lambda/lib/pipeline-action.ts index fa3789bd719eb..e2cc080b5a7c4 100644 --- a/packages/@aws-cdk/aws-lambda/lib/pipeline-action.ts +++ b/packages/@aws-cdk/aws-lambda/lib/pipeline-action.ts @@ -60,7 +60,7 @@ export class PipelineInvokeAction extends codepipeline.Action { // allow pipeline to list functions props.stage.pipelineRole.addToPolicy(new cdk.PolicyStatement() .addAction('lambda:ListFunctions') - .addResource('*')); + .addAllResources()); // allow pipeline to invoke this lambda functionn props.stage.pipelineRole.addToPolicy(new cdk.PolicyStatement() @@ -71,7 +71,7 @@ export class PipelineInvokeAction extends codepipeline.Action { const addToPolicy = props.addPutJobResultPolicy !== undefined ? props.addPutJobResultPolicy : true; if (addToPolicy) { props.lambda.addToRolePolicy(new cdk.PolicyStatement() - .addResource('*') // to avoid cycles (see docs) + .addAllResources() // to avoid cycles (see docs) .addAction('codepipeline:PutJobSuccessResult') .addAction('codepipeline:PutJobFailureResult')); } diff --git a/packages/@aws-cdk/aws-lambda/test/integ.lambda.ts b/packages/@aws-cdk/aws-lambda/test/integ.lambda.ts index 3ca2aac0bc342..acb51a6a17713 100644 --- a/packages/@aws-cdk/aws-lambda/test/integ.lambda.ts +++ b/packages/@aws-cdk/aws-lambda/test/integ.lambda.ts @@ -11,7 +11,7 @@ const fn = new lambda.Function(stack, 'MyLambda', { runtime: lambda.Runtime.NodeJS610, }); -fn.addToRolePolicy(new cdk.PolicyStatement().addResource('*').addAction('*')); +fn.addToRolePolicy(new cdk.PolicyStatement().addAllResources().addAction('*')); const version = fn.addVersion('1'); diff --git a/packages/@aws-cdk/aws-lambda/test/test.lambda.ts b/packages/@aws-cdk/aws-lambda/test/test.lambda.ts index 9c12f4e482579..117049e09960a 100644 --- a/packages/@aws-cdk/aws-lambda/test/test.lambda.ts +++ b/packages/@aws-cdk/aws-lambda/test/test.lambda.ts @@ -50,7 +50,7 @@ export = { code: new lambda.InlineCode('foo'), handler: 'index.handler', runtime: lambda.Runtime.NodeJS610, - initialPolicy: [new cdk.PolicyStatement().addAction("*").addResource("*")] + initialPolicy: [new cdk.PolicyStatement().addAction("*").addAllResources()] }); expect(stack).toMatch({ Resources: { MyLambdaServiceRole4539ECB6: @@ -186,7 +186,7 @@ export = { const stack = new cdk.Stack(); const fn = newTestLambda(stack); - test.throws(() => fn.addPermission('F1', { principal: new cdk.ArnPrincipal('just:arn') }), + test.throws(() => fn.addPermission('F1', { principal: new cdk.ArnPrincipal(new cdk.Arn('just:arn')) }), /Invalid principal type for Lambda permission statement/); fn.addPermission('S1', { principal: new cdk.ServicePrincipal('my-service') }); diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 4f7a62e481c16..6176d25470ab3 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -260,7 +260,7 @@ export abstract class BucketRef extends cdk.Construct { .addActions(...keyActions)); this.encryptionKey.addToResourcePolicy(new cdk.PolicyStatement() - .addResource('*') + .addAllResources() .addPrincipal(identity.principal) .addActions(...keyActions)); } diff --git a/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts b/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts index 209a85b3f52b3..38f2195f9fd71 100644 --- a/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts +++ b/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts @@ -63,7 +63,7 @@ export class NotificationsResourceHandler extends cdk.Construct { // handler allows to put bucket notification on s3 buckets. role.addToPolicy(new cdk.PolicyStatement() .addAction('s3:PutBucketNotification') - .addResource('*')); + .addAllResources()); const resource = new cdk.Resource(this, 'Resource', { type: 'AWS::Lambda::Function', @@ -162,4 +162,4 @@ const handler = (event: any, context: any) => { request.write(responseBody); request.end(); } -}; \ No newline at end of file +}; diff --git a/packages/@aws-cdk/aws-s3/test/test.bucket.ts b/packages/@aws-cdk/aws-s3/test/test.bucket.ts index 8c86ff387c6ca..9dd939d9de217 100644 --- a/packages/@aws-cdk/aws-s3/test/test.bucket.ts +++ b/packages/@aws-cdk/aws-s3/test/test.bucket.ts @@ -203,7 +203,7 @@ export = { const stack = new cdk.Stack(); const bucket = new s3.Bucket(stack, 'MyBucket', { encryption: s3.BucketEncryption.Unencrypted }); - bucket.addToResourcePolicy(new cdk.PolicyStatement().addResource('foo').addAction('bar')); + bucket.addToResourcePolicy(new cdk.PolicyStatement().addResource(new cdk.Arn('foo')).addAction('bar')); expect(stack).toMatch({ "Resources": { @@ -352,7 +352,7 @@ export = { const bucket = s3.Bucket.import(stack, 'ImportedBucket', { bucketArn }); // this is a no-op since the bucket is external - bucket.addToResourcePolicy(new cdk.PolicyStatement().addResource('foo').addAction('bar')); + bucket.addToResourcePolicy(new cdk.PolicyStatement().addResource(new cdk.Arn('foo')).addAction('bar')); const p = new cdk.PolicyStatement().addResource(bucket.bucketArn).addAction('s3:ListBucket'); @@ -373,7 +373,7 @@ export = { 'import can also be used to import arbitrary ARNs'(test: Test) { const stack = new cdk.Stack(); const bucket = s3.Bucket.import(stack, 'ImportedBucket', { bucketArn: new s3.BucketArn('arn:aws:s3:::my-bucket') }); - bucket.addToResourcePolicy(new cdk.PolicyStatement().addResource('*').addAction('*')); + bucket.addToResourcePolicy(new cdk.PolicyStatement().addAllResources().addAction('*')); // at this point we technically didn't create any resources in the consuming stack. expect(stack).toMatch({}); diff --git a/packages/@aws-cdk/aws-sns/test/test.sns.ts b/packages/@aws-cdk/aws-sns/test/test.sns.ts index e30672b03fa53..6ad8f4a689274 100644 --- a/packages/@aws-cdk/aws-sns/test/test.sns.ts +++ b/packages/@aws-cdk/aws-sns/test/test.sns.ts @@ -506,7 +506,10 @@ export = { const topic = new sns.Topic(stack, 'Topic'); // WHEN - topic.addToResourcePolicy(new cdk.PolicyStatement().addResource('*').addActions('sns:*').addPrincipal(new cdk.ArnPrincipal('arn'))); + topic.addToResourcePolicy(new cdk.PolicyStatement() + .addAllResources() + .addActions('sns:*') + .addPrincipal(new cdk.ArnPrincipal(new cdk.Arn('arn')))); // THEN expect(stack).to(haveResource('AWS::SNS::TopicPolicy', { diff --git a/packages/@aws-cdk/aws-sqs/lib/queue-ref.ts b/packages/@aws-cdk/aws-sqs/lib/queue-ref.ts index 047abfa325222..e7389d14c379b 100644 --- a/packages/@aws-cdk/aws-sqs/lib/queue-ref.ts +++ b/packages/@aws-cdk/aws-sqs/lib/queue-ref.ts @@ -98,7 +98,7 @@ export abstract class QueueRef extends cdk.Construct implements s3n.IBucketNotif .addServicePrincipal('s3.amazonaws.com') .addAction('kms:GenerateDataKey') .addAction('kms:Decrypt') - .addResource('*'), /* allowNoOp */ false); + .addAllResources(), /* allowNoOp */ false); } this.notifyingBuckets.add(bucketId); diff --git a/packages/@aws-cdk/aws-sqs/test/test.sqs.ts b/packages/@aws-cdk/aws-sqs/test/test.sqs.ts index 135d37d76a6d2..557a5be6a3abf 100644 --- a/packages/@aws-cdk/aws-sqs/test/test.sqs.ts +++ b/packages/@aws-cdk/aws-sqs/test/test.sqs.ts @@ -1,7 +1,7 @@ import { expect, haveResource } from '@aws-cdk/assert'; import kms = require('@aws-cdk/aws-kms'); import s3 = require('@aws-cdk/aws-s3'); -import { ArnPrincipal, PolicyStatement, resolve, Stack } from '@aws-cdk/cdk'; +import { Arn, ArnPrincipal, PolicyStatement, resolve, Stack } from '@aws-cdk/cdk'; import { Test } from 'nodeunit'; import sqs = require('../lib'); @@ -55,7 +55,7 @@ export = { 'addToPolicy will automatically create a policy for this queue'(test: Test) { const stack = new Stack(); const queue = new sqs.Queue(stack, 'MyQueue'); - queue.addToResourcePolicy(new PolicyStatement().addResource('*').addActions('sqs:*').addPrincipal(new ArnPrincipal('arn'))); + queue.addToResourcePolicy(new PolicyStatement().addAllResources().addActions('sqs:*').addPrincipal(new ArnPrincipal(new Arn('arn')))); expect(stack).toMatch({ "Resources": { "MyQueueE6CA6235": { diff --git a/packages/@aws-cdk/cdk/lib/cloudformation/permission.ts b/packages/@aws-cdk/cdk/lib/cloudformation/permission.ts index 6a40c260ad84f..3dd1610d4c2a5 100644 --- a/packages/@aws-cdk/cdk/lib/cloudformation/permission.ts +++ b/packages/@aws-cdk/cdk/lib/cloudformation/permission.ts @@ -234,7 +234,7 @@ export class PolicyStatement extends Token { // Resources // - public addResource(resource: string): PolicyStatement { + public addResource(resource: Arn): PolicyStatement { this.resource.push(resource); return this; } @@ -243,10 +243,10 @@ export class PolicyStatement extends Token { * Adds a ``"*"`` resource to this statement. */ public addAllResources(): PolicyStatement { - return this.addResource('*'); + return this.addResource(new Arn('*')); } - public addResources(...resources: string[]): PolicyStatement { + public addResources(...resources: Arn[]): PolicyStatement { resources.forEach(r => this.addResource(r)); return this; } @@ -258,13 +258,6 @@ export class PolicyStatement extends Token { return this.resource && this.resource.length > 0; } - /** - * Indicates if this permission has only a ``"*"`` resource associated with it. - */ - public get isOnlyStarResource() { - return this.resource && this.resource.length === 1 && this.resource[0] === '*'; - } - public describe(sid: string): PolicyStatement { this.sid = sid; return this; @@ -359,6 +352,8 @@ export class PolicyStatement extends Token { if (values.length === 1) { return values[0]; } + + return values; } if (typeof(values) === 'object') { diff --git a/packages/@aws-cdk/cdk/test/cloudformation/test.perms.ts b/packages/@aws-cdk/cdk/test/cloudformation/test.perms.ts index b924f0e418d93..6207ef63aceda 100644 --- a/packages/@aws-cdk/cdk/test/cloudformation/test.perms.ts +++ b/packages/@aws-cdk/cdk/test/cloudformation/test.perms.ts @@ -1,16 +1,16 @@ import { Test } from 'nodeunit'; -import { CanonicalUserPrincipal, FnConcat, PolicyDocument, PolicyStatement, resolve } from '../../lib'; +import { Arn, CanonicalUserPrincipal, FnConcat, PolicyDocument, PolicyStatement, resolve } from '../../lib'; export = { 'the Permission class is a programming model for iam'(test: Test) { const p = new PolicyStatement(); p.addAction('sqs:SendMessage'); p.addActions('dynamodb:CreateTable', 'dynamodb:DeleteTable'); - p.addResource('myQueue'); - p.addResource('yourQueue'); + p.addResource(new Arn('myQueue')); + p.addResource(new Arn('yourQueue')); p.addAllResources(); - p.addAwsAccountPrincipal(new FnConcat('my', 'account', 'name')); + p.addAwsAccountPrincipal(new FnConcat('my', 'account', 'name').toString()); p.limitToAccount('12221121221'); test.deepEqual(resolve(p), { Action: @@ -26,7 +26,7 @@ export = { [ 'arn:', { Ref: 'AWS::Partition' }, ':iam::', - 'my', 'account', 'name', + { 'Fn::Join': [ '', [ 'my', 'account', 'name' ] ] }, ':root' ] ] } }, Condition: { StringEquals: { 'sts:ExternalId': '12221121221' } } }); @@ -37,7 +37,7 @@ export = { const doc = new PolicyDocument(); const p1 = new PolicyStatement(); p1.addAction('sqs:SendMessage'); - p1.addResource('*'); + p1.addResource(new Arn('*')); const p2 = new PolicyStatement(); p2.deny(); @@ -65,7 +65,7 @@ export = { ] }; const doc = new PolicyDocument(base); - doc.addStatement(new PolicyStatement().addResource('resource').addAction('action')); + doc.addStatement(new PolicyStatement().addResource(new Arn('resource')).addAction('action')); test.deepEqual(resolve(doc), { Version: 'Foo', Something: 123, @@ -77,7 +77,7 @@ export = { }, 'Permission allows specifying multiple actions upon construction'(test: Test) { - const perm = new PolicyStatement().addResource('MyResource').addActions('Action1', 'Action2', 'Action3'); + const perm = new PolicyStatement().addResource(new Arn('MyResource')).addActions('Action1', 'Action2', 'Action3'); test.deepEqual(resolve(perm), { Effect: 'Allow', Action: [ 'Action1', 'Action2', 'Action3' ], @@ -149,14 +149,17 @@ export = { }, 'true if there is one resource'(test: Test) { - test.equal(new PolicyStatement().addResource('one-resource').hasResource, true, 'hasResource is true when there is one resource'); + test.equal( + new PolicyStatement().addResource(new Arn('one-resource')).hasResource, + true, + 'hasResource is true when there is one resource'); test.done(); }, 'true for multiple resources'(test: Test) { const p = new PolicyStatement(); - p.addResource('r1'); - p.addResource('r2'); + p.addResource(new Arn('r1')); + p.addResource(new Arn('r2')); test.equal(p.hasResource, true, 'hasResource is true when there are multiple resource'); test.done(); }, @@ -170,34 +173,12 @@ export = { 'true if there is a principal'(test: Test) { const p = new PolicyStatement(); - p.addAwsPrincipal('bla'); + p.addAwsPrincipal(new Arn('bla')); test.equal(p.hasPrincipal, true); test.done(); } }, - 'isOnlyStarResource': { - 'true if there is a single "*" resource'(test: Test) { - test.equal(new PolicyStatement().addAllResources().isOnlyStarResource, true); - test.done(); - }, - - 'false if there are other resources'(test: Test) { - const p = new PolicyStatement(); - p.addResources('r1', 'r2', 'r3'); - test.equal(p.isOnlyStarResource, false); - test.done(); - }, - - 'false if there are other resources and a "*" resource'(test: Test) { - const p = new PolicyStatement(); - p.addResources('r1', 'r2', 'r3'); - p.addResource('*'); - test.equal(p.isOnlyStarResource, false); - test.done(); - }, - }, - 'statementCount returns the number of statement in the policy document'(test: Test) { const p = new PolicyDocument(); test.equal(p.statementCount, 0);