* OriginalAddr -> SocketAddr

OriginalAddr records the originally dialed address for outbound peers,
rather than the peer's self reported address. For inbound peers, it was
nil. Here, we rename it to SocketAddr and for inbound peers, set it to
the RemoteAddr of the connection.

* use SocketAddr

Numerous places in the code call peer.NodeInfo().NetAddress().
However, this call to NetAddress() may perform a DNS lookup if the
reported NodeInfo.ListenAddr includes a name. Failure of this lookup
returns a nil address, which can lead to panics in the code.

Instead, call peer.SocketAddr() to return the static address of the
connection.

* remove nodeInfo.NetAddress()

Expose `transport.NetAddress()`, a static result determined
when the transport is created. Removing NetAddress() from the nodeInfo
prevents accidental DNS lookups.

* fixes from review

* linter

* fixes from review
This commit is contained in:
Ethan Buchman 2019-04-01 19:59:57 -04:00 committed by GitHub
parent 1ecf814838
commit 882622ec10
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 127 additions and 109 deletions

View File

@ -489,7 +489,7 @@ func NewNode(config *cfg.Config,
addrBook := pex.NewAddrBook(config.P2P.AddrBookFile(), config.P2P.AddrBookStrict)
// Add ourselves to addrbook to prevent dialing ourselves
addrBook.AddOurAddress(nodeInfo.NetAddress())
addrBook.AddOurAddress(sw.NetAddress())
addrBook.SetLogger(p2pLogger.With("book", config.P2P.AddrBookFile()))
if config.P2P.PexReactor {

View File

@ -62,7 +62,7 @@ func (mp *Peer) Get(key string) interface{} {
func (mp *Peer) Set(key string, value interface{}) {
mp.kv[key] = value
}
func (mp *Peer) RemoteIP() net.IP { return mp.ip }
func (mp *Peer) OriginalAddr() *p2p.NetAddress { return mp.addr }
func (mp *Peer) RemoteAddr() net.Addr { return &net.TCPAddr{IP: mp.ip, Port: 8800} }
func (mp *Peer) CloseConn() error { return nil }
func (mp *Peer) RemoteIP() net.IP { return mp.ip }
func (mp *Peer) SocketAddr() *p2p.NetAddress { return mp.addr }
func (mp *Peer) RemoteAddr() net.Addr { return &net.TCPAddr{IP: mp.ip, Port: 8800} }
func (mp *Peer) CloseConn() error { return nil }

View File

@ -23,14 +23,8 @@ func MaxNodeInfoSize() int {
// NodeInfo exposes basic info of a node
// and determines if we're compatible.
type NodeInfo interface {
nodeInfoAddress
nodeInfoTransport
}
// nodeInfoAddress exposes just the core info of a node.
type nodeInfoAddress interface {
ID() ID
NetAddress() *NetAddress
nodeInfoTransport
}
// nodeInfoTransport validates a nodeInfo and checks
@ -221,7 +215,7 @@ func (info DefaultNodeInfo) NetAddress() *NetAddress {
if err != nil {
switch err.(type) {
case ErrNetAddressLookup:
// XXX If the peer provided a host name and the lookup fails here
// XXX If the peer provided a host name and the lookup fails here
// we're out of luck.
// TODO: use a NetAddress in DefaultNodeInfo
default:

View File

@ -29,7 +29,7 @@ type Peer interface {
NodeInfo() NodeInfo // peer's info
Status() tmconn.ConnectionStatus
OriginalAddr() *NetAddress // original address for outbound peers
SocketAddr() *NetAddress // actual address of the socket
Send(byte, []byte) bool
TrySend(byte, []byte) bool
@ -46,7 +46,7 @@ type peerConn struct {
persistent bool
conn net.Conn // source connection
originalAddr *NetAddress // nil for inbound connections
socketAddr *NetAddress
// cached RemoteIP()
ip net.IP
@ -55,14 +55,14 @@ type peerConn struct {
func newPeerConn(
outbound, persistent bool,
conn net.Conn,
originalAddr *NetAddress,
socketAddr *NetAddress,
) peerConn {
return peerConn{
outbound: outbound,
persistent: persistent,
conn: conn,
originalAddr: originalAddr,
outbound: outbound,
persistent: persistent,
conn: conn,
socketAddr: socketAddr,
}
}
@ -223,13 +223,12 @@ func (p *peer) NodeInfo() NodeInfo {
return p.nodeInfo
}
// OriginalAddr returns the original address, which was used to connect with
// the peer. Returns nil for inbound peers.
func (p *peer) OriginalAddr() *NetAddress {
if p.peerConn.outbound {
return p.peerConn.originalAddr
}
return nil
// SocketAddr returns the address of the socket.
// For outbound peers, it's the address dialed (after DNS resolution).
// For inbound peers, it's the address returned by the underlying connection
// (not what's reported in the peer's NodeInfo).
func (p *peer) SocketAddr() *NetAddress {
return p.peerConn.socketAddr
}
// Status returns the peer's ConnectionStatus.

View File

@ -29,7 +29,7 @@ func (mp *mockPeer) IsPersistent() bool { return true }
func (mp *mockPeer) Get(s string) interface{} { return s }
func (mp *mockPeer) Set(string, interface{}) {}
func (mp *mockPeer) RemoteIP() net.IP { return mp.ip }
func (mp *mockPeer) OriginalAddr() *NetAddress { return nil }
func (mp *mockPeer) SocketAddr() *NetAddress { return nil }
func (mp *mockPeer) RemoteAddr() net.Addr { return &net.TCPAddr{IP: mp.ip, Port: 8800} }
func (mp *mockPeer) CloseConn() error { return nil }

View File

@ -109,25 +109,27 @@ func testOutboundPeerConn(
persistent bool,
ourNodePrivKey crypto.PrivKey,
) (peerConn, error) {
var pc peerConn
conn, err := testDial(addr, config)
if err != nil {
return peerConn{}, cmn.ErrorWrap(err, "Error creating peer")
return pc, cmn.ErrorWrap(err, "Error creating peer")
}
pc, err := testPeerConn(conn, config, true, persistent, ourNodePrivKey, addr)
pc, err = testPeerConn(conn, config, true, persistent, ourNodePrivKey, addr)
if err != nil {
if cerr := conn.Close(); cerr != nil {
return peerConn{}, cmn.ErrorWrap(err, cerr.Error())
return pc, cmn.ErrorWrap(err, cerr.Error())
}
return peerConn{}, err
return pc, err
}
// ensure dialed ID matches connection ID
if addr.ID != pc.ID() {
if cerr := conn.Close(); cerr != nil {
return peerConn{}, cmn.ErrorWrap(err, cerr.Error())
return pc, cmn.ErrorWrap(err, cerr.Error())
}
return peerConn{}, ErrSwitchAuthenticationFailure{addr, pc.ID()}
return pc, ErrSwitchAuthenticationFailure{addr, pc.ID()}
}
return pc, nil

View File

@ -167,7 +167,7 @@ func (r *PEXReactor) AddPeer(p Peer) {
}
} else {
// inbound peer is its own source
addr := p.NodeInfo().NetAddress()
addr := p.SocketAddr()
src := addr
// add to book. dont RequestAddrs right away because
@ -309,7 +309,7 @@ func (r *PEXReactor) ReceiveAddrs(addrs []*p2p.NetAddress, src Peer) error {
}
r.requestsSent.Delete(id)
srcAddr := src.NodeInfo().NetAddress()
srcAddr := src.SocketAddr()
for _, netAddr := range addrs {
// Validate netAddr. Disconnect from a peer if it sends us invalid data.
if netAddr == nil {

View File

@ -96,7 +96,7 @@ func TestPEXReactorRunning(t *testing.T) {
}
addOtherNodeAddrToAddrBook := func(switchIndex, otherSwitchIndex int) {
addr := switches[otherSwitchIndex].NodeInfo().NetAddress()
addr := switches[otherSwitchIndex].NetAddress()
books[switchIndex].AddAddress(addr, addr)
}
@ -127,7 +127,7 @@ func TestPEXReactorReceive(t *testing.T) {
r.RequestAddrs(peer)
size := book.Size()
addrs := []*p2p.NetAddress{peer.NodeInfo().NetAddress()}
addrs := []*p2p.NetAddress{peer.SocketAddr()}
msg := cdc.MustMarshalBinaryBare(&pexAddrsMessage{Addrs: addrs})
r.Receive(PexChannel, peer, msg)
assert.Equal(t, size+1, book.Size())
@ -184,7 +184,7 @@ func TestPEXReactorAddrsMessageAbuse(t *testing.T) {
assert.True(t, r.requestsSent.Has(id))
assert.True(t, sw.Peers().Has(peer.ID()))
addrs := []*p2p.NetAddress{peer.NodeInfo().NetAddress()}
addrs := []*p2p.NetAddress{peer.SocketAddr()}
msg := cdc.MustMarshalBinaryBare(&pexAddrsMessage{Addrs: addrs})
// receive some addrs. should clear the request
@ -229,7 +229,7 @@ func TestCheckSeeds(t *testing.T) {
badPeerConfig = &PEXReactorConfig{
Seeds: []string{"ed3dfd27bfc4af18f67a49862f04cc100696e84d@bad.network.addr:26657",
"d824b13cb5d40fa1d8a614e089357c7eff31b670@anotherbad.network.addr:26657",
seed.NodeInfo().NetAddress().String()},
seed.NetAddress().String()},
}
peer = testCreatePeerWithConfig(dir, 2, badPeerConfig)
require.Nil(t, peer.Start())
@ -263,12 +263,13 @@ func TestConnectionSpeedForPeerReceivedFromSeed(t *testing.T) {
defer os.RemoveAll(dir) // nolint: errcheck
// 1. create peer
peer := testCreateDefaultPeer(dir, 1)
require.Nil(t, peer.Start())
defer peer.Stop()
peerSwitch := testCreateDefaultPeer(dir, 1)
require.Nil(t, peerSwitch.Start())
defer peerSwitch.Stop()
// 2. Create seed which knows about the peer
seed := testCreateSeed(dir, 2, []*p2p.NetAddress{peer.NodeInfo().NetAddress()}, []*p2p.NetAddress{peer.NodeInfo().NetAddress()})
peerAddr := peerSwitch.NetAddress()
seed := testCreateSeed(dir, 2, []*p2p.NetAddress{peerAddr}, []*p2p.NetAddress{peerAddr})
require.Nil(t, seed.Start())
defer seed.Stop()
@ -295,7 +296,7 @@ func TestPEXReactorCrawlStatus(t *testing.T) {
// Create a peer, add it to the peer set and the addrbook.
peer := p2p.CreateRandomPeer(false)
p2p.AddPeerToSwitch(pexR.Switch, peer)
addr1 := peer.NodeInfo().NetAddress()
addr1 := peer.SocketAddr()
pexR.book.AddAddress(addr1, addr1)
// Add a non-connected address to the book.
@ -359,7 +360,7 @@ func TestPEXReactorSeedModeFlushStop(t *testing.T) {
reactor := switches[0].Reactors()["pex"].(*PEXReactor)
peerID := switches[1].NodeInfo().ID()
err = switches[1].DialPeerWithAddress(switches[0].NodeInfo().NetAddress(), false)
err = switches[1].DialPeerWithAddress(switches[0].NetAddress(), false)
assert.NoError(t, err)
// sleep up to a second while waiting for the peer to send us a message.
@ -397,7 +398,7 @@ func TestPEXReactorDoesNotAddPrivatePeersToAddrBook(t *testing.T) {
pexR.RequestAddrs(peer)
size := book.Size()
addrs := []*p2p.NetAddress{peer.NodeInfo().NetAddress()}
addrs := []*p2p.NetAddress{peer.SocketAddr()}
msg := cdc.MustMarshalBinaryBare(&pexAddrsMessage{Addrs: addrs})
pexR.Receive(PexChannel, peer, msg)
assert.Equal(t, size, book.Size())
@ -414,7 +415,7 @@ func TestPEXReactorDialPeer(t *testing.T) {
sw.SetAddrBook(book)
peer := mock.NewPeer(nil)
addr := peer.NodeInfo().NetAddress()
addr := peer.SocketAddr()
assert.Equal(t, 0, pexR.AttemptsToDial(addr))
@ -547,7 +548,7 @@ func testCreateSeed(dir string, id int, knownAddrs, srcAddrs []*p2p.NetAddress)
// Starting and stopping the peer is left to the caller
func testCreatePeerWithSeed(dir string, id int, seed *p2p.Switch) *p2p.Switch {
conf := &PEXReactorConfig{
Seeds: []string{seed.NodeInfo().NetAddress().String()},
Seeds: []string{seed.NetAddress().String()},
}
return testCreatePeerWithConfig(dir, id, conf)
}

View File

@ -86,6 +86,12 @@ type Switch struct {
metrics *Metrics
}
// NetAddress returns the address the switch is listening on.
func (sw *Switch) NetAddress() *NetAddress {
addr := sw.transport.NetAddress()
return &addr
}
// SwitchOption sets an optional parameter on the Switch.
type SwitchOption func(*Switch)
@ -289,13 +295,7 @@ func (sw *Switch) StopPeerForError(peer Peer, reason interface{}) {
sw.stopAndRemovePeer(peer, reason)
if peer.IsPersistent() {
addr := peer.OriginalAddr()
if addr == nil {
// FIXME: persistent peers can't be inbound right now.
// self-reported address for inbound persistent peers
addr = peer.NodeInfo().NetAddress()
}
go sw.reconnectToPeer(addr)
go sw.reconnectToPeer(peer.SocketAddr())
}
}
@ -383,7 +383,7 @@ func (sw *Switch) SetAddrBook(addrBook AddrBook) {
// like contributed to consensus.
func (sw *Switch) MarkPeerAsGood(peer Peer) {
if sw.addrBook != nil {
sw.addrBook.MarkGood(peer.NodeInfo().NetAddress())
sw.addrBook.MarkGood(peer.SocketAddr())
}
}
@ -400,7 +400,7 @@ func (sw *Switch) DialPeersAsync(addrBook AddrBook, peers []string, persistent b
sw.Logger.Error("Error in peer's address", "err", err)
}
ourAddr := sw.nodeInfo.NetAddress()
ourAddr := sw.NetAddress()
// TODO: this code feels like it's in the wrong place.
// The integration tests depend on the addrBook being saved
@ -536,7 +536,7 @@ func (sw *Switch) acceptRoutine() {
if in >= sw.config.MaxNumInboundPeers {
sw.Logger.Info(
"Ignoring inbound connection: already have enough inbound peers",
"address", p.NodeInfo().NetAddress().String(),
"address", p.SocketAddr(),
"have", in,
"max", sw.config.MaxNumInboundPeers,
)
@ -653,7 +653,7 @@ func (sw *Switch) addPeer(p Peer) error {
return err
}
p.SetLogger(sw.Logger.With("peer", p.NodeInfo().NetAddress()))
p.SetLogger(sw.Logger.With("peer", p.SocketAddr()))
// Handle the shut down case where the switch has stopped but we're
// concurrently trying to add a peer.

View File

@ -161,10 +161,6 @@ func assertMsgReceivedWithTimeout(t *testing.T, msgBytes []byte, channel byte, r
func TestSwitchFiltersOutItself(t *testing.T) {
s1 := MakeSwitch(cfg, 1, "127.0.0.1", "123.123.123", initSwitchFunc)
// addr := s1.NodeInfo().NetAddress()
// // add ourselves like we do in node.go#427
// s1.addrBook.AddOurAddress(addr)
// simulate s1 having a public IP by creating a remote peer with the same ID
rp := &remotePeer{PrivKey: s1.nodeKey.PrivKey, Config: cfg}
@ -498,7 +494,7 @@ func TestSwitchAcceptRoutine(t *testing.T) {
rp := &remotePeer{PrivKey: ed25519.GenPrivKey(), Config: cfg}
remotePeers = append(remotePeers, rp)
rp.Start()
c, err := rp.Dial(sw.NodeInfo().NetAddress())
c, err := rp.Dial(sw.NetAddress())
require.NoError(t, err)
// spawn a reading routine to prevent connection from closing
go func(c net.Conn) {
@ -517,7 +513,7 @@ func TestSwitchAcceptRoutine(t *testing.T) {
// 2. check we close new connections if we already have MaxNumInboundPeers peers
rp := &remotePeer{PrivKey: ed25519.GenPrivKey(), Config: cfg}
rp.Start()
conn, err := rp.Dial(sw.NodeInfo().NetAddress())
conn, err := rp.Dial(sw.NetAddress())
require.NoError(t, err)
// check conn is closed
one := make([]byte, 1)
@ -537,6 +533,10 @@ type errorTransport struct {
acceptErr error
}
func (et errorTransport) NetAddress() NetAddress {
panic("not implemented")
}
func (et errorTransport) Accept(c peerConfig) (Peer, error) {
return nil, et.acceptErr
}

View File

@ -35,7 +35,8 @@ func CreateRandomPeer(outbound bool) *peer {
addr, netAddr := CreateRoutableAddr()
p := &peer{
peerConn: peerConn{
outbound: outbound,
outbound: outbound,
socketAddr: netAddr,
},
nodeInfo: mockNodeInfo{netAddr},
mconn: &conn.MConnection{},
@ -174,10 +175,15 @@ func MakeSwitch(
PrivKey: ed25519.GenPrivKey(),
}
nodeInfo := testNodeInfo(nodeKey.ID(), fmt.Sprintf("node%d", i))
addr, err := NewNetAddressString(
IDAddressString(nodeKey.ID(), nodeInfo.(DefaultNodeInfo).ListenAddr),
)
if err != nil {
panic(err)
}
t := NewMultiplexTransport(nodeInfo, nodeKey, MConnConfig(cfg))
addr := nodeInfo.NetAddress()
if err := t.Listen(*addr); err != nil {
panic(err)
}
@ -214,7 +220,7 @@ func testPeerConn(
cfg *config.P2PConfig,
outbound, persistent bool,
ourNodePrivKey crypto.PrivKey,
originalAddr *NetAddress,
socketAddr *NetAddress,
) (pc peerConn, err error) {
conn := rawConn
@ -231,12 +237,7 @@ func testPeerConn(
}
// Only the information we already have
return peerConn{
outbound: outbound,
persistent: persistent,
conn: conn,
originalAddr: originalAddr,
}, nil
return newPeerConn(outbound, persistent, conn, socketAddr), nil
}
//----------------------------------------------------------------

View File

@ -24,6 +24,7 @@ type IPResolver interface {
// accept is the container to carry the upgraded connection and NodeInfo from an
// asynchronously running routine to the Accept method.
type accept struct {
netAddr *NetAddress
conn net.Conn
nodeInfo NodeInfo
err error
@ -47,6 +48,9 @@ type peerConfig struct {
// the transport. Each transport is also responsible to filter establishing
// peers specific to its domain.
type Transport interface {
// Listening address.
NetAddress() NetAddress
// Accept returns a newly connected Peer.
Accept(peerConfig) (Peer, error)
@ -115,6 +119,7 @@ func MultiplexTransportResolver(resolver IPResolver) MultiplexTransportOption {
// MultiplexTransport accepts and dials tcp connections and upgrades them to
// multiplexed peers.
type MultiplexTransport struct {
netAddr NetAddress
listener net.Listener
acceptc chan accept
@ -161,6 +166,11 @@ func NewMultiplexTransport(
}
}
// NetAddress implements Transport.
func (mt *MultiplexTransport) NetAddress() NetAddress {
return mt.netAddr
}
// Accept implements Transport.
func (mt *MultiplexTransport) Accept(cfg peerConfig) (Peer, error) {
select {
@ -173,7 +183,7 @@ func (mt *MultiplexTransport) Accept(cfg peerConfig) (Peer, error) {
cfg.outbound = false
return mt.wrapPeer(a.conn, a.nodeInfo, cfg, nil), nil
return mt.wrapPeer(a.conn, a.nodeInfo, cfg, a.netAddr), nil
case <-mt.closec:
return nil, ErrTransportClosed{}
}
@ -224,6 +234,7 @@ func (mt *MultiplexTransport) Listen(addr NetAddress) error {
return err
}
mt.netAddr = addr
mt.listener = ln
go mt.acceptPeers()
@ -258,15 +269,21 @@ func (mt *MultiplexTransport) acceptPeers() {
var (
nodeInfo NodeInfo
secretConn *conn.SecretConnection
netAddr *NetAddress
)
err := mt.filterConn(c)
if err == nil {
secretConn, nodeInfo, err = mt.upgrade(c, nil)
if err == nil {
addr := c.RemoteAddr()
id := PubKeyToID(secretConn.RemotePubKey())
netAddr = NewNetAddress(id, addr)
}
}
select {
case mt.acceptc <- accept{secretConn, nodeInfo, err}:
case mt.acceptc <- accept{netAddr, secretConn, nodeInfo, err}:
// Make the upgraded peer available.
case <-mt.closec:
// Give up if the transport was closed.
@ -426,14 +443,14 @@ func (mt *MultiplexTransport) wrapPeer(
c net.Conn,
ni NodeInfo,
cfg peerConfig,
dialedAddr *NetAddress,
socketAddr *NetAddress,
) Peer {
peerConn := newPeerConn(
cfg.outbound,
cfg.persistent,
c,
dialedAddr,
socketAddr,
)
p := newPeer(

View File

@ -8,6 +8,8 @@ import (
"testing"
"time"
"github.com/stretchr/testify/require"
"github.com/tendermint/tendermint/crypto/ed25519"
"github.com/tendermint/tendermint/p2p/conn"
)
@ -142,43 +144,23 @@ func TestTransportMultiplexConnFilterTimeout(t *testing.T) {
func TestTransportMultiplexAcceptMultiple(t *testing.T) {
mt := testSetupMultiplexTransport(t)
id, addr := mt.nodeKey.ID(), mt.listener.Addr().String()
laddr, err := NewNetAddressStringWithOptionalID(IDAddressString(id, addr))
require.NoError(t, err)
var (
seed = rand.New(rand.NewSource(time.Now().UnixNano()))
errc = make(chan error, seed.Intn(64)+64)
seed = rand.New(rand.NewSource(time.Now().UnixNano()))
nDialers = seed.Intn(64) + 64
errc = make(chan error, nDialers)
)
// Setup dialers.
for i := 0; i < cap(errc); i++ {
go func() {
var (
pv = ed25519.GenPrivKey()
dialer = newMultiplexTransport(
testNodeInfo(PubKeyToID(pv.PubKey()), defaultNodeName),
NodeKey{
PrivKey: pv,
},
)
)
addr, err := NewNetAddressStringWithOptionalID(IDAddressString(mt.nodeKey.ID(), mt.listener.Addr().String()))
if err != nil {
errc <- err
return
}
_, err = dialer.Dial(*addr, peerConfig{})
if err != nil {
errc <- err
return
}
// Signal that the connection was established.
errc <- nil
}()
for i := 0; i < nDialers; i++ {
go testDialer(*laddr, errc)
}
// Catch connection errors.
for i := 0; i < cap(errc); i++ {
for i := 0; i < nDialers; i++ {
if err := <-errc; err != nil {
t.Fatal(err)
}
@ -216,6 +198,27 @@ func TestTransportMultiplexAcceptMultiple(t *testing.T) {
}
}
func testDialer(dialAddr NetAddress, errc chan error) {
var (
pv = ed25519.GenPrivKey()
dialer = newMultiplexTransport(
testNodeInfo(PubKeyToID(pv.PubKey()), defaultNodeName),
NodeKey{
PrivKey: pv,
},
)
)
_, err := dialer.Dial(dialAddr, peerConfig{})
if err != nil {
errc <- err
return
}
// Signal that the connection was established.
errc <- nil
}
func TestTransportMultiplexAcceptNonBlocking(t *testing.T) {
mt := testSetupMultiplexTransport(t)
@ -591,6 +594,7 @@ func TestTransportHandshake(t *testing.T) {
}
}
// create listener
func testSetupMultiplexTransport(t *testing.T) *MultiplexTransport {
var (
pv = ed25519.GenPrivKey()

View File

@ -218,7 +218,7 @@ func DumpConsensusState(ctx *rpctypes.Context) (*ctypes.ResultDumpConsensusState
}
peerStates[i] = ctypes.PeerStateInfo{
// Peer basic info.
NodeAddress: peer.NodeInfo().NetAddress().String(),
NodeAddress: peer.SocketAddr().String(),
// Peer consensus state.
PeerState: peerStateJSON,
}