Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Django Strategy: support for X-Forwarded-* headers #741

Conversation

e-kolpakov
Copy link

Description: This PR adds support for getting host and port from X-Forwarded-* headers, if they are available.

Related: edx/edx-solutions-edx-platform#530, https://github.com/edx/edx-platform/pull/9848

Motivation: If an application using PSA is running behind the reverse proxy on a non-standard port, PSA (python-saml, to be precise) fails checking the SAML assertion response with message "Authentication failed: SAML login failed: ['invalid_response'] The response was received at http://host:port instead of http://host".

Common way of passing original host and port to proxied app is using X-Forwarded-* headers. Some webservers (i.e. gunicorn) already respect those headers and pass them to django as "real" host and port (i.e. request.get_host() and request.META['SERVER_PORT'] actually return X-Forwarded-Host and X-Forwarded-Port` values). Other webservers (i.e. django dev server, nginx(?)) does not do so.

This PR adds a web-server-agnostic way to use PSA behind the reverse proxy.

Author concerns:

  1. This might not be an optimal way to do it. Possible alternatives:
    1.1. It is already working fine; clients should either use web-servers that respect X-Forwarded-* headers, or solve this issue in some other, non-PSA-related way.
    1.2. Clients should provide custom strategy with request-host and request_port overridden (i.e. follow the same approach as in https://github.com/edx/edx-platform/pull/9848)
  2. There seems to be serious complications to testing frameworks-specific code - there're no tests for framework-specific strategies so far, and adding a test for django strategy required adding Django, specifying settings and so on - and eventually blowed up entire test suite. Hence tests are not included so far, but I would be glad to add them, if some architectural guidance is given.

@e-kolpakov
Copy link
Author

@omab I'm not sure who should I talk to to get this merged, so starting with you. The test failure is something that happened with all the other PRs opened at that time, plus it only happens to pypy version of it (is there a problem with test infrastructure). So, what would be the next steps to get this merged (or at least discussed)?

@e-kolpakov e-kolpakov force-pushed the ekolpakov/reverse-proxy-django-support branch from f2970ac to 172d90e Compare February 3, 2016 10:29
if self.setting("RESPECT_X_FORWARDED_HEADERS", False):
forwarded_host = self.request.META.get('HTTP_X_FORWARDED_PORT')
if forwarded_host:
return forwarded_host
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@e-kolpakov don't you want to return the port here rather than the host?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@omarkhan good catch

@e-kolpakov e-kolpakov force-pushed the ekolpakov/reverse-proxy-django-support branch from 172d90e to cf2c6f8 Compare February 11, 2016 11:25
@e-kolpakov
Copy link
Author

Closing in favor of #841

@e-kolpakov e-kolpakov closed this Apr 4, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants