From 7af91ddd6bda4f411d0025c5d5a2d32be540a34c Mon Sep 17 00:00:00 2001 From: deadbeef Date: Tue, 13 Dec 2016 11:29:11 -0800 Subject: [PATCH] Removing "crypto_required" from MediaContentDescription. "Crypto required" is a property of the PeerConnection of construction time; it has nothing to do with SDP. So I'm moving it out of MediaContentDescription and putting it in the BaseChannel constructor instead. This is more intuitive, and provides the added assurance that "secure_required_" can't be flipped from "true" to "false". BUG=None Review-Url: https://codereview.webrtc.org/2537343003 Cr-Commit-Position: refs/heads/master@{#15579} --- webrtc/api/rtcstatscollector_unittest.cc | 17 +++-- webrtc/api/rtpsenderreceiver_unittest.cc | 6 +- webrtc/api/statscollector_unittest.cc | 97 +++++++++++++----------- webrtc/api/webrtcsession.cc | 54 ++++--------- webrtc/api/webrtcsession.h | 5 +- webrtc/api/webrtcsession_unittest.cc | 8 +- webrtc/p2p/base/transportdescription.h | 2 + webrtc/pc/channel.cc | 32 ++++---- webrtc/pc/channel.h | 24 +++--- webrtc/pc/channel_unittest.cc | 21 +++-- webrtc/pc/channelmanager.cc | 44 +++++------ webrtc/pc/channelmanager.h | 17 +++-- webrtc/pc/channelmanager_unittest.cc | 33 ++++---- webrtc/pc/mediasession.cc | 8 +- webrtc/pc/mediasession_unittest.cc | 5 +- 15 files changed, 184 insertions(+), 189 deletions(-) diff --git a/webrtc/api/rtcstatscollector_unittest.cc b/webrtc/api/rtcstatscollector_unittest.cc index b973044c94..278d945378 100644 --- a/webrtc/api/rtcstatscollector_unittest.cc +++ b/webrtc/api/rtcstatscollector_unittest.cc @@ -101,6 +101,8 @@ void PrintTo(const RTCTransportStats& stats, ::std::ostream* os) { namespace { const int64_t kGetStatsReportTimeoutMs = 1000; +const bool kDefaultRtcpEnabled = false; +const bool kDefaultSrtpRequired = true; struct CertificateInfo { rtc::scoped_refptr certificate; @@ -657,12 +659,13 @@ TEST_F(RTCStatsCollectorTest, CollectRTCCodecStats) { MockVoiceMediaChannel* voice_media_channel = new MockVoiceMediaChannel(); cricket::VoiceChannel voice_channel( test_->worker_thread(), test_->network_thread(), test_->media_engine(), - voice_media_channel, nullptr, "VoiceContentName", false); + voice_media_channel, nullptr, "VoiceContentName", kDefaultRtcpEnabled, + kDefaultSrtpRequired); MockVideoMediaChannel* video_media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel( test_->worker_thread(), test_->network_thread(), video_media_channel, - nullptr, "VideoContentName", false); + nullptr, "VideoContentName", kDefaultRtcpEnabled, kDefaultSrtpRequired); // Audio cricket::VoiceMediaInfo voice_media_info; @@ -1307,7 +1310,8 @@ TEST_F(RTCStatsCollectorTest, CollectRTCInboundRTPStreamStats_Audio) { MockVoiceMediaChannel* voice_media_channel = new MockVoiceMediaChannel(); cricket::VoiceChannel voice_channel( test_->worker_thread(), test_->network_thread(), test_->media_engine(), - voice_media_channel, nullptr, "VoiceContentName", false); + voice_media_channel, nullptr, "VoiceContentName", kDefaultRtcpEnabled, + kDefaultSrtpRequired); cricket::VoiceMediaInfo voice_media_info; @@ -1377,7 +1381,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCInboundRTPStreamStats_Video) { MockVideoMediaChannel* video_media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel( test_->worker_thread(), test_->network_thread(), video_media_channel, - nullptr, "VideoContentName", false); + nullptr, "VideoContentName", kDefaultRtcpEnabled, kDefaultSrtpRequired); cricket::VideoMediaInfo video_media_info; @@ -1451,7 +1455,8 @@ TEST_F(RTCStatsCollectorTest, CollectRTCOutboundRTPStreamStats_Audio) { MockVoiceMediaChannel* voice_media_channel = new MockVoiceMediaChannel(); cricket::VoiceChannel voice_channel( test_->worker_thread(), test_->network_thread(), test_->media_engine(), - voice_media_channel, nullptr, "VoiceContentName", false); + voice_media_channel, nullptr, "VoiceContentName", kDefaultRtcpEnabled, + kDefaultSrtpRequired); cricket::VoiceMediaInfo voice_media_info; @@ -1516,7 +1521,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCOutboundRTPStreamStats_Video) { MockVideoMediaChannel* video_media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel( test_->worker_thread(), test_->network_thread(), video_media_channel, - nullptr, "VideoContentName", false); + nullptr, "VideoContentName", kDefaultRtcpEnabled, kDefaultSrtpRequired); cricket::VideoMediaInfo video_media_info; diff --git a/webrtc/api/rtpsenderreceiver_unittest.cc b/webrtc/api/rtpsenderreceiver_unittest.cc index 4e98f0c1e1..f4f550e906 100644 --- a/webrtc/api/rtpsenderreceiver_unittest.cc +++ b/webrtc/api/rtpsenderreceiver_unittest.cc @@ -62,12 +62,14 @@ class RtpSenderReceiverTest : public testing::Test { stream_(MediaStream::Create(kStreamLabel1)) { // Create channels to be used by the RtpSenders and RtpReceivers. channel_manager_.Init(); + bool rtcp_enabled = false; + bool srtp_required = true; voice_channel_ = channel_manager_.CreateVoiceChannel( &fake_media_controller_, &fake_transport_controller_, cricket::CN_AUDIO, - nullptr, false, cricket::AudioOptions()); + nullptr, rtcp_enabled, srtp_required, cricket::AudioOptions()); video_channel_ = channel_manager_.CreateVideoChannel( &fake_media_controller_, &fake_transport_controller_, cricket::CN_VIDEO, - nullptr, false, cricket::VideoOptions()); + nullptr, rtcp_enabled, srtp_required, cricket::VideoOptions()); voice_media_channel_ = media_engine_->GetVoiceChannel(0); video_media_channel_ = media_engine_->GetVideoChannel(0); RTC_CHECK(voice_channel_); diff --git a/webrtc/api/statscollector_unittest.cc b/webrtc/api/statscollector_unittest.cc index baa35d3659..bf60d0ed95 100644 --- a/webrtc/api/statscollector_unittest.cc +++ b/webrtc/api/statscollector_unittest.cc @@ -49,6 +49,11 @@ using webrtc::PeerConnectionInterface; using webrtc::StatsReport; using webrtc::StatsReports; +namespace { +const bool kDefaultRtcpEnabled = false; +const bool kDefaultSrtpRequired = true; +} + namespace cricket { class ChannelManager; @@ -853,9 +858,9 @@ TEST_F(StatsCollectorTest, BytesCounterHandles64Bits) { Return(true))); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); - cricket::VideoChannel video_channel(worker_thread_, network_thread_, - media_channel, nullptr, kVideoChannelName, - false); + cricket::VideoChannel video_channel( + worker_thread_, network_thread_, media_channel, nullptr, + kVideoChannelName, kDefaultRtcpEnabled, kDefaultSrtpRequired); StatsReports reports; // returned values. cricket::VideoSenderInfo video_sender_info; cricket::VideoMediaInfo stats_read; @@ -900,9 +905,9 @@ TEST_F(StatsCollectorTest, BandwidthEstimationInfoIsReported) { Return(true))); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); - cricket::VideoChannel video_channel(worker_thread_, network_thread_, - media_channel, nullptr, kVideoChannelName, - false); + cricket::VideoChannel video_channel( + worker_thread_, network_thread_, media_channel, nullptr, + kVideoChannelName, kDefaultRtcpEnabled, kDefaultSrtpRequired); StatsReports reports; // returned values. cricket::VideoSenderInfo video_sender_info; @@ -976,8 +981,9 @@ TEST_F(StatsCollectorTest, TrackObjectExistsWithoutUpdateStats) { StatsCollectorForTest stats(&pc_); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); - cricket::VideoChannel video_channel(worker_thread_, network_thread_, - media_channel, nullptr, "video", false); + cricket::VideoChannel video_channel( + worker_thread_, network_thread_, media_channel, nullptr, "video", + kDefaultRtcpEnabled, kDefaultSrtpRequired); AddOutgoingVideoTrackStats(); stats.AddStream(stream_); @@ -1012,9 +1018,9 @@ TEST_F(StatsCollectorTest, TrackAndSsrcObjectExistAfterUpdateSsrcStats) { Return(true))); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); - cricket::VideoChannel video_channel(worker_thread_, network_thread_, - media_channel, nullptr, kVideoChannelName, - false); + cricket::VideoChannel video_channel( + worker_thread_, network_thread_, media_channel, nullptr, + kVideoChannelName, kDefaultRtcpEnabled, kDefaultSrtpRequired); AddOutgoingVideoTrackStats(); stats.AddStream(stream_); @@ -1081,8 +1087,9 @@ TEST_F(StatsCollectorTest, TransportObjectLinkedFromSsrcObject) { MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); // The transport_name known by the video channel. const std::string kVcName("vcname"); - cricket::VideoChannel video_channel(worker_thread_, network_thread_, - media_channel, nullptr, kVcName, false); + cricket::VideoChannel video_channel( + worker_thread_, network_thread_, media_channel, nullptr, kVcName, + kDefaultRtcpEnabled, kDefaultSrtpRequired); AddOutgoingVideoTrackStats(); stats.AddStream(stream_); @@ -1139,8 +1146,9 @@ TEST_F(StatsCollectorTest, RemoteSsrcInfoIsAbsent) { MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); // The transport_name known by the video channel. const std::string kVcName("vcname"); - cricket::VideoChannel video_channel(worker_thread_, network_thread_, - media_channel, nullptr, kVcName, false); + cricket::VideoChannel video_channel( + worker_thread_, network_thread_, media_channel, nullptr, kVcName, + kDefaultRtcpEnabled, kDefaultSrtpRequired); AddOutgoingVideoTrackStats(); stats.AddStream(stream_); @@ -1165,8 +1173,9 @@ TEST_F(StatsCollectorTest, RemoteSsrcInfoIsPresent) { MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); // The transport_name known by the video channel. const std::string kVcName("vcname"); - cricket::VideoChannel video_channel(worker_thread_, network_thread_, - media_channel, nullptr, kVcName, false); + cricket::VideoChannel video_channel( + worker_thread_, network_thread_, media_channel, nullptr, kVcName, + kDefaultRtcpEnabled, kDefaultSrtpRequired); AddOutgoingVideoTrackStats(); stats.AddStream(stream_); @@ -1220,9 +1229,9 @@ TEST_F(StatsCollectorTest, ReportsFromRemoteTrack) { Return(true))); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); - cricket::VideoChannel video_channel(worker_thread_, network_thread_, - media_channel, nullptr, kVideoChannelName, - false); + cricket::VideoChannel video_channel( + worker_thread_, network_thread_, media_channel, nullptr, + kVideoChannelName, kDefaultRtcpEnabled, kDefaultSrtpRequired); AddIncomingVideoTrackStats(); stats.AddStream(stream_); @@ -1529,9 +1538,9 @@ TEST_F(StatsCollectorTest, FilterOutNegativeInitialValues) { MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel(); // The transport_name known by the voice channel. const std::string kVcName("vcname"); - cricket::VoiceChannel voice_channel(worker_thread_, network_thread_, - media_engine_, media_channel, nullptr, - kVcName, false); + cricket::VoiceChannel voice_channel( + worker_thread_, network_thread_, media_engine_, media_channel, nullptr, + kVcName, kDefaultRtcpEnabled, kDefaultSrtpRequired); // Create a local stream with a local audio track and adds it to the stats. if (stream_ == NULL) @@ -1636,9 +1645,9 @@ TEST_F(StatsCollectorTest, GetStatsFromLocalAudioTrack) { MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel(); // The transport_name known by the voice channel. const std::string kVcName("vcname"); - cricket::VoiceChannel voice_channel(worker_thread_, network_thread_, - media_engine_, media_channel, nullptr, - kVcName, false); + cricket::VoiceChannel voice_channel( + worker_thread_, network_thread_, media_engine_, media_channel, nullptr, + kVcName, kDefaultRtcpEnabled, kDefaultSrtpRequired); AddOutgoingAudioTrackStats(); stats.AddStream(stream_); stats.AddLocalAudioTrack(audio_track_, kSsrcOfTrack); @@ -1672,9 +1681,9 @@ TEST_F(StatsCollectorTest, GetStatsFromRemoteStream) { MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel(); // The transport_name known by the voice channel. const std::string kVcName("vcname"); - cricket::VoiceChannel voice_channel(worker_thread_, network_thread_, - media_engine_, media_channel, nullptr, - kVcName, false); + cricket::VoiceChannel voice_channel( + worker_thread_, network_thread_, media_engine_, media_channel, nullptr, + kVcName, kDefaultRtcpEnabled, kDefaultSrtpRequired); AddIncomingAudioTrackStats(); stats.AddStream(stream_); @@ -1702,9 +1711,9 @@ TEST_F(StatsCollectorTest, GetStatsAfterRemoveAudioStream) { MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel(); // The transport_name known by the voice channel. const std::string kVcName("vcname"); - cricket::VoiceChannel voice_channel(worker_thread_, network_thread_, - media_engine_, media_channel, nullptr, - kVcName, false); + cricket::VoiceChannel voice_channel( + worker_thread_, network_thread_, media_engine_, media_channel, nullptr, + kVcName, kDefaultRtcpEnabled, kDefaultSrtpRequired); AddOutgoingAudioTrackStats(); stats.AddStream(stream_); stats.AddLocalAudioTrack(audio_track_.get(), kSsrcOfTrack); @@ -1764,9 +1773,9 @@ TEST_F(StatsCollectorTest, LocalAndRemoteTracksWithSameSsrc) { MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel(); // The transport_name known by the voice channel. const std::string kVcName("vcname"); - cricket::VoiceChannel voice_channel(worker_thread_, network_thread_, - media_engine_, media_channel, nullptr, - kVcName, false); + cricket::VoiceChannel voice_channel( + worker_thread_, network_thread_, media_engine_, media_channel, nullptr, + kVcName, kDefaultRtcpEnabled, kDefaultSrtpRequired); // Create a local stream with a local audio track and adds it to the stats. AddOutgoingAudioTrackStats(); @@ -1852,9 +1861,9 @@ TEST_F(StatsCollectorTest, TwoLocalTracksWithSameSsrc) { MockVoiceMediaChannel* media_channel = new MockVoiceMediaChannel(); // The transport_name known by the voice channel. const std::string kVcName("vcname"); - cricket::VoiceChannel voice_channel(worker_thread_, network_thread_, - media_engine_, media_channel, nullptr, - kVcName, false); + cricket::VoiceChannel voice_channel( + worker_thread_, network_thread_, media_engine_, media_channel, nullptr, + kVcName, kDefaultRtcpEnabled, kDefaultSrtpRequired); // Create a local stream with a local audio track and adds it to the stats. AddOutgoingAudioTrackStats(); @@ -1909,9 +1918,9 @@ TEST_F(StatsCollectorTest, VerifyVideoSendSsrcStats) { .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), Return(true))); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); - cricket::VideoChannel video_channel(worker_thread_, network_thread_, - media_channel, nullptr, kVideoChannelName, - false); + cricket::VideoChannel video_channel( + worker_thread_, network_thread_, media_channel, nullptr, + kVideoChannelName, kDefaultRtcpEnabled, kDefaultSrtpRequired); StatsReports reports; // returned values. cricket::VideoSenderInfo video_sender_info; cricket::VideoMediaInfo stats_read; @@ -1954,9 +1963,9 @@ TEST_F(StatsCollectorTest, VerifyVideoReceiveSsrcStats) { .WillRepeatedly(DoAll(SetArgPointee<0>(session_stats_), Return(true))); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); - cricket::VideoChannel video_channel(worker_thread_, network_thread_, - media_channel, nullptr, kVideoChannelName, - false); + cricket::VideoChannel video_channel( + worker_thread_, network_thread_, media_channel, nullptr, + kVideoChannelName, kDefaultRtcpEnabled, kDefaultSrtpRequired); StatsReports reports; // returned values. cricket::VideoReceiverInfo video_receiver_info; cricket::VideoMediaInfo stats_read; diff --git a/webrtc/api/webrtcsession.cc b/webrtc/api/webrtcsession.cc index c5582d3630..b05862d37c 100644 --- a/webrtc/api/webrtcsession.cc +++ b/webrtc/api/webrtcsession.cc @@ -165,7 +165,7 @@ static bool VerifyMediaDescriptions( // Checks that each non-rejected content has SDES crypto keys or a DTLS // fingerprint. Mismatches, such as replying with a DTLS fingerprint to SDES // keys, will be caught in Transport negotiation, and backstopped by Channel's -// |secure_required| check. +// |srtp_required| check. static bool VerifyCrypto(const SessionDescription* desc, bool dtls_enabled, std::string* error) { @@ -231,30 +231,6 @@ static bool VerifyIceUfragPwdPresent(const SessionDescription* desc) { return true; } -// Forces |sdesc->crypto_required| to the appropriate state based on the -// current security policy, to ensure a failure occurs if there is an error -// in crypto negotiation. -// Called when processing the local session description. -static void UpdateSessionDescriptionSecurePolicy(cricket::CryptoType type, - SessionDescription* sdesc) { - if (!sdesc) { - return; - } - - // Updating the |crypto_required_| in MediaContentDescription to the - // appropriate state based on the current security policy. - for (cricket::ContentInfos::iterator iter = sdesc->contents().begin(); - iter != sdesc->contents().end(); ++iter) { - if (cricket::IsMediaContent(&*iter)) { - MediaContentDescription* mdesc = - static_cast (iter->description); - if (mdesc) { - mdesc->set_crypto_required(type); - } - } - } -} - static bool GetAudioSsrcByTrackId(const SessionDescription* session_description, const std::string& track_id, uint32_t* ssrc) { @@ -641,10 +617,6 @@ cricket::BaseChannel* WebRtcSession::GetChannel( return nullptr; } -void WebRtcSession::SetSdesPolicy(cricket::SecurePolicy secure_policy) { - webrtc_session_desc_factory_->SetSdesPolicy(secure_policy); -} - cricket::SecurePolicy WebRtcSession::SdesPolicy() const { return webrtc_session_desc_factory_->SdesPolicy(); } @@ -697,14 +669,6 @@ bool WebRtcSession::SetLocalDescription(SessionDescriptionInterface* desc, transport_controller_->SetIceRole(cricket::ICEROLE_CONTROLLING); } - cricket::SecurePolicy sdes_policy = - webrtc_session_desc_factory_->SdesPolicy(); - cricket::CryptoType crypto_required = dtls_enabled_ ? - cricket::CT_DTLS : (sdes_policy == cricket::SEC_REQUIRED ? - cricket::CT_SDES : cricket::CT_NONE); - // Update the MediaContentDescription crypto settings as per the policy set. - UpdateSessionDescriptionSecurePolicy(crypto_required, desc->description()); - local_desc_.reset(desc_temp.release()); // Transport and Media channels will be created only when offer is set. @@ -1671,7 +1635,8 @@ bool WebRtcSession::CreateVoiceChannel(const cricket::ContentInfo* content, bool create_rtcp_transport_channel = !require_rtcp_mux; voice_channel_.reset(channel_manager_->CreateVoiceChannel( media_controller_, transport_controller_.get(), content->name, - bundle_transport, create_rtcp_transport_channel, audio_options_)); + bundle_transport, create_rtcp_transport_channel, SrtpRequired(), + audio_options_)); if (!voice_channel_) { return false; } @@ -1695,7 +1660,8 @@ bool WebRtcSession::CreateVideoChannel(const cricket::ContentInfo* content, bool create_rtcp_transport_channel = !require_rtcp_mux; video_channel_.reset(channel_manager_->CreateVideoChannel( media_controller_, transport_controller_.get(), content->name, - bundle_transport, create_rtcp_transport_channel, video_options_)); + bundle_transport, create_rtcp_transport_channel, SrtpRequired(), + video_options_)); if (!video_channel_) { return false; } @@ -1727,8 +1693,9 @@ bool WebRtcSession::CreateDataChannel(const cricket::ContentInfo* content, rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire; bool create_rtcp_transport_channel = !sctp && !require_rtcp_mux; data_channel_.reset(channel_manager_->CreateDataChannel( - transport_controller_.get(), media_controller_, content->name, - bundle_transport, create_rtcp_transport_channel, data_channel_type_)); + media_controller_, transport_controller_.get(), content->name, + bundle_transport, create_rtcp_transport_channel, SrtpRequired(), + data_channel_type_)); if (!data_channel_) { return false; } @@ -1936,6 +1903,11 @@ bool WebRtcSession::ReadyToUseRemoteCandidate( return transport_controller_->ReadyForRemoteCandidates(transport_name); } +bool WebRtcSession::SrtpRequired() const { + return dtls_enabled_ || + webrtc_session_desc_factory_->SdesPolicy() == cricket::SEC_REQUIRED; +} + void WebRtcSession::OnTransportControllerGatheringState( cricket::IceGatheringState state) { ASSERT(signaling_thread()->IsCurrent()); diff --git a/webrtc/api/webrtcsession.h b/webrtc/api/webrtcsession.h index 1aa3e187ae..06d3375e15 100644 --- a/webrtc/api/webrtcsession.h +++ b/webrtc/api/webrtcsession.h @@ -197,7 +197,6 @@ class WebRtcSession : cricket::BaseChannel* GetChannel(const std::string& content_name); - void SetSdesPolicy(cricket::SecurePolicy secure_policy); cricket::SecurePolicy SdesPolicy() const; // Get current ssl role from transport. @@ -449,6 +448,10 @@ class WebRtcSession : const SessionDescriptionInterface* remote_desc, bool* valid); + // Returns true if SRTP (either using DTLS-SRTP or SDES) is required by + // this session. + bool SrtpRequired() const; + void OnTransportControllerConnectionState(cricket::IceConnectionState state); void OnTransportControllerReceiving(bool receiving); void OnTransportControllerGatheringState(cricket::IceGatheringState state); diff --git a/webrtc/api/webrtcsession_unittest.cc b/webrtc/api/webrtcsession_unittest.cc index 51d985c3d7..fe3468820e 100644 --- a/webrtc/api/webrtcsession_unittest.cc +++ b/webrtc/api/webrtcsession_unittest.cc @@ -3644,8 +3644,8 @@ TEST_F(WebRtcSessionTest, TestCryptoAfterSetLocalDescription) { SessionDescriptionInterface* jsep_offer_str = CreateSessionDescription(JsepSessionDescription::kOffer, offer_str, NULL); SetLocalDescriptionWithoutError(jsep_offer_str); - EXPECT_TRUE(session_->voice_channel()->secure_required()); - EXPECT_TRUE(session_->video_channel()->secure_required()); + EXPECT_TRUE(session_->voice_channel()->srtp_required_for_testing()); + EXPECT_TRUE(session_->video_channel()->srtp_required_for_testing()); } // This test verifies the crypto parameter when security is disabled. @@ -3663,8 +3663,8 @@ TEST_F(WebRtcSessionTest, TestCryptoAfterSetLocalDescriptionWithDisabled) { SessionDescriptionInterface* jsep_offer_str = CreateSessionDescription(JsepSessionDescription::kOffer, offer_str, NULL); SetLocalDescriptionWithoutError(jsep_offer_str); - EXPECT_FALSE(session_->voice_channel()->secure_required()); - EXPECT_FALSE(session_->video_channel()->secure_required()); + EXPECT_FALSE(session_->voice_channel()->srtp_required_for_testing()); + EXPECT_FALSE(session_->video_channel()->srtp_required_for_testing()); } // This test verifies that an answer contains new ufrag and password if an offer diff --git a/webrtc/p2p/base/transportdescription.h b/webrtc/p2p/base/transportdescription.h index df8ee92003..e7f9263b3e 100644 --- a/webrtc/p2p/base/transportdescription.h +++ b/webrtc/p2p/base/transportdescription.h @@ -28,6 +28,8 @@ namespace cricket { // SEC_ENABLED: Crypto in outgoing offer and answer (if supplied in offer). // SEC_REQUIRED: Crypto in outgoing offer and answer. Fail any offer with absent // or unsupported crypto. +// TODO(deadbeef): Remove this or rename it to something more appropriate, like +// SdesPolicy. enum SecurePolicy { SEC_DISABLED, SEC_ENABLED, diff --git a/webrtc/pc/channel.cc b/webrtc/pc/channel.cc index eb2f6340ea..1f29261ed2 100644 --- a/webrtc/pc/channel.cc +++ b/webrtc/pc/channel.cc @@ -164,7 +164,8 @@ BaseChannel::BaseChannel(rtc::Thread* worker_thread, MediaChannel* media_channel, TransportController* transport_controller, const std::string& content_name, - bool rtcp) + bool rtcp, + bool srtp_required) : worker_thread_(worker_thread), network_thread_(network_thread), @@ -172,6 +173,7 @@ BaseChannel::BaseChannel(rtc::Thread* worker_thread, transport_controller_(transport_controller), rtcp_enabled_(rtcp), + srtp_required_(srtp_required), media_channel_(media_channel), selected_candidate_pair_(nullptr) { RTC_DCHECK(worker_thread_ == rtc::Thread::Current()); @@ -726,7 +728,7 @@ bool BaseChannel::SendPacket(bool rtcp, // Update the length of the packet now that we've added the auth tag. packet->SetSize(len); - } else if (secure_required_) { + } else if (srtp_required_) { // The audio/video engines may attempt to send RTCP packets as soon as the // streams are created, so don't treat this as an error for RTCP. // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=6809 @@ -815,7 +817,7 @@ void BaseChannel::HandlePacket(bool rtcp, rtc::CopyOnWriteBuffer* packet, } packet->SetSize(len); - } else if (secure_required_) { + } else if (srtp_required_) { // Our session description indicates that SRTP is required, but we got a // packet before our SRTP filter is active. This means either that // a) we got SRTP packets before we received the SDES keys, in which case @@ -1091,7 +1093,7 @@ bool BaseChannel::SetRtpTransportParameters( return true; } - // Cache secure_required_ for belt and suspenders check on SendPacket + // Cache srtp_required_ for belt and suspenders check on SendPacket return network_thread_->Invoke( RTC_FROM_HERE, Bind(&BaseChannel::SetRtpTransportParameters_n, this, content, action, src, error_desc)); @@ -1104,10 +1106,6 @@ bool BaseChannel::SetRtpTransportParameters_n( std::string* error_desc) { RTC_DCHECK(network_thread_->IsCurrent()); - if (src == CS_LOCAL) { - set_secure_required(content->crypto_required() != CT_NONE); - } - if (!SetSrtp_n(content->cryptos(), action, src, error_desc)) { return false; } @@ -1476,13 +1474,15 @@ VoiceChannel::VoiceChannel(rtc::Thread* worker_thread, VoiceMediaChannel* media_channel, TransportController* transport_controller, const std::string& content_name, - bool rtcp) + bool rtcp, + bool srtp_required) : BaseChannel(worker_thread, network_thread, media_channel, transport_controller, content_name, - rtcp), + rtcp, + srtp_required), media_engine_(media_engine), received_media_(false) {} @@ -1889,13 +1889,15 @@ VideoChannel::VideoChannel(rtc::Thread* worker_thread, VideoMediaChannel* media_channel, TransportController* transport_controller, const std::string& content_name, - bool rtcp) + bool rtcp, + bool srtp_required) : BaseChannel(worker_thread, network_thread, media_channel, transport_controller, content_name, - rtcp) {} + rtcp, + srtp_required) {} bool VideoChannel::Init_w(const std::string* bundle_transport_name) { if (!BaseChannel::Init_w(bundle_transport_name)) { @@ -2150,13 +2152,15 @@ DataChannel::DataChannel(rtc::Thread* worker_thread, DataMediaChannel* media_channel, TransportController* transport_controller, const std::string& content_name, - bool rtcp) + bool rtcp, + bool srtp_required) : BaseChannel(worker_thread, network_thread, media_channel, transport_controller, content_name, - rtcp), + rtcp, + srtp_required), data_channel_type_(cricket::DCT_NONE), ready_to_send_data_(false) {} diff --git a/webrtc/pc/channel.h b/webrtc/pc/channel.h index 308903c2db..64c5f8253f 100644 --- a/webrtc/pc/channel.h +++ b/webrtc/pc/channel.h @@ -76,12 +76,15 @@ class BaseChannel public ConnectionStatsGetter { public: // |rtcp| represents whether or not this channel uses RTCP. + // If |srtp_required| is true, the channel will not send or receive any + // RTP/RTCP packets without using SRTP (either using SDES or DTLS-SRTP). BaseChannel(rtc::Thread* worker_thread, rtc::Thread* network_thread, MediaChannel* channel, TransportController* transport_controller, const std::string& content_name, - bool rtcp); + bool rtcp, + bool srtp_required); virtual ~BaseChannel(); bool Init_w(const std::string* bundle_transport_name); // Deinit may be called multiple times and is simply ignored if it's already @@ -100,8 +103,6 @@ class BaseChannel // DTLS-based keying. If you turned off SRTP later, however // you could have secure() == false and dtls_secure() == true. bool secure_dtls() const { return dtls_keyed_; } - // This function returns true if we require secure channel for call setup. - bool secure_required() const { return secure_required_; } bool writable() const { return writable_; } @@ -186,6 +187,9 @@ class BaseChannel bool SetCryptoOptions(const rtc::CryptoOptions& crypto_options); + // This function returns true if we require SRTP for call setup. + bool srtp_required_for_testing() const { return srtp_required_; } + protected: virtual MediaChannel* media_channel() const { return media_channel_; } @@ -206,9 +210,6 @@ class BaseChannel void set_remote_content_direction(MediaContentDirection direction) { remote_content_direction_ = direction; } - void set_secure_required(bool secure_required) { - secure_required_ = secure_required; - } // These methods verify that: // * The required content description directions have been set. // * The channel is enabled. @@ -397,7 +398,7 @@ class BaseChannel bool was_ever_writable_ = false; bool has_received_packet_ = false; bool dtls_keyed_ = false; - bool secure_required_ = false; + const bool srtp_required_ = true; rtc::CryptoOptions crypto_options_; int rtp_abs_sendtime_extn_id_ = -1; @@ -425,7 +426,8 @@ class VoiceChannel : public BaseChannel { VoiceMediaChannel* channel, TransportController* transport_controller, const std::string& content_name, - bool rtcp); + bool rtcp, + bool srtp_required); ~VoiceChannel(); bool Init_w(const std::string* bundle_transport_name); @@ -542,7 +544,8 @@ class VideoChannel : public BaseChannel { VideoMediaChannel* channel, TransportController* transport_controller, const std::string& content_name, - bool rtcp); + bool rtcp, + bool srtp_required); ~VideoChannel(); bool Init_w(const std::string* bundle_transport_name); @@ -620,7 +623,8 @@ class DataChannel : public BaseChannel { DataMediaChannel* media_channel, TransportController* transport_controller, const std::string& content_name, - bool rtcp); + bool rtcp, + bool srtp_required); ~DataChannel(); bool Init_w(const std::string* bundle_transport_name); diff --git a/webrtc/pc/channel_unittest.cc b/webrtc/pc/channel_unittest.cc index 85a7e697a8..5de3c94555 100644 --- a/webrtc/pc/channel_unittest.cc +++ b/webrtc/pc/channel_unittest.cc @@ -190,10 +190,9 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { typename T::MediaChannel* ch, cricket::TransportController* transport_controller, int flags) { - typename T::Channel* channel = - new typename T::Channel(worker_thread, network_thread, engine, ch, - transport_controller, cricket::CN_AUDIO, - (flags & RTCP) != 0); + typename T::Channel* channel = new typename T::Channel( + worker_thread, network_thread, engine, ch, transport_controller, + cricket::CN_AUDIO, (flags & RTCP) != 0, (flags & SECURE) != 0); rtc::CryptoOptions crypto_options; crypto_options.enable_gcm_crypto_suites = (flags & GCM_CIPHER) != 0; channel->SetCryptoOptions(crypto_options); @@ -2042,10 +2041,9 @@ cricket::VideoChannel* ChannelTest::CreateChannel( cricket::FakeVideoMediaChannel* ch, cricket::TransportController* transport_controller, int flags) { - cricket::VideoChannel* channel = - new cricket::VideoChannel(worker_thread, network_thread, ch, - transport_controller, cricket::CN_VIDEO, - (flags & RTCP) != 0); + cricket::VideoChannel* channel = new cricket::VideoChannel( + worker_thread, network_thread, ch, transport_controller, + cricket::CN_VIDEO, (flags & RTCP) != 0, (flags & SECURE) != 0); rtc::CryptoOptions crypto_options; crypto_options.enable_gcm_crypto_suites = (flags & GCM_CIPHER) != 0; channel->SetCryptoOptions(crypto_options); @@ -3315,10 +3313,9 @@ cricket::DataChannel* ChannelTest::CreateChannel( cricket::FakeDataMediaChannel* ch, cricket::TransportController* transport_controller, int flags) { - cricket::DataChannel* channel = - new cricket::DataChannel(worker_thread, network_thread, ch, - transport_controller, cricket::CN_DATA, - (flags & RTCP) != 0); + cricket::DataChannel* channel = new cricket::DataChannel( + worker_thread, network_thread, ch, transport_controller, cricket::CN_DATA, + (flags & RTCP) != 0, (flags & SECURE) != 0); rtc::CryptoOptions crypto_options; crypto_options.enable_gcm_crypto_suites = (flags & GCM_CIPHER) != 0; channel->SetCryptoOptions(crypto_options); diff --git a/webrtc/pc/channelmanager.cc b/webrtc/pc/channelmanager.cc index fec57004a6..a0b3609ecf 100644 --- a/webrtc/pc/channelmanager.cc +++ b/webrtc/pc/channelmanager.cc @@ -218,11 +218,12 @@ VoiceChannel* ChannelManager::CreateVoiceChannel( const std::string& content_name, const std::string* bundle_transport_name, bool rtcp, + bool srtp_required, const AudioOptions& options) { return worker_thread_->Invoke( RTC_FROM_HERE, Bind(&ChannelManager::CreateVoiceChannel_w, this, media_controller, transport_controller, content_name, - bundle_transport_name, rtcp, options)); + bundle_transport_name, rtcp, srtp_required, options)); } VoiceChannel* ChannelManager::CreateVoiceChannel_w( @@ -231,6 +232,7 @@ VoiceChannel* ChannelManager::CreateVoiceChannel_w( const std::string& content_name, const std::string* bundle_transport_name, bool rtcp, + bool srtp_required, const AudioOptions& options) { ASSERT(initialized_); ASSERT(worker_thread_ == rtc::Thread::Current()); @@ -240,9 +242,9 @@ VoiceChannel* ChannelManager::CreateVoiceChannel_w( if (!media_channel) return nullptr; - VoiceChannel* voice_channel = - new VoiceChannel(worker_thread_, network_thread_, media_engine_.get(), - media_channel, transport_controller, content_name, rtcp); + VoiceChannel* voice_channel = new VoiceChannel( + worker_thread_, network_thread_, media_engine_.get(), media_channel, + transport_controller, content_name, rtcp, srtp_required); voice_channel->SetCryptoOptions(crypto_options_); if (!voice_channel->Init_w(bundle_transport_name)) { delete voice_channel; @@ -281,11 +283,12 @@ VideoChannel* ChannelManager::CreateVideoChannel( const std::string& content_name, const std::string* bundle_transport_name, bool rtcp, + bool srtp_required, const VideoOptions& options) { return worker_thread_->Invoke( RTC_FROM_HERE, Bind(&ChannelManager::CreateVideoChannel_w, this, media_controller, transport_controller, content_name, - bundle_transport_name, rtcp, options)); + bundle_transport_name, rtcp, srtp_required, options)); } VideoChannel* ChannelManager::CreateVideoChannel_w( @@ -294,6 +297,7 @@ VideoChannel* ChannelManager::CreateVideoChannel_w( const std::string& content_name, const std::string* bundle_transport_name, bool rtcp, + bool srtp_required, const VideoOptions& options) { ASSERT(initialized_); ASSERT(worker_thread_ == rtc::Thread::Current()); @@ -306,7 +310,7 @@ VideoChannel* ChannelManager::CreateVideoChannel_w( VideoChannel* video_channel = new VideoChannel(worker_thread_, network_thread_, media_channel, - transport_controller, content_name, rtcp); + transport_controller, content_name, rtcp, srtp_required); video_channel->SetCryptoOptions(crypto_options_); if (!video_channel->Init_w(bundle_transport_name)) { delete video_channel; @@ -341,36 +345,28 @@ void ChannelManager::DestroyVideoChannel_w(VideoChannel* video_channel) { } DataChannel* ChannelManager::CreateDataChannel( - TransportController* transport_controller, - const std::string& content_name, - const std::string* bundle_transport_name, - bool rtcp, - DataChannelType channel_type) { - return CreateDataChannel(transport_controller, nullptr, content_name, - bundle_transport_name, rtcp, channel_type); -} - -DataChannel* ChannelManager::CreateDataChannel( - TransportController* transport_controller, webrtc::MediaControllerInterface* media_controller, + TransportController* transport_controller, const std::string& content_name, const std::string* bundle_transport_name, bool rtcp, + bool srtp_required, DataChannelType channel_type) { return worker_thread_->Invoke( RTC_FROM_HERE, - Bind(&ChannelManager::CreateDataChannel_w, this, transport_controller, - content_name, bundle_transport_name, rtcp, channel_type, - media_controller)); + Bind(&ChannelManager::CreateDataChannel_w, this, media_controller, + transport_controller, content_name, bundle_transport_name, rtcp, + srtp_required, channel_type)); } DataChannel* ChannelManager::CreateDataChannel_w( + webrtc::MediaControllerInterface* media_controller, TransportController* transport_controller, const std::string& content_name, const std::string* bundle_transport_name, bool rtcp, - DataChannelType data_channel_type, - webrtc::MediaControllerInterface* media_controller) { + bool srtp_required, + DataChannelType data_channel_type) { // This is ok to alloc from a thread other than the worker thread. ASSERT(initialized_); MediaConfig config; @@ -385,9 +381,11 @@ DataChannel* ChannelManager::CreateDataChannel_w( return NULL; } + // Only RTP data channels need SRTP. + srtp_required = srtp_required && data_channel_type == DCT_RTP; DataChannel* data_channel = new DataChannel(worker_thread_, network_thread_, media_channel, - transport_controller, content_name, rtcp); + transport_controller, content_name, rtcp, srtp_required); data_channel->SetCryptoOptions(crypto_options_); if (!data_channel->Init_w(bundle_transport_name)) { LOG(LS_WARNING) << "Failed to init data channel."; diff --git a/webrtc/pc/channelmanager.h b/webrtc/pc/channelmanager.h index cc4ad23c7b..7df49bb07d 100644 --- a/webrtc/pc/channelmanager.h +++ b/webrtc/pc/channelmanager.h @@ -94,6 +94,7 @@ class ChannelManager { const std::string& content_name, const std::string* bundle_transport_name, bool rtcp, + bool srtp_required, const AudioOptions& options); // Destroys a voice channel created with the Create API. void DestroyVoiceChannel(VoiceChannel* voice_channel); @@ -105,20 +106,17 @@ class ChannelManager { const std::string& content_name, const std::string* bundle_transport_name, bool rtcp, + bool srtp_required, const VideoOptions& options); // Destroys a video channel created with the Create API. void DestroyVideoChannel(VideoChannel* video_channel); - DataChannel* CreateDataChannel(TransportController* transport_controller, - const std::string& content_name, - const std::string* bundle_transport_name, - bool rtcp, - DataChannelType data_channel_type); DataChannel* CreateDataChannel( - TransportController* transport_controller, webrtc::MediaControllerInterface* media_controller, + TransportController* transport_controller, const std::string& content_name, const std::string* bundle_transport_name, bool rtcp, + bool srtp_required, DataChannelType data_channel_type); // Destroys a data channel created with the Create API. void DestroyDataChannel(DataChannel* data_channel); @@ -168,6 +166,7 @@ class ChannelManager { const std::string& content_name, const std::string* bundle_transport_name, bool rtcp, + bool srtp_required, const AudioOptions& options); void DestroyVoiceChannel_w(VoiceChannel* voice_channel); VideoChannel* CreateVideoChannel_w( @@ -176,15 +175,17 @@ class ChannelManager { const std::string& content_name, const std::string* bundle_transport_name, bool rtcp, + bool srtp_required, const VideoOptions& options); void DestroyVideoChannel_w(VideoChannel* video_channel); DataChannel* CreateDataChannel_w( + webrtc::MediaControllerInterface* media_controller, TransportController* transport_controller, const std::string& content_name, const std::string* bundle_transport_name, bool rtcp, - DataChannelType data_channel_type, - webrtc::MediaControllerInterface* media_controller); + bool srtp_required, + DataChannelType data_channel_type); void DestroyDataChannel_w(DataChannel* data_channel); std::unique_ptr media_engine_; diff --git a/webrtc/pc/channelmanager_unittest.cc b/webrtc/pc/channelmanager_unittest.cc index 4e14453c63..df3e9e5ba3 100644 --- a/webrtc/pc/channelmanager_unittest.cc +++ b/webrtc/pc/channelmanager_unittest.cc @@ -20,6 +20,11 @@ #include "webrtc/p2p/base/faketransportcontroller.h" #include "webrtc/pc/channelmanager.h" +namespace cricket { +const bool kDefaultRtcpEnabled = false; +const bool kDefaultSrtpRequired = true; +} + namespace cricket { static const AudioCodec kAudioCodecs[] = { @@ -98,16 +103,16 @@ TEST_F(ChannelManagerTest, StartupShutdownOnThread) { TEST_F(ChannelManagerTest, CreateDestroyChannels) { EXPECT_TRUE(cm_->Init()); cricket::VoiceChannel* voice_channel = cm_->CreateVoiceChannel( - &fake_mc_, transport_controller_, cricket::CN_AUDIO, nullptr, false, - AudioOptions()); + &fake_mc_, transport_controller_, cricket::CN_AUDIO, nullptr, + kDefaultRtcpEnabled, kDefaultSrtpRequired, AudioOptions()); EXPECT_TRUE(voice_channel != nullptr); cricket::VideoChannel* video_channel = cm_->CreateVideoChannel( - &fake_mc_, transport_controller_, cricket::CN_VIDEO, nullptr, false, - VideoOptions()); + &fake_mc_, transport_controller_, cricket::CN_VIDEO, nullptr, + kDefaultRtcpEnabled, kDefaultSrtpRequired, VideoOptions()); EXPECT_TRUE(video_channel != nullptr); - cricket::DataChannel* data_channel = - cm_->CreateDataChannel(transport_controller_, cricket::CN_DATA, nullptr, - false, cricket::DCT_RTP); + cricket::DataChannel* data_channel = cm_->CreateDataChannel( + &fake_mc_, transport_controller_, cricket::CN_DATA, nullptr, + kDefaultRtcpEnabled, kDefaultSrtpRequired, cricket::DCT_RTP); EXPECT_TRUE(data_channel != nullptr); cm_->DestroyVideoChannel(video_channel); cm_->DestroyVoiceChannel(voice_channel); @@ -126,16 +131,16 @@ TEST_F(ChannelManagerTest, CreateDestroyChannelsOnThread) { transport_controller_ = new cricket::FakeTransportController(&network_, ICEROLE_CONTROLLING); cricket::VoiceChannel* voice_channel = cm_->CreateVoiceChannel( - &fake_mc_, transport_controller_, cricket::CN_AUDIO, nullptr, false, - AudioOptions()); + &fake_mc_, transport_controller_, cricket::CN_AUDIO, nullptr, + kDefaultRtcpEnabled, kDefaultSrtpRequired, AudioOptions()); EXPECT_TRUE(voice_channel != nullptr); cricket::VideoChannel* video_channel = cm_->CreateVideoChannel( - &fake_mc_, transport_controller_, cricket::CN_VIDEO, nullptr, false, - VideoOptions()); + &fake_mc_, transport_controller_, cricket::CN_VIDEO, nullptr, + kDefaultRtcpEnabled, kDefaultSrtpRequired, VideoOptions()); EXPECT_TRUE(video_channel != nullptr); - cricket::DataChannel* data_channel = - cm_->CreateDataChannel(transport_controller_, cricket::CN_DATA, nullptr, - false, cricket::DCT_RTP); + cricket::DataChannel* data_channel = cm_->CreateDataChannel( + &fake_mc_, transport_controller_, cricket::CN_DATA, nullptr, + kDefaultRtcpEnabled, kDefaultSrtpRequired, cricket::DCT_RTP); EXPECT_TRUE(data_channel != nullptr); cm_->DestroyVideoChannel(video_channel); cm_->DestroyVoiceChannel(voice_channel); diff --git a/webrtc/pc/mediasession.cc b/webrtc/pc/mediasession.cc index fda38dfdff..2f973deb7f 100644 --- a/webrtc/pc/mediasession.cc +++ b/webrtc/pc/mediasession.cc @@ -749,9 +749,6 @@ static bool CreateMediaContentOffer( MediaContentDescriptionImpl* offer) { offer->AddCodecs(codecs); - if (secure_policy == SEC_REQUIRED) { - offer->set_crypto_required(CT_SDES); - } offer->set_rtcp_mux(options.rtcp_mux_enabled); if (offer->type() == cricket::MEDIA_TYPE_VIDEO) { offer->set_rtcp_reduced_size(true); @@ -777,7 +774,7 @@ static bool CreateMediaContentOffer( } #endif - if (offer->crypto_required() == CT_SDES && offer->cryptos().empty()) { + if (secure_policy == SEC_REQUIRED && offer->cryptos().empty()) { return false; } return true; @@ -1068,8 +1065,7 @@ static bool CreateMediaContentAnswer( } } - if (answer->cryptos().empty() && - (offer->crypto_required() == CT_SDES || sdes_policy == SEC_REQUIRED)) { + if (answer->cryptos().empty() && sdes_policy == SEC_REQUIRED) { return false; } diff --git a/webrtc/pc/mediasession_unittest.cc b/webrtc/pc/mediasession_unittest.cc index 4d6418159a..45de0d2b1a 100644 --- a/webrtc/pc/mediasession_unittest.cc +++ b/webrtc/pc/mediasession_unittest.cc @@ -26,13 +26,10 @@ #ifdef HAVE_SRTP #define ASSERT_CRYPTO(cd, s, cs) \ - ASSERT_EQ(cricket::CT_NONE, cd->crypto_required()); \ ASSERT_EQ(s, cd->cryptos().size()); \ ASSERT_EQ(std::string(cs), cd->cryptos()[0].cipher_suite) #else -#define ASSERT_CRYPTO(cd, s, cs) \ - ASSERT_EQ(cricket::CT_NONE, cd->crypto_required()); \ - ASSERT_EQ(0U, cd->cryptos().size()); +#define ASSERT_CRYPTO(cd, s, cs) ASSERT_EQ(0, cd->cryptos().size()); #endif typedef std::vector Candidates;