From b375afac66b1cb9b859a6c0d279b7602ee92e57d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Kripalani?= Date: Thu, 21 May 2020 11:52:04 +0100 Subject: [PATCH 1/2] fix: call InterceptSecured earlier; drop conn. --- transport.go | 63 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 22 deletions(-) diff --git a/transport.go b/transport.go index c7902b3..929e384 100644 --- a/transport.go +++ b/transport.go @@ -2,27 +2,31 @@ package libp2pquic import ( "context" + "crypto/x509" "errors" "fmt" "io" "net" - "github.com/libp2p/go-libp2p-core/connmgr" - n "github.com/libp2p/go-libp2p-core/network" - - "github.com/minio/sha256-simd" "golang.org/x/crypto/hkdf" - logging "github.com/ipfs/go-log" + "github.com/libp2p/go-libp2p-core/connmgr" ic "github.com/libp2p/go-libp2p-core/crypto" + "github.com/libp2p/go-libp2p-core/network" "github.com/libp2p/go-libp2p-core/peer" "github.com/libp2p/go-libp2p-core/pnet" tpt "github.com/libp2p/go-libp2p-core/transport" + p2ptls "github.com/libp2p/go-libp2p-tls" - quic "github.com/lucas-clemente/quic-go" + ma "github.com/multiformats/go-multiaddr" mafmt "github.com/multiformats/go-multiaddr-fmt" manet "github.com/multiformats/go-multiaddr-net" + + logging "github.com/ipfs/go-log" + + "github.com/lucas-clemente/quic-go" + "github.com/minio/sha256-simd" ) var log = logging.Logger("quic-transport") @@ -141,28 +145,55 @@ func NewTransport(key ic.PrivKey, psk pnet.PSK, gater connmgr.ConnectionGater) ( // Dial dials a new QUIC connection func (t *transport) Dial(ctx context.Context, raddr ma.Multiaddr, p peer.ID) (tpt.CapableConn, error) { - network, host, err := manet.DialArgs(raddr) + netwrk, host, err := manet.DialArgs(raddr) if err != nil { return nil, err } - addr, err := net.ResolveUDPAddr(network, host) + + addr, err := net.ResolveUDPAddr(netwrk, host) if err != nil { return nil, err } + remoteMultiaddr, err := toQuicMultiaddr(addr) if err != nil { return nil, err } - tlsConf, keyCh := t.identity.ConfigForPeer(p) - pconn, err := t.connManager.Dial(network, addr) + + pconn, err := t.connManager.Dial(netwrk, addr) if err != nil { return nil, err } + + localMultiaddr, err := toQuicMultiaddr(pconn.LocalAddr()) + if err != nil { + pconn.DecreaseCount() + return nil, err + } + + connaddrs := &connAddrs{lmAddr: localMultiaddr, rmAddr: remoteMultiaddr} + + tlsConf, keyCh := t.identity.ConfigForPeer(p) + vpc := tlsConf.VerifyPeerCertificate + tlsConf.VerifyPeerCertificate = func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error { + // delegate to the original peer certificate verification function. + if err := vpc(rawCerts, verifiedChains); err != nil { + return err + } + + if t.gater != nil && !t.gater.InterceptSecured(network.DirOutbound, p, connaddrs) { + return fmt.Errorf("gater denied secured connection to peer: %s", p.Pretty()) + } + return nil + } + + // now perform the QUIC dial. sess, err := quic.DialContext(ctx, pconn, addr, host, tlsConf, t.clientConfig) if err != nil { pconn.DecreaseCount() return nil, err } + // Should be ready by this point, don't block. var remotePubKey ic.PubKey select { @@ -178,18 +209,6 @@ func (t *transport) Dial(ctx context.Context, raddr ma.Multiaddr, p peer.ID) (tp pconn.DecreaseCount() }() - localMultiaddr, err := toQuicMultiaddr(pconn.LocalAddr()) - if err != nil { - pconn.DecreaseCount() - return nil, err - } - - connaddrs := &connAddrs{lmAddr: localMultiaddr, rmAddr: remoteMultiaddr} - if t.gater != nil && !t.gater.InterceptSecured(n.DirOutbound, p, connaddrs) { - pconn.DecreaseCount() - return nil, fmt.Errorf("secured connection gated") - } - return &conn{ sess: sess, transport: t, From 86aa71292b82b3f33192d7199855ec32e43e70ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Kripalani?= Date: Thu, 21 May 2020 18:39:16 +0100 Subject: [PATCH 2/2] fix a potential nil-pointer panic + add benchmark. --- conn_bench_test.go | 47 ++++++++++++++++++++++++++++++++++++++++++++++ conn_test.go | 17 ++++++++--------- filtered_conn.go | 3 +-- 3 files changed, 56 insertions(+), 11 deletions(-) create mode 100644 conn_bench_test.go diff --git a/conn_bench_test.go b/conn_bench_test.go new file mode 100644 index 0000000..df52b91 --- /dev/null +++ b/conn_bench_test.go @@ -0,0 +1,47 @@ +package libp2pquic + +import ( + "context" + "crypto/rand" + "testing" + + ic "github.com/libp2p/go-libp2p-core/crypto" + "github.com/libp2p/go-libp2p-core/peer" + ma "github.com/multiformats/go-multiaddr" +) + +func BenchmarkInterceptSecured(b *testing.B) { + priv1, _, _ := ic.GenerateEd25519Key(rand.Reader) + priv2, _, _ := ic.GenerateEd25519Key(rand.Reader) + + id2, _ := peer.IDFromPrivateKey(priv2) + gater := &mockGater{blockedPeer: id2, blockAccept: false} + + t1, _ := NewTransport(priv1, nil, gater) + t2, _ := NewTransport(priv2, nil, nil) + + laddr, _ := ma.NewMultiaddr("/ip4/127.0.0.1/udp/0/quic") + l, _ := t2.Listen(laddr) + defer l.Close() + + go func() { + for { + conn, err := l.Accept() + if err != nil { + return + } + _ = conn.Close() + } + }() + + raddr := l.Multiaddr() + + b.ResetTimer() + b.ReportAllocs() + for i := 0; i < b.N; i++ { + _, err := t1.Dial(context.Background(), raddr, id2) + if err == nil { + b.Fatal("expected error") + } + } +} diff --git a/conn_test.go b/conn_test.go index 7847c28..fb4aee5 100644 --- a/conn_test.go +++ b/conn_test.go @@ -20,22 +20,20 @@ import ( quicproxy "github.com/lucas-clemente/quic-go/integrationtests/tools/proxy" ma "github.com/multiformats/go-multiaddr" - manet "github.com/multiformats/go-multiaddr-net" - . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) type mockGater struct { lk sync.Mutex - acceptAll bool + blockAccept bool blockedPeer peer.ID } -func (c *mockGater) InterceptAccept(addrs network.ConnMultiaddrs) bool { +func (c *mockGater) InterceptAccept(_ network.ConnMultiaddrs) (allow bool) { c.lk.Lock() defer c.lk.Unlock() - return c.acceptAll || !manet.IsIPLoopback(addrs.RemoteMultiaddr()) + return !c.blockAccept } func (c *mockGater) InterceptPeerDial(p peer.ID) (allow bool) { @@ -199,10 +197,10 @@ var _ = Describe("Connection", func() { Eventually(done).Should(BeClosed()) }) - It("gates accepted connections", func() { + It("gates allows/denies connections on accept", func() { testMA, err := ma.NewMultiaddr("/ip4/127.0.0.1/udp/1234/quic") Expect(err).ToNot(HaveOccurred()) - cg := &mockGater{} + cg := &mockGater{blockAccept: true} Expect(cg.InterceptAccept(&connAddrs{rmAddr: testMA})).To(BeFalse()) serverTransport, err := NewTransport(serverKey, nil, cg) @@ -222,8 +220,9 @@ var _ = Describe("Connection", func() { // now allow the address and make sure the connection goes through clientTransport.(*transport).clientConfig.HandshakeTimeout = 2 * time.Second cg.lk.Lock() - cg.acceptAll = true + cg.blockAccept = false cg.lk.Unlock() + conn, err := clientTransport.Dial(context.Background(), ln.Multiaddr(), serverID) Expect(err).ToNot(HaveOccurred()) conn.Close() @@ -235,7 +234,7 @@ var _ = Describe("Connection", func() { ln := runServer(serverTransport, "/ip4/127.0.0.1/udp/0/quic") defer ln.Close() - cg := &mockGater{acceptAll: true, blockedPeer: serverID} + cg := &mockGater{blockedPeer: serverID} clientTransport, err := NewTransport(clientKey, nil, cg) Expect(err).ToNot(HaveOccurred()) diff --git a/filtered_conn.go b/filtered_conn.go index c5f046e..c99fbc7 100644 --- a/filtered_conn.go +++ b/filtered_conn.go @@ -50,8 +50,7 @@ func (c *filteredConn) ReadFrom(b []byte) (n int, addr net.Addr, rerr error) { } connAddrs := &connAddrs{lmAddr: c.lmAddr, rmAddr: rmAddr} - - if c.gater.InterceptAccept(connAddrs) { + if c.gater != nil && c.gater.InterceptAccept(connAddrs) { return } }