do connection setup here #9

marten-seemann · 2017-04-17T12:07:23Z

This package now sets up new connections. A go-libp2p-conn.Conn is an encrypted, stream-multiplexed connection. If the underlying tpt.Conn is a single-stream connection, it is first encrypted (using secio) and then a stream multiplexer is used to provide multistream support. If the tpt.Conn supports transport-level stream multiplexing, the secio handshake is performed on a new stream.

This PR depends on libp2p/go-libp2p-transport#20.
There are still a couple of TODOs here, especially regarding the timeouts for dialing connections as well as the connection protector.

whyrusleeping · 2017-04-18T03:56:16Z

dial.go

-			}
-			maconn = pconn
-		}
+		// TODO: reenable the protector


@Kubuxu how would the protector work with something like QUIC?

Solved by libp2p/go-libp2p-pnet#8.

whyrusleeping · 2017-04-18T03:57:28Z

dial.go

 		}

 		cryptoProtoChoice := SecioTag
 		if !iconn.EncryptConnections || d.PrivateKey == nil {
 			cryptoProtoChoice = NoEncryptionTag
 		}

-		maconn.SetReadDeadline(time.Now().Add(NegotiateReadTimeout))
+		var stream smux.Stream
+		if ssc, ok := tptConn.(tpt.SingleStreamConn); ok {


Use a type switch here:

switch con := tptConn.(type) { case tpt.SingleStreamConn: case tpt.MultiStreamConn: default: }

Kubuxu · 2017-05-19T07:13:35Z

dial.go

 	"fmt"
 	"math/rand"
 	"strings"
 	"time"

+	smux "github.com/jbenet/go-stream-muxer"


s/jbente/libp2p/

whyrusleeping · 2017-05-31T16:29:46Z

conn.go

+		}
+		defer stream.Close()
+
+		secSession, err = setupSecureSession(ctx, local, privKey, stream)


Hrm... i'm not sure this is the right way to do this. This is encrypting on top of of the stream muxer, instead of underneath it.

I think that instead of doing it this way, we can just not support private networks and QUIC at the same time. Just error out instead i'm thinking.

cc @Kubuxu @lgierth

oh wait, this isnt the private networks protector layer. i was mistaken.

The problem here was that I wasn't using the ReadWriter provided by the secio package. I fixed that.

whyrusleeping · 2017-05-31T16:31:48Z

conn.go

+
+	var streamConn smux.Conn
+	var secSession secio.Session
+	if ssc, ok := tptConn.(tpt.SingleStreamConn); ok {


you can do a type switch here:

switch sc := tptConn.(type) { case tpt.SingleStreamConn: // sc is a SingleStreamConn in this case block case tpt.MultiStreamConn: // sc is a multistreamconn in this case block default: return errWrongType }

Kubuxu · 2017-06-26T20:33:33Z

The mixing of the secio with the multiplexed connections looks good, what we also have to do is confirm that person on the other side of the secio is using the exactly the same QUIC connection.

This is to prevent someone proxying the secio over different QUIC. This is a problem as confirming identity over secio over QUIC doesn't prove the identity of the the other side of QUIC.

whyrusleeping · 2017-07-15T20:44:44Z

conn.go

+			return nil, err
+		}
+	case tpt.MultiStreamConn:
+		// 1. secure the connection on the first best stream


I think we want to add a big TODO and WARNING here, we're only doing this this way for now, for quic, until we have the ability to simply rely on QUICs crypto to validate the peers. Its also insecure (can be man in the middled) so we should note that QUIC is experimental for now.

whyrusleeping · 2017-07-15T21:15:28Z

dial.go


-		err = msmux.SelectProtoOrFail(cryptoProtoChoice, maconn)
+		stream.SetReadDeadline(time.Now().Add(NegotiateReadTimeout))


should check the error here

(oh, i guess this wasnt checked before, ignore this comment)

whyrusleeping · 2017-07-15T21:15:40Z

dial.go

 		if err != nil {
 			errOut = err
 			return
 		}

-		maconn.SetReadDeadline(time.Time{})
+		// clear deadline
+		stream.SetReadDeadline(time.Time{})


error check again

whyrusleeping · 2017-07-15T21:30:33Z

dial.go

 		if err != nil {
 			errOut = err
 			return
 		}

 		if d.Protector != nil {
-			pconn, err := d.Protector.Protect(maconn)
+			var pconn tpt.Conn
+			pconn, err = d.Protector.Protect(tptConn)


So in this case, if using a 'protector', we end up just stream multiplexing over a single QUIC conn, am i reading that correctly?

If that is the case, then we might just want to (for now) disable the use of QUIC and private networks at the same time.

The protector should work fine.
tptConn is the general tpt.Conn. pnet.Protect does another type assertion in https://github.com/marten-seemann/go-libp2p-pnet/blob/11a8850893ddc8d571abe57b5082d0110b77d412/protector.go#L33-L42. For MultiStreamConns, it wraps OpenStream and AcceptStream and uses the PSK with a separate nonce for every stream (see https://github.com/marten-seemann/go-libp2p-pnet/blob/11a8850893ddc8d571abe57b5082d0110b77d412/psk_multistreamconn.go#L22-L36).

whyrusleeping · 2017-07-15T21:40:07Z

So to move forward here, I think we want to disable private networks and QUIC being used together, as well as adding a few more comments about the security implications of the secio peerID verification we're doing.

Also, to make future integration easier, I think we should use the servers peer private key as the QUIC cert, even though this doesnt buy us any security right now, it will help make the interfaces clearer for when we are able to rely on tls to do the client<->server verification (aka, when we can trust quics crypto to do the peer handshake for us).

marten-seemann · 2017-07-19T07:27:38Z

I think there is an easy (although not very aesthetic) solution for the MITM vulnerability for QUIC connections (I justed pushed that one). The QUIC vulnerability arises because secio only secures the stream the handshake is performed on. All other streams are unencrypted. A MITM would just forward the secio stream and then intercept the data streams.
What we can do here is use the Protector to encrypt the data streams. The PSK needed to do that could is generated by server (it doesn't matter who does that) and sent as the first thing on the secio-secured stream.

I'm really looking forward to get rid of all those hacks. I recently started with the TLS integration in quic-go, but it will probably take me some time to get that production-ready.

@whyrusleeping: I don't understand your command about the certs. I guess you mean something that we establish a CA to sign the server's private key and use that cert as the cert for the QUIC connection?

whyrusleeping · 2017-10-18T22:00:56Z

listen.go

+					defer stream.Close()
+				}
+
+				if _, _, err := l.mux.Negotiate(stream); err != nil {


Definitely unrelated to this PR, but could we check that the protocol chosen here is secio, and error out otherwise? It would be helpful if nodes on older code don't behave weirdly when we start supporting tls1.3

whyrusleeping · 2017-10-18T22:02:38Z

@marten-seemann i'm fine moving forward with parts of this PR, if we can do the MultiplexConn stuff in a followup PR, i think we can get the rest in much more quickly.

marten-seemann · 2017-10-19T00:44:39Z

I removed the MultiplexConn type assertions (and everything related with that), so we could get the rest merged (this will also require merging another change to go-libp2p-transport).

Stebalien

Nice. I have a few comments that I can fix myself depending on your timing but this is mostly good to go.

Stebalien · 2017-10-26T01:36:00Z

dial.go

 		}

 		cryptoProtoChoice := SecioTag
 		if !iconn.EncryptConnections || d.PrivateKey == nil {
 			cryptoProtoChoice = NoEncryptionTag
 		}

-		maconn.SetReadDeadline(time.Now().Add(NegotiateReadTimeout))


Shouldn't we still be setting this (or, more generally, a timeout on the entire connection)? Otherwise, we could wait here forever. Yes, we'll move on elsewhere but we'll never free these resources.

Maybe change this to the dial timeout (and set both the read and write deadlines)/raw

This should be taken care of by the context. When the context times out, the rawConn is closed, causing the Read to return immediately.

Ah, good point.

Stebalien · 2017-10-26T18:36:04Z

conn.go

+		c = &secureConn{
+			insecure: tptConn,
+			secure:   secSession,
+		}


If remote is not "", we should check if remote == secSession.RemoteID() here. That way, we do this before doing anything else. Currently, we check in swarm but, IMO, that's too late. Note: this is an improvement, not a bug.

Can we do that in a separate PR?

Fair enough.

Stebalien · 2017-10-26T18:44:48Z

listen.go

-		if l.privk == nil || !iconn.EncryptConnections {
-			log.Warning("listener %s listening INSECURELY!", l)
-			return c, nil
+	for c := range l.incoming {


Don't need a loop. We had one for the IsTemporary error check but that now happens lower down.

This can be simplified to:

c := <- l.incoming return c.conn, c.err

Stebalien · 2017-10-26T18:46:20Z

secure_conn.go

 	secio "github.com/libp2p/go-libp2p-secio"
 	tpt "github.com/libp2p/go-libp2p-transport"
 	ma "github.com/multiformats/go-multiaddr"
 )

-// secureConn wraps another Conn object with an encrypted channel.


Comment to be reverted.

Stebalien · 2017-10-26T18:51:13Z

listen.go

-			if l.protec != nil {
-				pc, err := l.protec.Protect(maconn)
+
+			ctx, cancel := context.WithTimeout(l.ctx, ConnAcceptTimeout)


We should consider setting a ConnAcceptTimeout deadline on the connection itself to avoid leaking go routines.

As with dialing, the connection will be closed when this expires so ignore this.

Stebalien · 2017-10-26T18:52:47Z

Also, I'll go ahead and update the secio dep version.

Stebalien · 2017-10-27T05:33:49Z

listen.go

 	}
-	return nil, fmt.Errorf("listener is closed")
+
+	c := <-l.incoming


Actually, I was wrong. We'll also need to check if the channel is closed (c, ok := <-l.incoming...).

Right. Fixed.

Stebalien · 2017-10-27T05:53:24Z

So, apparently this still does multiplexing setup and needs libp2p/go-libp2p-interface-conn#1 (which we can't merge as it depends on libp2p/go-libp2p-transport#20 which we reverted).

Stebalien · 2017-11-03T21:29:51Z

listen.go

+				}
+
+				// If we have a wrapper func, wrap this conn
+				if l.wrapper != nil {


This swaps the order of wrapping and protecting. Was this a bug?

Yes, I believe it was. We wrap/protect in this order when dialing.

Stebalien · 2017-11-03T21:36:15Z

listen.go

+			}()
+
+			select {
+			case <-ctx.Done():


There's a race here. We can return the connection and then close it.

I pushed a fix for that.

Stebalien · 2017-11-03T21:53:37Z

listen.go

+				}
+
+				if _, _, err := l.mux.Negotiate(conn); err != nil {
+					log.Warning("incoming conn: negotiation of crypto protocol failed: ", err)


Wrong error.

Actually, what exactly is this doing? You didn't change this...

Ah. This is negotiating security (we overload mux way too much). Nevermind.

marten-seemann · 2017-11-06T06:33:31Z

@Stebalien: Is there anything else I need to fix here?

Stebalien · 2017-11-06T17:15:31Z

The only thing blocking this is #9 (comment). I've been working on a new PR with the secio/async stuff so we can move that forward (I can hand it off to you if you want, I'm hitting some odd test failures around timeouts that you've probably already figured out). I'm also working on the transport interfaces problems but it doesn't look like it's going to be quite as simple as I thought... (which is why I'd like to just get the async accept stuff merged first).

Port of async connection accepting from #9

Stebalien · 2017-11-07T06:54:44Z

That is, #18

Port of async connection accepting from #9 We need this to: 1. Perform the secio handshake immediately 2. Use a context as the dial timeout (to do this, we need to do the secio handshake immediately).

whyrusleeping reviewed Apr 18, 2017

View reviewed changes

marten-seemann mentioned this pull request Apr 18, 2017

remove connection setup libp2p/go-peerstream#2

Closed

marten-seemann force-pushed the master branch from ac781c4 to 25db3bc Compare May 19, 2017 03:34

Kubuxu reviewed May 19, 2017

View reviewed changes

dial.go Outdated

"fmt"

"math/rand"

"strings"

"time"

smux "github.com/jbenet/go-stream-muxer"

Copy link

Member

Kubuxu May 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/jbente/libp2p/

marten-seemann force-pushed the master branch from 25db3bc to 5ef7c48 Compare May 26, 2017 09:03

whyrusleeping reviewed May 31, 2017

View reviewed changes

marten-seemann force-pushed the master branch from d462a4d to d984492 Compare June 4, 2017 14:49

marten-seemann force-pushed the master branch from 8c3c9d3 to f1fb07e Compare July 3, 2017 21:00

marten-seemann force-pushed the master branch from f1fb07e to fdd302f Compare July 14, 2017 08:49

whyrusleeping reviewed Jul 15, 2017

View reviewed changes

marten-seemann force-pushed the master branch from fdd302f to 9379de6 Compare July 18, 2017 08:29

marten-seemann force-pushed the master branch from ee5f432 to be80072 Compare July 20, 2017 04:43

marten-seemann force-pushed the master branch 5 times, most recently from 425c27d to 603b821 Compare August 14, 2017 04:49

marten-seemann force-pushed the master branch 5 times, most recently from d006af1 to 17df6b5 Compare August 16, 2017 06:56

marten-seemann force-pushed the master branch from 49d8cf6 to dff3ff6 Compare September 12, 2017 08:47

marten-seemann mentioned this pull request Oct 7, 2017

immediately perform the handshake when setting up the secure session libp2p/go-libp2p-secio#16

Merged

whyrusleeping added the ready label Oct 17, 2017

whyrusleeping reviewed Oct 18, 2017

View reviewed changes

marten-seemann force-pushed the master branch from aa5f361 to 463aed2 Compare October 20, 2017 07:46

whyrusleeping added in progress and removed ready labels Oct 20, 2017

Stebalien suggested changes Oct 26, 2017

View reviewed changes

ghost assigned Stebalien Oct 26, 2017

remove the MultiplexConn type assertions

b2606fa

marten-seemann force-pushed the master branch from ad3a802 to ee6eb04 Compare October 27, 2017 05:16

Stebalien reviewed Oct 27, 2017

View reviewed changes

simplify accepting new connections

1820ac9

marten-seemann force-pushed the master branch from ee6eb04 to 1820ac9 Compare October 27, 2017 06:22

Stebalien reviewed Nov 3, 2017

View reviewed changes

fix race condition when accepting connections

8e5f941

Stebalien mentioned this pull request Nov 7, 2017

asynchronous accept, synchronous secio #18

Merged

Stebalien added a commit that referenced this pull request Nov 7, 2017

WIP async accept

3da564f

Port of async connection accepting from #9

Stebalien closed this Jun 8, 2018

ghost removed the in progress label Jun 8, 2018


		err = msmux.SelectProtoOrFail(cryptoProtoChoice, maconn)
		stream.SetReadDeadline(time.Now().Add(NegotiateReadTimeout))

do connection setup here #9

do connection setup here #9

Conversation

marten-seemann commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Jul 15, 2017

marten-seemann commented Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

whyrusleeping commented Oct 18, 2017

marten-seemann commented Oct 19, 2017

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann commented Nov 6, 2017

Stebalien commented Nov 6, 2017 • edited Loading

Stebalien commented Nov 7, 2017

marten-seemann commented Jul 19, 2017 •

edited

Loading

Stebalien commented Nov 6, 2017 •

edited

Loading