Add configuration option to toggle changing Host to X-Forwarded-Host #3045
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lewisheadden If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
rootfs/etc/nginx/template/nginx.tmpl
Outdated
| default $this_host; | ||
| } | ||
| {{ end }} | ||
| {{ else }} |
There was a problem hiding this comment.
Include a newline before this so that we know its for {{ if $cfg.UseForwardedHeaders }} not being true.
| @@ -1097,7 +1103,11 @@ stream { | |||
| {{ else }} | |||
| {{ $proxySetHeader }} X-Forwarded-For $the_real_ip; | |||
| {{ end }} | |||
| {{ if and $all.Cfg.UseForwardedHeaders (not $all.Cfg.UseForwardedHostHeader) }} | |||
There was a problem hiding this comment.
Should the not be removed? Why would we forward the host header if we set UseForwardedHostHeader to false?
There was a problem hiding this comment.
Yeah, this is a little bit confusing.
Existing behavior always sets X-Forwarded-Host and Host to $best_http_host. If we set UseForwardedHeaders, $best_http_host will be X-Forwarded-Host otherwise it will be the default value of the current backend.
Setting UseForwardedHostHeader to false prevents X-Forwarded-Host being set as the Host but we've still said we trust X-Forwarded-* by setting UseForwardedHeaders. As such the right behavior of the ingress seems to be to forward the X-Forwarded-Host value. I'm pretty sure this logic preserves this behavior:
If we're trusting the X-Forwarded-* headers (
$all.Cfg.UseForwardedHeaders) but not assigningX-Forwarded-HosttoHost(not $all.Cfg.UseForwardedHostHeader) then we should forwarded theX-Forwarded-Hoston. Otherwise we supply what was determined to be$best_http_host.
This makes X-Forwarded-Host be treated in the same way as X-Forwarded-Proto and X-Forwarded-Port that are just forwarded on to the backend.
Please let me know if this doesn't make sense or if I'm misunderstanding what the desired behavior should be!
There was a problem hiding this comment.
Perhaps changing the configuration option name would make this clearer? OverrideHostHeader perhaps?
aledbf
added
the
do-not-merge/hold
lewisheadden
force-pushed
the
AddCfgForXForwardedHostMapping
branch
from
34bcf65 to
3ac5ffa
Compare
|
@diazjf any thoughts on how you'd like me to proceed? I have some time this week free at work. |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
|
@fejta-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
Allows users to supply a
use-forwarded-host-headerboolean option in theConfigMapfor the ingress controller which toggles whether theX-Forwarded-Hostheader is placed into theHostheader.Many proxies in front of nginx-ingress may terminate SSL or similar and require usage of
X-Forwarded-Protoor pass client IPs inX-Forwarded-Forbut very few will move theHostheader toX-Forwarded-Hostand require us to placeX-Forwarded-Hostback into theHostheader in nginx-ingress. We could disable allX-Forwarded-*headers usinguse-forwarded-headershowever processingX-Forwarded-ProtoandX-Forwarded-Forcan still be useful even whenX-Forwarded-Hostshould not be considered for placement into theHostheader.Further certain L7 proxies or CDNs (such as Fastly) will produce multi-valued
X-Forwarded-Hostheaders which should never be placed into theHostheader.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Fixes #2463
Special notes for your reviewer:
I think this is the most appropriate and generic way to resolve #2463 but I'm open to alternate approaches as I wrestled with this for a while. I'm also not 100% sold on my resolution for
{{ $proxySetHeader }} X-Forwarded-Hostbut after talking with some co-workers we agreed that this is likely the correct approach in our opinion.