Skip to content

Commit 3d99b3a

Browse files
anthony-nhsanthony-nhs
authored
Chore: [AEA-0000] - review security (#296)
## Summary - Routine Change ### Details - setup audit logging bucket - remove slack commands endpoint - restrict kms key policy - remove access to secrets from slackbot - remove unused bedrock policies - only add direct lambda policy if needed for regression test - restrict nag suppressions
1 parent 3f8b82c commit 3d99b3a

12 files changed

Lines changed: 162 additions & 172 deletions

File tree

‎.github/scripts/fix_cdk_json.sh‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,3 +62,4 @@ fix_string_key slackBotToken "${SLACK_BOT_TOKEN}"
6262
fix_string_key slackSigningSecret "${SLACK_SIGNING_SECRET}"
6363
fix_string_key cfnDriftDetectionGroup "${CFN_DRIFT_DETECTION_GROUP}"
6464
fix_boolean_number_key isPullRequest "${IS_PULL_REQUEST}"
65+
fix_boolean_number_key runRegressionTests "${RUN_REGRESSION_TESTS}"

‎.github/workflows/release_all_stacks.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@ jobs:
149149
SLACK_SIGNING_SECRET: "${{ secrets.SLACK_SIGNING_SECRET }}"
150150
CDK_APP_NAME: ${{ inputs.CDK_APP_NAME }}
151151
IS_PULL_REQUEST: ${{ inputs.IS_PULL_REQUEST }}
152+
RUN_REGRESSION_TESTS: ${{ inputs.RUN_REGRESSION_TESTS }}
152153

153154
- name: Show diff for stack
154155
run: |

‎Makefile‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,10 @@ cdk-deploy: guard-STACK_NAME
102102
--context logRetentionInDays=$$LOG_RETENTION_IN_DAYS \
103103
--context slackBotToken=$$SLACK_BOT_TOKEN \
104104
--context slackSigningSecret=$$SLACK_SIGNING_SECRET
105-
cdk-synth:
105+
106+
cdk-synth: cdk-synth-pr cdk-synth-non-pr
107+
108+
cdk-synth-non-pr:
106109
mkdir -p .dependencies/slackBotFunction
107110
mkdir -p .dependencies/syncKnowledgeBaseFunction
108111
mkdir -p .dependencies/bedrockLoggingConfigFunction
@@ -114,6 +117,25 @@ cdk-synth:
114117
SLACK_SIGNING_SECRET=dummy_secret \
115118
LOG_RETENTION_IN_DAYS=30 \
116119
LOG_LEVEL=debug \
120+
RUN_REGRESSION_TESTS=true \
121+
./.github/scripts/fix_cdk_json.sh .local_config/epsam.config.json
122+
CONFIG_FILE_NAME=.local_config/epsam.config.json npx cdk synth \
123+
--quiet \
124+
--app "npx ts-node --prefer-ts-exts packages/cdk/bin/EpsAssistMeApp.ts"
125+
126+
cdk-synth-pr:
127+
mkdir -p .dependencies/slackBotFunction
128+
mkdir -p .dependencies/syncKnowledgeBaseFunction
129+
mkdir -p .dependencies/bedrockLoggingConfigFunction
130+
mkdir -p .local_config
131+
STACK_NAME=epsam-pr-123 \
132+
COMMIT_ID=undefined \
133+
VERSION_NUMBER=undefined \
134+
SLACK_BOT_TOKEN=dummy_token \
135+
SLACK_SIGNING_SECRET=dummy_secret \
136+
LOG_RETENTION_IN_DAYS=30 \
137+
LOG_LEVEL=debug \
138+
RUN_REGRESSION_TESTS=true \
117139
./.github/scripts/fix_cdk_json.sh .local_config/epsam.config.json
118140
CONFIG_FILE_NAME=.local_config/epsam.config.json npx cdk synth \
119141
--quiet \

‎packages/cdk/constructs/S3Bucket.ts‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@ import {
66
BlockPublicAccess,
77
ObjectOwnership,
88
CfnBucket,
9-
CfnBucketPolicy
9+
CfnBucketPolicy,
10+
IBucket
1011
} from "aws-cdk-lib/aws-s3"
1112
import {CfnKey, Key} from "aws-cdk-lib/aws-kms"
1213
import {
@@ -21,6 +22,7 @@ export interface S3BucketProps {
2122
readonly bucketName: string
2223
readonly versioned: boolean
2324
readonly deploymentRole: IPrincipal
25+
readonly auditLoggingBucket: IBucket
2426
}
2527

2628
export class S3Bucket extends Construct {
@@ -40,12 +42,14 @@ export class S3Bucket extends Construct {
4042
const bucket = new Bucket(this, props.bucketName, {
4143
blockPublicAccess: BlockPublicAccess.BLOCK_ALL,
4244
encryption: BucketEncryption.KMS,
43-
encryptionKey: this.kmsKey,
45+
encryptionKey: kmsKey,
4446
removalPolicy: RemovalPolicy.DESTROY,
4547
autoDeleteObjects: true,
4648
enforceSSL: true,
4749
versioned: props.versioned ?? false,
48-
objectOwnership: ObjectOwnership.BUCKET_OWNER_ENFORCED
50+
objectOwnership: ObjectOwnership.BUCKET_OWNER_ENFORCED,
51+
serverAccessLogsBucket: props.auditLoggingBucket,
52+
serverAccessLogsPrefix: "epsam-kb"
4953
})
5054

5155
// Adding full deployment roles to this bucket
@@ -83,7 +87,7 @@ export class S3Bucket extends Construct {
8387
principals: [props.deploymentRole],
8488
actions: [
8589
"kms:Encrypt",
86-
"kms:GenerateDataKey*"
90+
"kms:GenerateDataKey"
8791
],
8892
resources:["*"]
8993
})

‎packages/cdk/nagSuppressions.ts‎

Lines changed: 57 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,7 @@
33
import {Stack} from "aws-cdk-lib"
44
import {NagPackSuppression, NagSuppressions} from "cdk-nag"
55

6-
export const nagSuppressions = (stack: Stack) => {
7-
const stackName = stack.node.tryGetContext("stackName") || "epsam"
6+
export const nagSuppressions = (stack: Stack, account: string) => {
87
// Suppress granular wildcard on log stream for SlackBot Lambda
98
safeAddNagSuppression(
109
stack,
@@ -57,22 +56,6 @@ export const nagSuppressions = (stack: Stack) => {
5756
]
5857
)
5958

60-
// Suppress unauthenticated API route warnings
61-
safeAddNagSuppression(
62-
stack,
63-
"/EpsAssistMeStack/Apis/EpsAssistApiGateway/ApiGateway/Default/slack/commands/POST/Resource",
64-
[
65-
{
66-
id: "AwsSolutions-APIG4",
67-
reason: "Slack command endpoint is intentionally unauthenticated."
68-
},
69-
{
70-
id: "AwsSolutions-COG4",
71-
reason: "Cognito not required for this public endpoint."
72-
}
73-
]
74-
)
75-
7659
// Suppress missing WAF on API stage for Apis construct
7760
safeAddNagSuppression(
7861
stack,
@@ -92,31 +75,49 @@ export const nagSuppressions = (stack: Stack) => {
9275
[
9376
{
9477
id: "AwsSolutions-IAM5",
95-
reason: "Bedrock Knowledge Base requires these permissions to access S3 documents and OpenSearch collection."
78+
reason: "Bedrock Knowledge Base requires these permissions to access S3 documents and OpenSearch collection.",
79+
appliesTo: [
80+
"Action::bedrock:Delete*",
81+
"Resource::arn:aws:bedrock:eu-west-2:<AWS::AccountId>:knowledge-base/*",
82+
"Resource::arn:aws:aoss:eu-west-2:<AWS::AccountId>:collection/*",
83+
"Resource::arn:aws:logs:eu-west-2:<AWS::AccountId>:delivery-destination:*",
84+
"Resource::arn:aws:logs:eu-west-2:<AWS::AccountId>:delivery-source:*",
85+
"Resource::arn:aws:logs:eu-west-2:<AWS::AccountId>:delivery:*",
86+
`Resource::arn:aws:bedrock:eu-west-2:${account}:knowledge-base/*`,
87+
`Resource::arn:aws:aoss:eu-west-2:${account}:collection/*`,
88+
`Resource::arn:aws:logs:eu-west-2:${account}:delivery-destination:*`,
89+
`Resource::arn:aws:logs:eu-west-2:${account}:delivery-source:*`,
90+
`Resource::arn:aws:logs:eu-west-2:${account}:delivery:*`
91+
]
9692
}
9793
]
9894
)
99-
100-
// Suppress wildcard permissions for SlackBot policy
10195
safeAddNagSuppression(
10296
stack,
103-
"/EpsAssistMeStack/RuntimePolicies/SlackBotPolicy/Resource",
97+
"/EpsAssistMeStack/BedrockExecutionRole/WildcardPolicy/Resource",
10498
[
10599
{
106100
id: "AwsSolutions-IAM5",
107-
reason: "SlackBot Lambda needs wildcard permissions for guardrails, knowledge bases, and function invocation."
101+
reason: "Bedrock Knowledge Base requires these wildcard permissions to access S3 documents and OpenSearch collection."
108102
}
109103
]
110104
)
111105

112-
// Suppress S3 server access logs for knowledge base documents bucket
106+
// Suppress wildcard permissions for SlackBot policy
113107
safeAddNagSuppression(
114108
stack,
115-
`/EpsAssistMeStack/Storage/DocsBucket/${stackName}-Docs/Resource`,
109+
"/EpsAssistMeStack/RuntimePolicies/SlackBotPolicy/Resource",
116110
[
117111
{
118-
id: "AwsSolutions-S1",
119-
reason: "Server access logging not required for knowledge base documents bucket."
112+
id: "AwsSolutions-IAM5",
113+
reason: "SlackBot Lambda needs wildcard permissions for guardrails, knowledge bases, and function invocation.",
114+
appliesTo: [
115+
"Resource::arn:aws:lambda:eu-west-2:<AWS::AccountId>:function:epsam*",
116+
"Resource::arn:aws:cloudformation:eu-west-2:<AWS::AccountId>:stack/epsam-pr-*",
117+
`Resource::arn:aws:lambda:eu-west-2:${account}:function:epsam*`,
118+
`Resource::arn:aws:cloudformation:eu-west-2:${account}:stack/epsam-pr-*`,
119+
"Resource::arn:aws:bedrock:*"
120+
]
120121
}
121122
]
122123
)
@@ -136,17 +137,6 @@ export const nagSuppressions = (stack: Stack) => {
136137
]
137138
)
138139

139-
safeAddNagSuppression(
140-
stack,
141-
"/EpsAssistMeStack/Secrets/SlackBotSigning/Secret/Resource",
142-
[
143-
{
144-
id: "AwsSolutions-SMG4",
145-
reason: "Slack signing secret rotation is handled manually as part of the Slack app configuration process."
146-
}
147-
]
148-
)
149-
150140
// Suppress AWS managed policy usage in BucketNotificationsHandler (wildcard for any hash)
151141
const bucketNotificationHandlers = stack.node.findAll().filter(node =>
152142
node.node.id.startsWith("BucketNotificationsHandler")
@@ -159,47 +149,8 @@ export const nagSuppressions = (stack: Stack) => {
159149
[
160150
{
161151
id: "AwsSolutions-IAM4",
162-
reason: "Auto-generated CDK role uses AWS managed policy for basic Lambda execution."
163-
}
164-
]
165-
)
166-
167-
safeAddNagSuppression(
168-
stack,
169-
`${handler.node.path}/Role/DefaultPolicy/Resource`,
170-
[
171-
{
172-
id: "AwsSolutions-IAM5",
173-
reason: "Auto-generated CDK role requires wildcard permissions for S3 bucket notifications."
174-
}
175-
]
176-
)
177-
})
178-
179-
const logRetentionHandlers = stack.node.findAll().filter(node =>
180-
node.node.id.startsWith("LogRetention") &&
181-
!node.node.path.includes("DelayProvider")
182-
)
183-
184-
logRetentionHandlers.forEach(handler => {
185-
safeAddNagSuppression(
186-
stack,
187-
`${handler.node.path}/ServiceRole/Resource`,
188-
[
189-
{
190-
id: "AwsSolutions-IAM4",
191-
reason: "Auto-generated CDK log retention role uses AWS managed policy for basic Lambda execution."
192-
}
193-
]
194-
)
195-
196-
safeAddNagSuppression(
197-
stack,
198-
`${handler.node.path}/ServiceRole/DefaultPolicy/Resource`,
199-
[
200-
{
201-
id: "AwsSolutions-IAM5",
202-
reason: "Auto-generated CDK log retention role requires wildcard permissions for log management."
152+
reason: "Auto-generated CDK role uses AWS managed policy for basic Lambda execution.",
153+
appliesTo: ["Policy::arn:<AWS::Partition>:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"]
203154
}
204155
]
205156
)
@@ -212,7 +163,8 @@ export const nagSuppressions = (stack: Stack) => {
212163
[
213164
{
214165
id: "AwsSolutions-IAM4",
215-
reason: "DelayResource Lambda uses AWS managed policy for basic Lambda execution role."
166+
reason: "DelayResource Lambda uses AWS managed policy for basic Lambda execution role.",
167+
appliesTo: ["Policy::arn:<AWS::Partition>:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"]
216168
}
217169
]
218170
)
@@ -223,7 +175,8 @@ export const nagSuppressions = (stack: Stack) => {
223175
[
224176
{
225177
id: "AwsSolutions-IAM4",
226-
reason: "DelayResource Lambda uses AWS managed policy for basic Lambda execution role."
178+
reason: "DelayResource Lambda uses AWS managed policy for basic Lambda execution role.",
179+
appliesTo: ["Policy::arn:<AWS::Partition>:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"]
227180
}
228181
]
229182
)
@@ -235,7 +188,8 @@ export const nagSuppressions = (stack: Stack) => {
235188
[
236189
{
237190
id: "AwsSolutions-IAM4",
238-
reason: "Auto-generated CDK Provider role uses AWS managed policy for Lambda execution."
191+
reason: "Auto-generated CDK Provider role uses AWS managed policy for Lambda execution.",
192+
appliesTo: ["Policy::arn:<AWS::Partition>:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"]
239193
}
240194
]
241195
)
@@ -246,7 +200,8 @@ export const nagSuppressions = (stack: Stack) => {
246200
[
247201
{
248202
id: "AwsSolutions-IAM5",
249-
reason: "Auto-generated CDK Provider role requires wildcard permissions for Lambda invocation."
203+
reason: "Auto-generated CDK Provider role requires wildcard permissions for Lambda invocation.",
204+
appliesTo: ["Resource::<VectorIndexPolicySyncWaitDelayFunctionBDE3D308.Arn>:*"]
250205
}
251206
]
252207
)
@@ -257,7 +212,8 @@ export const nagSuppressions = (stack: Stack) => {
257212
[
258213
{
259214
id: "AwsSolutions-IAM4",
260-
reason: "Auto-generated CDK Provider role uses AWS managed policy for Lambda execution."
215+
reason: "Auto-generated CDK Provider role uses AWS managed policy for Lambda execution.",
216+
appliesTo: ["Policy::arn:<AWS::Partition>:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"]
261217
}
262218
]
263219
)
@@ -268,7 +224,8 @@ export const nagSuppressions = (stack: Stack) => {
268224
[
269225
{
270226
id: "AwsSolutions-IAM5",
271-
reason: "Auto-generated CDK Provider role requires wildcard permissions for Lambda invocation."
227+
reason: "Auto-generated CDK Provider role requires wildcard permissions for Lambda invocation.",
228+
appliesTo: ["Resource::<VectorIndexIndexReadyWaitDelayFunction56EB971B.Arn>:*"]
272229
}
273230
]
274231
)
@@ -302,7 +259,11 @@ export const nagSuppressions = (stack: Stack) => {
302259
[
303260
{
304261
id: "AwsSolutions-IAM5",
305-
reason: "Auto-generated CDK Provider role requires wildcard permissions for cloudformation stack listing."
262+
reason: "Auto-generated CDK Provider role requires wildcard permissions for cloudformation stack listing.",
263+
appliesTo: [
264+
"Resource::arn:aws:cloudformation:eu-west-2:<AWS::AccountId>:stack/epsam*",
265+
`Resource::arn:aws:cloudformation:eu-west-2:${account}:stack/epsam*`
266+
]
306267
}
307268
]
308269
)
@@ -338,7 +299,12 @@ export const nagSuppressions = (stack: Stack) => {
338299
[
339300
{
340301
id: "AwsSolutions-IAM5",
341-
reason: "KMS wildcard permissions (GenerateDataKey*, ReEncrypt*) are required for CloudWatch Logs encryption operations."
302+
reason: "KMS wildcard permissions (GenerateDataKey*, ReEncrypt*) are required for CloudWatch Logs encryption operations.",
303+
appliesTo: [
304+
"Action::kms:GenerateDataKey*",
305+
"Action::kms:ReEncrypt*",
306+
"Resource::<BedrockLoggingModelInvocationLogGroupC58FAE2D.Arn>:*"
307+
]
342308
}
343309
]
344310
)
@@ -374,7 +340,10 @@ export const nagSuppressions = (stack: Stack) => {
374340
[
375341
{
376342
id: "AwsSolutions-IAM4",
377-
reason: "Auto-generated CDK Provider role uses AWS managed policy for Lambda execution."
343+
reason: "Auto-generated CDK Provider role uses AWS managed policy for Lambda execution.",
344+
appliesTo: [
345+
"Policy::arn:<AWS::Partition>:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"
346+
]
378347
}
379348
]
380349
)

‎packages/cdk/resources/Apis.ts‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -37,17 +37,6 @@ export class Apis extends Construct {
3737
lambdaFunction: props.functions.slackBot
3838
})
3939

40-
// Create the '/slack/commands' POST endpoint for Slack Events API
41-
// This endpoint will handle slash commands, such as /test
42-
// eslint-disable-next-line @typescript-eslint/no-unused-vars
43-
const slackCommandsEndpoint = new LambdaEndpoint(this, "SlackCommandsEndpoint", {
44-
parentResource: slackResource,
45-
resourceName: "commands",
46-
method: HttpMethod.POST,
47-
restApiGatewayRole: apiGateway.role,
48-
lambdaFunction: props.functions.slackBot
49-
})
50-
5140
this.apis = {
5241
api: apiGateway
5342
}

0 commit comments

Comments
 (0)