swarm + handshake: better observed addr check

The check needed knowledge of the _listen_ addresses,
not just the interface addresses. Also, the handshake now
sends out all the addresses we accumulate about ourselves.
(this may be bad in the long run, but useful now to test)

net/conn/handshake.go

@@ -3,15 +3,12 @@ package conn
 import (
 	"errors"
 	"fmt"
-	"strings"
 
 	handshake "github.com/jbenet/go-ipfs/net/handshake"
 	hspb "github.com/jbenet/go-ipfs/net/handshake/pb"
-	u "github.com/jbenet/go-ipfs/util"
 
 	context "github.com/jbenet/go-ipfs/Godeps/_workspace/src/code.google.com/p/go.net/context"
 	proto "github.com/jbenet/go-ipfs/Godeps/_workspace/src/code.google.com/p/goprotobuf/proto"
-	ma "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-multiaddr"
 )
 
 // Handshake1 exchanges local and remote versions and compares them
@@ -62,87 +59,48 @@ func Handshake1(ctx context.Context, c Conn) error {
 }
 
 // Handshake3 exchanges local and remote service information
-func Handshake3(ctx context.Context, c Conn) error {
+func Handshake3(ctx context.Context, c Conn) (*handshake.Handshake3Result, error) {
 	rpeer := c.RemotePeer()
 	lpeer := c.LocalPeer()
 
+	// setup + send the message to remote
 	var remoteH, localH *hspb.Handshake3
-	localH = handshake.Handshake3Msg(lpeer)
-
-	rma := c.RemoteMultiaddr()
-	localH.ObservedAddr = proto.String(rma.String())
-
+	localH = handshake.Handshake3Msg(lpeer, c.RemoteMultiaddr())
 	localB, err := proto.Marshal(localH)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	c.Out() <- localB
 	log.Debugf("Handshake1: sent to %s", rpeer)
 
+	// wait + listen for response
 	select {
 	case <-ctx.Done():
-		return ctx.Err()
+		return nil, ctx.Err()
 
 	case <-c.Closing():
-		return errors.New("Handshake3: error remote connection closed")
+		return nil, errors.New("Handshake3: error remote connection closed")
 
 	case remoteB, ok := <-c.In():
 		if !ok {
-			return fmt.Errorf("Handshake3 error receiving from conn: %v", rpeer)
+			return nil, fmt.Errorf("Handshake3 error receiving from conn: %v", rpeer)
 		}
 
 		remoteH = new(hspb.Handshake3)
 		err = proto.Unmarshal(remoteB, remoteH)
 		if err != nil {
-			return fmt.Errorf("Handshake3 could not decode remote msg: %q", err)
+			return nil, fmt.Errorf("Handshake3 could not decode remote msg: %q", err)
 		}
 
 		log.Debugf("Handshake3 received from %s", rpeer)
 	}
 
-	if err := handshake.Handshake3UpdatePeer(rpeer, remoteH); err != nil {
+	// actually update our state based on the new knowledge
+	res, err := handshake.Handshake3Update(lpeer, rpeer, remoteH)
+	if err != nil {
 		log.Errorf("Handshake3 failed to update %s", rpeer)
-		return err
 	}
-
-	// If we are behind a NAT, inform the user that certain things might not work yet
-	nat, err := checkNAT(remoteH.GetObservedAddr())
-	if err != nil {
-		log.Errorf("Error in NAT detection: %s", err)
-	}
-	if nat {
-		msg := `Remote peer observed our address to be: %s
-		The local addresses are: %s
-		Thus, connection is going through NAT, and other connections may fail.
-
-		IPFS NAT traversal is still under development. Please bug us on github or irc to fix this.
-		Baby steps: http://jbenet.static.s3.amazonaws.com/271dfcf/baby-steps.gif
-		`
-		addrs, _ := u.GetLocalAddresses()
-		log.Warning(fmt.Sprintf(msg, remoteH.GetObservedAddr(), addrs))
-	}
-
-	return nil
-}
-
-// checkNAT returns whether or not we might be behind a NAT
-func checkNAT(observedaddr string) (bool, error) {
-	observedma, err := ma.NewMultiaddr(observedaddr)
-	if err != nil {
-		return false, err
-	}
-	addrs, err := u.GetLocalAddresses()
-	if err != nil {
-		return false, err
-	}
-
-	omastr := observedma.String()
-	for _, addr := range addrs {
-		if strings.HasPrefix(omastr, addr.String()) {
-			return false, nil
-		}
-	}
-
-	return true, nil
+	res.RemoteObservedAddress = c.RemoteMultiaddr()
+	return res, nil
 }

net/handshake/handshake3.go

@@ -13,18 +13,21 @@ import (
 var log = u.Logger("handshake")
 
 // Handshake3Msg constructs a Handshake3 msg.
-func Handshake3Msg(localPeer peer.Peer) *pb.Handshake3 {
+func Handshake3Msg(localPeer peer.Peer, remoteAddr ma.Multiaddr) *pb.Handshake3 {
 	var msg pb.Handshake3
 	// don't need publicKey after secure channel.
 	// msg.PublicKey = localPeer.PubKey().Bytes()
 
-	// addresses
+	// local listen addresses
 	addrs := localPeer.Addresses()
 	msg.ListenAddrs = make([][]byte, len(addrs))
 	for i, a := range addrs {
 		msg.ListenAddrs[i] = a.Bytes()
 	}
 
+	// observed remote address
+	msg.ObservedAddr = remoteAddr.Bytes()
+
 	// services
 	// srv := localPeer.Services()
 	// msg.Services = make([]mux.ProtocolID, len(srv))
@@ -35,20 +38,43 @@ func Handshake3Msg(localPeer peer.Peer) *pb.Handshake3 {
 	return &msg
 }
 
-// Handshake3UpdatePeer updates a remote peer with the information in the
-// handshake3 msg we received from them.
-func Handshake3UpdatePeer(remotePeer peer.Peer, msg *pb.Handshake3) error {
+// Handshake3Update updates local knowledge with the information in the
+// handshake3 msg we received from remote client.
+func Handshake3Update(lpeer, rpeer peer.Peer, msg *pb.Handshake3) (*Handshake3Result, error) {
+	res := &Handshake3Result{}
 
-	// addresses
+	// our observed address
+	observedAddr, err := ma.NewMultiaddrBytes(msg.GetObservedAddr())
+	if err != nil {
+		return res, err
+	}
+	lpeer.AddAddress(observedAddr)
+	res.LocalObservedAddress = observedAddr
+
+	// remote's reported addresses
 	for _, a := range msg.GetListenAddrs() {
 		addr, err := ma.NewMultiaddrBytes(a)
 		if err != nil {
 			err = fmt.Errorf("remote peer address not a multiaddr: %s", err)
-			log.Errorf("Handshake3: error %s", err)
-			return err
+			log.Errorf("Handshake3 error %s", err)
+			return res, err
 		}
-		remotePeer.AddAddress(addr)
+		rpeer.AddAddress(addr)
+		res.RemoteListenAddresses = append(res.RemoteListenAddresses, addr)
 	}
 
-	return nil
+	return res, nil
+}
+
+// Handshake3Result collects the knowledge gained in Handshake3.
+type Handshake3Result struct {
+
+	// The addresses reported by the remote client
+	RemoteListenAddresses []ma.Multiaddr
+
+	// The address of the remote client we observed in this connection
+	RemoteObservedAddress ma.Multiaddr
+
+	// The address the remote client observed from this connection
+	LocalObservedAddress ma.Multiaddr
 }

net/handshake/pb/handshake.pb.go

@@ -15,10 +15,12 @@ It has these top-level messages:
 package handshake_pb
 
 import proto "github.com/jbenet/go-ipfs/Godeps/_workspace/src/code.google.com/p/gogoprotobuf/proto"
+import json "encoding/json"
 import math "math"
 
-// Reference imports to suppress errors if they are not otherwise used.
+// Reference proto, json, and math imports to suppress error if they are not otherwise used.
 var _ = proto.Marshal
+var _ = &json.SyntaxError{}
 var _ = math.Inf
 
 // Handshake1 is delivered _before_ the secure channel is initialized
@@ -51,11 +53,13 @@ func (m *Handshake1) GetAgentVersion() string {
 
 // Handshake3 is delivered _after_ the secure channel is initialized
 type Handshake3 struct {
-	// listenAddrs are the multiaddrs this node listens for open connections on
+	// listenAddrs are the multiaddrs the sender node listens for open connections on
 	ListenAddrs [][]byte `protobuf:"bytes,2,rep,name=listenAddrs" json:"listenAddrs,omitempty"`
-	// we'll have more fields here later.
-	ObservedAddr     *string `protobuf:"bytes,4,opt,name=observedAddr" json:"observedAddr,omitempty"`
-	XXX_unrecognized []byte  `json:"-"`
+	// oservedAddr is the multiaddr of the remote endpoint that the sender node perceives
+	// this is useful information to convey to the other side, as it helps the remote endpoint
+	// determine whether its connection to the local peer goes through NAT.
+	ObservedAddr     []byte `protobuf:"bytes,4,opt,name=observedAddr" json:"observedAddr,omitempty"`
+	XXX_unrecognized []byte `json:"-"`
 }
 
 func (m *Handshake3) Reset()         { *m = Handshake3{} }
@@ -69,11 +73,11 @@ func (m *Handshake3) GetListenAddrs() [][]byte {
 	return nil
 }
 
-func (m *Handshake3) GetObservedAddr() string {
-	if m != nil && m.ObservedAddr != nil {
-		return *m.ObservedAddr
+func (m *Handshake3) GetObservedAddr() []byte {
+	if m != nil {
+		return m.ObservedAddr
 	}
-	return ""
+	return nil
 }
 
 func init() {

net/handshake/pb/handshake.proto

@@ -22,7 +22,7 @@ message Handshake3 {
   // - then again, if we change / disable secure channel, may still want it.
   // optional bytes publicKey = 1;
 
-  // listenAddrs are the multiaddrs this node listens for open connections on
+  // listenAddrs are the multiaddrs the sender node listens for open connections on
   repeated bytes listenAddrs = 2;
 
   // TODO
@@ -31,8 +31,8 @@ message Handshake3 {
 
   // we'll have more fields here later.
 
-  // oservedAddr is the multiaddr of the remote endpoint that the local node perceives
+  // oservedAddr is the multiaddr of the remote endpoint that the sender node perceives
   // this is useful information to convey to the other side, as it helps the remote endpoint
   // determine whether its connection to the local peer goes through NAT.
-  optional string observedAddr = 4;
+  optional bytes observedAddr = 4;
 }

net/swarm/addrs.go

@@ -72,3 +72,36 @@ func interfaceAddresses() ([]ma.Multiaddr, error) {
 
 	return nonLoopback, nil
 }
+
+// addrInList returns whether or not an address is part of a list.
+// this is useful to check if NAT is happening (or other bugs?)
+func addrInList(addr ma.Multiaddr, list []ma.Multiaddr) bool {
+	for _, addr2 := range list {
+		if addr.Equal(addr2) {
+			return true
+		}
+	}
+	return false
+}
+
+// checkNATWarning checks if our observed addresses differ. if so,
+// informs the user that certain things might not work yet
+func (s *Swarm) checkNATWarning(observed ma.Multiaddr) {
+	listen, err := s.InterfaceListenAddresses()
+	if err != nil {
+		log.Errorf("Error retrieving swarm.InterfaceListenAddresses: %s", err)
+		return
+	}
+
+	if !addrInList(observed, listen) { // probably a nat
+		log.Warningf(natWarning, observed, listen)
+	}
+}
+
+const natWarning = `Remote peer observed our address to be: %s
+The local addresses are: %s
+Thus, connection is going through NAT, and other connections may fail.
+
+IPFS NAT traversal is still under development. Please bug us on github or irc to fix this.
+Baby steps: http://jbenet.static.s3.amazonaws.com/271dfcf/baby-steps.gif
+`

net/swarm/conn.go

@@ -112,11 +112,15 @@ func (s *Swarm) connSetup(c conn.Conn) (conn.Conn, error) {
 
 	// handshake3
 	ctxT, _ := context.WithTimeout(c.Context(), conn.HandshakeTimeout)
-	if err := conn.Handshake3(ctxT, c); err != nil {
+	h3result, err := conn.Handshake3(ctxT, c)
+	if err != nil {
 		c.Close()
 		return nil, fmt.Errorf("Handshake3 failed: %s", err)
 	}
 
+	// check for nats. you know, just in case.
+	s.checkNATWarning(h3result.LocalObservedAddress)
+
 	// add to conns
 	s.connsLock.Lock()
 

util/util.go

@@ -4,16 +4,13 @@ import (
 	"errors"
 	"io"
 	"math/rand"
-	"net"
 	"os"
 	"path/filepath"
-	"reflect"
 	"strings"
 	"time"
 
 	ds "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore"
-	ma "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-multiaddr"
-	manet "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-multiaddr/net"
+
 	"github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/mitchellh/go-homedir"
 )
 
@@ -111,57 +108,3 @@ func GetenvBool(name string) bool {
 	v := strings.ToLower(os.Getenv(name))
 	return v == "true" || v == "t" || v == "1"
 }
-
-// IsLoopbackAddr returns whether or not the ip portion of the passed in multiaddr
-// string is a loopback address
-func IsLoopbackAddr(addr string) bool {
-	loops := []string{"/ip4/127.0.0.1", "/ip6/::1"}
-	for _, loop := range loops {
-		if strings.HasPrefix(addr, loop) {
-			return true
-		}
-	}
-	return false
-}
-
-// GetLocalAddresses returns a list of ip addresses associated with
-// the local machine
-func GetLocalAddresses() ([]ma.Multiaddr, error) {
-	// Enumerate interfaces on this machine
-	ifaces, err := net.Interfaces()
-	if err != nil {
-		return nil, err
-	}
-
-	var maddrs []ma.Multiaddr
-	for _, i := range ifaces {
-		addrs, err := i.Addrs()
-		if err != nil {
-			log.Warningf("Skipping addr: %s", err)
-			continue
-		}
-		// Check each address and convert to a multiaddr
-		for _, addr := range addrs {
-			switch v := addr.(type) {
-			case *net.IPNet:
-
-				// Build multiaddr
-				maddr, err := manet.FromIP(v.IP)
-				if err != nil {
-					log.Errorf("maddr parsing error: %s", err)
-					continue
-				}
-
-				// Dont list loopback addresses
-				if IsLoopbackAddr(maddr.String()) {
-					continue
-				}
-				maddrs = append(maddrs, maddr)
-			default:
-				// Not sure if any other types will show up here
-				log.Errorf("Got '%s' type = '%s'", v, reflect.TypeOf(v))
-			}
-		}
-	}
-	return maddrs, nil
-}