Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

Add SecureMuxer interface to facilitate simultaneous open #53

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions event/nattype.go
@@ -0,0 +1,18 @@
package event

import "github.com/libp2p/go-libp2p-core/network"

// EvtNATDeviceTypeChanged is an event struct to be emitted when the type of the NAT device changes for a Transport Protocol.
//
// Note: This event is meaningful ONLY if the AutoNAT Reachability is Private.
// Consumers of this event should ALSO consume the `EvtLocalReachabilityChanged` event and interpret
// this event ONLY if the Reachability on the `EvtLocalReachabilityChanged` is Private.
type EvtNATDeviceTypeChanged struct {
// TransportProtocol is the Transport Protocol for which the NAT Device Type has been determined.
TransportProtocol network.NATTransportProtocol
// NatDeviceType indicates the type of the NAT Device for the Transport Protocol.
// Currently, it can be either a `Cone NAT` or a `Symmetric NAT`. Please see the detailed documentation
// on `network.NATDeviceType` enumerations for a better understanding of what these types mean and
// how they impact Connectivity and Hole Punching.
NatDeviceType network.NATDeviceType
}
20 changes: 20 additions & 0 deletions network/context.go
Expand Up @@ -12,15 +12,35 @@ var DialPeerTimeout = 60 * time.Second

type noDialCtxKey struct{}
type dialPeerTimeoutCtxKey struct{}
type forceDirectDialCtxKey struct{}

var noDial = noDialCtxKey{}
var forceDirectDial = forceDirectDialCtxKey{}

// WithNoDial constructs a new context with an option that instructs the network
// to not attempt a new dial when opening a stream.
func WithNoDial(ctx context.Context, reason string) context.Context {
return context.WithValue(ctx, noDial, reason)
}

// EXPERIMENTAL
// WithForceDirectDial constructs a new context with an option that instructs the network
// to attempt to force a direct connection to a peer via a dial even if a proxied connection to it already exists.
func WithForceDirectDial(ctx context.Context, reason string) context.Context {
return context.WithValue(ctx, forceDirectDial, reason)
}

// EXPERIMENTAL
// GetForceDirectDial returns true if the force direct dial option is set in the context.
func GetForceDirectDial(ctx context.Context) (forceDirect bool, reason string) {
v := ctx.Value(forceDirectDial)
if v != nil {
return true, v.(string)
}

return false, ""
}

// GetNoDial returns true if the no dial option is set in the context.
func GetNoDial(ctx context.Context) (nodial bool, reason string) {
v := ctx.Value(noDial)
Expand Down
58 changes: 58 additions & 0 deletions network/nattype.go
@@ -0,0 +1,58 @@
package network

// NATDeviceType indicates the type of the NAT device.
type NATDeviceType int

const (
// NATDeviceTypeUnknown indicates that the type of the NAT device is unknown.
NATDeviceTypeUnknown NATDeviceType = iota

// NATDeviceTypeCone indicates that the NAT device is a Cone NAT.
// A Cone NAT is a NAT where all outgoing connections from the same source IP address and port are mapped by the NAT device
// to the same IP address and port irrespective of the destination address.
// With regards to RFC 3489, this could be either a Full Cone NAT, a Restricted Cone NAT or a
// Port Restricted Cone NAT. However, we do NOT differentiate between them here and simply classify all such NATs as a Cone NAT.
// NAT traversal with hole punching is possible with a Cone NAT ONLY if the remote peer is ALSO behind a Cone NAT.
// If the remote peer is behind a Symmetric NAT, hole punching will fail.
NATDeviceTypeCone

// NATDeviceTypeSymmetric indicates that the NAT device is a Symmetric NAT.
// A Symmetric NAT maps outgoing connections with different destination addresses to different IP addresses and ports,
// even if they originate from the same source IP address and port.
// NAT traversal with hole-punching is currently NOT possible in libp2p with Symmetric NATs irrespective of the remote peer's NAT type.
NATDeviceTypeSymmetric
)

func (r NATDeviceType) String() string {
switch r {
case 0:
return "Unknown"
case 1:
return "Cone"
case 2:
return "Symmetric"
default:
return "unrecognized"
}
}

// NATTransportProtocol is the transport protocol for which the NAT Device Type has been determined.
type NATTransportProtocol int

const (
// NATTransportUDP means that the NAT Device Type has been determined for the UDP Protocol.
NATTransportUDP NATTransportProtocol = iota
// NATTransportTCP means that the NAT Device Type has been determined for the TCP Protocol.
NATTransportTCP
)

func (n NATTransportProtocol) String() string {
switch n {
case 0:
return "UDP"
case 1:
return "TCP"
default:
return "unrecognized"
}
}
5 changes: 5 additions & 0 deletions sec/insecure/insecure.go
Expand Up @@ -108,6 +108,11 @@ func (t *Transport) SecureOutbound(ctx context.Context, insecure net.Conn, p pee
p, conn.remote)
}

if t.key == nil && conn.remote == "" {
// set the remote peer id if we were initialiazed without keys
conn.remote = p
}

return conn, nil
}

Expand Down
14 changes: 14 additions & 0 deletions sec/security.go
Expand Up @@ -24,3 +24,17 @@ type SecureTransport interface {
// SecureOutbound secures an outbound connection.
SecureOutbound(ctx context.Context, insecure net.Conn, p peer.ID) (SecureConn, error)
}

// A SecureMuxer is a wrapper around SecureTransport which can select security protocols
// and open outbound connections with simultaneous open.
type SecureMuxer interface {
// 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)
Copy link
Member

Choose a reason for hiding this comment

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

Per our discussion, let's figure out when this can happen. We normally won't get a "simultaneous accept".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


// SecureOutbound secures an outbound connection.
// 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)
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@vyzo vyzo Sep 25, 2019

Choose a reason for hiding this comment

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

What would the error buy us? We can still secure the connection with simultaneous open.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, simultaneous connect is not an error!

Copy link
Member

@Stebalien Stebalien Sep 26, 2019

Choose a reason for hiding this comment

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

I'm just trying to prevent this TCP specific feature from leaking into the interface.

Copy link
Member

Choose a reason for hiding this comment

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

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.