pass a conn that can be type asserted to a net.UDPConn to quic-go

[marten-seemann]

Fixes #178.

This PR does two (related) things:

1. it removes the filteredConn.
2. makes sure to pass a net.PacketConn that can be interface-asserted to a net.UDPConn for quic.Listen and quic.Dial, enabling quic-go to use UDP-specific optimizations (ECN, setting of kernel buffer sizes).

We used the filteredConn to implement InterceptAccept by running that callback for every QUIC Long Header packet we read from the connection. This was a suboptimal solution to begin with:

• the callback would be called multiple times
• it leads to a connection timeout on the client side (10s)
  With this PR, we treat InterceptAccept and InterceptSecured as equal. InterceptAccept is thus called after one network round-trip (same as for TCP, where it is called after the 3-way handshake), and it already conveys an encrypted and authenticated information to the client that the connection was gated (see ConnectionGater.InterceptAccept encourages acting upon unauthenticated information go-libp2p#1003).

[Stebalien]

On one hand, it's nice to avoid the extra work by doing this early. On the other hand, we can't fully trust the source address until the second round trip anyways.

[Stebalien]

Is this not going to cause type assertion issues?

[marten-seemann]

What type assertion issues are you worried about?

This is what makes it possible to interface assert methods defined on the net.UDPConn (which is how this PR fixes #178).

[Stebalien]

Oh, I see. I thought we were doing a concrete type assertion.

If we just need the methods, this change isn't strictly speaking necessary. We could just make filteredConn embed a UDPConn. Am I missing something?

[marten-seemann]

You could do that, but you still couldn't store the filteredConn as a UDPConn (as the UDPConn is a struct, not an interface). The problem is that Go only promotes methods that are explicitly declared, see https://play.golang.org/p/1godHvE_gAJ for a minimal example.

[marten-seemann]

It was very non-obvious to me when I first encountered it, and as quic-go will introduce more optimizations that rely on the PacketConn actually being a UDPConn, I added some unit tests in 9c346e1.

[Stebalien]

You could do that, but you still couldn't store the filteredConn as a UDPConn (as the UDPConn is a struct, not an interface)

Well no, but you don't need to if you just need some methods on it:

diff --git a/filtered_conn.go b/filtered_conn.go
index c5f046e..f12baa9 100644
--- a/filtered_conn.go
+++ b/filtered_conn.go
@@ -22,24 +22,24 @@ func (c *connAddrs) RemoteMultiaddr() ma.Multiaddr {
 }
 
 type filteredConn struct {
-	net.PacketConn
+	*net.UDPConn
 
 	lmAddr ma.Multiaddr
 	gater  connmgr.ConnectionGater
 }
 
-func newFilteredConn(c net.PacketConn, gater connmgr.ConnectionGater) net.PacketConn {
+func newFilteredConn(c *net.UDPConn, gater connmgr.ConnectionGater) *filteredConn {
 	lmAddr, err := toQuicMultiaddr(c.LocalAddr())
 	if err != nil {
 		panic(err)
 	}
 
-	return &filteredConn{PacketConn: c, gater: gater, lmAddr: lmAddr}
+	return &filteredConn{UDPConn: c, gater: gater, lmAddr: lmAddr}
 }
 
 func (c *filteredConn) ReadFrom(b []byte) (n int, addr net.Addr, rerr error) {
 	for {
-		n, addr, rerr = c.PacketConn.ReadFrom(b)
+		n, addr, rerr = c.UDPConn.ReadFrom(b)
 		// Short Header packet, see https://tools.ietf.org/html/draft-ietf-quic-invariants-07#section-4.2.
 		if n < 1 || b[0]&0x80 == 0 {
 			return
diff --git a/reuse.go b/reuse.go
index d7f7f76..5c3d69f 100644
--- a/reuse.go
+++ b/reuse.go
@@ -3,6 +3,7 @@ package libp2pquic
 import (
 	"net"
 	"sync"
+	"syscall"
 	"time"
 
 	"github.com/libp2p/go-libp2p-core/connmgr"
@@ -17,18 +18,25 @@ var (
 )
 
 type reuseConn struct {
-	net.PacketConn
+	udpConn
 
 	mutex       sync.Mutex
 	refCount    int
 	unusedSince time.Time
 }
 
-func newReuseConn(conn net.PacketConn, gater connmgr.ConnectionGater) *reuseConn {
+// all the "udp" functions we need.
+type udpConn interface {
+	net.PacketConn
+	SyscallConn() (syscall.RawConn, error)
+}
+
+func newReuseConn(conn *net.UDPConn, gater connmgr.ConnectionGater) *reuseConn {
+	var wrapped udpConn = conn
 	if gater != nil {
-		conn = newFilteredConn(conn, gater)
+		wrapped = newFilteredConn(conn, gater)
 	}
-	return &reuseConn{PacketConn: conn}
+	return &reuseConn{udpConn: wrapped}
 }
 
 func (c *reuseConn) IncreaseCount() {

[marten-seemann]

Unfortunately, SyscallConn is not the only function quic-go needs. In the next version, it will also use SetReadBuffer to set kernel buffer sizes. And in the future, it might use additional functions. It feels brittle to introduce a udpConn interface here, given that there’s no way to write a test case to check that it actually contains all the functions that quic-go would like to see.

I believe that the change in this PR (only using authenticated information when making interception decisions) is worth doing on its own.

[Stebalien]

I believe that the change in this PR (only using authenticated information when making interception decisions) is worth doing on its own.

I agree. I just like to make PR motivation clear. The issue isn't that we need this change, it's that:

1. It simplifies some things.
2. We want to make gating decisions on authenticated information.