Add server side ICE consent checks to detect silent WebRTC disconnection #1332

ibc · 2024-02-15T18:33:06Z

Fixes Support ICE Consent Freshness (RFC7675) #1127

Details

Implements ICE Lite server side of RFC 7675 "(STUN) Usage for Consent Freshness".
This PR provides mediasoup with the ability to detect downed networks or silent/abrupt WebRTC disconnections in client side.
mediasoup now monitor received ICE consent requests from WebRTC clients and moves to 'disconnected' state (and notify the app as usual) if no ICE consent request is received in the last 30 seconds as per spec (can be changed with a iceConsentTimeout setting in WebRtcTransportOptions).

TODO

Document iceConsentTimeout it in the website and extend the description of the "icestatechange" with state "disconnected" (also document that "icestatechange" with state "disconnected" happens after "dtlsstatechange" with state "closed" if client closes its PeerConnection).

- Fixes #1127 ### Details - This PR provides the app with the ability to detect downed networks or silent/abrupts disconnections in client side. - Make mediasoup send ICE consent requests every ~5 seconds once ICE is established. - Move to 'disconnected' state (and notify the app as usual) if no ICE response is received in 30 seconds as per spec (this may change with a setting, see TODO below). - This feature requires providing the mediasoup `WebRtcTransport` with the remote ICE username fragment and ICE password, which will be done by latest mediasoup-client once released. ### TODO - [ ] Add a `iceConsentTimeout` parameter to `WebRtcTransportOptions` to override the default value of 30 seconds (0 means "disable the mechanism"). - [ ] Rust. - [ ] React to 403 ICE error responses by also moving to 'disconnected' state.

- Add iceConsentTimeout in WebRtcTransportOptions (default to 30). - Limit max entries in the queue to 20.

jmillan

Overall it looks good to me.

The only concern I have is that the application will receive a 'disconnected' event, but it has no means to know the reason why, and that's IMO important.

I think a 'reason' argument should be added to the notification from C++ to Node/Rust so in this case it would be something like ConsentCheckExpired and the corresponding ones in other cases.

That could be done in a different PR though.

worker/src/RTC/IceServer.cpp

worker/include/RTC/IceServer.hpp

ibc · 2024-02-16T10:19:15Z

The only concern I have is that the application will receive a 'disconnected' event, but it has no means to know the reason why, and that's IMO important.

I think a 'reason' argument should be added to the notification from C++ to Node/Rust so in this case it would be something like ConsentCheckExpired and the corresponding ones in other cases.

reason argument where? This is what we have now:

this.safeEmit('icestatechange', iceState);

You mean adding a reason argument to that event in case iceState === 'disconnected'?

jmillan · 2024-02-16T10:29:32Z

You mean adding a reason argument to that event in case iceState === 'disconnected'?

Yes, for example.

The problem is not knowing the reason of the disconnected now that it can be due to this checks timeout.

ibc · 2024-02-16T10:42:54Z

The problem is not knowing the reason of the disconnected now that it can be due to this checks timeout.

NOTE: if the WebRTC client disconnects its side by closing the PeerConnection or by closing the browser tab, DTLS close alert arrives BEFORE to mediasoup and "dtlsstatechange" (state "closed") happens BEFORE. So if that happens the app should already close the transport. Basically "iceconnectionstatechange" event with state "disconnected" ONLY happens if consent timeout happens or if the client is connected via TCP and the TCP is abruptly closed, which in both cases means that the network is down.

Taking this into account I think that we don't need any "reason" argument.

ibc · 2024-02-16T10:44:37Z

Also note that, before this feature, "iceconnectionstatechange" with state "disconnected" ONLY may happen if the client was connected via TCP and it was abruptly closed without sending DTLS close first. So I strongly think we don't need any "reason" argument.

jmillan · 2024-02-16T11:09:11Z

Ok, we'll see if there is a need when troubleshooting. Not a blocker anyhow.

NOTE: Not working yet due to FBS issues.

ibc · 2024-02-16T17:06:38Z

This is done, Rust included.

ibc · 2024-02-18T16:55:53Z

Hell, demo crashed:

2024-02-18T13:55:03.784Z mediasoup-demo-server:WARN:Room WebRtcTransport "icestatechange" event [iceState:disconnected], closing peer

2024-02-18T13:55:03.793Z mediasoup-demo-server:WARN:Room WebRtcTransport "icestatechange" event [iceState:disconnected], closing peer

2024-02-18T13:55:04.824Z mediasoup:ERROR:Channel Producer Channel error: Error: read ECONNRESET
2024-02-18T13:55:04.825Z mediasoup:ERROR:Worker worker process died unexpectedly [pid:1580729, code:null, signal:SIGSEGV]
2024-02-18T13:55:04.876Z mediasoup-demo-server:ERROR mediasoup Worker died, exiting  in 2 seconds... [pid:1580729]
2024-02-18T13:55:04.877Z mediasoup-demo-server:WARN:Room _createConsumer() | failed:InvalidStateError: Channel closed, pending request aborted [method:CONSUMER_RESUME,

nazar-pc · 2024-02-18T17:38:01Z

Is it just me or everyone else is also fed up with memory-related issues in C++?

ibc · 2024-02-19T09:26:50Z

Ok I got core dump in demo server:

Program terminated with signal SIGSEGV, Segmentation fault.

#0  0x0000558176a1618b in RTC::StunPacket::CheckAuthentication(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
#1  0x0000558176975be1 in RTC::IceServer::ProcessStunPacket(RTC::StunPacket*, RTC::TransportTuple*) ()
#2  0x0000558176a43909 in RTC::WebRtcServer::OnUdpSocketPacketReceived(RTC::UdpSocket*, unsigned char const*, unsigned long, sockaddr const*) ()
#3  0x0000558176d47515 in uv.udp_recvmmsg ()
#4  0x0000558176d47f6b in uv.udp_io ()
#5  0x0000558176d4b469 in uv.io_poll ()
#6  0x0000558176d3eb2f in uv_run ()
#7  0x00005581768f6a69 in DepLibUV::RunLoop() ()
#8  0x0000558176900f82 in Worker::Worker(Channel::ChannelSocket*) ()
#9  0x00005581768f5b04 in mediasoup_worker_run ()
#10 0x00005581768f4c4c in main ()

ibc · 2024-02-19T09:39:03Z

It's crashing in StunPacket::CheckAuthentication() method.

I did changes in that method, of course, since we used to only verify that the first part of the USERNAME attribute in the received STUN packet matches our server side ICE username fragment plus colon. But now we also have (or may have) the remote username fragment, so we match the received USERNAME attribute with localICEusernameFrag:remoteICEusernameFrag as per spec. I've verified that it works fine despite the remote provided the mediasoup WebRtcTransport with its remote ICE usernameFragment/pwd or not. So no idea what is crashing.

NOTE that in this PR we are also calling packet->CheckAuthentication() in STUN responses received from client. So we don't know if the crash is happening when receiving a STUN request or a STUN response.

ibc · 2024-02-19T10:17:03Z

I'm refactoring C++ code a bit to have better error traces.

… responses

ibc · 2024-02-19T11:46:55Z

Wow, another crash in the demo and this is NOT related to STUN...

Program terminated with signal SIGSEGV, Segmentation fault.

#0  0x0000563fbc0e96f6 in RTC::WebRtcTransport::OnRtcpDataReceived(RTC::TransportTuple*, unsigned char const*, unsigned long) ()
[Current thread is 1 (Thread 0x7fb43c53e440 (LWP 1643202))]
(gdb) bt
#0  0x0000563fbc0e96f6 in RTC::WebRtcTransport::OnRtcpDataReceived(RTC::TransportTuple*, unsigned char const*, unsigned long) ()
#1  0x0000563fbc0dbc9f in RTC::WebRtcServer::OnUdpSocketPacketReceived(RTC::UdpSocket*, unsigned char const*, unsigned long, sockaddr const*) ()
#2  0x0000563fbc3df515 in uv.udp_recvmmsg ()
#3  0x0000563fbc3dff6b in uv.udp_io ()
#4  0x0000563fbc3e3469 in uv.io_poll ()
#5  0x0000563fbc3d6b2f in uv_run ()
#6  0x0000563fbbf8ea69 in DepLibUV::RunLoop() ()
#7  0x0000563fbbf98f82 in Worker::Worker(Channel::ChannelSocket*) ()
#8  0x0000563fbbf8db04 in mediasoup_worker_run ()
#9  0x0000563fbbf8cc4c in main ()

However ICE disconnection happened before:

2024-02-19T11:36:27.220Z mediasoup-demo-server:WARN:Room WebRtcTransport "icestatechange" event [iceState:disconnected], closing peer
2024-02-19T11:36:35.394Z mediasoup-demo-server:WARN:Room WebRtcTransport "icestatechange" event [iceState:disconnected], closing peer
2024-02-19T11:36:35.485Z mediasoup:ERROR:Worker worker process died unexpectedly [pid:1643202, code:null, signal:SIGSEGV]

…LL receive packets on other tuples

ibc · 2024-02-19T20:59:09Z

Ok, I've removed the sending of ICE requests from mediasoup and now it monitors received ICE request consents.

CC @ggarber @fippo

ibc · 2024-02-19T21:00:08Z

Wow, another crash in the demo and this is NOT related to STUN...

Program terminated with signal SIGSEGV, Segmentation fault.

#0  0x0000563fbc0e96f6 in RTC::WebRtcTransport::OnRtcpDataReceived(RTC::TransportTuple*, unsigned char const*, unsigned long) ()
[Current thread is 1 (Thread 0x7fb43c53e440 (LWP 1643202))]
(gdb) bt
#0  0x0000563fbc0e96f6 in RTC::WebRtcTransport::OnRtcpDataReceived(RTC::TransportTuple*, unsigned char const*, unsigned long) ()
#1  0x0000563fbc0dbc9f in RTC::WebRtcServer::OnUdpSocketPacketReceived(RTC::UdpSocket*, unsigned char const*, unsigned long, sockaddr const*) ()
#2  0x0000563fbc3df515 in uv.udp_recvmmsg ()
#3  0x0000563fbc3dff6b in uv.udp_io ()
#4  0x0000563fbc3e3469 in uv.io_poll ()
#5  0x0000563fbc3d6b2f in uv_run ()
#6  0x0000563fbbf8ea69 in DepLibUV::RunLoop() ()
#7  0x0000563fbbf98f82 in Worker::Worker(Channel::ChannelSocket*) ()
#8  0x0000563fbbf8db04 in mediasoup_worker_run ()
#9  0x0000563fbbf8cc4c in main ()

However ICE disconnection happened before:

2024-02-19T11:36:27.220Z mediasoup-demo-server:WARN:Room WebRtcTransport "icestatechange" event [iceState:disconnected], closing peer
2024-02-19T11:36:35.394Z mediasoup-demo-server:WARN:Room WebRtcTransport "icestatechange" event [iceState:disconnected], closing peer
2024-02-19T11:36:35.485Z mediasoup:ERROR:Worker worker process died unexpectedly [pid:1643202, code:null, signal:SIGSEGV]

And this has been fixed. Problem is that I was deallocating a tuple before notifying it to the listener who was then calling methods on a deallocated instance.

ibc · 2024-02-19T21:26:24Z

New stuff in FuzzerStunPacket.cpp also passing fuzzer.

ibc · 2024-02-20T09:38:57Z

I see a coredump in the demo but I strongly fail to correlate it with this PR:

Program terminated with signal SIGABRT, Aborted.
#0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
44	./nptl/pthread_kill.c: No such file or directory.
[Current thread is 1 (Thread 0x7f0774b3e440 (LWP 1695953))]
(gdb) bt
#0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
#1  0x00007f0774bcad9f in __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  0x00007f0774b7bf32 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3  0x00007f0774b66472 in __GI_abort () at ./stdlib/abort.c:79
#4  0x000055b064e4bd6e in RTC::RTCP::FeedbackRtpTransportPacket::AddPacket(unsigned short, unsigned long, unsigned long) ()
#5  0x000055b064dd9ade in RTC::TransportCongestionControlServer::FillAndSendTransportCcFeedback() ()
#6  0x000055b0650dcf3e in uv.run_timers ()
#7  0x000055b0650e0c88 in uv_run ()
#8  0x000055b064c9a849 in DepLibUV::RunLoop() ()
#9  0x000055b064ca4d62 in Worker::Worker(Channel::ChannelSocket*) ()
#10 0x000055b064c998e4 in mediasoup_worker_run ()
#11 0x000055b064c98a2c in main ()

Hell, problem is in FeedbackRtpTransport::AddPacket(), an assertion is failing. I'll open a separate issue for this:

2024-02-20T07:08:16.181Z mediasoup:ERROR:Worker (stderr) (ABORT) RTC::RTCP::FeedbackRtpTransport::AddPacket() | failed assertion `baseSet': base not set
2024-02-20T07:08:16.569Z mediasoup:ERROR:Channel Producer Channel error: Error: read ECONNRESET
2024-02-20T07:08:16.574Z mediasoup-demo-server:WARN:Room _createDataConsumer() | failed:Error: request timeout at Timeout._onTimeout (/var/www/v3demo.mediasoup.org/node_modules/protoo-server/lib/Peer.js:156:14) at listOnTimeout (node:internal/timers:569:17) at process.processTimers (node:internal/timers:512:7)
2024-02-20T07:08:16.784Z mediasoup:ERROR:Worker worker process died unexpectedly [pid:1695953, code:null, signal:SIGABRT]
2024-02-20T07:08:16.823Z mediasoup-demo-server:ERROR mediasoup Worker died, exiting  in 2 seconds... [pid:1695953]

ggarber · 2024-02-20T10:29:44Z

And this has been fixed. Problem is that I was deallocating a tuple before notifying it to the listener who was then calling methods on a deallocated instance.

Consider not destroying/deallocating anything. Just notify the application about "disconnected" state and let the application decide what to do.

Similarly in the browser side the ICE "disconnected" state is not destroying anything and can recover if the user decides to keep the connection open after that event.

I think it would simplify the logic and avoid some of the issues you are debugging here. Feel free to ignore it.

ibc · 2024-02-20T11:17:50Z

Consider not destroying/deallocating anything. Just notify the application about "disconnected" state and let the application decide what to do.

Similarly in the browser side the ICE "disconnected" state is not destroying anything and can recover if the user decides to keep the connection open after that event.

Perhaps I was not clear but I didn't mean that. We are not deallocating the WebRtcTransport when ICE is disconnected not even when remote sends us a DTLS Close Alert. We just notify the app about those events.

The bug I mean was an internal C++ thing fixed in this commit and has nothing to do with communication with Node or Rust: 7ee1753

worker/include/RTC/IceServer.hpp

jmillan · 2024-02-20T17:17:42Z

worker/src/RTC/IceServer.cpp

-							this->oldUsernameFragment.clear();
-							this->oldPassword.clear();
-						}
+				if (IsConsentCheckSupported() && IsConsentCheckRunning())


This would suffice:

Suggested change

if (IsConsentCheckSupported() && IsConsentCheckRunning())

if (IsConsentCheckRunning())

Yes, I know. But I'd prefer to be super explicit because lot of logic in IceServer file is too sensitive and probably should be refactored in the future.

Rationale is that I want to prevent that consent check is started when not supported. It cannot happen because the StartConsentCheck() method asserts IsConsentCheckSupported() but anyway.

jmillan · 2024-02-20T17:18:32Z

worker/src/RTC/IceServer.cpp

+		MS_ASSERT(IsConsentCheckSupported(), "ICE consent check not supported");
+		MS_ASSERT(IsConsentCheckRunning(), "ICE consent check not running");


This would suffice:

Suggested change

MS_ASSERT(IsConsentCheckSupported(), "ICE consent check not supported");

MS_ASSERT(IsConsentCheckRunning(), "ICE consent check not running");

MS_ASSERT(IsConsentCheckRunning(), "ICE consent check not running");

jmillan · 2024-02-20T17:19:56Z

worker/src/RTC/IceServer.cpp

+		MS_ASSERT(IsConsentCheckSupported(), "ICE consent check not supported");
+		MS_ASSERT(IsConsentCheckRunning(), "ICE consent check not running");


This would suffice.

In general, if we the check is running, we can assume it is supported. And we it's enough to check for support only when start running.

Suggested change

MS_ASSERT(IsConsentCheckSupported(), "ICE consent check not supported");

MS_ASSERT(IsConsentCheckRunning(), "ICE consent check not running");

MS_ASSERT(IsConsentCheckRunning(), "ICE consent check not running");

# Conflicts: # CHANGELOG.md

ibc mentioned this pull request Feb 15, 2024

Support ICE Consent Freshness (RFC7675) #1127

Closed

make format

3d1f3a1

ibc requested a review from jmillan February 15, 2024 18:40

Missing things 1

43f8d1f

- Add iceConsentTimeout in WebRtcTransportOptions (default to 30). - Limit max entries in the queue to 20.

jmillan approved these changes Feb 16, 2024

View reviewed changes

worker/src/RTC/IceServer.cpp Outdated Show resolved Hide resolved

worker/src/RTC/IceServer.cpp Outdated Show resolved Hide resolved

worker/include/RTC/IceServer.hpp Outdated Show resolved Hide resolved

React on STUN 403 response + address feedback 1

53fd7ef

Use find_if and limit value of consent timeout

bba4a78

ibc mentioned this pull request Feb 16, 2024

Include local iceParameters in Transport 'connect' event versatica/mediasoup-client#292

Merged

ibc added 4 commits February 16, 2024 14:01

Make webrtcTransport.connect() also receive remote IceParameters

39e3637

NOTE: Not working yet due to FBS issues.

fix fbs usage

4172e21

fix comment

8a13f8f

Disable consent if iceConsentTimeout: 0 and do Rust

b9f1d92

ibc requested a review from nazar-pc February 16, 2024 17:05

ibc marked this pull request as ready for review February 16, 2024 17:05

rename IsConsentCheckEnabled() to IsConsentCheckSupported()

adbd7ba

ibc added 3 commits February 19, 2024 11:24

IceServer: add a separate method to process requests, indications and…

fd6043f

… responses

cosmetic

06c0b8f

cosmetic

9e4eac9

ibc added 2 commits February 19, 2024 12:25

Improve StunPacket::Dump() and CheckAuthentication()

2c95e29

Rename StunPacket::Authenticate() to SetPassword()

aeb0d05

ibc added 6 commits February 19, 2024 13:25

cosmetic

033cfec

Update FuzzerStunPacket.cpp

1c36427

Don't clear ALL tuples when ICE consent timeouts because we could STI…

89450a9

…LL receive packets on other tuples

make format

215380e

Remove all tuples when ICE consent is ended, but do it right

7ee1753

Do not send send requests. Instead check whether we receive them

8ca8895

Merge branch 'v3' into server-side-ice-consent-checks

37003e2

jmillan reviewed Feb 20, 2024

View reviewed changes

worker/include/RTC/IceServer.hpp Outdated Show resolved Hide resolved

jmillan reviewed Feb 20, 2024

View reviewed changes

jmillan approved these changes Feb 20, 2024

View reviewed changes

ibc added 2 commits February 20, 2024 18:44

Merge branch 'v3' into server-side-ice-consent-checks

add4500

# Conflicts: # CHANGELOG.md

remove unneded SentConsent struct in IceServer

d4c23af

ibc merged commit 32e83e3 into v3 Feb 20, 2024
22 checks passed

ibc deleted the server-side-ice-consent-checks branch February 20, 2024 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add server side ICE consent checks to detect silent WebRTC disconnection #1332

Add server side ICE consent checks to detect silent WebRTC disconnection #1332

ibc commented Feb 15, 2024 •

edited

Loading

jmillan left a comment

ibc commented Feb 16, 2024

jmillan commented Feb 16, 2024

ibc commented Feb 16, 2024

ibc commented Feb 16, 2024

jmillan commented Feb 16, 2024

ibc commented Feb 16, 2024

ibc commented Feb 18, 2024 •

edited

Loading

nazar-pc commented Feb 18, 2024

ibc commented Feb 19, 2024 •

edited

Loading

ibc commented Feb 19, 2024 •

edited

Loading

ibc commented Feb 19, 2024

ibc commented Feb 19, 2024

ibc commented Feb 19, 2024

ibc commented Feb 19, 2024

ibc commented Feb 19, 2024

ibc commented Feb 20, 2024

ggarber commented Feb 20, 2024 •

edited

Loading

ibc commented Feb 20, 2024

jmillan Feb 20, 2024

ibc Feb 20, 2024

ibc Feb 20, 2024

jmillan Feb 20, 2024

ibc Feb 20, 2024

jmillan Feb 20, 2024

ibc Feb 20, 2024

	if (IsConsentCheckSupported() && IsConsentCheckRunning())
	if (IsConsentCheckRunning())

		MS_ASSERT(IsConsentCheckSupported(), "ICE consent check not supported");
		MS_ASSERT(IsConsentCheckRunning(), "ICE consent check not running");

Add server side ICE consent checks to detect silent WebRTC disconnection #1332

Add server side ICE consent checks to detect silent WebRTC disconnection #1332

Conversation

ibc commented Feb 15, 2024 • edited Loading

Details

TODO

jmillan left a comment

Choose a reason for hiding this comment

ibc commented Feb 16, 2024

jmillan commented Feb 16, 2024

ibc commented Feb 16, 2024

ibc commented Feb 16, 2024

jmillan commented Feb 16, 2024

ibc commented Feb 16, 2024

ibc commented Feb 18, 2024 • edited Loading

nazar-pc commented Feb 18, 2024

ibc commented Feb 19, 2024 • edited Loading

ibc commented Feb 19, 2024 • edited Loading

ibc commented Feb 19, 2024

ibc commented Feb 19, 2024

ibc commented Feb 19, 2024

ibc commented Feb 19, 2024

ibc commented Feb 19, 2024

ibc commented Feb 20, 2024

ggarber commented Feb 20, 2024 • edited Loading

ibc commented Feb 20, 2024

jmillan Feb 20, 2024

Choose a reason for hiding this comment

ibc Feb 20, 2024

Choose a reason for hiding this comment

ibc Feb 20, 2024

Choose a reason for hiding this comment

jmillan Feb 20, 2024

Choose a reason for hiding this comment

ibc Feb 20, 2024

Choose a reason for hiding this comment

jmillan Feb 20, 2024

Choose a reason for hiding this comment

ibc Feb 20, 2024

Choose a reason for hiding this comment

ibc commented Feb 15, 2024 •

edited

Loading

ibc commented Feb 18, 2024 •

edited

Loading

ibc commented Feb 19, 2024 •

edited

Loading

ibc commented Feb 19, 2024 •

edited

Loading

ggarber commented Feb 20, 2024 •

edited

Loading