IPNetwork.Contains for IPv4 networks should match IPv4-Mapped IPv6 Addresses

[hacst]

Background and motivation

Dual stack applications in .NET see connecting IPv4 addresses as IPv4-Mapped IPv6 Addresses. This is problematic when combined with the IPNetworks.Contains function specifying IPv4 networks. E.g. if I want to let the user provide a CIDR string for disallowing access I might write something like:

var disallowed = IPNetwork.Parse(disallowedCidrString);
if (disallowed.Contains(remoteIp)) {
  // Deny
} else {
  // Allow
}

As the user wants to deny an IPv4 range they write something like "10.0.0.8/8". However this creates problems because it will produce an IPv4 Network that will not match the dual stack Ipv4 mapped ipv6 address.

My interpretation is that the IPv4-Mapped IPv6 Address is actually an IPv4 address and hence should be matchable using a IPv4 IPNetwork. Under this assumption a change to the IPNetwork.Contains implementation as proposed here could prevent such confusions.

ps: I was unsure whether reporting this as an API Suggestion is correct as this does not change the visible API. However as it would be an API Behavior change it seemed to be the most fitting category. Let me know if this was incorrect.

API Proposal

No visible changes except maybe to documentation. My suggestion would be to make the Contains function use IPAddress.IsIPv4MappedToIPv6 to identify that it is actually passed an IPv4 address to make contains behave as expected.

API Usage

var address = IPAddress.Parse("::ffff:10.0.0.12");
var network = IPNetwork.Parse("10.0.0.0/8");

Debug.Assert(network.Contains(address));

Alternative Designs

Handling this as a developer without the proposed change certainly is possible by inspecting the address and network at the point of comparison to work around the existing behavior. However it has to be done carefully to get it right. E.g. simply trying to always match using a IPv6 network would fail if the server configuration was changed to bind only to IPv4.

Risks

Existing usage might in some way expect IPv6 addresses to never get be reported as being contained in IPv4 networks. I cannot think of actual use-case for this though.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

Dual stack applications in .NET see connecting IPv4 addresses as IPv4-Mapped IPv6 Addresses. This is problematic when combined with the IPNetworks.Contains function specifying IPv4 networks. E.g. if I want to let the user provide a CIDR string for disallowing access I might write something like:

var disallowed = IPNetwork.Parse(disallowedCidrString);
if (disallowed.Contains(remoteIp)) {
  // Deny
} else {
  // Allow
}

As the user wants to deny an IPv4 range they write something like "10.0.0.8/8". However this creates problems because it will produce an IPv4 Network that will not match the dual stack Ipv4 mapped ipv6 address.

My interpretation is that the IPv4-Mapped IPv6 Address is actually an IPv4 address and hence should be matchable using a IPv4 IPNetwork. Under this assumption a change to the IPNetwork.Contains implementation as proposed here could prevent such confusions.

ps: I was unsure whether reporting this as an API Suggestion is correct as this does not change the visible API. However as it would be an API Behavior change it seemed to be the most fitting category. Let me know if this was incorrect.

API Proposal

No visible changes except maybe to documentation. My suggestion would be to make the Contains function use IPAddress.IsIPv4MappedToIPv6 to identify that it is actually passed an IPv4 address to make contains behave as expected.

API Usage

var address = IPAddress.Parse("::ffff:10.0.0.12");
var network = IPNetwork.Parse("10.0.0.0/8");

Debug.Assert(network.Contains(address));

Alternative Designs

Handling this as a developer without the proposed change certainly is possible by inspecting the address and network at the point of comparison to work around the existing behavior. However it has to be done carefully to get it right. E.g. simply trying to always match using a IPv6 network would fail if the server configuration was changed to bind only to IPv4.

Risks

Existing usage might in some way expect IPv6 addresses to never get be reported as being contained in IPv4 networks. I cannot think of actual use-case for this though.

Author:	hacst
Assignees:	-
Labels:	api-suggestion, area-System.Net
Milestone:	-

[antonfirsov]

Apart from being a breaking change, the proposed behavior would contradict the semantics of IPAddress.Equals and the difference between the two methods could be counter-intuitive to some users. Another option to address this is to introduce a separate overload, eg. Contains(IPAddress, bool) or Contains(IPAddress, SomeComparisonMethodSelectorEnum).

@wfurt @MihaZupan thoughts/preferences?

[hacst]

That makes a lot of sense. Especially if it allows having something similar for IPAddress equality comparisons.

[wfurt]

IPAddress has IPAddress.MapToIPv6 but IPNetwork does not. If somebody wants to treat dual mode same as IPv4 it seems easy for the IPAddress.Equals. Maybe we could use some helpers for handling the duality.

[MihaZupan]

IMO users aren't generally creating IPv4-mapped IPv6 addresses themselves, this is more of an implementation detail of dual mode sockets.
E.g. someone connects to your Kestrel server over IPv4, but IPNetwork.Contains(HttpContext.Connection.RemoteIpAddress) fails because it's mapped to IPv6.

From a usability perspective, I would prefer if we treated such addresses as IPv4 for the purposes of IPNetwork.Contains. I think we should consider the breaking change to the existing overload.

[antonfirsov]

IPAddress has IPAddress.MapToIPv6 but IPNetwork does not. If somebody wants to treat dual mode same as IPv4 it seems easy for the IPAddress.Equals. Maybe we could use some helpers for handling the duality.

I don't know what helpers did you mean, but I think it's easier to map the IPAddress to be tested with Contains(address), than the IPNetwork. But now I tend to agree with OP and #98427 (comment), and I think we should consider changing the default behavior in 9.0. If we decide not to do it, we should document it providing examples for the mapping.

[wfurt]

I'm ok if we choose to change the behavior @antonfirsov. But I wanted to explore other options as well if we are concerned about compatibiliyty.