Make TCPMux IPv4/IPv6 aware #424

Sean-Der · 2022-02-20T14:19:02Z

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 · 2022-02-20T14:19:59Z

Codecov Report

Merging #424 (5d993ad) into master (2d70ec8) will decrease coverage by 0.16%.
The diff coverage is 59.52%.

@@            Coverage Diff             @@
##           master     pion/webrtc#424      +/-   ##
==========================================
- Coverage   78.98%   78.82%   -0.17%     
==========================================
  Files          34       34              
  Lines        3860     3882      +22     
==========================================
+ Hits         3049     3060      +11     
- Misses        630      636       +6     
- Partials      181      186       +5

Flag	Coverage Δ
go	`78.82% <59.52%> (-0.17%)`	⬇️
wasm	`25.16% <42.85%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tcp_mux.go	`69.45% <58.53%> (-6.24%)`	⬇️
gather.go	`66.80% <100.00%> (ø)`
candidate_base.go	`86.06% <0.00%> (+1.21%)`	⬆️
udp_mux.go	`81.97% <0.00%> (+1.74%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d70ec8...5d993ad. Read the comment docs.

Sean-Der · 2022-02-20T14:20:17Z

@jech Hopefully this fixes the issues you are seeing! Sorry it took so long to understand/reproduce the issue.

tcp_mux.go

jech · 2022-02-20T14:54:30Z

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 #518. I'm wondering if it's the same issue?

jech · 2022-02-20T15:25:24Z

Perhaps I'm doing something wrong, but it still doesn't work for me. I've put a TCP listener on port 8444, enabled NetworkTypeTCP4 and NetworkTypeTCP6 then did the following on the client (I'm running TURN on port 1194):

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.

boks1971

lgtm!

jech · 2022-02-20T17:07:41Z

Interestingly, I can only reproduce the issue if both TCP/IPv4 and TCP/IPv6 are unblocked. If I additionally block TCP/IPv6 (ip6tables -I INPUT -p tcp -j DROP), then everything works fine.

davidzhao · 2022-02-20T19:54:47Z

FWIW, I've seen similar symptoms with UDPMux, see pion/ice#518. 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

davidzhao

lg! I don't have a repro to validate this, and will defer to @jech to confirm that it resolves the issue.

jech · 2022-02-20T20:33:57Z

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 we all already have 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 · 2022-02-21T09:10:54Z

Hi!

Because our use-case rely on two PR that are not merge yet ( pion/webrtc#1812 & pion/mediadevices#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 a lot !

Sean-Der · 2022-02-21T13:39:26Z

@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

Sean-Der · 2022-02-21T13:40:22Z

I am going to merge this, but just makes this a Relates to on the two tickets. It fixes at least one problem

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

Sean-Der requested review from jeremija, boks1971 and davidzhao February 20, 2022 14:19

jeremija reviewed Feb 20, 2022

View reviewed changes

tcp_mux.go Outdated Show resolved Hide resolved

Sean-Der force-pushed the tcpmux-ip-version-aware branch from da0bbe4 to 4c93ae8 Compare February 20, 2022 15:32

boks1971 approved these changes Feb 20, 2022

View reviewed changes

davidzhao approved these changes Feb 20, 2022

View reviewed changes

Sean-Der force-pushed the tcpmux-ip-version-aware branch from 4c93ae8 to a813428 Compare February 21, 2022 13:17

pionbot force-pushed the tcpmux-ip-version-aware branch from a813428 to 24aa30d Compare February 21, 2022 13:18

Sean-Der force-pushed the tcpmux-ip-version-aware branch from 24aa30d to bad8502 Compare February 21, 2022 13:40

pionbot force-pushed the tcpmux-ip-version-aware branch from bad8502 to 5d993ad Compare February 21, 2022 13:41

Sean-Der merged commit fb4760c into master Feb 21, 2022

Sean-Der deleted the tcpmux-ip-version-aware branch February 21, 2022 13:55

Antonito mentioned this pull request Mar 2, 2022

Makes UDPMux IPv4/IPv6 aware #431

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make TCPMux IPv4/IPv6 aware #424

Make TCPMux IPv4/IPv6 aware #424

Sean-Der commented Feb 20, 2022 •

edited

Loading

codecov bot commented Feb 20, 2022 •

edited

Loading

Sean-Der commented Feb 20, 2022

jech commented Feb 20, 2022

jech commented Feb 20, 2022 •

edited

Loading

boks1971 left a comment

jech commented Feb 20, 2022 •

edited

Loading

davidzhao commented Feb 20, 2022

davidzhao left a comment

jech commented Feb 20, 2022 •

edited

Loading

maeln commented Feb 21, 2022 •

edited

Loading

Sean-Der commented Feb 21, 2022

Sean-Der commented Feb 21, 2022

Make TCPMux IPv4/IPv6 aware #424

Make TCPMux IPv4/IPv6 aware #424

Conversation

Sean-Der commented Feb 20, 2022 • edited Loading

codecov bot commented Feb 20, 2022 • edited Loading

Codecov Report

Sean-Der commented Feb 20, 2022

jech commented Feb 20, 2022

jech commented Feb 20, 2022 • edited Loading

boks1971 left a comment

Choose a reason for hiding this comment

jech commented Feb 20, 2022 • edited Loading

davidzhao commented Feb 20, 2022

davidzhao left a comment

Choose a reason for hiding this comment

jech commented Feb 20, 2022 • edited Loading

maeln commented Feb 21, 2022 • edited Loading

Sean-Der commented Feb 21, 2022

Sean-Der commented Feb 21, 2022

Sean-Der commented Feb 20, 2022 •

edited

Loading

codecov bot commented Feb 20, 2022 •

edited

Loading

jech commented Feb 20, 2022 •

edited

Loading

jech commented Feb 20, 2022 •

edited

Loading

jech commented Feb 20, 2022 •

edited

Loading

maeln commented Feb 21, 2022 •

edited

Loading