Properly handle different transports having different SSL roles.
This meant splitting "transport_options" into audio/video/data options,
for when creating the answer, and giving "GetSslRole" a "transport_name"
parameter so we can retrieve the current role on a per-transport basis.
BUG=webrtc:4525
R=pthatcher@webrtc.org
Review URL: https://codereview.webrtc.org/1516993002 .
Cr-Commit-Position: refs/heads/master@{#11192}
diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc
index 5f1e3eb..ccca18a 100644
--- a/talk/app/webrtc/peerconnection.cc
+++ b/talk/app/webrtc/peerconnection.cc
@@ -461,7 +461,11 @@
}
session_options->vad_enabled = rtc_options.voice_activity_detection;
- session_options->transport_options.ice_restart = rtc_options.ice_restart;
+ session_options->audio_transport_options.ice_restart =
+ rtc_options.ice_restart;
+ session_options->video_transport_options.ice_restart =
+ rtc_options.ice_restart;
+ session_options->data_transport_options.ice_restart = rtc_options.ice_restart;
session_options->bundle_enabled = rtc_options.use_rtp_mux;
return true;
@@ -507,10 +511,14 @@
if (FindConstraint(constraints, MediaConstraintsInterface::kIceRestart,
&value, &mandatory_constraints_satisfied)) {
- session_options->transport_options.ice_restart = value;
+ session_options->audio_transport_options.ice_restart = value;
+ session_options->video_transport_options.ice_restart = value;
+ session_options->data_transport_options.ice_restart = value;
} else {
// kIceRestart defaults to false according to spec.
- session_options->transport_options.ice_restart = false;
+ session_options->audio_transport_options.ice_restart = false;
+ session_options->video_transport_options.ice_restart = false;
+ session_options->data_transport_options.ice_restart = false;
}
if (!constraints) {
@@ -962,7 +970,7 @@
// SCTP sids.
rtc::SSLRole role;
if (session_->data_channel_type() == cricket::DCT_SCTP &&
- session_->GetSslRole(&role)) {
+ session_->GetSslRole(session_->data_channel(), &role)) {
AllocateSctpSids(role);
}
@@ -1040,7 +1048,7 @@
// SCTP sids.
rtc::SSLRole role;
if (session_->data_channel_type() == cricket::DCT_SCTP &&
- session_->GetSslRole(&role)) {
+ session_->GetSslRole(session_->data_channel(), &role)) {
AllocateSctpSids(role);
}
@@ -1833,7 +1841,7 @@
if (session_->data_channel_type() == cricket::DCT_SCTP) {
if (new_config.id < 0) {
rtc::SSLRole role;
- if (session_->GetSslRole(&role) &&
+ if ((session_->GetSslRole(session_->data_channel(), &role)) &&
!sid_allocator_.AllocateSid(role, &new_config.id)) {
LOG(LS_ERROR) << "No id can be allocated for the SCTP data channel.";
return nullptr;
diff --git a/talk/app/webrtc/peerconnectioninterface_unittest.cc b/talk/app/webrtc/peerconnectioninterface_unittest.cc
index b705da4..c3789b7 100644
--- a/talk/app/webrtc/peerconnectioninterface_unittest.cc
+++ b/talk/app/webrtc/peerconnectioninterface_unittest.cc
@@ -2323,7 +2323,9 @@
EXPECT_FALSE(options.has_video());
EXPECT_TRUE(options.bundle_enabled);
EXPECT_TRUE(options.vad_enabled);
- EXPECT_FALSE(options.transport_options.ice_restart);
+ EXPECT_FALSE(options.audio_transport_options.ice_restart);
+ EXPECT_FALSE(options.video_transport_options.ice_restart);
+ EXPECT_FALSE(options.data_transport_options.ice_restart);
}
// Test that a correct MediaSessionOptions is created for an offer if
@@ -2358,18 +2360,22 @@
// Test that a correct MediaSessionOptions is created to restart ice if
// IceRestart is set. It also tests that subsequent MediaSessionOptions don't
-// have |transport_options.ice_restart| set.
+// have |audio_transport_options.ice_restart| etc. set.
TEST(CreateSessionOptionsTest, GetMediaSessionOptionsForOfferWithIceRestart) {
RTCOfferAnswerOptions rtc_options;
rtc_options.ice_restart = true;
cricket::MediaSessionOptions options;
EXPECT_TRUE(ConvertRtcOptionsForOffer(rtc_options, &options));
- EXPECT_TRUE(options.transport_options.ice_restart);
+ EXPECT_TRUE(options.audio_transport_options.ice_restart);
+ EXPECT_TRUE(options.video_transport_options.ice_restart);
+ EXPECT_TRUE(options.data_transport_options.ice_restart);
rtc_options = RTCOfferAnswerOptions();
EXPECT_TRUE(ConvertRtcOptionsForOffer(rtc_options, &options));
- EXPECT_FALSE(options.transport_options.ice_restart);
+ EXPECT_FALSE(options.audio_transport_options.ice_restart);
+ EXPECT_FALSE(options.video_transport_options.ice_restart);
+ EXPECT_FALSE(options.data_transport_options.ice_restart);
}
// Test that the MediaConstraints in an answer don't affect if audio and video
diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc
index 29a4f33..d8f7637 100644
--- a/talk/app/webrtc/webrtcsession.cc
+++ b/talk/app/webrtc/webrtcsession.cc
@@ -761,14 +761,20 @@
return webrtc_session_desc_factory_->SdesPolicy();
}
-bool WebRtcSession::GetSslRole(rtc::SSLRole* role) {
+bool WebRtcSession::GetSslRole(const std::string& transport_name,
+ rtc::SSLRole* role) {
if (!local_desc_ || !remote_desc_) {
LOG(LS_INFO) << "Local and Remote descriptions must be applied to get "
<< "SSL Role of the session.";
return false;
}
- return transport_controller_->GetSslRole(role);
+ return transport_controller_->GetSslRole(transport_name, role);
+}
+
+bool WebRtcSession::GetSslRole(const cricket::BaseChannel* channel,
+ rtc::SSLRole* role) {
+ return channel && GetSslRole(channel->transport_name(), role);
}
void WebRtcSession::CreateOffer(
@@ -970,15 +976,12 @@
return BadPranswerSdp(source, GetSessionErrorMsg(), err_desc);
}
} else if (action == kAnswer) {
- if (!PushdownTransportDescription(source, cricket::CA_ANSWER, &td_err)) {
- return BadAnswerSdp(source, MakeTdErrorString(td_err), err_desc);
- }
const cricket::ContentGroup* local_bundle =
local_desc_->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE);
const cricket::ContentGroup* remote_bundle =
remote_desc_->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE);
if (local_bundle && remote_bundle) {
- // The answerer decides the transport to bundle on
+ // The answerer decides the transport to bundle on.
const cricket::ContentGroup* answer_bundle =
(source == cricket::CS_LOCAL ? local_bundle : remote_bundle);
if (!EnableBundle(*answer_bundle)) {
@@ -986,6 +989,11 @@
return BadAnswerSdp(source, kEnableBundleFailed, err_desc);
}
}
+ // Only push down the transport description after enabling BUNDLE; we don't
+ // want to push down a description on a transport about to be destroyed.
+ if (!PushdownTransportDescription(source, cricket::CA_ANSWER, &td_err)) {
+ return BadAnswerSdp(source, MakeTdErrorString(td_err), err_desc);
+ }
EnableChannels();
SetState(STATE_INPROGRESS);
if (!PushdownMediaDescription(cricket::CA_ANSWER, source, err_desc)) {
diff --git a/talk/app/webrtc/webrtcsession.h b/talk/app/webrtc/webrtcsession.h
index dd5ca18..b79e0ec 100644
--- a/talk/app/webrtc/webrtcsession.h
+++ b/talk/app/webrtc/webrtcsession.h
@@ -204,7 +204,11 @@
cricket::SecurePolicy SdesPolicy() const;
// Get current ssl role from transport.
- bool GetSslRole(rtc::SSLRole* role);
+ bool GetSslRole(const std::string& transport_name, rtc::SSLRole* role);
+
+ // Get current SSL role for this channel's transport.
+ // If |transport| is null, returns false.
+ bool GetSslRole(const cricket::BaseChannel* channel, rtc::SSLRole* role);
void CreateOffer(
CreateSessionDescriptionObserver* observer,
diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc
index bf7fc83..e81b8b5 100644
--- a/talk/app/webrtc/webrtcsession_unittest.cc
+++ b/talk/app/webrtc/webrtcsession_unittest.cc
@@ -1957,6 +1957,67 @@
SetLocalDescriptionWithoutError(answer);
}
+// Test that we can create and set an answer correctly when different
+// SSL roles have been negotiated for different transports.
+// See: https://bugs.chromium.org/p/webrtc/issues/detail?id=4525
+TEST_P(WebRtcSessionTest, TestCreateAnswerWithDifferentSslRoles) {
+ SendAudioVideoStream1();
+ InitWithDtls(GetParam());
+ SetFactoryDtlsSrtp();
+
+ SessionDescriptionInterface* offer = CreateOffer();
+ SetLocalDescriptionWithoutError(offer);
+
+ cricket::MediaSessionOptions options;
+ options.recv_video = true;
+
+ // First, negotiate different SSL roles.
+ SessionDescriptionInterface* answer =
+ CreateRemoteAnswer(offer, options, cricket::SEC_DISABLED);
+ TransportInfo* audio_transport_info =
+ answer->description()->GetTransportInfoByName("audio");
+ audio_transport_info->description.connection_role =
+ cricket::CONNECTIONROLE_ACTIVE;
+ TransportInfo* video_transport_info =
+ answer->description()->GetTransportInfoByName("video");
+ video_transport_info->description.connection_role =
+ cricket::CONNECTIONROLE_PASSIVE;
+ SetRemoteDescriptionWithoutError(answer);
+
+ // Now create an offer in the reverse direction, and ensure the initial
+ // offerer responds with an answer with correct SSL roles.
+ offer = CreateRemoteOfferWithVersion(options, cricket::SEC_DISABLED,
+ kSessionVersion,
+ session_->remote_description());
+ SetRemoteDescriptionWithoutError(offer);
+
+ answer = CreateAnswer(nullptr);
+ audio_transport_info = answer->description()->GetTransportInfoByName("audio");
+ EXPECT_EQ(cricket::CONNECTIONROLE_PASSIVE,
+ audio_transport_info->description.connection_role);
+ video_transport_info = answer->description()->GetTransportInfoByName("video");
+ EXPECT_EQ(cricket::CONNECTIONROLE_ACTIVE,
+ video_transport_info->description.connection_role);
+ SetLocalDescriptionWithoutError(answer);
+
+ // Lastly, start BUNDLE-ing on "audio", expecting that the "passive" role of
+ // audio is transferred over to video in the answer that completes the BUNDLE
+ // negotiation.
+ options.bundle_enabled = true;
+ offer = CreateRemoteOfferWithVersion(options, cricket::SEC_DISABLED,
+ kSessionVersion,
+ session_->remote_description());
+ SetRemoteDescriptionWithoutError(offer);
+ answer = CreateAnswer(nullptr);
+ audio_transport_info = answer->description()->GetTransportInfoByName("audio");
+ EXPECT_EQ(cricket::CONNECTIONROLE_PASSIVE,
+ audio_transport_info->description.connection_role);
+ video_transport_info = answer->description()->GetTransportInfoByName("video");
+ EXPECT_EQ(cricket::CONNECTIONROLE_PASSIVE,
+ video_transport_info->description.connection_role);
+ SetLocalDescriptionWithoutError(answer);
+}
+
TEST_F(WebRtcSessionTest, TestSetLocalOfferTwice) {
Init();
SendNothing();
@@ -3625,7 +3686,9 @@
SetLocalDescriptionWithoutError(answer.release());
// Receive an offer with new ufrag and password.
- options.transport_options.ice_restart = true;
+ options.audio_transport_options.ice_restart = true;
+ options.video_transport_options.ice_restart = true;
+ options.data_transport_options.ice_restart = true;
rtc::scoped_ptr<JsepSessionDescription> updated_offer1(
CreateRemoteOffer(options, session_->remote_description()));
SetRemoteDescriptionWithoutError(updated_offer1.release());
@@ -3656,7 +3719,9 @@
SetLocalDescriptionWithoutError(answer.release());
// Receive an offer without changed ufrag or password.
- options.transport_options.ice_restart = false;
+ options.audio_transport_options.ice_restart = false;
+ options.video_transport_options.ice_restart = false;
+ options.data_transport_options.ice_restart = false;
rtc::scoped_ptr<JsepSessionDescription> updated_offer2(
CreateRemoteOffer(options, session_->remote_description()));
SetRemoteDescriptionWithoutError(updated_offer2.release());
diff --git a/talk/app/webrtc/webrtcsessiondescriptionfactory.cc b/talk/app/webrtc/webrtcsessiondescriptionfactory.cc
index bfdbb1a..f08b77e 100644
--- a/talk/app/webrtc/webrtcsessiondescriptionfactory.cc
+++ b/talk/app/webrtc/webrtcsessiondescriptionfactory.cc
@@ -392,7 +392,9 @@
return;
}
if (session_->local_description() &&
- !request.options.transport_options.ice_restart) {
+ !request.options.audio_transport_options.ice_restart &&
+ !request.options.video_transport_options.ice_restart &&
+ !request.options.data_transport_options.ice_restart) {
// Include all local ice candidates in the SessionDescription unless
// the an ice restart has been requested.
CopyCandidatesFromSessionDescription(session_->local_description(), offer);
@@ -405,12 +407,25 @@
// According to http://tools.ietf.org/html/rfc5245#section-9.2.1.1
// an answer should also contain new ice ufrag and password if an offer has
// been received with new ufrag and password.
- request.options.transport_options.ice_restart = session_->IceRestartPending();
+ request.options.audio_transport_options.ice_restart =
+ session_->IceRestartPending();
+ request.options.video_transport_options.ice_restart =
+ session_->IceRestartPending();
+ request.options.data_transport_options.ice_restart =
+ session_->IceRestartPending();
// We should pass current ssl role to the transport description factory, if
// there is already an existing ongoing session.
rtc::SSLRole ssl_role;
- if (session_->GetSslRole(&ssl_role)) {
- request.options.transport_options.prefer_passive_role =
+ if (session_->GetSslRole(session_->voice_channel(), &ssl_role)) {
+ request.options.audio_transport_options.prefer_passive_role =
+ (rtc::SSL_SERVER == ssl_role);
+ }
+ if (session_->GetSslRole(session_->video_channel(), &ssl_role)) {
+ request.options.video_transport_options.prefer_passive_role =
+ (rtc::SSL_SERVER == ssl_role);
+ }
+ if (session_->GetSslRole(session_->data_channel(), &ssl_role)) {
+ request.options.data_transport_options.prefer_passive_role =
(rtc::SSL_SERVER == ssl_role);
}
@@ -439,7 +454,9 @@
return;
}
if (session_->local_description() &&
- !request.options.transport_options.ice_restart) {
+ !request.options.audio_transport_options.ice_restart &&
+ !request.options.video_transport_options.ice_restart &&
+ !request.options.data_transport_options.ice_restart) {
// Include all local ice candidates in the SessionDescription unless
// the remote peer has requested an ice restart.
CopyCandidatesFromSessionDescription(session_->local_description(), answer);
diff --git a/talk/session/media/mediasession.cc b/talk/session/media/mediasession.cc
index 2dfaa1e..24f01b4 100644
--- a/talk/session/media/mediasession.cc
+++ b/talk/session/media/mediasession.cc
@@ -549,8 +549,8 @@
// Updates the transport infos of the |sdesc| according to the given
// |bundle_group|. The transport infos of the content names within the
-// |bundle_group| should be updated to use the ufrag and pwd of the first
-// content within the |bundle_group|.
+// |bundle_group| should be updated to use the ufrag, pwd and DTLS role of the
+// first content within the |bundle_group|.
static bool UpdateTransportInfoForBundle(const ContentGroup& bundle_group,
SessionDescription* sdesc) {
// The bundle should not be empty.
@@ -571,6 +571,8 @@
selected_transport_info->description.ice_ufrag;
const std::string& selected_pwd =
selected_transport_info->description.ice_pwd;
+ ConnectionRole selected_connection_role =
+ selected_transport_info->description.connection_role;
for (TransportInfos::iterator it =
sdesc->transport_infos().begin();
it != sdesc->transport_infos().end(); ++it) {
@@ -578,6 +580,7 @@
it->content_name != selected_content_name) {
it->description.ice_ufrag = selected_ufrag;
it->description.ice_pwd = selected_pwd;
+ it->description.connection_role = selected_connection_role;
}
}
return true;
@@ -1600,7 +1603,7 @@
}
desc->AddContent(content_name, NS_JINGLE_RTP, audio.release());
- if (!AddTransportOffer(content_name, options.transport_options,
+ if (!AddTransportOffer(content_name, options.audio_transport_options,
current_description, desc)) {
return false;
}
@@ -1660,7 +1663,7 @@
}
desc->AddContent(content_name, NS_JINGLE_RTP, video.release());
- if (!AddTransportOffer(content_name, options.transport_options,
+ if (!AddTransportOffer(content_name, options.video_transport_options,
current_description, desc)) {
return false;
}
@@ -1724,7 +1727,7 @@
SetMediaProtocol(secure_transport, data.get());
desc->AddContent(content_name, NS_JINGLE_RTP, data.release());
}
- if (!AddTransportOffer(content_name, options.transport_options,
+ if (!AddTransportOffer(content_name, options.data_transport_options,
current_description, desc)) {
return false;
}
@@ -1739,10 +1742,9 @@
SessionDescription* answer) const {
const ContentInfo* audio_content = GetFirstAudioContent(offer);
- scoped_ptr<TransportDescription> audio_transport(
- CreateTransportAnswer(audio_content->name, offer,
- options.transport_options,
- current_description));
+ scoped_ptr<TransportDescription> audio_transport(CreateTransportAnswer(
+ audio_content->name, offer, options.audio_transport_options,
+ current_description));
if (!audio_transport) {
return false;
}
@@ -1798,10 +1800,9 @@
StreamParamsVec* current_streams,
SessionDescription* answer) const {
const ContentInfo* video_content = GetFirstVideoContent(offer);
- scoped_ptr<TransportDescription> video_transport(
- CreateTransportAnswer(video_content->name, offer,
- options.transport_options,
- current_description));
+ scoped_ptr<TransportDescription> video_transport(CreateTransportAnswer(
+ video_content->name, offer, options.video_transport_options,
+ current_description));
if (!video_transport) {
return false;
}
@@ -1854,10 +1855,9 @@
StreamParamsVec* current_streams,
SessionDescription* answer) const {
const ContentInfo* data_content = GetFirstDataContent(offer);
- scoped_ptr<TransportDescription> data_transport(
- CreateTransportAnswer(data_content->name, offer,
- options.transport_options,
- current_description));
+ scoped_ptr<TransportDescription> data_transport(CreateTransportAnswer(
+ data_content->name, offer, options.data_transport_options,
+ current_description));
if (!data_transport) {
return false;
}
diff --git a/talk/session/media/mediasession.h b/talk/session/media/mediasession.h
index 8a7f4cc..1540274 100644
--- a/talk/session/media/mediasession.h
+++ b/talk/session/media/mediasession.h
@@ -134,6 +134,10 @@
bool HasSendMediaStream(MediaType type) const;
+ // TODO(deadbeef): Put all the audio/video/data-specific options into a map
+ // structure (content name -> options).
+ // MediaSessionDescriptionFactory assumes there will never be more than one
+ // audio/video/data content, but this will change with unified plan.
bool recv_audio;
bool recv_video;
DataChannelType data_channel_type;
@@ -144,7 +148,9 @@
// bps. -1 == auto.
int video_bandwidth;
int data_bandwidth;
- TransportOptions transport_options;
+ TransportOptions audio_transport_options;
+ TransportOptions video_transport_options;
+ TransportOptions data_transport_options;
struct Stream {
Stream(MediaType type,
diff --git a/webrtc/p2p/base/transportcontroller.cc b/webrtc/p2p/base/transportcontroller.cc
index 22b827a..053388e 100644
--- a/webrtc/p2p/base/transportcontroller.cc
+++ b/webrtc/p2p/base/transportcontroller.cc
@@ -66,9 +66,10 @@
rtc::Bind(&TransportController::SetIceRole_w, this, ice_role));
}
-bool TransportController::GetSslRole(rtc::SSLRole* role) {
- return worker_thread_->Invoke<bool>(
- rtc::Bind(&TransportController::GetSslRole_w, this, role));
+bool TransportController::GetSslRole(const std::string& transport_name,
+ rtc::SSLRole* role) {
+ return worker_thread_->Invoke<bool>(rtc::Bind(
+ &TransportController::GetSslRole_w, this, transport_name, role));
}
bool TransportController::SetLocalCertificate(
@@ -343,13 +344,16 @@
}
}
-bool TransportController::GetSslRole_w(rtc::SSLRole* role) {
+bool TransportController::GetSslRole_w(const std::string& transport_name,
+ rtc::SSLRole* role) {
RTC_DCHECK(worker_thread()->IsCurrent());
- if (transports_.empty()) {
+ Transport* t = GetTransport_w(transport_name);
+ if (!t) {
return false;
}
- return transports_.begin()->second->GetSslRole(role);
+
+ return t->GetSslRole(role);
}
bool TransportController::SetLocalCertificate_w(
diff --git a/webrtc/p2p/base/transportcontroller.h b/webrtc/p2p/base/transportcontroller.h
index 8d57b46..e26f3b5 100644
--- a/webrtc/p2p/base/transportcontroller.h
+++ b/webrtc/p2p/base/transportcontroller.h
@@ -48,11 +48,7 @@
void SetIceConfig(const IceConfig& config);
void SetIceRole(IceRole ice_role);
- // TODO(deadbeef) - Return role of each transport, as role may differ from
- // one another.
- // In current implementaion we just return the role of the first transport
- // alphabetically.
- bool GetSslRole(rtc::SSLRole* role);
+ bool GetSslRole(const std::string& transport_name, rtc::SSLRole* role);
// Specifies the identity to use in this session.
// Can only be called once.
@@ -160,7 +156,7 @@
bool SetSslMaxProtocolVersion_w(rtc::SSLProtocolVersion version);
void SetIceConfig_w(const IceConfig& config);
void SetIceRole_w(IceRole ice_role);
- bool GetSslRole_w(rtc::SSLRole* role);
+ bool GetSslRole_w(const std::string& transport_name, rtc::SSLRole* role);
bool SetLocalCertificate_w(
const rtc::scoped_refptr<rtc::RTCCertificate>& certificate);
bool GetLocalCertificate_w(
diff --git a/webrtc/p2p/base/transportcontroller_unittest.cc b/webrtc/p2p/base/transportcontroller_unittest.cc
index 1408529..6ff158e 100644
--- a/webrtc/p2p/base/transportcontroller_unittest.cc
+++ b/webrtc/p2p/base/transportcontroller_unittest.cc
@@ -262,7 +262,8 @@
ASSERT_NE(nullptr, channel);
ASSERT_TRUE(channel->SetSslRole(rtc::SSL_CLIENT));
rtc::SSLRole role;
- EXPECT_TRUE(transport_controller_->GetSslRole(&role));
+ EXPECT_FALSE(transport_controller_->GetSslRole("video", &role));
+ EXPECT_TRUE(transport_controller_->GetSslRole("audio", &role));
EXPECT_EQ(rtc::SSL_CLIENT, role);
}
