Skip to content

Security#2

Open
theory wants to merge 3 commits into
lopnor:masterfrom
theory:security
Open

Security#2
theory wants to merge 3 commits into
lopnor:masterfrom
theory:security

Conversation

@theory

@theory theory commented Oct 13, 2010

Copy link
Copy Markdown

Okay, based on feedback on my previous pull request, here is a new one. This changes things so that one can specify a list of headers to check. This will make it more secure. It also adds two additional headers (which will only be checked if they're specified): HTTP_X_FORWARDED_SCRIPT_NAME and HTTP_X_FORWARDED_PATH_INFO.

They must be explicitly named in the list of headers to pay attention to.
Also, add a test to make sure that an explicit list of header names is obeyed.
Include discussion of explicitly naming headers to check and the security
reasons for doing so. Also list the headers that are inspected.
@miyagawa

Copy link
Copy Markdown
Collaborator

We agree that specifying headers instead of doing the guess work is nice, while keeping the backward compatibility.

However, i still don't get the particular instance of usefulness of HTTP_X_FORWARDED_SCRIPT_NAME and HTTP_X_FORWARDED_PATH_INFO. I assume you're doing some routing stuff in the backend using SCRIPT_NAME and PATH_INFO which is fine, but carrying that information from frontend sounds like really confusing. Even if you figured out that kind of info is necessary, I don't think it should be added to the list of "standard" headers that this module supports.

here's why: In your commit note, you're linking to Ian Bicking's suggestion. Further googling shows that he wanted to set those headers to implement WSGIProxy, which is the opposite of what we usually do: making a WSGI proxy app that turns WSGI into an outgoing HTTP calls. http://pythonpaste.org/wsgiproxy/

This is a fine idea, so implementing that (i.e. maybe Plack::App::Proxy extension) should be great, but I really, really don't think we should support manglign SCRIPT_NAME and PATH_INFO in the ReverseProxy setup.

My vote is to merge c6v1c70 and a01a25c but drop 0b84671. You could still use your own TweakEnv middleware (or write your own non-Plack namespaced middleware to do the same in one shot) to bring in X-Forwarded-Script-Name.

@miyagawa

Copy link
Copy Markdown
Collaborator

On that note, adding X-Real-Ip to the list of recognizable headers (off by default, of course) would be a good idea since it's one of nginx's default module. http://wiki.nginx.org/NginxHttpRealIpModule

If there's any other particular headers that nginx/lighttpd/pound/mod_proxy/perlbal supports, then i'm open for adding those, too.

@theory

theory commented Oct 14, 2010

Copy link
Copy Markdown
Author

Here's the issue I want to solve with X-Forwarded-ScriptName:

  • One Plack app, with subapps (if you will), /pub and /priv. The former is public, the latter private, behind Plack::Middleware::Auth::Basic
  • Two virtual hosts served by my proxy server. One is http, the other https. The former reverse proxies / to /pub, while the latter reverse proxies / to /priv.
  • I need a way to generate URIs (uri_for()) that work for the front-end. They would need to link to, say, /about rather than /pub/about.

I was assuming this was a common-ish need. I could have my uri_for() method respect HTTP_X_FORWARDED_SCRIPT_NAME, I guess. I'd just need a way to turn it off for when it's not running behind a proxy server that sets such a header.

@theory

theory commented Oct 14, 2010

Copy link
Copy Markdown
Author

Oh, and as to your last note, I think it'd be reasonable to allow the headers parameter to be a string rather than an array ref. If said string matches a known proxy server, we could use that proxy. Something like:

my %HEADERS_FOR = (
    mod_proxy => [qw(HTTP_X_FORWARDED_FOR HTTP_X_FORWARDED_HOST HTTP_X_FORWARDED_SERVER)],
    nginx => [qw(HTTP_X_REAL_IP ...)],
    ....
);

—Theory

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants