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

proxy-set-headers appends instead of overwriting #3481

Closed
xcq1 opened this issue Nov 28, 2018 · 23 comments
Closed

proxy-set-headers appends instead of overwriting #3481

xcq1 opened this issue Nov 28, 2018 · 23 comments
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@xcq1
Copy link

xcq1 commented Nov 28, 2018

Is this a request for help? No

What keywords did you search in NGINX Ingress controller issues before filing this one? proxy-set-headers, overwrite, override, add-headers


Is this a BUG REPORT or FEATURE REQUEST? Bug Report

NGINX Ingress controller version: 0.21.0

Kubernetes version (use kubectl version): 1.10.9

Environment:

  • Cloud provider: AWS, can provide specifics but I assume they're not relevant

What happened:
My instance of ingress-nginx is running behind an AWS ELB that terminates the TLS connection. Thus all requests show up with the "X-Forwarded-*" headers indicating a plain old HTTP connection. However my backend services generate user-facing URLs from the values supplied from these headers. Therefore, I must change them as if the ingress-nginx had received an HTTPS request so it returns the proper URLs.

In order to do this I have configured the configuration option "proxy-set-headers" to point to a ConfigMap with the following data:

X-Forwarded-Port: "443"
X-Forwarded-Proto: https

But when I checked on the requests that arrive at an echo server (gcr.io/google_containers/echoserver:1.4) this is what the backend would get: (Values get appended instead of set)

x-forwarded-port=80443
x-forwarded-proto=httphttps

What you expected to happen:
The backend should receive (Values should get replaced)

x-forwarded-port=443
x-forwarded-proto=https

How to reproduce it (as minimally and precisely as possible): See "What happened"

Anything else we need to know: Can't think of anything, but will answer any follow-up questions

@dcherniv
Copy link

@xcq1 what are the relevant configmap, annotations configuration snippets? Can you paste them verbatim?

@xcq1
Copy link
Author

xcq1 commented Nov 30, 2018

@dcherniv I think these would be the relevant bits:

kind: ConfigMap
apiVersion: v1
metadata:
  name: ingress-nginx
  labels:
    k8s-addon: ingress-nginx.addons.k8s.io
data:
  use-proxy-protocol: "true"
  proxy-body-size: "100m"
  proxy-read-timeout: "900"
  proxy-send-timeout: "900"
  proxy-set-headers: "infrastructure/ingress-nginx-headers"
  custom-http-errors: "404,502,503,504"
  log-format-upstream: '{"time": "$msec", "format": "access", "message": "$request", "request_id": "$request_id", "request_status": "$status", "request_method": "$request_method", "request_host": "$host", "request_uri": "$request_uri", "request_length": "$request_length", "request_time": "$request_time", "request_body_bytes_sent": "$body_bytes_sent", "request_referrer": "$http_referer", "request_user_agent": "$http_user_agent", "request_remote_addr": "$proxy_protocol_addr", "request_remote_user": "$remote_user", "request_forwarded_for": "$proxy_add_x_forwarded_for", "request_upstream_address": "$upstream_addr", "request_upstream_response_length": "$upstream_response_length", "request_upstream_response_time": "$upstream_response_time", "request_upstream_status": "$upstream_status" }'
  http-snippet: |
    server {
      listen 8000 proxy_protocol;
      server_tokens off;
      return 301 https://$host$request_uri;
    }

  hide-headers: "Server"
  hide-headers: "X-Application-Context"
  hide-headers: "X-Powered-By"
---
kind: ConfigMap
apiVersion: v1
metadata:
  name: ingress-nginx-headers
  labels:
    k8s-addon: ingress-nginx.addons.k8s.io
data:
  Request-Id: "$request_id"
  Strict-Transport-Security: "max-age=31536000; includeSubDomains; preload"
  Content-Security-Policy: "frame-ancestors 'self' https://*.example1.com https://*.example2.com https://*.example3.com"
  X-Content-Type-Options: "nosniff"
  X-XSS-Protection: "1; mode=block"
  Referrer-Policy: "strict-origin-when-cross-origin"
  X-Forwarded-Port: "443"
  X-Forwarded-Proto: https
---
kind: Service
apiVersion: v1
metadata:
  name: ingress-nginx
  labels:
    k8s-addon: ingress-nginx.addons.k8s.io
  annotations:
    service.beta.kubernetes.io/aws-load-balancer-ssl-cert: '$((elb-certificate-arn))'
    service.beta.kubernetes.io/aws-load-balancer-ssl-ports: '443'
    service.beta.kubernetes.io/aws-load-balancer-proxy-protocol: '*'
    service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout: '900'
spec:
  type: LoadBalancer
  selector:
    app: ingress-nginx
  ports:
  - name: http
    port: 80
    targetPort: 8000
  - name: https
    port: 443
    targetPort: 80

Port 8000 has also been opened on the deployment.
Right now I'm guessing in my case it would be preferable if I just switched the ELB to HTTPS backend requests, but I figured the behavior is a little bit odd nonetheless.

@dcherniv
Copy link

dcherniv commented Nov 30, 2018

I think this is the default proxy_set_header behavior in nginx.
To overwrite headers we might need to use headers_more.
https://www.nginx.com/resources/wiki/modules/headers_more/
Or use lua maybe.
My apologies i couldn't be of much help. This does appear to be a valid concern however. We should be able to override headers passed to upstream.

@xcq1
Copy link
Author

xcq1 commented Dec 5, 2018

Just found out add-headers is guilty of a similar problem:
With the above ConfigMaps, when a backend returns X-XSS-Protection: 1; mode=block the client receives those headers like this:

...
X-XSS-Protection: 1; mode=block
X-XSS-Protection: 1; mode=block
...

Which in this case leads to problems because the appended headers are not valid: Error parsing header X-XSS-Protection: 1; mode=block, 1; mode=block: expected semicolon at character position 13. The default protections will be applied.

I got around this for now by replacing add_headers in line 219 of the nginx.tmpl with your suggestion more_set_headers. However it's not exactly clear to me where I would replace proxy_set_header with more_set_headers to solve it for the proxy-set-header config option (or if that would break other things).

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 5, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 4, 2019
@fdlk
Copy link

fdlk commented Apr 23, 2019

Running into this on our ingress which runs on port 80 behind an SSL proxy that incorrectly sends us an X-Forwarded-Port: 80 header.
I cannot override this with the correct value since proxy-set-headers configmap values get appended.

@xcq1
Copy link
Author

xcq1 commented Apr 23, 2019

/remove-lifecycle rotten

@fdlk In case it might help you: We ended up workarounding this by hard patching the default nginx.tmpl and including it as a separate config map as described in https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/custom-template/

It works if you have no HTTP-only connections, however it isn't going to win any beauty awards. And you have to accept the fact that any version upgrade can theoretically break your config.

This is the patch:

--- nginx.tmpl.orig	2019-01-15 00:15:21.000000000 +0100
+++ nginx.tmpl	2019-02-12 13:09:05.069068796 +0100
@@ -1278,8 +1278,8 @@
             {{ $proxySetHeader }} X-Forwarded-For        $the_real_ip;
             {{ end }}
             {{ $proxySetHeader }} X-Forwarded-Host       $best_http_host;
-            {{ $proxySetHeader }} X-Forwarded-Port       $pass_port;
-            {{ $proxySetHeader }} X-Forwarded-Proto      $pass_access_scheme;
+            {{ $proxySetHeader }} X-Forwarded-Port       "443";
+            {{ $proxySetHeader }} X-Forwarded-Proto      "https";
             {{ if $all.Cfg.ProxyAddOriginalURIHeader }}
             {{ $proxySetHeader }} X-Original-URI         $request_uri;
             {{ end }}

Which is then deployed like so:
kubectl create cm nginx-template -n infrastructure --from-file=controller/service/nginx.tmpl --dry-run -o yaml | kubectl apply -f -
And then included as a volume as described in the link to the user guide.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Apr 23, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 22, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 21, 2019
@xcq1
Copy link
Author

xcq1 commented Aug 23, 2019

/remove-lifecycle rotten
To whom it may concern: The workaround in the referenced issue #4096 (comment) is even simpler, avoids the hard template dependency and works just as well (under the same assumptions).

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 23, 2019
@hobti01
Copy link
Contributor

hobti01 commented Oct 3, 2019

It looks like this has been fixed in a44b5cf, Version 0.26.0 via #4463

Edit: The referenced fix is for set-headers not proxy-set-headers. Unfortunately proxy-set-headers continues to append values, not overwrite v0.41.0

@gdmello
Copy link

gdmello commented Dec 4, 2019

The upgrade in a44b5cf did not work for me. What did work was adding the header as so:

more_set_input_headers "X-Forwarded-Proto: https";

@owengo
Copy link

owengo commented Dec 17, 2019

Ok my problem with X-Forwarded-Proto was not caused by ingress-nginx but by the AWS ALB in front of it.
It seems the ALB always overrides this header.

@prcongithub
Copy link

How did you fix it in ALB?
I am running my stack using EKS with ALB at the front and I am getting X-Forwarded-Proto as HTTP. I want to change it to https.

@owengo
Copy link

owengo commented Dec 30, 2019

@prcongithub
You can't fix ALB, it will trash any incoming X-Forwarded-Proto and set its own. X-F-P is badly designed: opposed to X-F-F this is not a list of incoming protocols, so intermediate proxies must choose between passing it as is ( which is a security issue, among other things ) or setting it to the protocol with which they received the request. It seems unlikely that AWS will give the possibility to pass the header.
If you know that your traffic will always be https at the edge, you can hack your ingress with something like this:

    annotations:
       nginx.ingress.kubernetes.io/configuration-snippet: |
           more_set_input_headers "X-Forwarded-Proto: https";

It makes sense, especially if you force a http => https redirect somewhere between your edge and ingress-nginx ( both included ).
Alternatively, if you can set your own header at the edge you can set your own "My-Forwarded-Proto: .." ( or use a header setup by CloudFront or your edge servers ) and do some hacking after the alb..

@prcongithub
Copy link

I just fixed it in kong. Had to add trusted_ips in kong configuration to allow X-Forwarded-* headers to pass through. I am using https everywhere. ALB redirects http to https. SSL Terminates at ALB and connects with Kong using HTTP. But if I enable trusted_ips, kong passes X-Forwarded-* headers with correct values.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 29, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 28, 2020
@k8s-ci-robot k8s-ci-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Apr 28, 2020
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

@dohoangkhiem
Copy link
Contributor

/remove-lifecycle rotten
To whom it may concern: The workaround in the referenced issue #4096 (comment) is even simpler, avoids the hard template dependency and works just as well (under the same assumptions).

@xcq1 wondering how could this work, I see the variables set in "server" blocks are always overridden by "location", so such $pass_access_scheme and $pass_port will be reverted, here's what I see in each of my location block

server {
           location / {
                        ...
                        set $pass_access_scheme  $scheme;
			set $pass_server_port    $server_port;
			
			set $pass_port           $pass_server_port;
	                ...
            }
            # Custom code snippet configured in the configuration configmap
	    set $pass_access_scheme "https";
	    set $pass_port 443;
}

@stahh
Copy link

stahh commented Jun 14, 2024

Is this still not fixed?

ingress controller: v1.5.1
Client Version: v1.30.0
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: v1.23.2

I need to change header Host and I use this:

nginx.ingress.kubernetes.io/configuration-snippet: |
      if ($http_host_original) {
        set $custom_host $http_host_original;
      }
      if ($http_host_original = false) {
        set $custom_host $http_host;
      }
      proxy_set_header Host $custom_host;

and as result I get concatenated hosts - Host: host1.com, host2.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests