Closed Bug 1510487 Opened 2 years ago Closed 2 years ago
DTLS without SRTP extension (for datachannel only) closes connection
10.58 KB, text/plain
6.54 KB, application/octet-stream
47 bytes, text/x-phabricator-request
|Details | Review|
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.102 Safari/537.36 Steps to reproduce: Connect to Firefox 64 datachannel session through webrtc. The remote/local SDP is attached. Actual results: The DTLS exchange failed. Specifically after the DTLS certificate message is send, the remote DTLS server connection closes. Our Native DTLS exchange that we capture in our native stack is also attached. Expected results: I expected DTLS exchange to pass with datachannels. Did something change in Firefox 64 that could effect this?(maybe in the sdp ,DTLS or SCTP). I will attach a success version of DTLS exchange when connected to chrome (or firefox 63). Our data channel native stack connects fine with firefox 63 and before. It also working fine with Chrome. The DTLS exchange works fine with Audio/Video connection in firefox 64 also.
I am assigning a component to this issue in order to involve the development team and get an opinion on this.
Component: Untriaged → WebRTC
Product: Firefox → Core
(In reply to undefined from comment #undefined) > Thanks
This is the wireShark pcapng file from the failted datachannels dtls exchange with firefox 64. 192.168.124.173 is firefox in this case.
We found that there is bug in firefox 64 where if connections with only data streams that don’t negotiate an SRTP profiles are immediately closed after the DTLS handshake completes.
Status: UNCONFIRMED → NEW
Component: WebRTC → WebRTC: Networking
Ever confirmed: true
jib, can you triage this?
Thanks stapatrick9 for the 63-64 log comparison! If this readily reproduces, would you be able to use the mozregression tool  to narrow down the regression range further? The only datachannel-related WebRTC change I see in 64 release , appears to be bug 1051685, though it's not clear to me how it could be affected.  https://mozilla.github.io/mozregression/  https://bugzilla.mozilla.org/buglist.cgi?list_id=14470550&columnlist=cf_rank%2Cshort_desc%2Cstatus_whiteboard%2Cassigned_to%2Cbug_status%2Cpriority%2Ccf_fx_points%2Cchangeddate%2Ccomponent&resolution=FIXED&query_based_on=64%20release&query_format=advanced&bug_status=RESOLVED&bug_status=VERIFIED&bug_status=CLOSED&component=WebRTC&component=WebRTC%3A%20Audio%2FVideo&component=WebRTC%3A%20Networking&component=WebRTC%3A%20Signaling&target_milestone=mozilla64&product=Core&known_name=64%20release
Flags: needinfo?(jib) → needinfo?(stapatrick9)
Could LiveSwitch be objecting to the new SCTP window? Nils, WDYT?
No this is not caused by the window size. Most likely it's the code from bug 1485883 which is causing this. @mt: looks like we over looked a legit use case for not negotiating any SRTP profiles with DTLS, when you only want to negotiate a data channel only. Do you think this is covered by the specs in any way? Assuming for now this a regression and setting therefore high priority.
I'm surprised that this is a problem. An error in DTLS will result in an error in the SRTP layer. I think that we were previously ignoring failures from the SRTP negotiation. If so, then what I will attach will likely work. I had a look at the code here and removing the check changes the guarantees that TransportLayerDtls provides. Basically, if we offer SRTP (which we always do), and the peer doesn't accept it because it expects to only be using data channels, then we will die later. We don't conditionally add TransportLayerSrtp based on the availability of SRTP. I have some changes (attached), but no time to go over them carefully. I don't build gecko often enough and I'm currently stuck upgrading all sorts of things to allow it to build. So I haven't tested this. I think that we'll want lots of tests around this to be sure that we don't regress more.
(In reply to Martin Thomson [:mt:] from comment #9) > I had a look at the code here and removing the check changes the guarantees > that TransportLayerDtls provides. Basically, if we offer SRTP (which we > always do), and the peer doesn't accept it because it expects to only be > using data channels, then we will die later. We don't conditionally add > TransportLayerSrtp based on the availability of SRTP. Maybe this is the right thing to do going forward: only offer SRTP extensions if the transport contains audio or video. And then also only insert TransportLayerSrtp if it's needed. I think the biggest issue is at which layer to figure out if SRTP is needed or not and then pass it down to the transportlayer code. The other question is: if we would no longer offer SRTP extension in DTLS for datachannel only transport how many implementations would barf? I guess the less problematic option would to continue to negotiate the SRTP extension, but simply not insert TransportLayerSrtp if we know it a data channel only transport. > I have some changes (attached), but no time to go over them carefully. I > don't build gecko often enough and I'm currently stuck upgrading all sorts > of things to allow it to build. So I haven't tested this. I think that > we'll want lots of tests around this to be sure that we don't regress more. Thanks for the patch. It turns out that your last patch already had tests for both situations (client not offering SRTP and server not accepting SRTP), but they expected to fail. So I think that was simply the wrong expectation. Unfortunately it looks like the TransportLayerSrtp code itself doesn't have any test coverage, except indirectly through mochitests. But these don't handle the non SRTP scenario. I guess in this case we would want to have a combined test of TransportLayerSrtp and TransportLayerDtls. I think we should land as is for now and request uplift to Beta to fix this regression. And then take care of improvements in follow up bugs which ride the trains.
Summary: DTLS Exchange closing connection - datachannel Webrtc → DTLS without SRTP extension (for datachannel only) closes connection
Byron, can you drive this until Nils is back?
I've added a review comment, this is pretty close to being able to land, but Nils owns the patch.
Attachment #9031339 - Attachment is obsolete: true
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/8fb54b5db199 allow DTLS connection w/o SRTP extension. r=bwc
You need to log in before you can comment on or make changes to this bug.