Skip to content

eli-537 enabling WAF blocks#591

Open
eddalmond1 wants to merge 10 commits intomainfrom
feature/eja-eli-537-enable-WAF-blocking-in-prod
Open

eli-537 enabling WAF blocks#591
eddalmond1 wants to merge 10 commits intomainfrom
feature/eja-eli-537-enable-WAF-blocking-in-prod

Conversation

@eddalmond1
Copy link
Collaborator

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

  • Refactoring (non-breaking change)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would change existing functionality)
  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I am familiar with the contributing guidelines
  • I have followed the code style of the project
  • I have added tests to cover my changes
  • I have updated the documentation accordingly
  • This PR is a result of pair or mob programming

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.

  • I confirm that neither PII/PID nor sensitive data are included in this PR and the codebase changes.

Copilot AI review requested due to automatic review settings March 3, 2026 09:57
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_Header sub-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 preprod and prod environments.

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_counted no longer matches what this alarm does (it now alarms on blocked requests and is named WAF-NonUK-BlockedRequests-*). Renaming the resource block to something like waf_non_uk_blocked would reduce confusion for future maintenance (you may need a moved block / 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 AllowedRequests metric, 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 preprod and prod (and disables it in dev). 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_enabled is driven by var.waf_enabled_environments which now includes preprod as 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.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 description still 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.

Karthikeyannhs
Karthikeyannhs previously approved these changes Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants