diff --git a/webrtc/p2p/base/dtlstransportchannel_unittest.cc b/webrtc/p2p/base/dtlstransportchannel_unittest.cc index 696135c5b0..17e7a0bcad 100644 --- a/webrtc/p2p/base/dtlstransportchannel_unittest.cc +++ b/webrtc/p2p/base/dtlstransportchannel_unittest.cc @@ -818,12 +818,8 @@ TEST_F(DtlsTransportChannelTest, TestTransferDtlsAnswererIsPassive) { TEST_F(DtlsTransportChannelTest, TestDtlsSetupWithLegacyAsAnswerer) { PrepareDtls(true, true, rtc::KT_DEFAULT); NegotiateWithLegacy(); - rtc::SSLRole channel1_role; - rtc::SSLRole channel2_role; - client1_.transport()->GetSslRole(&channel1_role); - client2_.transport()->GetSslRole(&channel2_role); - EXPECT_EQ(rtc::SSL_SERVER, channel1_role); - EXPECT_EQ(rtc::SSL_CLIENT, channel2_role); + EXPECT_EQ(rtc::SSL_SERVER, *client1_.transport()->GetSslRole()); + EXPECT_EQ(rtc::SSL_CLIENT, *client2_.transport()->GetSslRole()); } // Testing re offer/answer after the session is estbalished. Roles will be diff --git a/webrtc/p2p/base/jseptransport.cc b/webrtc/p2p/base/jseptransport.cc index 4bc24fd07b..301ae24003 100644 --- a/webrtc/p2p/base/jseptransport.cc +++ b/webrtc/p2p/base/jseptransport.cc @@ -278,9 +278,8 @@ bool JsepTransport::NeedsIceRestart() const { return needs_ice_restart_; } -void JsepTransport::GetSslRole(rtc::SSLRole* ssl_role) const { - RTC_DCHECK(ssl_role); - *ssl_role = secure_role_; +rtc::Optional JsepTransport::GetSslRole() const { + return ssl_role_; } bool JsepTransport::GetStats(TransportStats* stats) { @@ -342,11 +341,6 @@ bool JsepTransport::ApplyLocalTransportDescription( bool JsepTransport::ApplyRemoteTransportDescription( DtlsTransportInternal* dtls_transport, std::string* error_desc) { - // Currently, all ICE-related calls still go through this DTLS channel. But - // that will change once we get rid of TransportChannelImpl, and the DTLS - // channel interface no longer includes ICE-specific methods. Then this class - // will need to call dtls->ice()->SetIceRole(), for example, assuming the Dtls - // interface will expose its inner ICE channel. dtls_transport->ice_transport()->SetRemoteIceParameters( remote_description_->GetIceParameters()); dtls_transport->ice_transport()->SetRemoteIceMode( @@ -359,7 +353,7 @@ bool JsepTransport::ApplyNegotiatedTransportDescription( std::string* error_desc) { // Set SSL role. Role must be set before fingerprint is applied, which // initiates DTLS setup. - if (!dtls_transport->SetSslRole(secure_role_)) { + if (ssl_role_ && !dtls_transport->SetSslRole(*ssl_role_)) { return BadTransportDescription("Failed to set SSL role for the channel.", error_desc); } @@ -374,8 +368,9 @@ bool JsepTransport::ApplyNegotiatedTransportDescription( return true; } -bool JsepTransport::NegotiateTransportDescription(ContentAction local_role, - std::string* error_desc) { +bool JsepTransport::NegotiateTransportDescription( + ContentAction local_description_type, + std::string* error_desc) { if (!local_description_ || !remote_description_) { const std::string msg = "Applying an answer transport description " @@ -388,10 +383,10 @@ bool JsepTransport::NegotiateTransportDescription(ContentAction local_role, remote_description_->identity_fingerprint.get(); if (remote_fp && local_fp) { remote_fingerprint_.reset(new rtc::SSLFingerprint(*remote_fp)); - if (!NegotiateRole(local_role, &secure_role_, error_desc)) { + if (!NegotiateRole(local_description_type, error_desc)) { return false; } - } else if (local_fp && (local_role == CA_ANSWER)) { + } else if (local_fp && (local_description_type == CA_ANSWER)) { return BadTransportDescription( "Local fingerprint supplied when caller didn't offer DTLS.", error_desc); @@ -412,10 +407,8 @@ bool JsepTransport::NegotiateTransportDescription(ContentAction local_role, return true; } -bool JsepTransport::NegotiateRole(ContentAction local_role, - rtc::SSLRole* ssl_role, - std::string* error_desc) const { - RTC_DCHECK(ssl_role); +bool JsepTransport::NegotiateRole(ContentAction local_description_type, + std::string* error_desc) { if (!local_description_ || !remote_description_) { const std::string msg = "Local and Remote description must be set before " @@ -450,7 +443,7 @@ bool JsepTransport::NegotiateRole(ContentAction local_role, ConnectionRole remote_connection_role = remote_description_->connection_role; bool is_remote_server = false; - if (local_role == CA_OFFER) { + if (local_description_type == CA_OFFER) { if (local_connection_role != CONNECTIONROLE_ACTPASS) { return BadTransportDescription( "Offerer must use actpass value for setup attribute.", error_desc); @@ -470,8 +463,23 @@ bool JsepTransport::NegotiateRole(ContentAction local_role, } else { if (remote_connection_role != CONNECTIONROLE_ACTPASS && remote_connection_role != CONNECTIONROLE_NONE) { - return BadTransportDescription( - "Offerer must use actpass value for setup attribute.", error_desc); + // Accept a remote role attribute that's not "actpass", but matches the + // current negotiated role. This is allowed by dtls-sdp, though our + // implementation will never generate such an offer as it's not + // recommended. + // + // See https://datatracker.ietf.org/doc/html/draft-ietf-mmusic-dtls-sdp, + // section 5.5. + if (!ssl_role_ || + (*ssl_role_ == rtc::SSL_CLIENT && + remote_connection_role == CONNECTIONROLE_ACTIVE) || + (*ssl_role_ == rtc::SSL_SERVER && + remote_connection_role == CONNECTIONROLE_PASSIVE)) { + return BadTransportDescription( + "Offerer must use actpass value or current negotiated role for " + "setup attribute.", + error_desc); + } } if (local_connection_role == CONNECTIONROLE_ACTIVE || @@ -487,7 +495,7 @@ bool JsepTransport::NegotiateRole(ContentAction local_role, // If local is passive, local will act as server. } - *ssl_role = is_remote_server ? rtc::SSL_CLIENT : rtc::SSL_SERVER; + ssl_role_.emplace(is_remote_server ? rtc::SSL_CLIENT : rtc::SSL_SERVER); return true; } diff --git a/webrtc/p2p/base/jseptransport.h b/webrtc/p2p/base/jseptransport.h index e29dd0d02a..7b81be06f6 100644 --- a/webrtc/p2p/base/jseptransport.h +++ b/webrtc/p2p/base/jseptransport.h @@ -298,7 +298,9 @@ class JsepTransport : public sigslot::has_slots<> { // changed ufrag/password). bool NeedsIceRestart() const; - void GetSslRole(rtc::SSLRole* ssl_role) const; + // Returns role if negotiated, or empty Optional if it hasn't been negotiated + // yet. + rtc::Optional GetSslRole() const; // TODO(deadbeef): Make this const. See comment in transportcontroller.h. bool GetStats(TransportStats* stats); @@ -325,22 +327,21 @@ class JsepTransport : public sigslot::has_slots<> { const rtc::SSLFingerprint* fingerprint, std::string* error_desc) const; - // Negotiates the SSL role based off the offer and answer as specified by - // RFC 4145, section-4.1. Returns false if the SSL role cannot be determined - // from the local description and remote description. - bool NegotiateRole(ContentAction local_role, - rtc::SSLRole* ssl_role, - std::string* error_desc) const; - private: // Negotiates the transport parameters based on the current local and remote // transport description, such as the ICE role to use, and whether DTLS // should be activated. // // Called when an answer TransportDescription is applied. - bool NegotiateTransportDescription(ContentAction local_role, + bool NegotiateTransportDescription(ContentAction local_description_type, std::string* error_desc); + // Negotiates the SSL role based off the offer and answer as specified by + // RFC 4145, section-4.1. Returns false if the SSL role cannot be determined + // from the local description and remote description. + bool NegotiateRole(ContentAction local_description_type, + std::string* error_desc); + // Pushes down the transport parameters from the local description, such // as the ICE ufrag and pwd. bool ApplyLocalTransportDescription(DtlsTransportInternal* dtls_transport, @@ -360,7 +361,7 @@ class JsepTransport : public sigslot::has_slots<> { // needs-ice-restart bit as described in JSEP. bool needs_ice_restart_ = false; rtc::scoped_refptr certificate_; - rtc::SSLRole secure_role_ = rtc::SSL_CLIENT; + rtc::Optional ssl_role_; std::unique_ptr remote_fingerprint_; std::unique_ptr local_description_; std::unique_ptr remote_description_; diff --git a/webrtc/p2p/base/jseptransport_unittest.cc b/webrtc/p2p/base/jseptransport_unittest.cc index ccb7016e05..23ed4d857d 100644 --- a/webrtc/p2p/base/jseptransport_unittest.cc +++ b/webrtc/p2p/base/jseptransport_unittest.cc @@ -32,16 +32,21 @@ static const char kIcePwd2[] = "TESTICEPWD00000000000002"; class JsepTransportTest : public testing::Test, public sigslot::has_slots<> { public: - JsepTransportTest() - : transport_(new JsepTransport("test content name", nullptr)) {} + JsepTransportTest() { RecreateTransport(); } + bool SetupChannel() { fake_ice_transport_.reset(new FakeIceTransport(transport_->mid(), 1)); fake_dtls_transport_.reset( new FakeDtlsTransport(fake_ice_transport_.get())); return transport_->AddChannel(fake_dtls_transport_.get(), 1); } + void DestroyChannel() { transport_->RemoveChannel(1); } + void RecreateTransport() { + transport_.reset(new JsepTransport("test content name", nullptr)); + } + protected: std::unique_ptr fake_dtls_transport_; std::unique_ptr fake_ice_transport_; @@ -167,10 +172,22 @@ TEST_F(JsepTransportTest, TestVerifyCertificateFingerprint) { } } -// Tests that NegotiateRole sets the SSL role correctly. -TEST_F(JsepTransportTest, TestNegotiateRole) { +// Tests the logic of DTLS role negotiation for an initial offer/answer. +TEST_F(JsepTransportTest, DtlsRoleNegotiation) { + rtc::scoped_refptr certificate = + rtc::RTCCertificate::Create(std::unique_ptr( + rtc::SSLIdentity::Generate("testing", rtc::KT_ECDSA))); + std::unique_ptr fingerprint( + rtc::SSLFingerprint::CreateFromCertificate(certificate)); + TransportDescription local_desc(kIceUfrag1, kIcePwd1); TransportDescription remote_desc(kIceUfrag2, kIcePwd2); + // Just use the same fingerprint in both descriptions; the remote fingerprint + // doesn't matter in a non end-to-end test. + local_desc.identity_fingerprint.reset( + TransportDescription::CopyFingerprint(fingerprint.get())); + remote_desc.identity_fingerprint.reset( + TransportDescription::CopyFingerprint(fingerprint.get())); struct NegotiateRoleParams { cricket::ConnectionRole local_role; @@ -179,7 +196,6 @@ TEST_F(JsepTransportTest, TestNegotiateRole) { cricket::ContentAction remote_action; }; - rtc::SSLRole ssl_role; std::string error_desc; // Parameters which set the SSL role to SSL_CLIENT. @@ -194,16 +210,25 @@ TEST_F(JsepTransportTest, TestNegotiateRole) { cricket::CA_OFFER, cricket::CA_PRANSWER}}; for (auto& param : valid_client_params) { + RecreateTransport(); + transport_->SetLocalCertificate(certificate); + local_desc.connection_role = param.local_role; remote_desc.connection_role = param.remote_role; - ASSERT_TRUE(transport_->SetRemoteTransportDescription( - remote_desc, param.remote_action, nullptr)); - ASSERT_TRUE(transport_->SetLocalTransportDescription( - local_desc, param.local_action, nullptr)); - EXPECT_TRUE( - transport_->NegotiateRole(param.local_action, &ssl_role, &error_desc)); - EXPECT_EQ(rtc::SSL_CLIENT, ssl_role); + // Set the offer first. + if (param.local_action == cricket::CA_OFFER) { + EXPECT_TRUE(transport_->SetLocalTransportDescription( + local_desc, param.local_action, nullptr)); + EXPECT_TRUE(transport_->SetRemoteTransportDescription( + remote_desc, param.remote_action, nullptr)); + } else { + EXPECT_TRUE(transport_->SetRemoteTransportDescription( + remote_desc, param.remote_action, nullptr)); + EXPECT_TRUE(transport_->SetLocalTransportDescription( + local_desc, param.local_action, nullptr)); + } + EXPECT_EQ(rtc::SSL_CLIENT, *transport_->GetSslRole()); } // Parameters which set the SSL role to SSL_SERVER. @@ -218,16 +243,25 @@ TEST_F(JsepTransportTest, TestNegotiateRole) { cricket::CA_OFFER, cricket::CA_PRANSWER}}; for (auto& param : valid_server_params) { + RecreateTransport(); + transport_->SetLocalCertificate(certificate); + local_desc.connection_role = param.local_role; remote_desc.connection_role = param.remote_role; - ASSERT_TRUE(transport_->SetRemoteTransportDescription( - remote_desc, param.remote_action, nullptr)); - ASSERT_TRUE(transport_->SetLocalTransportDescription( - local_desc, param.local_action, nullptr)); - EXPECT_TRUE( - transport_->NegotiateRole(param.local_action, &ssl_role, &error_desc)); - EXPECT_EQ(rtc::SSL_SERVER, ssl_role); + // Set the offer first. + if (param.local_action == cricket::CA_OFFER) { + EXPECT_TRUE(transport_->SetLocalTransportDescription( + local_desc, param.local_action, nullptr)); + EXPECT_TRUE(transport_->SetRemoteTransportDescription( + remote_desc, param.remote_action, nullptr)); + } else { + EXPECT_TRUE(transport_->SetRemoteTransportDescription( + remote_desc, param.remote_action, nullptr)); + EXPECT_TRUE(transport_->SetLocalTransportDescription( + local_desc, param.local_action, nullptr)); + } + EXPECT_EQ(rtc::SSL_SERVER, *transport_->GetSslRole()); } // Invalid parameters due to both peers having a duplicate role. @@ -258,15 +292,24 @@ TEST_F(JsepTransportTest, TestNegotiateRole) { cricket::CA_OFFER, cricket::CA_PRANSWER}}; for (auto& param : duplicate_params) { + RecreateTransport(); + transport_->SetLocalCertificate(certificate); + local_desc.connection_role = param.local_role; remote_desc.connection_role = param.remote_role; - ASSERT_TRUE(transport_->SetRemoteTransportDescription( - remote_desc, param.remote_action, nullptr)); - ASSERT_TRUE(transport_->SetLocalTransportDescription( - local_desc, param.local_action, nullptr)); - EXPECT_FALSE( - transport_->NegotiateRole(param.local_action, &ssl_role, &error_desc)); + // Set the offer first. + if (param.local_action == cricket::CA_OFFER) { + EXPECT_TRUE(transport_->SetLocalTransportDescription( + local_desc, param.local_action, nullptr)); + EXPECT_FALSE(transport_->SetRemoteTransportDescription( + remote_desc, param.remote_action, nullptr)); + } else { + EXPECT_TRUE(transport_->SetRemoteTransportDescription( + remote_desc, param.remote_action, nullptr)); + EXPECT_FALSE(transport_->SetLocalTransportDescription( + local_desc, param.local_action, nullptr)); + } } // Invalid parameters due to the offerer not using ACTPASS. @@ -297,14 +340,113 @@ TEST_F(JsepTransportTest, TestNegotiateRole) { cricket::CA_OFFER, cricket::CA_PRANSWER}}; for (auto& param : offerer_without_actpass_params) { + RecreateTransport(); + transport_->SetLocalCertificate(certificate); + local_desc.connection_role = param.local_role; remote_desc.connection_role = param.remote_role; - ASSERT_TRUE(transport_->SetRemoteTransportDescription( - remote_desc, param.remote_action, nullptr)); - ASSERT_TRUE(transport_->SetLocalTransportDescription( - local_desc, param.local_action, nullptr)); - EXPECT_FALSE( - transport_->NegotiateRole(param.local_action, &ssl_role, &error_desc)); + // Set the offer first. + // TODO(deadbeef): Really this should fail as soon as the offer is + // attempted to be applied, and not when the answer is applied. + if (param.local_action == cricket::CA_OFFER) { + EXPECT_TRUE(transport_->SetLocalTransportDescription( + local_desc, param.local_action, nullptr)); + EXPECT_FALSE(transport_->SetRemoteTransportDescription( + remote_desc, param.remote_action, nullptr)); + } else { + EXPECT_TRUE(transport_->SetRemoteTransportDescription( + remote_desc, param.remote_action, nullptr)); + EXPECT_FALSE(transport_->SetLocalTransportDescription( + local_desc, param.local_action, nullptr)); + } } } + +// Test that a remote offer with the current negotiated role can be accepted. +// This is allowed by dtls-sdp, though we'll never generate such an offer, +// since JSEP requires generating "actpass". +TEST_F(JsepTransportTest, RemoteOfferWithCurrentNegotiatedDtlsRole) { + rtc::scoped_refptr certificate = + rtc::RTCCertificate::Create(std::unique_ptr( + rtc::SSLIdentity::Generate("testing", rtc::KT_ECDSA))); + std::unique_ptr fingerprint( + rtc::SSLFingerprint::CreateFromCertificate(certificate)); + transport_->SetLocalCertificate(certificate); + + TransportDescription local_desc(kIceUfrag1, kIcePwd1); + TransportDescription remote_desc(kIceUfrag2, kIcePwd2); + // Just use the same fingerprint in both descriptions; the remote fingerprint + // doesn't matter in a non end-to-end test. + local_desc.identity_fingerprint.reset( + TransportDescription::CopyFingerprint(fingerprint.get())); + remote_desc.identity_fingerprint.reset( + TransportDescription::CopyFingerprint(fingerprint.get())); + + remote_desc.connection_role = cricket::CONNECTIONROLE_ACTPASS; + local_desc.connection_role = cricket::CONNECTIONROLE_ACTIVE; + + // Normal initial offer/answer with "actpass" in the offer and "active" in + // the answer. + ASSERT_TRUE(transport_->SetRemoteTransportDescription( + remote_desc, cricket::CA_OFFER, nullptr)); + ASSERT_TRUE(transport_->SetLocalTransportDescription( + local_desc, cricket::CA_ANSWER, nullptr)); + + // Sanity check that role was actually negotiated. + rtc::Optional role = transport_->GetSslRole(); + ASSERT_TRUE(role); + EXPECT_EQ(rtc::SSL_CLIENT, *role); + + // Subsequent offer with current negotiated role of "passive". + remote_desc.connection_role = cricket::CONNECTIONROLE_PASSIVE; + EXPECT_TRUE(transport_->SetRemoteTransportDescription( + remote_desc, cricket::CA_OFFER, nullptr)); + EXPECT_TRUE(transport_->SetLocalTransportDescription( + local_desc, cricket::CA_ANSWER, nullptr)); +} + +// Test that a remote offer with the inverse of the current negotiated DTLS +// role is rejected. +TEST_F(JsepTransportTest, RemoteOfferThatChangesNegotiatedDtlsRole) { + rtc::scoped_refptr certificate = + rtc::RTCCertificate::Create(std::unique_ptr( + rtc::SSLIdentity::Generate("testing", rtc::KT_ECDSA))); + std::unique_ptr fingerprint( + rtc::SSLFingerprint::CreateFromCertificate(certificate)); + transport_->SetLocalCertificate(certificate); + + TransportDescription local_desc(kIceUfrag1, kIcePwd1); + TransportDescription remote_desc(kIceUfrag2, kIcePwd2); + // Just use the same fingerprint in both descriptions; the remote fingerprint + // doesn't matter in a non end-to-end test. + local_desc.identity_fingerprint.reset( + TransportDescription::CopyFingerprint(fingerprint.get())); + remote_desc.identity_fingerprint.reset( + TransportDescription::CopyFingerprint(fingerprint.get())); + + remote_desc.connection_role = cricket::CONNECTIONROLE_ACTPASS; + local_desc.connection_role = cricket::CONNECTIONROLE_ACTIVE; + + // Normal initial offer/answer with "actpass" in the offer and "active" in + // the answer. + ASSERT_TRUE(transport_->SetRemoteTransportDescription( + remote_desc, cricket::CA_OFFER, nullptr)); + ASSERT_TRUE(transport_->SetLocalTransportDescription( + local_desc, cricket::CA_ANSWER, nullptr)); + + // Sanity check that role was actually negotiated. + rtc::Optional role = transport_->GetSslRole(); + ASSERT_TRUE(role); + EXPECT_EQ(rtc::SSL_CLIENT, *role); + + // Subsequent offer with "active", which is the opposite of the remote + // endpoint's negotiated role. + // TODO(deadbeef): Really this should fail as soon as the offer is + // attempted to be applied, and not when the answer is applied. + remote_desc.connection_role = cricket::CONNECTIONROLE_ACTIVE; + EXPECT_TRUE(transport_->SetRemoteTransportDescription( + remote_desc, cricket::CA_OFFER, nullptr)); + EXPECT_FALSE(transport_->SetLocalTransportDescription( + local_desc, cricket::CA_ANSWER, nullptr)); +} diff --git a/webrtc/p2p/base/transportcontroller.cc b/webrtc/p2p/base/transportcontroller.cc index 3277ee9702..f06a180088 100644 --- a/webrtc/p2p/base/transportcontroller.cc +++ b/webrtc/p2p/base/transportcontroller.cc @@ -521,7 +521,11 @@ bool TransportController::GetSslRole_n(const std::string& transport_name, if (!t) { return false; } - t->GetSslRole(role); + rtc::Optional current_role = t->GetSslRole(); + if (!current_role) { + return false; + } + *role = *current_role; return true; } diff --git a/webrtc/p2p/base/transportcontroller_unittest.cc b/webrtc/p2p/base/transportcontroller_unittest.cc index 6ea83a4809..44ad54dcbe 100644 --- a/webrtc/p2p/base/transportcontroller_unittest.cc +++ b/webrtc/p2p/base/transportcontroller_unittest.cc @@ -272,13 +272,42 @@ TEST_F(TransportControllerTest, TestIceRoleConflict) { } TEST_F(TransportControllerTest, TestGetSslRole) { - FakeDtlsTransport* transport = CreateFakeDtlsTransport("audio", 1); - ASSERT_NE(nullptr, transport); - ASSERT_TRUE(transport->SetSslRole(rtc::SSL_CLIENT)); rtc::SSLRole role; - EXPECT_FALSE(transport_controller_->GetSslRole("video", &role)); + CreateFakeDtlsTransport("audio", 1); + + // Should return false before role has been negotiated. + EXPECT_FALSE(transport_controller_->GetSslRole("audio", &role)); + + // To negotiate an SSL role, need to set a local certificate, and + // local/remote transport descriptions with DTLS info. + rtc::scoped_refptr certificate = + rtc::RTCCertificate::Create(std::unique_ptr( + rtc::SSLIdentity::Generate("testing", rtc::KT_ECDSA))); + std::unique_ptr fingerprint( + rtc::SSLFingerprint::CreateFromCertificate(certificate)); + transport_controller_->SetLocalCertificate(certificate); + + // Set the same fingerprint on both sides since the remote fingerprint + // doesn't really matter for this test. + TransportDescription local_desc(std::vector(), kIceUfrag1, + kIcePwd1, ICEMODE_FULL, + CONNECTIONROLE_ACTPASS, fingerprint.get()); + TransportDescription remote_desc(std::vector(), kIceUfrag2, + kIcePwd2, ICEMODE_FULL, + CONNECTIONROLE_ACTIVE, fingerprint.get()); + std::string err; + EXPECT_TRUE(transport_controller_->SetLocalTransportDescription( + "audio", local_desc, cricket::CA_OFFER, &err)); + EXPECT_TRUE(transport_controller_->SetRemoteTransportDescription( + "audio", remote_desc, cricket::CA_ANSWER, &err)); + + // Finally we can get the role. Should be "server" since the remote + // endpoint's role was "active". EXPECT_TRUE(transport_controller_->GetSslRole("audio", &role)); - EXPECT_EQ(rtc::SSL_CLIENT, role); + EXPECT_EQ(rtc::SSL_SERVER, role); + + // Lastly, test that GetSslRole returns false for a nonexistent transport. + EXPECT_FALSE(transport_controller_->GetSslRole("video", &role)); } TEST_F(TransportControllerTest, TestSetAndGetLocalCertificate) {