search

python / cpython

The Python programming language
https://www.python.org
Other
63.62k stars 30.47k forks source link

Some IPv4 and IPv4-mapped IPv6 properties don't match #122792

Open sethmlarson opened 3 months ago

sethmlarson commented 3 months ago

Bug report

Bug description:

The following properties on an IPv6Address don't match their IPv4Address counterparts when using an IPv6-mapped IPv4 address (ie ::ffff:<ipv4>):

Proposed fix is to make all properties use their IPv4Address values for IPv4-mapped addresses.

CPython versions tested on:

CPython main branch

Operating systems tested on:

No response

Linked PRs

jstasiak commented 2 months ago

Hey, I learned about this issue just today and, as a user of this API, I'd like to express some opposition to it.

My reasons, in no particular order:

  1. The proposed change makes the API more unpredictable and confusing (somewhat subjective) by "unwrapping" the IPv4-mapped addresses.
  2. This change makes the properties inconsistent with the already documented behavior of the properties (and the PR, https://github.com/python/cpython/pull/122793, doesn't attempt to change the documentation).
  3. The new behavior of the properties breaks backwards compatibility as code that already depends on the existing behavior (which I'll claim there are good reasons for) will now produce incorrect results. Breaking compatibility is fine if the old behavior is a legitimate mistake and didn't make much sense etc. but I don't see this here.
  4. The proposed change makes the API more limiting. Previously, as the user of this API, I was free to decide what to do in case of IPv4-mapped IPv6 addresses – I could unwrap them or not before checking is_link_local, for example. With this change I'm left with no API at all to do the latter.

cc @gpshead

gpshead commented 2 months ago

I think we're lacking the motivating reason behind the change, @sethmlarson can explain more.

Agreed with (2), we need a docs update. But the merged change does not make it inconsistent, the ipaddress docs today are sparse and don't specify the exact behavior when these are used on IPv6Address. To me this change matches what I would assume they would do based on the existing terse docs. https://docs.python.org/3/library/ipaddress.html#ipaddress.IPv6Address.is_multicast

Regarding (3) / (4) do you have practical examples of actual code in use that does not want the new behavior? (got links?) That'd help reason about where this falls into breaking change vs bug fix categories.

Some general background: We've gotten security reports over the years about the ipaddress module in various places because people write code using it for security purposes and wind up surprised when it doesn't behave in the manner their specific application needed and thus maybe they did something undesirable with network traffic as a result.

sethmlarson commented 2 months ago

@jstasiak Thanks for the question, indeed the primary motivation for this change is security, specifically folks using IP address filtering before establishing a connection. Since an IPv4-mapped IPv6 address gets established as an IPv4 address I figured these properties should match the mapped IPv4 address and not the IPv6 address that's mostly being used for framing. If you have better guidance about this case, please share!

sethmlarson commented 2 months ago

@jstasiak Do you have an opinion on the above comment? Otherwise we will continue back-porting this change, we want to make sure we're not breaking any legitimate use-cases unintentionally.