1510487 - DTLS without SRTP extension (for datachannel only) closes connection

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[stapatrick9]

Attachment: SDP_and_DTLS_exchange_Logs.txt

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.

Comment 1

[laszlo.bialis]

I am assigning a component to this issue in order to involve the development team and get an opinion on this.

Comment 2

[stapatrick9]

(In reply to undefined from comment #undefined)
> 

Thanks

Comment 3

[stapatrick9]

Attachment: 64_netFIRSTIN_filter_applied_1.pcapng

This is the wireShark pcapng file from the failted datachannels dtls exchange with firefox 64. 192.168.124.173 is firefox in this case.

Comment 4

[stapatrick9]

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.

Comment 5

[Andreas Pehrson [:pehrsons]]

jib, can you triage this?

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Thanks stapatrick9 for the 63-64 log comparison!   

If this readily reproduces, would you be able to use the mozregression tool [1] to narrow down the regression range further?

The only datachannel-related WebRTC change I see in 64 release [1], appears to be bug 1051685, though it's not clear to me how it could be affected.

[1] https://mozilla.github.io/mozregression/

[2] 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

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Could LiveSwitch be objecting to the new SCTP window? Nils, WDYT?

Comment 8

[Nils Ohlmeier [:drno]]

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.

Comment 9

[Martin Thomson [:mt:]]

Attachment: 1510487-wip.patch

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.

Comment 10

[Nils Ohlmeier [:drno]]

Attachment: Bug 1510487: allow DTLS connection w/o SRTP extension. r?bwc

Comment 11

[Nils Ohlmeier [:drno]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=922f8195f69bcec3e3086916aa102240844189e0

Comment 12

[Nils Ohlmeier [:drno]]

(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.

Comment 13

[Andreas Pehrson [:pehrsons]]

Byron, can you drive this until Nils is back?

Comment 14

[Byron Campen [:bwc]]

I've added a review comment, this is pretty close to being able to land, but Nils owns the patch.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Is this close to ready to land? Tomorrow's the final Beta of the Fx65 cycle.

Comment 17

[Nils Ohlmeier [:drno]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)

Is this close to ready to land? Tomorrow's the final Beta of the Fx65 cycle.

Yes I can get it in today.

Comment 18

[Nils Ohlmeier [:drno]]

Try run looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=8383c03ad4d9e443eebd85d6c4cb10f110831c65

Comment 19

[Nils Ohlmeier [:drno]]

Comment on attachment 9031730 [details]
Bug 1510487: allow DTLS connection w/o SRTP extension. r?bwc

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1485883

User impact if declined: For certain WebRTC services Data Channel connections fail to establish (it depends on how the service negotiates the connection on the DTLS layer).

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): It only removes an error check introduced by bug 1485883. Also the scenario which triggers this not very common.

String changes made/needed: N/A

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8fb54b5db199
allow DTLS connection w/o SRTP extension. r=bwc

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8fb54b5db199

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9031730 [details]
Bug 1510487: allow DTLS connection w/o SRTP extension. r?bwc

[Triage Comment]
Fixes Data Channel connection failures for certain WebRTC services. Approved for 65.0b12.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/435e0e27532d

Comment 24

[Nils Ohlmeier [:drno]]

Can you verify that the latest Firefox Beta build fixes the issue for you?

Comment 25

[scottjmaddox]

Yes, the issue is fixed in Firefox 65.0b12!