Skip to content

Clean util/network to further align with stdlib #114941

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
paravoid wants to merge 1 commit into from
Closed

Clean util/network to further align with stdlib #114941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 3 additions & 29 deletions homeassistant/util/network.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,42 +7,16 @@

import yarl

# RFC6890 - IP addresses of loopback interfaces
IPV6_IPV4_LOOPBACK = ip_network("::ffff:127.0.0.0/104")

LOOPBACK_NETWORKS = (
ip_network("127.0.0.0/8"),
ip_network("::1/128"),
IPV6_IPV4_LOOPBACK,
)

# RFC6890 - Address allocation for Private Internets
PRIVATE_NETWORKS = (
ip_network("10.0.0.0/8"),
ip_network("172.16.0.0/12"),
ip_network("192.168.0.0/16"),
ip_network("fd00::/8"),
ip_network("::ffff:10.0.0.0/104"),
ip_network("::ffff:172.16.0.0/108"),
ip_network("::ffff:192.168.0.0/112"),
)

# RFC6890 - Link local ranges
LINK_LOCAL_NETWORKS = (
ip_network("169.254.0.0/16"),
ip_network("fe80::/10"),
ip_network("::ffff:169.254.0.0/112"),
)


def is_loopback(address: IPv4Address | IPv6Address) -> bool:
"""Check if an address is a loopback address."""
return address.is_loopback or address in IPV6_IPV4_LOOPBACK
# the ::ffff: check is a workaround for python/cpython#117566
return address.is_loopback or address in ip_network("::ffff:127.0.0.0/104")


def is_private(address: IPv4Address | IPv6Address) -> bool:
"""Check if an address is a unique local non-loopback address."""
return any(address in network for network in PRIVATE_NETWORKS)
return address.is_private and not is_loopback(address) and not address.is_link_local


def is_link_local(address: IPv4Address | IPv6Address) -> bool:
Expand Down
2 changes: 0 additions & 2 deletions tests/components/auth/test_indieauth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,6 @@ def test_client_id_hostname() -> None:
assert indieauth._parse_client_id("http://192.168.0.0")
assert indieauth._parse_client_id("http://192.168.255.255")

with pytest.raises(ValueError):
assert indieauth._parse_client_id("http://255.255.255.255/")
Comment on lines -86 to -87
Copy link
Member

@bdraco bdraco Apr 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a semantics change. Maybe we should keep a check for this

Copy link
Contributor Author

@paravoid paravoid Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really? 255.255.255.255 is "limited broadcast", and has been reserved from being used for unicast traffic for the past... ~40 years (RFC 919 section 7 dated October 1984). I'm not even sure how usable it'll actually be even if you force your OS to use it, but if someone really managed to, I guess they get to keep the pieces when things break? (As a sidenote, we are already (purposefully) diverging from the indieauth spec, which prohibits bare IP addresses entirely, if this wasn't an edge case enough.)

Copy link
Member

@bdraco bdraco Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more concerned about the change to the semantics in is_private, the test here is only highlighting that change.

This PR

>>> from homeassistant.util import network
>>> from ipaddress import ip_address
>>> network.is_private(ip_address("255.255.255.255"))
True

dev

>>> from homeassistant.util import network
>>> from ipaddress import ip_address
>>> network.is_private(ip_address("255.255.255.255"))
False

I'm assuming 255.255.255.255 was chosen for a reason in that test so it seems wrong to remove it without letting the original author of the PR that it was added in have chance to review (#15369). Alternatively, we can keep compatibility with something like:

diff --git a/homeassistant/util/network.py b/homeassistant/util/network.py
index d5830d25b69..3360347b1b4 100644
--- a/homeassistant/util/network.py
+++ b/homeassistant/util/network.py
@@ -7,6 +7,8 @@ import re
 
 import yarl
 
+BROADCAST = ip_address("255.255.255.255")
+
 
 def is_loopback(address: IPv4Address | IPv6Address) -> bool:
     """Check if an address is a loopback address."""
@@ -16,7 +18,12 @@ def is_loopback(address: IPv4Address | IPv6Address) -> bool:
 
 def is_private(address: IPv4Address | IPv6Address) -> bool:
     """Check if an address is a unique local non-loopback address."""
-    return address.is_private and not is_loopback(address) and not address.is_link_local
+    return (
+        address.is_private
+        and not is_loopback(address)
+        and not address.is_link_local
+        and address != BROADCAST
+    )
 
 
 def is_link_local(address: IPv4Address | IPv6Address) -> bool:

Copy link
Contributor Author

@paravoid paravoid Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly believe we should not be adding those kind of "special" semantics, especially for reasons that are not clear. 255.255.255.255 is not routable, per RFC 919, section 7, and RFC8190. You literally cannot use it, e.g. try a traceroute from your computer, it will refuse to even route the traffic to your default gateway.

The fact that HA's is_private returned False before, was a bug. Python's is_private is following those RFCs (and IANA's special-purpose registry assignments) and has the correct semantic here.

I can't imagine a scenario where bug-for-bug compatibity is desired, but I acknowledge I may be missing something! @Baloob I've noticed you're the #15369/indieauth author, perhaps you could shed some light on why http://255.255.255.255/ was chosen as a test case?

BTW, the only case in this entire file where I see the need to override stdlib, is with is_loopback, and that is due to a Python stdlib bug. But in this case, I've sent a PR to Python to fix that in stdlib itself and over time remove our custom is_loopback override. I intend to entirely deprecate the rest of these utility functions in a subsequent PR.

Copy link
Member

@bdraco bdraco Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest keeping bug-for-bug compatibility for now as it makes this PR mergable now. It can be removed in a followup when others are available to review.

Copy link
Contributor Author

@paravoid paravoid Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick response & review! Honestly I don't think that makes sense. This whole PR is about resolving long-standing bugs resulting from HA hardcoding IANA-reserved ranges, and not doing it right (either in the first place, or due to drift). Reintroducing some of these bugs without a clear rationale is a slippery slope and kind of defeats the whole point. I have tried to keep semantics in a couple of places where it made sense (see the commit message), but in the 255.255.255.255 example in particular, I don't think it does.

As demonstrated above, 255.2555.255.255 is unroutable in Linux, including in HA's Docker/HassOS, so there is no realistic way one can use this IP in a network, and still exchange traffic with HA. IOW, the test suite tests for something that is just not possible. I hope this demonstration makes the change easier to review, but if you'd like to wait for additional reviewers, I understand :)

Copy link
Member

@bdraco bdraco Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best to wait for another reviewer as I don't feel comfortable making that decision in isolation.

Copy link
Member

@frenck frenck May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda agree with bdraco, and judging by your comments, you do too:

Is it really? 255.255.255.255 is "limited broadcast", and has been reserved from being used for unicast traffic for the past... ~40 years (RFC 919 section 7 dated October 1984).

Meaning this should have raised on this given test, as this is not a valid address for authing. This test should be re-instated, and the code should be fixed to raise for this case.

Copy link
Contributor Author

@paravoid paravoid May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! Ι'll admit that I'm not super familiar with indieauth, and I may be misunderstanding something. And I'd like to add code that I understand :)

First of all: the indieauth code accepts only "local" addresses for auth. The networking code previously (erroneously) considered this address "globally routable" and thus indieauth did not accept it. The address is definitely not globally routable. Is this all correct?

Second: should 255.255.255.255 be singled out in the indieauth code? Why? What's the real-life
condition under which this address would be used? Basically, under which scenario would this code path be reached (outside of tests)?

with pytest.raises(ValueError):
assert indieauth._parse_client_id("http://11.0.0.0/")
with pytest.raises(ValueError):
Expand Down
8 changes: 4 additions & 4 deletions tests/components/auth/test_login_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
[
("192.168.1.10", True),
("::ffff:192.168.0.10", True),
("1.2.3.4", False),
("2001:db8::1", False),
("192.175.48.1", False),
("2620:4f:8000::1", False),
],
)
@pytest.mark.parametrize(
Expand Down Expand Up @@ -81,8 +81,8 @@ async def test_fetch_auth_providers(
[{"name": "Trusted Networks", "type": "trusted_networks", "id": None}],
),
("::ffff:192.168.0.10", []),
("1.2.3.4", []),
("2001:db8::1", []),
("192.175.48.1", []),
("2620:4f:8000::1", []),
],
)
async def test_fetch_auth_providers_trusted_network(
Expand Down
2 changes: 1 addition & 1 deletion tests/components/http/test_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
ip_network("FD01:DB8::1"),
]
TRUSTED_ADDRESSES = ["100.64.0.1", "192.0.2.100", "FD01:DB8::1", "2001:DB8:ABCD::1"]
EXTERNAL_ADDRESSES = ["198.51.100.1", "2001:DB8:FA1::1"]
EXTERNAL_ADDRESSES = ["192.175.48.1", "2620:4f:8000::1"]
LOCALHOST_ADDRESSES = ["127.0.0.1", "::1"]
UNTRUSTED_ADDRESSES = [*EXTERNAL_ADDRESSES, *LOCALHOST_ADDRESSES]
PRIVATE_ADDRESSES = [
Expand Down
16 changes: 14 additions & 2 deletions tests/util/test_network.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,26 @@ def test_is_invalid() -> None:

def test_is_local() -> None:
"""Test local addresses."""
# RFC 1918 space
assert network_util.is_local(ip_address("192.168.0.1"))
# loopback
assert network_util.is_local(ip_address("127.0.0.1"))
assert network_util.is_local(ip_address("::ffff:127.0.0.1"))
assert network_util.is_local(ip_address("::1"))
# IPv6 ULA
assert network_util.is_local(ip_address("fd12:3456:789a:1::1"))
# IPv6 link-local
assert network_util.is_local(ip_address("fe80::1234:5678:abcd"))
# mapped ipv4-to-ipv6
assert network_util.is_local(ip_address("::ffff:192.168.0.1"))
# Documentation/TEST-NET2 IP space, marked as Globally Reachable: False by IANA
assert network_util.is_local(ip_address("198.51.100.1"))
assert network_util.is_local(ip_address("2001:DB8:FA1::1"))
# AS112 space, marked as Globally Reachable: True by IANA
assert not network_util.is_local(ip_address("192.175.48.1"))
assert not network_util.is_local(ip_address("2620:4f:8000::1"))
# random globally routable IP space
assert not network_util.is_local(ip_address("208.5.4.2"))
assert not network_util.is_local(ip_address("198.51.100.1"))
assert not network_util.is_local(ip_address("2001:DB8:FA1::1"))
assert not network_util.is_local(ip_address("::ffff:208.5.4.2"))


Expand Down