Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

envoy: Never use x-forwarded-for header, add for Cilium Ingress #25674

Merged
merged 2 commits into from
May 26, 2023

Conversation

jrajahalme
Copy link
Member

@jrajahalme jrajahalme commented May 25, 2023

Envoy by default gets the source address from the x-forwarded-for header, if present. Always add an explicit use_remote_address: true for Envoy HTTP Connection Manager configuration to disable the default behavior.

Also add skip_xff_append: true config to keep the existing behavior of not adding x-forwarded-for headers, except for Cilium Ingress, where we now use skip_xff_append: false to explicitly append the source IP to x-forwarded-for header so that Hubble flow records have a trace for the original source of the traffic traversing Cilium Ingress.

Fixes: #25630

Fix incorrect hubble flow data when HTTP requests contain an `x-forwarded-for` header by adding an explicit `use_remote_address: true` config to Envoy HTTP configuration to always use the actual remote address of the incoming connection rather than the value of `x-forwarded-for` header, which may originate from an untrusted source. This change has no effect on Cilium policy enforcement where the source security identity is always resolved before HTTP headers are parsed. Previous Cilium behavior of not adding `x-forwarded-for` headers is retained via an explicit `skip_xff_append: true` config setting, except for Cilium Ingress where the source IP address is now appended to `x-forwarded-for` header.

@jrajahalme jrajahalme added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. release-blocker/1.11 This issue will prevent the release of the next version of Cilium. needs-backport/1.11 release-blocker/1.12 This issue will prevent the release of the next version of Cilium. affects/v1.11 This issue affects v1.11 branch affects/v1.12 This issue affects v1.12 branch affects/v1.10 This issue affects v1.10 branch release-blocker/1.13 This issue will prevent the release of the next version of Cilium. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch affects/v1.13 This issue affects v1.13 branch release-blocker/1.14 This issue will prevent the release of the next version of Cilium. labels May 25, 2023
@jrajahalme jrajahalme requested review from a team as code owners May 25, 2023 12:45
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.11.18 May 25, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.12.10 May 25, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.3 May 25, 2023
@jrajahalme jrajahalme marked this pull request as draft May 25, 2023 12:46
@jrajahalme jrajahalme force-pushed the envoy-do-not-trust-x-forwarded-for branch from 17e1d5c to 541509c Compare May 25, 2023 13:50
@jrajahalme jrajahalme changed the title envoy: Never trust x-forwarded-for envoy: Never use x-forwarded-for header May 25, 2023
@jrajahalme jrajahalme force-pushed the envoy-do-not-trust-x-forwarded-for branch from 541509c to 8e05023 Compare May 25, 2023 14:03
@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme changed the title envoy: Never use x-forwarded-for header envoy: Never use x-forwarded-for header, add for Cilium Ingress May 25, 2023
Envoy by default gets the source address from the `x-forwarded-for`
header, if present. Always add an explicit `use_remote_address: true` for
Envoy HTTP Connection Manager configuration to disable the default
behavior.

Also set the `skip_xff_append: true` option to retain the old behavior of
not adding `x-forwarded-for` headers on cilium envoy proxy.

Setting these options is not really needed for admin and metrics
listeners, or most of the tests, but we add them there too in case anyone
uses them as a source of inspiration for a real proxy configuration.

This fixes incorrect hubble flow data when HTTP requests contain an
`x-forwarded-for` header. This change has no effect on Cilium policy
enforcement where the source security identity is always resolved before
HTTP headers are parsed.

Fixes: cilium#25630
Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Do not skip adding `x-forwarded-for` in Cilium Ingress.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@jrajahalme jrajahalme merged commit abb355e into cilium:main May 26, 2023
58 checks passed
@sayboras sayboras mentioned this pull request May 28, 2023
10 tasks
@sayboras sayboras added the backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. label May 28, 2023
@sayboras sayboras mentioned this pull request May 28, 2023
5 tasks
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.12 in 1.12.10 May 28, 2023
@sayboras sayboras mentioned this pull request May 28, 2023
1 task
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.11 in 1.11.18 May 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.11 in 1.11.18 May 28, 2023
@sayboras sayboras added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Jun 1, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.11 to Backport done to v1.11 in 1.11.18 Jun 1, 2023
@sayboras sayboras added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Jun 2, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport done to v1.13 in 1.13.3 Jun 2, 2023
@julianwiedmann julianwiedmann added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Jul 31, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.12 to Backport done to v1.12 in 1.12.10 Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.10 This issue affects v1.10 branch affects/v1.11 This issue affects v1.11 branch affects/v1.12 This issue affects v1.12 branch affects/v1.13 This issue affects v1.13 branch backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.11.18
Backport done to v1.11
1.12.10
Backport done to v1.12
1.13.3
Backport done to v1.13
Development

Successfully merging this pull request may close these issues.

Cilium is confused about source address if io.cilium.proxy-visibility observes http traffic
3 participants