From 0e463c3ff4d23b570d57fee3254ff2e5a514a556 Mon Sep 17 00:00:00 2001 From: Robert Kenny Date: Fri, 15 Sep 2023 14:33:59 +0100 Subject: [PATCH 1/5] feat: Allow access on all indexes of a table in GuDynamoDBPolicy This change includes all table indexes in the blanket GuDynamoDBReadPolicy and GuDynamoDBWritePolicy classes. Without this consumers will see failures querying of updating items in Global Secondary Indexes. Adding read/write access to indexes of a table in these policies is intended to make it easier to use these policies by allowing access to indexes as might be expected. --- src/constructs/iam/policies/dynamodb.test.ts | 96 ++++++++++++++------ src/constructs/iam/policies/dynamodb.ts | 5 +- 2 files changed, 70 insertions(+), 31 deletions(-) diff --git a/src/constructs/iam/policies/dynamodb.test.ts b/src/constructs/iam/policies/dynamodb.test.ts index aa7b07a937..6be03aa321 100644 --- a/src/constructs/iam/policies/dynamodb.test.ts +++ b/src/constructs/iam/policies/dynamodb.test.ts @@ -21,22 +21,40 @@ describe("The GuDynamoDBReadPolicy construct", () => { "dynamodb:GetRecords", ], Effect: "Allow", - Resource: { - "Fn::Join": [ - "", - [ - "arn:aws:dynamodb:", - { - Ref: "AWS::Region", - }, - ":", - { - Ref: "AWS::AccountId", - }, - ":table/MyTable", + Resource: [ + { + "Fn::Join": [ + "", + [ + "arn:aws:dynamodb:", + { + Ref: "AWS::Region", + }, + ":", + { + Ref: "AWS::AccountId", + }, + ":table/MyTable", + ], ], - ], - }, + }, + { + "Fn::Join": [ + "", + [ + "arn:aws:dynamodb:", + { + Ref: "AWS::Region", + }, + ":", + { + Ref: "AWS::AccountId", + }, + ":table/MyTable/index/*", + ], + ], + }, + ], }, ], }, @@ -57,22 +75,40 @@ describe("The GuDynamoDBWritePolicy construct", () => { { Action: ["dynamodb:BatchWriteItem", "dynamodb:PutItem", "dynamodb:DeleteItem", "dynamodb:UpdateItem"], Effect: "Allow", - Resource: { - "Fn::Join": [ - "", - [ - "arn:aws:dynamodb:", - { - Ref: "AWS::Region", - }, - ":", - { - Ref: "AWS::AccountId", - }, - ":table/MyTable", + Resource: [ + { + "Fn::Join": [ + "", + [ + "arn:aws:dynamodb:", + { + Ref: "AWS::Region", + }, + ":", + { + Ref: "AWS::AccountId", + }, + ":table/MyTable", + ], ], - ], - }, + }, + { + "Fn::Join": [ + "", + [ + "arn:aws:dynamodb:", + { + Ref: "AWS::Region", + }, + ":", + { + Ref: "AWS::AccountId", + }, + ":table/MyTable/index/*", + ], + ], + }, + ], }, ], }, diff --git a/src/constructs/iam/policies/dynamodb.ts b/src/constructs/iam/policies/dynamodb.ts index b84b7e6b30..c607dbfad7 100644 --- a/src/constructs/iam/policies/dynamodb.ts +++ b/src/constructs/iam/policies/dynamodb.ts @@ -15,7 +15,10 @@ abstract class GuDynamoDBPolicy extends GuAllowPolicy { super(scope, id, { ...props, actions: props.actions.map((action) => `dynamodb:${action}`), - resources: [`arn:aws:dynamodb:${scope.region}:${scope.account}:table/${props.tableName}`], + resources: [ + `arn:aws:dynamodb:${scope.region}:${scope.account}:table/${props.tableName}`, + `arn:aws:dynamodb:${scope.region}:${scope.account}:table/${props.tableName}/index/*`, + ], }); } } From 0a86d828cccb81d6a765443b40c5d0769d3222ac Mon Sep 17 00:00:00 2001 From: Robert Kenny Date: Fri, 15 Sep 2023 14:58:37 +0100 Subject: [PATCH 2/5] update snapshots and associated ddb tests --- .../ecs/__snapshots__/ecs-task.test.ts.snap | 6 -- .../ec2-app/__snapshots__/base.test.ts.snap | 6 -- src/patterns/ec2-app/base.test.ts | 64 ++++++++++++------- 3 files changed, 41 insertions(+), 35 deletions(-) diff --git a/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap b/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap index 6c6bdecc71..058b979031 100644 --- a/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap +++ b/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap @@ -34,9 +34,6 @@ exports[`The GuEcsTask pattern should create the correct resources with lots of "ap-southeast-3": { "states": "states.ap-southeast-3.amazonaws.com", }, - "ap-southeast-4": { - "states": "states.ap-southeast-4.amazonaws.com", - }, "ca-central-1": { "states": "states.ca-central-1.amazonaws.com", }, @@ -70,9 +67,6 @@ exports[`The GuEcsTask pattern should create the correct resources with lots of "eu-west-3": { "states": "states.eu-west-3.amazonaws.com", }, - "il-central-1": { - "states": "states.il-central-1.amazonaws.com", - }, "me-central-1": { "states": "states.me-central-1.amazonaws.com", }, diff --git a/src/patterns/ec2-app/__snapshots__/base.test.ts.snap b/src/patterns/ec2-app/__snapshots__/base.test.ts.snap index 1866c44070..36090fe911 100644 --- a/src/patterns/ec2-app/__snapshots__/base.test.ts.snap +++ b/src/patterns/ec2-app/__snapshots__/base.test.ts.snap @@ -830,9 +830,6 @@ exports[`the GuEC2App pattern can produce a restricted EC2 app locked to specifi "Type": "AWS::EC2::SecurityGroupIngress", }, "teststackTESTtestguec2appAA7F41BE": { - "DependsOn": [ - "InstanceRoleTestguec2appC325BE42", - ], "Properties": { "LaunchTemplateData": { "IamInstanceProfile": { @@ -1672,9 +1669,6 @@ exports[`the GuEC2App pattern should produce a functional EC2 app with minimal a "Type": "AWS::EC2::SecurityGroupIngress", }, "teststackTESTtestguec2appAA7F41BE": { - "DependsOn": [ - "InstanceRoleTestguec2appC325BE42", - ], "Properties": { "LaunchTemplateData": { "IamInstanceProfile": { diff --git a/src/patterns/ec2-app/base.test.ts b/src/patterns/ec2-app/base.test.ts index ff8a61f094..65d6d9f729 100644 --- a/src/patterns/ec2-app/base.test.ts +++ b/src/patterns/ec2-app/base.test.ts @@ -311,9 +311,9 @@ describe("the GuEC2App pattern", function () { }, monitoringConfiguration: { noMonitoring: true }, userData: "", - }), + }) ).toThrowError( - "Restricted apps cannot be globally accessible. Adjust CIDR ranges (0.0.0.0/0, 1.2.3.4/32) or use Public.", + "Restricted apps cannot be globally accessible. Adjust CIDR ranges (0.0.0.0/0, 1.2.3.4/32) or use Public." ); }); @@ -335,9 +335,9 @@ describe("the GuEC2App pattern", function () { }, monitoringConfiguration: { noMonitoring: true }, userData: "", - }), + }) ).toThrowError( - "Internal apps should only be accessible on 10. ranges. Adjust CIDR ranges (93.1.2.3/12) or use Restricted.", + "Internal apps should only be accessible on 10. ranges. Adjust CIDR ranges (93.1.2.3/12) or use Restricted." ); }); @@ -406,22 +406,40 @@ describe("the GuEC2App pattern", function () { { Action: ["dynamodb:BatchWriteItem", "dynamodb:PutItem", "dynamodb:DeleteItem", "dynamodb:UpdateItem"], Effect: "Allow", - Resource: { - "Fn::Join": [ - "", - [ - "arn:aws:dynamodb:", - { - Ref: "AWS::Region", - }, - ":", - { - Ref: "AWS::AccountId", - }, - ":table/my-dynamo-table", + Resource: [ + { + "Fn::Join": [ + "", + [ + "arn:aws:dynamodb:", + { + Ref: "AWS::Region", + }, + ":", + { + Ref: "AWS::AccountId", + }, + ":table/my-dynamo-table", + ], ], - ], - }, + }, + { + "Fn::Join": [ + "", + [ + "arn:aws:dynamodb:", + { + Ref: "AWS::Region", + }, + ":", + { + Ref: "AWS::AccountId", + }, + ":table/my-dynamo-table/index/*", + ], + ], + }, + ], }, ], }, @@ -729,7 +747,7 @@ describe("the GuEC2App pattern", function () { }); }).toThrowError( "Application logging has been enabled (via the `applicationLogging` prop) but your `roleConfiguration` sets " + - "`withoutLogShipping` to true. Please turn off application logging or remove `withoutLogShipping`", + "`withoutLogShipping` to true. Please turn off application logging or remove `withoutLogShipping`" ); }); @@ -824,7 +842,7 @@ describe("the GuEC2App pattern", function () { domain, allowedGroups: [], }, - }), + }) ).toThrowError("googleAuth.allowedGroups cannot be empty!"); }); @@ -854,7 +872,7 @@ describe("the GuEC2App pattern", function () { domain, sessionTimeoutInMinutes: 61, }, - }), + }) ).toThrowError("googleAuth.sessionTimeoutInMinutes must be <= 60!"); }); @@ -884,7 +902,7 @@ describe("the GuEC2App pattern", function () { domain, allowedGroups: ["apple@guardian.co.uk", "engineering@theguardian.com"], }, - }), + }) ).toThrowError("googleAuth.allowedGroups must use the @guardian.co.uk domain."); }); }); From cea500b53d862c85fa539c668a17c391a6bdfa65 Mon Sep 17 00:00:00 2001 From: Robert Kenny Date: Fri, 15 Sep 2023 15:22:19 +0100 Subject: [PATCH 3/5] prettier --- src/patterns/ec2-app/base.test.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/patterns/ec2-app/base.test.ts b/src/patterns/ec2-app/base.test.ts index 65d6d9f729..b38cabe959 100644 --- a/src/patterns/ec2-app/base.test.ts +++ b/src/patterns/ec2-app/base.test.ts @@ -311,9 +311,9 @@ describe("the GuEC2App pattern", function () { }, monitoringConfiguration: { noMonitoring: true }, userData: "", - }) + }), ).toThrowError( - "Restricted apps cannot be globally accessible. Adjust CIDR ranges (0.0.0.0/0, 1.2.3.4/32) or use Public." + "Restricted apps cannot be globally accessible. Adjust CIDR ranges (0.0.0.0/0, 1.2.3.4/32) or use Public.", ); }); @@ -335,9 +335,9 @@ describe("the GuEC2App pattern", function () { }, monitoringConfiguration: { noMonitoring: true }, userData: "", - }) + }), ).toThrowError( - "Internal apps should only be accessible on 10. ranges. Adjust CIDR ranges (93.1.2.3/12) or use Restricted." + "Internal apps should only be accessible on 10. ranges. Adjust CIDR ranges (93.1.2.3/12) or use Restricted.", ); }); @@ -747,7 +747,7 @@ describe("the GuEC2App pattern", function () { }); }).toThrowError( "Application logging has been enabled (via the `applicationLogging` prop) but your `roleConfiguration` sets " + - "`withoutLogShipping` to true. Please turn off application logging or remove `withoutLogShipping`" + "`withoutLogShipping` to true. Please turn off application logging or remove `withoutLogShipping`", ); }); @@ -842,7 +842,7 @@ describe("the GuEC2App pattern", function () { domain, allowedGroups: [], }, - }) + }), ).toThrowError("googleAuth.allowedGroups cannot be empty!"); }); @@ -872,7 +872,7 @@ describe("the GuEC2App pattern", function () { domain, sessionTimeoutInMinutes: 61, }, - }) + }), ).toThrowError("googleAuth.sessionTimeoutInMinutes must be <= 60!"); }); @@ -902,7 +902,7 @@ describe("the GuEC2App pattern", function () { domain, allowedGroups: ["apple@guardian.co.uk", "engineering@theguardian.com"], }, - }) + }), ).toThrowError("googleAuth.allowedGroups must use the @guardian.co.uk domain."); }); }); From af1d6e7edc17437b2febedeb9e723af5c4048c01 Mon Sep 17 00:00:00 2001 From: Robert Kenny Date: Fri, 15 Sep 2023 15:26:44 +0100 Subject: [PATCH 4/5] snapshot test updates --- src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap | 6 ++++++ src/patterns/ec2-app/__snapshots__/base.test.ts.snap | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap b/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap index 058b979031..6c6bdecc71 100644 --- a/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap +++ b/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap @@ -34,6 +34,9 @@ exports[`The GuEcsTask pattern should create the correct resources with lots of "ap-southeast-3": { "states": "states.ap-southeast-3.amazonaws.com", }, + "ap-southeast-4": { + "states": "states.ap-southeast-4.amazonaws.com", + }, "ca-central-1": { "states": "states.ca-central-1.amazonaws.com", }, @@ -67,6 +70,9 @@ exports[`The GuEcsTask pattern should create the correct resources with lots of "eu-west-3": { "states": "states.eu-west-3.amazonaws.com", }, + "il-central-1": { + "states": "states.il-central-1.amazonaws.com", + }, "me-central-1": { "states": "states.me-central-1.amazonaws.com", }, diff --git a/src/patterns/ec2-app/__snapshots__/base.test.ts.snap b/src/patterns/ec2-app/__snapshots__/base.test.ts.snap index 36090fe911..1866c44070 100644 --- a/src/patterns/ec2-app/__snapshots__/base.test.ts.snap +++ b/src/patterns/ec2-app/__snapshots__/base.test.ts.snap @@ -830,6 +830,9 @@ exports[`the GuEC2App pattern can produce a restricted EC2 app locked to specifi "Type": "AWS::EC2::SecurityGroupIngress", }, "teststackTESTtestguec2appAA7F41BE": { + "DependsOn": [ + "InstanceRoleTestguec2appC325BE42", + ], "Properties": { "LaunchTemplateData": { "IamInstanceProfile": { @@ -1669,6 +1672,9 @@ exports[`the GuEC2App pattern should produce a functional EC2 app with minimal a "Type": "AWS::EC2::SecurityGroupIngress", }, "teststackTESTtestguec2appAA7F41BE": { + "DependsOn": [ + "InstanceRoleTestguec2appC325BE42", + ], "Properties": { "LaunchTemplateData": { "IamInstanceProfile": { From bfffa245fb213d07727388fe1ea68055b3b290ed Mon Sep 17 00:00:00 2001 From: Robert Kenny Date: Mon, 18 Sep 2023 12:38:59 +0100 Subject: [PATCH 5/5] add comment on unsupported actions with multiple resources --- src/constructs/iam/policies/dynamodb.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/constructs/iam/policies/dynamodb.ts b/src/constructs/iam/policies/dynamodb.ts index c607dbfad7..87c70e960c 100644 --- a/src/constructs/iam/policies/dynamodb.ts +++ b/src/constructs/iam/policies/dynamodb.ts @@ -15,6 +15,9 @@ abstract class GuDynamoDBPolicy extends GuAllowPolicy { super(scope, id, { ...props, actions: props.actions.map((action) => `dynamodb:${action}`), + // Note: although the index resource is not supported for all attached actions + // (e.g. BatchWriteItem), it will not cause issues to include it here as it is ignored. + // See: https://docs.aws.amazon.com/service-authorization/latest/reference/reference_policies_actions-resources-contextkeys.html resources: [ `arn:aws:dynamodb:${scope.region}:${scope.account}:table/${props.tableName}`, `arn:aws:dynamodb:${scope.region}:${scope.account}:table/${props.tableName}/index/*`,