Bug 1510487 - Allow DTLS connection w/o SRTP extension. r=bwc, a=RyanVM
authorNils Ohlmeier [:drno] <drno@ohlmeier.org>
Thu, 17 Jan 2019 14:16:01 +0000
changeset 509539 435e0e27532d79d15538cd8376ea0f6d706065ca
parent 509538 8f904ee0cae1a2252b79534dee93d814d297e231
child 509540 d850ab97f26b8824425de7d88a8603265de6e3ae
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbwc, RyanVM
bugs1510487
milestone65.0
Bug 1510487 - Allow DTLS connection w/o SRTP extension. r=bwc, a=RyanVM Differential Revision: https://phabricator.services.mozilla.com/D14692
media/mtransport/test/transport_unittests.cpp
media/mtransport/transportlayerdtls.cpp
media/mtransport/transportlayersrtp.cpp
media/webrtc/signaling/src/peerconnection/MediaTransportHandler.cpp
--- a/media/mtransport/test/transport_unittests.cpp
+++ b/media/mtransport/test/transport_unittests.cpp
@@ -1383,28 +1383,31 @@ TEST_F(TransportTest, TestSrtpErrorClien
   SetupSrtp();
   SetDtlsPeer();
   ConnectSocketExpectFail();
 }
 
 TEST_F(TransportTest, OnlyServerSendsSrtpXtn) {
   p1_->SetupSrtp();
   SetDtlsPeer();
-  ConnectSocketExpectState(TransportLayer::TS_ERROR,
-                           TransportLayer::TS_CLOSED);
+  // This should connect, but with no SRTP extension neogtiated.
+  // The client side might negotiate a data channel only.
+  ConnectSocket();
+  ASSERT_NE(TLS_NULL_WITH_NULL_NULL, p1_->cipherSuite());
+  ASSERT_EQ(0, p1_->srtpCipher());
 }
 
 TEST_F(TransportTest, OnlyClientSendsSrtpXtn) {
   p2_->SetupSrtp();
   SetDtlsPeer();
-  // This means that the server won't semd the extension as well.  The server
-  // (p1) thinks that everything is OK.  The client (p2) notices the problem
-  // after connecting and aborts.
-  ConnectSocketExpectState(TransportLayer::TS_CLOSED,
-                           TransportLayer::TS_ERROR);
+  // This should connect, but with no SRTP extension neogtiated.
+  // The server side might negotiate a data channel only.
+  ConnectSocket();
+  ASSERT_NE(TLS_NULL_WITH_NULL_NULL, p1_->cipherSuite());
+  ASSERT_EQ(0, p1_->srtpCipher());
 }
 
 class TransportSrtpParameterTest : public TransportTest,
                                    public ::testing::WithParamInterface<uint16_t> {
 };
 
 INSTANTIATE_TEST_CASE_P(SrtpParamInit,
                         TransportSrtpParameterTest,
--- a/media/mtransport/transportlayerdtls.cpp
+++ b/media/mtransport/transportlayerdtls.cpp
@@ -880,22 +880,16 @@ void TransportLayerDtls::Handshake() {
     if (!CheckAlpn()) {
       // Despite connecting, the connection doesn't have a valid ALPN label.
       // Forcibly close the connection so that the peer isn't left hanging
       // (assuming the close_notify isn't dropped).
       ssl_fd_ = nullptr;
       TL_SET_STATE(TS_ERROR);
       return;
     }
-    if (!enabled_srtp_ciphers_.empty() && srtp_cipher_ == 0) {
-      // We enabled SRTP, but got no cipher, this should have failed.
-      ssl_fd_ = nullptr;
-      TL_SET_STATE(TS_ERROR);
-      return;
-    }
 
     TL_SET_STATE(TS_OPEN);
 
     RecordTlsTelemetry();
   } else {
     int32_t err = PR_GetError();
     switch (err) {
       case SSL_ERROR_RX_MALFORMED_HANDSHAKE:
@@ -1588,17 +1582,17 @@ void TransportLayerDtls::RecordTlsTeleme
   }
 
   Telemetry::Accumulate(Telemetry::WEBRTC_DTLS_CIPHER, telemetry_cipher);
 
   uint16_t cipher;
   nsresult rv = GetSrtpCipher(&cipher);
 
   if (NS_FAILED(rv)) {
-    MOZ_MTLOG(ML_ERROR, "Failed to get SRTP cipher suite");
+    MOZ_MTLOG(ML_DEBUG, "No SRTP cipher suite");
     return;
   }
 
   auto cipher_label = mozilla::Telemetry::LABELS_WEBRTC_SRTP_CIPHER::Unknown;
 
   switch (cipher) {
     case kDtlsSrtpAes128CmHmacSha1_80:
       cipher_label = Telemetry::LABELS_WEBRTC_SRTP_CIPHER::Aes128CmHmacSha1_80;
--- a/media/mtransport/transportlayersrtp.cpp
+++ b/media/mtransport/transportlayersrtp.cpp
@@ -107,17 +107,17 @@ TransportResult TransportLayerSrtp::Send
 void TransportLayerSrtp::StateChange(TransportLayer* layer, State state) {
   if (state == TS_OPEN) {
     TransportLayerDtls* dtls = static_cast<TransportLayerDtls*>(layer);
     MOZ_ASSERT(dtls);  // DTLS is mandatory
 
     uint16_t cipher_suite;
     nsresult res = dtls->GetSrtpCipher(&cipher_suite);
     if (NS_FAILED(res)) {
-      MOZ_MTLOG(ML_ERROR, "Failed to negotiate DTLS-SRTP. This is an error");
+      MOZ_MTLOG(ML_DEBUG, "DTLS-SRTP disabled");
       TL_SET_STATE(TS_ERROR);
       return;
     }
 
     unsigned int key_size = SrtpFlow::KeySize(cipher_suite);
     unsigned int salt_size = SrtpFlow::SaltSize(cipher_suite);
     unsigned int master_key_size = key_size + salt_size;
     MOZ_ASSERT(master_key_size <= SRTP_MAX_KEY_LENGTH);
--- a/media/webrtc/signaling/src/peerconnection/MediaTransportHandler.cpp
+++ b/media/webrtc/signaling/src/peerconnection/MediaTransportHandler.cpp
@@ -496,16 +496,18 @@ void MediaTransportHandler::SendPacket(c
   if (layer->SendPacket(aPacket) < 0) {
     CSFLogError(LOGTAG, "%s: Transport flow (%s) failed to send packet",
                 mIceCtx->name().c_str(), aTransportId.c_str());
   }
 }
 
 TransportLayer::State MediaTransportHandler::GetState(
     const std::string& aTransportId, bool aRtcp) const {
+  // TODO Bug 1520692: we should allow Datachannel to connect without
+  // DTLS SRTP keys
   RefPtr<TransportFlow> flow = GetTransportFlow(aTransportId, aRtcp);
   if (flow) {
     return flow->GetLayer(TransportLayerDtls::ID())->state();
   }
   return TransportLayer::TS_NONE;
 }
 
 RefPtr<RTCStatsQueryPromise> MediaTransportHandler::GetIceStats(