diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc index a736fa2edf..7fafad7114 100644 --- a/talk/app/webrtc/peerconnection.cc +++ b/talk/app/webrtc/peerconnection.cc @@ -545,6 +545,7 @@ void PeerConnection::OnSessionStateChange(cricket::BaseSession* /*session*/, switch (state) { case cricket::BaseSession::STATE_INIT: ChangeSignalingState(PeerConnectionInterface::kStable); + break; case cricket::BaseSession::STATE_SENTINITIATE: ChangeSignalingState(PeerConnectionInterface::kHaveLocalOffer); break; diff --git a/talk/app/webrtc/peerconnection_unittest.cc b/talk/app/webrtc/peerconnection_unittest.cc index 522d528020..44f76ca2fd 100644 --- a/talk/app/webrtc/peerconnection_unittest.cc +++ b/talk/app/webrtc/peerconnection_unittest.cc @@ -633,12 +633,24 @@ class JsepTestClient } void SetReceiveAudioVideo(bool audio, bool video) { - session_description_constraints_.SetMandatoryReceiveAudio(audio); - session_description_constraints_.SetMandatoryReceiveVideo(video); + SetReceiveAudio(audio); + SetReceiveVideo(video); ASSERT_EQ(audio, can_receive_audio()); ASSERT_EQ(video, can_receive_video()); } + void SetReceiveAudio(bool audio) { + if (audio && can_receive_audio()) + return; + session_description_constraints_.SetMandatoryReceiveAudio(audio); + } + + void SetReceiveVideo(bool video) { + if (video && can_receive_video()) + return; + session_description_constraints_.SetMandatoryReceiveVideo(video); + } + void RemoveMsidFromReceivedSdp(bool remove) { remove_msid_ = remove; } @@ -1047,6 +1059,20 @@ TEST_F(JsepPeerConnectionP2PTestClient, LocalP2PTestDtls) { VerifyRenderedSize(640, 480); } +// This test sets up a audio call initially and then upgrades to audio/video, +// using DTLS. +TEST_F(JsepPeerConnectionP2PTestClient, LocalP2PTestDtlsRenegotiate) { + MAYBE_SKIP_TEST(talk_base::SSLStreamAdapter::HaveDtlsSrtp); + FakeConstraints setup_constraints; + setup_constraints.AddMandatory(MediaConstraintsInterface::kEnableDtlsSrtp, + true); + ASSERT_TRUE(CreateTestClients(&setup_constraints, &setup_constraints)); + receiving_client()->SetReceiveAudioVideo(true, false); + LocalP2PTest(); + receiving_client()->SetReceiveAudioVideo(true, true); + receiving_client()->Negotiate(); +} + // This test sets up a call between an endpoint configured to use either SDES or // DTLS (the offerer) and just SDES (the answerer). As a result, SDES is used // instead of DTLS. diff --git a/talk/app/webrtc/peerconnectioninterface_unittest.cc b/talk/app/webrtc/peerconnectioninterface_unittest.cc index 7aa06ef424..b1d3544840 100644 --- a/talk/app/webrtc/peerconnectioninterface_unittest.cc +++ b/talk/app/webrtc/peerconnectioninterface_unittest.cc @@ -1095,7 +1095,7 @@ TEST_F(PeerConnectionInterfaceTest, TestRejectDataChannelInAnswer) { // Test that we can create a session description from an SDP string from // FireFox, use it as a remote session description, generate an answer and use // the answer as a local description. -TEST_F(PeerConnectionInterfaceTest, ReceiveFireFoxOffer) { +TEST_F(PeerConnectionInterfaceTest, DISABLED_ReceiveFireFoxOffer) { MAYBE_SKIP_TEST(talk_base::SSLStreamAdapter::HaveDtlsSrtp); FakeConstraints constraints; constraints.AddMandatory(webrtc::MediaConstraintsInterface::kEnableDtlsSrtp, diff --git a/talk/app/webrtc/statscollector.cc b/talk/app/webrtc/statscollector.cc index f5ff708a02..42ae662c1c 100644 --- a/talk/app/webrtc/statscollector.cc +++ b/talk/app/webrtc/statscollector.cc @@ -68,6 +68,8 @@ const char StatsReport::kStatsValueNameEchoReturnLossEnhancement[] = "googEchoCancellationReturnLossEnhancement"; const char StatsReport::kStatsValueNameFingerprint[] = "googFingerprint"; +const char StatsReport::kStatsValueNameFingerprintAlgorithm[] = + "googFingerprintAlgorithm"; const char StatsReport::kStatsValueNameFirsReceived[] = "googFirsReceived"; const char StatsReport::kStatsValueNameFirsSent[] = "googFirsSent"; const char StatsReport::kStatsValueNameFrameHeightReceived[] = @@ -449,8 +451,13 @@ std::string StatsCollector::AddOneCertificateReport( // TODO(bemasc): Move this computation to a helper class that caches these // values to reduce CPU use in GetStats. This will require adding a fast // SSLCertificate::Equals() method to detect certificate changes. + + std::string digest_algorithm; + if (!cert->GetSignatureDigestAlgorithm(&digest_algorithm)) + return std::string(); + talk_base::scoped_ptr ssl_fingerprint( - talk_base::SSLFingerprint::Create(talk_base::DIGEST_SHA_256, cert)); + talk_base::SSLFingerprint::Create(digest_algorithm, cert)); std::string fingerprint = ssl_fingerprint->GetRfc4572Fingerprint(); talk_base::Buffer der_buffer; @@ -464,6 +471,8 @@ std::string StatsCollector::AddOneCertificateReport( report.id = StatsId(report.type, fingerprint); report.timestamp = stats_gathering_started_; report.AddValue(StatsReport::kStatsValueNameFingerprint, fingerprint); + report.AddValue(StatsReport::kStatsValueNameFingerprintAlgorithm, + digest_algorithm); report.AddValue(StatsReport::kStatsValueNameDer, der_base64); if (!issuer_id.empty()) report.AddValue(StatsReport::kStatsValueNameIssuerId, issuer_id); diff --git a/talk/app/webrtc/statscollector_unittest.cc b/talk/app/webrtc/statscollector_unittest.cc index 982983217c..d9cde5d83a 100644 --- a/talk/app/webrtc/statscollector_unittest.cc +++ b/talk/app/webrtc/statscollector_unittest.cc @@ -168,12 +168,29 @@ void CheckCertChainReports(const webrtc::StatsReports& reports, while (true) { const webrtc::StatsReport* report = FindReportById(reports, certificate_id); ASSERT_TRUE(report != NULL); + std::string der_base64; EXPECT_TRUE(GetValue( report, webrtc::StatsReport::kStatsValueNameDer, &der_base64)); std::string der = talk_base::Base64::Decode(der_base64, talk_base::Base64::DO_STRICT); EXPECT_EQ(ders[i], der); + + std::string fingerprint_algorithm; + EXPECT_TRUE(GetValue( + report, + webrtc::StatsReport::kStatsValueNameFingerprintAlgorithm, + &fingerprint_algorithm)); + // The digest algorithm for a FakeSSLCertificate is always SHA-1. + std::string sha_1_str = talk_base::DIGEST_SHA_1; + EXPECT_EQ(sha_1_str, fingerprint_algorithm); + + std::string dummy_fingerprint; // Value is not checked. + EXPECT_TRUE(GetValue( + report, + webrtc::StatsReport::kStatsValueNameFingerprint, + &dummy_fingerprint)); + ++i; if (!GetValue( report, webrtc::StatsReport::kStatsValueNameIssuerId, &certificate_id)) @@ -560,7 +577,7 @@ TEST_F(StatsCollectorTest, TransportObjectLinkedFromSsrcObject) { // This test verifies that all chained certificates are correctly // reported -TEST_F(StatsCollectorTest, DISABLED_ChainedCertificateReportsCreated) { +TEST_F(StatsCollectorTest, ChainedCertificateReportsCreated) { // Build local certificate chain. std::vector local_ders(5); local_ders[0] = "These"; @@ -583,7 +600,7 @@ TEST_F(StatsCollectorTest, DISABLED_ChainedCertificateReportsCreated) { // This test verifies that all certificates without chains are correctly // reported. -TEST_F(StatsCollectorTest, DISABLED_ChainlessCertificateReportsCreated) { +TEST_F(StatsCollectorTest, ChainlessCertificateReportsCreated) { // Build local certificate. std::string local_der = "This is the local der."; talk_base::FakeSSLCertificate local_cert(DerToPem(local_der)); @@ -598,7 +615,7 @@ TEST_F(StatsCollectorTest, DISABLED_ChainlessCertificateReportsCreated) { // This test verifies that the stats are generated correctly when no // transport is present. -TEST_F(StatsCollectorTest, DISABLED_NoTransport) { +TEST_F(StatsCollectorTest, NoTransport) { webrtc::StatsCollector stats; // Implementation under test. webrtc::StatsReports reports; // returned values. stats.set_session(&session_); @@ -644,7 +661,7 @@ TEST_F(StatsCollectorTest, DISABLED_NoTransport) { // This test verifies that the stats are generated correctly when the transport // does not have any certificates. -TEST_F(StatsCollectorTest, DISABLED_NoCertificates) { +TEST_F(StatsCollectorTest, NoCertificates) { webrtc::StatsCollector stats; // Implementation under test. webrtc::StatsReports reports; // returned values. stats.set_session(&session_); diff --git a/talk/app/webrtc/statstypes.h b/talk/app/webrtc/statstypes.h index fe368859fc..fb8c15b8ef 100644 --- a/talk/app/webrtc/statstypes.h +++ b/talk/app/webrtc/statstypes.h @@ -162,6 +162,7 @@ class StatsReport { static const char kStatsValueNameTypingNoiseState[]; static const char kStatsValueNameDer[]; static const char kStatsValueNameFingerprint[]; + static const char kStatsValueNameFingerprintAlgorithm[]; static const char kStatsValueNameIssuerId[]; static const char kStatsValueNameLocalCertificateId[]; static const char kStatsValueNameRemoteCertificateId[]; diff --git a/talk/app/webrtc/test/fakeconstraints.h b/talk/app/webrtc/test/fakeconstraints.h index 0299afa83c..927432da8b 100644 --- a/talk/app/webrtc/test/fakeconstraints.h +++ b/talk/app/webrtc/test/fakeconstraints.h @@ -54,21 +54,36 @@ class FakeConstraints : public webrtc::MediaConstraintsInterface { mandatory_.push_back(Constraint(key, talk_base::ToString(value))); } + template + void SetMandatory(const std::string& key, const T& value) { + std::string value_str; + if (mandatory_.FindFirst(key, &value_str)) { + for (Constraints::iterator iter = mandatory_.begin(); + iter != mandatory_.end(); ++iter) { + if (iter->key == key) { + mandatory_.erase(iter); + break; + } + } + } + mandatory_.push_back(Constraint(key, talk_base::ToString(value))); + } + template void AddOptional(const std::string& key, const T& value) { optional_.push_back(Constraint(key, talk_base::ToString(value))); } void SetMandatoryMinAspectRatio(double ratio) { - AddMandatory(MediaConstraintsInterface::kMinAspectRatio, ratio); + SetMandatory(MediaConstraintsInterface::kMinAspectRatio, ratio); } void SetMandatoryMinWidth(int width) { - AddMandatory(MediaConstraintsInterface::kMinWidth, width); + SetMandatory(MediaConstraintsInterface::kMinWidth, width); } void SetMandatoryMinHeight(int height) { - AddMandatory(MediaConstraintsInterface::kMinHeight, height); + SetMandatory(MediaConstraintsInterface::kMinHeight, height); } void SetOptionalMaxWidth(int width) { @@ -76,27 +91,27 @@ class FakeConstraints : public webrtc::MediaConstraintsInterface { } void SetMandatoryMaxFrameRate(int frame_rate) { - AddMandatory(MediaConstraintsInterface::kMaxFrameRate, frame_rate); + SetMandatory(MediaConstraintsInterface::kMaxFrameRate, frame_rate); } void SetMandatoryReceiveAudio(bool enable) { - AddMandatory(MediaConstraintsInterface::kOfferToReceiveAudio, enable); + SetMandatory(MediaConstraintsInterface::kOfferToReceiveAudio, enable); } void SetMandatoryReceiveVideo(bool enable) { - AddMandatory(MediaConstraintsInterface::kOfferToReceiveVideo, enable); + SetMandatory(MediaConstraintsInterface::kOfferToReceiveVideo, enable); } void SetMandatoryUseRtpMux(bool enable) { - AddMandatory(MediaConstraintsInterface::kUseRtpMux, enable); + SetMandatory(MediaConstraintsInterface::kUseRtpMux, enable); } void SetMandatoryIceRestart(bool enable) { - AddMandatory(MediaConstraintsInterface::kIceRestart, enable); + SetMandatory(MediaConstraintsInterface::kIceRestart, enable); } void SetAllowRtpDataChannels() { - AddMandatory(MediaConstraintsInterface::kEnableRtpDataChannels, true); + SetMandatory(MediaConstraintsInterface::kEnableRtpDataChannels, true); } void SetOptionalVAD(bool enable) { @@ -104,8 +119,8 @@ class FakeConstraints : public webrtc::MediaConstraintsInterface { } void SetAllowDtlsSctpDataChannels() { - AddMandatory(MediaConstraintsInterface::kEnableSctpDataChannels, true); - AddMandatory(MediaConstraintsInterface::kEnableDtlsSrtp, true); + SetMandatory(MediaConstraintsInterface::kEnableSctpDataChannels, true); + SetMandatory(MediaConstraintsInterface::kEnableDtlsSrtp, true); } private: diff --git a/talk/app/webrtc/webrtcsdp.cc b/talk/app/webrtc/webrtcsdp.cc index ae171178e4..610250ead5 100644 --- a/talk/app/webrtc/webrtcsdp.cc +++ b/talk/app/webrtc/webrtcsdp.cc @@ -1241,12 +1241,9 @@ void BuildMediaDescription(const ContentInfo* content_info, // Making sure we are not using "passive" mode. cricket::ConnectionRole role = transport_info->description.connection_role; - ASSERT(role == cricket::CONNECTIONROLE_ACTIVE || - role == cricket::CONNECTIONROLE_ACTPASS); + std::string dtls_role_str; + VERIFY(cricket::ConnectionRoleToString(role, &dtls_role_str)); InitAttrLine(kAttributeSetup, &os); - std::string dtls_role_str = role == cricket::CONNECTIONROLE_ACTPASS ? - cricket::CONNECTIONROLE_ACTPASS_STR : - cricket::CONNECTIONROLE_ACTIVE_STR; os << kSdpDelimiterColon << dtls_role_str; AddLine(os.str(), message); } diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc index 04cbe029f8..c7805c16d4 100644 --- a/talk/app/webrtc/webrtcsession.cc +++ b/talk/app/webrtc/webrtcsession.cc @@ -85,7 +85,9 @@ const char kMlineMismatch[] = "Rejecting answer."; const char kSdpWithoutCrypto[] = "Called with a SDP without crypto enabled."; const char kSdpWithoutSdesAndDtlsDisabled[] = - "Called with a SDP without SDES crypto and DTLS disabled locally."; + "Called with an SDP without SDES crypto and DTLS disabled locally."; +const char kSdpWithoutIceUfragPwd[] = + "Called with an SDP without ice-ufrag and ice-pwd."; const char kSessionError[] = "Session error code: "; const char kUpdateStateFailed[] = "Failed to update session state: "; const char kPushDownOfferTDFailed[] = @@ -154,6 +156,31 @@ static bool VerifyCrypto(const SessionDescription* desc, return true; } +// Checks that each non-rejected content has ice-ufrag and ice-pwd set. +static bool VerifyIceUfragPwdPresent(const SessionDescription* desc) { + const ContentInfos& contents = desc->contents(); + for (size_t index = 0; index < contents.size(); ++index) { + const ContentInfo* cinfo = &contents[index]; + if (cinfo->rejected) { + continue; + } + + // If the content isn't rejected, ice-ufrag and ice-pwd must be present. + const TransportInfo* tinfo = desc->GetTransportInfoByName(cinfo->name); + if (!tinfo) { + // Something is not right. + LOG(LS_ERROR) << kInvalidSdp; + return false; + } + if (tinfo->description.ice_ufrag.empty() || + tinfo->description.ice_pwd.empty()) { + LOG(LS_ERROR) << "Session description must have ice ufrag and pwd."; + return false; + } + } + 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. @@ -1446,6 +1473,11 @@ bool WebRtcSession::ValidateSessionDescription( return BadSdp(source, crypto_error, error_desc); } + // Verify ice-ufrag and ice-pwd. + if (!VerifyIceUfragPwdPresent(sdesc->description())) { + return BadSdp(source, kSdpWithoutIceUfragPwd, error_desc); + } + if (!ValidateBundleSettings(sdesc->description())) { return BadSdp(source, kBundleWithoutRtcpMux, error_desc); } diff --git a/talk/app/webrtc/webrtcsession.h b/talk/app/webrtc/webrtcsession.h index e4610283c3..be8979801c 100644 --- a/talk/app/webrtc/webrtcsession.h +++ b/talk/app/webrtc/webrtcsession.h @@ -69,6 +69,7 @@ extern const char kInvalidSdp[]; extern const char kMlineMismatch[]; extern const char kSdpWithoutCrypto[]; extern const char kSdpWithoutSdesAndDtlsDisabled[]; +extern const char kSdpWithoutIceUfragPwd[]; extern const char kSessionError[]; extern const char kUpdateStateFailed[]; extern const char kPushDownOfferTDFailed[]; diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc index c47fb177f7..493294fcb1 100644 --- a/talk/app/webrtc/webrtcsession_unittest.cc +++ b/talk/app/webrtc/webrtcsession_unittest.cc @@ -42,6 +42,7 @@ #include "talk/base/logging.h" #include "talk/base/network.h" #include "talk/base/physicalsocketserver.h" +#include "talk/base/ssladapter.h" #include "talk/base/sslstreamadapter.h" #include "talk/base/stringutils.h" #include "talk/base/thread.h" @@ -87,6 +88,7 @@ using webrtc::WebRtcSession; using webrtc::kMlineMismatch; using webrtc::kSdpWithoutCrypto; using webrtc::kSdpWithoutSdesAndDtlsDisabled; +using webrtc::kSdpWithoutIceUfragPwd; using webrtc::kSessionError; using webrtc::kSetLocalSdpFailed; using webrtc::kSetRemoteSdpFailed; @@ -290,6 +292,14 @@ class WebRtcSessionTest : public testing::Test { desc_factory_->set_add_legacy_streams(false); } + static void SetUpTestCase() { + talk_base::InitializeSSL(); + } + + static void TearDownTestCase() { + talk_base::CleanupSSL(); + } + void AddInterface(const SocketAddress& addr) { network_manager_.AddInterface(addr); } @@ -436,7 +446,6 @@ class WebRtcSessionTest : public testing::Test { talk_base::ToString(talk_base::CreateRandomId()); identity_.reset(talk_base::SSLIdentity::Generate(identity_name)); tdesc_factory_->set_identity(identity_.get()); - tdesc_factory_->set_digest_algorithm(talk_base::DIGEST_SHA_256); tdesc_factory_->set_secure(cricket::SEC_REQUIRED); } @@ -445,17 +454,9 @@ class WebRtcSessionTest : public testing::Test { const TransportInfo* audio = sdp->GetTransportInfoByName("audio"); ASSERT_TRUE(audio != NULL); ASSERT_EQ(expected, audio->description.identity_fingerprint.get() != NULL); - if (expected) { - ASSERT_EQ(std::string(talk_base::DIGEST_SHA_256), audio->description. - identity_fingerprint->algorithm); - } const TransportInfo* video = sdp->GetTransportInfoByName("video"); ASSERT_TRUE(video != NULL); ASSERT_EQ(expected, video->description.identity_fingerprint.get() != NULL); - if (expected) { - ASSERT_EQ(std::string(talk_base::DIGEST_SHA_256), video->description. - identity_fingerprint->algorithm); - } } void VerifyAnswerFromNonCryptoOffer() { @@ -515,6 +516,31 @@ class WebRtcSessionTest : public testing::Test { } EXPECT_TRUE(expect_equal); } + + void RemoveIceUfragPwdLines(const SessionDescriptionInterface* current_desc, + std::string *sdp) { + const cricket::SessionDescription* desc = current_desc->description(); + EXPECT_TRUE(current_desc->ToString(sdp)); + + const cricket::ContentInfos& contents = desc->contents(); + cricket::ContentInfos::const_iterator it = contents.begin(); + // Replace ufrag and pwd lines with empty strings. + for (; it != contents.end(); ++it) { + const cricket::TransportDescription* transport_desc = + desc->GetTransportDescriptionByName(it->name); + std::string ufrag_line = "a=ice-ufrag:" + transport_desc->ice_ufrag + + "\r\n"; + std::string pwd_line = "a=ice-pwd:" + transport_desc->ice_pwd + + "\r\n"; + talk_base::replace_substrs(ufrag_line.c_str(), ufrag_line.length(), + "", 0, + sdp); + talk_base::replace_substrs(pwd_line.c_str(), pwd_line.length(), + "", 0, + sdp); + } + } + // Creates a remote offer and and applies it as a remote description, // creates a local answer and applies is as a local description. // Call mediastream_signaling_.UseOptionsWithStreamX() before this function @@ -609,6 +635,35 @@ class WebRtcSessionTest : public testing::Test { kSessionVersion, current_desc); } + JsepSessionDescription* CreateRemoteOfferWithSctpPort( + const char* sctp_stream_name, int new_port, + cricket::MediaSessionOptions options) { + options.data_channel_type = cricket::DCT_SCTP; + options.AddStream(cricket::MEDIA_TYPE_DATA, "datachannel", + sctp_stream_name); + return ChangeSDPSctpPort(new_port, CreateRemoteOffer(options)); + } + + // Takes ownership of offer_basis (and deletes it). + JsepSessionDescription* ChangeSDPSctpPort( + int new_port, webrtc::SessionDescriptionInterface *offer_basis) { + // Stringify the input SDP, swap the 5000 for 'new_port' and create a new + // SessionDescription from the mutated string. + const char* default_port_str = "5000"; + char new_port_str[16]; + talk_base::sprintfn(new_port_str, sizeof(new_port_str), "%d", new_port); + std::string offer_str; + offer_basis->ToString(&offer_str); + talk_base::replace_substrs(default_port_str, strlen(default_port_str), + new_port_str, strlen(new_port_str), + &offer_str); + JsepSessionDescription* offer = new JsepSessionDescription( + offer_basis->type()); + delete offer_basis; + offer->Initialize(offer_str, NULL); + return offer; + } + // Create a remote offer. Call mediastream_signaling_.UseOptionsWithStreamX() // before this function to decide which streams to create. JsepSessionDescription* CreateRemoteOffer() { @@ -1897,6 +1952,31 @@ TEST_F(WebRtcSessionTest, VerifyAnswerFromCryptoOffer) { VerifyAnswerFromCryptoOffer(); } +// This test verifies that setLocalDescription fails if +// no a=ice-ufrag and a=ice-pwd lines are present in the SDP. +TEST_F(WebRtcSessionTest, TestSetLocalDescriptionWithoutIce) { + Init(NULL); + mediastream_signaling_.SendAudioVideoStream1(); + talk_base::scoped_ptr offer(CreateOffer(NULL)); + std::string sdp; + RemoveIceUfragPwdLines(offer.get(), &sdp); + SessionDescriptionInterface* modified_offer = + CreateSessionDescription(JsepSessionDescription::kOffer, sdp, NULL); + SetLocalDescriptionExpectError(kSdpWithoutIceUfragPwd, modified_offer); +} + +// This test verifies that setRemoteDescription fails if +// no a=ice-ufrag and a=ice-pwd lines are present in the SDP. +TEST_F(WebRtcSessionTest, TestSetRemoteDescriptionWithoutIce) { + Init(NULL); + talk_base::scoped_ptr offer(CreateRemoteOffer()); + std::string sdp; + RemoveIceUfragPwdLines(offer.get(), &sdp); + SessionDescriptionInterface* modified_offer = + CreateSessionDescription(JsepSessionDescription::kOffer, sdp, NULL); + SetRemoteDescriptionExpectError(kSdpWithoutIceUfragPwd, modified_offer); +} + TEST_F(WebRtcSessionTest, VerifyBundleFlagInPA) { // This test verifies BUNDLE flag in PortAllocator, if BUNDLE information in // local description is removed by the application, BUNDLE flag should be @@ -2554,6 +2634,64 @@ TEST_F(WebRtcSessionTest, TestSctpDataChannelWithDtls) { EXPECT_EQ(cricket::DCT_SCTP, data_engine_->last_channel_type()); } +TEST_F(WebRtcSessionTest, TestSctpDataChannelSendPortParsing) { + MAYBE_SKIP_TEST(talk_base::SSLStreamAdapter::HaveDtlsSrtp); + const int new_send_port = 9998; + const int new_recv_port = 7775; + + constraints_.reset(new FakeConstraints()); + constraints_->AddOptional( + webrtc::MediaConstraintsInterface::kEnableSctpDataChannels, true); + + InitWithDtls(false); + SetFactoryDtlsSrtp(); + + // By default, don't actually add the codecs to desc_factory_; they don't + // actually get serialized for SCTP in BuildMediaDescription(). Instead, + // let the session description get parsed. That'll get the proper codecs + // into the stream. + cricket::MediaSessionOptions options; + JsepSessionDescription* offer = CreateRemoteOfferWithSctpPort( + "stream1", new_send_port, options); + + // SetRemoteDescription will take the ownership of the offer. + SetRemoteDescriptionWithoutError(offer); + + SessionDescriptionInterface* answer = ChangeSDPSctpPort( + new_recv_port, CreateAnswer(NULL)); + ASSERT_TRUE(answer != NULL); + + // Now set the local description, which'll take ownership of the answer. + SetLocalDescriptionWithoutError(answer); + + // TEST PLAN: Set the port number to something new, set it in the SDP, + // and pass it all the way down. + webrtc::DataChannelInit dci; + dci.reliable = true; + EXPECT_EQ(cricket::DCT_SCTP, data_engine_->last_channel_type()); + talk_base::scoped_refptr dc = + session_->CreateDataChannel("datachannel", &dci); + + cricket::FakeDataMediaChannel* ch = data_engine_->GetChannel(0); + int portnum = -1; + ASSERT_TRUE(ch != NULL); + ASSERT_EQ(1UL, ch->send_codecs().size()); + EXPECT_EQ(cricket::kGoogleSctpDataCodecId, ch->send_codecs()[0].id); + EXPECT_TRUE(!strcmp(cricket::kGoogleSctpDataCodecName, + ch->send_codecs()[0].name.c_str())); + EXPECT_TRUE(ch->send_codecs()[0].GetParam(cricket::kCodecParamPort, + &portnum)); + EXPECT_EQ(new_send_port, portnum); + + ASSERT_EQ(1UL, ch->recv_codecs().size()); + EXPECT_EQ(cricket::kGoogleSctpDataCodecId, ch->recv_codecs()[0].id); + EXPECT_TRUE(!strcmp(cricket::kGoogleSctpDataCodecName, + ch->recv_codecs()[0].name.c_str())); + EXPECT_TRUE(ch->recv_codecs()[0].GetParam(cricket::kCodecParamPort, + &portnum)); + EXPECT_EQ(new_recv_port, portnum); +} + // Verifies that CreateOffer succeeds when CreateOffer is called before async // identity generation is finished. TEST_F(WebRtcSessionTest, TestCreateOfferBeforeIdentityRequestReturnSuccess) { diff --git a/talk/app/webrtc/webrtcsessiondescriptionfactory.cc b/talk/app/webrtc/webrtcsessiondescriptionfactory.cc index 30c49a718a..51427d2f5c 100644 --- a/talk/app/webrtc/webrtcsessiondescriptionfactory.cc +++ b/talk/app/webrtc/webrtcsessiondescriptionfactory.cc @@ -438,7 +438,6 @@ void WebRtcSessionDescriptionFactory::SetIdentity( SignalIdentityReady(identity); transport_desc_factory_.set_identity(identity); - transport_desc_factory_.set_digest_algorithm(talk_base::DIGEST_SHA_256); transport_desc_factory_.set_secure(cricket::SEC_ENABLED); while (!create_session_description_requests_.empty()) { diff --git a/talk/base/fakesslidentity.h b/talk/base/fakesslidentity.h index 5efa268f8f..203bb83bf0 100644 --- a/talk/base/fakesslidentity.h +++ b/talk/base/fakesslidentity.h @@ -58,6 +58,12 @@ class FakeSSLCertificate : public talk_base::SSLCertificate { VERIFY(SSLIdentity::PemToDer(kPemTypeCertificate, data_, &der_string)); der_buffer->SetData(der_string.c_str(), der_string.size()); } + virtual bool GetSignatureDigestAlgorithm(std::string* algorithm) const { + // SHA-1 is chosen because it is available in all build configurations + // used for unit testing. + *algorithm = DIGEST_SHA_1; + return true; + } virtual bool ComputeDigest(const std::string &algorithm, unsigned char *digest, std::size_t size, std::size_t *length) const { diff --git a/talk/base/nssidentity.cc b/talk/base/nssidentity.cc index 96bfcc3b09..053035e562 100644 --- a/talk/base/nssidentity.cc +++ b/talk/base/nssidentity.cc @@ -175,6 +175,54 @@ bool NSSCertificate::GetDigestLength(const std::string &algorithm, return true; } +bool NSSCertificate::GetSignatureDigestAlgorithm(std::string* algorithm) const { + // The function sec_DecodeSigAlg in NSS provides this mapping functionality. + // Unfortunately it is private, so the functionality must be duplicated here. + // See https://bugzilla.mozilla.org/show_bug.cgi?id=925165 . + SECOidTag sig_alg = SECOID_GetAlgorithmTag(&certificate_->signature); + switch (sig_alg) { + case SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION: + *algorithm = DIGEST_MD5; + break; + case SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION: + case SEC_OID_ISO_SHA_WITH_RSA_SIGNATURE: + case SEC_OID_ISO_SHA1_WITH_RSA_SIGNATURE: + case SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST: + case SEC_OID_BOGUS_DSA_SIGNATURE_WITH_SHA1_DIGEST: + case SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE: + case SEC_OID_MISSI_DSS: + case SEC_OID_MISSI_KEA_DSS: + case SEC_OID_MISSI_KEA_DSS_OLD: + case SEC_OID_MISSI_DSS_OLD: + *algorithm = DIGEST_SHA_1; + break; + case SEC_OID_ANSIX962_ECDSA_SHA224_SIGNATURE: + case SEC_OID_PKCS1_SHA224_WITH_RSA_ENCRYPTION: + case SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA224_DIGEST: + *algorithm = DIGEST_SHA_224; + break; + case SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE: + case SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION: + case SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST: + *algorithm = DIGEST_SHA_256; + break; + case SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE: + case SEC_OID_PKCS1_SHA384_WITH_RSA_ENCRYPTION: + *algorithm = DIGEST_SHA_384; + break; + case SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE: + case SEC_OID_PKCS1_SHA512_WITH_RSA_ENCRYPTION: + *algorithm = DIGEST_SHA_512; + break; + default: + // Unknown algorithm. There are several unhandled options that are less + // common and more complex. + algorithm->clear(); + return false; + } + return true; +} + bool NSSCertificate::ComputeDigest(const std::string &algorithm, unsigned char *digest, std::size_t size, std::size_t *length) const { diff --git a/talk/base/nssidentity.h b/talk/base/nssidentity.h index f4bfc8bcc1..3f97ebbb61 100644 --- a/talk/base/nssidentity.h +++ b/talk/base/nssidentity.h @@ -81,6 +81,8 @@ class NSSCertificate : public SSLCertificate { virtual void ToDER(Buffer* der_buffer) const; + virtual bool GetSignatureDigestAlgorithm(std::string* algorithm) const; + virtual bool ComputeDigest(const std::string& algorithm, unsigned char* digest, std::size_t size, std::size_t* length) const; diff --git a/talk/base/openssldigest.cc b/talk/base/openssldigest.cc index bb0e027cfe..3d9276de84 100644 --- a/talk/base/openssldigest.cc +++ b/talk/base/openssldigest.cc @@ -98,6 +98,34 @@ bool OpenSSLDigest::GetDigestEVP(const std::string& algorithm, return true; } +bool OpenSSLDigest::GetDigestName(const EVP_MD* md, + std::string* algorithm) { + ASSERT(md != NULL); + ASSERT(algorithm != NULL); + + int md_type = EVP_MD_type(md); + if (md_type == NID_md5) { + *algorithm = DIGEST_MD5; + } else if (md_type == NID_sha1) { + *algorithm = DIGEST_SHA_1; +#if OPENSSL_VERSION_NUMBER >= 0x00908000L + } else if (md_type == NID_sha224) { + *algorithm = DIGEST_SHA_224; + } else if (md_type == NID_sha256) { + *algorithm = DIGEST_SHA_256; + } else if (md_type == NID_sha384) { + *algorithm = DIGEST_SHA_384; + } else if (md_type == NID_sha512) { + *algorithm = DIGEST_SHA_512; +#endif + } else { + algorithm->clear(); + return false; + } + + return true; +} + bool OpenSSLDigest::GetDigestSize(const std::string& algorithm, size_t* length) { const EVP_MD *md; diff --git a/talk/base/openssldigest.h b/talk/base/openssldigest.h index 0a12189d0a..d5cf878786 100644 --- a/talk/base/openssldigest.h +++ b/talk/base/openssldigest.h @@ -2,26 +2,26 @@ * libjingle * Copyright 2004--2012, Google Inc. * - * Redistribution and use in source and binary forms, with or without + * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are met: * - * 1. Redistributions of source code must retain the above copyright notice, + * 1. Redistributions of source code must retain the above copyright notice, * this list of conditions and the following disclaimer. * 2. Redistributions in binary form must reproduce the above copyright notice, * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. - * 3. The name of the author may not be used to endorse or promote products + * 3. The name of the author may not be used to endorse or promote products * derived from this software without specific prior written permission. * * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR IMPLIED - * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF + * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO - * EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, - * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR - * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR + * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ @@ -47,9 +47,12 @@ class OpenSSLDigest : public MessageDigest { // Outputs the digest value to |buf| with length |len|. virtual size_t Finish(void* buf, size_t len); - // Helper function to look up a digest. + // Helper function to look up a digest's EVP by name. static bool GetDigestEVP(const std::string &algorithm, const EVP_MD** md); + // Helper function to look up a digest's name by EVP. + static bool GetDigestName(const EVP_MD* md, + std::string* algorithm); // Helper function to get the length of a digest. static bool GetDigestSize(const std::string &algorithm, size_t* len); diff --git a/talk/base/opensslidentity.cc b/talk/base/opensslidentity.cc index 7408af1ab6..4ff7601618 100644 --- a/talk/base/opensslidentity.cc +++ b/talk/base/opensslidentity.cc @@ -235,6 +235,14 @@ OpenSSLCertificate* OpenSSLCertificate::FromPEMString( return ret; } +// NOTE: This implementation only functions correctly after InitializeSSL +// and before CleanupSSL. +bool OpenSSLCertificate::GetSignatureDigestAlgorithm( + std::string* algorithm) const { + return OpenSSLDigest::GetDigestName( + EVP_get_digestbyobj(x509_->sig_alg->algorithm), algorithm); +} + bool OpenSSLCertificate::ComputeDigest(const std::string &algorithm, unsigned char *digest, std::size_t size, diff --git a/talk/base/opensslidentity.h b/talk/base/opensslidentity.h index 0d1bf73b07..af18c5c4d0 100644 --- a/talk/base/opensslidentity.h +++ b/talk/base/opensslidentity.h @@ -105,6 +105,8 @@ class OpenSSLCertificate : public SSLCertificate { std::size_t size, std::size_t *length); + virtual bool GetSignatureDigestAlgorithm(std::string* algorithm) const; + virtual bool GetChain(SSLCertChain** chain) const { // Chains are not yet supported when using OpenSSL. // OpenSSLStreamAdapter::SSLVerifyCallback currently requires the remote diff --git a/talk/base/sslidentity.h b/talk/base/sslidentity.h index 345691c398..89b1008983 100644 --- a/talk/base/sslidentity.h +++ b/talk/base/sslidentity.h @@ -76,6 +76,10 @@ class SSLCertificate { // Provides a DER encoded binary representation of the certificate. virtual void ToDER(Buffer* der_buffer) const = 0; + // Gets the name of the digest algorithm that was used to compute this + // certificate's signature. + virtual bool GetSignatureDigestAlgorithm(std::string* algorithm) const = 0; + // Compute the digest of the certificate given algorithm virtual bool ComputeDigest(const std::string &algorithm, unsigned char* digest, std::size_t size, diff --git a/talk/base/sslidentity_unittest.cc b/talk/base/sslidentity_unittest.cc index 6970426781..e425df2274 100644 --- a/talk/base/sslidentity_unittest.cc +++ b/talk/base/sslidentity_unittest.cc @@ -83,6 +83,22 @@ class SSLIdentityTest : public testing::Test { ASSERT_TRUE(test_cert_); } + void TestGetSignatureDigestAlgorithm() { + std::string digest_algorithm; + // Both NSSIdentity::Generate and OpenSSLIdentity::Generate are + // hard-coded to generate RSA-SHA1 certificates. + ASSERT_TRUE(identity1_->certificate().GetSignatureDigestAlgorithm( + &digest_algorithm)); + ASSERT_EQ(talk_base::DIGEST_SHA_1, digest_algorithm); + ASSERT_TRUE(identity2_->certificate().GetSignatureDigestAlgorithm( + &digest_algorithm)); + ASSERT_EQ(talk_base::DIGEST_SHA_1, digest_algorithm); + + // The test certificate has an MD5-based signature. + ASSERT_TRUE(test_cert_->GetSignatureDigestAlgorithm(&digest_algorithm)); + ASSERT_EQ(talk_base::DIGEST_MD5, digest_algorithm); + } + void TestDigest(const std::string &algorithm, size_t expected_len, const unsigned char *expected_digest = NULL) { unsigned char digest1[64]; @@ -203,3 +219,7 @@ TEST_F(SSLIdentityTest, PemDerConversion) { "CERTIFICATE", reinterpret_cast(der.data()), der.length())); } + +TEST_F(SSLIdentityTest, GetSignatureDigestAlgorithm) { + TestGetSignatureDigestAlgorithm(); +} diff --git a/talk/base/thread.cc b/talk/base/thread.cc index 0c43e635e5..316e14b061 100644 --- a/talk/base/thread.cc +++ b/talk/base/thread.cc @@ -411,11 +411,15 @@ void Thread::Send(MessageHandler *phandler, uint32 id, MessageData *pdata) { ss_->WakeUp(); bool waited = false; + crit_.Enter(); while (!ready) { + crit_.Leave(); current_thread->ReceiveSends(); current_thread->socketserver()->Wait(kForever, false); waited = true; + crit_.Enter(); } + crit_.Leave(); // Our Wait loop above may have consumed some WakeUp events for this // MessageQueue, that weren't relevant to this Send. Losing these WakeUps can diff --git a/talk/build/OWNERS b/talk/build/OWNERS deleted file mode 100644 index a833cd41dd..0000000000 --- a/talk/build/OWNERS +++ /dev/null @@ -1,3 +0,0 @@ -andrew@webrtc.org -kjellander@webrtc.org - diff --git a/talk/p2p/base/dtlstransportchannel.cc b/talk/p2p/base/dtlstransportchannel.cc index a92e7ccad4..7412e5e45a 100644 --- a/talk/p2p/base/dtlstransportchannel.cc +++ b/talk/p2p/base/dtlstransportchannel.cc @@ -42,6 +42,7 @@ namespace cricket { static const size_t kDtlsRecordHeaderLen = 13; static const size_t kMaxDtlsPacketLen = 2048; static const size_t kMinRtpPacketLen = 12; +static const size_t kDefaultVideoAndDataCryptos = 1; static bool IsDtlsPacket(const char* data, size_t len) { const uint8* u = reinterpret_cast(data); @@ -286,14 +287,37 @@ bool DtlsTransportChannelWrapper::SetupDtls() { return true; } -bool DtlsTransportChannelWrapper::SetSrtpCiphers(const std::vector& - ciphers) { - // SRTP ciphers must be set before the DTLS handshake starts. - // TODO(juberti): In multiplex situations, we may end up calling this function - // once for each muxed channel. Depending on the order of calls, this may - // result in slightly undesired results, e.g. 32 vs 80-bit MAC. The right way to - // fix this would be for the TransportProxyChannels to intersect the ciphers - // instead of overwriting, so that "80" followed by "32, 80" results in "80". +bool DtlsTransportChannelWrapper::SetSrtpCiphers( + const std::vector& ciphers) { + if (srtp_ciphers_ == ciphers) + return true; + + if (dtls_state_ == STATE_OPEN) { + // We don't support DTLS renegotiation currently. If new set of srtp ciphers + // are different than what's being used currently, we will not use it. + // So for now, let's be happy (or sad) with a warning message. + std::string current_srtp_cipher; + if (!dtls_->GetDtlsSrtpCipher(¤t_srtp_cipher)) { + LOG(LS_ERROR) << "Failed to get the current SRTP cipher for DTLS channel"; + return false; + } + const std::vector::const_iterator iter = + std::find(ciphers.begin(), ciphers.end(), current_srtp_cipher); + if (iter == ciphers.end()) { + std::string requested_str; + for (size_t i = 0; i < ciphers.size(); ++i) { + requested_str.append(" "); + requested_str.append(ciphers[i]); + requested_str.append(" "); + } + LOG(LS_WARNING) << "Ignoring new set of SRTP ciphers, as DTLS " + << "renegotiation is not supported currently " + << "current cipher = " << current_srtp_cipher << " and " + << "requested = " << "[" << requested_str << "]"; + } + return true; + } + if (dtls_state_ != STATE_NONE && dtls_state_ != STATE_OFFERED && dtls_state_ != STATE_ACCEPTED) { diff --git a/talk/p2p/base/transportdescription.cc b/talk/p2p/base/transportdescription.cc index 5687342728..2793d6c617 100644 --- a/talk/p2p/base/transportdescription.cc +++ b/talk/p2p/base/transportdescription.cc @@ -48,5 +48,25 @@ bool StringToConnectionRole(const std::string& role_str, ConnectionRole* role) { return false; } +bool ConnectionRoleToString(const ConnectionRole& role, std::string* role_str) { + switch (role) { + case cricket::CONNECTIONROLE_ACTIVE: + *role_str = cricket::CONNECTIONROLE_ACTIVE_STR; + break; + case cricket::CONNECTIONROLE_ACTPASS: + *role_str = cricket::CONNECTIONROLE_ACTPASS_STR; + break; + case cricket::CONNECTIONROLE_PASSIVE: + *role_str = cricket::CONNECTIONROLE_PASSIVE_STR; + break; + case cricket::CONNECTIONROLE_HOLDCONN: + *role_str = cricket::CONNECTIONROLE_HOLDCONN_STR; + break; + default: + return false; + } + return true; +} + } // namespace cricket diff --git a/talk/p2p/base/transportdescription.h b/talk/p2p/base/transportdescription.h index 64fbb89a80..59dfa0b47d 100644 --- a/talk/p2p/base/transportdescription.h +++ b/talk/p2p/base/transportdescription.h @@ -94,6 +94,7 @@ extern const char CONNECTIONROLE_ACTPASS_STR[]; extern const char CONNECTIONROLE_HOLDCONN_STR[]; bool StringToConnectionRole(const std::string& role_str, ConnectionRole* role); +bool ConnectionRoleToString(const ConnectionRole& role, std::string* role_str); typedef std::vector Candidates; @@ -159,7 +160,7 @@ struct TransportDescription { void AddOption(const std::string& option) { transport_options.push_back(option); } - bool secure() { return identity_fingerprint != NULL; } + bool secure() const { return identity_fingerprint != NULL; } static talk_base::SSLFingerprint* CopyFingerprint( const talk_base::SSLFingerprint* from) { diff --git a/talk/p2p/base/transportdescriptionfactory.cc b/talk/p2p/base/transportdescriptionfactory.cc index 0c129437cc..c8fb0b34f8 100644 --- a/talk/p2p/base/transportdescriptionfactory.cc +++ b/talk/p2p/base/transportdescriptionfactory.cc @@ -37,13 +37,11 @@ namespace cricket { static TransportProtocol kDefaultProtocol = ICEPROTO_GOOGLE; -static const char* kDefaultDigestAlg = talk_base::DIGEST_SHA_1; TransportDescriptionFactory::TransportDescriptionFactory() : protocol_(kDefaultProtocol), secure_(SEC_DISABLED), - identity_(NULL), - digest_alg_(kDefaultDigestAlg) { + identity_(NULL) { } TransportDescription* TransportDescriptionFactory::CreateOffer( @@ -153,10 +151,20 @@ bool TransportDescriptionFactory::SetSecurityInfo( return false; } + // This digest algorithm is used to produce the a=fingerprint lines in SDP. + // RFC 4572 Section 5 requires that those lines use the same hash function as + // the certificate's signature. + std::string digest_alg; + if (!identity_->certificate().GetSignatureDigestAlgorithm(&digest_alg)) { + LOG(LS_ERROR) << "Failed to retrieve the certificate's digest algorithm"; + return false; + } + desc->identity_fingerprint.reset( - talk_base::SSLFingerprint::Create(digest_alg_, identity_)); + talk_base::SSLFingerprint::Create(digest_alg, identity_)); if (!desc->identity_fingerprint.get()) { - LOG(LS_ERROR) << "Failed to create identity digest, alg=" << digest_alg_; + LOG(LS_ERROR) << "Failed to create identity fingerprint, alg=" + << digest_alg; return false; } diff --git a/talk/p2p/base/transportdescriptionfactory.h b/talk/p2p/base/transportdescriptionfactory.h index ddf2799818..53dd238dbe 100644 --- a/talk/p2p/base/transportdescriptionfactory.h +++ b/talk/p2p/base/transportdescriptionfactory.h @@ -59,8 +59,6 @@ class TransportDescriptionFactory { void set_secure(SecurePolicy s) { secure_ = s; } // Specifies the identity to use (only used when secure is not SEC_DISABLED). void set_identity(talk_base::SSLIdentity* identity) { identity_ = identity; } - // Specifies the algorithm to use when creating an identity digest. - void set_digest_algorithm(const std::string& alg) { digest_alg_ = alg; } // Creates a transport description suitable for use in an offer. TransportDescription* CreateOffer(const TransportOptions& options, @@ -78,7 +76,6 @@ class TransportDescriptionFactory { TransportProtocol protocol_; SecurePolicy secure_; talk_base::SSLIdentity* identity_; - std::string digest_alg_; }; } // namespace cricket diff --git a/talk/p2p/base/transportdescriptionfactory_unittest.cc b/talk/p2p/base/transportdescriptionfactory_unittest.cc index c0c88297f0..8d9a73f56f 100644 --- a/talk/p2p/base/transportdescriptionfactory_unittest.cc +++ b/talk/p2p/base/transportdescriptionfactory_unittest.cc @@ -39,16 +39,11 @@ using cricket::TransportDescriptionFactory; using cricket::TransportDescription; using cricket::TransportOptions; -// TODO(juberti): Change this to SHA-256 once we have Win32 using OpenSSL. -static const char* kDefaultDigestAlg = talk_base::DIGEST_SHA_1; - class TransportDescriptionFactoryTest : public testing::Test { public: TransportDescriptionFactoryTest() : id1_(new talk_base::FakeSSLIdentity("User1")), id2_(new talk_base::FakeSSLIdentity("User2")) { - f1_.set_digest_algorithm(kDefaultDigestAlg); - f2_.set_digest_algorithm(kDefaultDigestAlg); } void CheckDesc(const TransportDescription* desc, const std::string& type, const std::string& opt, const std::string& ice_ufrag, @@ -167,15 +162,17 @@ TEST_F(TransportDescriptionFactoryTest, TestOfferHybridDtls) { f1_.set_protocol(cricket::ICEPROTO_HYBRID); f1_.set_secure(cricket::SEC_ENABLED); f1_.set_identity(id1_.get()); + std::string digest_alg; + ASSERT_TRUE(id1_->certificate().GetSignatureDigestAlgorithm(&digest_alg)); scoped_ptr desc(f1_.CreateOffer( TransportOptions(), NULL)); CheckDesc(desc.get(), cricket::NS_JINGLE_ICE_UDP, "google-ice", "", "", - kDefaultDigestAlg); + digest_alg); // Ensure it also works with SEC_REQUIRED. f1_.set_secure(cricket::SEC_REQUIRED); desc.reset(f1_.CreateOffer(TransportOptions(), NULL)); CheckDesc(desc.get(), cricket::NS_JINGLE_ICE_UDP, "google-ice", "", "", - kDefaultDigestAlg); + digest_alg); } // Test generating a hybrid offer with DTLS fails with no identity. @@ -187,23 +184,14 @@ TEST_F(TransportDescriptionFactoryTest, TestOfferHybridDtlsWithNoIdentity) { ASSERT_TRUE(desc.get() == NULL); } -// Test generating a hybrid offer with DTLS fails with an unsupported digest. -TEST_F(TransportDescriptionFactoryTest, TestOfferHybridDtlsWithBadDigestAlg) { - f1_.set_protocol(cricket::ICEPROTO_HYBRID); - f1_.set_secure(cricket::SEC_ENABLED); - f1_.set_identity(id1_.get()); - f1_.set_digest_algorithm("bogus"); - scoped_ptr desc(f1_.CreateOffer( - TransportOptions(), NULL)); - ASSERT_TRUE(desc.get() == NULL); -} - // Test updating a hybrid offer with DTLS to pick ICE. // The ICE credentials should stay the same in the new offer. TEST_F(TransportDescriptionFactoryTest, TestOfferHybridDtlsReofferIceDtls) { f1_.set_protocol(cricket::ICEPROTO_HYBRID); f1_.set_secure(cricket::SEC_ENABLED); f1_.set_identity(id1_.get()); + std::string digest_alg; + ASSERT_TRUE(id1_->certificate().GetSignatureDigestAlgorithm(&digest_alg)); scoped_ptr old_desc(f1_.CreateOffer( TransportOptions(), NULL)); ASSERT_TRUE(old_desc.get() != NULL); @@ -211,7 +199,7 @@ TEST_F(TransportDescriptionFactoryTest, TestOfferHybridDtlsReofferIceDtls) { scoped_ptr desc( f1_.CreateOffer(TransportOptions(), old_desc.get())); CheckDesc(desc.get(), cricket::NS_JINGLE_ICE_UDP, "", - old_desc->ice_ufrag, old_desc->ice_pwd, kDefaultDigestAlg); + old_desc->ice_ufrag, old_desc->ice_pwd, digest_alg); } // Test that we can answer a GICE offer with GICE. @@ -358,21 +346,25 @@ TEST_F(TransportDescriptionFactoryTest, TestAnswerHybridDtlsToHybridDtls) { f1_.set_protocol(cricket::ICEPROTO_HYBRID); f1_.set_secure(cricket::SEC_ENABLED); f1_.set_identity(id1_.get()); + f2_.set_protocol(cricket::ICEPROTO_HYBRID); f2_.set_secure(cricket::SEC_ENABLED); f2_.set_identity(id2_.get()); + // f2_ produces the answer that is being checked in this test, so the + // answer must contain fingerprint lines with id2_'s digest algorithm. + std::string digest_alg2; + ASSERT_TRUE(id2_->certificate().GetSignatureDigestAlgorithm(&digest_alg2)); + scoped_ptr offer( f1_.CreateOffer(TransportOptions(), NULL)); ASSERT_TRUE(offer.get() != NULL); scoped_ptr desc( f2_.CreateAnswer(offer.get(), TransportOptions(), NULL)); - CheckDesc(desc.get(), cricket::NS_JINGLE_ICE_UDP, "", "", "", - kDefaultDigestAlg); + CheckDesc(desc.get(), cricket::NS_JINGLE_ICE_UDP, "", "", "", digest_alg2); f2_.set_secure(cricket::SEC_REQUIRED); desc.reset(f2_.CreateAnswer(offer.get(), TransportOptions(), NULL)); - CheckDesc(desc.get(), cricket::NS_JINGLE_ICE_UDP, "", "", "", - kDefaultDigestAlg); + CheckDesc(desc.get(), cricket::NS_JINGLE_ICE_UDP, "", "", "", digest_alg2); } // Test that ice ufrag and password is changed in an updated offer and answer diff --git a/talk/session/media/channel.cc b/talk/session/media/channel.cc index e00ba16d04..1cb5992e69 100644 --- a/talk/session/media/channel.cc +++ b/talk/session/media/channel.cc @@ -1081,32 +1081,42 @@ bool BaseChannel::SetMaxSendBandwidth_w(int max_bandwidth) { return media_channel()->SetSendBandwidth(true, max_bandwidth); } +// |dtls| will be set to true if DTLS is active for transport channel and +// crypto is empty. +bool BaseChannel::CheckSrtpConfig(const std::vector& cryptos, + bool* dtls) { + *dtls = transport_channel_->IsDtlsActive(); + if (*dtls && !cryptos.empty()) { + LOG(LS_WARNING) << "Cryptos must be empty when DTLS is active."; + return false; + } + return true; +} + bool BaseChannel::SetSrtp_w(const std::vector& cryptos, ContentAction action, ContentSource src) { bool ret = false; + bool dtls = false; + ret = CheckSrtpConfig(cryptos, &dtls); switch (action) { case CA_OFFER: - ret = srtp_filter_.SetOffer(cryptos, src); + // If DTLS is already active on the channel, we could be renegotiating + // here. We don't update the srtp filter. + if (ret && !dtls) { + ret = srtp_filter_.SetOffer(cryptos, src); + } break; case CA_PRANSWER: // If we're doing DTLS-SRTP, we don't want to update the filter // with an answer, because we already have SRTP parameters. - if (transport_channel_->IsDtlsActive()) { - LOG(LS_INFO) << - "Ignoring SDES answer parameters because we are using DTLS-SRTP"; - ret = true; - } else { + if (ret && !dtls) { ret = srtp_filter_.SetProvisionalAnswer(cryptos, src); } break; case CA_ANSWER: // If we're doing DTLS-SRTP, we don't want to update the filter // with an answer, because we already have SRTP parameters. - if (transport_channel_->IsDtlsActive()) { - LOG(LS_INFO) << - "Ignoring SDES answer parameters because we are using DTLS-SRTP"; - ret = true; - } else { + if (ret && !dtls) { ret = srtp_filter_.SetAnswer(cryptos, src); } break; @@ -2582,6 +2592,9 @@ bool DataChannel::SetLocalContent_w(const MediaContentDescription* content, ret = UpdateLocalStreams_w(data->streams(), action); if (ret) { set_local_content_direction(content->direction()); + // As in SetRemoteContent_w, make sure we set the local SCTP port + // number as specified in our DataContentDescription. + ret = media_channel()->SetRecvCodecs(data->codecs()); } } else { ret = SetBaseLocalContent_w(content, action); @@ -2620,6 +2633,9 @@ bool DataChannel::SetRemoteContent_w(const MediaContentDescription* content, ret = UpdateRemoteStreams_w(content->streams(), action); if (ret) { set_remote_content_direction(content->direction()); + // We send the SCTP port number (not to be confused with the underlying + // UDP port number) as a codec parameter. Make sure it gets there. + ret = media_channel()->SetSendCodecs(data->codecs()); } } else { // If the remote data doesn't have codecs and isn't an update, it diff --git a/talk/session/media/channel.h b/talk/session/media/channel.h index ccfdd07fe6..6ee2e8cda4 100644 --- a/talk/session/media/channel.h +++ b/talk/session/media/channel.h @@ -318,6 +318,7 @@ class BaseChannel virtual bool SetRemoteContent_w(const MediaContentDescription* content, ContentAction action) = 0; + bool CheckSrtpConfig(const std::vector& cryptos, bool* dtls); bool SetSrtp_w(const std::vector& params, ContentAction action, ContentSource src); bool SetRtcpMux_w(bool enable, ContentAction action, ContentSource src); diff --git a/talk/session/media/channel_unittest.cc b/talk/session/media/channel_unittest.cc index fda89b3c6e..24ece06962 100644 --- a/talk/session/media/channel_unittest.cc +++ b/talk/session/media/channel_unittest.cc @@ -233,6 +233,10 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { this, &ChannelTest::OnMediaChannelError); channel1_->SignalAutoMuted.connect( this, &ChannelTest::OnMediaMuted); + if ((flags1 & DTLS) && (flags2 & DTLS)) { + flags1 = (flags1 & ~SECURE); + flags2 = (flags2 & ~SECURE); + } CreateContent(flags1, kPcmuCodec, kH264Codec, &local_media_content1_); CreateContent(flags2, kPcmuCodec, kH264Codec, diff --git a/talk/session/media/mediasession.cc b/talk/session/media/mediasession.cc index 3d1c46842c..ea23591f32 100644 --- a/talk/session/media/mediasession.cc +++ b/talk/session/media/mediasession.cc @@ -978,6 +978,42 @@ static void SetMediaProtocol(bool secure_transport, desc->set_protocol(kMediaProtocolAvpf); } +// Gets the TransportInfo of the given |content_name| from the +// |current_description|. If doesn't exist, returns a new one. +static const TransportDescription* GetTransportDescription( + const std::string& content_name, + const SessionDescription* current_description) { + const TransportDescription* desc = NULL; + if (current_description) { + const TransportInfo* info = + current_description->GetTransportInfoByName(content_name); + if (info) { + desc = &info->description; + } + } + return desc; +} + +// Gets the current DTLS state from the transport description. +static bool IsDtlsActive( + const std::string& content_name, + const SessionDescription* current_description) { + if (!current_description) + return false; + + const ContentInfo* content = + current_description->GetContentByName(content_name); + if (!content) + return false; + + const TransportDescription* current_tdesc = + GetTransportDescription(content_name, current_description); + if (!current_tdesc) + return false; + + return current_tdesc->secure(); +} + void MediaSessionOptions::AddStream(MediaType type, const std::string& id, const std::string& sync_label) { @@ -1053,13 +1089,17 @@ SessionDescription* MediaSessionDescriptionFactory::CreateOffer( // Handle m=audio. if (options.has_audio) { + cricket::SecurePolicy sdes_policy = + IsDtlsActive(CN_AUDIO, current_description) ? + cricket::SEC_DISABLED : secure(); + scoped_ptr audio(new AudioContentDescription()); std::vector crypto_suites; GetSupportedAudioCryptoSuites(&crypto_suites); if (!CreateMediaContentOffer( options, audio_codecs, - secure(), + sdes_policy, GetCryptos(GetFirstAudioContentDescription(current_description)), crypto_suites, audio_rtp_extensions, @@ -1080,13 +1120,17 @@ SessionDescription* MediaSessionDescriptionFactory::CreateOffer( // Handle m=video. if (options.has_video) { + cricket::SecurePolicy sdes_policy = + IsDtlsActive(CN_VIDEO, current_description) ? + cricket::SEC_DISABLED : secure(); + scoped_ptr video(new VideoContentDescription()); std::vector crypto_suites; GetSupportedVideoCryptoSuites(&crypto_suites); if (!CreateMediaContentOffer( options, video_codecs, - secure(), + sdes_policy, GetCryptos(GetFirstVideoContentDescription(current_description)), crypto_suites, video_rtp_extensions, @@ -1110,8 +1154,10 @@ SessionDescription* MediaSessionDescriptionFactory::CreateOffer( scoped_ptr data(new DataContentDescription()); bool is_sctp = (options.data_channel_type == DCT_SCTP); + cricket::SecurePolicy sdes_policy = + IsDtlsActive(CN_DATA, current_description) ? + cricket::SEC_DISABLED : secure(); std::vector crypto_suites; - cricket::SecurePolicy sdes_policy = secure(); if (is_sctp) { // SDES doesn't make sense for SCTP, so we disable it, and we only // get SDES crypto suites for RTP-based data channels. @@ -1371,22 +1417,6 @@ SessionDescription* MediaSessionDescriptionFactory::CreateAnswer( return answer.release(); } -// Gets the TransportInfo of the given |content_name| from the -// |current_description|. If doesn't exist, returns a new one. -static const TransportDescription* GetTransportDescription( - const std::string& content_name, - const SessionDescription* current_description) { - const TransportDescription* desc = NULL; - if (current_description) { - const TransportInfo* info = - current_description->GetTransportInfoByName(content_name); - if (info) { - desc = &info->description; - } - } - return desc; -} - void MediaSessionDescriptionFactory::GetCodecsToOffer( const SessionDescription* current_description, AudioCodecs* audio_codecs, diff --git a/talk/session/media/mediasession_unittest.cc b/talk/session/media/mediasession_unittest.cc index f014e78fe3..850d67f21b 100644 --- a/talk/session/media/mediasession_unittest.cc +++ b/talk/session/media/mediasession_unittest.cc @@ -1814,6 +1814,26 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCryptoDtls) { ASSERT_TRUE(video_trans_desc != NULL); ASSERT_TRUE(audio_trans_desc->identity_fingerprint.get() != NULL); ASSERT_TRUE(video_trans_desc->identity_fingerprint.get() != NULL); + + // Try creating offer again. DTLS enabled now, crypto's should be empty + // in new offer. + offer.reset(f1_.CreateOffer(options, offer.get())); + ASSERT_TRUE(offer.get() != NULL); + audio_media_desc = static_cast( + offer->GetContentDescriptionByName("audio")); + ASSERT_TRUE(audio_media_desc != NULL); + video_media_desc = static_cast( + offer->GetContentDescriptionByName("video")); + ASSERT_TRUE(video_media_desc != NULL); + EXPECT_TRUE(audio_media_desc->cryptos().empty()); + EXPECT_TRUE(video_media_desc->cryptos().empty()); + + audio_trans_desc = offer->GetTransportDescriptionByName("audio"); + ASSERT_TRUE(audio_trans_desc != NULL); + video_trans_desc = offer->GetTransportDescriptionByName("video"); + ASSERT_TRUE(video_trans_desc != NULL); + ASSERT_TRUE(audio_trans_desc->identity_fingerprint.get() != NULL); + ASSERT_TRUE(video_trans_desc->identity_fingerprint.get() != NULL); } // Test that an answer can't be created if cryptos are required but the offer is