Skip to content
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.

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

net: inconsistent behaviour with IPv4-mapped IPv6 CIDRs #51906

Open
adam-p opened this issue Mar 24, 2022 · 7 comments
Open

net: inconsistent behaviour with IPv4-mapped IPv6 CIDRs #51906

adam-p opened this issue Mar 24, 2022 · 7 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@adam-p
Copy link
Contributor

adam-p commented Mar 24, 2022

What version of Go are you using (go version)?

$ go version
go version go1.18 windows/amd64

Same behaviour on playground.

Does this issue reproduce with the latest release?

Yes.

What did you do?

https://go.dev/play/p/tohTC3rXoZt

	cidrString := "::ffff:4.4.4.4/64"
	fmt.Println(cidrString)
	_, cidr, _ := net.ParseCIDR(cidrString)
	fmt.Println(cidr)              // == ::/64, seems like it could be the problem
	ip := net.ParseIP("::ffff:4.4.4.4")
	fmt.Println(ip)
	fmt.Println(cidr.Contains(ip)) // == false, seems wrong

	ip = net.ParseIP("4.4.4.4")
	fmt.Println(ip)
	fmt.Println(cidr.Contains(ip)) // == false, seems maybe wrong

	fmt.Println("---")

	cidrString = "::ffff:4.4.4.4/124"
	fmt.Println(cidrString)
	_, cidr, _ = net.ParseCIDR(cidrString)
	fmt.Println(cidr)
	ip = net.ParseIP("::ffff:4.4.4.4")
	fmt.Println(ip)
	fmt.Println(cidr.Contains(ip)) // == true, seems right

	ip = net.ParseIP("4.4.4.4")
	fmt.Println(ip)
	fmt.Println(cidr.Contains(ip)) // == true, seems right

What did you expect to see?

That ::ffff:4.4.4.4/64 would contain ::ffff:4.4.4.4 and 4.4.4.4 (or at least one of them, like netip.Prefix).

What did you see instead?

It doesn't.

My guess is that the problem is that ::ffff:4.4.4.4/64 is turning into the IPv6 CIDR ::/64, but ::ffff:4.4.4.4 is turning into the IPv4 4.4.4.4. And the v4 IP will never be contained in the v6 CIDR. In contrast, ::ffff:4.4.4.4/124 turns into a v4 CIDR and contains the IPs.

@mknyszek mknyszek changed the title net: Inconsistent behaviour with IPv4-mapped IPv6 CIDRs net: inconsistent behaviour with IPv4-mapped IPv6 CIDRs Mar 24, 2022
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 24, 2022
@mknyszek mknyszek added this to the Backlog milestone Mar 24, 2022
@mknyszek
Copy link
Contributor

Is this new in 1.18? Thanks for the reproducer.

CC @neild

@adam-p
Copy link
Contributor Author

adam-p commented Mar 24, 2022

@mknyszek I tested with 1.13, 1.15, and 1.17 with identical results.

@neild
Copy link
Contributor

neild commented Mar 24, 2022

The problem is that the net package aggressively converts IPv4-mapped-IPv6 addresses (like ::ffff:4.4.4.4) into IPv4 addresses, but net.IPNet.Contains doesn't know how to test an IPv6 IPNet against an IPv4 address.

Two possible fixes:

  1. Change ParseCIDR to not convert IPv4-mapped-IPv6 addresses to IPv4, and change NetIP.Contains to not convert addresses to IPv4 when the NetIP is an IPv6 prefix.
  2. or leave ParseCIDR alone, and change NetIP.Contains to convert addresses to IPv6 when the NetIP is a suffix of ::ffff:0000/80.

@adam-p
Copy link
Contributor Author

adam-p commented Mar 24, 2022

  1. Change ParseCIDR to not convert IPv4-mapped-IPv6 addresses to IPv4, and change NetIP.Contains to not convert addresses to IPv4 when the NetIP is an IPv6 prefix.

That sounds like it would mimic the behaviour of netip.Prefix.Contains, which is probably a good thing. "An IPv4 address will not match an IPv6 prefix. A v6-mapped IPv6 address will not match an IPv4 prefix." (godoc) This is annoying and convoluted enough that simplicity and consistency counts for a lot.

(Wait, does that godoc quote have a typo? "v6-mapped IPv6 address" should be "v4-mapped IPv6 address"?)

  1. or leave ParseCIDR alone, and change NetIP.Contains to convert addresses to IPv6 when the NetIP is a suffix of ::ffff:0000/80.

Small typo: I think you mean ::ffff:0.0.0.0/80 or ::ffff:0:0/96. (And... you mean "IPNet" rather than "NetIP", I'm sure.)

I'm having trouble with the word "suffix". What does it mean for a range to be a suffix of another range?

In case this is helpful...

_, cidr, _ := net.ParseCIDR("::ffff:1.1.1.1/80")
fmt.Println(cidr) 
==> ::/80

fmt.Printf("%#v\n", cidr)
==> &net.IPNet{IP:net.IP{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, Mask:net.IPMask{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}}
...which is all zeros in the IP part, so the mapped-ness is lost

@neild
Copy link
Contributor

neild commented Mar 24, 2022

Small typo: I think you mean ::ffff:0.0.0.0/80 or ::ffff:0:0/96. (And... you mean "IPNet" rather than "NetIP", I'm sure.)

Yes, too many "net"s and "ip"s.

I'm having trouble with the word "suffix". What does it mean for a range to be a suffix of another range?

A simpler and hopefully clearer thought: We could say that IPNet.Contains where the IPNet is IPv6 and the IP is IPv4 converts the address to an IPv4-mapped IPv6 address before performing the containment check.

@adam-p
Copy link
Contributor Author

adam-p commented Mar 25, 2022

A simpler and hopefully clearer thought: We could say that IPNet.Contains where the IPNet is IPv6 and the IP is IPv4 converts the address to an IPv4-mapped IPv6 address before performing the containment check.

That seems reasonable.

It does mean that ::/80 (and any mask bits fewer than that) contains all IPv4 addresses. I'm not saying that's a bad thing, but it should be considered. Would it surprise a user in a bad way?

@mkowalski
Copy link

Hey folks, I'm not sure if this has been silently abandoned, but I just want to add 5 cents here...

x, y, _ := net.ParseCIDR("::ffff:192.168.0.14/64")
fmt.Println(x) // == 192.168.0.14
fmt.Println(y) // == ::/64

This behaviour is mildly confusing. I am working a lot with code which gets user input into net.ParseCIDR and then operates on net.IP object returned. The fact that the latter holds no memory of the original IP stack leads to some misunderstandings.

In some cases I can overcome it by never looking at net.IP itself but always together with net.IPMask. This way, seeing IPv4 and mask e.g. /64 it's clear that the address was originally IPv6 and was mapped. But this approach is not always ideal because we can take another example

x, y, _ := net.ParseCIDR("::ffff:192.168.0.14/16")
fmt.Println(x) // == 192.168.0.14
fmt.Println(y) // == ::16
fmt.Println(y.Mask.Size()) // == 16 128

Now we have a mask /16 and IP that looks like IPv4, so the pair looks like a legit IPv4 configuration. Sure, one could say that the mask till holds the knowledge about the origin in the total length, 128 in here.

But the very last part is only an implementation detail and from what I can read it's length is not guaranteed, just like

  • net.IP([]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 192, 168, 1, 160})
  • net.IP([]byte{192, 168, 1, 160})

are both valid representations of IPv4 address because of how they are created (i.e. func IPv4(a, b, c, d byte) IP in https://go.dev/src/net/ip.go)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants