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

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

Closed
asvetlov opened this issue Aug 27, 2016 · 16 comments

Comments

@asvetlov
Copy link
Member

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.

@asvetlov asvetlov added the web label Aug 27, 2016
@asvetlov asvetlov changed the title Deprecate secure_proxy_ssl_header, support X-Forwarded-* and implicitly Support X-Forwarded-* and Forwarded implicitly, deprecate secure_proxy_ssl_header. Aug 27, 2016
@asvetlov asvetlov changed the title Support X-Forwarded-* and Forwarded implicitly, deprecate secure_proxy_ssl_header. Support X-Forwarded-* and Forwarded implicitly, deprecate secure_proxy_ssl_header. Aug 27, 2016
@fafhrd91 fafhrd91 added this to the 2.1 milestone Feb 16, 2017
@fafhrd91 fafhrd91 modified the milestones: 2.2, 2.1 Apr 8, 2017
@evertlammerts
Copy link
Contributor

evertlammerts commented May 10, 2017

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

... 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
Copy link
Member

@evertlammerts PR would be very good!

@evertlammerts
Copy link
Contributor

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
Copy link
Member

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

@evertlammerts
Copy link
Contributor

Title says "deprecate secure_proxy_ssl_header"

@fafhrd91
Copy link
Member

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

@evertlammerts
Copy link
Contributor

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
Copy link
Contributor

Any chance this can still make it into 2.1?

@fafhrd91
Copy link
Member

sure

asvetlov pushed a commit that referenced this issue May 22, 2017
…eme and host resolution (#1881)

* #1134 Support X-Forwarded-* and Forwarded implicitly

* #1134 tests

* added myself to contributors

* added line to changes.rst

* Update CHANGES.rst

* Update CHANGES.rst

* deprecating secure_proxy_ssl_header

* more extensive Forwarded parsing

* flake

* make isort with new parameters

* Fix #1839: python 3.7 regression for regexp escaping (#1908)

* Fix #1839: python 3.7 regression for regexp escaping

* Fix import sort order

* small changes

* unpacking tuple implicit

* #1134 Support X-Forwarded-* and Forwarded implicitly

* #1134 tests

* added myself to contributors

* added line to changes.rst

* deprecating secure_proxy_ssl_header

* more extensive Forwarded parsing

* flake

* small changes

* unpacking tuple implicit

* handling multiple field-values, fixed quoted-pair bug (now using vchar instead of tchar), changed return type of forwarded
asvetlov pushed a commit that referenced this issue May 24, 2017
* #1134 Support X-Forwarded-* and Forwarded implicitly

* #1134 tests

* added myself to contributors

* added line to changes.rst

* Update CHANGES.rst

* Update CHANGES.rst

* deprecating secure_proxy_ssl_header

* more extensive Forwarded parsing

* flake

* small changes

* unpacking tuple implicit

* #1134 Support X-Forwarded-* and Forwarded implicitly

* #1134 tests

* added myself to contributors

* added line to changes.rst

* deprecating secure_proxy_ssl_header

* more extensive Forwarded parsing

* flake

* small changes

* unpacking tuple implicit

* handling multiple field-values, fixed quoted-pair bug (now using vchar instead of tchar), changed return type of forwarded

* docs for forwarded

* removed double entry from changes

* Docs for forwarded

* docs pass spellchecker
@a-urth
Copy link
Contributor

a-urth commented May 29, 2017

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
Copy link
Contributor

evertlammerts commented May 29, 2017

#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
Copy link
Contributor

a-urth commented May 29, 2017

@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
Copy link
Member Author

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

@asvetlov
Copy link
Member Author

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

fafhrd91 added a commit that referenced this issue Jun 6, 2017
Remove duplicate change entry for #1134 for 2.1.0 (2017-05-26)
@adamcharnock
Copy link

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
Copy link

lock bot commented Oct 28, 2019

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

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants