Repository: pion/turn (go)

Issue:
Error when handling datagram
### Your environment.
Windows Server 2019
Pion Turn server 2.0.2

### What did you do?
I executed "turn-server-simple.exe" to create a Turn server for a WebRTC project Im developing

### What did you expect?
To work ok. It does sometimes. Others it get an error.

### What happened?
When haviong a videocall using WebRTC, the console throws a lots of errors. For example:

turn ERROR: 2021/03/09 09:47:34 error when handling datagram: failed to handle Refresh-request from 94.73.44.131:30866: no allocation found for 94.73.44.131:30866:0.0.0.0:3478
turn ERROR: 2021/03/09 09:47:34 error when handling datagram: failed to handle Refresh-request from 94.73.44.131:30858: no allocation found for 94.73.44.131:30858:0.0.0.0:3478
turn ERROR: 2021/03/09 09:47:34 error when handling datagram: failed to handle Refresh-request from 94.73.44.131:30860: no allocation found for 94.73.44.131:30860:0.0.0.0:3478
turn ERROR: 2021/03/09 09:52:12 error when handling datagram: failed to create stun message from packet: unexpected EOF: not enough bytes to read header
turn ERROR: 2021/03/09 09:52:12 error when handling datagram: failed to create stun message from packet: unexpected EOF: not enough bytes to read header
turn ERROR: 2021/03/09 09:52:25 error when handling datagram: failed to create stun message from packet: unexpected EOF: not enough bytes to read header

Others, I get:

turn ERROR: 2021/03/09 12:44:52 error when handling datagram: failed to handle Allocate-request from 176.83.80.192:7363: relay already allocated for 5-TUPLE
turn ERROR: 2021/03/09 12:44:52 error when handling datagram: failed to handle Allocate-request from 176.83.80.192:7543: relay already allocated for 5-TUPLE
turn ERROR: 2021/03/09 12:44:52 error when handling datagram: failed to handle Allocate-request from 176.83.80.192:7370: relay already allocated for 5-TUPLE
turn ERROR: 2021/03/09 12:44:52 error when handling datagram: failed to handle Allocate-request from 176.83.80.192:7381: relay already allocated for 5-TUPLE

Sometimes the videocall works ok althought the errors; sometimes not.

Pull request description:
Fix retry allocation
Implement 'stateless stack approach' for allocation retransmit.
Some clients like firefox don't retry allocation with different
transport when 437 received as RFC required. Server side should 
process the retransmit of allocation.

#### Description

#### Reference issue
Fixes #206 

https://github.com/livekit/livekit-server/issues/156



Patch:
diff --git a/AUTHORS.txt b/AUTHORS.txt
index 8515b5c..ed79eeb 100644
--- a/AUTHORS.txt
+++ b/AUTHORS.txt
@@ -29,6 +29,7 @@ Sean DuBois <seaduboi@amazon.com>
 Sean DuBois <sean@pion.ly>
 Sean DuBois <sean@siobud.com>
 songjiayang <songjiayang@jd.com>
+ted <ted@skydio.com>
 Tom Clift <tom@clift.id.au>
 Yusuke Nakamura <yusuke1994525@gmail.com>
 Yutaka Takeda <yt0916@gmail.com>
diff --git a/internal/allocation/allocation.go b/internal/allocation/allocation.go
index 2b346f1..ac696ec 100644
--- a/internal/allocation/allocation.go
+++ b/internal/allocation/allocation.go
@@ -4,6 +4,7 @@ package allocation
 import (
 	"net"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/pion/logging"
@@ -12,6 +13,11 @@ import (
 	"github.com/pion/turn/v2/internal/proto"
 )
 
+type allocationResponse struct {
+	transactionID [stun.TransactionIDSize]byte
+	responseAttrs []stun.Setter
+}
+
 // Allocation is tied to a FiveTuple and relays traffic
 // use CreateAllocation and GetAllocation to operate
 type Allocation struct {
@@ -27,6 +33,12 @@ type Allocation struct {
 	lifetimeTimer       *time.Timer
 	closed              chan interface{}
 	log                 logging.LeveledLogger
+
+	// some clients (firefox or others using resiprocate's nICE lib) may retry allocation
+	// with same 5 tuple when received 413, for compatible with these clients,
+	// cache for response lost and client retry to implement 'stateless stack approach'
+	// https://datatracker.ietf.org/doc/html/rfc5766#section-6.2
+	responseCache atomic.Value // *allocationResponse
 }
 
 func addr2IPFingerprint(addr net.Addr) string {
@@ -164,6 +176,23 @@ func (a *Allocation) Refresh(lifetime time.Duration) {
 	}
 }
 
+// SetResponseCache cache allocation response for retransmit allocation request
+func (a *Allocation) SetResponseCache(transactionID [stun.TransactionIDSize]byte, attrs []stun.Setter) {
+	a.responseCache.Store(&allocationResponse{
+		transactionID: transactionID,
+		responseAttrs: attrs,
+	})
+}
+
+// GetResponseCache return response cache for retransmit allocation request
+func (a *Allocation) GetResponseCache() (id [stun.TransactionIDSize]byte, attrs []stun.Setter) {
+	if r := a.responseCache.Load(); r != nil {
+		res := r.(*allocationResponse)
+		id, attrs = res.transactionID, res.responseAttrs
+	}
+	return
+}
+
 // Close closes the allocation
 func (a *Allocation) Close() error {
 	select {
diff --git a/internal/server/turn.go b/internal/server/turn.go
index cbac09c..ca92f8d 100644
--- a/internal/server/turn.go
+++ b/internal/server/turn.go
@@ -39,8 +39,14 @@ func handleAllocateRequest(r Request, m *stun.Message) error {
 	//    existing allocation.  If yes, the server rejects the request with
 	//    a 437 (Allocation Mismatch) error.
 	if alloc := r.AllocationManager.GetAllocation(fiveTuple); alloc != nil {
-		msg := buildMsg(m.TransactionID, stun.NewType(stun.MethodAllocate, stun.ClassErrorResponse), &stun.ErrorCodeAttribute{Code: stun.CodeAllocMismatch})
-		return buildAndSendErr(r.Conn, r.SrcAddr, errRelayAlreadyAllocatedForFiveTuple, msg...)
+		id, attrs := alloc.GetResponseCache()
+		if id != m.TransactionID {
+			msg := buildMsg(m.TransactionID, stun.NewType(stun.MethodAllocate, stun.ClassErrorResponse), &stun.ErrorCodeAttribute{Code: stun.CodeAllocMismatch})
+			return buildAndSendErr(r.Conn, r.SrcAddr, errRelayAlreadyAllocatedForFiveTuple, msg...)
+		}
+		// a retry allocation
+		msg := buildMsg(m.TransactionID, stun.NewType(stun.MethodAllocate, stun.ClassSuccessResponse), append(attrs, messageIntegrity)...)
+		return buildAndSend(r.Conn, r.SrcAddr, msg...)
 	}
 
 	// 3. The server checks if the request contains a REQUESTED-TRANSPORT
@@ -162,6 +168,7 @@ func handleAllocateRequest(r Request, m *stun.Message) error {
 	}
 
 	msg := buildMsg(m.TransactionID, stun.NewType(stun.MethodAllocate, stun.ClassSuccessResponse), append(responseAttrs, messageIntegrity)...)
+	a.SetResponseCache(m.TransactionID, responseAttrs)
 	return buildAndSend(r.Conn, r.SrcAddr, msg...)
 }
 


Test patch:
diff --git a/internal/allocation/allocation_test.go b/internal/allocation/allocation_test.go
index 7c24958..1f1ba26 100644
--- a/internal/allocation/allocation_test.go
+++ b/internal/allocation/allocation_test.go
@@ -30,6 +30,7 @@ func TestAllocation(t *testing.T) {
 		{"Refresh", subTestAllocationRefresh},
 		{"Close", subTestAllocationClose},
 		{"packetHandler", subTestPacketHandler},
+		{"ResponseCache", subTestResponseCache},
 	}
 
 	for _, tc := range tt {
@@ -311,3 +312,18 @@ func subTestPacketHandler(t *testing.T) {
 	_ = peerListener1.Close()
 	_ = peerListener2.Close()
 }
+
+func subTestResponseCache(t *testing.T) {
+	a := NewAllocation(nil, nil, nil)
+	transactionID := [stun.TransactionIDSize]byte{1, 2, 3}
+	responseAttrs := []stun.Setter{
+		&proto.Lifetime{
+			Duration: proto.DefaultLifetime,
+		},
+	}
+	a.SetResponseCache(transactionID, responseAttrs)
+
+	cacheID, cacheAttr := a.GetResponseCache()
+	assert.Equal(t, transactionID, cacheID)
+	assert.Equal(t, responseAttrs, cacheAttr)
+}