From 3917c4b98c1e3bc51d0b2c39e1ff609168722034 Mon Sep 17 00:00:00 2001 From: Jonathan Goldwasser Date: Mon, 30 Sep 2019 10:31:39 +0200 Subject: [PATCH] fix(iam): validate actions (#4278) * fix(iam): validate actions Fail early if IAM actions are invalid. * fix s3 test * fix sqs test * update regex --- .../aws-ecr-assets/test/test.image-asset.ts | 4 +- .../@aws-cdk/aws-iam/lib/policy-statement.ts | 7 ++++ .../aws-iam/test/policy-document.test.ts | 42 +++++++++++++------ packages/@aws-cdk/aws-iam/test/role.test.ts | 4 +- packages/@aws-cdk/aws-s3/test/test.bucket.ts | 6 +-- packages/@aws-cdk/aws-sns/test/test.sns.ts | 8 ++-- packages/@aws-cdk/aws-sqs/test/test.sqs.ts | 6 +-- 7 files changed, 50 insertions(+), 27 deletions(-) diff --git a/packages/@aws-cdk/aws-ecr-assets/test/test.image-asset.ts b/packages/@aws-cdk/aws-ecr-assets/test/test.image-asset.ts index abc5c604f02e0..29f0458a9aaff 100644 --- a/packages/@aws-cdk/aws-ecr-assets/test/test.image-asset.ts +++ b/packages/@aws-cdk/aws-ecr-assets/test/test.image-asset.ts @@ -132,7 +132,7 @@ export = { // WHEN asset.repository.addToResourcePolicy(new iam.PolicyStatement({ - actions: ['BOOM'], + actions: ['BAM:BOOM'], principals: [new iam.ServicePrincipal('test.service')] })); @@ -144,7 +144,7 @@ export = { "PolicyDocument": { "Statement": [ { - "Action": "BOOM", + "Action": "BAM:BOOM", "Effect": "Allow", "Principal": { "Service": "test.service" diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts index e9e2c805ec0da..a26f64a313cbf 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts @@ -22,6 +22,13 @@ export class PolicyStatement { private readonly condition: { [key: string]: any } = { }; constructor(props: PolicyStatementProps = {}) { + // Validate actions + for (const action of [...props.actions || [], ...props.notActions || []]) { + if (!/^(\*|[a-zA-Z0-9-]+:[a-zA-Z0-9*]+)$/.test(action)) { + throw new Error(`Action '${action}' is invalid. An action string consists of a service namespace, a colon, and the name of an action. Action names can include wildcards.`); + } + } + this.effect = props.effect || Effect.ALLOW; this.addActions(...props.actions || []); 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 597b5d7c06cfd..83766d109cb98 100644 --- a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts +++ b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts @@ -71,12 +71,28 @@ describe('IAM polocy document', () => { test('Cannot combine Actions and NotActions', () => { expect(() => { new PolicyStatement({ - actions: ['abc'], - notActions: ['def'], + actions: ['abc:def'], + notActions: ['abc:def'], }); }).toThrow(/Cannot add 'NotActions' to policy statement if 'Actions' have been added/); }); + test('Throws with invalid actions', () => { + expect(() => { + new PolicyStatement({ + actions: ['service:action', '*', 'service:acti*', 'in:val:id'] + }); + }).toThrow(/Action 'in:val:id' is invalid/); + }); + + test('Throws with invalid not actions', () => { + expect(() => { + new PolicyStatement({ + notActions: ['service:action', '*', 'service:acti*', 'in:val:id'] + }); + }).toThrow(/Action 'in:val:id' is invalid/); + }); + test('Cannot combine Resources and NotResources', () => { expect(() => { new PolicyStatement({ @@ -229,9 +245,9 @@ describe('IAM polocy document', () => { test('statementCount returns the number of statement in the policy document', () => { const p = new PolicyDocument(); expect(p.statementCount).toEqual(0); - p.addStatements(new PolicyStatement({ actions: ['action1'] })); + p.addStatements(new PolicyStatement({ actions: ['service:action1'] })); expect(p.statementCount).toEqual(1); - p.addStatements(new PolicyStatement({ actions: ['action2'] })); + p.addStatements(new PolicyStatement({ actions: ['service:action2'] })); expect(p.statementCount).toEqual(2); }); @@ -513,19 +529,19 @@ describe('IAM polocy document', () => { }); // WHEN - doc.addStatements(new PolicyStatement({ actions: ['action1'], resources: ['resource1']})); - doc.addStatements(new PolicyStatement({ actions: ['action1'], resources: ['resource1']})); - doc.addStatements(new PolicyStatement({ actions: ['action1'], resources: ['resource1']})); - doc.addStatements(new PolicyStatement({ actions: ['action1'], resources: ['resource1']})); - doc.addStatements(new PolicyStatement({ actions: ['action2'], resources: ['resource2']})); + doc.addStatements(new PolicyStatement({ actions: ['service:action1'], resources: ['resource1']})); + doc.addStatements(new PolicyStatement({ actions: ['service:action1'], resources: ['resource1']})); + doc.addStatements(new PolicyStatement({ actions: ['service:action1'], resources: ['resource1']})); + doc.addStatements(new PolicyStatement({ actions: ['service:action1'], resources: ['resource1']})); + doc.addStatements(new PolicyStatement({ actions: ['service:action2'], resources: ['resource2']})); // THEN const stack = new Stack(); expect(stack.resolve(doc)).toEqual({ Version: '2012-10-17', Statement: [ - { Action: 'action1', Effect: 'Allow', Resource: 'resource1', Sid: '0' }, - { Action: 'action2', Effect: 'Allow', Resource: 'resource2', Sid: '1' } + { Action: 'service:action1', Effect: 'Allow', Resource: 'resource1', Sid: '0' }, + { Action: 'service:action2', Effect: 'Allow', Resource: 'resource2', Sid: '1' } ], }); }); @@ -534,7 +550,7 @@ describe('IAM polocy document', () => { const stack = new Stack(); const s = new PolicyStatement(); - s.addActions('action1', 'action2'); + s.addActions('service:action1', 'service:action2'); s.addAllResources(); s.addArnPrincipal('arn'); s.addCondition('key', { equals: 'value' }); @@ -544,7 +560,7 @@ describe('IAM polocy document', () => { const doc2 = new PolicyDocument(); doc2.addStatements(new PolicyStatement({ - actions: ['action1', 'action2'], + actions: ['service:action1', 'service:action2'], resources: ['*'], principals: [new ArnPrincipal('arn')], conditions: { diff --git a/packages/@aws-cdk/aws-iam/test/role.test.ts b/packages/@aws-cdk/aws-iam/test/role.test.ts index 9580b97556038..1aafc2e7e5f21 100644 --- a/packages/@aws-cdk/aws-iam/test/role.test.ts +++ b/packages/@aws-cdk/aws-iam/test/role.test.ts @@ -139,12 +139,12 @@ describe('IAM role', () => { // add a policy to the role const after = new Stack(); const afterRole = new Role(after, 'MyRole', { assumedBy: new ServicePrincipal('sns.amazonaws.com') }); - afterRole.addToPolicy(new PolicyStatement({ resources: ['myresource'], actions: ['myaction'] })); + afterRole.addToPolicy(new PolicyStatement({ resources: ['myresource'], actions: ['service:myaction'] })); expect(after).toHaveResource('AWS::IAM::Policy', { PolicyDocument: { Statement: [ { - Action: "myaction", + Action: "service:myaction", Effect: "Allow", Resource: "myresource" } diff --git a/packages/@aws-cdk/aws-s3/test/test.bucket.ts b/packages/@aws-cdk/aws-s3/test/test.bucket.ts index 5a93008816811..e88dfd77bb0f4 100644 --- a/packages/@aws-cdk/aws-s3/test/test.bucket.ts +++ b/packages/@aws-cdk/aws-s3/test/test.bucket.ts @@ -460,7 +460,7 @@ export = { const stack = new cdk.Stack(); const bucket = new s3.Bucket(stack, 'MyBucket', { encryption: s3.BucketEncryption.UNENCRYPTED }); - bucket.addToResourcePolicy(new iam.PolicyStatement({ resources: ['foo'], actions: [ 'bar' ]})); + bucket.addToResourcePolicy(new iam.PolicyStatement({ resources: ['foo'], actions: [ 'bar:baz' ]})); expect(stack).toMatch({ "Resources": { @@ -478,7 +478,7 @@ export = { "PolicyDocument": { "Statement": [ { - "Action": "bar", + "Action": "bar:baz", "Effect": "Allow", "Resource": "foo" } @@ -593,7 +593,7 @@ export = { const bucket = s3.Bucket.fromBucketAttributes(stack, 'ImportedBucket', { bucketArn }); // this is a no-op since the bucket is external - bucket.addToResourcePolicy(new iam.PolicyStatement({ resources: ['foo'], actions: ['bar']})); + bucket.addToResourcePolicy(new iam.PolicyStatement({ resources: ['foo'], actions: ['bar:baz']})); const p = new iam.PolicyStatement({ resources: [bucket.bucketArn], actions: ['s3:ListBucket'] }); diff --git a/packages/@aws-cdk/aws-sns/test/test.sns.ts b/packages/@aws-cdk/aws-sns/test/test.sns.ts index 2057dd3d850bd..099a2b2e472da 100644 --- a/packages/@aws-cdk/aws-sns/test/test.sns.ts +++ b/packages/@aws-cdk/aws-sns/test/test.sns.ts @@ -151,8 +151,8 @@ export = { const topic = new sns.Topic(stack, 'MyTopic'); - topic.addToResourcePolicy(new iam.PolicyStatement({ actions: ['statement0'] })); - topic.addToResourcePolicy(new iam.PolicyStatement({ actions: ['statement1'] })); + topic.addToResourcePolicy(new iam.PolicyStatement({ actions: ['service:statement0'] })); + topic.addToResourcePolicy(new iam.PolicyStatement({ actions: ['service:statement1'] })); expect(stack).toMatch({ "Resources": { @@ -165,12 +165,12 @@ export = { "PolicyDocument": { "Statement": [ { - "Action": "statement0", + "Action": "service:statement0", "Effect": "Allow", "Sid": "0" }, { - "Action": "statement1", + "Action": "service:statement1", "Effect": "Allow", "Sid": "1" } diff --git a/packages/@aws-cdk/aws-sqs/test/test.sqs.ts b/packages/@aws-cdk/aws-sqs/test/test.sqs.ts index 4a4977c713f1c..befa79ffef3a0 100644 --- a/packages/@aws-cdk/aws-sqs/test/test.sqs.ts +++ b/packages/@aws-cdk/aws-sqs/test/test.sqs.ts @@ -156,9 +156,9 @@ export = { }, 'grant() is general purpose'(test: Test) { - testGrant((q, p) => q.grant(p, 'hello', 'world'), - 'hello', - 'world' + testGrant((q, p) => q.grant(p, 'service:hello', 'service:world'), + 'service:hello', + 'service:world' ); test.done(); },