From ca97ca4c8dcbc4df3e6f10518bc7b249afb206d2 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Sun, 9 Aug 2020 20:25:43 +0200 Subject: [PATCH 01/35] Improve X-Forwarded-* request headers handling --- homeassistant/components/http/forwarded.py | 98 +++++++++++++++++++ homeassistant/components/http/real_ip.py | 41 -------- .../{test_real_ip.py => test_forwarded.py} | 23 ++--- 3 files changed, 108 insertions(+), 54 deletions(-) create mode 100644 homeassistant/components/http/forwarded.py delete mode 100644 homeassistant/components/http/real_ip.py rename tests/components/http/{test_real_ip.py => test_forwarded.py} (82%) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py new file mode 100644 index 00000000000000..a05666c5158261 --- /dev/null +++ b/homeassistant/components/http/forwarded.py @@ -0,0 +1,98 @@ +"""Middleware to handle forwarded data by a reverse proxy.""" +from ipaddress import ip_address +import logging + +from aiohttp.hdrs import X_FORWARDED_FOR, X_FORWARDED_HOST, X_FORWARDED_PROTO +from aiohttp.web import HTTPBadRequest, middleware + +from homeassistant.core import callback + +_LOGGER = logging.getLogger(__name__) + +# mypy: allow-untyped-defs + + +@callback +def setup_forwarded(app, use_x_forwarded_for, trusted_proxies): + """Create IP Ban middleware for the app.""" + + @middleware + async def forwarded_middleware(request, handler): + overrides = {} + + # Handle X-Forwarded-For + forwarded_for = request.headers.getall(X_FORWARDED_FOR, []) + if not forwarded_for: + # No forwarding headers, continue as normal + return await handler(request) + + # Multiple X-Forwarded-For headers + if len(forwarded_for) > 1: + _LOGGER.error("Too many headers for X-Forwarded-For", extra=request.headers) + raise HTTPBadRequest + + # Process IP addresses in the forwarded for header + forwarded_for = list(reversed(forwarded_for[0].split(","))) + try: + forwarded_for = [ + ip_address(addr) for addr in (a.strip() for a in forwarded_for) if addr + ] + except ValueError: + _LOGGER.error( + "Invalid IP address in X-Forwarded-For header", extra=request.headers + ) + raise HTTPBadRequest + + # Find the last trusted index in the X-Forwarded-For list + index = 0 + for forwarded_ip in forwarded_for: + if any(forwarded_ip in trusted_proxy for trusted_proxy in trusted_proxies): + index += 1 + continue + overrides["remote"] = str(forwarded_ip) + break + + # If all the IP addresses are from trusted networks, take the left-most. + if "remote" not in overrides: + index = -1 + overrides["remote"] = str(forwarded_for[-1]) + + # Handle X-Forwarded-Proto + forwarded_proto = list(reversed(request.headers.getall(X_FORWARDED_PROTO, []))) + if forwarded_proto: + if len(forwarded_proto) > 1: + _LOGGER.error( + "Too many headers for X_FORWARDED_PROTO header", + extra=request.headers, + ) + raise HTTPBadRequest + forwarded_proto = forwarded_proto[0].split(",") + forwarded_proto = [p.strip() for p in forwarded_proto] + + # Ideally this should take the scheme corresponding to the entry + # in X-Forwarded-For that was chosen, but some proxies (the + # Kubernetes NGINX ingress, for example) only retain one element + # in X-Forwarded-Proto. In that case, use what we have. + if index >= len(forwarded_proto): + index = -1 + + overrides["scheme"] = forwarded_proto[index] + + # Handle X-Forwarded-Host + forwarded_host = request.headers.getall(X_FORWARDED_HOST, []) + if forwarded_host: + # Multiple X-Forwarded-Host headers + if len(forwarded_host) > 1: + _LOGGER.error( + "Too many headers for X-Forwarded-Host", extra=request.headers + ) + raise HTTPBadRequest + + overrides["host"] = forwarded_host[0] + + request = request.clone(**overrides) + return await handler(request) + + # Only register middleware if `use_x_forwarded_for` is enabled + if use_x_forwarded_for: + app.middlewares.append(forwarded_middleware) diff --git a/homeassistant/components/http/real_ip.py b/homeassistant/components/http/real_ip.py deleted file mode 100644 index f2334ce0a2ff81..00000000000000 --- a/homeassistant/components/http/real_ip.py +++ /dev/null @@ -1,41 +0,0 @@ -"""Middleware to fetch real IP.""" -from ipaddress import ip_address - -from aiohttp.hdrs import X_FORWARDED_FOR -from aiohttp.web import middleware - -from homeassistant.core import callback - -from .const import KEY_REAL_IP - -# mypy: allow-untyped-defs - - -@callback -def setup_real_ip(app, use_x_forwarded_for, trusted_proxies): - """Create IP Ban middleware for the app.""" - - @middleware - async def real_ip_middleware(request, handler): - """Real IP middleware.""" - connected_ip = ip_address(request.transport.get_extra_info("peername")[0]) - request[KEY_REAL_IP] = connected_ip - - # Only use the XFF header if enabled, present, and from a trusted proxy - try: - if ( - use_x_forwarded_for - and X_FORWARDED_FOR in request.headers - and any( - connected_ip in trusted_proxy for trusted_proxy in trusted_proxies - ) - ): - request[KEY_REAL_IP] = ip_address( - request.headers.get(X_FORWARDED_FOR).split(", ")[-1] - ) - except ValueError: - pass - - return await handler(request) - - app.middlewares.append(real_ip_middleware) diff --git a/tests/components/http/test_real_ip.py b/tests/components/http/test_forwarded.py similarity index 82% rename from tests/components/http/test_real_ip.py rename to tests/components/http/test_forwarded.py index 2cb74df3176790..2dca3251e0d938 100644 --- a/tests/components/http/test_real_ip.py +++ b/tests/components/http/test_forwarded.py @@ -1,23 +1,22 @@ -"""Test real IP middleware.""" +"""Test real forwarded middleware.""" from ipaddress import ip_network from aiohttp import web from aiohttp.hdrs import X_FORWARDED_FOR -from homeassistant.components.http.const import KEY_REAL_IP -from homeassistant.components.http.real_ip import setup_real_ip +from homeassistant.components.http.forwarded import setup_forwarded async def mock_handler(request): """Return the real IP as text.""" - return web.Response(text=str(request[KEY_REAL_IP])) + return web.Response(text=request.remote) async def test_ignore_x_forwarded_for(aiohttp_client): """Test that we get the IP from the transport.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_real_ip(app, False, []) + setup_forwarded(app, False, []) mock_api_client = await aiohttp_client(app) @@ -31,7 +30,7 @@ async def test_use_x_forwarded_for_without_trusted_proxy(aiohttp_client): """Test that we get the IP from the transport.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_real_ip(app, True, []) + setup_forwarded(app, True, []) mock_api_client = await aiohttp_client(app) @@ -45,7 +44,7 @@ async def test_use_x_forwarded_for_with_trusted_proxy(aiohttp_client): """Test that we get the IP from the transport.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_real_ip(app, True, [ip_network("127.0.0.1")]) + setup_forwarded(app, True, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) @@ -59,7 +58,7 @@ async def test_use_x_forwarded_for_with_untrusted_proxy(aiohttp_client): """Test that we get the IP from the transport.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_real_ip(app, True, [ip_network("1.1.1.1")]) + setup_forwarded(app, True, [ip_network("1.1.1.1")]) mock_api_client = await aiohttp_client(app) @@ -73,7 +72,7 @@ async def test_use_x_forwarded_for_with_spoofed_header(aiohttp_client): """Test that we get the IP from the transport.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_real_ip(app, True, [ip_network("127.0.0.1")]) + setup_forwarded(app, True, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) @@ -89,13 +88,11 @@ async def test_use_x_forwarded_for_with_nonsense_header(aiohttp_client): """Test that we get the IP from the transport.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_real_ip(app, True, [ip_network("127.0.0.1")]) + setup_forwarded(app, True, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( "/", headers={X_FORWARDED_FOR: "This value is invalid"} ) - assert resp.status == 200 - text = await resp.text() - assert text == "127.0.0.1" + assert resp.status == 400 From 6d1af44276630bd0208215c731f56232f8701900 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Sun, 9 Aug 2020 20:43:28 +0200 Subject: [PATCH 02/35] Replace middleware in http server --- homeassistant/components/http/__init__.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/homeassistant/components/http/__init__.py b/homeassistant/components/http/__init__.py index 36e445089283bc..d4683ba6e61909 100644 --- a/homeassistant/components/http/__init__.py +++ b/homeassistant/components/http/__init__.py @@ -26,9 +26,9 @@ from .auth import setup_auth from .ban import setup_bans -from .const import KEY_AUTHENTICATED, KEY_HASS, KEY_HASS_USER, KEY_REAL_IP # noqa: F401 +from .const import KEY_AUTHENTICATED, KEY_HASS, KEY_HASS_USER # noqa: F401 from .cors import setup_cors -from .real_ip import setup_real_ip +from .forwarded import setup_forwarded from .request_context import setup_request_context from .static import CACHE_HEADERS, CachingStaticResource from .view import HomeAssistantView # noqa: F401 @@ -298,7 +298,7 @@ def __init__( # This order matters setup_request_context(app, current_request) - setup_real_ip(app, use_x_forwarded_for, trusted_proxies) + setup_forwarded(app, use_x_forwarded_for, trusted_proxies) if is_ban_enabled: setup_bans(hass, app, login_threshold) From 8d06bf27358f72ab8c2863c1779f4463aba84751 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Sun, 9 Aug 2020 20:46:18 +0200 Subject: [PATCH 03/35] Remove use of KEY_REAL_IP, replace by request.remote --- homeassistant/components/auth/__init__.py | 9 ++------- homeassistant/components/auth/login_flow.py | 10 +++++----- .../components/emulated_hue/hue_api.py | 18 +++++++++--------- homeassistant/components/hassio/auth.py | 6 +++--- homeassistant/components/http/auth.py | 4 ++-- homeassistant/components/http/ban.py | 8 +++----- homeassistant/components/http/const.py | 1 - homeassistant/components/http/view.py | 7 ++----- .../components/telegram_bot/webhooks.py | 4 ++-- homeassistant/components/webhook/__init__.py | 3 +-- tests/components/http/__init__.py | 4 +--- 11 files changed, 30 insertions(+), 44 deletions(-) diff --git a/homeassistant/components/auth/__init__.py b/homeassistant/components/auth/__init__.py index 5b1ca46c41bd65..fba96b24084380 100644 --- a/homeassistant/components/auth/__init__.py +++ b/homeassistant/components/auth/__init__.py @@ -127,7 +127,6 @@ User, ) from homeassistant.components import websocket_api -from homeassistant.components.http import KEY_REAL_IP from homeassistant.components.http.auth import async_sign_path from homeassistant.components.http.ban import log_invalid_auth from homeassistant.components.http.data_validator import RequestDataValidator @@ -252,14 +251,10 @@ async def post(self, request): return await self._async_handle_revoke_token(hass, data) if grant_type == "authorization_code": - return await self._async_handle_auth_code( - hass, data, str(request[KEY_REAL_IP]) - ) + return await self._async_handle_auth_code(hass, data, request.remote) if grant_type == "refresh_token": - return await self._async_handle_refresh_token( - hass, data, str(request[KEY_REAL_IP]) - ) + return await self._async_handle_refresh_token(hass, data, request.remote) return self.json( {"error": "unsupported_grant_type"}, status_code=HTTP_BAD_REQUEST diff --git a/homeassistant/components/auth/login_flow.py b/homeassistant/components/auth/login_flow.py index c5d824ce617d9d..7baa3e3421fd4e 100644 --- a/homeassistant/components/auth/login_flow.py +++ b/homeassistant/components/auth/login_flow.py @@ -71,7 +71,6 @@ import voluptuous_serialize from homeassistant import data_entry_flow -from homeassistant.components.http import KEY_REAL_IP from homeassistant.components.http.ban import ( log_invalid_auth, process_success_login, @@ -183,7 +182,7 @@ async def post(self, request, data): result = await self._flow_mgr.async_init( handler, context={ - "ip_address": request[KEY_REAL_IP], + "ip_address": request.remote, "credential_only": data.get("type") == "link_user", }, ) @@ -229,9 +228,10 @@ async def post(self, request, flow_id, data): try: # do not allow change ip during login flow for flow in self._flow_mgr.async_progress(): - if flow["flow_id"] == flow_id and flow["context"][ - "ip_address" - ] != request.get(KEY_REAL_IP): + if ( + flow["flow_id"] == flow_id + and flow["context"]["ip_address"] != request.remote + ): return self.json_message("IP address changed", HTTP_BAD_REQUEST) result = await self._flow_mgr.async_configure(flow_id, data) diff --git a/homeassistant/components/emulated_hue/hue_api.py b/homeassistant/components/emulated_hue/hue_api.py index b84e64e6cc6b1b..239dd85d5a0ace 100644 --- a/homeassistant/components/emulated_hue/hue_api.py +++ b/homeassistant/components/emulated_hue/hue_api.py @@ -1,6 +1,7 @@ """Support for a Hue API to control Home Assistant.""" import asyncio import hashlib +from ipaddress import ip_address import logging import time @@ -34,7 +35,6 @@ SUPPORT_SET_SPEED, ) from homeassistant.components.http import HomeAssistantView -from homeassistant.components.http.const import KEY_REAL_IP from homeassistant.components.humidifier.const import ( ATTR_HUMIDITY, SERVICE_SET_HUMIDITY, @@ -131,7 +131,7 @@ class HueUsernameView(HomeAssistantView): async def post(self, request): """Handle a POST request.""" - if not is_local(request[KEY_REAL_IP]): + if not is_local(ip_address(request.remote)): return self.json_message("Only local IPs allowed", HTTP_UNAUTHORIZED) try: @@ -159,7 +159,7 @@ def __init__(self, config): @core.callback def get(self, request, username): """Process a request to make the Brilliant Lightpad work.""" - if not is_local(request[KEY_REAL_IP]): + if not is_local(ip_address(request.remote)): return self.json_message("Only local IPs allowed", HTTP_UNAUTHORIZED) return self.json({}) @@ -179,7 +179,7 @@ def __init__(self, config): @core.callback def put(self, request, username): """Process a request to make the Logitech Pop working.""" - if not is_local(request[KEY_REAL_IP]): + if not is_local(ip_address(request.remote)): return self.json_message("Only local IPs allowed", HTTP_UNAUTHORIZED) return self.json( @@ -209,7 +209,7 @@ def __init__(self, config): @core.callback def get(self, request, username): """Process a request to get the list of available lights.""" - if not is_local(request[KEY_REAL_IP]): + if not is_local(ip_address(request.remote)): return self.json_message("Only local IPs allowed", HTTP_UNAUTHORIZED) return self.json(create_list_of_entities(self.config, request)) @@ -229,7 +229,7 @@ def __init__(self, config): @core.callback def get(self, request, username): """Process a request to get the list of available lights.""" - if not is_local(request[KEY_REAL_IP]): + if not is_local(ip_address(request.remote)): return self.json_message("only local IPs allowed", HTTP_UNAUTHORIZED) if username != HUE_API_USERNAME: return self.json(UNAUTHORIZED_USER) @@ -256,7 +256,7 @@ def __init__(self, config): @core.callback def get(self, request, username): """Process a request to get the configuration.""" - if not is_local(request[KEY_REAL_IP]): + if not is_local(ip_address(request.remote)): return self.json_message("only local IPs allowed", HTTP_UNAUTHORIZED) if username != HUE_API_USERNAME: return self.json(UNAUTHORIZED_USER) @@ -280,7 +280,7 @@ def __init__(self, config): @core.callback def get(self, request, username, entity_id): """Process a request to get the state of an individual light.""" - if not is_local(request[KEY_REAL_IP]): + if not is_local(ip_address(request.remote)): return self.json_message("Only local IPs allowed", HTTP_UNAUTHORIZED) hass = request.app["hass"] @@ -321,7 +321,7 @@ def __init__(self, config): async def put(self, request, username, entity_number): """Process a request to set the state of an individual light.""" - if not is_local(request[KEY_REAL_IP]): + if not is_local(ip_address(request.remote)): return self.json_message("Only local IPs allowed", HTTP_UNAUTHORIZED) config = self.config diff --git a/homeassistant/components/hassio/auth.py b/homeassistant/components/hassio/auth.py index 066219d77e8d8f..9b45a135712ee5 100644 --- a/homeassistant/components/hassio/auth.py +++ b/homeassistant/components/hassio/auth.py @@ -13,7 +13,7 @@ from homeassistant.auth.models import User from homeassistant.components.http import HomeAssistantView -from homeassistant.components.http.const import KEY_HASS_USER, KEY_REAL_IP +from homeassistant.components.http.const import KEY_HASS_USER from homeassistant.components.http.data_validator import RequestDataValidator from homeassistant.const import HTTP_OK from homeassistant.core import callback @@ -63,8 +63,8 @@ def _check_access(self, request: web.Request): """Check if this call is from Supervisor.""" # Check caller IP hassio_ip = os.environ["HASSIO"].split(":")[0] - if request[KEY_REAL_IP] != ip_address(hassio_ip): - _LOGGER.error("Invalid auth request from %s", request[KEY_REAL_IP]) + if ip_address(request.remote) != ip_address(hassio_ip): + _LOGGER.error("Invalid auth request from %s", request.remote) raise HTTPUnauthorized() # Check caller token diff --git a/homeassistant/components/http/auth.py b/homeassistant/components/http/auth.py index 18d8ce72d912d9..f9e6df944898c4 100644 --- a/homeassistant/components/http/auth.py +++ b/homeassistant/components/http/auth.py @@ -9,7 +9,7 @@ from homeassistant.core import callback from homeassistant.util import dt as dt_util -from .const import KEY_AUTHENTICATED, KEY_HASS_USER, KEY_REAL_IP +from .const import KEY_AUTHENTICATED, KEY_HASS_USER # mypy: allow-untyped-defs, no-check-untyped-defs @@ -118,7 +118,7 @@ async def auth_middleware(request, handler): if authenticated: _LOGGER.debug( "Authenticated %s for %s using %s", - request[KEY_REAL_IP], + request.remote, request.path, auth_type, ) diff --git a/homeassistant/components/http/ban.py b/homeassistant/components/http/ban.py index 8b8d2bc5671308..bf48012f6cec54 100644 --- a/homeassistant/components/http/ban.py +++ b/homeassistant/components/http/ban.py @@ -16,8 +16,6 @@ import homeassistant.helpers.config_validation as cv from homeassistant.util.yaml import dump -from .const import KEY_REAL_IP - # mypy: allow-untyped-defs, no-check-untyped-defs _LOGGER = logging.getLogger(__name__) @@ -61,7 +59,7 @@ async def ban_middleware(request, handler): return await handler(request) # Verify if IP is not banned - ip_address_ = request[KEY_REAL_IP] + ip_address_ = request.remote is_banned = any( ip_ban.ip_address == ip_address_ for ip_ban in request.app[KEY_BANNED_IPS] ) @@ -95,7 +93,7 @@ async def process_wrong_login(request): Increase failed login attempts counter for remote IP address. Add ip ban entry if failed login attempts exceeds threshold. """ - remote_addr = request[KEY_REAL_IP] + remote_addr = request.remote msg = f"Login attempt or request with invalid authentication from {remote_addr}" _LOGGER.warning(msg) @@ -144,7 +142,7 @@ async def process_success_login(request): No release IP address from banned list function, it can only be done by manual modify ip bans config file. """ - remote_addr = request[KEY_REAL_IP] + remote_addr = request.remote # Check if ban middleware is loaded if KEY_BANNED_IPS not in request.app or request.app[KEY_LOGIN_THRESHOLD] < 1: diff --git a/homeassistant/components/http/const.py b/homeassistant/components/http/const.py index 9392e790d62998..ebbc6cb9b81511 100644 --- a/homeassistant/components/http/const.py +++ b/homeassistant/components/http/const.py @@ -2,4 +2,3 @@ KEY_AUTHENTICATED = "ha_authenticated" KEY_HASS = "hass" KEY_HASS_USER = "hass_user" -KEY_REAL_IP = "ha_real_ip" diff --git a/homeassistant/components/http/view.py b/homeassistant/components/http/view.py index eb6c757384ece5..7c8e9281e420ef 100644 --- a/homeassistant/components/http/view.py +++ b/homeassistant/components/http/view.py @@ -18,7 +18,7 @@ from homeassistant.core import Context, is_callback from homeassistant.helpers.json import JSONEncoder -from .const import KEY_AUTHENTICATED, KEY_HASS, KEY_REAL_IP +from .const import KEY_AUTHENTICATED, KEY_HASS _LOGGER = logging.getLogger(__name__) @@ -116,10 +116,7 @@ async def handle(request: web.Request) -> web.StreamResponse: raise HTTPUnauthorized() _LOGGER.debug( - "Serving %s to %s (auth: %s)", - request.path, - request.get(KEY_REAL_IP), - authenticated, + "Serving %s to %s (auth: %s)", request.path, request.remote, authenticated, ) try: diff --git a/homeassistant/components/telegram_bot/webhooks.py b/homeassistant/components/telegram_bot/webhooks.py index 7c8f976a049413..4d5f41066e2d33 100644 --- a/homeassistant/components/telegram_bot/webhooks.py +++ b/homeassistant/components/telegram_bot/webhooks.py @@ -1,11 +1,11 @@ """Support for Telegram bots using webhooks.""" import datetime as dt +from ipaddress import ip_address import logging from telegram.error import TimedOut from homeassistant.components.http import HomeAssistantView -from homeassistant.components.http.const import KEY_REAL_IP from homeassistant.const import ( EVENT_HOMEASSISTANT_STOP, HTTP_BAD_REQUEST, @@ -96,7 +96,7 @@ def __init__(self, hass, allowed_chat_ids, trusted_networks): async def post(self, request): """Accept the POST from telegram.""" - real_ip = request[KEY_REAL_IP] + real_ip = ip_address(request.remote) if not any(real_ip in net for net in self.trusted_networks): _LOGGER.warning("Access denied from %s", real_ip) return self.json_message("Access denied", HTTP_UNAUTHORIZED) diff --git a/homeassistant/components/webhook/__init__.py b/homeassistant/components/webhook/__init__.py index 99226cabaa7a48..9c6dfe45e746cb 100644 --- a/homeassistant/components/webhook/__init__.py +++ b/homeassistant/components/webhook/__init__.py @@ -6,7 +6,6 @@ import voluptuous as vol from homeassistant.components import websocket_api -from homeassistant.components.http.const import KEY_REAL_IP from homeassistant.components.http.view import HomeAssistantView from homeassistant.const import HTTP_OK from homeassistant.core import callback @@ -80,7 +79,7 @@ async def async_handle_webhook(hass, webhook_id, request): if isinstance(request, MockRequest): received_from = request.mock_source else: - received_from = request[KEY_REAL_IP] + received_from = request.remote _LOGGER.warning( "Received message for unregistered webhook %s from %s", diff --git a/tests/components/http/__init__.py b/tests/components/http/__init__.py index e96f4a7fcf2418..671167313ec460 100644 --- a/tests/components/http/__init__.py +++ b/tests/components/http/__init__.py @@ -3,8 +3,6 @@ from aiohttp import web -from homeassistant.components.http.const import KEY_REAL_IP - # Relic from the past. Kept here so we can run negative tests. HTTP_HEADER_HA_AUTH = "X-HA-access" @@ -25,7 +23,7 @@ async def mock_real_ip(request, handler): """Mock Real IP middleware.""" nonlocal ip_to_mock - request[KEY_REAL_IP] = ip_address(ip_to_mock) + request["remote"] = ip_address(ip_to_mock) return await handler(request) From 167748e056e7bc9bc35adabf6637d731453d7950 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 06:31:08 +0200 Subject: [PATCH 04/35] Only set up middleware when enabled and when trusted proxies are configured --- homeassistant/components/http/__init__.py | 8 ++++++-- homeassistant/components/http/forwarded.py | 9 ++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/homeassistant/components/http/__init__.py b/homeassistant/components/http/__init__.py index d4683ba6e61909..e90667a7a0a659 100644 --- a/homeassistant/components/http/__init__.py +++ b/homeassistant/components/http/__init__.py @@ -296,9 +296,13 @@ def __init__( ) app[KEY_HASS] = hass - # This order matters + # Order matters, forwarded middleware needs to go first. + # Only register middleware if `use_x_forwarded_for` is enabled + # and trusted proxies are provided + if use_x_forwarded_for and trusted_proxies: + setup_forwarded(app, trusted_proxies) + setup_request_context(app, current_request) - setup_forwarded(app, use_x_forwarded_for, trusted_proxies) if is_ban_enabled: setup_bans(hass, app, login_threshold) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index a05666c5158261..c29cb55f9b2479 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -13,11 +13,12 @@ @callback -def setup_forwarded(app, use_x_forwarded_for, trusted_proxies): - """Create IP Ban middleware for the app.""" +def setup_forwarded(app, trusted_proxies): + """Create forwarded middleware for the app.""" @middleware async def forwarded_middleware(request, handler): + """Process forwarded data by a reverse proxy.""" overrides = {} # Handle X-Forwarded-For @@ -93,6 +94,4 @@ async def forwarded_middleware(request, handler): request = request.clone(**overrides) return await handler(request) - # Only register middleware if `use_x_forwarded_for` is enabled - if use_x_forwarded_for: - app.middlewares.append(forwarded_middleware) + app.middlewares.append(forwarded_middleware) From 1f6acbe127b4d583a203034898a5c097e912226f Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 06:48:00 +0200 Subject: [PATCH 05/35] Guard socket connected peer is trusted --- homeassistant/components/http/forwarded.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index c29cb55f9b2479..4711c4cf1c253b 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -27,6 +27,12 @@ async def forwarded_middleware(request, handler): # No forwarding headers, continue as normal return await handler(request) + # Ensure the IP of the connected peer is trusted + connected_ip = ip_address(request.transport.get_extra_info("peername")[0]) + if not any(connected_ip in trusted_proxy for trusted_proxy in trusted_proxies): + # Not trusted, continue as normal + return await handler(request) + # Multiple X-Forwarded-For headers if len(forwarded_for) > 1: _LOGGER.error("Too many headers for X-Forwarded-For", extra=request.headers) From 46b7c7bb277bb471a051ffc09059efc3a1295a99 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 06:56:06 +0200 Subject: [PATCH 06/35] Comments for clarification on the processing --- homeassistant/components/http/forwarded.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index 4711c4cf1c253b..a8cf3292008874 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -14,8 +14,18 @@ @callback def setup_forwarded(app, trusted_proxies): - """Create forwarded middleware for the app.""" + """Create forwarded middleware for the app. + Process IP addresses in the forwarded for headers + + `X-Forwarded-For: client, proxy1, proxy2` + + We go trough the list from the right side, and skip all entries that are in our + trusted proxies list. The first non-trusted IP is used as the client IP. If all + items in the X-Forwarded-For are trusted, including the most left item (client), + the most left item is used. + """ + # @middleware async def forwarded_middleware(request, handler): """Process forwarded data by a reverse proxy.""" @@ -38,7 +48,7 @@ async def forwarded_middleware(request, handler): _LOGGER.error("Too many headers for X-Forwarded-For", extra=request.headers) raise HTTPBadRequest - # Process IP addresses in the forwarded for header + # Process X-Forwarded-For from the right side (by reversing the list) forwarded_for = list(reversed(forwarded_for[0].split(","))) try: forwarded_for = [ From 2818d5d0eb54c55a6223572876560836152b4c45 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 07:11:14 +0200 Subject: [PATCH 07/35] Extensive information on the processing --- homeassistant/components/http/forwarded.py | 30 +++++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index a8cf3292008874..f2e526843c266a 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -16,7 +16,7 @@ def setup_forwarded(app, trusted_proxies): """Create forwarded middleware for the app. - Process IP addresses in the forwarded for headers + Process IP addresses, proto and host information in the forwarded for headers. `X-Forwarded-For: client, proxy1, proxy2` @@ -24,6 +24,29 @@ def setup_forwarded(app, trusted_proxies): trusted proxies list. The first non-trusted IP is used as the client IP. If all items in the X-Forwarded-For are trusted, including the most left item (client), the most left item is used. + + `X-Forwarded-Proto: client, proxy1, proxy2` + e.g, `X-Forwarded-Proto: https, http, http` + OR `X-Forwarded-Proto: https` (one entry, even with multiple proxies) + + The X-Forwarded-Proto is determined based on the corresponding entry of the + X-Forwarded-For header that is used / chosen as the client IP. However, + some proxies, for example, Kubernetes NGINX ingress, only retain one element + in the X-Forwarded-Proto header. In that case, we'll just use what we have. + + `X-Forwarded-Host: example.com` + + If the previous headers are processed successfully, and the X-Forwarded-Host is + present, it will be used. + + Additionally: + - If no X-Forwarded-For header is found, processing of all headers is skipped. + - If multiple instances of a X-Forwarded-For, X-Forwarded-Proto or + X-Forwarded-Host are found, a HTTP 400 status code is thrown. + - If malformed or invalid (IP) data in X-Forwarded-For header is found, + a HTTP 400 status code is thrown. + - The connected client peer on the socket of the incoming connection, + must be trusted for any processing to take place. """ # @middleware @@ -87,9 +110,8 @@ async def forwarded_middleware(request, handler): forwarded_proto = [p.strip() for p in forwarded_proto] # Ideally this should take the scheme corresponding to the entry - # in X-Forwarded-For that was chosen, but some proxies (the - # Kubernetes NGINX ingress, for example) only retain one element - # in X-Forwarded-Proto. In that case, use what we have. + # in X-Forwarded-For that was chosen, but some proxies only retain + # one element. In that case, use what we have. if index >= len(forwarded_proto): index = -1 From 5a0da60737f29097fb0ed49ccd7c483aa1643207 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 07:40:33 +0200 Subject: [PATCH 08/35] Tweaks information on the processing --- homeassistant/components/http/forwarded.py | 25 ++++++++++++---------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index f2e526843c266a..58a8848ec7434b 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -18,33 +18,36 @@ def setup_forwarded(app, trusted_proxies): Process IP addresses, proto and host information in the forwarded for headers. - `X-Forwarded-For: client, proxy1, proxy2` + `X-Forwarded-For: , , ` + e.g., `X-Forwarded-For: 203.0.113.195, 70.41.3.18, 150.172.238.178` - We go trough the list from the right side, and skip all entries that are in our + We go through the list from the right side, and skip all entries that are in our trusted proxies list. The first non-trusted IP is used as the client IP. If all items in the X-Forwarded-For are trusted, including the most left item (client), - the most left item is used. + the most left item is used. In the latter case, the client connection originated + from an IP that is also listed as a trusted proxy IP or network. - `X-Forwarded-Proto: client, proxy1, proxy2` - e.g, `X-Forwarded-Proto: https, http, http` + `X-Forwarded-Proto: , , ` + e.g., `X-Forwarded-Proto: https, http, http` OR `X-Forwarded-Proto: https` (one entry, even with multiple proxies) The X-Forwarded-Proto is determined based on the corresponding entry of the - X-Forwarded-For header that is used / chosen as the client IP. However, + X-Forwarded-For header that is used/chosen as the client IP. However, some proxies, for example, Kubernetes NGINX ingress, only retain one element in the X-Forwarded-Proto header. In that case, we'll just use what we have. - `X-Forwarded-Host: example.com` + `X-Forwarded-Host: ` + e.g., `X-Forwarded-Host: example.com` If the previous headers are processed successfully, and the X-Forwarded-Host is present, it will be used. Additionally: - - If no X-Forwarded-For header is found, processing of all headers is skipped. - - If multiple instances of a X-Forwarded-For, X-Forwarded-Proto or - X-Forwarded-Host are found, a HTTP 400 status code is thrown. + - If no X-Forwarded-For header is found, the processing of all headers is skipped. + - If multiple instances of X-Forwarded-For, X-Forwarded-Proto or + X-Forwarded-Host are found, an HTTP 400 status code is thrown. - If malformed or invalid (IP) data in X-Forwarded-For header is found, - a HTTP 400 status code is thrown. + an HTTP 400 status code is thrown. - The connected client peer on the socket of the incoming connection, must be trusted for any processing to take place. """ From 99ee73e34c1ce76930cbce830db774d553b7ddfa Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 08:01:38 +0200 Subject: [PATCH 09/35] Remove real_ip/forwarded middleware from emulated hue, doesn't do anything --- homeassistant/components/emulated_hue/__init__.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/homeassistant/components/emulated_hue/__init__.py b/homeassistant/components/emulated_hue/__init__.py index 9b4da5cfb21a72..8e8556536423d7 100644 --- a/homeassistant/components/emulated_hue/__init__.py +++ b/homeassistant/components/emulated_hue/__init__.py @@ -5,7 +5,6 @@ import voluptuous as vol from homeassistant import util -from homeassistant.components.http import real_ip from homeassistant.const import EVENT_HOMEASSISTANT_START, EVENT_HOMEASSISTANT_STOP from homeassistant.exceptions import HomeAssistantError import homeassistant.helpers.config_validation as cv @@ -101,7 +100,6 @@ async def async_setup(hass, yaml_config): app = web.Application() app["hass"] = hass - real_ip.setup_real_ip(app, False, []) # We misunderstood the startup signal. You're not allowed to change # anything during startup. Temp workaround. # pylint: disable=protected-access From f15fca9294075131702a9cfbbb365a7c69072e7e Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 08:07:38 +0200 Subject: [PATCH 10/35] Blow up on empty IP entry in X-Forwarded-For --- homeassistant/components/http/forwarded.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index 58a8848ec7434b..0d8a233e16ae5f 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -77,9 +77,7 @@ async def forwarded_middleware(request, handler): # Process X-Forwarded-For from the right side (by reversing the list) forwarded_for = list(reversed(forwarded_for[0].split(","))) try: - forwarded_for = [ - ip_address(addr) for addr in (a.strip() for a in forwarded_for) if addr - ] + forwarded_for = [ip_address(addr.strip()) for addr in forwarded_for] except ValueError: _LOGGER.error( "Invalid IP address in X-Forwarded-For header", extra=request.headers From 50e4837242294a9cfb60b1ea46ec86f36347e9a5 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 08:48:26 +0200 Subject: [PATCH 11/35] Adjust forwarded tests setup signature --- tests/components/http/test_forwarded.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/components/http/test_forwarded.py b/tests/components/http/test_forwarded.py index 2dca3251e0d938..43a675478b3023 100644 --- a/tests/components/http/test_forwarded.py +++ b/tests/components/http/test_forwarded.py @@ -16,7 +16,7 @@ async def test_ignore_x_forwarded_for(aiohttp_client): """Test that we get the IP from the transport.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, False, []) + setup_forwarded(app, []) mock_api_client = await aiohttp_client(app) @@ -30,7 +30,7 @@ async def test_use_x_forwarded_for_without_trusted_proxy(aiohttp_client): """Test that we get the IP from the transport.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, True, []) + setup_forwarded(app, []) mock_api_client = await aiohttp_client(app) @@ -44,7 +44,7 @@ async def test_use_x_forwarded_for_with_trusted_proxy(aiohttp_client): """Test that we get the IP from the transport.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, True, [ip_network("127.0.0.1")]) + setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) @@ -58,7 +58,7 @@ async def test_use_x_forwarded_for_with_untrusted_proxy(aiohttp_client): """Test that we get the IP from the transport.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, True, [ip_network("1.1.1.1")]) + setup_forwarded(app, [ip_network("1.1.1.1")]) mock_api_client = await aiohttp_client(app) @@ -72,7 +72,7 @@ async def test_use_x_forwarded_for_with_spoofed_header(aiohttp_client): """Test that we get the IP from the transport.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, True, [ip_network("127.0.0.1")]) + setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) @@ -88,7 +88,7 @@ async def test_use_x_forwarded_for_with_nonsense_header(aiohttp_client): """Test that we get the IP from the transport.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, True, [ip_network("127.0.0.1")]) + setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) From 87d9fe929035ed5e396a04871aaa44c989766788 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 09:03:57 +0200 Subject: [PATCH 12/35] Fix common IP mock in http tests --- tests/components/http/__init__.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/components/http/__init__.py b/tests/components/http/__init__.py index 671167313ec460..238f5c7050a9a1 100644 --- a/tests/components/http/__init__.py +++ b/tests/components/http/__init__.py @@ -1,6 +1,4 @@ """Tests for the HTTP component.""" -from ipaddress import ip_address - from aiohttp import web # Relic from the past. Kept here so we can run negative tests. @@ -23,7 +21,7 @@ async def mock_real_ip(request, handler): """Mock Real IP middleware.""" nonlocal ip_to_mock - request["remote"] = ip_address(ip_to_mock) + request = request.clone(remote=ip_to_mock) return await handler(request) From b440d8a6706a9f23cade24386c66db09ef6c8424 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 09:10:01 +0200 Subject: [PATCH 13/35] Fix auth tests --- tests/components/http/test_auth.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/components/http/test_auth.py b/tests/components/http/test_auth.py index 9282bf4587be1e..ef208a7e54e3ec 100644 --- a/tests/components/http/test_auth.py +++ b/tests/components/http/test_auth.py @@ -9,7 +9,7 @@ from homeassistant.auth.providers import trusted_networks from homeassistant.components.http.auth import async_sign_path, setup_auth from homeassistant.components.http.const import KEY_AUTHENTICATED -from homeassistant.components.http.real_ip import setup_real_ip +from homeassistant.components.http.forwarded import setup_forwarded from homeassistant.setup import async_setup_component from . import HTTP_HEADER_HA_AUTH, mock_real_ip @@ -54,7 +54,7 @@ def app(hass): app = web.Application() app["hass"] = hass app.router.add_get("/", mock_handler) - setup_real_ip(app, False, []) + setup_forwarded(app, []) return app From 7894b1425be6be776e292899f1770ffa41b860db Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 09:10:28 +0200 Subject: [PATCH 14/35] Adjust IP Ban handling --- homeassistant/components/http/ban.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/homeassistant/components/http/ban.py b/homeassistant/components/http/ban.py index bf48012f6cec54..5a5b08a05c82da 100644 --- a/homeassistant/components/http/ban.py +++ b/homeassistant/components/http/ban.py @@ -59,7 +59,7 @@ async def ban_middleware(request, handler): return await handler(request) # Verify if IP is not banned - ip_address_ = request.remote + ip_address_ = ip_address(request.remote) is_banned = any( ip_ban.ip_address == ip_address_ for ip_ban in request.app[KEY_BANNED_IPS] ) @@ -93,7 +93,7 @@ async def process_wrong_login(request): Increase failed login attempts counter for remote IP address. Add ip ban entry if failed login attempts exceeds threshold. """ - remote_addr = request.remote + remote_addr = ip_address(request.remote) msg = f"Login attempt or request with invalid authentication from {remote_addr}" _LOGGER.warning(msg) @@ -142,7 +142,7 @@ async def process_success_login(request): No release IP address from banned list function, it can only be done by manual modify ip bans config file. """ - remote_addr = request.remote + remote_addr = ip_address(request.remote) # Check if ban middleware is loaded if KEY_BANNED_IPS not in request.app or request.app[KEY_LOGIN_THRESHOLD] < 1: From 08dd958016019142898ff824729c0815562047c7 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 09:16:30 +0200 Subject: [PATCH 15/35] Fix emulated_hue tests --- tests/components/emulated_hue/test_hue_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/components/emulated_hue/test_hue_api.py b/tests/components/emulated_hue/test_hue_api.py index 510aa0ef8ee10a..9c6fb9d37a8dbb 100644 --- a/tests/components/emulated_hue/test_hue_api.py +++ b/tests/components/emulated_hue/test_hue_api.py @@ -1157,7 +1157,7 @@ async def test_external_ip_blocked(hue_client): postUrls = ["/api"] putUrls = ["/api/username/lights/light.ceiling_lights/state"] with patch( - "homeassistant.components.http.real_ip.ip_address", + "homeassistant.components.emulated_hue.hue_api.ip_address", return_value=ip_address("45.45.45.45"), ): for getUrl in getUrls: From ca5e7a648e2f7d19841f015a6d58ebf60e5c3eaf Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 10:12:31 +0200 Subject: [PATCH 16/35] Improve forwarded middleware tests + extend coverage --- homeassistant/components/http/forwarded.py | 13 +- tests/components/http/test_forwarded.py | 181 +++++++++++++++++---- 2 files changed, 154 insertions(+), 40 deletions(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index 0d8a233e16ae5f..4d6a7d504382a4 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -71,7 +71,7 @@ async def forwarded_middleware(request, handler): # Multiple X-Forwarded-For headers if len(forwarded_for) > 1: - _LOGGER.error("Too many headers for X-Forwarded-For", extra=request.headers) + _LOGGER.error("Too many headers for X-Forwarded-For: %s", forwarded_for) raise HTTPBadRequest # Process X-Forwarded-For from the right side (by reversing the list) @@ -79,9 +79,7 @@ async def forwarded_middleware(request, handler): try: forwarded_for = [ip_address(addr.strip()) for addr in forwarded_for] except ValueError: - _LOGGER.error( - "Invalid IP address in X-Forwarded-For header", extra=request.headers - ) + _LOGGER.error("Invalid IP address in X-Forwarded-For: %s", forwarded_for) raise HTTPBadRequest # Find the last trusted index in the X-Forwarded-For list @@ -102,10 +100,7 @@ async def forwarded_middleware(request, handler): forwarded_proto = list(reversed(request.headers.getall(X_FORWARDED_PROTO, []))) if forwarded_proto: if len(forwarded_proto) > 1: - _LOGGER.error( - "Too many headers for X_FORWARDED_PROTO header", - extra=request.headers, - ) + _LOGGER.error("Too many headers for X-Forward-For: %s", forwarded_proto) raise HTTPBadRequest forwarded_proto = forwarded_proto[0].split(",") forwarded_proto = [p.strip() for p in forwarded_proto] @@ -124,7 +119,7 @@ async def forwarded_middleware(request, handler): # Multiple X-Forwarded-Host headers if len(forwarded_host) > 1: _LOGGER.error( - "Too many headers for X-Forwarded-Host", extra=request.headers + "Too many headers for X-Forwarded-Host: %s", forwarded_host ) raise HTTPBadRequest diff --git a/tests/components/http/test_forwarded.py b/tests/components/http/test_forwarded.py index 43a675478b3023..1841921e13b88f 100644 --- a/tests/components/http/test_forwarded.py +++ b/tests/components/http/test_forwarded.py @@ -2,7 +2,7 @@ from ipaddress import ip_network from aiohttp import web -from aiohttp.hdrs import X_FORWARDED_FOR +from aiohttp.hdrs import X_FORWARDED_FOR, X_FORWARDED_HOST, X_FORWARDED_PROTO from homeassistant.components.http.forwarded import setup_forwarded @@ -12,64 +12,153 @@ async def mock_handler(request): return web.Response(text=request.remote) -async def test_ignore_x_forwarded_for(aiohttp_client): +async def test_x_forwarded_for_without_trusted_proxy(aiohttp_client): """Test that we get the IP from the transport.""" + + async def handler(request): + url = mock_api_client.make_url("/") + assert request.host == f"{url.host}:{url.port}" + assert request.scheme == "http" + assert not request.secure + assert request.remote == "127.0.0.1" + + return web.Response() + app = web.Application() - app.router.add_get("/", mock_handler) + app.router.add_get("/", handler) + setup_forwarded(app, []) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get("/", headers={X_FORWARDED_FOR: "255.255.255.255"}) assert resp.status == 200 - text = await resp.text() - assert text != "255.255.255.255" -async def test_use_x_forwarded_for_without_trusted_proxy(aiohttp_client): - """Test that we get the IP from the transport.""" +async def test_x_forwarded_for_with_trusted_proxy(aiohttp_client): + """Test that we get the IP from the forwarded for header.""" + + async def handler(request): + url = mock_api_client.make_url("/") + assert request.host == f"{url.host}:{url.port}" + assert request.scheme == "http" + assert not request.secure + assert request.remote == "255.255.255.255" + + return web.Response() + app = web.Application() - app.router.add_get("/", mock_handler) - setup_forwarded(app, []) + app.router.add_get("/", handler) + setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get("/", headers={X_FORWARDED_FOR: "255.255.255.255"}) assert resp.status == 200 - text = await resp.text() - assert text != "255.255.255.255" -async def test_use_x_forwarded_for_with_trusted_proxy(aiohttp_client): - """Test that we get the IP from the transport.""" +async def test_x_forwarded_for_with_untrusted_proxy(aiohttp_client): + """Test that we get the IP from transport with untrusted proxy.""" + + async def handler(request): + url = mock_api_client.make_url("/") + assert request.host == f"{url.host}:{url.port}" + assert request.scheme == "http" + assert not request.secure + assert request.remote == "127.0.0.1" + + return web.Response() + app = web.Application() - app.router.add_get("/", mock_handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + app.router.add_get("/", handler) + setup_forwarded(app, [ip_network("1.1.1.1")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get("/", headers={X_FORWARDED_FOR: "255.255.255.255"}) assert resp.status == 200 - text = await resp.text() - assert text == "255.255.255.255" -async def test_use_x_forwarded_for_with_untrusted_proxy(aiohttp_client): - """Test that we get the IP from the transport.""" +async def test_x_forwarded_for_with_spoofed_header(aiohttp_client): + """Test that we get the IP from the transport with a spoofed header.""" + + async def handler(request): + url = mock_api_client.make_url("/") + assert request.host == f"{url.host}:{url.port}" + assert request.scheme == "http" + assert not request.secure + assert request.remote == "255.255.255.255" + + return web.Response() + + app = web.Application() + app.router.add_get("/", handler) + setup_forwarded(app, [ip_network("127.0.0.1")]) + + mock_api_client = await aiohttp_client(app) + + resp = await mock_api_client.get( + "/", headers={X_FORWARDED_FOR: "222.222.222.222, 255.255.255.255"} + ) + assert resp.status == 200 + + +async def test_x_forwarded_for_with_nonsense_header(aiohttp_client): + """Test that we get a HTTP 400 bad request with a malformed header.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, [ip_network("1.1.1.1")]) + setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) - resp = await mock_api_client.get("/", headers={X_FORWARDED_FOR: "255.255.255.255"}) + resp = await mock_api_client.get( + "/", headers={X_FORWARDED_FOR: "This value is invalid"} + ) + assert resp.status == 400 + + +async def test_x_forwarded_for_with_multiple_headers(aiohttp_client): + """Test that we get a HTTP 400 bad request with multiple headers.""" + app = web.Application() + app.router.add_get("/", mock_handler) + setup_forwarded(app, [ip_network("127.0.0.1")]) + + mock_api_client = await aiohttp_client(app) + + resp = await mock_api_client.get( + "/", + headers=[ + (X_FORWARDED_FOR, "222.222.222.222"), + (X_FORWARDED_FOR, "123.123.123.123"), + ], + ) + assert resp.status == 400 + + +async def test_x_forwarded_proto_not_processed_without_for(aiohttp_client): + """Test that proto header isn't processed without a for header.""" + + async def handler(request): + url = mock_api_client.make_url("/") + assert request.host == f"{url.host}:{url.port}" + assert request.scheme == "http" + assert not request.secure + assert request.remote == "127.0.0.1" + + return web.Response() + + app = web.Application() + app.router.add_get("/", handler) + setup_forwarded(app, [ip_network("127.0.0.1")]) + + mock_api_client = await aiohttp_client(app) + + resp = await mock_api_client.get("/", headers={X_FORWARDED_PROTO: "https"}) assert resp.status == 200 - text = await resp.text() - assert text != "255.255.255.255" -async def test_use_x_forwarded_for_with_spoofed_header(aiohttp_client): - """Test that we get the IP from the transport.""" +async def test_x_forwarded_proto_with_multiple_headers(aiohttp_client): + """Test that we get a HTTP 400 bad request with multiple headers.""" app = web.Application() app.router.add_get("/", mock_handler) setup_forwarded(app, [ip_network("127.0.0.1")]) @@ -77,15 +166,40 @@ async def test_use_x_forwarded_for_with_spoofed_header(aiohttp_client): mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( - "/", headers={X_FORWARDED_FOR: "222.222.222.222, 255.255.255.255"} + "/", + headers=[ + (X_FORWARDED_FOR, "222.222.222.222"), + (X_FORWARDED_PROTO, "https"), + (X_FORWARDED_PROTO, "http"), + ], ) + assert resp.status == 400 + + +async def test_x_forwarded_host_not_processed_without_for(aiohttp_client): + """Test that host header isn't processed without a for header.""" + + async def handler(request): + url = mock_api_client.make_url("/") + assert request.host == f"{url.host}:{url.port}" + assert request.scheme == "http" + assert not request.secure + assert request.remote == "127.0.0.1" + + return web.Response() + + app = web.Application() + app.router.add_get("/", handler) + setup_forwarded(app, [ip_network("127.0.0.1")]) + + mock_api_client = await aiohttp_client(app) + + resp = await mock_api_client.get("/", headers={X_FORWARDED_HOST: "example.com"}) assert resp.status == 200 - text = await resp.text() - assert text == "255.255.255.255" -async def test_use_x_forwarded_for_with_nonsense_header(aiohttp_client): - """Test that we get the IP from the transport.""" +async def test_x_forwarded_host_with_multiple_headers(aiohttp_client): + """Test that we get a HTTP 400 bad request with multiple headers.""" app = web.Application() app.router.add_get("/", mock_handler) setup_forwarded(app, [ip_network("127.0.0.1")]) @@ -93,6 +207,11 @@ async def test_use_x_forwarded_for_with_nonsense_header(aiohttp_client): mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( - "/", headers={X_FORWARDED_FOR: "This value is invalid"} + "/", + headers=[ + (X_FORWARDED_FOR, "222.222.222.222"), + (X_FORWARDED_HOST, "example.com"), + (X_FORWARDED_HOST, "example.spoof"), + ], ) assert resp.status == 400 From 61370998a4e9cc7d37cfa05f0b703876f50af2d7 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 10:16:59 +0200 Subject: [PATCH 17/35] Add more test cases with malformed data in forwarding headers --- tests/components/http/test_forwarded.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/components/http/test_forwarded.py b/tests/components/http/test_forwarded.py index 1841921e13b88f..7cb21ebb97bd85 100644 --- a/tests/components/http/test_forwarded.py +++ b/tests/components/http/test_forwarded.py @@ -116,6 +116,16 @@ async def test_x_forwarded_for_with_nonsense_header(aiohttp_client): ) assert resp.status == 400 + resp = await mock_api_client.get( + "/", headers={X_FORWARDED_FOR: "1.1.1.1, , 1.2.3.4"} + ) + assert resp.status == 400 + + resp = await mock_api_client.get( + "/", headers={X_FORWARDED_FOR: "1.1.1.1, batman, 1.2.3.4"} + ) + assert resp.status == 400 + async def test_x_forwarded_for_with_multiple_headers(aiohttp_client): """Test that we get a HTTP 400 bad request with multiple headers.""" From 370d06024f200458dea25b48e7a2983797a5d2bb Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 14:34:00 +0200 Subject: [PATCH 18/35] Protect against empty host value --- homeassistant/components/http/forwarded.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index 4d6a7d504382a4..9d7edea2ef4629 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -123,8 +123,14 @@ async def forwarded_middleware(request, handler): ) raise HTTPBadRequest - overrides["host"] = forwarded_host[0] + forwarded_host = forwarded_host[0].strip() + if not forwarded_host: + _LOGGER.error("Empty value received in X-Forward-Host header") + raise HTTPBadRequest + + overrides["host"] = forwarded_host + # Done, create a new request based on gathered data. request = request.clone(**overrides) return await handler(request) From e518fc38b2bfbec39fd99b33b1be0b3b52607bf3 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 14:34:40 +0200 Subject: [PATCH 19/35] Require number of proto elements to be 1 or match forwarded for length --- homeassistant/components/http/forwarded.py | 25 ++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index 9d7edea2ef4629..b51181e9eb87fe 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -100,15 +100,32 @@ async def forwarded_middleware(request, handler): forwarded_proto = list(reversed(request.headers.getall(X_FORWARDED_PROTO, []))) if forwarded_proto: if len(forwarded_proto) > 1: - _LOGGER.error("Too many headers for X-Forward-For: %s", forwarded_proto) + _LOGGER.error( + "Too many headers for X-Forward-Proto: %s", forwarded_proto + ) + raise HTTPBadRequest + + forwarded_proto = [ + proto + for proto in (p.strip() for p in forwarded_proto[0].split(",")) + if proto + ] + + # The X-Forwarded-Proto contains either one element, or the equals number + # of elements as X-Forwarded-For + if len(forwarded_proto) != 1 and len(forwarded_proto) != len(forwarded_for): + _LOGGER.error( + "Incorrect number of elements in X-Forward-Proto. Expected 1 or %d, got %d: %s", + len(forwarded_for), + len(forwarded_proto), + forwarded_proto, + ) raise HTTPBadRequest - forwarded_proto = forwarded_proto[0].split(",") - forwarded_proto = [p.strip() for p in forwarded_proto] # Ideally this should take the scheme corresponding to the entry # in X-Forwarded-For that was chosen, but some proxies only retain # one element. In that case, use what we have. - if index >= len(forwarded_proto): + if index != -1 and index >= len(forwarded_proto): index = -1 overrides["scheme"] = forwarded_proto[index] From 0e3b587b1a67501d14adcb10f248b213b35d70fa Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 14:35:26 +0200 Subject: [PATCH 20/35] Add some serious test coverage to forwarded middleware --- tests/components/http/test_forwarded.py | 257 +++++++++++++++++++++--- 1 file changed, 230 insertions(+), 27 deletions(-) diff --git a/tests/components/http/test_forwarded.py b/tests/components/http/test_forwarded.py index 7cb21ebb97bd85..8080512a667065 100644 --- a/tests/components/http/test_forwarded.py +++ b/tests/components/http/test_forwarded.py @@ -3,6 +3,7 @@ from aiohttp import web from aiohttp.hdrs import X_FORWARDED_FOR, X_FORWARDED_HOST, X_FORWARDED_PROTO +import pytest from homeassistant.components.http.forwarded import setup_forwarded @@ -30,12 +31,30 @@ async def handler(request): setup_forwarded(app, []) mock_api_client = await aiohttp_client(app) - resp = await mock_api_client.get("/", headers={X_FORWARDED_FOR: "255.255.255.255"}) + assert resp.status == 200 -async def test_x_forwarded_for_with_trusted_proxy(aiohttp_client): +@pytest.mark.parametrize( + "trusted_proxies,x_forwarded_for,remote", + [ + ( + ["127.0.0.0/24", "1.1.1.1", "10.10.10.0/24"], + "10.10.10.10, 1.1.1.1", + "10.10.10.10", + ), + (["127.0.0.0/24", "1.1.1.1"], "123.123.123.123, 2.2.2.2, 1.1.1.1", "2.2.2.2"), + (["127.0.0.0/24"], "123.123.123.123, 2.2.2.2, 1.1.1.1", "1.1.1.1"), + (["127.0.0.0/24"], "127.0.0.1", "127.0.0.1"), + (["127.0.0.1", "1.1.1.1"], "123.123.123.123, 1.1.1.1", "123.123.123.123"), + (["127.0.0.1", "1.1.1.1"], "123.123.123.123, 2.2.2.2, 1.1.1.1", "2.2.2.2"), + (["127.0.0.1"], "255.255.255.255", "255.255.255.255"), + ], +) +async def test_x_forwarded_for_with_trusted_proxy( + trusted_proxies, x_forwarded_for, remote, aiohttp_client +): """Test that we get the IP from the forwarded for header.""" async def handler(request): @@ -43,17 +62,19 @@ async def handler(request): assert request.host == f"{url.host}:{url.port}" assert request.scheme == "http" assert not request.secure - assert request.remote == "255.255.255.255" + assert request.remote == remote return web.Response() app = web.Application() app.router.add_get("/", handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + setup_forwarded( + app, [ip_network(trusted_proxy) for trusted_proxy in trusted_proxies] + ) mock_api_client = await aiohttp_client(app) + resp = await mock_api_client.get("/", headers={X_FORWARDED_FOR: x_forwarded_for}) - resp = await mock_api_client.get("/", headers={X_FORWARDED_FOR: "255.255.255.255"}) assert resp.status == 200 @@ -74,8 +95,8 @@ async def handler(request): setup_forwarded(app, [ip_network("1.1.1.1")]) mock_api_client = await aiohttp_client(app) - resp = await mock_api_client.get("/", headers={X_FORWARDED_FOR: "255.255.255.255"}) + assert resp.status == 200 @@ -96,14 +117,28 @@ async def handler(request): setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) - resp = await mock_api_client.get( "/", headers={X_FORWARDED_FOR: "222.222.222.222, 255.255.255.255"} ) + assert resp.status == 200 -async def test_x_forwarded_for_with_nonsense_header(aiohttp_client): +@pytest.mark.parametrize( + "x_forwarded_for", + [ + "This value is invalid", + "1.1.1.1, , 1.2.3.4", + "1.1.1.1, batman, 1.2.3.4", + "192.168.0.0/24", + "192.168.0.0/24, 1.1.1.1", + ",", + "", + ], +) +async def test_x_forwarded_for_with_malformed_header( + x_forwarded_for, aiohttp_client, caplog +): """Test that we get a HTTP 400 bad request with a malformed header.""" app = web.Application() app.router.add_get("/", mock_handler) @@ -111,23 +146,13 @@ async def test_x_forwarded_for_with_nonsense_header(aiohttp_client): mock_api_client = await aiohttp_client(app) - resp = await mock_api_client.get( - "/", headers={X_FORWARDED_FOR: "This value is invalid"} - ) - assert resp.status == 400 - - resp = await mock_api_client.get( - "/", headers={X_FORWARDED_FOR: "1.1.1.1, , 1.2.3.4"} - ) - assert resp.status == 400 + resp = await mock_api_client.get("/", headers={X_FORWARDED_FOR: x_forwarded_for}) - resp = await mock_api_client.get( - "/", headers={X_FORWARDED_FOR: "1.1.1.1, batman, 1.2.3.4"} - ) assert resp.status == 400 + assert "Invalid IP address in X-Forwarded-For" in caplog.text -async def test_x_forwarded_for_with_multiple_headers(aiohttp_client): +async def test_x_forwarded_for_with_multiple_headers(aiohttp_client, caplog): """Test that we get a HTTP 400 bad request with multiple headers.""" app = web.Application() app.router.add_get("/", mock_handler) @@ -142,7 +167,86 @@ async def test_x_forwarded_for_with_multiple_headers(aiohttp_client): (X_FORWARDED_FOR, "123.123.123.123"), ], ) + assert resp.status == 400 + assert "Too many headers for X-Forwarded-For" in caplog.text + + +async def test_x_forwarded_proto_without_trusted_proxy(aiohttp_client): + """Test that proto header is ignored when untrusted.""" + + async def handler(request): + url = mock_api_client.make_url("/") + assert request.host == f"{url.host}:{url.port}" + assert request.scheme == "http" + assert not request.secure + assert request.remote == "127.0.0.1" + + return web.Response() + + app = web.Application() + app.router.add_get("/", handler) + + setup_forwarded(app, []) + + mock_api_client = await aiohttp_client(app) + resp = await mock_api_client.get( + "/", headers={X_FORWARDED_FOR: "255.255.255.255", X_FORWARDED_PROTO: "https"} + ) + + assert resp.status == 200 + + +async def test_x_forwarded_proto_with_trusted_proxy(aiohttp_client): + """Test that we get the proto header if proxy is trusted.""" + + async def handler(request): + url = mock_api_client.make_url("/") + assert request.host == f"{url.host}:{url.port}" + assert request.scheme == "https" + assert request.secure + assert request.remote == "255.255.255.255" + + return web.Response() + + app = web.Application() + app.router.add_get("/", handler) + setup_forwarded(app, [ip_network("127.0.0.1")]) + + mock_api_client = await aiohttp_client(app) + resp = await mock_api_client.get( + "/", headers={X_FORWARDED_FOR: "255.255.255.255", X_FORWARDED_PROTO: "https"} + ) + + assert resp.status == 200 + + +async def test_x_forwarded_proto_with_trusted_proxy_multiple_for(aiohttp_client): + """Test that we get the proto with 1 element in the proto, multiple in the for.""" + + async def handler(request): + url = mock_api_client.make_url("/") + assert request.host == f"{url.host}:{url.port}" + assert request.scheme == "https" + assert request.secure + assert request.remote == "255.255.255.255" + + return web.Response() + + app = web.Application() + app.router.add_get("/", handler) + setup_forwarded(app, [ip_network("127.0.0.0/24")]) + + mock_api_client = await aiohttp_client(app) + resp = await mock_api_client.get( + "/", + headers={ + X_FORWARDED_FOR: "255.255.255.255, 127.0.0.1, 127.0.0.2", + X_FORWARDED_PROTO: "https", + }, + ) + + assert resp.status == 200 async def test_x_forwarded_proto_not_processed_without_for(aiohttp_client): @@ -162,19 +266,18 @@ async def handler(request): setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) - resp = await mock_api_client.get("/", headers={X_FORWARDED_PROTO: "https"}) + assert resp.status == 200 -async def test_x_forwarded_proto_with_multiple_headers(aiohttp_client): +async def test_x_forwarded_proto_with_multiple_headers(aiohttp_client, caplog): """Test that we get a HTTP 400 bad request with multiple headers.""" app = web.Application() app.router.add_get("/", mock_handler) setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) - resp = await mock_api_client.get( "/", headers=[ @@ -183,7 +286,91 @@ async def test_x_forwarded_proto_with_multiple_headers(aiohttp_client): (X_FORWARDED_PROTO, "http"), ], ) + + assert resp.status == 400 + assert "Too many headers for X-Forward-Proto" in caplog.text + + +@pytest.mark.parametrize( + "x_forwarded_for,x_forwarded_proto,expected,got", + [ + ("1.1.1.1", "", 1, 0), + ("1.1.1.1, 2.2.2.2", "https, https, https", 2, 3), + ("1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4", "https, https, https", 4, 3), + ], +) +async def test_x_forwarded_proto_incorrect_number_of_elements( + x_forwarded_for, x_forwarded_proto, expected, got, aiohttp_client, caplog +): + """Test that we get a HTTP 400 bad request with multiple headers.""" + app = web.Application() + app.router.add_get("/", mock_handler) + setup_forwarded(app, [ip_network("127.0.0.1")]) + + mock_api_client = await aiohttp_client(app) + resp = await mock_api_client.get( + "/", + headers={ + X_FORWARDED_FOR: x_forwarded_for, + X_FORWARDED_PROTO: x_forwarded_proto, + }, + ) + assert resp.status == 400 + assert ( + f"Incorrect number of elements in X-Forward-Proto. Expected 1 or {expected}, got {got}" + in caplog.text + ) + + +async def test_x_forwarded_host_without_trusted_proxy(aiohttp_client): + """Test that host header is ignored when untrusted.""" + + async def handler(request): + url = mock_api_client.make_url("/") + assert request.host == f"{url.host}:{url.port}" + assert request.scheme == "http" + assert not request.secure + assert request.remote == "127.0.0.1" + + return web.Response() + + app = web.Application() + app.router.add_get("/", handler) + + setup_forwarded(app, []) + + mock_api_client = await aiohttp_client(app) + resp = await mock_api_client.get( + "/", + headers={X_FORWARDED_FOR: "255.255.255.255", X_FORWARDED_HOST: "example.com"}, + ) + + assert resp.status == 200 + + +async def test_x_forwarded_host_with_trusted_proxy(aiohttp_client): + """Test that we get the host header if proxy is trusted.""" + + async def handler(request): + assert request.host == "example.com" + assert request.scheme == "http" + assert not request.secure + assert request.remote == "255.255.255.255" + + return web.Response() + + app = web.Application() + app.router.add_get("/", handler) + setup_forwarded(app, [ip_network("127.0.0.1")]) + + mock_api_client = await aiohttp_client(app) + resp = await mock_api_client.get( + "/", + headers={X_FORWARDED_FOR: "255.255.255.255", X_FORWARDED_HOST: "example.com"}, + ) + + assert resp.status == 200 async def test_x_forwarded_host_not_processed_without_for(aiohttp_client): @@ -203,19 +390,18 @@ async def handler(request): setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) - resp = await mock_api_client.get("/", headers={X_FORWARDED_HOST: "example.com"}) + assert resp.status == 200 -async def test_x_forwarded_host_with_multiple_headers(aiohttp_client): +async def test_x_forwarded_host_with_multiple_headers(aiohttp_client, caplog): """Test that we get a HTTP 400 bad request with multiple headers.""" app = web.Application() app.router.add_get("/", mock_handler) setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) - resp = await mock_api_client.get( "/", headers=[ @@ -224,4 +410,21 @@ async def test_x_forwarded_host_with_multiple_headers(aiohttp_client): (X_FORWARDED_HOST, "example.spoof"), ], ) + + assert resp.status == 400 + assert "Too many headers for X-Forwarded-Host" in caplog.text + + +async def test_x_forwarded_host_with_empty_header(aiohttp_client, caplog): + """Test that we get a HTTP 400 bad request with empty host value.""" + app = web.Application() + app.router.add_get("/", mock_handler) + setup_forwarded(app, [ip_network("127.0.0.1")]) + + mock_api_client = await aiohttp_client(app) + resp = await mock_api_client.get( + "/", headers={X_FORWARDED_FOR: "222.222.222.222", X_FORWARDED_HOST: ""} + ) + assert resp.status == 400 + assert "Empty value received in X-Forward-Host header" in caplog.text From 3071fcef6f648e0bc6468130937f8d26407cd265 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 15:04:04 +0200 Subject: [PATCH 21/35] Log warning when forwarding headers received from untrusted connected peer --- homeassistant/components/http/forwarded.py | 5 +++++ tests/components/http/test_forwarded.py | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index b51181e9eb87fe..6455ec789455d0 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -44,6 +44,7 @@ def setup_forwarded(app, trusted_proxies): Additionally: - If no X-Forwarded-For header is found, the processing of all headers is skipped. + - Log a warning when untrusted connected peer provices X-Forwarded-For headers. - If multiple instances of X-Forwarded-For, X-Forwarded-Proto or X-Forwarded-Host are found, an HTTP 400 status code is thrown. - If malformed or invalid (IP) data in X-Forwarded-For header is found, @@ -66,6 +67,10 @@ async def forwarded_middleware(request, handler): # Ensure the IP of the connected peer is trusted connected_ip = ip_address(request.transport.get_extra_info("peername")[0]) if not any(connected_ip in trusted_proxy for trusted_proxy in trusted_proxies): + _LOGGER.warning( + "Received X-Forwarded-For header from untrusted proxy %s, headers not processed", + str(connected_ip), + ) # Not trusted, continue as normal return await handler(request) diff --git a/tests/components/http/test_forwarded.py b/tests/components/http/test_forwarded.py index 8080512a667065..55a664395283a3 100644 --- a/tests/components/http/test_forwarded.py +++ b/tests/components/http/test_forwarded.py @@ -13,7 +13,7 @@ async def mock_handler(request): return web.Response(text=request.remote) -async def test_x_forwarded_for_without_trusted_proxy(aiohttp_client): +async def test_x_forwarded_for_without_trusted_proxy(aiohttp_client, caplog): """Test that we get the IP from the transport.""" async def handler(request): @@ -34,6 +34,10 @@ async def handler(request): resp = await mock_api_client.get("/", headers={X_FORWARDED_FOR: "255.255.255.255"}) assert resp.status == 200 + assert ( + "Received X-Forwarded-For header from untrusted proxy 127.0.0.1, headers not processed" + in caplog.text + ) @pytest.mark.parametrize( From 463cb46aa153094e463105a7415017e9d5e9470d Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 15:04:24 +0200 Subject: [PATCH 22/35] Extend inline documentation in code --- homeassistant/components/http/forwarded.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index 6455ec789455d0..de49fd86d5f1ff 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -51,6 +51,12 @@ def setup_forwarded(app, trusted_proxies): an HTTP 400 status code is thrown. - The connected client peer on the socket of the incoming connection, must be trusted for any processing to take place. + - If the number of elements in X-Forwarded-Proto does not equal 1 or + is equal to the number of elements in X-Forwarded-For, an HTTP 400 + status code is thrown. + - If an empty X-Forwarded-Host is provided, a HTTP 400 status code is thrown. + - If an empty X-Forwarded-Proto is provided, or an empty element in the list, + a HTTP 400 status code is thrown. """ # @middleware From 3bcd039b32e784d02b5aaca5742ce5658c7406a9 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 15:30:43 +0200 Subject: [PATCH 23/35] Ensure ip_address is used with login flow --- homeassistant/components/auth/login_flow.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/homeassistant/components/auth/login_flow.py b/homeassistant/components/auth/login_flow.py index 7baa3e3421fd4e..31e3b7ea648d9d 100644 --- a/homeassistant/components/auth/login_flow.py +++ b/homeassistant/components/auth/login_flow.py @@ -66,6 +66,8 @@ "version": 1 } """ +from ipaddress import ip_address + from aiohttp import web import voluptuous as vol import voluptuous_serialize @@ -182,7 +184,7 @@ async def post(self, request, data): result = await self._flow_mgr.async_init( handler, context={ - "ip_address": request.remote, + "ip_address": ip_address(request.remote), "credential_only": data.get("type") == "link_user", }, ) @@ -228,10 +230,9 @@ async def post(self, request, flow_id, data): try: # do not allow change ip during login flow for flow in self._flow_mgr.async_progress(): - if ( - flow["flow_id"] == flow_id - and flow["context"]["ip_address"] != request.remote - ): + if flow["flow_id"] == flow_id and flow["context"][ + "ip_address" + ] != ip_address(request.remote): return self.json_message("IP address changed", HTTP_BAD_REQUEST) result = await self._flow_mgr.async_configure(flow_id, data) From 82a1569731c73190b5fc7045415240f02fda7d00 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 17:44:54 +0200 Subject: [PATCH 24/35] Apply suggestions from code review Co-authored-by: Paulus Schoutsen --- homeassistant/components/http/forwarded.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index de49fd86d5f1ff..c91b31ae371c32 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -54,9 +54,9 @@ def setup_forwarded(app, trusted_proxies): - If the number of elements in X-Forwarded-Proto does not equal 1 or is equal to the number of elements in X-Forwarded-For, an HTTP 400 status code is thrown. - - If an empty X-Forwarded-Host is provided, a HTTP 400 status code is thrown. + - If an empty X-Forwarded-Host is provided, an HTTP 400 status code is thrown. - If an empty X-Forwarded-Proto is provided, or an empty element in the list, - a HTTP 400 status code is thrown. + an HTTP 400 status code is thrown. """ # @middleware From 68c0025283a35a190726af8c97f4285606aa5957 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 18:20:25 +0200 Subject: [PATCH 25/35] Process review comments --- homeassistant/components/http/forwarded.py | 60 ++++++++++++---------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index c91b31ae371c32..e9c700eaeebd7f 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -58,15 +58,15 @@ def setup_forwarded(app, trusted_proxies): - If an empty X-Forwarded-Proto is provided, or an empty element in the list, an HTTP 400 status code is thrown. """ - # + @middleware async def forwarded_middleware(request, handler): """Process forwarded data by a reverse proxy.""" overrides = {} # Handle X-Forwarded-For - forwarded_for = request.headers.getall(X_FORWARDED_FOR, []) - if not forwarded_for: + forwarded_for_headers = request.headers.getall(X_FORWARDED_FOR, []) + if not forwarded_for_headers: # No forwarding headers, continue as normal return await handler(request) @@ -75,50 +75,56 @@ async def forwarded_middleware(request, handler): if not any(connected_ip in trusted_proxy for trusted_proxy in trusted_proxies): _LOGGER.warning( "Received X-Forwarded-For header from untrusted proxy %s, headers not processed", - str(connected_ip), + connected_ip, ) # Not trusted, continue as normal return await handler(request) # Multiple X-Forwarded-For headers - if len(forwarded_for) > 1: - _LOGGER.error("Too many headers for X-Forwarded-For: %s", forwarded_for) + if len(forwarded_for_headers) > 1: + _LOGGER.error( + "Too many headers for X-Forwarded-For: %s", forwarded_for_headers + ) raise HTTPBadRequest # Process X-Forwarded-For from the right side (by reversing the list) - forwarded_for = list(reversed(forwarded_for[0].split(","))) + forwarded_for_split = list(reversed(forwarded_for_headers[0].split(","))) try: - forwarded_for = [ip_address(addr.strip()) for addr in forwarded_for] + forwarded_for = [ip_address(addr.strip()) for addr in forwarded_for_split] except ValueError: - _LOGGER.error("Invalid IP address in X-Forwarded-For: %s", forwarded_for) + _LOGGER.error( + "Invalid IP address in X-Forwarded-For: %s", forwarded_for_headers[0] + ) raise HTTPBadRequest # Find the last trusted index in the X-Forwarded-For list - index = 0 + forwarded_for_index = 0 for forwarded_ip in forwarded_for: if any(forwarded_ip in trusted_proxy for trusted_proxy in trusted_proxies): - index += 1 + forwarded_for_index += 1 continue overrides["remote"] = str(forwarded_ip) break # If all the IP addresses are from trusted networks, take the left-most. if "remote" not in overrides: - index = -1 + forwarded_for_index = -1 overrides["remote"] = str(forwarded_for[-1]) # Handle X-Forwarded-Proto - forwarded_proto = list(reversed(request.headers.getall(X_FORWARDED_PROTO, []))) - if forwarded_proto: - if len(forwarded_proto) > 1: + forwarded_proto_headers = list( + reversed(request.headers.getall(X_FORWARDED_PROTO, [])) + ) + if forwarded_proto_headers: + if len(forwarded_proto_headers) > 1: _LOGGER.error( - "Too many headers for X-Forward-Proto: %s", forwarded_proto + "Too many headers for X-Forward-Proto: %s", forwarded_proto_headers ) raise HTTPBadRequest forwarded_proto = [ proto - for proto in (p.strip() for p in forwarded_proto[0].split(",")) + for proto in (p.strip() for p in forwarded_proto_headers[0].split(",")) if proto ] @@ -129,29 +135,31 @@ async def forwarded_middleware(request, handler): "Incorrect number of elements in X-Forward-Proto. Expected 1 or %d, got %d: %s", len(forwarded_for), len(forwarded_proto), - forwarded_proto, + forwarded_proto_headers[0], ) raise HTTPBadRequest # Ideally this should take the scheme corresponding to the entry # in X-Forwarded-For that was chosen, but some proxies only retain # one element. In that case, use what we have. - if index != -1 and index >= len(forwarded_proto): - index = -1 + if forwarded_for_index != -1 and forwarded_for_index >= len( + forwarded_proto + ): + forwarded_for_index = -1 - overrides["scheme"] = forwarded_proto[index] + overrides["scheme"] = forwarded_proto[forwarded_for_index] # Handle X-Forwarded-Host - forwarded_host = request.headers.getall(X_FORWARDED_HOST, []) - if forwarded_host: + forwarded_host_headers = request.headers.getall(X_FORWARDED_HOST, []) + if forwarded_host_headers: # Multiple X-Forwarded-Host headers - if len(forwarded_host) > 1: + if len(forwarded_host_headers) > 1: _LOGGER.error( - "Too many headers for X-Forwarded-Host: %s", forwarded_host + "Too many headers for X-Forwarded-Host: %s", forwarded_host_headers ) raise HTTPBadRequest - forwarded_host = forwarded_host[0].strip() + forwarded_host = forwarded_host_headers[0].strip() if not forwarded_host: _LOGGER.error("Empty value received in X-Forward-Host header") raise HTTPBadRequest From a6c1b1c69f23c5bc65ef5eaec098309a1cb04740 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 18:34:49 +0200 Subject: [PATCH 26/35] Check empty proto headers --- homeassistant/components/http/forwarded.py | 15 ++++++++++----- tests/components/http/test_forwarded.py | 21 ++++++++++++++++++++- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index e9c700eaeebd7f..b2f508d8eef9d1 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -122,11 +122,16 @@ async def forwarded_middleware(request, handler): ) raise HTTPBadRequest - forwarded_proto = [ - proto - for proto in (p.strip() for p in forwarded_proto_headers[0].split(",")) - if proto - ] + forwarded_proto_split = forwarded_proto_headers[0].split(",") + forwarded_proto = [proto.strip() for proto in forwarded_proto_split] + + # Catch empty values + if "" in forwarded_proto: + _LOGGER.error( + "Empty item received in X-Forward-Proto header: %s", + forwarded_proto_headers[0], + ) + raise HTTPBadRequest # The X-Forwarded-Proto contains either one element, or the equals number # of elements as X-Forwarded-For diff --git a/tests/components/http/test_forwarded.py b/tests/components/http/test_forwarded.py index 55a664395283a3..c6d269138ff02b 100644 --- a/tests/components/http/test_forwarded.py +++ b/tests/components/http/test_forwarded.py @@ -295,10 +295,29 @@ async def test_x_forwarded_proto_with_multiple_headers(aiohttp_client, caplog): assert "Too many headers for X-Forward-Proto" in caplog.text +@pytest.mark.parametrize( + "x_forwarded_proto", ["", ",", "https, , https", "https, https, "], +) +async def test_x_forwarded_proto_empty_element( + x_forwarded_proto, aiohttp_client, caplog +): + """Test that we get a HTTP 400 bad request with multiple headers.""" + app = web.Application() + app.router.add_get("/", mock_handler) + setup_forwarded(app, [ip_network("127.0.0.1")]) + + mock_api_client = await aiohttp_client(app) + resp = await mock_api_client.get( + "/", headers={X_FORWARDED_FOR: "1.1.1.1", X_FORWARDED_PROTO: x_forwarded_proto}, + ) + + assert resp.status == 400 + assert "Empty item received in X-Forward-Proto header" in caplog.text + + @pytest.mark.parametrize( "x_forwarded_for,x_forwarded_proto,expected,got", [ - ("1.1.1.1", "", 1, 0), ("1.1.1.1, 2.2.2.2", "https, https, https", 2, 3), ("1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4", "https, https, https", 4, 3), ], From db480846d9ce7102290fd69b36d7a23429e403f8 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 21:31:54 +0200 Subject: [PATCH 27/35] Fix X-Forwarded-Proto handling, adds tests --- homeassistant/components/http/forwarded.py | 18 ++++----- tests/components/http/test_forwarded.py | 47 ++++++++++++++++++---- 2 files changed, 47 insertions(+), 18 deletions(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index b2f508d8eef9d1..0184d24f96b4f1 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -112,9 +112,7 @@ async def forwarded_middleware(request, handler): overrides["remote"] = str(forwarded_for[-1]) # Handle X-Forwarded-Proto - forwarded_proto_headers = list( - reversed(request.headers.getall(X_FORWARDED_PROTO, [])) - ) + forwarded_proto_headers = request.headers.getall(X_FORWARDED_PROTO, []) if forwarded_proto_headers: if len(forwarded_proto_headers) > 1: _LOGGER.error( @@ -122,7 +120,9 @@ async def forwarded_middleware(request, handler): ) raise HTTPBadRequest - forwarded_proto_split = forwarded_proto_headers[0].split(",") + forwarded_proto_split = list( + reversed(forwarded_proto_headers[0].split(",")) + ) forwarded_proto = [proto.strip() for proto in forwarded_proto_split] # Catch empty values @@ -147,12 +147,10 @@ async def forwarded_middleware(request, handler): # Ideally this should take the scheme corresponding to the entry # in X-Forwarded-For that was chosen, but some proxies only retain # one element. In that case, use what we have. - if forwarded_for_index != -1 and forwarded_for_index >= len( - forwarded_proto - ): - forwarded_for_index = -1 - - overrides["scheme"] = forwarded_proto[forwarded_for_index] + if forwarded_for_index > 1 or len(forwarded_proto) == 1: + overrides["scheme"] = forwarded_proto[-1] + else: + overrides["scheme"] = forwarded_proto[forwarded_for_index] # Handle X-Forwarded-Host forwarded_host_headers = request.headers.getall(X_FORWARDED_HOST, []) diff --git a/tests/components/http/test_forwarded.py b/tests/components/http/test_forwarded.py index c6d269138ff02b..df5b8bb0fef4cd 100644 --- a/tests/components/http/test_forwarded.py +++ b/tests/components/http/test_forwarded.py @@ -201,25 +201,56 @@ async def handler(request): assert resp.status == 200 -async def test_x_forwarded_proto_with_trusted_proxy(aiohttp_client): +@pytest.mark.parametrize( + "x_forwarded_for,remote,x_forwarded_proto,secure", + [ + ("10.10.10.10, 127.0.0.1, 127.0.0.2", "10.10.10.10", "https, http, http", True), + ("10.10.10.10, 127.0.0.1, 127.0.0.2", "10.10.10.10", "http", False), + ( + "10.10.10.10, 127.0.0.1, 127.0.0.2", + "10.10.10.10", + "http, https, https", + False, + ), + ("10.10.10.10, 127.0.0.1, 127.0.0.2", "10.10.10.10", "https", True), + ( + "255.255.255.255, 10.10.10.10, 127.0.0.1", + "10.10.10.10", + "http, https, http", + True, + ), + ( + "255.255.255.255, 10.10.10.10, 127.0.0.1", + "10.10.10.10", + "https, http, https", + False, + ), + ("255.255.255.255, 10.10.10.10, 127.0.0.1", "10.10.10.10", "https", True), + ], +) +async def test_x_forwarded_proto_with_trusted_proxy( + x_forwarded_for, remote, x_forwarded_proto, secure, aiohttp_client +): """Test that we get the proto header if proxy is trusted.""" async def handler(request): - url = mock_api_client.make_url("/") - assert request.host == f"{url.host}:{url.port}" - assert request.scheme == "https" - assert request.secure - assert request.remote == "255.255.255.255" + assert request.remote == remote + assert request.scheme == ("https" if secure else "http") + assert request.secure == secure return web.Response() app = web.Application() app.router.add_get("/", handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + setup_forwarded(app, [ip_network("127.0.0.0/24")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( - "/", headers={X_FORWARDED_FOR: "255.255.255.255", X_FORWARDED_PROTO: "https"} + "/", + headers={ + X_FORWARDED_FOR: x_forwarded_for, + X_FORWARDED_PROTO: x_forwarded_proto, + }, ) assert resp.status == 200 From bd6c75f7d0dc608af1b0f8b76d6ea7bf5ee4e290 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Mon, 10 Aug 2020 23:18:01 +0200 Subject: [PATCH 28/35] Additional tests for handling spaceless separators in headers --- tests/components/http/test_forwarded.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/components/http/test_forwarded.py b/tests/components/http/test_forwarded.py index df5b8bb0fef4cd..d0d226d2b470e0 100644 --- a/tests/components/http/test_forwarded.py +++ b/tests/components/http/test_forwarded.py @@ -49,6 +49,7 @@ async def handler(request): "10.10.10.10", ), (["127.0.0.0/24", "1.1.1.1"], "123.123.123.123, 2.2.2.2, 1.1.1.1", "2.2.2.2"), + (["127.0.0.0/24", "1.1.1.1"], "123.123.123.123,2.2.2.2,1.1.1.1", "2.2.2.2"), (["127.0.0.0/24"], "123.123.123.123, 2.2.2.2, 1.1.1.1", "1.1.1.1"), (["127.0.0.0/24"], "127.0.0.1", "127.0.0.1"), (["127.0.0.1", "1.1.1.1"], "123.123.123.123, 1.1.1.1", "123.123.123.123"), @@ -133,6 +134,7 @@ async def handler(request): [ "This value is invalid", "1.1.1.1, , 1.2.3.4", + "1.1.1.1,,1.2.3.4", "1.1.1.1, batman, 1.2.3.4", "192.168.0.0/24", "192.168.0.0/24, 1.1.1.1", @@ -205,6 +207,7 @@ async def handler(request): "x_forwarded_for,remote,x_forwarded_proto,secure", [ ("10.10.10.10, 127.0.0.1, 127.0.0.2", "10.10.10.10", "https, http, http", True), + ("10.10.10.10, 127.0.0.1, 127.0.0.2", "10.10.10.10", "https,http,http", True), ("10.10.10.10, 127.0.0.1, 127.0.0.2", "10.10.10.10", "http", False), ( "10.10.10.10, 127.0.0.1, 127.0.0.2", From 4b164c695f0e4168396a8e271b8c814e85935a5f Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Tue, 11 Aug 2020 00:22:28 +0200 Subject: [PATCH 29/35] Use for/else instead of list lookup --- homeassistant/components/http/forwarded.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index 0184d24f96b4f1..1780c36e8f7ca9 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -105,9 +105,8 @@ async def forwarded_middleware(request, handler): continue overrides["remote"] = str(forwarded_ip) break - - # If all the IP addresses are from trusted networks, take the left-most. - if "remote" not in overrides: + else: + # If all the IP addresses are from trusted networks, take the left-most. forwarded_for_index = -1 overrides["remote"] = str(forwarded_for[-1]) From c67eb133986b8cf79a156f614bf49e7e34f520a1 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Tue, 11 Aug 2020 09:14:19 +0200 Subject: [PATCH 30/35] Apply suggestions from code review Co-authored-by: Martin Hjelmare --- homeassistant/components/http/forwarded.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index 1780c36e8f7ca9..a92a57584461de 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -44,7 +44,7 @@ def setup_forwarded(app, trusted_proxies): Additionally: - If no X-Forwarded-For header is found, the processing of all headers is skipped. - - Log a warning when untrusted connected peer provices X-Forwarded-For headers. + - Log a warning when untrusted connected peer provides X-Forwarded-For headers. - If multiple instances of X-Forwarded-For, X-Forwarded-Proto or X-Forwarded-Host are found, an HTTP 400 status code is thrown. - If malformed or invalid (IP) data in X-Forwarded-For header is found, @@ -134,7 +134,7 @@ async def forwarded_middleware(request, handler): # The X-Forwarded-Proto contains either one element, or the equals number # of elements as X-Forwarded-For - if len(forwarded_proto) != 1 and len(forwarded_proto) != len(forwarded_for): + if len(forwarded_proto) not in (1, len(forwarded_for)): _LOGGER.error( "Incorrect number of elements in X-Forward-Proto. Expected 1 or %d, got %d: %s", len(forwarded_for), From cf3eaf31cf9d2b0d9c8618e355529bfe2ebef76b Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Tue, 11 Aug 2020 09:19:11 +0200 Subject: [PATCH 31/35] async_setup_forwarded --- homeassistant/components/http/__init__.py | 4 +-- homeassistant/components/http/forwarded.py | 2 +- tests/components/http/test_auth.py | 4 +-- tests/components/http/test_forwarded.py | 38 +++++++++++----------- 4 files changed, 24 insertions(+), 24 deletions(-) diff --git a/homeassistant/components/http/__init__.py b/homeassistant/components/http/__init__.py index e90667a7a0a659..cb0ecec8a2b356 100644 --- a/homeassistant/components/http/__init__.py +++ b/homeassistant/components/http/__init__.py @@ -28,7 +28,7 @@ from .ban import setup_bans from .const import KEY_AUTHENTICATED, KEY_HASS, KEY_HASS_USER # noqa: F401 from .cors import setup_cors -from .forwarded import setup_forwarded +from .forwarded import async_setup_forwarded from .request_context import setup_request_context from .static import CACHE_HEADERS, CachingStaticResource from .view import HomeAssistantView # noqa: F401 @@ -300,7 +300,7 @@ def __init__( # Only register middleware if `use_x_forwarded_for` is enabled # and trusted proxies are provided if use_x_forwarded_for and trusted_proxies: - setup_forwarded(app, trusted_proxies) + async_setup_forwarded(app, trusted_proxies) setup_request_context(app, current_request) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index a92a57584461de..86db130a4851bc 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -13,7 +13,7 @@ @callback -def setup_forwarded(app, trusted_proxies): +def async_setup_forwarded(app, trusted_proxies): """Create forwarded middleware for the app. Process IP addresses, proto and host information in the forwarded for headers. diff --git a/tests/components/http/test_auth.py b/tests/components/http/test_auth.py index ef208a7e54e3ec..e3274ddfa7d33e 100644 --- a/tests/components/http/test_auth.py +++ b/tests/components/http/test_auth.py @@ -9,7 +9,7 @@ from homeassistant.auth.providers import trusted_networks from homeassistant.components.http.auth import async_sign_path, setup_auth from homeassistant.components.http.const import KEY_AUTHENTICATED -from homeassistant.components.http.forwarded import setup_forwarded +from homeassistant.components.http.forwarded import async_setup_forwarded from homeassistant.setup import async_setup_component from . import HTTP_HEADER_HA_AUTH, mock_real_ip @@ -54,7 +54,7 @@ def app(hass): app = web.Application() app["hass"] = hass app.router.add_get("/", mock_handler) - setup_forwarded(app, []) + async_setup_forwarded(app, []) return app diff --git a/tests/components/http/test_forwarded.py b/tests/components/http/test_forwarded.py index d0d226d2b470e0..7d0d96c235291a 100644 --- a/tests/components/http/test_forwarded.py +++ b/tests/components/http/test_forwarded.py @@ -5,7 +5,7 @@ from aiohttp.hdrs import X_FORWARDED_FOR, X_FORWARDED_HOST, X_FORWARDED_PROTO import pytest -from homeassistant.components.http.forwarded import setup_forwarded +from homeassistant.components.http.forwarded import async_setup_forwarded async def mock_handler(request): @@ -28,7 +28,7 @@ async def handler(request): app = web.Application() app.router.add_get("/", handler) - setup_forwarded(app, []) + async_setup_forwarded(app, []) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get("/", headers={X_FORWARDED_FOR: "255.255.255.255"}) @@ -73,7 +73,7 @@ async def handler(request): app = web.Application() app.router.add_get("/", handler) - setup_forwarded( + async_setup_forwarded( app, [ip_network(trusted_proxy) for trusted_proxy in trusted_proxies] ) @@ -97,7 +97,7 @@ async def handler(request): app = web.Application() app.router.add_get("/", handler) - setup_forwarded(app, [ip_network("1.1.1.1")]) + async_setup_forwarded(app, [ip_network("1.1.1.1")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get("/", headers={X_FORWARDED_FOR: "255.255.255.255"}) @@ -119,7 +119,7 @@ async def handler(request): app = web.Application() app.router.add_get("/", handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + async_setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( @@ -148,7 +148,7 @@ async def test_x_forwarded_for_with_malformed_header( """Test that we get a HTTP 400 bad request with a malformed header.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + async_setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) @@ -162,7 +162,7 @@ async def test_x_forwarded_for_with_multiple_headers(aiohttp_client, caplog): """Test that we get a HTTP 400 bad request with multiple headers.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + async_setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) @@ -193,7 +193,7 @@ async def handler(request): app = web.Application() app.router.add_get("/", handler) - setup_forwarded(app, []) + async_setup_forwarded(app, []) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( @@ -245,7 +245,7 @@ async def handler(request): app = web.Application() app.router.add_get("/", handler) - setup_forwarded(app, [ip_network("127.0.0.0/24")]) + async_setup_forwarded(app, [ip_network("127.0.0.0/24")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( @@ -273,7 +273,7 @@ async def handler(request): app = web.Application() app.router.add_get("/", handler) - setup_forwarded(app, [ip_network("127.0.0.0/24")]) + async_setup_forwarded(app, [ip_network("127.0.0.0/24")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( @@ -301,7 +301,7 @@ async def handler(request): app = web.Application() app.router.add_get("/", handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + async_setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get("/", headers={X_FORWARDED_PROTO: "https"}) @@ -313,7 +313,7 @@ async def test_x_forwarded_proto_with_multiple_headers(aiohttp_client, caplog): """Test that we get a HTTP 400 bad request with multiple headers.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + async_setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( @@ -338,7 +338,7 @@ async def test_x_forwarded_proto_empty_element( """Test that we get a HTTP 400 bad request with multiple headers.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + async_setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( @@ -362,7 +362,7 @@ async def test_x_forwarded_proto_incorrect_number_of_elements( """Test that we get a HTTP 400 bad request with multiple headers.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + async_setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( @@ -395,7 +395,7 @@ async def handler(request): app = web.Application() app.router.add_get("/", handler) - setup_forwarded(app, []) + async_setup_forwarded(app, []) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( @@ -419,7 +419,7 @@ async def handler(request): app = web.Application() app.router.add_get("/", handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + async_setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( @@ -444,7 +444,7 @@ async def handler(request): app = web.Application() app.router.add_get("/", handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + async_setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get("/", headers={X_FORWARDED_HOST: "example.com"}) @@ -456,7 +456,7 @@ async def test_x_forwarded_host_with_multiple_headers(aiohttp_client, caplog): """Test that we get a HTTP 400 bad request with multiple headers.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + async_setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( @@ -476,7 +476,7 @@ async def test_x_forwarded_host_with_empty_header(aiohttp_client, caplog): """Test that we get a HTTP 400 bad request with empty host value.""" app = web.Application() app.router.add_get("/", mock_handler) - setup_forwarded(app, [ip_network("127.0.0.1")]) + async_setup_forwarded(app, [ip_network("127.0.0.1")]) mock_api_client = await aiohttp_client(app) resp = await mock_api_client.get( From 4892a999bb39dfc94cf7f4bef095c0bfc4a0791a Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Tue, 11 Aug 2020 09:21:04 +0200 Subject: [PATCH 32/35] Process review comments --- homeassistant/components/http/forwarded.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/homeassistant/components/http/forwarded.py b/homeassistant/components/http/forwarded.py index 86db130a4851bc..4d9ec69a018319 100644 --- a/homeassistant/components/http/forwarded.py +++ b/homeassistant/components/http/forwarded.py @@ -146,9 +146,8 @@ async def forwarded_middleware(request, handler): # Ideally this should take the scheme corresponding to the entry # in X-Forwarded-For that was chosen, but some proxies only retain # one element. In that case, use what we have. - if forwarded_for_index > 1 or len(forwarded_proto) == 1: - overrides["scheme"] = forwarded_proto[-1] - else: + overrides["scheme"] = forwarded_proto[-1] + if len(forwarded_proto) != 1: overrides["scheme"] = forwarded_proto[forwarded_for_index] # Handle X-Forwarded-Host From d8f953a0865522ecfe2e4078c2b19ac7eda31d96 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Tue, 11 Aug 2020 10:19:06 +0200 Subject: [PATCH 33/35] Apply suggestions from code review Co-authored-by: Martin Hjelmare --- tests/components/http/test_forwarded.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/components/http/test_forwarded.py b/tests/components/http/test_forwarded.py index 7d0d96c235291a..45853687f1355b 100644 --- a/tests/components/http/test_forwarded.py +++ b/tests/components/http/test_forwarded.py @@ -335,7 +335,7 @@ async def test_x_forwarded_proto_with_multiple_headers(aiohttp_client, caplog): async def test_x_forwarded_proto_empty_element( x_forwarded_proto, aiohttp_client, caplog ): - """Test that we get a HTTP 400 bad request with multiple headers.""" + """Test that we get a HTTP 400 bad request with empty proto.""" app = web.Application() app.router.add_get("/", mock_handler) async_setup_forwarded(app, [ip_network("127.0.0.1")]) @@ -359,7 +359,7 @@ async def test_x_forwarded_proto_empty_element( async def test_x_forwarded_proto_incorrect_number_of_elements( x_forwarded_for, x_forwarded_proto, expected, got, aiohttp_client, caplog ): - """Test that we get a HTTP 400 bad request with multiple headers.""" + """Test that we get a HTTP 400 bad request with incorrect number of elements.""" app = web.Application() app.router.add_get("/", mock_handler) async_setup_forwarded(app, [ip_network("127.0.0.1")]) From 226f3e1deb73fe975e71ec8952aa36366e67e976 Mon Sep 17 00:00:00 2001 From: Pascal Vizeli Date: Tue, 11 Aug 2020 15:10:16 +0200 Subject: [PATCH 34/35] Update homeassistant/components/hassio/auth.py Co-authored-by: Franck Nijhof --- homeassistant/components/hassio/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/homeassistant/components/hassio/auth.py b/homeassistant/components/hassio/auth.py index 9b45a135712ee5..b92229de599122 100644 --- a/homeassistant/components/hassio/auth.py +++ b/homeassistant/components/hassio/auth.py @@ -63,7 +63,7 @@ def _check_access(self, request: web.Request): """Check if this call is from Supervisor.""" # Check caller IP hassio_ip = os.environ["HASSIO"].split(":")[0] - if ip_address(request.remote) != ip_address(hassio_ip): + if ip_address(request.transport.get_extra_info("peername")[0]) != ip_address(hassio_ip): _LOGGER.error("Invalid auth request from %s", request.remote) raise HTTPUnauthorized() From 59de0c35b8216d48f9bb993fdb37a9fba240f514 Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Tue, 11 Aug 2020 15:18:44 +0200 Subject: [PATCH 35/35] Black --- homeassistant/components/hassio/auth.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/homeassistant/components/hassio/auth.py b/homeassistant/components/hassio/auth.py index b92229de599122..48f0abd6617e0b 100644 --- a/homeassistant/components/hassio/auth.py +++ b/homeassistant/components/hassio/auth.py @@ -63,7 +63,9 @@ def _check_access(self, request: web.Request): """Check if this call is from Supervisor.""" # Check caller IP hassio_ip = os.environ["HASSIO"].split(":")[0] - if ip_address(request.transport.get_extra_info("peername")[0]) != ip_address(hassio_ip): + if ip_address(request.transport.get_extra_info("peername")[0]) != ip_address( + hassio_ip + ): _LOGGER.error("Invalid auth request from %s", request.remote) raise HTTPUnauthorized()