imsweb / django-saml-sp

A Django application for running one or more SAML service providers (SP)
BSD 3-Clause "New" or "Revised" License
15 stars 10 forks source link

Add support for proxies #7

Closed merit-mthompson closed 4 years ago

merit-mthompson commented 4 years ago

When running behind something like Nginx proxying to Gunicorn the scheme and port and other bits might fall out of alignment with the publicly-accessibly URI stuff.

One fix from https://github.com/onelogin/python-saml/issues/75 is to tweak the prepare to check for X-Forwarded related headers.

Looks like this could be added to: https://github.com/imsweb/django-saml-sp/blob/8e112faecc7a5866a44a95b16c6f694fce5cecd1/sp/models.py#L98-L107

We're importing stuff from this library and sub-classing as needed to effectively override sp.models.IdP.prepare_request() with these changes.

dcwatson commented 4 years ago

Can you tell me a bit more about which pieces you're overriding? Django has a bunch of settings for controlling these things via proxy headers. For instance, it can determine the URL scheme (and thus request.is_secure()) via a setting/header:

https://docs.djangoproject.com/en/3.0/ref/settings/#secure-proxy-ssl-header https://github.com/django/django/blob/stable/3.0.x/django/http/request.py#L226

Similar deal for host/port:

https://docs.djangoproject.com/en/3.0/ref/settings/#use-x-forwarded-host

That said, I have seen things like multiple proxies not chaining headers correctly or dropping them. So I'm not opposed to having some override settings, but I don't think checking X-Forwarded headers will do anything you can't already with Django.

merit-mthompson commented 4 years ago

Thanks for the reply! Looking at the settings, I think the only thing we'd need to maybe change is how the port stuff is handled. We have different prod/stage/dev settings so we can tweak most things in there without issue.

While it certainly needs a change if we're going to use settings and the rest, here's what we've done so far:

some_app/apps.py

from django.apps import AppConfig

class SomeAppConfig(AppConfig):
    name = 'some_proj.some_app'
    verbose_name = 'Some App'

    def ready(self):
        """When apps are ready/loaded, load our patches."""
        import some_proj.some_app.patches  # noqa

some_app/patches.py

from sp.models import IdP

def prepare_request_proxy(self, request):
    """Add support for proxies when preparing SP settings for python-saml.

    Based on code adapted from:
    https://github.com/onelogin/python-saml/issues/75#issuecomment-108647017

    When this was written, returned object is/was:

        {
            "get_data": request.GET.copy(),
            "http_host": request.get_host(),
            "https": "on" if request.is_secure() else "off",
            "lowercase_urlencoding": self.lowercase_encoding,
            "post_data": request.POST.copy(),
            "script_name": request.path_info,
            "server_port": 443 if request.is_secure() else request.get_port(),
        }

    Sample Nginx config to support this:

        # NOTE: we're behind HAProxy (proxy-ception)
        location / {
            proxy_pass http://127.0.0.1:8000;
            proxy_set_header Host $host;
            proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
            proxy_set_header X-Forwarded-Port $server_port;
            proxy_set_header X-Forwarded-Proto $http_x_forwarded_proto;
        }

    """
    http_host = request.get_host()
    https = request.is_secure()
    ignored_ports = ('80', '8080', '443')
    server_port = 443 if request.is_secure() else request.get_port()

    if 'HTTP_X_FORWARDED_PORT' in request.META:
        server_port = request.META.get('HTTP_X_FORWARDED_PORT')

    if 'HTTP_X_FORWARDED_PROTO' in request.META:
        https = request.META.get('HTTP_X_FORWARDED_PROTO') == 'https'

    prepared = {
        'get_data': request.GET.copy(),
        'http_host': http_host,
        'https': 'on' if https else 'off',
        'lowercase_urlencoding': self.lowercase_encoding,
        'post_data': request.POST.copy(),
        'script_name': request.path_info,
    }

    # Empty port yields colon in the URL; avoid if we can.
    # Drop known/common ports to match meta.
    if server_port and server_port not in ignored_ports:
        prepared['server_port'] = server_port

    return prepared

IdP.prepare_request = prepare_request_proxy
merit-mthompson commented 4 years ago

And sorry, after looking and re-reading I think we might not need to change anything. Going to undo-my shenanigans and see what I can fix.

merit-mthompson commented 4 years ago

Removed all my monkey-patching and was able to get things working with this in our settings for certain hosts:

SECURE_PROXY_SSL_HEADER = ('HTTP_X_FORWARDED_PROTO', 'https')
USE_X_FORWARDED_HOST = True
USE_X_FORWARDED_PORT = True

Closing this, thanks for the reply and the direction!

dcwatson commented 4 years ago

Excellent, glad it’s working for you! Feel free to open issues for anything else you find.