Conversation
There was a problem hiding this comment.
Pull request overview
Enables active blocking in the API Gateway WAF configuration (moving from COUNT/monitoring to enforcement) and updates CloudWatch alarms to align with the new blocking behaviour.
Changes:
- Switch AWS managed rule groups and custom rules from COUNT to enforce (block), with a targeted COUNT override for the
NoUserAgent_Headersub-rule to preserve healthchecks. - Update WAF CloudWatch alarms to focus on blocked traffic (rate-limit, geo blocking) and adjust thresholds/evaluation periods for the new enforcement posture.
- Restrict default WAF deployment to
preprodandprodenvironments.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| infrastructure/stacks/api-layer/waf.tf | Enables WAF rule enforcement (block) and adds a COUNT override for NoUserAgent_Header; updates geo rule to block non-allowed countries (GB-only in prod; GB+US in preprod). |
| infrastructure/stacks/api-layer/waf_alarms.tf | Aligns alarms with blocking mode (BlockedRequests), updates thresholds/severities, and adds clarifying alarm intent text. |
| infrastructure/stacks/api-layer/variables.tf | Changes default WAF-enabled environments to preprod and prod. |
Comments suppressed due to low confidence (4)
infrastructure/stacks/api-layer/waf_alarms.tf:142
- The Terraform resource name
waf_non_uk_countedno longer matches what this alarm does (it now alarms on blocked requests and is namedWAF-NonUK-BlockedRequests-*). Renaming the resource block to something likewaf_non_uk_blockedwould reduce confusion for future maintenance (you may need amovedblock / state mv to avoid recreation).
resource "aws_cloudwatch_metric_alarm" "waf_non_uk_counted" {
count = local.waf_enabled ? 1 : 0
alarm_name = "WAF-NonUK-BlockedRequests-${local.workspace}"
alarm_description = "Alerts when non-UK requests are blocked by geo rule - may indicate stolen mTLS cert use from outside UK"
comparison_operator = "GreaterThanThreshold"
infrastructure/stacks/api-layer/waf_alarms.tf:177
- This alarm is described as monitoring “total request volume”, but it is using the
AllowedRequestsmetric, which excludes blocked/counted requests. Either adjust the description/comment to say “allowed request volume”, or switch to a metric-math expression that reflects total requests (allowed + blocked + counted) if that’s what you want to alert on.
alarm_name = "WAF-AllRequests-High-${local.workspace}"
alarm_description = "Monitors total request volume through WAF"
comparison_operator = "GreaterThanThreshold"
evaluation_periods = 2
metric_name = "AllowedRequests"
namespace = "AWS/WAFV2"
infrastructure/stacks/api-layer/variables.tf:16
- The variable description mentions disabling WAF “in test after evaluation”, but the default now only enables WAF in
preprodandprod(and disables it indev). Please update the description to reflect the intended environments, so callers don’t assume WAF is still deployed in dev/test by default.
variable "waf_enabled_environments" {
type = list(string)
description = "Environments in which WAF resources are deployed. Adjust to disable in test after evaluation."
default = ["preprod", "prod"]
infrastructure/stacks/api-layer/waf.tf:2
- The header comment says the WAF is “Only deployed in production environment for cost optimization”, but
local.waf_enabledis driven byvar.waf_enabled_environmentswhich now includespreprodas well. Please update/remove this comment so it matches the actual deployment behaviour (preprod+prod).
# Only deployed in production environment for cost optimization
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
infrastructure/stacks/api-layer/waf_alarms.tf:149
- The non-UK block alarm description suggests a single blocked request could indicate stolen mTLS cert use, but the alarm won’t fire until >=30 blocked requests across 2 periods (10 minutes). Either lower the threshold/evaluation_periods to match the incident intent, or adjust the description/severity to reflect that this is only for sustained volume.
alarm_name = "WAF-NonUK-BlockedRequests-${local.workspace}"
alarm_description = "Alerts when non-UK requests are blocked by geo rule - may indicate stolen mTLS cert use from outside UK"
comparison_operator = "GreaterThanThreshold"
evaluation_periods = 2
metric_name = "BlockedRequests"
namespace = "AWS/WAFV2"
period = 300
statistic = "Sum"
threshold = 30
treat_missing_data = "notBreaching"
infrastructure/stacks/api-layer/waf.tf:135
country_codes = var.environment == "preprod" ? ["GB", "US"] : ["GB"]assumes GitHub Actions traffic will reliably originate from the US in preprod. GitHub-hosted runner egress geolocation isn’t guaranteed, so this may intermittently block CI traffic. Consider switching the preprod exception to an IP allowlist (e.g., GitHub Actions IP ranges via an IP set) or another deterministic approach, rather than a country-code exception.
geo_match_statement {
country_codes = var.environment == "preprod" ? ["GB", "US"] : ["GB"]
}
infrastructure/stacks/api-layer/waf.tf:6
- The WebACL
descriptionstill says "- Production" even though WAF is now enabled for preprod as well. Please update the description to be environment-agnostic or include${var.environment}/${local.workspace}so it stays accurate.
name = "${local.workspace}-eligibility-signposting-api-waf"
description = "WAF Web ACL for Eligibility Signposting API Gateway - Production"
scope = "REGIONAL"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Following on from https://nhsd-jira.digital.nhs.uk/browse/ELI-536, we now want to enable traffic blocking in our WAF
Context
We do this to block malicious traffic (in addition to other security protections).
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.