From 08f1507f70e37c749a0e506624b1ef41cc8bce1d Mon Sep 17 00:00:00 2001 From: Chris Meyers Date: Fri, 13 Feb 2026 10:09:29 -0500 Subject: [PATCH] Change remote host finding logic * When the remote host header values contains a comma separated list, only consider the first entry. Previously we considered every item in the list. --- awx/api/views/__init__.py | 5 +- .../tests/functional/api/test_job_template.py | 44 ++++ awx/main/tests/unit/utils/test_proxy.py | 207 ++++++++++++++++++ awx/main/utils/proxy.py | 35 +++ 4 files changed, 289 insertions(+), 2 deletions(-) create mode 100644 awx/main/tests/unit/utils/test_proxy.py diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py index 1456e8bb16..4d86b61320 100644 --- a/awx/api/views/__init__.py +++ b/awx/api/views/__init__.py @@ -55,7 +55,6 @@ from wsgiref.util import FileWrapper from drf_spectacular.utils import extend_schema_view, extend_schema # django-ansible-base -from ansible_base.lib.utils.requests import get_remote_hosts from ansible_base.rbac.models import RoleEvaluation from ansible_base.lib.utils.schema import extend_schema_if_available @@ -98,6 +97,7 @@ from awx.main.utils import ( from awx.main.utils.encryption import encrypt_value from awx.main.utils.filters import SmartFilter from awx.main.utils.plugins import compute_cloud_inventory_sources +from awx.main.utils.proxy import get_first_remote_host_from_headers from awx.main.utils.common import memoize from awx.main.redact import UriCleaner from awx.api.permissions import ( @@ -2877,7 +2877,8 @@ class JobTemplateCallback(GenericAPIView): host for the current request. """ # Find the list of remote host names/IPs to check. - remote_hosts = set(get_remote_hosts(self.request)) + # Only consider the first entry from each header (for comma-separated values like X-Forwarded-For) + remote_hosts = get_first_remote_host_from_headers(self.request, settings.REMOTE_HOST_HEADERS) # Add the reverse lookup of IP addresses. for rh in list(remote_hosts): try: diff --git a/awx/main/tests/functional/api/test_job_template.py b/awx/main/tests/functional/api/test_job_template.py index 4cdf43d37b..ac648b3f39 100644 --- a/awx/main/tests/functional/api/test_job_template.py +++ b/awx/main/tests/functional/api/test_job_template.py @@ -485,3 +485,47 @@ class TestJobTemplateCallbackProxyIntegration: expect=400, **headers ) + + @override_settings(REMOTE_HOST_HEADERS=['HTTP_X_FROM_THE_LOAD_BALANCER', 'REMOTE_ADDR', 'REMOTE_HOST'], PROXY_IP_ALLOWED_LIST=[]) + def test_only_first_entry_in_comma_separated_header_is_considered(self, job_template, admin_user, post): + """ + Test that only the first entry in a comma-separated header value is used for host matching. + This is important for X-Forwarded-For style headers where the format is "client, proxy1, proxy2". + Only the original client (first entry) should be matched against inventory hosts. + """ + # Create host that matches the SECOND entry in the comma-separated list + job_template.inventory.hosts.create(name='second-host.example.com') + + headers = { + # First entry is 'first-host.example.com', second is 'second-host.example.com' + # Only the first should be considered, so this should NOT match + 'HTTP_X_FROM_THE_LOAD_BALANCER': 'first-host.example.com, second-host.example.com', + 'REMOTE_ADDR': 'unrelated-addr', + 'REMOTE_HOST': 'unrelated-host', + } + + # Should return 400 because only 'first-host.example.com' is considered, + # and that host is NOT in the inventory + r = post( + url=reverse('api:job_template_callback', kwargs={'pk': job_template.pk}), data={'host_config_key': 'abcd'}, user=admin_user, expect=400, **headers + ) + assert r.data['msg'] == 'No matching host could be found!' + + @override_settings(REMOTE_HOST_HEADERS=['HTTP_X_FROM_THE_LOAD_BALANCER', 'REMOTE_ADDR', 'REMOTE_HOST'], PROXY_IP_ALLOWED_LIST=[]) + def test_first_entry_in_comma_separated_header_matches(self, job_template, admin_user, post): + """ + Test that the first entry in a comma-separated header value correctly matches an inventory host. + """ + # Create host that matches the FIRST entry in the comma-separated list + job_template.inventory.hosts.create(name='first-host.example.com') + + headers = { + # First entry is 'first-host.example.com', second is 'second-host.example.com' + # The first entry matches the inventory host + 'HTTP_X_FROM_THE_LOAD_BALANCER': 'first-host.example.com, second-host.example.com', + 'REMOTE_ADDR': 'unrelated-addr', + 'REMOTE_HOST': 'unrelated-host', + } + + # Should return 201 because 'first-host.example.com' is the first entry and matches + post(url=reverse('api:job_template_callback', kwargs={'pk': job_template.pk}), data={'host_config_key': 'abcd'}, user=admin_user, expect=201, **headers) diff --git a/awx/main/tests/unit/utils/test_proxy.py b/awx/main/tests/unit/utils/test_proxy.py new file mode 100644 index 0000000000..775e1fed66 --- /dev/null +++ b/awx/main/tests/unit/utils/test_proxy.py @@ -0,0 +1,207 @@ +# Copyright (c) 2024 Ansible, Inc. +# All Rights Reserved. + +from unittest import mock + +from awx.main.utils.proxy import get_first_remote_host_from_headers, is_proxy_in_headers + + +class TestGetFirstRemoteHostFromHeaders: + """Tests for get_first_remote_host_from_headers function.""" + + def _make_mock_request(self, environ): + """Create a mock request with the given environ dict.""" + request = mock.MagicMock() + request.environ = environ + return request + + def test_single_value_headers(self): + """Test extraction from headers with single values (no commas).""" + request = self._make_mock_request( + { + "REMOTE_ADDR": "192.168.1.1", + "REMOTE_HOST": "client.example.com", + } + ) + headers = ["REMOTE_ADDR", "REMOTE_HOST"] + + result = get_first_remote_host_from_headers(request, headers) + + assert result == {"192.168.1.1", "client.example.com"} + + def test_comma_separated_only_first_entry(self): + """Test that only the first entry is extracted from comma-separated values.""" + request = self._make_mock_request( + { + "HTTP_X_FORWARDED_FOR": "10.0.0.1, 192.168.1.1, 172.16.0.1", + } + ) + headers = ["HTTP_X_FORWARDED_FOR"] + + result = get_first_remote_host_from_headers(request, headers) + + # Only the first IP should be included + assert result == {"10.0.0.1"} + # Subsequent IPs should NOT be included + assert "192.168.1.1" not in result + assert "172.16.0.1" not in result + + def test_comma_separated_with_whitespace(self): + """Test that whitespace is properly stripped from first entry.""" + request = self._make_mock_request( + { + "HTTP_X_FORWARDED_FOR": " 10.0.0.1 , 192.168.1.1", + } + ) + headers = ["HTTP_X_FORWARDED_FOR"] + + result = get_first_remote_host_from_headers(request, headers) + + assert result == {"10.0.0.1"} + + def test_multiple_headers_with_comma_separated(self): + """Test multiple headers where some have comma-separated values.""" + request = self._make_mock_request( + { + "HTTP_X_FORWARDED_FOR": "client.example.com, proxy1.example.com, proxy2.example.com", + "REMOTE_ADDR": "172.16.0.1", + "REMOTE_HOST": "proxy2.example.com", + } + ) + headers = ["HTTP_X_FORWARDED_FOR", "REMOTE_ADDR", "REMOTE_HOST"] + + result = get_first_remote_host_from_headers(request, headers) + + # Should have first entry from X-Forwarded-For plus the single values from other headers + assert result == {"client.example.com", "172.16.0.1", "proxy2.example.com"} + # Should NOT have subsequent entries from X-Forwarded-For + assert "proxy1.example.com" not in result + + def test_empty_header_value(self): + """Test handling of empty header values.""" + request = self._make_mock_request( + { + "HTTP_X_FORWARDED_FOR": "", + "REMOTE_ADDR": "192.168.1.1", + } + ) + headers = ["HTTP_X_FORWARDED_FOR", "REMOTE_ADDR"] + + result = get_first_remote_host_from_headers(request, headers) + + assert result == {"192.168.1.1"} + + def test_missing_header(self): + """Test handling of headers that don't exist in environ.""" + request = self._make_mock_request( + { + "REMOTE_ADDR": "192.168.1.1", + } + ) + headers = ["HTTP_X_FORWARDED_FOR", "REMOTE_ADDR", "REMOTE_HOST"] + + result = get_first_remote_host_from_headers(request, headers) + + assert result == {"192.168.1.1"} + + def test_empty_headers_list(self): + """Test with no headers specified.""" + request = self._make_mock_request( + { + "REMOTE_ADDR": "192.168.1.1", + } + ) + headers = [] + + result = get_first_remote_host_from_headers(request, headers) + + assert result == set() + + def test_whitespace_only_first_entry(self): + """Test handling when first entry is whitespace only.""" + request = self._make_mock_request( + { + "HTTP_X_FORWARDED_FOR": " , 192.168.1.1", + } + ) + headers = ["HTTP_X_FORWARDED_FOR"] + + result = get_first_remote_host_from_headers(request, headers) + + # Empty/whitespace first entry should be skipped + assert result == set() + + def test_single_entry_with_trailing_comma(self): + """Test single entry that happens to have a trailing comma.""" + request = self._make_mock_request( + { + "HTTP_X_FORWARDED_FOR": "10.0.0.1,", + } + ) + headers = ["HTTP_X_FORWARDED_FOR"] + + result = get_first_remote_host_from_headers(request, headers) + + assert result == {"10.0.0.1"} + + +class TestIsProxyInHeaders: + """Tests for is_proxy_in_headers function.""" + + def _make_mock_request(self, environ): + """Create a mock request with the given environ dict.""" + request = mock.MagicMock() + request.environ = environ + return request + + def test_proxy_found_in_single_value(self): + """Test proxy detection in single-value header.""" + request = self._make_mock_request( + { + "REMOTE_ADDR": "192.168.1.1", + } + ) + + result = is_proxy_in_headers(request, ["192.168.1.1"], ["REMOTE_ADDR"]) + + assert result is True + + def test_proxy_found_in_comma_separated(self): + """Test proxy detection in comma-separated header value.""" + request = self._make_mock_request( + { + "HTTP_X_FORWARDED_FOR": "10.0.0.1, 192.168.1.1, 172.16.0.1", + } + ) + + result = is_proxy_in_headers(request, ["192.168.1.1"], ["HTTP_X_FORWARDED_FOR"]) + + assert result is True + + def test_proxy_not_found(self): + """Test when proxy is not in any header.""" + request = self._make_mock_request( + { + "REMOTE_ADDR": "10.0.0.1", + } + ) + + result = is_proxy_in_headers(request, ["192.168.1.1"], ["REMOTE_ADDR"]) + + assert result is False + + def test_multiple_proxies_one_match(self): + """Test with multiple allowed proxies, one matches.""" + request = self._make_mock_request( + { + "REMOTE_HOST": "proxy.example.com", + } + ) + + result = is_proxy_in_headers( + request, + ["proxy1.example.com", "proxy.example.com", "proxy2.example.com"], + ["REMOTE_HOST"], + ) + + assert result is True diff --git a/awx/main/utils/proxy.py b/awx/main/utils/proxy.py index 0676e8f44a..89aa605094 100644 --- a/awx/main/utils/proxy.py +++ b/awx/main/utils/proxy.py @@ -45,3 +45,38 @@ def delete_headers_starting_with_http(request: Request, headers: list[str]): for header in headers: if header.startswith('HTTP_'): request.environ.pop(header, None) + + +def get_first_remote_host_from_headers(request: Request, headers: list[str]) -> set[str]: + """ + Extract remote host addresses from headers, considering only the first entry + in comma-separated values. + + For headers like X-Forwarded-For that may contain multiple IPs (e.g., "client, proxy1, proxy2"), + only the first entry (the original client) is considered. + + Example: + request.environ = { + "HTTP_X_FORWARDED_FOR": "10.0.0.1, 192.168.1.1, 172.16.0.1", + "REMOTE_ADDR": "192.168.1.1", + "REMOTE_HOST": "proxy.example.com" + } + headers = ["HTTP_X_FORWARDED_FOR", "REMOTE_ADDR", "REMOTE_HOST"] + + Returns: {"10.0.0.1", "192.168.1.1", "proxy.example.com"} + (Only the first IP "10.0.0.1" from X-Forwarded-For, not the full chain) + + request: The DRF/Django request. request.environ dict will be used for extracting hosts + headers: A list of header keys to check for remote host values + """ + remote_hosts = set() + + for header in headers: + header_value = request.environ.get(header, '') + if header_value: + # Only take the first entry if comma-separated + first_value = header_value.split(',')[0].strip() + if first_value: + remote_hosts.add(first_value) + + return remote_hosts