Reland "Change default secure SCTP protocol to UDP/DTLS/SCTP"

This reverts commit 4436887ed2d3324279e0f2e091c9e9355392721a.

Reason for revert: The original revert was intended to be temporary.

Original change's description:
> Revert "Change default secure SCTP protocol to UDP/DTLS/SCTP"
> 
> This reverts commit c3f4820e129d44471b366b8885a67b5392918d5a.
> 
> Reason for revert: Will temporarily revert to fix an issue and reland afterwards.
> 
> Original change's description:
> > Change default secure SCTP protocol to UDP/DTLS/SCTP
> > 
> > The old value - DTLS/SCTP - is not standards conformant,
> > and the new value should be parsable since Chrome M61.
> > 
> > Bug: webrtc:7706
> > Change-Id: I7468cc9597dec4ef4b102fccddc4e981fed7e8d8
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/136804
> > Reviewed-by: Steve Anton <steveanton@webrtc.org>
> > Commit-Queue: Harald Alvestrand <hta@webrtc.org>
> > Cr-Commit-Position: refs/heads/master@{#27940}
> 
> TBR=steveanton@webrtc.org,mbonadei@webrtc.org,hbos@webrtc.org,hta@webrtc.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug: webrtc:7706
> Change-Id: Ida8ae20767485c75edc44dff8a3fa1af2006f207
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/139244
> Reviewed-by: Guido Urdaneta <guidou@webrtc.org>
> Reviewed-by: Henrik Boström <hbos@webrtc.org>
> Commit-Queue: Guido Urdaneta <guidou@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#28121}

TBR=steveanton@webrtc.org,mbonadei@webrtc.org,hbos@webrtc.org,hta@webrtc.org,guidou@webrtc.org

Change-Id: I381fa18b644874c20ddaa4cd13fec79a5fd9555a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:7706
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/139246
Reviewed-by: Guido Urdaneta <guidou@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Guido Urdaneta <guidou@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28122}

pc/media_session.cc

@@ -2236,10 +2236,7 @@ bool MediaSessionDescriptionFactory::AddSctpDataContentForOffer(
   // before we call CreateMediaContentOffer.  Otherwise,
   // CreateMediaContentOffer won't know this is SCTP and will
   // generate SSRCs rather than SIDs.
-  // TODO(deadbeef): Offer kMediaProtocolUdpDtlsSctp (or TcpDtlsSctp), once
+  data->set_protocol(secure_transport ? kMediaProtocolUdpDtlsSctp
-  // it's safe to do so. Older versions of webrtc would reject these
-  // protocols; see https://bugs.chromium.org/p/webrtc/issues/detail?id=7706.
-  data->set_protocol(secure_transport ? kMediaProtocolDtlsSctp
                                       : kMediaProtocolSctp);
   data->set_use_sctpmap(session_options.use_obsolete_sctp_sdp);
   data->set_max_message_size(kSctpSendBufferSize);

pc/media_session_unittest.cc

@@ -916,6 +916,26 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateSctpDataOffer) {
   std::unique_ptr<SessionDescription> offer = f1_.CreateOffer(opts, NULL);
   EXPECT_TRUE(offer.get() != NULL);
   EXPECT_TRUE(offer->GetContentByName("data") != NULL);
+  auto dcd = GetFirstSctpDataContentDescription(offer.get());
+  ASSERT_TRUE(dcd);
+  // Since this transport is insecure, the protocol should be "SCTP".
+  EXPECT_EQ(cricket::kMediaProtocolSctp, dcd->protocol());
+}
+
+// Create an SCTP data offer with bundle without error.
+TEST_F(MediaSessionDescriptionFactoryTest, TestCreateSecureSctpDataOffer) {
+  MediaSessionOptions opts;
+  opts.bundle_enabled = true;
+  AddDataSection(cricket::DCT_SCTP, RtpTransceiverDirection::kSendRecv, &opts);
+  f1_.set_secure(SEC_ENABLED);
+  tdf1_.set_secure(SEC_ENABLED);
+  std::unique_ptr<SessionDescription> offer = f1_.CreateOffer(opts, NULL);
+  EXPECT_TRUE(offer.get() != NULL);
+  EXPECT_TRUE(offer->GetContentByName("data") != NULL);
+  auto dcd = GetFirstSctpDataContentDescription(offer.get());
+  ASSERT_TRUE(dcd);
+  // The protocol should now be "UDP/DTLS/SCTP"
+  EXPECT_EQ(cricket::kMediaProtocolUdpDtlsSctp, dcd->protocol());
 }
 
 // Test creating an sctp data channel from an already generated offer.

pc/peer_connection_data_channel_unittest.cc

@@ -414,7 +414,7 @@ TEST_P(PeerConnectionDataChannelTest, MediaTransportWithoutSdesFails) {
   EXPECT_EQ(nullptr, caller);
 }
 
-TEST_P(PeerConnectionDataChannelTest, DISABLED_ModernSdpSyntaxByDefault) {
+TEST_P(PeerConnectionDataChannelTest, ModernSdpSyntaxByDefault) {
   PeerConnectionInterface::RTCOfferAnswerOptions options;
   auto caller = CreatePeerConnectionWithDataChannel();
   auto offer = caller->CreateOffer(options);

pc/webrtc_sdp_unittest.cc

@@ -278,7 +278,7 @@ static const char kSdpRtpDataChannelString[] =
 
 // draft-ietf-mmusic-sctp-sdp-03
 static const char kSdpSctpDataChannelString[] =
-    "m=application 9 DTLS/SCTP 5000\r\n"
+    "m=application 9 UDP/DTLS/SCTP 5000\r\n"
     "c=IN IP4 0.0.0.0\r\n"
     "a=ice-ufrag:ufrag_data\r\n"
     "a=ice-pwd:pwd_data\r\n"
@@ -289,7 +289,7 @@ static const char kSdpSctpDataChannelString[] =
 // Note - this is invalid per draft-ietf-mmusic-sctp-sdp-26,
 // since the separator after "sctp-port" needs to be a colon.
 static const char kSdpSctpDataChannelStringWithSctpPort[] =
-    "m=application 9 DTLS/SCTP webrtc-datachannel\r\n"
+    "m=application 9 UDP/DTLS/SCTP webrtc-datachannel\r\n"
     "a=sctp-port 5000\r\n"
     "c=IN IP4 0.0.0.0\r\n"
     "a=ice-ufrag:ufrag_data\r\n"
@@ -298,7 +298,7 @@ static const char kSdpSctpDataChannelStringWithSctpPort[] =
 
 // draft-ietf-mmusic-sctp-sdp-26
 static const char kSdpSctpDataChannelStringWithSctpColonPort[] =
-    "m=application 9 DTLS/SCTP webrtc-datachannel\r\n"
+    "m=application 9 UDP/DTLS/SCTP webrtc-datachannel\r\n"
     "a=sctp-port:5000\r\n"
     "c=IN IP4 0.0.0.0\r\n"
     "a=ice-ufrag:ufrag_data\r\n"
@@ -306,7 +306,7 @@ static const char kSdpSctpDataChannelStringWithSctpColonPort[] =
     "a=mid:data_content_name\r\n";
 
 static const char kSdpSctpDataChannelWithCandidatesString[] =
-    "m=application 2345 DTLS/SCTP 5000\r\n"
+    "m=application 2345 UDP/DTLS/SCTP 5000\r\n"
     "c=IN IP4 74.125.127.126\r\n"
     "a=candidate:a0+B/1 1 udp 2130706432 192.168.1.5 1234 typ host "
     "generation 2\r\n"
@@ -1784,7 +1784,7 @@ class WebRtcSdpTest : public ::testing::Test {
         new SctpDataContentDescription());
     sctp_desc_ = data.get();
     sctp_desc_->set_use_sctpmap(use_sctpmap);
-    sctp_desc_->set_protocol(cricket::kMediaProtocolDtlsSctp);
+    sctp_desc_->set_protocol(cricket::kMediaProtocolUdpDtlsSctp);
     sctp_desc_->set_port(kDefaultSctpPort);
     desc_.AddContent(kDataContentName, MediaProtocolType::kSctp,
                      data.release());
@@ -2822,18 +2822,18 @@ TEST_F(WebRtcSdpTest, DeserializeSdpWithSctpDataChannels) {
   sdp_with_data.append(kSdpSctpDataChannelString);
   JsepSessionDescription jdesc_output(kDummyType);
 
-  // Verify with DTLS/SCTP (already in kSdpSctpDataChannelString).
+  // Verify with UDP/DTLS/SCTP (already in kSdpSctpDataChannelString).
   EXPECT_TRUE(SdpDeserialize(sdp_with_data, &jdesc_output));
   EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output));
 
-  // Verify with UDP/DTLS/SCTP.
+  // Verify with DTLS/SCTP.
-  sdp_with_data.replace(sdp_with_data.find(kDtlsSctp), strlen(kDtlsSctp),
+  sdp_with_data.replace(sdp_with_data.find(kUdpDtlsSctp), strlen(kUdpDtlsSctp),
-                        kUdpDtlsSctp);
+                        kDtlsSctp);
   EXPECT_TRUE(SdpDeserialize(sdp_with_data, &jdesc_output));
   EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output));
 
   // Verify with TCP/DTLS/SCTP.
-  sdp_with_data.replace(sdp_with_data.find(kUdpDtlsSctp), strlen(kUdpDtlsSctp),
+  sdp_with_data.replace(sdp_with_data.find(kDtlsSctp), strlen(kDtlsSctp),
                         kTcpDtlsSctp);
   EXPECT_TRUE(SdpDeserialize(sdp_with_data, &jdesc_output));
   EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output));
@@ -2849,19 +2849,6 @@ TEST_F(WebRtcSdpTest, DeserializeSdpWithSctpDataChannelsWithSctpPort) {
   sdp_with_data.append(kSdpSctpDataChannelStringWithSctpPort);
   JsepSessionDescription jdesc_output(kDummyType);
 
-  // Verify with DTLS/SCTP (already in kSdpSctpDataChannelStringWithSctpPort).
-  EXPECT_TRUE(SdpDeserialize(sdp_with_data, &jdesc_output));
-  EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output));
-
-  // Verify with UDP/DTLS/SCTP.
-  sdp_with_data.replace(sdp_with_data.find(kDtlsSctp), strlen(kDtlsSctp),
-                        kUdpDtlsSctp);
-  EXPECT_TRUE(SdpDeserialize(sdp_with_data, &jdesc_output));
-  EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output));
-
-  // Verify with TCP/DTLS/SCTP.
-  sdp_with_data.replace(sdp_with_data.find(kUdpDtlsSctp), strlen(kUdpDtlsSctp),
-                        kTcpDtlsSctp);
   EXPECT_TRUE(SdpDeserialize(sdp_with_data, &jdesc_output));
   EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output));
 }
@@ -2876,19 +2863,6 @@ TEST_F(WebRtcSdpTest, DeserializeSdpWithSctpDataChannelsWithSctpColonPort) {
   sdp_with_data.append(kSdpSctpDataChannelStringWithSctpColonPort);
   JsepSessionDescription jdesc_output(kDummyType);
 
-  // Verify with DTLS/SCTP.
-  EXPECT_TRUE(SdpDeserialize(sdp_with_data, &jdesc_output));
-  EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output));
-
-  // Verify with UDP/DTLS/SCTP.
-  sdp_with_data.replace(sdp_with_data.find(kDtlsSctp), strlen(kDtlsSctp),
-                        kUdpDtlsSctp);
-  EXPECT_TRUE(SdpDeserialize(sdp_with_data, &jdesc_output));
-  EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output));
-
-  // Verify with TCP/DTLS/SCTP.
-  sdp_with_data.replace(sdp_with_data.find(kUdpDtlsSctp), strlen(kUdpDtlsSctp),
-                        kTcpDtlsSctp);
   EXPECT_TRUE(SdpDeserialize(sdp_with_data, &jdesc_output));
   EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output));
 }
@@ -2916,19 +2890,6 @@ TEST_F(WebRtcSdpTest, DeserializeSdpWithSctpDataChannelsWithMaxMessageSize) {
   MutateJsepSctpMaxMessageSize(desc_, 12345, &jdesc);
   JsepSessionDescription jdesc_output(kDummyType);
 
-  // Verify with DTLS/SCTP.
-  EXPECT_TRUE(SdpDeserialize(sdp_with_data, &jdesc_output));
-  EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output));
-
-  // Verify with UDP/DTLS/SCTP.
-  sdp_with_data.replace(sdp_with_data.find(kDtlsSctp), strlen(kDtlsSctp),
-                        kUdpDtlsSctp);
-  EXPECT_TRUE(SdpDeserialize(sdp_with_data, &jdesc_output));
-  EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output));
-
-  // Verify with TCP/DTLS/SCTP.
-  sdp_with_data.replace(sdp_with_data.find(kUdpDtlsSctp), strlen(kUdpDtlsSctp),
-                        kTcpDtlsSctp);
   EXPECT_TRUE(SdpDeserialize(sdp_with_data, &jdesc_output));
   EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_output));
 }
@@ -4559,3 +4520,29 @@ TEST_F(WebRtcSdpTest, SerializeMediaTransportSettingsTestCopy) {
   EXPECT_EQ("name", copy->MediaTransportSettings()[0].transport_name);
   EXPECT_EQ("setting", copy->MediaTransportSettings()[0].transport_setting);
 }
+
+TEST_F(WebRtcSdpTest, SerializeWithDefaultSctpProtocol) {
+  AddSctpDataChannel(false);  // Don't use sctpmap
+  JsepSessionDescription jsep_desc(kDummyType);
+  MakeDescriptionWithoutCandidates(&jsep_desc);
+  std::string message = webrtc::SdpSerialize(jsep_desc);
+  EXPECT_NE(std::string::npos,
+            message.find(cricket::kMediaProtocolUdpDtlsSctp));
+}
+
+TEST_F(WebRtcSdpTest, DeserializeWithAllSctpProtocols) {
+  AddSctpDataChannel(false);
+  std::string protocols[] = {cricket::kMediaProtocolDtlsSctp,
+                             cricket::kMediaProtocolUdpDtlsSctp,
+                             cricket::kMediaProtocolTcpDtlsSctp};
+  for (const auto& protocol : protocols) {
+    sctp_desc_->set_protocol(protocol);
+    JsepSessionDescription jsep_desc(kDummyType);
+    MakeDescriptionWithoutCandidates(&jsep_desc);
+    std::string message = webrtc::SdpSerialize(jsep_desc);
+    EXPECT_NE(std::string::npos, message.find(protocol));
+    JsepSessionDescription jsep_output(kDummyType);
+    SdpParseError error;
+    EXPECT_TRUE(webrtc::SdpDeserialize(message, &jsep_output, &error));
+  }
+}