Accept remote offers with current DTLS role, rather than "actpass".

JSEP implementations are required to always generate offers with
"actpass", but remote endpoints are not. So we should accept remote
offers with the current negotiated DTLS role.

This was recently clarified in dtls-sdp; it was somewhat ambiguous
before.

Also doing a bit of refactoring of JsepTransport (making a method
private that should have been private, fixing unit tests that were
directly calling said method).

BUG=webrtc:7072

Review-Url: https://codereview.webrtc.org/2770903003
Cr-Commit-Position: refs/heads/master@{#17396}
This commit is contained in:
deadbeef 2017-03-27 10:33:26 -07:00 committed by Commit bot
parent a5e8aa6e2b
commit d8cfa1af38
6 changed files with 254 additions and 74 deletions

View File

@ -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

View File

@ -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<rtc::SSLRole> 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;
}

View File

@ -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<rtc::SSLRole> 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<rtc::RTCCertificate> certificate_;
rtc::SSLRole secure_role_ = rtc::SSL_CLIENT;
rtc::Optional<rtc::SSLRole> ssl_role_;
std::unique_ptr<rtc::SSLFingerprint> remote_fingerprint_;
std::unique_ptr<TransportDescription> local_description_;
std::unique_ptr<TransportDescription> remote_description_;

View File

@ -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));
}

View File

@ -521,7 +521,11 @@ bool TransportController::GetSslRole_n(const std::string& transport_name,
if (!t) {
return false;
}
t->GetSslRole(role);
rtc::Optional<rtc::SSLRole> current_role = t->GetSslRole();
if (!current_role) {
return false;
}
*role = *current_role;
return true;
}

View File

@ -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<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_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<std::string>(), kIceUfrag1,
kIcePwd1, ICEMODE_FULL,
CONNECTIONROLE_ACTPASS, fingerprint.get());
TransportDescription remote_desc(std::vector<std::string>(), 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) {