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 · Sep 30, 2019 · 3917c4b · 3917c4b
1 parent f2a6d46
commit 3917c4b
Show file tree

Hide file tree

Showing 7 changed files with 50 additions and 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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();
     },