Skip to content

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

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

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 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 affects/v1.13 This issue affects v1.13 branch labels May 25, 2023
@jrajahalme jrajahalme requested review from a team as code owners May 25, 2023 12:45
@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>
@jrajahalme jrajahalme force-pushed the envoy-do-not-trust-x-forwarded-for branch from df3c23c to 868dbe5 Compare May 25, 2023 16:03
@jrajahalme jrajahalme marked this pull request as ready for review May 25, 2023 16:03
@jrajahalme
Copy link
Member Author

jrajahalme commented May 25, 2023

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Unexpected missed tail call

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/184/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@jrajahalme
Copy link
Member Author

net-next hit known flake #24514

@jrajahalme
Copy link
Member Author

/test-1.26-net-next

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
@sayboras sayboras mentioned this pull request May 28, 2023
10 tasks
@sayboras sayboras mentioned this pull request May 28, 2023
5 tasks
@sayboras sayboras mentioned this pull request May 28, 2023
1 task
@sayboras sayboras added backport-pending/1.11 backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed needs-backport/1.11 labels May 28, 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
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
Status: Released
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