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

Do not override REMOTE_ADDR with X-Fowarded-For #633

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@mardiros

mardiros commented Oct 18, 2013

From rfc 3875
The REMOTE_ADDR variable MUST be set to the network address of the
client sending the request to the server.

For exemple

WSGI app

def application(environ, start_response):
start_response('200 OK', [('Content-Type','text/html')])
ret = []
for k, v in environ.items():
if k not in ('REMOTE_ADDR', 'HTTP_X_FORWARDED_FOR'):
continue
ret.append('{0}={1}\n'.format(k, v))

return ret

Client

import requests
response = requests.get('http://localhost:5000',
headers={'X-Forwarded-For': '1.2.3.4'})

print response.text


Running server and client:

python client.py
REMOTE_ADDR=127.0.0.1
HTTP_X_FORWARDED_FOR=1.2.3.4


Before that patch, using gunicorn, it is impossible to get the proxy address inside the WSGi application.
Every other tested WSGI Server (mod_wsgi and uwsgi), it works.
Gunicorn should works too.

Do not override REMOTE_ADDR with X-Fowarded-For
From rfc 3875
The REMOTE_ADDR variable MUST be set to the network address of the
client sending the request to the server.
@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Oct 31, 2013

Owner

You can actually disable this behaviour by setting the x_forwarded_for_header to "" . Or check if ` environ['gunicorn.sock'].getpeername()[0] == environ['HTTP_X_FORWARDED_FOR'] .

Anyway the old Web CGI interface spec [1] is more explicit than the final spec:

6.1.9. REMOTE_ADDR

   The IP address of the client sending the request to the
   server. This is not necessarily that of the user agent (such
   as if the request came through a proxy).

where the final spec [2] says:

4.1.8.  REMOTE_ADDR

   The REMOTE_ADDR variable MUST be set to the network address of the
   client sending the request to the server.

It is not clear in the final spec if the client is the proxy or not. Anyway if other wsgi server are doing the first one, we should fix it in gunicorn.

I'm reluctant to remove all that useful code though :) so maybe we could add another environ I dunno.

Feedback is welcome.

[1] http://ken.coar.org/cgi/draft-coar-cgi-v11-03.txt
[2] http://www.ietf.org/rfc/rfc3875

Owner

benoitc commented Oct 31, 2013

You can actually disable this behaviour by setting the x_forwarded_for_header to "" . Or check if ` environ['gunicorn.sock'].getpeername()[0] == environ['HTTP_X_FORWARDED_FOR'] .

Anyway the old Web CGI interface spec [1] is more explicit than the final spec:

6.1.9. REMOTE_ADDR

   The IP address of the client sending the request to the
   server. This is not necessarily that of the user agent (such
   as if the request came through a proxy).

where the final spec [2] says:

4.1.8.  REMOTE_ADDR

   The REMOTE_ADDR variable MUST be set to the network address of the
   client sending the request to the server.

It is not clear in the final spec if the client is the proxy or not. Anyway if other wsgi server are doing the first one, we should fix it in gunicorn.

I'm reluctant to remove all that useful code though :) so maybe we could add another environ I dunno.

Feedback is welcome.

[1] http://ken.coar.org/cgi/draft-coar-cgi-v11-03.txt
[2] http://www.ietf.org/rfc/rfc3875

@benoitc

This comment has been minimized.

Show comment
Hide comment
Owner

benoitc commented Nov 4, 2013

@ghost ghost assigned benoitc Nov 4, 2013

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Nov 6, 2013

Owner

The patch is OK for the case where the socket is a TCP socket but what information should we set when this is a UNIX socket? In that case the address is always the socket path and the port is empty. Any idea?

Same problem somehow with #628 .

Owner

benoitc commented Nov 6, 2013

The patch is OK for the case where the socket is a TCP socket but what information should we set when this is a UNIX socket? In that case the address is always the socket path and the port is empty. Any idea?

Same problem somehow with #628 .

Be more flexbile with reverse proxy usage
Add an option to force the remote addr with the X-Forwarded-For header
Fix the usage when bound to a unix socket instead of an TCP one.
@mardiros

This comment has been minimized.

Show comment
Hide comment
@mardiros

mardiros Nov 6, 2013

When using a Unix socket:

  • The REMOTE_ADDR contains the unix socket prefixed by "unix:"

    The RCF says
    " REMOTE_ADDR variable MUST be set "
    REMOTE_ADDR = hostnumber
    hostnumber = ipv4-address | ipv6-address

    So in that case, it is not possible to respect the RFC.

    I don't have any use case for that.
    For me, don't set the REMOTE_ADDR sounds OK too.

  • The RFC did not mention REMOTE_PORT, so it's not a mandatory header.
    Don't set it if it's not possible sounds OK

  • add an option "--override-remote-addr" that permit to fallback to the previous behaviour

mardiros commented Nov 6, 2013

When using a Unix socket:

  • The REMOTE_ADDR contains the unix socket prefixed by "unix:"

    The RCF says
    " REMOTE_ADDR variable MUST be set "
    REMOTE_ADDR = hostnumber
    hostnumber = ipv4-address | ipv6-address

    So in that case, it is not possible to respect the RFC.

    I don't have any use case for that.
    For me, don't set the REMOTE_ADDR sounds OK too.

  • The RFC did not mention REMOTE_PORT, so it's not a mandatory header.
    Don't set it if it's not possible sounds OK

  • add an option "--override-remote-addr" that permit to fallback to the previous behaviour

cli = ["--override-remote-addr"]
validator = validate_bool
action = "store_true"
default = False

This comment has been minimized.

@georgexsh

georgexsh Nov 21, 2013

Contributor

should not this option be True by default for downward compatibility?

@georgexsh

georgexsh Nov 21, 2013

Contributor

should not this option be True by default for downward compatibility?

This comment has been minimized.

@benoitc

benoitc Nov 21, 2013

Owner

That's a good question , the code is here since a long time....

@benoitc

benoitc Nov 21, 2013

Owner

That's a good question , the code is here since a long time....

This comment has been minimized.

@mardiros

mardiros Nov 21, 2013

I am ok with that too, how do you wan't to call that option in that case ?

@mardiros

mardiros Nov 21, 2013

I am ok with that too, how do you wan't to call that option in that case ?

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 27, 2013

Owner

I'm finally in favour of introducing the following breaking change like the initial patch suggested:

  • if we are using a unix socket just display its address and no REMOTE_PORT
  • if we are behind a TCP socket just pass the client address like.

To facilitate the transition we could propose some code to get the forwarded IP in the doc.

Thoughts?

cc @davisp @tilgovi @sirkonst @kennethreitz

Owner

benoitc commented Dec 27, 2013

I'm finally in favour of introducing the following breaking change like the initial patch suggested:

  • if we are using a unix socket just display its address and no REMOTE_PORT
  • if we are behind a TCP socket just pass the client address like.

To facilitate the transition we could propose some code to get the forwarded IP in the doc.

Thoughts?

cc @davisp @tilgovi @sirkonst @kennethreitz

@SeMeKh

This comment has been minimized.

Show comment
Hide comment
@SeMeKh

SeMeKh Sep 2, 2014

@benoitc This breaks the usefulness of gunicorn's access log! The remote_addr is always the proxy IP.

What do you think is the correct way to handle this? Do you think it would be possible to add a symbol for "forwarded ip" in log formats?

SeMeKh commented Sep 2, 2014

@benoitc This breaks the usefulness of gunicorn's access log! The remote_addr is always the proxy IP.

What do you think is the correct way to handle this? Do you think it would be possible to add a symbol for "forwarded ip" in log formats?

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Sep 2, 2014

Collaborator

I think %{X-Forwarded-For}i should work as a token in your access log format, @SeMeKh.

Collaborator

tilgovi commented Sep 2, 2014

I think %{X-Forwarded-For}i should work as a token in your access log format, @SeMeKh.

@Starefossen

This comment has been minimized.

Show comment
Hide comment
@Starefossen

Starefossen May 20, 2015

Contributor

I can see why this was changed, but I think it should be mentioned better in the docs, and a suggested workaround should maybe also be mentioned.

Contributor

Starefossen commented May 20, 2015

I can see why this was changed, but I think it should be mentioned better in the docs, and a suggested workaround should maybe also be mentioned.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc May 20, 2015

Owner

@Starefossen +1, can you open an issue for that? Would help to track it :)

Owner

benoitc commented May 20, 2015

@Starefossen +1, can you open an issue for that? Would help to track it :)

Starefossen added a commit to Starefossen/gunicorn that referenced this pull request May 21, 2015

Document why `REMOTE_ADD` may not be the user's IP
Gunicorn v19 removed functionality which updated `REMOTE_ADDR` to the value of
the `X-Forwared-For` header if received from a trusted upstream client.  This
was a violation of RFC 3875 CGI Version 1.1, and was hence removed.

Close: #1035
PR-URL: #1037
Related: #633

Signed-off-by: Hans Kristian Flaatten <hans.kristian.flaatten@turistforeningen.no>

Starefossen added a commit to Starefossen/gunicorn that referenced this pull request May 21, 2015

Document why `REMOTE_ADD` may not be the user's IP
Gunicorn v19 removed functionality which updated `REMOTE_ADDR` to the value of
the `X-Forwared-For` header if received from a trusted upstream client.  This
was a violation of RFC 3875 CGI Version 1.1, and was hence removed.

Close: #1035
PR-URL: #1037
Related: #633

Signed-off-by: Hans Kristian Flaatten <hans.kristian.flaatten@turistforeningen.no>

Starefossen added a commit to Starefossen/gunicorn that referenced this pull request May 21, 2015

Document why `REMOTE_ADD` may not be the user's IP
Gunicorn v19 removed functionality which updated `REMOTE_ADDR` to the value of
the `X-Forwared-For` header if received from a trusted upstream client.  This
was a violation of RFC 3875 CGI Version 1.1, and was hence removed.

Close: #1035
PR-URL: #1037
Related: #633

Signed-off-by: Hans Kristian Flaatten <hans.kristian.flaatten@turistforeningen.no>

Starefossen added a commit to Starefossen/gunicorn that referenced this pull request May 21, 2015

Document why `REMOTE_ADD` may not be the user's IP
Gunicorn v19 removed functionality which updated `REMOTE_ADDR` to the value of
the `X-Forwared-For` header if received from a trusted upstream client.  This
was a violation of RFC 3875 CGI Version 1.1, and was hence removed.

Close: #1035
PR-URL: #1037
Related: #633

Signed-off-by: Hans Kristian Flaatten <hans.kristian.flaatten@turistforeningen.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment