Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(254)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: webrtc/p2p/base/jseptransport_unittest.cc

Issue 2770903003: Accept remote offers with current DTLS role, rather than "actpass". (Closed)
Patch Set: Created 3 years, 9 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« webrtc/p2p/base/jseptransport.cc ('K') | « webrtc/p2p/base/jseptransport.cc ('k') | webrtc/p2p/base/transportcontroller.cc » ('j') | webrtc/p2p/base/transportcontroller_unittest.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: webrtc/p2p/base/jseptransport_unittest.cc
diff --git a/webrtc/p2p/base/jseptransport_unittest.cc b/webrtc/p2p/base/jseptransport_unittest.cc
index ccb7016e0531f82a2b5c8376772813a3f3484dfd..23ed4d857df70378cdc5d90eef8ca19040886cb1 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<FakeDtlsTransport> fake_dtls_transport_;
std::unique_ptr<FakeIceTransport> 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<rtc::RTCCertificate> certificate =
+ rtc::RTCCertificate::Create(std::unique_ptr<rtc::SSLIdentity>(
+ rtc::SSLIdentity::Generate("testing", rtc::KT_ECDSA)));
+ std::unique_ptr<rtc::SSLFingerprint> 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<rtc::RTCCertificate> certificate =
+ rtc::RTCCertificate::Create(std::unique_ptr<rtc::SSLIdentity>(
+ rtc::SSLIdentity::Generate("testing", rtc::KT_ECDSA)));
+ std::unique_ptr<rtc::SSLFingerprint> 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<rtc::SSLRole> 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<rtc::RTCCertificate> certificate =
+ rtc::RTCCertificate::Create(std::unique_ptr<rtc::SSLIdentity>(
+ rtc::SSLIdentity::Generate("testing", rtc::KT_ECDSA)));
+ std::unique_ptr<rtc::SSLFingerprint> 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<rtc::SSLRole> 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));
+}
« webrtc/p2p/base/jseptransport.cc ('K') | « webrtc/p2p/base/jseptransport.cc ('k') | webrtc/p2p/base/transportcontroller.cc » ('j') | webrtc/p2p/base/transportcontroller_unittest.cc » ('J')

Powered by Google App Engine
This is Rietveld 408576698