Add SecureMuxer interface to facilitate simultaneous open #53
Conversation
| // The returned boolean indicates whether the connection should be treated as a server | ||
| // connection due to simultaneous open. | ||
| SecureOutbound(ctx context.Context, insecure net.Conn, p peer.ID) (SecureConn, bool, error) | ||
| } |
There was a problem hiding this comment.
I'd like to consider using a special sentinel error instead. That is, return a connection and a special ErrSimultaniousConnect.
This way we can avoid polluting the interfaces too much.
There was a problem hiding this comment.
What would the error buy us? We can still secure the connection with simultaneous open.
There was a problem hiding this comment.
also, simultaneous connect is not an error!
There was a problem hiding this comment.
I'm just trying to prevent this TCP specific feature from leaking into the interface.
There was a problem hiding this comment.
We discussed this synchronously. I was assuming we'd be able to come up with a better interface the next time we add a transport from scratch. However, @vyzo convinced me otherwise.
The only viable alternative is to add a third Secure(ctx, insecure, p) (Conn, iAmInitiator bool, error) function. However, that has no real benefit over SecureOutbound
Well, I guess it has a slight benefit: Transports that aren't TCP could skip this whole dance. Maybe that's worth it @vyzo? Regardless, my hope to avoid messing with the interfaces seems moot.
| // SecureInbound secures an inbound connection. | ||
| // The returned boolean indicates whether the connection should be trated as a server | ||
| // connection; in the case of SecureInbound it should always be true. | ||
| SecureInbound(ctx context.Context, insecure net.Conn) (SecureConn, bool, error) |
There was a problem hiding this comment.
Per our discussion, let's figure out when this can happen. We normally won't get a "simultaneous accept".
There was a problem hiding this comment.
Well, there is symmetry in having the boolean indicator in the interface, plus it opens the door to active implementations that can deal with the situation.
There was a problem hiding this comment.
@vyzo I have the same concern as @Stebalien. Can you explain what you mean by active implementations? Is that some kind of hole punching technique you're referring to?
There was a problem hiding this comment.
I don't have anything that is doing active listen yet, but we could conceivably solve the accept-accept scenario in hole punching (that according to the literature can happen, we'll see in practice).
Other than that, it's symmetry of the interface.
There was a problem hiding this comment.
What is the accept-accept scenario? Opening a TCP socket is a purely local operation that doesn't send any packets on the wire, isn't it? So this can't possibly lead to the establishment of a connection.
There was a problem hiding this comment.
It something that can happen with hole-punching, because you are both listening and opening a connection on the same time.
So it is conceivable that the SYNs will get through the NAT/firewall and the OS routes them to the listening socket instead of the connecting socket.
It is supposed to be unlikely, but something that can happen nonetheless.
aarshkshah1992
force-pushed
the
feat/simopen
branch
from
3f9dbee
to
24d7f19
Compare
|
@Stebalien @marten-seemann @vyzo Given our goal of getting to TCP hole punching quickly, I'd like to put a hold on the discussion and use this interface changes as is because it does do a good job of solving the simultaneous connect problem in I am happy to mark the interface as experimental if we want to evolve this in the future. Please let me know if there are strong objections to this, |
Co-authored-by: Marten Seemann <[email protected]>
Co-authored-by: Marten Seemann <[email protected]>
Co-authored-by: Marten Seemann <[email protected]>
aarshkshah1992
force-pushed
the
feat/simopen
branch
from
f4c94c2
to
7116e28
Compare
|
Hey, his was subsusmed in #180 and #181 . However, I believe the "Also fixes a bug in the insecure transport that was breaking tests in go-conn-security: the remote peer ID was not set for connections with /plaintext/1.0.0." stuff hasn't made in I think. We can probably raise a separate PR just for that and get it in. |
|
yeah, let's cherry-pick that. |
For libp2p/go-libp2p#1039.
Also fixes a bug in the insecure transport that was breaking tests in go-conn-security: the remote peer ID was not set for connections with
/plaintext/1.0.0.Edit:
The two main changes this PR introduces are:
A context API to instruct libp2p Swarm to force a direct connection to the peer even if a proxied connection to it already exists. This is to implemented the Hole Punching co-ordination Spec described in relay/DCUtR: Add Direct Connection Upgrade through Relay protocol specs#173. You can see how this API is used at https://github.com/libp2p/go-libp2p/pull/711/files.
We introduce an interface that allows negotiating a security protocol between peers even if it's a simultaneous connect. This is for multistream-select 1.0: simultaneous open protocol extension specs#196.