Core: X-Forwarded-For support broken

[mweinelt]

This commit (https://github.com/home-assistant/home-assistant/commit/519d9f2fd01751492909e574b8ba3812487d7d6b#diff-eb76e695595582bfd0beb58c113181b6R373) broke support for the X-Forwarded-For header that is necessary to pass the real remote_addr in proxied setups.

This happened because of the change to aiohttp and I assume you, @balloob, read https://github.com/KeepSafe/aiohttp/issues/642#issuecomment-158888961, but not https://github.com/KeepSafe/aiohttp/issues/642#issuecomment-158893196, which changes the behaviour of get_real_ip. As we are obviously missing a testcase that injects a proper X-Forwarded-For header and then validates it, this has gone unnoticed so far.

Unfortunately there is no direct support for the X-Forwarded-* header family in aiohttp, see https://github.com/KeepSafe/aiohttp/issues/1134.

Too tired right now, to research this further, but a new solution has to be found. Currently peername will always be ('127.0.0.1', 43270) (with rotating ports of course), not the real remote_addr.

[balloob]

Looks like the lack of a proper test indeed caused us to ship broken functionality.

After some extra thought, I actually think that doing auth based on a header that anyone can set (including a malicious user who can pretend to be a proxy) is a bad idea. So instead of fixing it I suggest we kill the feature.

[robbiet480]

It's an opt in feature. Users should be entirely aware of the consequences of the opt in, but I don't think that we should remove this feature because of those consequences.

[mweinelt]

Indeed exposing HASS directly to untrusted networks would allow spoofing the header and therefore the originating IP address, which is undesirable. In a reverse proxied setup however this feature is very valuable, which why I'd kindly ask not to throw the idea of it out.

So a pull request fixing this should address:

• Configuration option to enable/disable X-Forwarded-For parsing
• Update documentation to address security concern of enabling it in untrusted networks
• Test case that injects X-Forwarded-For header and verifies it
• Reimplement get_real_ip to address the X-Forwarded-For header

[balloob]

OK that sounds good

On Mon, Nov 7, 2016, 04:32 hexa- [email protected] wrote:

Indeed exposing HASS directly to untrusted networks would allow spoofing
the header and therefore the originating IP address, which is undesirable.
In a reverse proxied setup however this feature is very valuable, which why
I'd kindly ask not to throw the idea of it out.

So a pull request fixing this should address:

• Configuration option to enable/disable X-Forwarded-For parsing
• Update documentation to address security concern of enabling it in
  untrusted networks
• Test case that injects X-Forwarded-For header and verifies it
• Reimplement get_real_ip to address the X-Forwarded-For header

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/home-assistant/home-assistant/issues/4265#issuecomment-258824094,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABYJ2mRvb7SEbyuxGK5u4W18-QAyLrUqks5q7xpWgaJpZM4Kqx4Q
.

[robbiet480]

:+1: