From 84ffd4a9924bdd92a5b607c28e7b0bd653f02d31 Mon Sep 17 00:00:00 2001 From: Jake Howard Date: Tue, 7 May 2024 16:46:27 +0100 Subject: [PATCH] Defer IP detection to the user The correct way to do this varies so much, that it's not possible to implement 1 universal implementation --- README.rst | 27 ++++++++++++++++++++++----- baipw/middleware.py | 9 +++------ baipw/tests/test_utils.py | 36 ------------------------------------ baipw/utils.py | 24 +++++------------------- 4 files changed, 30 insertions(+), 66 deletions(-) diff --git a/README.rst b/README.rst index a6e88e7..d9973dc 100644 --- a/README.rst +++ b/README.rst @@ -61,6 +61,8 @@ that you want to be able to access the website without authentication from. It must be either a string with networks separated by comma or Python iterable. +**Warning**: See [Getting IP Address](#getting-ip-address) below for caveats around IP address detection. + ``BASIC_AUTH_REALM`` ~~~~~~~~~~~~~~~~~~~~ @@ -84,12 +86,27 @@ Example settings Advanced customisation ---------------------- -Getting IP -~~~~~~~~~~ +Getting IP Address +~~~~~~~~~~~~~~~~~~ + +By default, ``BasicAuthIPWhitelistMiddleware`` uses ``request.META["REMOTE_ADDR"]`` +as the client's IP, which corresponds to the IP address connecting to Django. +If you have a reverse proxy (eg ``nginx`` in front), this will result in the IP address of +``nginx``, not the client. + +Correctly determining the IP address can vary between deployments. Guessing incorrectly can +result in security issues. Instead, this library requires you configure this yourselves. + +In most deployments, the ``X-Forwarded-For`` header can be used to correctly determine the +client's IP. We recommend `django-xff `__ to help parse this +header correctly. Because ``django-xff`` overrides ``REMOTE_ADDR`` by default, it is natively +supported by ``BasicAuthIPWhitelistMiddleware``. + +`django-ipware `__ is another popular +library, however may take more customization to implement. -If you want to have a custom behaviour when getting IP, you can create a -custom function that takes request as a parameter and specify path to it -in the ``BASIC_AUTH_GET_CLIENT_IP_FUNCTION`` settings, e.g. +To fully customize IP address detection, you can set ``BASIC_AUTH_GET_CLIENT_IP_FUNCTION`` to +a function which takes a request and returns a valid IP address: .. code:: python diff --git a/baipw/middleware.py b/baipw/middleware.py index 9cd989f..381e2b6 100644 --- a/baipw/middleware.py +++ b/baipw/middleware.py @@ -6,7 +6,7 @@ from .exceptions import Unauthorized from .response import HttpUnauthorizedResponse -from .utils import authorize, get_client_ip +from .utils import authorize class BasicAuthIPWhitelistMiddleware: @@ -61,11 +61,8 @@ def _basic_auth_response(self, request): return self.get_response_class()(request=request) def _get_client_ip(self, request): - function_path = getattr(settings, "BASIC_AUTH_GET_CLIENT_IP_FUNCTION", None) - func = get_client_ip - if function_path is not None: - func = import_string(function_path) - return func(request) + function_path = getattr(settings, "BASIC_AUTH_GET_CLIENT_IP_FUNCTION", "baipw.utils.get_client_ip") + return import_string(function_path)(request) def _get_whitelisted_networks(self): networks = getattr(settings, "BASIC_AUTH_WHITELISTED_IP_NETWORKS", []) diff --git a/baipw/tests/test_utils.py b/baipw/tests/test_utils.py index 6012abb..10f1e68 100644 --- a/baipw/tests/test_utils.py +++ b/baipw/tests/test_utils.py @@ -43,39 +43,3 @@ def test_authorise_with_invalid_header(self): self.assertEqual( str(e.exception), "Invalid format of the authorization header." ) - - -class TestGetClientIP(TestCase): - def setUp(self): - self.request = RequestFactory().get("/") - - def test_get_client_ip_from_remote_addr(self): - self.request.META["REMOTE_ADDR"] = "192.168.0.17" - self.assertNotIn("HTTP_X_FORWARDED_FOR", self.request.META) - self.assertIn("REMOTE_ADDR", self.request.META) - self.assertEqual(get_client_ip(self.request), "192.168.0.17") - - def test_get_client_ip_if_no_remote_addr_or_x_forwaded_for(self): - del self.request.META["REMOTE_ADDR"] - self.assertNotIn("HTTP_X_FORWARDED_FOR", self.request.META) - self.assertNotIn("REMOTE_ADDR", self.request.META) - self.assertIsNone(get_client_ip(self.request)) - - def test_get_client_ip_from_x_forwaded_for(self): - self.request.META["HTTP_X_FORWARDED_FOR"] = "72.123.123.89" - self.assertIn("HTTP_X_FORWARDED_FOR", self.request.META) - self.assertIn("REMOTE_ADDR", self.request.META) - self.assertEqual(get_client_ip(self.request), "72.123.123.89") - - def test_get_client_ip_from_x_forwaded_for_when_multiple_values(self): - self.request.META["HTTP_X_FORWARDED_FOR"] = "72.123.123.89,5.123.2.45" - self.assertIn("HTTP_X_FORWARDED_FOR", self.request.META) - self.assertIn("REMOTE_ADDR", self.request.META) - # Should use the last IP from the list. - self.assertEqual(get_client_ip(self.request), "5.123.2.45") - - def test_get_client_ip_prioritises_cloudflare_ip(self): - self.request.META["HTTP_CF_CONNECTING_IP"] = "72.123.123.90" - self.request.META["HTTP_X_FORWARDED_FOR"] = "110.123.123.89" - self.assertIn("REMOTE_ADDR", self.request.META) - self.assertEqual(get_client_ip(self.request), "72.123.123.90") diff --git a/baipw/utils.py b/baipw/utils.py index ba53d3b..9eb0239 100644 --- a/baipw/utils.py +++ b/baipw/utils.py @@ -7,26 +7,12 @@ def get_client_ip(request): - # IP retrieved from CloudFlare - cf_connecting_ip = request.META.get("HTTP_CF_CONNECTING_IP") - - # Header usually set by proxies - x_forwarded_for = request.META.get("HTTP_X_FORWARDED_FOR") - - # Header set by the connecting party, usually not the actual client making - # the request, but a web server that the request goes through. - remote_addr = request.META.get("REMOTE_ADDR") - - # Prioritise IPs from proxies. - final_ip = cf_connecting_ip or x_forwarded_for or remote_addr - - # If no IP address was attached to the address, return nothing. - if final_ip is None: - return + """ + Get the client's IP address - # If there is a list of IPs provided, use the last one (should be - # the most recent one). This may not work on Google Cloud. - return final_ip.split(",")[-1].strip() + Note: This is the address connecting to Django, which is likely incorrect. + """ + return request.META.get("REMOTE_ADDR") def authorize(request, configured_username, configured_password):