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}
diff --git a/p2p/base/transport_description_factory.cc b/p2p/base/transport_description_factory.cc index 5cce2ac..6beae34 100644 --- a/p2p/base/transport_description_factory.cc +++ b/p2p/base/transport_description_factory.cc
@@ -91,11 +91,26 @@ 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. - // Setting DTLS role to active. - ConnectionRole role = (options.prefer_passive_role) - ? CONNECTIONROLE_PASSIVE - : CONNECTIONROLE_ACTIVE; + ConnectionRole role = CONNECTIONROLE_NONE; + // If the offer does not constrain the role, go with preference. + if (offer->connection_role == CONNECTIONROLE_ACTPASS) { + role = (options.prefer_passive_role) ? CONNECTIONROLE_PASSIVE + : 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;
diff --git a/p2p/base/transport_description_factory_unittest.cc b/p2p/base/transport_description_factory_unittest.cc index 01120a8..77a56ef 100644 --- a/p2p/base/transport_description_factory_unittest.cc +++ b/p2p/base/transport_description_factory_unittest.cc
@@ -352,3 +352,50 @@ 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); +}
diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index 791bf7f..d490819 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc
@@ -619,6 +619,9 @@ // 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 @@ // See https://datatracker.ietf.org/doc/html/draft-ietf-mmusic-dtls-sdp, // section 5.5. auto current_dtls_role = GetDtlsRole(); - if (!current_dtls_role || - (*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 actpass value or current negotiated role for " - "setup attribute."); + if (!current_dtls_role) { + // Role not assigned yet. Verify that local role fits with remote role. + switch (remote_connection_role) { + case CONNECTIONROLE_ACTIVE: + if (local_connection_role != CONNECTIONROLE_PASSIVE) { + return webrtc::RTCError( + webrtc::RTCErrorType::INVALID_PARAMETER, + "Answerer must be passive when offerer is active"); + } + 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."); + } } }
diff --git a/pc/jsep_transport_unittest.cc b/pc/jsep_transport_unittest.cc index a511cd3..41c9bab 100644 --- a/pc/jsep_transport_unittest.cc +++ b/pc/jsep_transport_unittest.cc
@@ -42,6 +42,20 @@ 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 @@ {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 @@ {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 @@ } } - // 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, - SdpType::kOffer}, - {CONNECTIONROLE_PASSIVE, CONNECTIONROLE_ACTIVE, SdpType::kAnswer, - SdpType::kOffer}, + // Cannot use ACTPASS in an answer {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 @@ 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; } } }
diff --git a/pc/webrtc_sdp.cc b/pc/webrtc_sdp.cc index d0faf78..d5fcddb 100644 --- a/pc/webrtc_sdp.cc +++ b/pc/webrtc_sdp.cc
@@ -2264,7 +2264,6 @@ session_extmaps->push_back(extmap); } } - return true; }
diff --git a/pc/webrtc_sdp_unittest.cc b/pc/webrtc_sdp_unittest.cc index 310da38..a2a884a 100644 --- a/pc/webrtc_sdp_unittest.cc +++ b/pc/webrtc_sdp_unittest.cc
@@ -3675,7 +3675,7 @@ 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 @@ 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) {