Support X-Forwarded-* and Forwarded implicitly, deprecate secure_proxy_ssl_header.

[asvetlov]

It's not obvious and the current doc is misleading: it proposes secure_proxy_ssl_header='X-Forwarded-Proto' but should do secure_proxy_ssl_header=('X-Forwarded-Proto', 'https').

Looks like @popravich made a mistake on documenting it by d4954ef

1. X-Forwarded-For, X-Forwarded-By, and X-Forwarded-Proto should be supported. See the spec at http://docs.aws.amazon.com/elasticloadbalancing/latest/classic/x-forwarded-headers.html
2. Forwarded header from RFC 7239 https://tools.ietf.org/html/rfc7239 should be supported as well.

The issue doesn't require a single Pull Request, most likely it should be several PRs, each for other aspect for sake of easy reviewing etc.

Until we'll remove deprecated secure_proxy_ssl_header support it should be processed first before all other rules.

[evertlammerts]

What's the status on this? I'm going to use:

• Forwarded
• X-Forwarded-Proto, X-Forwarded-Host and X-Forwarded-For
• Host

... to try to determine how & where my service can be reached during runtime. I can do this in my own code but I can also shoot in a PR. How about this approach:

• add the above headers to aiohttp.hdrs
• use them in the above order in Request._scheme, Request.host and Request.url in an attempt to determine the correct vales
• explain resolution order in the docs

Would that be useful enough to be included?

[fafhrd91]

@evertlammerts PR would be very good!

[evertlammerts]

How about something like this? There are no tests in there yet and I'm not sure how deprecation works in aiohttp. Any pointers?

I'll revisit soon, just about to leave on a trip.

[fafhrd91]

we use deprecation warning. but what do you want to deprecate?

[evertlammerts]

Title says "deprecate secure_proxy_ssl_header"

[fafhrd91]

@samuelcolvin could you check? I think you last one who modified "secure_proxy_ssl_header"

[evertlammerts]

Personally I think making a best effort to determine scheme, host and port using standard and defacto standard headers is enough. People can always check for less common headers manually. In any case, maybe deprecation of secure_proxy_ssl_header could be a different PR?

I'm not completely sure about the most common semantics of X-Forwarded-Host, and whether its value is mostly the same as what the Host header contains without a proxy; in particular, does it normally contain the port number if not 80/443? I'll research next week.

[evertlammerts]

Any chance this can still make it into 2.1?

[fafhrd91]

sure

[a-urth]

So, am I missing something or support for X-Forwarded-For actually wasn't added during this issue?
If so intended, then is it expected to be added later on?

[evertlammerts]

#1881, which was about better host and scheme resolution for BaseRequest.url, only looks at X-Forwarded-Scheme and X-Forwarded-Host. You're right that there's no explicit support for X-Forwarded-For (nor for other variations like X-Forwarded-Port and X-Forwarded-By). I'm not sure whether you'd need more than req.headers.get(...) to use them though. Since there's no ABNF for X-Forwarded-For, which is a list, you can't do much more than splitting on comma. But @asvetlov does mention it here and he hasn't closed the issue, so maybe he sees more use for it.

[a-urth]

@evertlammerts Thanks for answer. For me there is no problem since we already doing split by comma, as You proposed. I was just confused since this issue mentions such problem, but I found nothing about that in merge request. Anyway, good to know and thanks again.

[asvetlov]

I don't think that forwarded-for is interesting to anybody. But please feel free to open a new PR if needed.

[asvetlov]

Let's close the issue, it's completed from my perspective.

[adamcharnock]

For anyone else chasing their tail trying to figure out where this feature has gone, it has been removed and moved into middleware. See here:

#2171 (comment)

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new