Properly report bound addresses when using Tor

[jholdstock]

I noticed when using dcrd over Tor that getpeerinfo would return malformed addrlocal values. Tracked that down to a bug which is fixed in the first commit. Then every address was reported as "0.0.0.0:0" which is fixed by the second commit.

[davecgh]

The first commit makes sense and is an obvious bug fix.

This probably should be the following to match the described semantics:

Suggested change

	if ip != nil && !ip.IsUnspecified() {
	if ip == nil || !ip.IsUnspecified() {

That would allow it to continue returning hostnames which is explicitly allowed by RFC 1928. Under the requests section (section 4) of the RFC, it explicitly allows for ATYP == DOMAINNAME: X'03.

Your diff, as written, would fail to parse a hostname making ip == nil and then fall through to return the local address of the local connection.

That said, I concede that, in practice, every SOCK5 proxy I've ever seen only ever returns IP addresses, so in practice, it really won't matter. Still, I think the suggestion code is strictly more accurate.

[davecgh]

This is not something that should hold up this PR, because it is simply addresses the abstraction that already exists, and in that regard, it is fine for what it does.

However, upon looking at this diff, I personally think the abstraction is not really all that accurate and it should probably be changed. The package is effectively treating the returned net.Conn as a logical end-to-end connection, but that runs counter to virtually every other abstraction used in the Go standard library which is to instead treat it as a transport connection.

By reporting the proxy's bound address as LocalAddr, it no longer means what you would expect. Namely, I expect that LocalAddr is the address of the local side of the connection, specifically the connection to the proxy. Instead, this package makes it the local address of the connection the proxy made which is actually a separate connection.

Personally, I think it would make much more sense to return the actual local address of the connection made to the proxy, because that is in fact the connection we've made. Then, because the caller might also want to know details about the local address the proxy ultimately selected (say it's a multi-homed proxy, or maybe the proxy exposes a NAT address, etc) when it makes the separate connection on your behalf, I would expose that via something like ProxyBoundAddr() net.Addr.

That approach would keep the net.Conn semantics identical to the standard library and still expose the SOCKS-specific information from the RFC if the caller wants it.

For a more concrete example of what I'm describing in case it isn't clear.

Imagine the following setup:

• Client: multi-homed with two different interface addrs: 10.0.0.5, and 192.168.1.30.
• Proxy: 198.51.x.x:1080
• Destination: 203.0.x.x:9108

Then you do:

conn, err := proxy.Dial("tcp", 203.0.x.x:9108) 
// err handling elided

With the current abstraction, conn.LocalAddr() == 198.51.x.x:<ephemeral port for conn created by the prox> instead of either 10.0.0.5:<ephemeral port for our actual connection to the proxy> or 192.168.1.30:<ephemeral port for our actual connection to the proxy>

So now, if you were want to find out which interface the connection is using in the typical way everything else in Go works, you can't!

e.g.

if host(conn.LocalAddr()) == host(listener1.Addr() {
	// Using interface associated with listener1
} else if host(conn.LocalAddr()) == host(listener2.Addr() {
	// Using interface associated with listener2
}

On the other hand, if it were changed in the way I think makes more sense, not only would that work, but if you want to know the address the proxy actually reported via BND.ADDR and BND.PORT (aka the local address the proxy ultimately selected, which could very well be on a different machine even), it could be obtained via type asserting the conn implements ProxyBoundAddr() net.Addr and invoking it.