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

proposal: net: add func(IP) Unmap() IP to help when using net/netip #54234

Closed
bradfitz opened this issue Aug 3, 2022 · 14 comments
Closed

proposal: net: add func(IP) Unmap() IP to help when using net/netip #54234

bradfitz opened this issue Aug 3, 2022 · 14 comments

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Aug 3, 2022

net.TCPAddr.AddrPort and net.UDPAddr.AddrPort can return a netip.AddrPort containing a 4-in-6 IP. (See #53607 (comment))

To "fix" those with a netip.Addr, we have https://pkg.go.dev/net/netip#Addr.Unmap

But with an AddrPort, it's verbose: netip.AddrPortFrom(ap.Addr().Unmap(), ap.Port())

I propose we add:

// Unmap returns p with any IPv4-mapped IPv6 address prefix of its IP address removed.
// The port number is returned unmodified.
func (p AddrPort) Unmap() AddrPort {
        return AddrPortFrom(p.Addr().Unmap(), p.Port())
}

/cc @rsc @josharian @maisem @ianlancetaylor

@gopherbot gopherbot added this to the Proposal milestone Aug 3, 2022
@bradfitz
Copy link
Contributor Author

bradfitz commented Aug 3, 2022

And/or:

package net

// Addr returns ip in its netip.Addr form, unmapped. ....
func (ip IP) Addr() netip.Addr { ... }

Then people can get the unmapped form from net.IP.Addr and the "raw" unmapped form from netip.AddrFromSlice.

/cc @rsc

@rsc
Copy link
Contributor

rsc commented Aug 3, 2022

And then make the net.TCPAddr.AddrPort (and UDPAddr) method use .Addr to unmap the net.IP.

@rsc rsc changed the title proposal: net/netip: add AddrPort.Unmap proposal: net: add func(IP) Addr() net.Addr and use in AddrPort methods Aug 3, 2022
@rsc
Copy link
Contributor

rsc commented Aug 3, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Aug 3, 2022

FWIW, IPv4 returning a 16-byte IP address dates to the very first commit in this package: https://go.googlesource.com/go/+/e8a02230f215efb075cccd4146b3d0d1ada4870e/src/lib/net/ip.go

// IPv4 addresses are 4 bytes; IPv6 addresses are 16 bytes.
// An IPv4 address can be converted to an IPv6 address by
// adding a canonical prefix (10 zeros, 2 0xFFs).
// This library accepts either size of byte array but always
// returns 16-byte addresses.

At the time (2008) that seemed like the way of the future.
Our future has turned out differently.

@rsc rsc moved this to Active in Proposals Aug 10, 2022
@rsc rsc added this to Proposals Aug 10, 2022
@rsc
Copy link
Contributor

rsc commented Aug 10, 2022

The proposal is two pieces:

  1. Add net.IP.Addr (converter from net.IP to netip.Addr), which does the 4-in-6 unmapping. This converts from the package net standard of 'mostly everything is 16 byte addresses' to the netip standard of distinguishing IPv4 from IPv6 at the representation level.

  2. Make net.TCPAddr.AddrPort and net.UDPAddr.AddrPort use this new method, so that they will produce the canonical IPv4 representation for package netip.

The method name Addr is confusing because net.Addr is something else. There are no good names for that method. So here's another possibility:

  • Add func (IP) Unmap() IP that returns a 4-byte slice for IPv4-in-6 addresses. Then that slice can be passed to netip.AddrFromSlice. The new Unmap method essentially provides the convention conversion.

  • Make net.TCPAddr.AddrPort and net.UDPAddr.AddrPort use netip.AddrFromSlice(_.IP.Unmap()).

That will accomplish the same thing but will not have the confusing method name.

We could do just the second too, if we didn't want to imply in package net's API that there is an important semantic difference within package net. But others will probably need Unmap to do the conversion, and there is a semantic difference when you're crossing the boundary into netip. We will need to document in the IP.Unmap method that package net will treat the input and output exactly the same in all cases. (But netip may not.)

@database64128
Copy link
Contributor

database64128 commented Aug 12, 2022

I don't think net.TCPAddr.AddrPort and net.UDPAddr.AddrPort should do the unmapping.

The *net.TCPAddr or *net.UDPAddr instance returned by a LocalAddr() or RemoteAddr() call is directly converted from a system socket address type. The length of the IP slice reflects the address family of the socket. If I open an IPv6 socket, I can expect that the length of the returned IP is always 16. Just like how you can expect the address returned by ReadFromUDPAddrPort and ReadMsgUDPAddrPort on an IPv6 socket is either IPv4-mapped IPv6 or IPv6.

IMO this behavior should be preserved, because this is how the underlying socket works. Those who wish to unmap IPv4-mapped IPv6 addresses can always do the unmapping themselves in their own code.

@bradfitz
Copy link
Contributor Author

If I open an IPv6 socket, I can expect that the length of the returned IP is always 16

And if there's no address family specified, as in net.Listen("udp", ":1234") (#53607)? I don't think users expect to randomly get either IPv4 or IPv6 addresses depending on which operating system they're on and how that OS was configured. That's not a great API to not know what you're going to get.

@database64128
Copy link
Contributor

database64128 commented Aug 12, 2022

I don't think users expect to randomly get either IPv4 or IPv6 addresses depending on which operating system they're on and how that OS was configured. That's not a great API to not know what you're going to get.

Except it has always been like this. On most systems net.Listen("udp", ":1234") is going to get you a dual-stack IPv6 UDP socket. On OpenBSD and DragonFly BSD this is equivalent to doing net.Listen("udp4", ":1234") on other systems, because these two BSDs do not support dual-stack sockets.

Unless being able to run on OpenBSD and DragonFly BSD is a requirement, one can usually safely assume that net.Listen("udp", ":1234") is going to open a dual-stack IPv6 UDP socket. And in my experience this is usually the way things are being done too.

@bradfitz
Copy link
Contributor Author

(Rather than fork this whole conversation in two threads, I've only replied in #53607 (comment))

@rsc rsc changed the title proposal: net: add func(IP) Addr() net.Addr and use in AddrPort methods proposal: net: add func(IP) Unmap() IP to help when using net/netip Aug 17, 2022
@rsc
Copy link
Contributor

rsc commented Aug 17, 2022

Last week I wrote that there could be two parts:

  • Add func (IP) Unmap() IP that returns a 4-byte slice for IPv4-in-6 addresses. Then that slice can be passed to netip.AddrFromSlice. The new Unmap method essentially provides the convention conversion.

  • Make net.TCPAddr.AddrPort and net.UDPAddr.AddrPort use netip.AddrFromSlice(_.IP.Unmap()).

#53607 seems to have taken on the question of whether to do the second.

For this issue, let's decide whether to do the first. Given that we have one package (net) that almost always returns 16-byte addresses and another package (net/netip) that cares about 4-byte vs 16-byte, it makes sense to have a converter available. That would be IP.Unmap.

Does anyone object to adding IP.Unmap?

@rsc
Copy link
Contributor

rsc commented Aug 31, 2022

With this issue restricted to adding IP.Unmap, with a clear doc comment saying that it is only meaningful when using net/netip - net doesn't treat the input and output differently - the consensus seems to be that it makes sense to provide this functionality, specifically for crossing the net -> netip boundary. Do I have that right? Does anyone object to adding IP.Unmap?

The discussion about TCPAddr / UDPAddr is now #53607.

@ainar-g
Copy link
Contributor

ainar-g commented Aug 31, 2022

I don't object to that per se, but I don't think that it's that useful, since you can already do:

newIP := netip.AddrFromSlice(oldIP).Unmap()

In other words, it really makes no difference whether one unmaps net.IP or converts it first and then unmaps the resulting netip.Addr. (Or at least I don't see one.)

I feel like the original API proposed in the OP is more of a convenience specifically for net.TCPAddrs and net.UDPAddrs. If that discussion results in actually returning the unmapped addresses then, perhaps, this addition wouldn't be necessary.

@rsc
Copy link
Contributor

rsc commented Sep 7, 2022

I think at the turn in the discussion at #54234 (comment), I didn't realize that we already had netip.AddrFromSlice(old).Unmap() when I proposed we add old.Unmap to use in netip.AddrFromSlice(old.Unmap()).

The proposal started out as about TCPAddr and UDPAddr but that has moved to #53607, so maybe there's nothing left here.

Closing this as a duplicate of #53607.

@rsc
Copy link
Contributor

rsc commented Sep 7, 2022

This proposal is a duplicate of a previously discussed proposal, as noted above,
and there is no significant new information to justify reopening the discussion.
The issue has therefore been declined as a duplicate.
— rsc for the proposal review group

@rsc rsc moved this from Active to Declined in Proposals Sep 7, 2022
@rsc rsc closed this as completed Sep 7, 2022
@julieqiu julieqiu removed this from Go Security Sep 8, 2022
@golang golang locked and limited conversation to collaborators Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants