fix(terraform): tune the GCP Cloud Armor rule sets with field exclusions (#15553)

deployment/terraform/modules/gcp/README.md

@@ -285,9 +285,33 @@ object access and the narrow read that Onyx needs at startup.
 
 A backend security policy with the OWASP Core Rule Set at sensitivity 1, an API
 rate limit, a global rate limit and Adaptive Protection. It takes effect only on
-an L7 load balancer; see below. The WAF rules skip `/api/license/upload`,
-because its multipart body trips the protocol attack signatures. The rate
-limits still apply to it.
+an L7 load balancer; see below.
+
+The rule sets are tuned the standard way, with request field exclusions, so
+they deny without breaking Onyx. Without the tuning, sensitivity 1 denies chat
+messages that hold code, shell commands or file paths, every upload, and the
+Google sign-in callback.
+
+- No rule set except `methodenforcement`, `scannerdetection` and
+  `sessionfixation` reads the values of the Onyx fields that carry chat text,
+  prompts, code, URLs and secrets, such as `message`, `system_prompt`,
+  `api_base` and `password`. A name covers the query string, a form body and
+  the top-level keys of a JSON body, on every path. A nested key is reached
+  only through its parent, so free-form objects such as a connector's
+  configuration are covered by prefix. Everything else in the request is still
+  checked: the path, the headers, the cookies, the other parameters, and the
+  parameter names.
+- The same rule sets skip an upload, a `POST`, `PUT` or `PATCH` with a
+  `multipart/form-data` body. Cloud Armor does not parse a multipart body and
+  reads the file content as parameter names, which no exclusion covers. The
+  header alone exempts nothing: a `GET` is checked in full whatever it sends.
+- `scannerdetection` and `sessionfixation` check every request in full.
+
+When a legitimate request gets a 403, the load balancer log names the
+signature and the request. Add the field to
+`cloud_armor_extra_uninspected_fields`, or its parent object to
+`cloud_armor_extra_uninspected_field_prefixes`. Both add to the module
+defaults. The rate limits apply to every request.
 
 ### `l7-ingress`
 

deployment/terraform/modules/gcp/cloud-armor/main.tf

@@ -12,7 +12,7 @@
 #   1000+  blocked ranges               deny(403)
 #   2000   outside the allowlist        deny(403)
 #   2100+  blocked countries            deny(403)
-#   3000+  preconfigured WAF rules      deny(403), except the license upload
+#   3000+  preconfigured WAF rules      deny(403), tuned for Onyx traffic
 #   4000+  rate limit exempt ranges     allow, so they skip both limits
 #   5000   API path rate limit          throttle, deny(429)
 #   5100   global rate limit            throttle, deny(429)
@@ -29,10 +29,57 @@ locals {
 
   waf_rule_keys = sort(keys(var.preconfigured_rules))
 
-  # The WAF rules skip this path and the rate limits still count it. The body
-  # is a signed license in a multipart form, which the protocol attack
-  # signatures deny, and Onyx accepts it from an admin only.
-  license_upload_path = "/api/license/upload"
+  # Rule sets left untuned, because nothing Onyx sends trips them. The rest
+  # read field values, and an Onyx field value is often code, a URL or a secret.
+  untuned_rule_sets = ["methodenforcement", "scannerdetection", "sessionfixation"]
+
+  # The standard WAF tuning: a signature keeps running but does not read the
+  # value of a named field. These are the Onyx fields that hold chat text,
+  # prompts, code, URLs and secrets, which the signatures match at any
+  # sensitivity. A field missing here shows up as a 403 and a load balancer
+  # log line naming the signature.
+  default_uninspected_fields = [
+    # chat and prompts
+    "message", "query", "user_query", "prompt", "system_prompt", "task_prompt",
+    "instructions", "instructions_markdown", "description", "content", "text",
+    "answer", "feedback_text", "reason", "summary", "title", "name", "match_pattern",
+    # code and errors
+    "code", "reasoning_content", "error_message",
+    # URLs, which the remote file inclusion signatures match on an address
+    "api_base", "base_url", "server_url", "url", "api_url",
+    # secrets, whose random symbols the injection signatures match
+    "password", "api_key", "client_secret", "access_token", "bot_token",
+    "app_token", "user_token", "token", "api_secret",
+    # the Google sign-in callback carries userinfo.profile here
+    "scope",
+  ]
+
+  # A nested JSON key is reached only through the key it sits under, so whole
+  # free-form objects are named by prefix.
+  default_uninspected_field_prefixes = [
+    "connector_specific_config", "credential_json", "definition", "custom_config",
+    "new_custom_config", "existing_custom_config", "connection_headers", "config", "environment",
+  ]
+
+  # The names cover query string and body parameters, and the top-level keys
+  # of a JSON body.
+  uninspected_params = concat(
+    [for f in concat(local.default_uninspected_fields, var.extra_uninspected_fields) : { operator = "EQUALS", value = f }],
+    [for f in concat(local.default_uninspected_field_prefixes, var.extra_uninspected_field_prefixes) : { operator = "STARTS_WITH", value = f }],
+  )
+
+  # Cloud Armor does not parse a multipart body. It reads the file content as
+  # parameter names, which no exclusion covers, so the content rule sets skip
+  # an upload. The method check keeps the header from exempting a GET.
+  multipart_guard = "!(request.method.matches('POST|PUT|PATCH') && request.headers['content-type'].lower().startsWith('multipart/form-data')) && "
+
+  # One exclusion per content rule set, for every signature in it.
+  waf_exclusions = {
+    for k in local.waf_rule_keys : k => contains(local.untuned_rule_sets, k) ? [] : [{
+      rule_set     = "${k}-${var.crs_version}-stable"
+      query_params = local.uninspected_params
+    }]
+  }
 
   waf_expressions = {
     for k, r in var.preconfigured_rules : k => format(
@@ -54,6 +101,7 @@ locals {
       src_ip_ranges = chunk
       expression    = null
       rate_limit    = null
+      exclusions    = []
     }],
     length(var.allowed_ip_cidrs) > 0 ? [{
       priority      = 2000
@@ -63,6 +111,7 @@ locals {
       src_ip_ranges = null
       expression    = "!(${join(" || ", [for c in var.allowed_ip_cidrs : "inIpRange(origin.ip, '${c}')"])})"
       rate_limit    = null
+      exclusions    = []
     }] : [],
     [for i, chunk in local.country_chunks : {
       priority      = 2100 + i
@@ -72,6 +121,7 @@ locals {
       src_ip_ranges = null
       expression    = join(" || ", [for c in chunk : "origin.region_code == '${c}'"])
       rate_limit    = null
+      exclusions    = []
     }],
     [for i, k in local.waf_rule_keys : {
       priority      = 3000 + i
@@ -79,8 +129,9 @@ locals {
       description   = "OWASP CRS ${k}"
       preview       = coalesce(var.preconfigured_rules[k].preview, var.preview)
       src_ip_ranges = null
-      expression    = "request.path != '${local.license_upload_path}' && ${local.waf_expressions[k]}"
+      expression    = "${contains(local.untuned_rule_sets, k) ? "" : local.multipart_guard}${local.waf_expressions[k]}"
       rate_limit    = null
+      exclusions    = local.waf_exclusions[k]
     }],
     [for i, chunk in local.exempt_chunks : {
       priority      = 4000 + i
@@ -90,6 +141,7 @@ locals {
       src_ip_ranges = chunk
       expression    = null
       rate_limit    = null
+      exclusions    = []
     }],
     # A throttle rule allows requests under its limit and stops there, so API
     # requests count against this limit only, not also against the global one.
@@ -101,6 +153,7 @@ locals {
       src_ip_ranges = null
       expression    = "request.path.startsWith('${var.api_path_prefix}')"
       rate_limit    = { count = var.api_rate_limit_threshold }
+      exclusions    = []
     }],
     [{
       priority      = 5100
@@ -110,6 +163,7 @@ locals {
       src_ip_ranges = ["*"]
       expression    = null
       rate_limit    = { count = var.rate_limit_threshold }
+      exclusions    = []
     }],
     [{
       priority      = 2147483647
@@ -119,6 +173,7 @@ locals {
       src_ip_ranges = ["*"]
       expression    = null
       rate_limit    = null
+      exclusions    = []
     }],
   )
 }
@@ -172,6 +227,26 @@ resource "google_compute_security_policy" "this" {
         }
       }
 
+      dynamic "preconfigured_waf_config" {
+        for_each = length(rule.value.exclusions) > 0 ? [rule.value.exclusions] : []
+        content {
+          dynamic "exclusion" {
+            for_each = preconfigured_waf_config.value
+            content {
+              target_rule_set = exclusion.value.rule_set
+
+              dynamic "request_query_param" {
+                for_each = exclusion.value.query_params
+                content {
+                  operator = request_query_param.value.operator
+                  value    = request_query_param.value.value
+                }
+              }
+            }
+          }
+        }
+      }
+
       dynamic "rate_limit_options" {
         for_each = rule.value.rate_limit != null ? [rule.value.rate_limit] : []
         content {

deployment/terraform/modules/gcp/cloud-armor/tests/cloud-armor.tftest.hcl

@@ -76,32 +76,131 @@ run "defaults_enforce_the_owasp_rule_sets_at_sensitivity_one" {
     condition = one([
       for r in google_compute_security_policy.this.rule :
       one(r.match).expr[0].expression if r.description == "OWASP CRS sqli"
-    ]) == "request.path != '/api/license/upload' && evaluatePreconfiguredWaf('sqli-v33-stable', {'sensitivity': 1})"
+    ]) == "!(request.method.matches('POST|PUT|PATCH') && request.headers['content-type'].lower().startsWith('multipart/form-data')) && evaluatePreconfiguredWaf('sqli-v33-stable', {'sensitivity': 1})"
     error_message = "The SQLi rule should evaluate sqli-v33-stable at sensitivity 1."
   }
 }
 
-run "the_license_upload_skips_the_waf_rules_but_not_the_rate_limits" {
+run "content_rule_sets_skip_multipart_uploads_only" {
   command = plan
 
   assert {
     condition = alltrue([
       for r in google_compute_security_policy.this.rule :
-      startswith(one(r.match).expr[0].expression, "request.path != '/api/license/upload' && evaluatePreconfiguredWaf(")
-      if r.priority >= 3000 && r.priority < 4000
+      startswith(one(r.match).expr[0].expression, "!(request.method.matches('POST|PUT|PATCH') && request.headers['content-type'].lower().startsWith('multipart/form-data')) && evaluatePreconfiguredWaf(")
+      if startswith(r.description, "OWASP CRS ") && !contains(["methodenforcement", "scannerdetection", "sessionfixation"], trimprefix(r.description, "OWASP CRS "))
     ])
-    error_message = "Every WAF rule should leave the license upload alone, because its multipart body trips the protocol attack signatures."
+    error_message = "Rule sets that read field values should skip multipart uploads, whose file content Cloud Armor reads as parameter names."
+  }
+
+  assert {
+    condition = alltrue([
+      for r in google_compute_security_policy.this.rule :
+      startswith(one(r.match).expr[0].expression, "evaluatePreconfiguredWaf(")
+      if contains(["methodenforcement", "scannerdetection", "sessionfixation"], trimprefix(r.description, "OWASP CRS "))
+    ])
+    error_message = "The untuned rule sets should check every request."
+  }
+}
+
+run "content_rule_sets_do_not_read_the_free_text_fields" {
+  command = plan
+
+  assert {
+    condition = alltrue([
+      for r in google_compute_security_policy.this.rule :
+      length(one(r.preconfigured_waf_config).exclusion) == 1
+      && one(one(r.preconfigured_waf_config).exclusion).target_rule_set == "${trimprefix(r.description, "OWASP CRS ")}-v33-stable"
+      && one(one(r.preconfigured_waf_config).exclusion).target_rule_ids == null
+      && alltrue([for p in one(one(r.preconfigured_waf_config).exclusion).request_query_param : contains(["EQUALS", "STARTS_WITH"], p.operator)])
+      && length(one(one(r.preconfigured_waf_config).exclusion).request_query_param) > 30
+      if startswith(r.description, "OWASP CRS ") && !contains(["methodenforcement", "scannerdetection", "sessionfixation"], trimprefix(r.description, "OWASP CRS "))
+    ])
+    error_message = "Every content rule set should carry one exclusion for every signature, naming each uninspected field and prefix."
+  }
+
+  assert {
+    condition = alltrue([
+      for r in google_compute_security_policy.this.rule : length(r.preconfigured_waf_config) == 0
+      if !startswith(r.description, "OWASP CRS ") || contains(["methodenforcement", "scannerdetection", "sessionfixation"], trimprefix(r.description, "OWASP CRS "))
+    ])
+    error_message = "Only the content rule sets should carry exclusions."
+  }
+
+  assert {
+    condition = alltrue([
+      for f in [
+        { operator = "EQUALS", value = "message" },
+        { operator = "EQUALS", value = "password" },
+        { operator = "EQUALS", value = "api_base" },
+        { operator = "EQUALS", value = "scope" },
+        { operator = "EQUALS", value = "query" },
+        { operator = "STARTS_WITH", value = "connector_specific_config" },
+      ] :
+      contains(one([
+        for r in google_compute_security_policy.this.rule :
+        one(one(r.preconfigured_waf_config).exclusion).request_query_param if r.description == "OWASP CRS sqli"
+      ]), f)
+    ])
+    error_message = "The defaults should hold the fields measured to trip the signatures: chat text, passwords, URLs, the sign-in scope, search text, and the nested connector configuration."
+  }
+}
+
+run "extra_fields_join_the_defaults" {
+  command = plan
+
+  variables {
+    extra_uninspected_fields         = ["comment"]
+    extra_uninspected_field_prefixes = ["custom_config"]
+  }
+
+  assert {
+    condition = alltrue([
+      for r in google_compute_security_policy.this.rule :
+      contains(one(one(r.preconfigured_waf_config).exclusion).request_query_param, { operator = "EQUALS", value = "comment" })
+      && contains(one(one(r.preconfigured_waf_config).exclusion).request_query_param, { operator = "STARTS_WITH", value = "custom_config" })
+      && contains(one(one(r.preconfigured_waf_config).exclusion).request_query_param, { operator = "EQUALS", value = "message" })
+      if startswith(r.description, "OWASP CRS ") && !contains(["methodenforcement", "scannerdetection", "sessionfixation"], trimprefix(r.description, "OWASP CRS "))
+    ])
+    error_message = "Caller extras should be added to the defaults, not replace them."
+  }
+}
+
+run "a_rule_set_outside_the_defaults_is_tuned_too" {
+  command = plan
+
+  variables {
+    crs_version         = "v422"
+    preconfigured_rules = { generic = {} }
+  }
+
+  assert {
+    condition = startswith(one([
+      for r in google_compute_security_policy.this.rule : one(r.match).expr[0].expression if r.description == "OWASP CRS generic"
+    ]), "!(request.method.matches('POST|PUT|PATCH') && request.headers['content-type'].lower().startsWith('multipart/form-data')) && evaluatePreconfiguredWaf('generic-v422-stable'")
+    error_message = "Every rule set outside the untuned three should skip multipart uploads."
   }
 
   assert {
     condition = one([
       for r in google_compute_security_policy.this.rule :
-      one(r.match).expr[0].expression if r.priority == 5000
-    ]) == "request.path.startsWith('/api')"
-    error_message = "The API rate limit should still count the license upload."
+      one(one(r.preconfigured_waf_config).exclusion).target_rule_set if r.description == "OWASP CRS generic"
+    ]) == "generic-v422-stable"
+    error_message = "The exclusion should name the rule set of the chosen CRS version."
   }
 }
 
+run "rejects_a_field_name_that_is_not_a_parameter_name" {
+  command = plan
+
+  variables {
+    extra_uninspected_fields = ["a b"]
+  }
+
+  expect_failures = [var.extra_uninspected_fields]
+}
+
+
 run "rate_limits_mirror_the_aws_defaults" {
   command = plan
 
@@ -260,7 +359,7 @@ run "per_rule_settings_override_the_module_wide_ones" {
   assert {
     condition = one([
       for r in google_compute_security_policy.this.rule : one(r.match).expr[0].expression if r.description == "OWASP CRS sqli"
-    ]) == "request.path != '/api/license/upload' && evaluatePreconfiguredWaf('sqli-v33-stable', {'sensitivity': 2, 'opt_out_rule_ids': ['owasp-crs-v030301-id942421-sqli', 'owasp-crs-v030301-id942432-sqli']})"
+    ]) == "!(request.method.matches('POST|PUT|PATCH') && request.headers['content-type'].lower().startsWith('multipart/form-data')) && evaluatePreconfiguredWaf('sqli-v33-stable', {'sensitivity': 2, 'opt_out_rule_ids': ['owasp-crs-v030301-id942421-sqli', 'owasp-crs-v030301-id942432-sqli']})"
     error_message = "The SQLi rule should carry its own sensitivity and opt-outs."
   }
 

deployment/terraform/modules/gcp/cloud-armor/variables.tf

@@ -121,6 +121,28 @@ variable "sensitivity" {
   }
 }
 
+variable "extra_uninspected_fields" {
+  type        = list(string)
+  description = "Request parameter and JSON body field names whose values the content WAF rule sets do not read, on top of the module default list of Onyx fields that carry free text, URLs and secrets. Add a field when the load balancer log shows a signature denying it."
+  default     = []
+
+  validation {
+    condition     = alltrue([for f in var.extra_uninspected_fields : can(regex("^[A-Za-z0-9_.-]+$", f))])
+    error_message = "Each uninspected field is a parameter name: letters, digits, _ . and - only."
+  }
+}
+
+variable "extra_uninspected_field_prefixes" {
+  type        = list(string)
+  description = "Prefixes of field names whose values the content WAF rule sets do not read, for whole free-form objects, on top of the module default list."
+  default     = []
+
+  validation {
+    condition     = alltrue([for f in var.extra_uninspected_field_prefixes : can(regex("^[A-Za-z0-9_.-]+$", f))])
+    error_message = "Each uninspected field prefix is the start of a parameter name: letters, digits, _ . and - only."
+  }
+}
+
 # The whole-policy equivalent of Azure Detection mode. IP and country rules
 # still enforce, because the caller listed those addresses on purpose.
 variable "preview" {

deployment/terraform/modules/gcp/onyx/main.tf

@@ -269,15 +269,17 @@ module "cloud_armor" {
   deletion_protection = local.deletion_protection.cloud_armor
   labels              = local.merged_labels
 
-  preview                     = var.cloud_armor_preview
-  sensitivity                 = var.cloud_armor_sensitivity
-  allowed_ip_cidrs            = var.cloud_armor_allowed_ip_cidrs
-  blocked_ip_cidrs            = var.cloud_armor_blocked_ip_cidrs
-  rate_limit_exempt_ip_cidrs  = var.cloud_armor_rate_limit_exempt_ip_cidrs
-  geo_restriction_countries   = var.cloud_armor_geo_restriction_countries
-  rate_limit_threshold        = var.cloud_armor_rate_limit_threshold
-  api_rate_limit_threshold    = var.cloud_armor_api_rate_limit_threshold
-  adaptive_protection_enabled = var.cloud_armor_adaptive_protection_enabled
+  preview                          = var.cloud_armor_preview
+  extra_uninspected_fields         = var.cloud_armor_extra_uninspected_fields
+  extra_uninspected_field_prefixes = var.cloud_armor_extra_uninspected_field_prefixes
+  sensitivity                      = var.cloud_armor_sensitivity
+  allowed_ip_cidrs                 = var.cloud_armor_allowed_ip_cidrs
+  blocked_ip_cidrs                 = var.cloud_armor_blocked_ip_cidrs
+  rate_limit_exempt_ip_cidrs       = var.cloud_armor_rate_limit_exempt_ip_cidrs
+  geo_restriction_countries        = var.cloud_armor_geo_restriction_countries
+  rate_limit_threshold             = var.cloud_armor_rate_limit_threshold
+  api_rate_limit_threshold         = var.cloud_armor_api_rate_limit_threshold
+  adaptive_protection_enabled      = var.cloud_armor_adaptive_protection_enabled
 }
 
 module "l7_ingress" {

deployment/terraform/modules/gcp/onyx/variables.tf

@@ -380,6 +380,18 @@ variable "enable_cloud_armor" {
   default     = true
 }
 
+variable "cloud_armor_extra_uninspected_fields" {
+  type        = list(string)
+  description = "Field names whose values the WAF content rule sets do not read, on top of the cloud-armor module default list of Onyx fields that carry free text, URLs and secrets. Add a field when the load balancer log shows a signature denying it."
+  default     = []
+}
+
+variable "cloud_armor_extra_uninspected_field_prefixes" {
+  type        = list(string)
+  description = "Prefixes of field names whose values the WAF content rule sets do not read, for whole free-form objects, on top of the cloud-armor module default list."
+  default     = []
+}
+
 variable "cloud_armor_preview" {
   type        = bool
   description = "Log what the WAF and rate limit rules match instead of acting on it"