Repository: pion/turn (go)

Issue:
Relay socket should not listen on 0.0.0.0
### Your environment.
  - Version: v1.2.0
  - Browser: n/a

### What did you do?
This bug has been introduced since we removed ipv4.FlagDst.
Turn protocol listen ports are now using "specified" IP addresses, but relay ports have not been taken care of.

### What did you expect?
Obviously, if listen addr is 0.0.0.0, permission search would fail because the permission hash is based on "specified" (non-any-addr) address.

### What happened?
Permission check was always failing because the IP address did not match the incoming packets source IP.

Fixes are incoming!

Pull request:
Use specified IP for relay socket
Fixes #69

See #69 for details.

I have added AddRelayIPAddr() method also. If this method is not used, then it will default to listening IPs.

#### Limitation/TODO:
You may add multiple relay IPs, but this fix only uses the first relay IP address. This limitation will need to be fixed by the time when we allow IPv6 or other transport protocols.


Patch:
diff --git a/internal/allocation/allocation_manager.go b/internal/allocation/allocation_manager.go
index 598fc97..5509b51 100644
--- a/internal/allocation/allocation_manager.go
+++ b/internal/allocation/allocation_manager.go
@@ -59,7 +59,13 @@ func (m *Manager) Close() error {
 }
 
 // CreateAllocation creates a new allocation and starts relaying
-func (m *Manager) CreateAllocation(fiveTuple *FiveTuple, turnSocket net.PacketConn, requestedPort int, lifetime time.Duration) (*Allocation, error) {
+func (m *Manager) CreateAllocation(
+	fiveTuple *FiveTuple,
+	turnSocket net.PacketConn,
+	relayIP net.IP, // nolint:interfacer
+	requestedPort int,
+	lifetime time.Duration) (*Allocation, error) {
+
 	if fiveTuple == nil {
 		return nil, errors.Errorf("Allocations must not be created with nil FivTuple")
 	}
@@ -82,11 +88,14 @@ func (m *Manager) CreateAllocation(fiveTuple *FiveTuple, turnSocket net.PacketCo
 	a := NewAllocation(turnSocket, fiveTuple, m.log)
 
 	network := "udp4"
-	conn, err := m.net.ListenPacket(network, fmt.Sprintf("0.0.0.0:%d", requestedPort))
+	relayAddr := fmt.Sprintf("%s:%d", relayIP.String(), requestedPort)
+	conn, err := m.net.ListenPacket(network, relayAddr)
 	if err != nil {
 		return nil, err
 	}
 
+	m.log.Debugf("listening on relay addr: %s", conn.LocalAddr().String())
+
 	a.RelaySocket = conn
 	a.RelayAddr = conn.LocalAddr()
 
diff --git a/server.go b/server.go
index 1e7c1e4..2d7d130 100644
--- a/server.go
+++ b/server.go
@@ -52,6 +52,7 @@ type Server struct {
 	lock               sync.RWMutex
 	listeners          []*listener
 	listenIPs          []net.IP
+	relayIPs           []net.IP
 	listenPort         int
 	realm              string
 	authHandler        AuthHandler
@@ -100,6 +101,8 @@ func NewServer(config *ServerConfig) *Server {
 }
 
 // AddListeningIPAddr adds a listening IP address.
+// If not specified, it will automatically assigns the listening IP addresses
+// from the system.
 // This method must be called before calling Start().
 func (s *Server) AddListeningIPAddr(addrStr string) error {
 	ip := net.ParseIP(addrStr)
@@ -114,13 +117,37 @@ func (s *Server) AddListeningIPAddr(addrStr string) error {
 	return nil
 }
 
+// AddRelayIPAddr adds a listening IP address.
+// Note: current implementation can have only one relay IP address.
+// If not specified, it will automatically assigns the relay IP addresses
+// from the system.
+// This method must be called before calling Start().
+func (s *Server) AddRelayIPAddr(addrStr string) error {
+	ip := net.ParseIP(addrStr)
+	if ip.To4() == nil {
+		return fmt.Errorf("Non-IPv4 address is not supported")
+	}
+
+	if ip.IsLinkLocalUnicast() {
+		return fmt.Errorf("link-local unicast address is not allowed")
+	}
+
+	if ip.IsUnspecified() {
+		return fmt.Errorf("unspecified IP is not allowed")
+	}
+	s.relayIPs = append(s.relayIPs, ip)
+	return nil
+}
+
 // caller must hold the mutex
-func (s *Server) gatherSystemIPAddrs() error {
+func (s *Server) gatherSystemIPAddrs() ([]net.IP, error) {
 	s.log.Debug("gathering local IP address...")
 
+	var ips []net.IP
+
 	ifs, err := s.net.Interfaces()
 	if err != nil {
-		return err
+		return nil, err
 	}
 	for _, ifc := range ifs {
 		if ifc.Flags&net.FlagUp == 0 {
@@ -146,7 +173,7 @@ func (s *Server) gatherSystemIPAddrs() error {
 			}
 
 			if ip == nil {
-				return fmt.Errorf("invalid IP address: %s", addr.String())
+				return nil, fmt.Errorf("invalid IP address: %s", addr.String())
 			}
 
 			if ip.To4() == nil {
@@ -158,11 +185,11 @@ func (s *Server) gatherSystemIPAddrs() error {
 			}
 
 			s.log.Debugf("- found local IP: %s", ip.String())
-			s.listenIPs = append(s.listenIPs, ip)
+			ips = append(ips, ip)
 		}
 	}
 
-	return nil
+	return ips, nil
 }
 
 // Listen starts listening and handling TURN traffic
@@ -209,16 +236,24 @@ func (s *Server) Start() error {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
-	// If s.listenIPs is empty, gather system IPs
+	var ips []net.IP
+
 	if len(s.listenIPs) == 0 {
-		err := s.gatherSystemIPAddrs()
+		var err error
+		ips, err = s.gatherSystemIPAddrs()
 		if err != nil {
 			return err
 		}
-
-		if len(s.listenIPs) == 0 {
+		if len(ips) == 0 {
 			return fmt.Errorf("no local IP address found")
 		}
+
+		s.listenIPs = ips
+	}
+
+	// If s.relayIPs is empty, use s.listenIPs
+	if len(s.relayIPs) == 0 {
+		s.relayIPs = s.listenIPs
 	}
 
 	for _, localIP := range s.listenIPs {
diff --git a/turn.go b/turn.go
index 48593d6..3e8c7d3 100644
--- a/turn.go
+++ b/turn.go
@@ -212,7 +212,11 @@ func (s *Server) handleAllocateRequest(conn net.PacketConn, srcAddr net.Addr, m
 	// Check current usage vs redis usage of other servers
 	// if bad, redirect { stun.AttrErrorCode, 300 }
 	lifetimeDuration := allocationLifeTime(m)
-	a, err := s.manager.CreateAllocation(fiveTuple, conn, requestedPort, lifetimeDuration)
+	a, err := s.manager.CreateAllocation(
+		fiveTuple, conn,
+		s.relayIPs[0], // TODO: allow more than one relay IP
+		requestedPort,
+		lifetimeDuration)
 	if err != nil {
 		return respondWithError(err, messageIntegrity, stun.CodeInsufficientCapacity)
 	}


Test patch:
diff --git a/internal/allocation/allocation_manager_test.go b/internal/allocation/allocation_manager_test.go
index c460110..d343bcd 100644
--- a/internal/allocation/allocation_manager_test.go
+++ b/internal/allocation/allocation_manager_test.go
@@ -48,13 +48,13 @@ func TestManager(t *testing.T) {
 // test invalid Allocation creations
 func subTestCreateInvalidAllocation(t *testing.T, turnSocket ipnet.PacketConn) {
 	m := newTestManager()
-	if a, err := m.CreateAllocation(nil, turnSocket, 0, turn.DefaultLifetime); a != nil || err == nil {
+	if a, err := m.CreateAllocation(nil, turnSocket, net.IPv4zero, 0, turn.DefaultLifetime); a != nil || err == nil {
 		t.Errorf("Illegally created allocation with nil FiveTuple")
 	}
-	if a, err := m.CreateAllocation(randomFiveTuple(), nil, 0, turn.DefaultLifetime); a != nil || err == nil {
+	if a, err := m.CreateAllocation(randomFiveTuple(), nil, net.IPv4zero, 0, turn.DefaultLifetime); a != nil || err == nil {
 		t.Errorf("Illegally created allocation with nil turnSocket")
 	}
-	if a, err := m.CreateAllocation(randomFiveTuple(), turnSocket, 0, 0); a != nil || err == nil {
+	if a, err := m.CreateAllocation(randomFiveTuple(), turnSocket, net.IPv4zero, 0, 0); a != nil || err == nil {
 		t.Errorf("Illegally created allocation with 0 lifetime")
 	}
 }
@@ -63,7 +63,7 @@ func subTestCreateInvalidAllocation(t *testing.T, turnSocket ipnet.PacketConn) {
 func subTestCreateAllocation(t *testing.T, turnSocket ipnet.PacketConn) {
 	m := newTestManager()
 	fiveTuple := randomFiveTuple()
-	if a, err := m.CreateAllocation(fiveTuple, turnSocket, 0, turn.DefaultLifetime); a == nil || err != nil {
+	if a, err := m.CreateAllocation(fiveTuple, turnSocket, net.IPv4zero, 0, turn.DefaultLifetime); a == nil || err != nil {
 		t.Errorf("Failed to create allocation %v %v", a, err)
 	}
 
@@ -76,11 +76,11 @@ func subTestCreateAllocation(t *testing.T, turnSocket ipnet.PacketConn) {
 func subTestCreateAllocationDuplicateFiveTuple(t *testing.T, turnSocket ipnet.PacketConn) {
 	m := newTestManager()
 	fiveTuple := randomFiveTuple()
-	if a, err := m.CreateAllocation(fiveTuple, turnSocket, 0, turn.DefaultLifetime); a == nil || err != nil {
+	if a, err := m.CreateAllocation(fiveTuple, turnSocket, net.IPv4zero, 0, turn.DefaultLifetime); a == nil || err != nil {
 		t.Errorf("Failed to create allocation %v %v", a, err)
 	}
 
-	if a, err := m.CreateAllocation(fiveTuple, turnSocket, 0, turn.DefaultLifetime); a != nil || err == nil {
+	if a, err := m.CreateAllocation(fiveTuple, turnSocket, net.IPv4zero, 0, turn.DefaultLifetime); a != nil || err == nil {
 		t.Errorf("Was able to create allocation with same FiveTuple twice")
 	}
 }
@@ -88,7 +88,7 @@ func subTestCreateAllocationDuplicateFiveTuple(t *testing.T, turnSocket ipnet.Pa
 func subTestDeleteAllocation(t *testing.T, turnSocket ipnet.PacketConn) {
 	m := newTestManager()
 	fiveTuple := randomFiveTuple()
-	if a, err := m.CreateAllocation(fiveTuple, turnSocket, 0, turn.DefaultLifetime); a == nil || err != nil {
+	if a, err := m.CreateAllocation(fiveTuple, turnSocket, net.IPv4zero, 0, turn.DefaultLifetime); a == nil || err != nil {
 		t.Errorf("Failed to create allocation %v %v", a, err)
 	}
 
@@ -111,7 +111,7 @@ func subTestAllocationTimeout(t *testing.T, turnSocket ipnet.PacketConn) {
 	for index := range allocations {
 		fiveTuple := randomFiveTuple()
 
-		a, err := m.CreateAllocation(fiveTuple, turnSocket, 0, lifetime)
+		a, err := m.CreateAllocation(fiveTuple, turnSocket, net.IPv4zero, 0, lifetime)
 		if err != nil {
 			t.Errorf("Failed to create allocation with %v", fiveTuple)
 		}
@@ -133,9 +133,9 @@ func subTestManagerClose(t *testing.T, turnSocket ipnet.PacketConn) {
 	m := newTestManager()
 	allocations := make([]*Allocation, 2)
 
-	a1, _ := m.CreateAllocation(randomFiveTuple(), turnSocket, 0, time.Second)
+	a1, _ := m.CreateAllocation(randomFiveTuple(), turnSocket, net.IPv4zero, 0, time.Second)
 	allocations[0] = a1
-	a2, _ := m.CreateAllocation(randomFiveTuple(), turnSocket, 0, time.Minute)
+	a2, _ := m.CreateAllocation(randomFiveTuple(), turnSocket, net.IPv4zero, 0, time.Minute)
 	allocations[1] = a2
 
 	// make a1 timeout
diff --git a/server_test.go b/server_test.go
index 2366008..e265f26 100644
--- a/server_test.go
+++ b/server_test.go
@@ -14,9 +14,10 @@ func TestServer(t *testing.T) {
 	loggerFactory := logging.NewDefaultLoggerFactory()
 	log := loggerFactory.NewLogger("test")
 
+	credMap := map[string]string{}
+	credMap["user"] = "pass"
+
 	t.Run("simple", func(t *testing.T) {
-		credMap := map[string]string{}
-		credMap["user"] = "pass"
 
 		server := NewServer(&ServerConfig{
 			AuthHandler: func(username string, srcAddr net.Addr) (password string, ok bool) {
@@ -34,16 +35,9 @@ func TestServer(t *testing.T) {
 		err := server.AddListeningIPAddr("127.0.0.1")
 		assert.NoError(t, err, "should succeed")
 
-		doneCh := make(chan struct{})
-
-		go func() {
-			log.Debug("start listening...")
-			err2 := server.Start()
-			if err2 != nil {
-				t.Logf("Start returned with err: %v", err2)
-			}
-			close(doneCh)
-		}()
+		log.Debug("start listening...")
+		err = server.Start()
+		assert.NoError(t, err, "should succeed")
 
 		// make sure the server is listening before running
 		// the client.
@@ -68,7 +62,78 @@ func TestServer(t *testing.T) {
 		// Close server
 		err = server.Close()
 		assert.NoError(t, err, "should succeed")
-		log.Debug("wainting the server to terminate...")
-		<-doneCh
+	})
+
+	t.Run("Relay IPs default to the listening IPs", func(t *testing.T) {
+		server := NewServer(&ServerConfig{
+			AuthHandler: func(username string, srcAddr net.Addr) (password string, ok bool) {
+				if pw, ok := credMap[username]; ok {
+					return pw, true
+				}
+				return "", false
+			},
+			Realm:         "pion.ly",
+			LoggerFactory: loggerFactory,
+		})
+
+		assert.Equal(t, turn.DefaultLifetime, server.channelBindTimeout, "should match")
+
+		err := server.AddListeningIPAddr("127.0.0.1")
+		assert.NoError(t, err, "should succeed")
+
+		log.Debug("start listening...")
+		err = server.Start()
+		assert.NoError(t, err, "should succeed")
+
+		// make sure the server is listening before running
+		// the client.
+		time.Sleep(100 * time.Microsecond)
+
+		assert.Equal(t, 1, len(server.relayIPs), "should match")
+		assert.True(t, server.relayIPs[0].Equal(net.IPv4(127, 0, 0, 1)), "should match")
+
+		// Close server
+		err = server.Close()
+		assert.NoError(t, err, "should succeed")
+	})
+
+	t.Run("AddRelayIPAddr", func(t *testing.T) {
+		server := NewServer(&ServerConfig{
+			AuthHandler: func(username string, srcAddr net.Addr) (password string, ok bool) {
+				if pw, ok := credMap[username]; ok {
+					return pw, true
+				}
+				return "", false
+			},
+			Realm:         "pion.ly",
+			LoggerFactory: loggerFactory,
+		})
+
+		assert.Equal(t, turn.DefaultLifetime, server.channelBindTimeout, "should match")
+
+		err := server.AddListeningIPAddr("127.0.0.1")
+		assert.NoError(t, err, "should succeed")
+
+		err = server.AddRelayIPAddr("127.0.0.2")
+		assert.NoError(t, err, "should succeed")
+
+		err = server.AddRelayIPAddr("127.0.0.3")
+		assert.NoError(t, err, "should succeed")
+
+		log.Debug("start listening...")
+		err = server.Start()
+		assert.NoError(t, err, "should succeed")
+
+		// make sure the server is listening before running
+		// the client.
+		time.Sleep(100 * time.Microsecond)
+
+		assert.Equal(t, 2, len(server.relayIPs), "should match")
+		assert.True(t, server.relayIPs[0].Equal(net.IPv4(127, 0, 0, 2)), "should match")
+		assert.True(t, server.relayIPs[1].Equal(net.IPv4(127, 0, 0, 3)), "should match")
+
+		// Close server
+		err = server.Close()
+		assert.NoError(t, err, "should succeed")
 	})
 }