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

set ForwardedAllowIPS via cli #642

Merged
merged 1 commit into from
Dec 26, 2013
Merged

Conversation

georgexsh
Copy link
Contributor

set ForwardedAllowIPS option via cli --forwarded-allow-ips
example:

exec gunicorn \
--bind=192.168.11.1:8080 \
--forwarded-allow-ips='*'

@wong2
Copy link
Contributor

wong2 commented Nov 14, 2013

🍺

@tonicmuroq
Copy link

🍻

@benoitc
Copy link
Owner

benoitc commented Nov 21, 2013

Sorry for the delay to handle that patch . It's related to #633. I am not quite sure what to do at the moment, we should probably use th real remote. Hopefully @davisp @tilgovi have an idea on that too.

@georgexsh
Copy link
Contributor Author

@benoitc problem addressed in #633 is whether override REMOTE_ADDR,
but here the main influence of option forwarded_allow_ips is the behavior of reading cfg.secure_scheme_headers,
as I do not think those two issues are connected.

@benoitc
Copy link
Owner

benoitc commented Nov 21, 2013

@georgexsh well if we use the remote address then we should probably make some naming change like --allows-remote-ip=* since you will get the remote address, isn't it?

@georgexsh
Copy link
Contributor Author

@benoitc I dont quite get your point ..
the "remote address" used for checking with forwarded_allow_ips, is read from socket, not from headers

@benoitc
Copy link
Owner

benoitc commented Nov 21, 2013

@georgexsh OK so just to be clear we are right now parsing this forward headers to construct the remote address. Which is apparently against the spec. We don't need to parse the if we just collect the remote address from the client (except maybe when using a unix socket).

If I understand well you are using it to filters the client that connect to you app. At this point I wonder if it should be done at the server level or at the the application level.

If we do it at the server level, the it may be relevant to have an option to check all remotes ips. the one coming as a remote ip and the one in the forward headers. That why I was speaking about that change.

Will see what we decide later in the day.

@georgexsh
Copy link
Contributor Author

@benoitc your latest explanation confused me even deeper ...

we are right now parsing this forward headers to construct the remote address

yes for REMOTE_ADDR, but no for this case
client[0] not in cfg.forwarded_allow_ips link
and client is produced via client, addr = sock.accept()
when we need set forwarded_allow_ips, the "client" would not be the real user-agent, but a reverse proxy.
the influence is whether guncorn reads secure_headers or not.

you are using it to filters the client that connect to you app

not for my app, but wsgi spec: wsgi.url_scheme, which gunicorn should set.

@benoitc
Copy link
Owner

benoitc commented Nov 22, 2013

@georgexsh you're right I'm just confused with the option name right now. I really think it should something else. not sure what. Maybe it should ne remote_allow_ips instead? I'm +1 to merge your patch anyway. Will do it tomorrow.

@georgexsh
Copy link
Contributor Author

ping @benoitc

benoitc added a commit that referenced this pull request Dec 26, 2013
@benoitc benoitc merged commit 824b540 into benoitc:master Dec 26, 2013
@benoitc
Copy link
Owner

benoitc commented Dec 26, 2013

applied thanks :)

On Thu, Dec 26, 2013 at 11:59 AM, georgexsh notifications@github.51.alwrote:

ping @benoitc https://github.com/benoitc


Reply to this email directly or view it on GitHubhttps://github.com//pull/642#issuecomment-31217954
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants