p2p: check secret conn id matches dialed id (#3321)

ref: [#3010 (comment)](https://github.com/tendermint/tendermint/issues/3010#issuecomment-464287627)

> I tried searching for code where we authenticate a peer against its NetAddress.ID and couldn't find it. I don't see a reason to switch to Noise, but a need to ensure that the node's ID is authenticated e.g. after dialing from the address book.

* p2p: check secret conn id matches dialed id

* Fix all p2p tests & make code compile

* add simple test for dialing with wrong ID

* update changelog

* address review comments

* yet another place where to use IDAddressString and fix
testSetupMultiplexTransport

CHANGELOG_PENDING.md

@@ -33,3 +33,4 @@ Special thanks to external contributors on this release:
   * IPv6 ("2001:db8::1"), if ip is a valid IPv6 address
 * [cmd] \#3314 Return an error on `show_validator` when the private validator
   file does not exist
+* [p2p] \#3321 Authenticate a peer against its NetAddress.ID while dialing 

p2p/transport.go

@@ -194,7 +194,7 @@ func (mt *MultiplexTransport) Dial(
 		return nil, err
 	}
 
-	secretConn, nodeInfo, err := mt.upgrade(c)
+	secretConn, nodeInfo, err := mt.upgrade(c, &addr)
 	if err != nil {
 		return nil, err
 	}
@@ -262,7 +262,7 @@ func (mt *MultiplexTransport) acceptPeers() {
 
 			err := mt.filterConn(c)
 			if err == nil {
-				secretConn, nodeInfo, err = mt.upgrade(c)
+				secretConn, nodeInfo, err = mt.upgrade(c, nil)
 			}
 
 			select {
@@ -279,9 +279,9 @@ func (mt *MultiplexTransport) acceptPeers() {
 
 // Cleanup removes the given address from the connections set and
 // closes the connection.
-func (mt *MultiplexTransport) Cleanup(peer Peer) {
+func (mt *MultiplexTransport) Cleanup(p Peer) {
-	mt.conns.RemoveAddr(peer.RemoteAddr())
+	mt.conns.RemoveAddr(p.RemoteAddr())
-	_ = peer.CloseConn()
+	_ = p.CloseConn()
 }
 
 func (mt *MultiplexTransport) cleanup(c net.Conn) error {
@@ -335,6 +335,7 @@ func (mt *MultiplexTransport) filterConn(c net.Conn) (err error) {
 
 func (mt *MultiplexTransport) upgrade(
 	c net.Conn,
+	dialedAddr *NetAddress,
 ) (secretConn *conn.SecretConnection, nodeInfo NodeInfo, err error) {
 	defer func() {
 		if err != nil {
@@ -351,6 +352,23 @@ func (mt *MultiplexTransport) upgrade(
 		}
 	}
 
+	// For outgoing conns, ensure connection key matches dialed key.
+	connID := PubKeyToID(secretConn.RemotePubKey())
+	if dialedAddr != nil {
+		if dialedID := dialedAddr.ID; connID != dialedID {
+			return nil, nil, ErrRejected{
+				conn: c,
+				id:   connID,
+				err: fmt.Errorf(
+					"conn.ID (%v) dialed ID (%v) missmatch",
+					connID,
+					dialedID,
+				),
+				isAuthFailure: true,
+			}
+		}
+	}
+
 	nodeInfo, err = handshake(secretConn, mt.handshakeTimeout, mt.nodeInfo)
 	if err != nil {
 		return nil, nil, ErrRejected{
@@ -369,7 +387,7 @@ func (mt *MultiplexTransport) upgrade(
 	}
 
 	// Ensure connection key matches self reported key.
-	if connID := PubKeyToID(secretConn.RemotePubKey()); connID != nodeInfo.ID() {
+	if connID != nodeInfo.ID() {
 		return nil, nil, ErrRejected{
 			conn: c,
 			id:   connID,

p2p/transport_test.go

@@ -160,8 +160,7 @@ func TestTransportMultiplexAcceptMultiple(t *testing.T) {
 					},
 				)
 			)
-
+			addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
-			addr, err := NewNetAddressStringWithOptionalID(mt.listener.Addr().String())
 			if err != nil {
 				errc <- err
 				return
@@ -230,7 +229,7 @@ func TestTransportMultiplexAcceptNonBlocking(t *testing.T) {
 
 	// Simulate slow Peer.
 	go func() {
-		addr, err := NewNetAddressStringWithOptionalID(mt.listener.Addr().String())
+		addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
 		if err != nil {
 			errc <- err
 			return
@@ -281,8 +280,7 @@ func TestTransportMultiplexAcceptNonBlocking(t *testing.T) {
 				},
 			)
 		)
-
+		addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
-		addr, err := NewNetAddressStringWithOptionalID(mt.listener.Addr().String())
 		if err != nil {
 			errc <- err
 			return
@@ -328,7 +326,7 @@ func TestTransportMultiplexValidateNodeInfo(t *testing.T) {
 			)
 		)
 
-		addr, err := NewNetAddressStringWithOptionalID(mt.listener.Addr().String())
+		addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
 		if err != nil {
 			errc <- err
 			return
@@ -371,8 +369,7 @@ func TestTransportMultiplexRejectMissmatchID(t *testing.T) {
 				PrivKey: ed25519.GenPrivKey(),
 			},
 		)
-
+		addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
-		addr, err := NewNetAddressStringWithOptionalID(mt.listener.Addr().String())
 		if err != nil {
 			errc <- err
 			return
@@ -401,6 +398,38 @@ func TestTransportMultiplexRejectMissmatchID(t *testing.T) {
 	}
 }
 
+func TestTransportMultiplexDialRejectWrongID(t *testing.T) {
+	mt := testSetupMultiplexTransport(t)
+
+	var (
+		pv     = ed25519.GenPrivKey()
+		dialer = newMultiplexTransport(
+			testNodeInfo(PubKeyToID(pv.PubKey()), ""), // Should not be empty
+			NodeKey{
+				PrivKey: pv,
+			},
+		)
+	)
+
+	wrongID := PubKeyToID(ed25519.GenPrivKey().PubKey())
+	addr, err := NewNetAddressStringWithOptionalID(IDAddressString(wrongID, mt.listener.Addr().String()))
+	if err != nil {
+		t.Fatalf("invalid address with ID: %v", err)
+	}
+
+	_, err = dialer.Dial(*addr, peerConfig{})
+	if err != nil {
+		t.Logf("connection failed: %v", err)
+		if err, ok := err.(ErrRejected); ok {
+			if !err.IsAuthFailure() {
+				t.Errorf("expected auth failure")
+			}
+		} else {
+			t.Errorf("expected ErrRejected")
+		}
+	}
+}
+
 func TestTransportMultiplexRejectIncompatible(t *testing.T) {
 	mt := testSetupMultiplexTransport(t)
 
@@ -416,8 +445,7 @@ func TestTransportMultiplexRejectIncompatible(t *testing.T) {
 				},
 			)
 		)
-
+		addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
-		addr, err := NewNetAddressStringWithOptionalID(mt.listener.Addr().String())
 		if err != nil {
 			errc <- err
 			return
@@ -448,7 +476,7 @@ func TestTransportMultiplexRejectSelf(t *testing.T) {
 	errc := make(chan error)
 
 	go func() {
-		addr, err := NewNetAddressStringWithOptionalID(mt.listener.Addr().String())
+		addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
 		if err != nil {
 			errc <- err
 			return
@@ -466,7 +494,7 @@ func TestTransportMultiplexRejectSelf(t *testing.T) {
 	if err := <-errc; err != nil {
 		if err, ok := err.(ErrRejected); ok {
 			if !err.IsSelf() {
-				t.Errorf("expected to reject self")
+				t.Errorf("expected to reject self, got: %v", err)
 			}
 		} else {
 			t.Errorf("expected ErrRejected")
@@ -478,7 +506,7 @@ func TestTransportMultiplexRejectSelf(t *testing.T) {
 	_, err := mt.Accept(peerConfig{})
 	if err, ok := err.(ErrRejected); ok {
 		if !err.IsSelf() {
-			t.Errorf("expected to reject self")
+			t.Errorf("expected to reject self, got: %v", err)
 		}
 	} else {
 		t.Errorf("expected ErrRejected")
@@ -566,9 +594,10 @@ func TestTransportHandshake(t *testing.T) {
 func testSetupMultiplexTransport(t *testing.T) *MultiplexTransport {
 	var (
 		pv = ed25519.GenPrivKey()
+		id = PubKeyToID(pv.PubKey())
 		mt = newMultiplexTransport(
 			testNodeInfo(
-				PubKeyToID(pv.PubKey()), "transport",
+				id, "transport",
 			),
 			NodeKey{
 				PrivKey: pv,
@@ -576,7 +605,7 @@ func testSetupMultiplexTransport(t *testing.T) *MultiplexTransport {
 		)
 	)
 
-	addr, err := NewNetAddressStringWithOptionalID("127.0.0.1:0")
+	addr, err := NewNetAddressStringWithOptionalID(IDAddressString(id, "127.0.0.1:0"))
 	if err != nil {
 		t.Fatal(err)
 	}