Merge pull request #118 from NHSDigital/feature/eja-eli-238-address-checkov-flagged-issues

eddalmond1 · web-flow · commit a916061816d2 · 2025-06-05T17:21:03.000+01:00
Feature/eja eli 238 address checkov flagged issues
diff --git a/.github/workflows/stage-1-commit.yaml b/.github/workflows/stage-1-commit.yaml
@@ -97,9 +97,14 @@ jobs:
         uses: bridgecrewio/checkov-action@v12
         with:
           directory: infrastructure/
-          soft_fail: true
+          soft_fail: false
           output_format: sarif
           output_file_path: checkov-report.sarif
+      - name: Upload Checkov results to GitHub Security tab
+        uses: actions/upload-artifact@v4
+        with:
+          name: checkov_results
+          path: checkov-report.sarif
   count-lines-of-code:
     name: "Count lines of code"
     runs-on: ubuntu-latest
diff --git a/infrastructure/modules/api_gateway/cloudwatch.tf b/infrastructure/modules/api_gateway/cloudwatch.tf
@@ -1,6 +1,6 @@
 resource "aws_cloudwatch_log_group" "api_gateway" {
   name              = "/aws/apigateway/${var.workspace}-${var.api_gateway_name}"
-  retention_in_days = 14
+  retention_in_days = 365
   tags              = var.tags
   kms_key_id        = aws_kms_key.api_gateway.arn
 
diff --git a/infrastructure/modules/api_gateway/iam.tf b/infrastructure/modules/api_gateway/iam.tf
@@ -27,7 +27,7 @@ data "aws_iam_policy_document" "api_gateway_logging" {
       "logs:GetLogEvents",
       "logs:FilterLogEvents"
     ]
-    resources = ["*"]
+    resources = [aws_cloudwatch_log_group.api_gateway.arn]
   }
 }
 
diff --git a/infrastructure/modules/api_gateway/outputs.tf b/infrastructure/modules/api_gateway/outputs.tf
@@ -25,3 +25,7 @@ output "logging_policy_attachment" {
 output "iam_role_name" {
   value = aws_iam_role.api_gateway.name
 }
+
+output "kms_key_arn" {
+  value = aws_kms_key.api_gateway.arn
+}
diff --git a/infrastructure/modules/bootstrap/tfstate/data.tf b/infrastructure/modules/bootstrap/tfstate/data.tf
@@ -0,0 +1 @@
+data "aws_caller_identity" "current" {}
diff --git a/infrastructure/modules/bootstrap/tfstate/kms.tf b/infrastructure/modules/bootstrap/tfstate/kms.tf
@@ -10,3 +10,21 @@ resource "aws_kms_alias" "terraform_state_bucket_cmk" {
   name          = "alias/${var.project_name}-tfstate_bucket_cmk"
   target_key_id = aws_kms_key.terraform_state_bucket_cmk.key_id
 }
+
+resource "aws_kms_key_policy" "terraform_state_bucket_cmk" {
+  key_id = aws_kms_key.terraform_state_bucket_cmk.id
+  policy = data.aws_iam_policy_document.terraform_state_bucket_cmk.json
+}
+
+data "aws_iam_policy_document" "terraform_state_bucket_cmk" {
+  statement {
+    sid    = "Enable IAM User Permissions for s3 buckets"
+    effect = "Allow"
+    principals {
+      type        = "AWS"
+      identifiers = ["arn:aws:iam::${data.aws_caller_identity.current.account_id}:root"]
+    }
+    actions   = ["kms:*"]
+    resources = [aws_kms_key.terraform_state_bucket_cmk.arn]
+  }
+}
diff --git a/infrastructure/modules/bootstrap/tfstate/s3.tf b/infrastructure/modules/bootstrap/tfstate/s3.tf
@@ -1,5 +1,7 @@
 # Main state bucket
 resource "aws_s3_bucket" "tfstate_bucket" {
+  #checkov:skip=CKV_AWS_144: We don't want to replicate outside our region
+  #checkov:skip=CKV2_AWS_62: We won't enable event notifications for this bucket, yet
   bucket = "${var.project_name}-${var.environment}-tfstate"
   tags = {
     Stack = "Bootstrap"
@@ -95,6 +97,9 @@ resource "aws_s3_bucket_lifecycle_configuration" "tfstate_bucket" {
 # Logging
 
 resource "aws_s3_bucket" "tfstate_s3_access_logs" {
+  #checkov:skip=CKV_AWS_144: We don't want to replicate outside our region
+  #checkov:skip=CKV2_AWS_62: We won't enable event notifications for this bucket, yet
+  #checkov:skip=CKV_AWS_21: Versioning not needed given short lifecycle of logs
   bucket = "${var.project_name}-${var.environment}-tfstate-access-logs"
 }
 
@@ -109,7 +114,8 @@ resource "aws_s3_bucket_server_side_encryption_configuration" "tfstate_s3_access
 
   rule {
     apply_server_side_encryption_by_default {
-      sse_algorithm = "AES256"
+      sse_algorithm     = "aws:kms"
+      kms_master_key_id = aws_kms_key.terraform_state_bucket_cmk.arn
     }
   }
 }
@@ -131,6 +137,16 @@ resource "aws_s3_bucket_lifecycle_configuration" "tfstate_s3_access_logs_object_
       noncurrent_days = var.log_retention_in_days
     }
   }
+  rule {
+    id     = "StateBucketLogsMultipartUploadExpiration"
+    status = "Enabled"
+    filter {
+      prefix = ""
+    }
+    abort_incomplete_multipart_upload {
+      days_after_initiation = 7
+    }
+  }
 }
 
 resource "aws_s3_bucket_public_access_block" "s3logs" {
diff --git a/infrastructure/modules/dynamodb/data.tf b/infrastructure/modules/dynamodb/data.tf
@@ -0,0 +1 @@
+data "aws_caller_identity" "current" {}
diff --git a/infrastructure/modules/dynamodb/dynamodb.tf b/infrastructure/modules/dynamodb/dynamodb.tf
@@ -23,5 +23,10 @@ resource "aws_dynamodb_table" "dynamodb_table" {
     kms_key_arn = aws_kms_key.dynamodb_cmk.arn
   }
 
+  #checkov:skip=CKV_AWS_28: Point-in-time recovery is enabled only for production environments
+  point_in_time_recovery {
+    enabled = var.environment == "prod"
+  }
+
   tags = var.tags
 }
diff --git a/infrastructure/modules/lambda/data.tf b/infrastructure/modules/lambda/data.tf
@@ -0,0 +1 @@
+data "aws_caller_identity" "current" {}
diff --git a/infrastructure/modules/lambda/kms.tf b/infrastructure/modules/lambda/kms.tf
@@ -10,3 +10,21 @@ resource "aws_kms_alias" "lambda_cmk" {
   name          = "alias/${terraform.workspace == "default" ? "" : "${terraform.workspace}-"}${var.lambda_func_name}-cmk"
   target_key_id = aws_kms_key.lambda_cmk.key_id
 }
+
+resource "aws_kms_key_policy" "lambda_cmk" {
+  key_id = aws_kms_key.lambda_cmk.id
+  policy = data.aws_iam_policy_document.lambda_cmk.json
+}
+
+data "aws_iam_policy_document" "lambda_cmk" {
+  statement {
+    sid    = "Enable IAM User Permissions for s3 buckets"
+    effect = "Allow"
+    principals {
+      type        = "AWS"
+      identifiers = ["arn:aws:iam::${data.aws_caller_identity.current.account_id}:root"]
+    }
+    actions   = ["kms:*"]
+    resources = [aws_kms_key.lambda_cmk.arn]
+  }
+}
diff --git a/infrastructure/modules/lambda/lambda.tf b/infrastructure/modules/lambda/lambda.tf
@@ -1,4 +1,7 @@
 resource "aws_lambda_function" "eligibility_signposting_lambda" {
+  #checkov:skip=CKV_AWS_116: No deadletter queue is configured for this Lambda function, yet
+  #checkov:skip=CKV_AWS_115: Concurrent execution limit will be set at APIM level, not at Lambda level
+  #checkov:skip=CKV_AWS_272: Skipping code signing but flagged to create ticket to investigate on ELI-238
   # If the file is not in the current working directory you will need to include a
   # path.module in the filename.
   filename      = var.file_name
@@ -19,8 +22,15 @@ resource "aws_lambda_function" "eligibility_signposting_lambda" {
       ENV               = var.environment
     }
   }
+
+  kms_key_arn = aws_kms_key.lambda_cmk.arn
+
   vpc_config {
     subnet_ids         = var.vpc_intra_subnets
     security_group_ids = var.security_group_ids
   }
+
+  tracing_config {
+    mode = "Active"
+  }
 }
diff --git a/infrastructure/modules/lambda/outputs.tf b/infrastructure/modules/lambda/outputs.tf
@@ -12,3 +12,7 @@ output "aws_lambda_function_name" {
 output "aws_lambda_invoke_arn" {
   value = aws_lambda_function.eligibility_signposting_lambda.invoke_arn
 }
+
+output "lambda_cmk_arn" {
+  value = aws_kms_key.lambda_cmk.arn
+}
diff --git a/infrastructure/modules/s3/data.tf b/infrastructure/modules/s3/data.tf
@@ -0,0 +1 @@
+data "aws_caller_identity" "current" {}
diff --git a/infrastructure/modules/s3/kms.tf b/infrastructure/modules/s3/kms.tf
@@ -9,3 +9,21 @@ resource "aws_kms_alias" "storage_bucket_cmk" {
   name          = "alias/${terraform.workspace == "default" ? "" : "${terraform.workspace}-"}${var.bucket_name}-cmk"
   target_key_id = aws_kms_key.storage_bucket_cmk.key_id
 }
+
+resource "aws_kms_key_policy" "storage_bucket_cmk" {
+  key_id = aws_kms_key.storage_bucket_cmk.id
+  policy = data.aws_iam_policy_document.storage_bucket_cmk.json
+}
+
+data "aws_iam_policy_document" "storage_bucket_cmk" {
+  statement {
+    sid    = "Enable IAM User Permissions for s3 buckets"
+    effect = "Allow"
+    principals {
+      type        = "AWS"
+      identifiers = ["arn:aws:iam::${data.aws_caller_identity.current.account_id}:root"]
+    }
+    actions   = ["kms:*"]
+    resources = [aws_kms_key.storage_bucket_cmk.arn]
+  }
+}
diff --git a/infrastructure/modules/s3/outputs.tf b/infrastructure/modules/s3/outputs.tf
@@ -17,3 +17,7 @@ output "storage_bucket_name" {
 output "storage_bucket_id" {
   value = aws_s3_bucket.storage_bucket.id
 }
+
+output "storage_bucket_kms_key_arn" {
+  value = aws_kms_key.storage_bucket_cmk.arn
+}
diff --git a/infrastructure/modules/s3/s3.tf b/infrastructure/modules/s3/s3.tf
@@ -1,6 +1,8 @@
 # s3
 # Define bucket
 resource "aws_s3_bucket" "storage_bucket" {
+  #checkov:skip=CKV_AWS_144: We don't want to replicate outside our region
+  #checkov:skip=CKV2_AWS_62: We won't enable event notifications for this bucket, yet
   bucket = "${terraform.workspace == "default" ? "" : "${terraform.workspace}-"}${var.project_name}-${var.environment}-${var.bucket_name}"
 }
 
@@ -63,6 +65,9 @@ resource "aws_s3_bucket_lifecycle_configuration" "storage_bucket" {
 
 #same again for logging buckets
 resource "aws_s3_bucket" "storage_bucket_access_logs" {
+  #checkov:skip=CKV_AWS_144: We don't want to replicate outside our region
+  #checkov:skip=CKV2_AWS_62: We won't enable event notifications for this bucket, yet
+  #checkov:skip=CKV_AWS_21: Versioning not needed given short lifecycle of logs
   bucket = "${terraform.workspace == "default" ? "" : "${terraform.workspace}-"}${var.project_name}-${var.environment}-${var.bucket_name}-access-logs"
 }
 
@@ -77,7 +82,8 @@ resource "aws_s3_bucket_server_side_encryption_configuration" "storage_bucket_ac
 
   rule {
     apply_server_side_encryption_by_default {
-      sse_algorithm = "AES256"
+      sse_algorithm = "aws:kms"
+      kms_master_key_id = aws_kms_key.storage_bucket_cmk.arn
     }
   }
 }
@@ -99,6 +105,17 @@ resource "aws_s3_bucket_lifecycle_configuration" "storage_bucket_access_logs_obj
       noncurrent_days = var.log_retention_in_days
     }
   }
+  rule {
+    id     = "StorageBucketLogsMultipartUploadExpiration"
+    status = "Enabled"
+
+    filter {
+      prefix = ""
+    }
+    abort_incomplete_multipart_upload {
+      days_after_initiation = 7
+    }
+  }
 }
 
 resource "aws_s3_bucket_public_access_block" "storage_bucket_access_logs_public_access_block" {
@@ -109,5 +126,3 @@ resource "aws_s3_bucket_public_access_block" "storage_bucket_access_logs_public_
   ignore_public_acls      = true
   restrict_public_buckets = true
 }
-
-
diff --git a/infrastructure/stacks/api-layer/api_gateway.tf b/infrastructure/stacks/api-layer/api_gateway.tf
@@ -44,6 +44,8 @@ resource "aws_api_gateway_deployment" "eligibility_signposting_api" {
 }
 
 resource "aws_api_gateway_stage" "eligibility-signposting-api" {
+  #checkov:skip=CKV2_AWS_51: mTLS is enforced at the custom domain, not at the stage level
+  #checkov:skip=CKV_AWS_120: We're not enabling caching for this API Gateway, yet
   deployment_id = aws_api_gateway_deployment.eligibility_signposting_api.id
   rest_api_id   = module.eligibility_signposting_api_gateway.rest_api_id
   stage_name    = "${local.workspace}-eligibility-signposting-api-live"
@@ -61,6 +63,7 @@ resource "aws_api_gateway_stage" "eligibility-signposting-api" {
 }
 
 resource "aws_api_gateway_method_settings" "check_eligibility" {
+  #checkov:skip=CKV_AWS_225: We're not enabling caching for this API Gateway, yet
   rest_api_id = module.eligibility_signposting_api_gateway.rest_api_id
   stage_name  = aws_api_gateway_stage.eligibility-signposting-api.stage_name
   method_path = "*/*"
diff --git a/infrastructure/stacks/api-layer/cloudwatch.tf b/infrastructure/stacks/api-layer/cloudwatch.tf
@@ -1,7 +1,8 @@
 # CloudWatch Log Group for lambda Flow Logs
 resource "aws_cloudwatch_log_group" "lambda_logs" {
   name              = "/aws/lambda/${module.eligibility_signposting_lambda_function.aws_lambda_function_id}"
-  retention_in_days = 14
+  retention_in_days = 365
+  kms_key_id        = module.eligibility_signposting_lambda_function.lambda_cmk_arn
 
   tags = {
     Name  = "lambda-execution-logs"
diff --git a/infrastructure/stacks/api-layer/healthcheck_status.tf b/infrastructure/stacks/api-layer/healthcheck_status.tf
@@ -1,4 +1,6 @@
 resource "aws_api_gateway_method" "_status" {
+  #checkov:skip=CKV_AWS_59: API is secured via Apigee proxy with mTLS, API keys are not used
+  #checkov:skip=CKV2_AWS_53: No request parameters to validate for static healthcheck endpoint
   rest_api_id   = module.eligibility_signposting_api_gateway.rest_api_id
   resource_id   = aws_api_gateway_resource._status.id
   http_method   = "GET"
diff --git a/infrastructure/stacks/api-layer/iam_policies.tf b/infrastructure/stacks/api-layer/iam_policies.tf
@@ -136,10 +136,16 @@ data "aws_iam_policy_document" "kms_key_policy" {
       identifiers = ["arn:aws:iam::${data.aws_caller_identity.current.account_id}:root"]
     }
     actions   = ["kms:*"]
-    resources = ["*"]
+    resources = [
+      module.eligibility_status_table.dynamodb_kms_key_arn,
+      module.s3_rules_bucket.storage_bucket_kms_key_arn,
+      module.s3_audit_bucket.storage_bucket_kms_key_arn,
+      module.eligibility_signposting_api_gateway.kms_key_arn,
+
+    ]
   }
   statement {
-    sid    = "Allow lambda role"
+    sid    = "Allow lambda decrypt role"
     effect = "Allow"
     principals {
       type = "AWS"
@@ -151,7 +157,28 @@ data "aws_iam_policy_document" "kms_key_policy" {
       "kms:Decrypt"
     ]
     resources = [
-      module.eligibility_status_table.dynamodb_kms_key_arn
+      module.eligibility_status_table.dynamodb_kms_key_arn,
+      module.s3_rules_bucket.storage_bucket_kms_key_arn,
+    ]
+  }
+
+  statement {
+    sid    = "Allow lambda full write role"
+    effect = "Allow"
+    principals {
+      type = "AWS"
+      identifiers = [
+        aws_iam_role.eligibility_lambda_role.arn
+      ]
+    }
+    actions = [
+      "kms:Decrypt",
+      "kms:Encrypt",
+      "kms:GenerateDataKey",
+      "kms:DescribeKey"
+    ]
+    resources = [
+      module.s3_audit_bucket.storage_bucket_kms_key_arn
     ]
   }
 }
diff --git a/infrastructure/stacks/api-layer/patient_check.tf b/infrastructure/stacks/api-layer/patient_check.tf
@@ -1,10 +1,25 @@
+
+resource "aws_api_gateway_request_validator" "patient_check_validator" {
+  rest_api_id = module.eligibility_signposting_api_gateway.rest_api_id
+  name        = "validate-path-params"
+  validate_request_body = false
+  validate_request_parameters = true
+}
+
 resource "aws_api_gateway_method" "get_patient_check" {
+  #checkov:skip=CKV_AWS_59: API is secured via Apigee proxy with mTLS, API keys are not used
   rest_api_id      = module.eligibility_signposting_api_gateway.rest_api_id
   resource_id      = aws_api_gateway_resource.patient.id
   http_method      = "GET"
   authorization    = "NONE"
   api_key_required = false
 
+  request_validator_id = aws_api_gateway_request_validator.patient_check_validator.id
+
+  request_parameters = {
+    "method.request.path.id" = true  # Require the 'id' path parameter
+  }
+
   depends_on = [
     aws_api_gateway_resource.patient,
     aws_api_gateway_resource.patient_check,
diff --git a/infrastructure/stacks/iams-developer-roles/github_actions_policies.tf b/infrastructure/stacks/iams-developer-roles/github_actions_policies.tf
@@ -33,6 +33,13 @@ resource "aws_iam_policy" "terraform_state" {
 
 # API Infrastructure Management Policy
 resource "aws_iam_policy" "api_infrastructure" {
+  #checkov:skip=CKV_AWS_287 Ensure IAM policies does not allow credentials exposure
+  #checkov:skip=CKV_AWS_355 Ensure no IAM policies documents allow "*" as a statement's resource for restrictable actions
+  #checkov:skip=CKV_AWS_288 Ensure IAM policies does not allow data exfiltration
+  #checkov:skip=CKV_AWS_289 Ensure IAM policies does not allow permissions management / resource exposure without constraints
+  #checkov:skip=CKV_AWS_286 Ensure IAM policies does not allow privilege escalation
+  #checkov:skip=CKV_AWS_290 Ensure IAM policies does not allow write access without constraints
+
   name        = "api-infrastructure-management"
   description = "Policy granting permissions to manage API infrastructure"
   path        = "/service-policies/"
diff --git a/infrastructure/stacks/iams-developer-roles/terraform_developer_policies.tf b/infrastructure/stacks/iams-developer-roles/terraform_developer_policies.tf
diff --git a/infrastructure/stacks/networking/acm_certificates.tf b/infrastructure/stacks/networking/acm_certificates.tf
diff --git a/infrastructure/stacks/networking/kms.tf b/infrastructure/stacks/networking/kms.tf
diff --git a/infrastructure/stacks/networking/network_acls.tf b/infrastructure/stacks/networking/network_acls.tf
diff --git a/infrastructure/stacks/networking/vpc_flow_logs.tf b/infrastructure/stacks/networking/vpc_flow_logs.tf

Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,7 @@ data "aws_iam_policy_document" "api_gateway_logging" {`
`27`	`27`	`"logs:GetLogEvents",`
`28`	`28`	`"logs:FilterLogEvents"`
`29`	`29`	`]`
`30`		`- resources = ["*"]`
	`30`	`+ resources = [aws_cloudwatch_log_group.api_gateway.arn]`
`31`	`31`	`}`
`32`	`32`	`}`
`33`	`33`
Original file line number	Diff line number	Diff line change
`@@ -25,3 +25,7 @@ output "logging_policy_attachment" {`
`25`	`25`	`output "iam_role_name" {`
`26`	`26`	`value = aws_iam_role.api_gateway.name`
`27`	`27`	`}`
	`28`	`+`
	`29`	`+output "kms_key_arn" {`
	`30`	`+ value = aws_kms_key.api_gateway.arn`
	`31`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+data "aws_caller_identity" "current" {}`
Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,7 @@`
`1`	`1`	`# Main state bucket`
`2`	`2`	`resource "aws_s3_bucket" "tfstate_bucket" {`
	`3`	`+ #checkov:skip=CKV_AWS_144: We don't want to replicate outside our region`
	`4`	`+ #checkov:skip=CKV2_AWS_62: We won't enable event notifications for this bucket, yet`
`3`	`5`	`bucket = "${var.project_name}-${var.environment}-tfstate"`
`4`	`6`	`tags = {`
`5`	`7`	`Stack = "Bootstrap"`
`@@ -95,6 +97,9 @@ resource "aws_s3_bucket_lifecycle_configuration" "tfstate_bucket" {`
`95`	`97`	`# Logging`
`96`	`98`
`97`	`99`	`resource "aws_s3_bucket" "tfstate_s3_access_logs" {`
	`100`	`+ #checkov:skip=CKV_AWS_144: We don't want to replicate outside our region`
	`101`	`+ #checkov:skip=CKV2_AWS_62: We won't enable event notifications for this bucket, yet`
	`102`	`+ #checkov:skip=CKV_AWS_21: Versioning not needed given short lifecycle of logs`
`98`	`103`	`bucket = "${var.project_name}-${var.environment}-tfstate-access-logs"`
`99`	`104`	`}`
`100`	`105`
`@@ -109,7 +114,8 @@ resource "aws_s3_bucket_server_side_encryption_configuration" "tfstate_s3_access`
`109`	`114`
`110`	`115`	`rule {`
`111`	`116`	`apply_server_side_encryption_by_default {`
`112`		`- sse_algorithm = "AES256"`
	`117`	`+ sse_algorithm = "aws:kms"`
	`118`	`+ kms_master_key_id = aws_kms_key.terraform_state_bucket_cmk.arn`
`113`	`119`	`}`
`114`	`120`	`}`
`115`	`121`	`}`
`@@ -131,6 +137,16 @@ resource "aws_s3_bucket_lifecycle_configuration" "tfstate_s3_access_logs_object_`
`131`	`137`	`noncurrent_days = var.log_retention_in_days`
`132`	`138`	`}`
`133`	`139`	`}`
	`140`	`+ rule {`
	`141`	`+ id = "StateBucketLogsMultipartUploadExpiration"`
	`142`	`+ status = "Enabled"`
	`143`	`+ filter {`
	`144`	`+ prefix = ""`
	`145`	`+ }`
	`146`	`+ abort_incomplete_multipart_upload {`
	`147`	`+ days_after_initiation = 7`
	`148`	`+ }`
	`149`	`+ }`
`134`	`150`	`}`
`135`	`151`
`136`	`152`	`resource "aws_s3_bucket_public_access_block" "s3logs" {`
Original file line number	Diff line number	Diff line change
`@@ -23,5 +23,10 @@ resource "aws_dynamodb_table" "dynamodb_table" {`
`23`	`23`	`kms_key_arn = aws_kms_key.dynamodb_cmk.arn`
`24`	`24`	`}`
`25`	`25`
	`26`	`+ #checkov:skip=CKV_AWS_28: Point-in-time recovery is enabled only for production environments`
	`27`	`+ point_in_time_recovery {`
	`28`	`+ enabled = var.environment == "prod"`
	`29`	`+ }`
	`30`	`+`
`26`	`31`	`tags = var.tags`
`27`	`32`	`}`