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 · 6 comments
Open

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

adam-p opened this issue Mar 24, 2022 · 6 comments
Labels
NeedsInvestigation
Milestone

Comments

@adam-p
Copy link
Contributor

@adam-p 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 label Mar 24, 2022
@mknyszek mknyszek added this to the Backlog milestone Mar 24, 2022
@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Mar 24, 2022

Is this new in 1.18? Thanks for the reproducer.

CC @neild

@adam-p
Copy link
Contributor Author

@adam-p 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 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 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 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 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

3 participants