Repository: pion/ice
Pull request #424 (2022-02-20 14:19:02)

Make TCPMux IPv4/IPv6 aware
TCPMux before would create one internal connection per ufrag. This could
cause remote IPv6 traffic to be dispatched to a local IPv4 handler (or
the inverse). The ice.Agent would then discard the traffic since a
candidate pair must be the same IP version.

This commit now creates two connections per ufrag. When requesting a
connection for a ufrag the user must specify if they want IPv4 or IPv6.

Relates to pion/webrtc#2125
Relates to pion/webrtc#1356


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/ice/pull/424?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#424](https://codecov.io/gh/pion/ice/pull/424?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (da0bbe4) into [master](https://codecov.io/gh/pion/ice/commit/7723b468a604075d3a51884478f5bba4fc708c43?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (7723b46) will **decrease** coverage by `52.77%`.
> The diff coverage is `44.18%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/ice/pull/424/graphs/tree.svg?width=650&height=150&src=pr&token=1ivIyHDzva&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)](https://codecov.io/gh/pion/ice/pull/424?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

```diff
@@             Coverage Diff             @@
##           master     #424       +/-   ##
===========================================
- Coverage   79.39%   26.62%   -52.78%     
===========================================
  Files          33       33               
  Lines        3650     3673       +23     
===========================================
- Hits         2898      978     -1920     
- Misses        587     2662     +2075     
+ Partials      165       33      -132     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `26.62% <44.18%> (-0.01%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/ice/pull/424?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [gather.go](https://codecov.io/gh/pion/ice/pull/424/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Z2F0aGVyLmdv) | `0.00% <0.00%> (-66.67%)` | :arrow_down: |
| [tcp\_mux.go](https://codecov.io/gh/pion/ice/pull/424/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-dGNwX211eC5nbw==) | `63.72% <45.23%> (-11.97%)` | :arrow_down: |
| [agent\_stats.go](https://codecov.io/gh/pion/ice/pull/424/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-YWdlbnRfc3RhdHMuZ28=) | `0.00% <0.00%> (-84.38%)` | :arrow_down: |
| [agent\_config.go](https://codecov.io/gh/pion/ice/pull/424/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-YWdlbnRfY29uZmlnLmdv) | `0.00% <0.00%> (-82.15%)` | :arrow_down: |
| [selection.go](https://codecov.io/gh/pion/ice/pull/424/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c2VsZWN0aW9uLmdv) | `0.00% <0.00%> (-82.08%)` | :arrow_down: |
| [udp\_mux.go](https://codecov.io/gh/pion/ice/pull/424/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-dWRwX211eC5nbw==) | `0.00% <0.00%> (-81.98%)` | :arrow_down: |
| [agent.go](https://codecov.io/gh/pion/ice/pull/424/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-YWdlbnQuZ28=) | `0.00% <0.00%> (-81.21%)` | :arrow_down: |
| [candidate\_peer\_reflexive.go](https://codecov.io/gh/pion/ice/pull/424/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y2FuZGlkYXRlX3BlZXJfcmVmbGV4aXZlLmdv) | `0.00% <0.00%> (-80.65%)` | :arrow_down: |
| [transport.go](https://codecov.io/gh/pion/ice/pull/424/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-dHJhbnNwb3J0Lmdv) | `0.00% <0.00%> (-79.73%)` | :arrow_down: |
| [stun.go](https://codecov.io/gh/pion/ice/pull/424/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c3R1bi5nbw==) | `0.00% <0.00%> (-75.00%)` | :arrow_down: |
| ... and [13 more](https://codecov.io/gh/pion/ice/pull/424/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/ice/pull/424?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/ice/pull/424?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Last update [7723b46...da0bbe4](https://codecov.io/gh/pion/ice/pull/424?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

Sean-Der: @jech Hopefully this fixes the issues you are seeing! Sorry it took so long to understand/reproduce the issue.
jech: Thanks a lot, Sean, I'll test it during the night (when nobody is using the sample server).

FWIW, I've seen similar symptoms with UDPMux, see https://github.com/pion/webrtc/issues/1915.  I'm wondering if it's the same issue?
jech: Perhaps I'm doing something wrong, but it still doesn't work for me.  I've put a TCP listener on port 8444, then did the following on the client to block all UDP and TCP TURN:
```
iptables -I INPUT -s 51.210.14.2 -p udp -j DROP
ip6tables -I INPUT -p udp -j DROP
iptables -I INPUT -p tcp --sport 1194 -j DROP
ip6tables -I INPUT -p tcp --sport 1194 -j DROP
```
The client is the offerer and the media sender.  It connects to the TCP listener, and sends some data:
```
16:17:43.149235 IP 192.168.0.27.43936 > 51.210.14.2.8444: Flags [SEW], seq 1304065160, win 64240, options [mss 1460,sackOK,TS val 3863861007 ecr 0,nop,wscale 7], length 0
16:17:43.156764 IP 51.210.14.2.8444 > 192.168.0.27.43936: Flags [S.E], seq 24426563, ack 1304065161, win 65160, options [mss 1460,sackOK,TS val 4145864461 ecr 3863861007,nop,wscale 7], length 0
16:17:43.156800 IP 192.168.0.27.43936 > 51.210.14.2.8444: Flags [.], ack 1, win 502, options [nop,nop,TS val 3863861015 ecr 4145864461], length 0
```
It's sending some data:
```
\16:17:44.168249 IP 192.168.0.27.43936 > 51.210.14.2.8444: Flags [P.], seq 115:229, ack 1, win 502, options [nop,nop,TS val 3863862026 ecr 4145864799], length 114
16:17:44.175367 IP 51.210.14.2.8444 > 192.168.0.27.43936: Flags [.], ack 229, win 509, options [nop,nop,TS val 4145865479 ecr 3863862026], length 0
```
However, I see no traffic in the opposite direction.  After a while, the connection drops:
```
16:18:28.705050 IP 192.168.0.27.43938 > 51.210.14.2.8444: Flags [P.], seq 2509:2623, ack 1, win 502, options [nop,nop,TS val 3863906563 ecr 4145909334], length 114
16:18:28.712831 IP 51.210.14.2.8444 > 192.168.0.27.43938: Flags [.], ack 2623, win 509, options [nop,nop,TS val 4145910016 ecr 3863906563], length 0
16:18:28.753946 IP 192.168.0.27.43938 > 51.210.14.2.8444: Flags [F.], seq 2623, ack 1, win 502, options [nop,nop,TS val 3863906612 ecr 4145910016], length 0
```
At that point, ICE gets into failed state, which triggers an ICE restart, and the cycle starts again.
jech: Interestingly, I can only reproduce the issue if both TCP/IPv4 and TCP/IPv6 are unblocked.  If I aablesdditionally block TCP/IPv6 (`ip6tables -I INPUT -p tcp -j DROP`), then everything works fine.
davidzhao: > FWIW, I've seen similar symptoms with UDPMux, see [pion/webrtc#1915](https://github.com/pion/webrtc/issues/1915). I'm wondering if it's the same issue?

Fairly certain UDPMux is also not IPv6 aware. If this works, we'll port the same fix to UDPMux
jech: The behaviour is definitely better than it was: I now get an ICE failure consistently across browsers (tested with Brave and Firefox), which is something I already know how to handle gracefully, while previously I was getting erratic behaviour that I didn't know how to detect.

Hence, unless Sean has any further ideas, I recommend merging this patch while keeping the bug report open.

maeln: Hi!

Because our use-case rely on two PR that are not merge yet ( https://github.com/pion/webrtc/pull/1812 & https://github.com/pion/mediadevices/pull/338 ), it is a bit difficult to test this PR against our own issue in a timely manner.

So don't wait for my feedback. I will see with my colleague to try to organize our own fork/pr to test this ASAP.

Thanks !
Sean-Der: @jech I still have ice-tcp issues as well! With FireFox I am not able to connect when Pion is the controlled agent.

* FireFox sends a binding request with UseCandidate. Pion creates a peer reflexive candidate + candidate pair
* Pion sends a binding request (to validate the candidate)
* FireFox sends another binding request with UseCandiate. Pion hasn't gotten a response to it's binding request yet and ignores (candidate pair isn't valid yet)
* FireFox responds with Binding Success. Pion marks candidate pair as valid.

If FireFox sent another Binding Request everything would go to connected, not sure why it doesn't. I have attached the pcap here.  You can reproduce at http://18.216.219.191:8080/ it is just running https://github.com/pion/webrtc/tree/master/examples/ice-tcp

[firefox-tcp-client.pcap.zip](https://github.com/pion/ice/files/8109577/firefox-tcp-client.pcap.zip)
Sean-Der: I am going to merge this, but just makes this a `Relates to` on the two tickets. It fixes at least one problem 

Patch:
diff --git a/AUTHORS.txt b/AUTHORS.txt
index 8c8b66b..4b96858 100644
--- a/AUTHORS.txt
+++ b/AUTHORS.txt
@@ -33,6 +33,7 @@ Luke Curley <kixelated@gmail.com>
 Meelap Shah <meelapshah@gmail.com>
 Michael MacDonald <github@macdonald.cx>
 Michael MacDonald <mike.macdonald@savantsystems.com>
+Mikhail Bragin <misha@wiretrustee.com>
 Nevio Vesic <nevio@subspace.com>
 Ori Bernstein <ori@eigenstate.org>
 Robert Eperjesi <eperjesi@uber.com>
diff --git a/gather.go b/gather.go
index 8d2c40d..a1dc437 100644
--- a/gather.go
+++ b/gather.go
@@ -177,7 +177,7 @@ func (a *Agent) gatherCandidatesLocal(ctx context.Context, networkTypes []Networ
 			case tcp:
 				// Handle ICE TCP passive mode
 				a.log.Debugf("GetConn by ufrag: %s\n", a.localUfrag)
-				conn, err = a.tcpMux.GetConnByUfrag(a.localUfrag)
+				conn, err = a.tcpMux.GetConnByUfrag(a.localUfrag, mappedIP.To4() == nil)
 				if err != nil {
 					if !errors.Is(err, ErrTCPMuxNotInitialized) {
 						a.log.Warnf("error getting tcp conn by ufrag: %s %s %s\n", network, ip, a.localUfrag)
diff --git a/tcp_mux.go b/tcp_mux.go
index e5da608..86cf530 100644
--- a/tcp_mux.go
+++ b/tcp_mux.go
@@ -19,7 +19,7 @@ import (
 // 2. allow mocking in tests.
 type TCPMux interface {
 	io.Closer
-	GetConnByUfrag(ufrag string) (net.PacketConn, error)
+	GetConnByUfrag(ufrag string, isIPv6 bool) (net.PacketConn, error)
 	RemoveConnByUfrag(ufrag string)
 }
 
@@ -36,7 +36,7 @@ func (m *invalidTCPMux) Close() error {
 }
 
 // GetConnByUfrag implements TCPMux interface.
-func (m *invalidTCPMux) GetConnByUfrag(ufrag string) (net.PacketConn, error) {
+func (m *invalidTCPMux) GetConnByUfrag(ufrag string, isIPv6 bool) (net.PacketConn, error) {
 	return nil, ErrTCPMuxNotInitialized
 }
 
@@ -49,8 +49,8 @@ type TCPMuxDefault struct {
 	params *TCPMuxParams
 	closed bool
 
-	// conns is a map of all tcpPacketConns indexed by ufrag
-	conns map[string]*tcpPacketConn
+	// connsIPv4 and connsIPv6 are maps of all tcpPacketConns indexed by ufrag
+	connsIPv4, connsIPv6 map[string]*tcpPacketConn
 
 	mu sync.Mutex
 	wg sync.WaitGroup
@@ -72,7 +72,8 @@ func NewTCPMuxDefault(params TCPMuxParams) *TCPMuxDefault {
 	m := &TCPMuxDefault{
 		params: &params,
 
-		conns: map[string]*tcpPacketConn{},
+		connsIPv4: map[string]*tcpPacketConn{},
+		connsIPv6: map[string]*tcpPacketConn{},
 	}
 
 	m.wg.Add(1)
@@ -109,7 +110,7 @@ func (m *TCPMuxDefault) LocalAddr() net.Addr {
 }
 
 // GetConnByUfrag retrieves an existing or creates a new net.PacketConn.
-func (m *TCPMuxDefault) GetConnByUfrag(ufrag string) (net.PacketConn, error) {
+func (m *TCPMuxDefault) GetConnByUfrag(ufrag string, isIPv6 bool) (net.PacketConn, error) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
@@ -117,25 +118,25 @@ func (m *TCPMuxDefault) GetConnByUfrag(ufrag string) (net.PacketConn, error) {
 		return nil, io.ErrClosedPipe
 	}
 
-	conn, ok := m.conns[ufrag]
-
-	if ok {
+	if conn, ok := m.getConn(ufrag, isIPv6); ok {
 		return conn, nil
-		// return nil, fmt.Errorf("duplicate ufrag %v", ufrag)
 	}
 
-	conn = m.createConn(ufrag, m.LocalAddr())
-
-	return conn, nil
+	return m.createConn(ufrag, m.LocalAddr(), isIPv6), nil
 }
 
-func (m *TCPMuxDefault) createConn(ufrag string, localAddr net.Addr) *tcpPacketConn {
+func (m *TCPMuxDefault) createConn(ufrag string, localAddr net.Addr, isIPv6 bool) *tcpPacketConn {
 	conn := newTCPPacketConn(tcpPacketParams{
 		ReadBuffer: m.params.ReadBufferSize,
 		LocalAddr:  localAddr,
 		Logger:     m.params.Logger,
 	})
-	m.conns[ufrag] = conn
+
+	if isIPv6 {
+		m.connsIPv6[ufrag] = conn
+	} else {
+		m.connsIPv4[ufrag] = conn
+	}
 
 	m.wg.Add(1)
 	go func() {
@@ -199,9 +200,17 @@ func (m *TCPMuxDefault) handleConn(conn net.Conn) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
-	packetConn, ok := m.conns[ufrag]
+	host, _, err := net.SplitHostPort(conn.RemoteAddr().String())
+	if err != nil {
+		m.closeAndLogError(conn)
+		m.params.Logger.Warnf("Failed to get host in STUN message from %s to %s\n", conn.RemoteAddr(), conn.LocalAddr())
+		return
+	}
+
+	isIPv6 := net.ParseIP(host).To4() == nil
+	packetConn, ok := m.getConn(ufrag, isIPv6)
 	if !ok {
-		packetConn = m.createConn(ufrag, conn.LocalAddr())
+		packetConn = m.createConn(ufrag, conn.LocalAddr(), isIPv6)
 	}
 
 	if err := packetConn.AddConn(conn, buf); err != nil {
@@ -216,10 +225,15 @@ func (m *TCPMuxDefault) Close() error {
 	m.mu.Lock()
 	m.closed = true
 
-	for _, conn := range m.conns {
+	for _, conn := range m.connsIPv4 {
 		m.closeAndLogError(conn)
 	}
-	m.conns = map[string]*tcpPacketConn{}
+	for _, conn := range m.connsIPv6 {
+		m.closeAndLogError(conn)
+	}
+
+	m.connsIPv4 = map[string]*tcpPacketConn{}
+	m.connsIPv6 = map[string]*tcpPacketConn{}
 
 	err := m.params.Listener.Close()
 
@@ -235,12 +249,27 @@ func (m *TCPMuxDefault) RemoveConnByUfrag(ufrag string) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
-	if conn, ok := m.conns[ufrag]; ok {
+	if conn, ok := m.connsIPv4[ufrag]; ok {
+		m.closeAndLogError(conn)
+		delete(m.connsIPv4, ufrag)
+	}
+
+	if conn, ok := m.connsIPv6[ufrag]; ok {
 		m.closeAndLogError(conn)
-		delete(m.conns, ufrag)
+		delete(m.connsIPv6, ufrag)
 	}
 }
 
+func (m *TCPMuxDefault) getConn(ufrag string, isIPv6 bool) (val *tcpPacketConn, ok bool) {
+	if isIPv6 {
+		val, ok = m.connsIPv6[ufrag]
+	} else {
+		val, ok = m.connsIPv4[ufrag]
+	}
+
+	return
+}
+
 const streamingPacketHeaderLen = 2
 
 // readStreamingPacket reads 1 packet from stream


Test patch:
diff --git a/tcp_mux_test.go b/tcp_mux_test.go
index 8340b94..56fee6e 100644
--- a/tcp_mux_test.go
+++ b/tcp_mux_test.go
@@ -55,7 +55,7 @@ func TestTCPMux_Recv(t *testing.T) {
 	n, err := writeStreamingPacket(conn, msg.Raw)
 	require.NoError(t, err, "error writing tcp stun packet")
 
-	pktConn, err := tcpMux.GetConnByUfrag("myufrag")
+	pktConn, err := tcpMux.GetConnByUfrag("myufrag", false)
 	require.NoError(t, err, "error retrieving muxed connection for ufrag")
 	defer func() {
 		_ = pktConn.Close()
@@ -90,12 +90,12 @@ func TestTCPMux_NoDeadlockWhenClosingUnusedPacketConn(t *testing.T) {
 		ReadBufferSize: 20,
 	})
 
-	_, err = tcpMux.GetConnByUfrag("test")
+	_, err = tcpMux.GetConnByUfrag("test", false)
 	require.NoError(t, err, "error getting conn by ufrag")
 
 	require.NoError(t, tcpMux.Close(), "error closing tcpMux")
 
-	conn, err := tcpMux.GetConnByUfrag("test")
+	conn, err := tcpMux.GetConnByUfrag("test", false)
 	assert.Nil(t, conn, "should receive nil because mux is closed")
 	assert.Equal(t, io.ErrClosedPipe, err, "should receive error because mux is closed")
 }