Allow remote SDP offers to be "active" or "passive"

Bug: webrtc:12933
Change-Id: I75f148a1700143571e0ef8bce8a99123bae9c918
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/229181
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34812}

p2p/base/transport_description_factory.cc

@@ -91,11 +91,26 @@ std::unique_ptr<TransportDescription> TransportDescriptionFactory::CreateAnswer(
   if (offer && offer->identity_fingerprint.get()) {
     // The offer supports DTLS, so answer with DTLS, as long as we support it.
     if (secure_ == SEC_ENABLED || secure_ == SEC_REQUIRED) {
-      // Fail if we can't create the fingerprint.
+      ConnectionRole role = CONNECTIONROLE_NONE;
-      // Setting DTLS role to active.
+      // If the offer does not constrain the role, go with preference.
-      ConnectionRole role = (options.prefer_passive_role)
+      if (offer->connection_role == CONNECTIONROLE_ACTPASS) {
-                                ? CONNECTIONROLE_PASSIVE
+        role = (options.prefer_passive_role) ? CONNECTIONROLE_PASSIVE
-                                : CONNECTIONROLE_ACTIVE;
+                                             : CONNECTIONROLE_ACTIVE;
+      } else if (offer->connection_role == CONNECTIONROLE_ACTIVE) {
+        role = CONNECTIONROLE_PASSIVE;
+      } else if (offer->connection_role == CONNECTIONROLE_PASSIVE) {
+        role = CONNECTIONROLE_ACTIVE;
+      } else if (offer->connection_role == CONNECTIONROLE_NONE) {
+        // This case may be reached if a=setup is not present in the SDP.
+        RTC_LOG(LS_WARNING) << "Remote offer connection role is NONE, which is "
+                               "a protocol violation";
+        role = (options.prefer_passive_role) ? CONNECTIONROLE_PASSIVE
+                                             : CONNECTIONROLE_ACTIVE;
+      } else {
+        RTC_LOG(LS_ERROR) << "Remote offer connection role is " << role
+                          << " which is a protocol violation";
+        RTC_NOTREACHED();
+      }
 
       if (!SetSecurityInfo(desc.get(), role)) {
         return NULL;

p2p/base/transport_description_factory_unittest.cc

@@ -352,3 +352,50 @@ TEST_F(TransportDescriptionFactoryTest, CreateAnswerIceCredentialsIterator) {
   EXPECT_EQ(answer->GetIceParameters().ufrag, credentials[0].ufrag);
   EXPECT_EQ(answer->GetIceParameters().pwd, credentials[0].pwd);
 }
+
+TEST_F(TransportDescriptionFactoryTest, CreateAnswerToDtlsActpassOffer) {
+  f1_.set_secure(cricket::SEC_ENABLED);
+  f1_.set_certificate(cert1_);
+
+  f2_.set_secure(cricket::SEC_ENABLED);
+  f2_.set_certificate(cert2_);
+  cricket::TransportOptions options;
+  std::unique_ptr<TransportDescription> offer =
+      f1_.CreateOffer(options, nullptr, &ice_credentials_);
+
+  std::unique_ptr<TransportDescription> answer =
+      f2_.CreateAnswer(offer.get(), options, false, nullptr, &ice_credentials_);
+  EXPECT_EQ(answer->connection_role, cricket::CONNECTIONROLE_ACTIVE);
+}
+
+TEST_F(TransportDescriptionFactoryTest, CreateAnswerToDtlsActiveOffer) {
+  f1_.set_secure(cricket::SEC_ENABLED);
+  f1_.set_certificate(cert1_);
+
+  f2_.set_secure(cricket::SEC_ENABLED);
+  f2_.set_certificate(cert2_);
+  cricket::TransportOptions options;
+  std::unique_ptr<TransportDescription> offer =
+      f1_.CreateOffer(options, nullptr, &ice_credentials_);
+  offer->connection_role = cricket::CONNECTIONROLE_ACTIVE;
+
+  std::unique_ptr<TransportDescription> answer =
+      f2_.CreateAnswer(offer.get(), options, false, nullptr, &ice_credentials_);
+  EXPECT_EQ(answer->connection_role, cricket::CONNECTIONROLE_PASSIVE);
+}
+
+TEST_F(TransportDescriptionFactoryTest, CreateAnswerToDtlsPassiveOffer) {
+  f1_.set_secure(cricket::SEC_ENABLED);
+  f1_.set_certificate(cert1_);
+
+  f2_.set_secure(cricket::SEC_ENABLED);
+  f2_.set_certificate(cert2_);
+  cricket::TransportOptions options;
+  std::unique_ptr<TransportDescription> offer =
+      f1_.CreateOffer(options, nullptr, &ice_credentials_);
+  offer->connection_role = cricket::CONNECTIONROLE_PASSIVE;
+
+  std::unique_ptr<TransportDescription> answer =
+      f2_.CreateAnswer(offer.get(), options, false, nullptr, &ice_credentials_);
+  EXPECT_EQ(answer->connection_role, cricket::CONNECTIONROLE_ACTIVE);
+}

pc/jsep_transport.cc

@@ -619,6 +619,9 @@ webrtc::RTCError JsepTransport::NegotiateDtlsRole(
   // ClientHello over each flow (host/port quartet).
   // IOW - actpass and passive modes should be treated as server and
   // active as client.
+  // RFC 8842 section 5.3 updates this text, so that it is mandated
+  // for the responder to handle offers with "active" and "passive"
+  // as well as "actpass"
   bool is_remote_server = false;
   if (local_description_type == SdpType::kOffer) {
     if (local_connection_role != CONNECTIONROLE_ACTPASS) {
@@ -649,15 +652,37 @@ webrtc::RTCError JsepTransport::NegotiateDtlsRole(
       // See https://datatracker.ietf.org/doc/html/draft-ietf-mmusic-dtls-sdp,
       // section 5.5.
       auto current_dtls_role = GetDtlsRole();
-      if (!current_dtls_role ||
+      if (!current_dtls_role) {
-          (*current_dtls_role == rtc::SSL_CLIENT &&
+        // Role not assigned yet. Verify that local role fits with remote role.
-           remote_connection_role == CONNECTIONROLE_ACTIVE) ||
+        switch (remote_connection_role) {
-          (*current_dtls_role == rtc::SSL_SERVER &&
+          case CONNECTIONROLE_ACTIVE:
-           remote_connection_role == CONNECTIONROLE_PASSIVE)) {
+            if (local_connection_role != CONNECTIONROLE_PASSIVE) {
-        return webrtc::RTCError(
+              return webrtc::RTCError(
-            webrtc::RTCErrorType::INVALID_PARAMETER,
+                  webrtc::RTCErrorType::INVALID_PARAMETER,
-            "Offerer must use actpass value or current negotiated role for "
+                  "Answerer must be passive when offerer is active");
-            "setup attribute.");
+            }
+            break;
+          case CONNECTIONROLE_PASSIVE:
+            if (local_connection_role != CONNECTIONROLE_ACTIVE) {
+              return webrtc::RTCError(
+                  webrtc::RTCErrorType::INVALID_PARAMETER,
+                  "Answerer must be active when offerer is passive");
+            }
+            break;
+          default:
+            RTC_NOTREACHED();
+            break;
+        }
+      } else {
+        if ((*current_dtls_role == rtc::SSL_CLIENT &&
+             remote_connection_role == CONNECTIONROLE_ACTIVE) ||
+            (*current_dtls_role == rtc::SSL_SERVER &&
+             remote_connection_role == CONNECTIONROLE_PASSIVE)) {
+          return webrtc::RTCError(
+              webrtc::RTCErrorType::INVALID_PARAMETER,
+              "Offerer must use current negotiated role for "
+              "setup attribute.");
+        }
       }
     }
 

pc/jsep_transport_unittest.cc

@@ -42,6 +42,20 @@ struct NegotiateRoleParams {
   SdpType remote_type;
 };
 
+std::ostream& operator<<(std::ostream& os, const ConnectionRole& role) {
+  std::string str = "invalid";
+  ConnectionRoleToString(role, &str);
+  os << str;
+  return os;
+}
+
+std::ostream& operator<<(std::ostream& os, const NegotiateRoleParams& param) {
+  os << "[Local role " << param.local_role << " Remote role "
+     << param.remote_role << " LocalType " << SdpTypeToString(param.local_type)
+     << " RemoteType " << SdpTypeToString(param.remote_type) << "]";
+  return os;
+}
+
 rtc::scoped_refptr<webrtc::IceTransportInterface> CreateIceTransport(
     std::unique_ptr<FakeIceTransport> internal) {
   if (!internal) {
@@ -445,7 +459,13 @@ TEST_P(JsepTransport2WithRtcpMux, ValidDtlsRoleNegotiation) {
       {CONNECTIONROLE_ACTPASS, CONNECTIONROLE_PASSIVE, SdpType::kOffer,
        SdpType::kAnswer},
       {CONNECTIONROLE_ACTPASS, CONNECTIONROLE_PASSIVE, SdpType::kOffer,
-       SdpType::kPrAnswer}};
+       SdpType::kPrAnswer},
+      // Combinations permitted by RFC 8842 section 5.3
+      {CONNECTIONROLE_ACTIVE, CONNECTIONROLE_PASSIVE, SdpType::kAnswer,
+       SdpType::kOffer},
+      {CONNECTIONROLE_ACTIVE, CONNECTIONROLE_PASSIVE, SdpType::kPrAnswer,
+       SdpType::kOffer},
+  };
 
   for (auto& param : valid_client_params) {
     jsep_transport_ =
@@ -487,7 +507,11 @@ TEST_P(JsepTransport2WithRtcpMux, ValidDtlsRoleNegotiation) {
       {CONNECTIONROLE_ACTPASS, CONNECTIONROLE_ACTIVE, SdpType::kOffer,
        SdpType::kAnswer},
       {CONNECTIONROLE_ACTPASS, CONNECTIONROLE_ACTIVE, SdpType::kOffer,
-       SdpType::kPrAnswer}};
+       SdpType::kPrAnswer},
+      // Combinations permitted by RFC 8842 section 5.3
+      {CONNECTIONROLE_PASSIVE, CONNECTIONROLE_ACTIVE, SdpType::kPrAnswer,
+       SdpType::kOffer},
+  };
 
   for (auto& param : valid_server_params) {
     jsep_transport_ =
@@ -590,20 +614,15 @@ TEST_P(JsepTransport2WithRtcpMux, InvalidDtlsRoleNegotiation) {
     }
   }
 
-  // Invalid parameters due to the offerer not using ACTPASS.
+  // Invalid parameters due to the offerer not using a role consistent with the
+  // state
   NegotiateRoleParams offerer_without_actpass_params[] = {
-      {CONNECTIONROLE_ACTIVE, CONNECTIONROLE_PASSIVE, SdpType::kAnswer,
+      // Cannot use ACTPASS in an answer
-       SdpType::kOffer},
-      {CONNECTIONROLE_PASSIVE, CONNECTIONROLE_ACTIVE, SdpType::kAnswer,
-       SdpType::kOffer},
       {CONNECTIONROLE_ACTPASS, CONNECTIONROLE_PASSIVE, SdpType::kAnswer,
        SdpType::kOffer},
-      {CONNECTIONROLE_ACTIVE, CONNECTIONROLE_PASSIVE, SdpType::kPrAnswer,
-       SdpType::kOffer},
-      {CONNECTIONROLE_PASSIVE, CONNECTIONROLE_ACTIVE, SdpType::kPrAnswer,
-       SdpType::kOffer},
       {CONNECTIONROLE_ACTPASS, CONNECTIONROLE_PASSIVE, SdpType::kPrAnswer,
        SdpType::kOffer},
+      // Cannot send ACTIVE or PASSIVE in an offer (must handle, must not send)
       {CONNECTIONROLE_ACTIVE, CONNECTIONROLE_PASSIVE, SdpType::kOffer,
        SdpType::kAnswer},
       {CONNECTIONROLE_PASSIVE, CONNECTIONROLE_ACTIVE, SdpType::kOffer,
@@ -629,20 +648,24 @@ TEST_P(JsepTransport2WithRtcpMux, InvalidDtlsRoleNegotiation) {
       EXPECT_TRUE(jsep_transport_
                       ->SetLocalJsepTransportDescription(local_description,
                                                          param.local_type)
-                      .ok());
+                      .ok())
+          << param;
       EXPECT_FALSE(jsep_transport_
                        ->SetRemoteJsepTransportDescription(remote_description,
                                                            param.remote_type)
-                       .ok());
+                       .ok())
+          << param;
     } else {
       EXPECT_TRUE(jsep_transport_
                       ->SetRemoteJsepTransportDescription(remote_description,
                                                           param.remote_type)
-                      .ok());
+                      .ok())
+          << param;
       EXPECT_FALSE(jsep_transport_
                        ->SetLocalJsepTransportDescription(local_description,
                                                           param.local_type)
-                       .ok());
+                       .ok())
+          << param;
     }
   }
 }

pc/webrtc_sdp.cc

@@ -2264,7 +2264,6 @@ bool ParseSessionDescription(const std::string& message,
       session_extmaps->push_back(extmap);
     }
   }
-
   return true;
 }
 

pc/webrtc_sdp_unittest.cc

@@ -3675,7 +3675,7 @@ TEST_F(WebRtcSdpTest, SerializeDtlsSetupAttribute) {
   EXPECT_EQ(sdp_with_dtlssetup, message);
 }
 
-TEST_F(WebRtcSdpTest, DeserializeDtlsSetupAttribute) {
+TEST_F(WebRtcSdpTest, DeserializeDtlsSetupAttributeActpass) {
   JsepSessionDescription jdesc_with_dtlssetup(kDummyType);
   std::string sdp_with_dtlssetup = kSdpFullString;
   InjectAfter(kSessionTime, "a=setup:actpass\r\n", &sdp_with_dtlssetup);
@@ -3691,6 +3691,37 @@ TEST_F(WebRtcSdpTest, DeserializeDtlsSetupAttribute) {
             vtinfo->description.connection_role);
 }
 
+TEST_F(WebRtcSdpTest, DeserializeDtlsSetupAttributeActive) {
+  JsepSessionDescription jdesc_with_dtlssetup(kDummyType);
+  std::string sdp_with_dtlssetup = kSdpFullString;
+  InjectAfter(kSessionTime, "a=setup:active\r\n", &sdp_with_dtlssetup);
+  EXPECT_TRUE(SdpDeserialize(sdp_with_dtlssetup, &jdesc_with_dtlssetup));
+  cricket::SessionDescription* desc = jdesc_with_dtlssetup.description();
+  const cricket::TransportInfo* atinfo =
+      desc->GetTransportInfoByName("audio_content_name");
+  EXPECT_EQ(cricket::CONNECTIONROLE_ACTIVE,
+            atinfo->description.connection_role);
+  const cricket::TransportInfo* vtinfo =
+      desc->GetTransportInfoByName("video_content_name");
+  EXPECT_EQ(cricket::CONNECTIONROLE_ACTIVE,
+            vtinfo->description.connection_role);
+}
+TEST_F(WebRtcSdpTest, DeserializeDtlsSetupAttributePassive) {
+  JsepSessionDescription jdesc_with_dtlssetup(kDummyType);
+  std::string sdp_with_dtlssetup = kSdpFullString;
+  InjectAfter(kSessionTime, "a=setup:passive\r\n", &sdp_with_dtlssetup);
+  EXPECT_TRUE(SdpDeserialize(sdp_with_dtlssetup, &jdesc_with_dtlssetup));
+  cricket::SessionDescription* desc = jdesc_with_dtlssetup.description();
+  const cricket::TransportInfo* atinfo =
+      desc->GetTransportInfoByName("audio_content_name");
+  EXPECT_EQ(cricket::CONNECTIONROLE_PASSIVE,
+            atinfo->description.connection_role);
+  const cricket::TransportInfo* vtinfo =
+      desc->GetTransportInfoByName("video_content_name");
+  EXPECT_EQ(cricket::CONNECTIONROLE_PASSIVE,
+            vtinfo->description.connection_role);
+}
+
 // Verifies that the order of the serialized m-lines follows the order of the
 // ContentInfo in SessionDescription, and vise versa for deserialization.
 TEST_F(WebRtcSdpTest, MediaContentOrderMaintainedRoundTrip) {