feat: add CSRF origin validation to SecurityPolicy - #8836
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8836 +/- ##
==========================================
+ Coverage 75.80% 75.82% +0.01%
==========================================
Files 258 259 +1
Lines 42795 42903 +108
==========================================
+ Hits 32440 32530 +90
- Misses 8176 8185 +9
- Partials 2179 2188 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
eb1feaf to
d7ee907
Compare
|
Hi @albsga4 thanks for the contribution! Would you mind removing the changes that are unrelated to this PR, and adding an e2e test for CSRF? |
826db28 to
c6f1413
Compare
|
Hi @zhaohuabing, thanks for the review! Sorry about that — I had some test examples containing my company name that I needed to remove, and while doing that I wrongly rebased my fork which caused all those unrelated changes to leak into the PR. I have now started fresh from latest main and the PR only contains CSRF-related files. I also added an e2e test following the CORS test pattern, covering all four StringMatch types (Exact, Prefix, Suffix, RegularExpression) with both allow and reject scenarios. Let me know if there is anything else! |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16c2ded4f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f859c44134
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
d2055fd to
ec0319f
Compare
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 878f37cd50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
878f37c to
fc8c1ea
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a3da5810e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for _, origin := range csrf.AdditionalOrigins { | ||
| // Envoy's CSRF filter matches the host and port of the Origin header against the | ||
| // target host, so the scheme is dropped before the matcher is built. | ||
| hostAndPort := originHostAndPort(string(origin)) |
There was a problem hiding this comment.
When additionalOrigins contains a valid but non-canonical URL such as https://EXAMPLE.com or https://example.com:443, browsers serialize the Origin host in lowercase and omit its default port, but this code only strips the scheme and builds a case-sensitive matcher for EXAMPLE.com or example.com:443. Such legitimate cross-origin requests are consequently rejected with 403 despite matching the configured origin; parse and canonicalize the host and port before constructing the matcher.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid observation, but pre-existing and not CSRF-specific. buildCORS builds its exact matcher straight from the configured value with no canonicalization either, so allowOrigins: [https://EXAMPLE.com] has behaved this way for a long time. additionalOrigins deliberately reuses the CORS Origin type, so canonicalizing only on the CSRF path would make two fields with the same type and the same docs behave differently.
I'd also argue the matcher is the wrong layer for it. Rewriting example.com:443 → example.com at translation time hides the misconfiguration rather than surfacing it. Better to reject it at admission on the shared Origin type, which covers CORS and CSRF together. Follow-up rather than this PR.
5386dba to
b0741b1
Compare
b0741b1 to
98f9340
Compare
Add a new CSRF field to SecurityPolicy that enables Envoy's CSRF filter for cross-site request forgery protection. The filter validates the Origin header on mutating requests (POST, PUT, DELETE, PATCH) against the destination and any additional allowed origins. Closes envoyproxy#8835 Signed-off-by: asalvador <asalvador@newrelic.com>
Add Prefix and RegularExpression to unit tests (gatewayapi + xds-ir), and Prefix and Suffix to e2e test, so all four match types (Exact, Prefix, Suffix, RegularExpression) are exercised at every test level. Signed-off-by: asalvador <asalvador@newrelic.com>
Fix yamllint error: expected 6 but found 8 spaces indentation for additionalOrigins list items. Signed-off-by: asalvador <asalvador@newrelic.com>
Give the CSRF filter the same order as CORS (3) so it runs before auth filters (ext_authz, JWT, etc.), rejecting cross-site mutating requests early without invoking upstream auth logic. Signed-off-by: asalvador <asalvador@newrelic.com>
Add envoy.filters.http.csrf to the kubebuilder validation enum so filterOrder entries referencing CSRF pass CRD validation. Also update the default filter order documentation to include CSRF. Signed-off-by: asalvador <asalvador@newrelic.com>
… URLs Envoy CSRF filter calls hostAndPort() on the Origin header before matching against additional_origins, stripping the scheme. Values must be specified as host or host:port (e.g., "www.example.com" not "https://www.example.com"). This follows the same approach as kgateway: pass values directly to Envoy without transformation, and document the expected format clearly. Updated all test fixtures and documentation to use host-only values. Signed-off-by: asalvador <asalvador@newrelic.com>
Expose full Envoy CSRF filter capabilities: - filterEnabled: percentage of requests with CSRF enforced (default 100%) - shadowEnabled: percentage evaluated in dry-run mode (track but not enforce) - additionalOrigins: values must be host:port (not full URLs) Values are passed directly to Envoy without transformation. Envoy strips the scheme from the Origin header before matching, so origins must be specified as host or host:port. Also fixes E2E test failures by using host-only origin values. Changes: - API: add FilterEnabled, ShadowEnabled fields to CSRF type - IR: add fields to CSRF struct - GatewayAPI: pass new fields in buildCSRF - xDS: buildXdsCSRFPolicy uses percentage fields - Golden tests: second route exercising filterEnabled/shadowEnabled - E2E: shadow mode test (filterEnabled=0, shadowEnabled=100) - Docs: shadow mode example and host:port format note Signed-off-by: asalvador <asalvador@newrelic.com>
Signed-off-by: asalvador <asalvador@newrelic.com>
- Replace `filterEnabled`/`shadowEnabled` (`*int32`) with `enforcedFraction`/ `shadowFraction` (`gwapiv1.Fraction`), matching how EG already models Envoy's RuntimeFractionalPercent (Tracing.SamplingFraction, RetryBudget.Percent), and translate them with the shared fractionalpercent helper. - Give the CSRF filter its own explicit order instead of sharing CORS's, so it runs after cors (preflights are answered by the cors filter) and before the authn/authz filters. - Preallocate the additionalOrigins slice to satisfy the prealloc linter. - Regenerate CRDs, deepcopy, helm golden files, API reference and testdata. - Move the release note to the new release-notes/current fragment layout so it lands in v1.9.0 instead of the already-cut v1.8.0-rc.1. Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Envoy strips the scheme from the Origin header before matching, and the header never carries a path, so a value like `https://www.example.com` is accepted by the CRD but can never match, silently 403ing legitimate requests. Add a CEL rule on the CSRF type that rejects such values at admission, with cel-validation coverage for the valid host / host:port forms and the scheme and path forms. Also point the Gateway/HTTPRoute/GRPCRoute links in the CSRF task doc at /reference/api-types/, matching the other task docs; the old /api-types/ paths 404 and broke docs-check-links. Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Partially enforcing an origin check is not a useful security posture: with enforcedFraction below 100% a fraction of cross-site mutating requests is let through unvalidated and unobserved. The legitimate use case behind the knob is dry running the filter before turning it on, which shadowFraction already covers, so drop enforcedFraction and make shadowFraction the single rollout control. Envoy only lets an invalid request through in shadow mode if that request was not also selected by filter_enabled, so filter_enabled is now derived as the complement of the shadow fraction. Every request is therefore either enforced or observed, never neither, and a shadow fraction of 100% is a complete dry run instead of a no-op. Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
additionalOrigins described a list of trusted origins with a generic
StringMatch, so the same concept was expressed two different ways inside one
SecurityPolicy: cors.allowOrigins took "https://*.example.com" while
csrf.additionalOrigins took {type: Suffix, value: .example.com}. Reuse Origin
so both read the same, and drop the CEL rule that had to reject schemes and
paths, since the Origin pattern already constrains the shape.
Envoy accepts a StringMatcher for cors allow_origin_string_match and for csrf
additional_origins alike, so the narrower syntax is an Envoy Gateway choice
rather than a filter limitation, and it is the choice CORS already made. It
also drops the two matchers that widen trust by accident: a Prefix of
"app.example" matches the attacker-controlled app.example.attacker.com, and a
Suffix that omits the leading dot matches eviltrusted.com. A wildcard label
goes through wildcard2regex, which escapes the separators and anchors the
result, so neither hole is reachable.
Envoy compares the host and port of the origin, so buildCSRF strips the scheme
when lowering to the IR matcher: an origin with a wildcard host becomes an
anchored regex, everything else an exact match on host[:port].
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
Envoy draws filter_enabled and shadow_enabled independently, so a partial shadow_enabled compounded with filter_enabled instead of partitioning with it: a request selected by neither was left unenforced and unevaluated, never reaching the csrf stats. Enforcement was skewed upwards too, since the filter re-randomizes both predicates at each of its two call sites -- shadowFraction 90 rejected ~17% of invalid requests rather than 10%. Pin shadow_enabled to 100% and let filter_enabled alone carry the rollout. featureEnabled() short-circuits at 100% without touching the PRNG, so the filter's first gate becomes unconditional -- every mutating request reaches the stats block -- and the reject decision reduces to a single filter_enabled draw. shadowFraction 40 now yields exactly 60% enforced and 40% shadowed, with nothing unevaluated. Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
98f9340 to
bebe2c0
Compare
3 participants
What type of PR is this?
api: addcsrffield toSecurityPolicyWhat this PR does / why we need it:
Envoy has a built-in
envoy.filters.http.csrffilter that validates theOriginheader against allowed origins on mutating requests (POST, PUT, DELETE, PATCH). Currently there's no way to configure this through Envoy Gateway withoutEnvoyPatchPolicy.This PR adds native CSRF support following the exact same pattern as CORS:
Implementation:
Follows the CORS pattern exactly:
SecurityPolicy.csrfwithadditionalOrigins []StringMatch(reuses existingStringMatchtype)CSRFstruct inSecurityFeatureswithAdditionalOrigins []*StringMatchbuildCSRF()usingirStringMatch()helper (same as CORS)csrf.goimplementinghttpFilterinterface:patchHCM: adds CSRF filter to listener disabled by default (0%)patchRoute: enables CSRF per-route viatyped_per_filter_config(100%)patchResources: no-opBehavior:
additionalOriginsOriginheader on mutating requests → 403Origin→ 403Changes:
api/v1alpha1/csrf_types.go(new) — CSRF API typeapi/v1alpha1/securitypolicy_types.go— addCSRF *CSRFfieldinternal/ir/xds.go— CSRF IR struct in SecurityFeaturesinternal/gatewayapi/securitypolicy.go—buildCSRF(), translation for route and gateway targets, TCP validationinternal/xds/translator/csrf.go(new) — httpFilter implementationsecuritypolicy-with-csrfsite/content/en/latest/tasks/security/csrf.mdWhich issue(s) this PR fixes:
Fixes #8835
Release Notes: Yes
/cc @arkodg