Always load middle to handle forwarded proxy data (home-assistant#51332)

homeassistant/auth/providers/trusted_networks.py

@@ -14,13 +14,10 @@
     ip_address,
     ip_network,
 )
-import logging
 from typing import Any, Dict, List, Union, cast
 
-from aiohttp import hdrs
 import voluptuous as vol
 
-from homeassistant.components import http
 from homeassistant.core import callback
 from homeassistant.data_entry_flow import FlowResult
 from homeassistant.exceptions import HomeAssistantError
@@ -89,31 +86,9 @@ def support_mfa(self) -> bool:
         """Trusted Networks auth provider does not support MFA."""
         return False
 
-    @callback
-    def is_allowed_request(self) -> bool:
-        """Return if it is an allowed request."""
-        request = http.current_request.get()
-        if request is not None and (
-            self.hass.http.use_x_forwarded_for
-            or hdrs.X_FORWARDED_FOR not in request.headers
-        ):
-            return True
-
-        logging.getLogger(__name__).warning(
-            "A request contained an x-forwarded-for header but your HTTP integration is not set-up "
-            "for reverse proxies. This usually means that you have not configured your reverse proxy "
-            "correctly. This request will be blocked in Home Assistant 2021.7 unless you configure "
-            "your HTTP integration to allow this header."
-        )
-        return True
-
     async def async_login_flow(self, context: dict | None) -> LoginFlow:
         """Return a flow to login."""
         assert context is not None
-
-        if not self.is_allowed_request():
-            return MisconfiguredTrustedNetworksLoginFlow(self)
-
         ip_addr = cast(IPAddress, context.get("ip_address"))
         users = await self.store.async_get_users()
         available_users = [
@@ -195,11 +170,6 @@ def async_validate_access(self, ip_addr: IPAddress) -> None:
         Raise InvalidAuthError if not.
         Raise InvalidAuthError if trusted_networks is not configured.
         """
-        if not self.is_allowed_request():
-            raise InvalidAuthError(
-                "No request or it contains x-forwarded-for header and that's not allowed by configuration"
-            )
-
         if not self.trusted_networks:
             raise InvalidAuthError("trusted_networks is not configured")
 
@@ -220,16 +190,6 @@ def async_validate_refresh_token(
         self.async_validate_access(ip_address(remote_ip))
 
 
-class MisconfiguredTrustedNetworksLoginFlow(LoginFlow):
-    """Login handler for misconfigured trusted networks."""
-
-    async def async_step_init(
-        self, user_input: dict[str, str] | None = None
-    ) -> FlowResult:
-        """Handle the step of the form."""
-        return self.async_abort(reason="forwared_for_header_not_allowed")
-
-
 class TrustedNetworksLoginFlow(LoginFlow):
     """Handler for the login flow."""
 

homeassistant/components/http/__init__.py

@@ -232,10 +232,7 @@ def __init__(
         # forwarded middleware needs to go second.
         setup_security_filter(app)
 
-        # Only register middleware if `use_x_forwarded_for` is enabled
-        # and trusted proxies are provided
-        if use_x_forwarded_for and trusted_proxies:
-            async_setup_forwarded(app, trusted_proxies)
+        async_setup_forwarded(app, use_x_forwarded_for, trusted_proxies)
 
         setup_request_context(app, current_request)
 
@@ -252,7 +249,6 @@ def __init__(
         self.ssl_key = ssl_key
         self.server_host = server_host
         self.server_port = server_port
-        self.use_x_forwarded_for = use_x_forwarded_for
         self.trusted_proxies = trusted_proxies
         self.is_ban_enabled = is_ban_enabled
         self.ssl_profile = ssl_profile

homeassistant/components/http/forwarded.py

@@ -14,7 +14,9 @@
 
 
 @callback
-def async_setup_forwarded(app: Application, trusted_proxies: list[str]) -> None:
+def async_setup_forwarded(
+    app: Application, use_x_forwarded_for: bool | None, trusted_proxies: list[str]
+) -> None:
     """Create forwarded middleware for the app.
 
     Process IP addresses, proto and host information in the forwarded for headers.
@@ -73,15 +75,37 @@ async def forwarded_middleware(
             # No forwarding headers, continue as normal
             return await handler(request)
 
-        # Ensure the IP of the connected peer is trusted
-        assert request.transport is not None
+        # Get connected IP
+        if (
+            request.transport is None
+            or request.transport.get_extra_info("peername") is None
+        ):
+            # Connected IP isn't retrieveable from the request transport, continue
+            return await handler(request)
+
         connected_ip = ip_address(request.transport.get_extra_info("peername")[0])
+
+        # We have X-Forwarded-For, but config does not agree
+        if not use_x_forwarded_for:
+            _LOGGER.warning(
+                "A request from a reverse proxy was received from %s, but your "
+                "HTTP integration is not set-up for reverse proxies; "
+                "This request will be blocked in Home Assistant 2021.7 unless "
+                "you configure your HTTP integration to allow this header",
+                connected_ip,
+            )
+            # Block this request in the future, for now we pass.
+            return await handler(request)
+
+        # Ensure the IP of the connected peer is trusted
         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",
+                "Received X-Forwarded-For header from untrusted proxy %s, headers not processed; "
+                "This request will be blocked in Home Assistant 2021.7 unless you configure "
+                "your HTTP integration to allow this proxy to reverse your Home Assistant instance",
                 connected_ip,
             )
-            # Not trusted, continue as normal
+            # Not trusted, Block this request in the future, continue as normal
             return await handler(request)
 
         # Multiple X-Forwarded-For headers

tests/auth/providers/test_trusted_networks.py

@@ -5,13 +5,10 @@
 import pytest
 import voluptuous as vol
 
-from homeassistant import auth, const
+from homeassistant import auth
 from homeassistant.auth import auth_store
 from homeassistant.auth.providers import trusted_networks as tn_auth
 from homeassistant.data_entry_flow import RESULT_TYPE_ABORT, RESULT_TYPE_CREATE_ENTRY
-from homeassistant.setup import async_setup_component
-
-FORWARD_FOR_IS_WARNING = (const.MAJOR_VERSION, const.MINOR_VERSION) < (2021, 8)
 
 
 @pytest.fixture
@@ -115,17 +112,7 @@ def manager_bypass_login(hass, store, provider_bypass_login):
     )
 
 
-@pytest.fixture
-def mock_allowed_request():
-    """Mock that the request is allowed."""
-    with patch(
-        "homeassistant.auth.providers.trusted_networks.TrustedNetworksAuthProvider.is_allowed_request",
-        return_value=True,
-    ):
-        yield
-
-
-async def test_trusted_networks_credentials(manager, provider, mock_allowed_request):
+async def test_trusted_networks_credentials(manager, provider):
     """Test trusted_networks credentials related functions."""
     owner = await manager.async_create_user("test-owner")
     tn_owner_cred = await provider.async_get_or_create_credentials({"user": owner.id})
@@ -142,7 +129,7 @@ async def test_trusted_networks_credentials(manager, provider, mock_allowed_requ
         await provider.async_get_or_create_credentials({"user": "invalid-user"})
 
 
-async def test_validate_access(provider, mock_allowed_request):
+async def test_validate_access(provider):
     """Test validate access from trusted networks."""
     provider.async_validate_access(ip_address("192.168.0.1"))
     provider.async_validate_access(ip_address("192.168.128.10"))
@@ -157,7 +144,7 @@ async def test_validate_access(provider, mock_allowed_request):
         provider.async_validate_access(ip_address("2001:db8::ff00:42:8329"))
 
 
-async def test_validate_refresh_token(provider, mock_allowed_request):
+async def test_validate_refresh_token(provider):
     """Verify re-validation of refresh token."""
     with patch.object(provider, "async_validate_access") as mock:
         with pytest.raises(tn_auth.InvalidAuthError):
@@ -167,7 +154,7 @@ async def test_validate_refresh_token(provider, mock_allowed_request):
         mock.assert_called_once_with(ip_address("127.0.0.1"))
 
 
-async def test_login_flow(manager, provider, mock_allowed_request):
+async def test_login_flow(manager, provider):
     """Test login flow."""
     owner = await manager.async_create_user("test-owner")
     user = await manager.async_create_user("test-user")
@@ -194,9 +181,7 @@ async def test_login_flow(manager, provider, mock_allowed_request):
     assert step["data"]["user"] == user.id
 
 
-async def test_trusted_users_login(
-    manager_with_user, provider_with_user, mock_allowed_request
-):
+async def test_trusted_users_login(manager_with_user, provider_with_user):
     """Test available user list changed per different IP."""
     owner = await manager_with_user.async_create_user("test-owner")
     sys_user = await manager_with_user.async_create_system_user(
@@ -276,9 +261,7 @@ async def test_trusted_users_login(
         assert schema({"user": sys_user.id})
 
 
-async def test_trusted_group_login(
-    manager_with_user, provider_with_user, mock_allowed_request
-):
+async def test_trusted_group_login(manager_with_user, provider_with_user):
     """Test config trusted_user with group_id."""
     owner = await manager_with_user.async_create_user("test-owner")
     # create a user in user group
@@ -331,9 +314,7 @@ async def test_trusted_group_login(
     assert schema({"user": user.id})
 
 
-async def test_bypass_login_flow(
-    manager_bypass_login, provider_bypass_login, mock_allowed_request
-):
+async def test_bypass_login_flow(manager_bypass_login, provider_bypass_login):
     """Test login flow can be bypass if only one user available."""
     owner = await manager_bypass_login.async_create_user("test-owner")
 
@@ -364,42 +345,3 @@ async def test_bypass_login_flow(
     # both owner and user listed
     assert schema({"user": owner.id})
     assert schema({"user": user.id})
-
-
-async def test_allowed_request(hass, provider, current_request, caplog):
-    """Test allowing requests."""
-    assert await async_setup_component(hass, "http", {})
-
-    provider.async_validate_access(ip_address("192.168.0.1"))
-
-    current_request.get.return_value = current_request.get.return_value.clone(
-        headers={
-            **current_request.get.return_value.headers,
-            "x-forwarded-for": "1.2.3.4",
-        }
-    )
-
-    if FORWARD_FOR_IS_WARNING:
-        caplog.clear()
-        provider.async_validate_access(ip_address("192.168.0.1"))
-        assert "This request will be blocked" in caplog.text
-    else:
-        with pytest.raises(tn_auth.InvalidAuthError):
-            provider.async_validate_access(ip_address("192.168.0.1"))
-
-    hass.http.use_x_forwarded_for = True
-
-    provider.async_validate_access(ip_address("192.168.0.1"))
-
-
-@pytest.mark.skipif(FORWARD_FOR_IS_WARNING, reason="Currently a warning")
-async def test_login_flow_no_request(provider):
-    """Test getting a login flow."""
-    login_flow = await provider.async_login_flow({"ip_address": ip_address("1.1.1.1")})
-    assert await login_flow.async_step_init() == {
-        "description_placeholders": None,
-        "flow_id": None,
-        "handler": None,
-        "reason": "forwared_for_header_not_allowed",
-        "type": "abort",
-    }

tests/components/http/test_auth.py

@@ -53,7 +53,7 @@ def app(hass):
     app = web.Application()
     app["hass"] = hass
     app.router.add_get("/", mock_handler)
-    async_setup_forwarded(app, [])
+    async_setup_forwarded(app, True, [])
     return app