Set value of `$remote_addr` to client IP when TLSPassthrough and Proxy Protocol are enabled #3341

shaun-nx · 2022-12-08T09:31:46Z

Proposed changes

This change updates the nginx plus and open source templates to allow the set-real-ip-from directive to be set in the server block for the stream context when proxy_protocol is enabled.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

for more information, see https://pre-commit.ci

codecov-commenter · 2022-12-08T09:38:14Z

Codecov Report

Merging #3341 (89cc68b) into main (7e2397f) will not change coverage.
The diff coverage is n/a.

❗ Current head 89cc68b differs from pull request most recent head e81fa7f. Consider uploading reports for the commit e81fa7f to get more accurate results

@@           Coverage Diff           @@
##             main    #3341   +/-   ##
=======================================
  Coverage   51.56%   51.56%           
=======================================
  Files          60       60           
  Lines       16678    16678           
=======================================
  Hits         8600     8600           
  Misses       7788     7788           
  Partials      290      290

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jjngx

👍🏻

ydixken · 2023-02-06T19:29:41Z

Hi @coolbry95 - We actually asked the F5 Support for this change a couple of weeks ago. I've tested today for >10hrs and didn't get this feature working.

X-Real-IP remains unset / empty. I've confirmed this behaviour on our side with proxy_protocol enabled and disabled using a fairly default NGiNX as upstream backend (with self-signed certs) and a tool (without NGiNX, to rule the config there out) to just send all set request headers as a repsonse.

We're using the Ingress Controller with Helm, here's a part of our configuration:

apiVersion: charts.nginx.org/v1alpha1
kind: NginxIngress
metadata:
  name: nginx-ingress
spec:
  controller:
    affinity: {}
    appprotect:
      enable: true
    appprotectdos:
      debug: false
      enable: false
      maxDaemons: 0
      maxWorkers: 0
      memory: 0
    config:
      annotations: {}
      entries:
        set-real-ip-from: 10.0.0.0/16
    customPorts: []
    defaultTLS:
      secret: ""
    enableCertManager: false
    enableCustomResources: true
    enableLatencyMetrics: false
    enableOIDC: true
    includeYear: false
    enablePreviewPolicies: false
    enableSnippets: false
    enableTLSPassthrough: true
    extraContainers: []
    globalConfiguration:
      create: false
      spec: {}
    healthStatus: true
    healthStatusURI: /*redacted*-health
    hostNetwork: false
    image:
      pullPolicy: Always
      repository: image-registry.openshift-image-registry.svc:5000/*redacted*-services/nginx-plus-ingress
      tag: 3.0.1_s2023.01.26-1_t2023.02.01-1
    lifecycle: {}
    customConfigMap: ""
    ingressClass: nginx
    initContainers: []
    kind: deployment
    logLevel: 2
    nginxDebug: false
    nginxReloadTimeout: 60000
    nginxStatus:
      allowCidrs: 127.0.0.1
      enable: true
      port: 8080
    nginxplus: true
    nodeSelector: {}
    pod:
      annotations: {}
      extraLabels: {}
    priorityClassName: openshift-user-critical
    readyStatus:
      enable: true
      port: 8081
    replicaCount: 1
    reportIngressStatus: #review later
      annotations: {}
      enable: true
      enableLeaderElection: true
      ingressLink: ""
    resources:
      requests:
        cpu: 100m
        memory: 128Mi
    service:
      annotations:
        service.beta.kubernetes.io/aws-load-balancer-proxy-protocol: '*'
      create: true
      customPorts: []
      externalIPs: []
      externalTrafficPolicy: Local
      extraLabels: {}
      httpPort:
        enable: true
        port: 80
        targetPort: 80
      httpsPort:
        enable: true
        port: 443
        targetPort: 443
      loadBalancerIP: ""
      loadBalancerSourceRanges: []
      name: nginx-ingress
      type: LoadBalancer
    serviceAccount:
      imagePullSecretName: ""
    setAsDefaultIngress: false
    terminationGracePeriodSeconds: 30
    tolerations: []
    volumeMounts: []
    volumes: []
    watchNamespace: ""
    wildcardTLS:
      secret: null
  nginxServiceMesh:
    enable: false
    enableEgress: false
  prometheus:
    create: true
    port: 9113
    scheme: http
    secret: ""
  disableIPV6: false
  rbac:
    create: true

For the Upstream:

apiVersion: v1
data:
  nginx.conf: |
    #
    # see:
    #       https://gist.github.com/WhisperingChaos/033699f3b8d0df89ac50ab185b8628b4
    #

    error_log stderr debug;

    events {
            # nginx requires this section even when applying all default values
    }

    http {

            # Upstream keyword is followed by a URL (domain name/IP). This reference
            # encapsulates the list of back-end servers defined for a virtual proxy.
            # When authenticating a certificate from a back-end server, the upstream
            # URL is supplied to the certificate authentication process instead of the
            # back-end server name. See the comments associated with proxy_pass below
            # for a detailed discussion.
            upstream httpd-testapp {
                    server localhost:8080;
            }

            server {

                    server_name tls-proxy;
                    listen 8443 ssl proxy_protocol;
                    #listen 8443 ssl; # same behaviour


                    ssl_certificate /var/run/secrets/tls-proxy/proxy.crt;
                    ssl_certificate_key /var/run/secrets/tls-proxy/proxy.key;

                    # limit protocols
                    proxy_ssl_protocols TLSv1.2 TLSv1.3;
                    proxy_ssl_ciphers HIGH:!aNULL:!MD5;

                    ssl_client_certificate /var/run/secrets/tls-proxy/ca.crt;
                    ssl_verify_client on;

                    location / {
                      #proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
                            proxy_set_header X-Remote $remote_addr;
                            proxy_set_header Host $http_host;
                            proxy_set_header X-Forwarded-Proto https;
                            proxy_redirect off;
                            proxy_pass http://httpd-testapp;
                            proxy_ssl_verify off;
                    }
            }
    }
kind: ConfigMap
metadata:
  creationTimestamp: null
  name: nginx

Transport Server:

apiVersion: k8s.nginx.org/v1alpha1
kind: TransportServer
metadata:
  name: hello-app
  annotations:
    k8s.nginx.org/server-tokens: "test/nginx"
spec:
  ingressClassName: "nginx"
  listener:
    name: tls-passthrough
    protocol: TLS_PASSTHROUGH
  host: nginx-test.apps.test.*redacted*.cloud.*redacted* 
  upstreams:
  - name: hello-app-upstream
    service: hello-app
    port: 443
  action:
    pass: hello-app-upstream

Is there any additional configuration needed that I'm missing?

Kind regards,
Yannick

“shaun-nx” and others added 4 commits December 6, 2022 10:48

Add set-real-ip-from in server block in stream context

167ba37

set real ip on when proxy-protocol is enabled

b98b7cc

Update README for proxy-protocol example

d322e0d

Merge branch 'main' into tls-set-real-ip

89cc68b

github-actions bot added the documentation Pull requests/issues for documentation label Dec 8, 2022

[pre-commit.ci] auto fixes from pre-commit.com hooks

e81fa7f

for more information, see https://pre-commit.ci

shaun-nx marked this pull request as ready for review December 8, 2022 10:16

shaun-nx requested a review from a team as a code owner December 8, 2022 10:16

shaun-nx requested review from lucacome, haywoodsh, jjngx and ciarams87 December 8, 2022 10:16

shaun-nx changed the title ~~Tls set real ip~~ Set value of $remote_addr to client IP when TLSPassthrough and Proxy Protocol are enabled Dec 8, 2022

Remove blank space

5269934

jjngx approved these changes Dec 8, 2022

View reviewed changes

ciarams87 approved these changes Dec 9, 2022

View reviewed changes

Merge branch 'main' into tls-set-real-ip

96ab55f

shaun-nx merged commit f3669ea into main Dec 9, 2022

shaun-nx deleted the tls-set-real-ip branch December 9, 2022 16:33

ydixken mentioned this pull request Jan 25, 2023

Ingress Controller Version 3.x nginx/nginx-ingress-helm-operator#67

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Set value of `$remote_addr` to client IP when TLSPassthrough and Proxy Protocol are enabled #3341

Set value of `$remote_addr` to client IP when TLSPassthrough and Proxy Protocol are enabled #3341

Uh oh!

shaun-nx commented Dec 8, 2022 •

edited

Loading

Uh oh!

codecov-commenter commented Dec 8, 2022

Uh oh!

jjngx left a comment

Uh oh!

ydixken commented Feb 6, 2023

Uh oh!

Uh oh!

Set value of $remote_addr to client IP when TLSPassthrough and Proxy Protocol are enabled #3341

Set value of $remote_addr to client IP when TLSPassthrough and Proxy Protocol are enabled #3341

Uh oh!

Conversation

shaun-nx commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Checklist

Uh oh!

codecov-commenter commented Dec 8, 2022

Codecov Report

Uh oh!

jjngx left a comment

Choose a reason for hiding this comment

Uh oh!

ydixken commented Feb 6, 2023

Uh oh!

Uh oh!

Set value of `$remote_addr` to client IP when TLSPassthrough and Proxy Protocol are enabled #3341

Set value of `$remote_addr` to client IP when TLSPassthrough and Proxy Protocol are enabled #3341

shaun-nx commented Dec 8, 2022 •

edited

Loading