From d2c7f8dbcf238ccc3a2469e350523c7ed28be154 Mon Sep 17 00:00:00 2001 From: Ismail Khoffi Date: Mon, 18 Feb 2019 11:08:22 +0100 Subject: [PATCH] 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 | 1 + p2p/transport.go | 30 +++++++++++++++++----- p2p/transport_test.go | 59 ++++++++++++++++++++++++++++++++----------- 3 files changed, 69 insertions(+), 21 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 253d4f98d..fa96c00e1 100644 --- a/CHANGELOG_PENDING.md +++ b/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 diff --git a/p2p/transport.go b/p2p/transport.go index 2d4420a11..d1bccf9b8 100644 --- a/p2p/transport.go +++ b/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) { - mt.conns.RemoveAddr(peer.RemoteAddr()) - _ = peer.CloseConn() +func (mt *MultiplexTransport) Cleanup(p Peer) { + mt.conns.RemoveAddr(p.RemoteAddr()) + _ = 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, diff --git a/p2p/transport_test.go b/p2p/transport_test.go index 7d9c17fb4..81f9d1b8e 100644 --- a/p2p/transport_test.go +++ b/p2p/transport_test.go @@ -160,8 +160,7 @@ func TestTransportMultiplexAcceptMultiple(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 @@ -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(mt.listener.Addr().String()) + addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), 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(mt.listener.Addr().String()) + addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), 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(mt.listener.Addr().String()) + addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), 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) }