Trusted networks auth provider warns if detects a requests with x-forwarded-for header while the http integration is not configured for reverse proxies #51319

balloob · 2021-06-01T04:50:43Z

Breaking change

Trusted networks need explicit configuration of the HTTP integration to work with x-forwarded-for headers to authenticate IPs from requests that contain the x-forwarded-for header.

This is a warning and will become an error in Home Assistant 2021.7.

Proposed change

This PR will make the trusted network auth provider warn on all requests that contain an x-forwarded-for header unless the http integration is configured to parse these headers.

This means that if a request comes from a reverse-proxy but Home Assistant is not configured to work with reverse proxies, it will warn while authenticating with that request.

I will issue a follow-up PR to make this an error for Home Assistant 2021.7.

If a user wants to allow such requests, the bare minimum configuration is:

http:
  use_x_forwarded_for: true
  trusted_proxies: []

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

… for proxies to allow logging in with requests with x-forwarded-for header

balloob · 2021-06-01T05:36:03Z

tests/auth/providers/test_trusted_networks.py

 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)


This is a temporary constant to automatically fail the tests when we bump the version. It's my intention to remove it directly in a follow-up PR such that this PR can be cherry-picked into the beta.

balloob · 2021-06-01T05:36:31Z

homeassistant/auth/providers/trusted_networks.py

-                cred = self.async_create_credentials({"user_id": user_id})
-                await self.store.async_link_user(user, cred)
-                return cred
+            if user.id != user_id:


I rewrote the if-for-if spaghetti to individual statements to make it easier to read.

balloob · 2021-06-01T05:36:58Z

homeassistant/auth/providers/trusted_networks.py

-                    )
-                ]
-                break
+            if ip_addr not in ip_net:


Changed if-statement to a guard clause to improve readability.

elupus · 2021-06-01T10:06:43Z

homeassistant/auth/providers/trusted_networks.py

+        if request is not None and (
+            self.hass.http.use_x_forwarded_for
+            or hdrs.X_FORWARDED_FOR not in request.headers
+        ):


Can this be reformulated/split a bit. My head has a heard time with all the nots,ands and ors.

Perhaps even Invert the function to is_blocked_request since that is how it's used.

I think in the end, the question is; should we even ever process forwarded-for requests, without a reverse proxy being configured at all...

If so, we can move this hole logic into the middleware of the HTTP server, instead of having it in the authentication.

I think a warning and getting this into the beta is fine for now, lets re-evaluate the handling for the next release.

I wasn't against the warning. I was against the hard to read logic with many negations.

Yes, I agree, but I think it is good to have in the beta. Given the time is close, we should improve later.

I'm opening a PR as a follow-up, that moves it out of here, into the HTTP server directly.
If that is an acceptable solution, this can be reverted.

Alternative approach in #51332

…warded-for header while the http integration is not configured for reverse proxies (#51319) * Trusted networks auth provider to require http integration configured for proxies to allow logging in with requests with x-forwarded-for header * Make it a warning

…th x-forwarded-for header while the http integration is not configured for reverse proxies (#51319)" This reverts commit 413fd1b.

Trusted networks auth provider to require http integration configured…

d73e09f

… for proxies to allow logging in with requests with x-forwarded-for header

balloob added the breaking-change label Jun 1, 2021

homeassistant added the cla-signed label Jun 1, 2021

probot-home-assistant bot added core integration: http new-feature labels Jun 1, 2021

Make it a warning

0917816

balloob commented Jun 1, 2021

View reviewed changes

elupus reviewed Jun 1, 2021

View reviewed changes

frenck approved these changes Jun 1, 2021

View reviewed changes

frenck merged commit bd03733 into dev Jun 1, 2021

frenck deleted the trusted-networks-opt-in-allow-x-forward-header branch June 1, 2021 10:51

bramkragten added this to the 2021.6.0 milestone Jun 1, 2021

bramkragten added the cherry-picked label Jun 1, 2021

frenck mentioned this pull request Jun 1, 2021

Always load middle to handle forwarded proxy data #51332

Merged

28 tasks

frenck added a commit that referenced this pull request Jun 1, 2021

Revert "Trusted networks auth provider warns if detects a requests wi…

c9450a7

…th x-forwarded-for header while the http integration is not configured for reverse proxies (#51319)" This reverts commit 413fd1b.

github-actions bot locked and limited conversation to collaborators Jun 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trusted networks auth provider warns if detects a requests with x-forwarded-for header while the http integration is not configured for reverse proxies #51319

Trusted networks auth provider warns if detects a requests with x-forwarded-for header while the http integration is not configured for reverse proxies #51319

balloob commented Jun 1, 2021 •

edited

Loading

balloob Jun 1, 2021

balloob Jun 1, 2021

balloob Jun 1, 2021

elupus Jun 1, 2021

elupus Jun 1, 2021

frenck Jun 1, 2021 •

edited

Loading

elupus Jun 1, 2021

frenck Jun 1, 2021 •

edited

Loading

frenck Jun 1, 2021

Trusted networks auth provider warns if detects a requests with x-forwarded-for header while the http integration is not configured for reverse proxies #51319

Trusted networks auth provider warns if detects a requests with x-forwarded-for header while the http integration is not configured for reverse proxies #51319

Conversation

balloob commented Jun 1, 2021 • edited Loading

Breaking change

Proposed change

Type of change

Additional information

Checklist

balloob Jun 1, 2021

Choose a reason for hiding this comment

balloob Jun 1, 2021

Choose a reason for hiding this comment

balloob Jun 1, 2021

Choose a reason for hiding this comment

elupus Jun 1, 2021

Choose a reason for hiding this comment

elupus Jun 1, 2021

Choose a reason for hiding this comment

frenck Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

elupus Jun 1, 2021

Choose a reason for hiding this comment

frenck Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

frenck Jun 1, 2021

Choose a reason for hiding this comment

balloob commented Jun 1, 2021 •

edited

Loading

frenck Jun 1, 2021 •

edited

Loading

frenck Jun 1, 2021 •

edited

Loading