diff --git a/p2p/base/dtls_transport.cc b/p2p/base/dtls_transport.cc index 2ebc983e10..904a0cbbc9 100644 --- a/p2p/base/dtls_transport.cc +++ b/p2p/base/dtls_transport.cc @@ -227,6 +227,34 @@ bool DtlsTransport::GetSslCipherSuite(int* cipher) { return dtls_->GetSslCipherSuite(cipher); } +webrtc::RTCError DtlsTransport::SetRemoteParameters( + absl::string_view digest_alg, + const uint8_t* digest, + size_t digest_len, + absl::optional role) { + rtc::Buffer remote_fingerprint_value(digest, digest_len); + bool is_dtls_restart = + dtls_active_ && remote_fingerprint_value_ != remote_fingerprint_value; + // Set SSL role. Role must be set before fingerprint is applied, which + // initiates DTLS setup. + if (role) { + if (is_dtls_restart) { + dtls_role_ = *role; + } else { + if (!SetDtlsRole(*role)) { + return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, + "Failed to set SSL role for the transport."); + } + } + } + // Apply remote fingerprint. + if (!SetRemoteFingerprint(digest_alg, digest, digest_len)) { + return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, + "Failed to apply remote fingerprint."); + } + return webrtc::RTCError::OK(); +} + bool DtlsTransport::SetRemoteFingerprint(absl::string_view digest_alg, const uint8_t* digest, size_t digest_len) { diff --git a/p2p/base/dtls_transport.h b/p2p/base/dtls_transport.h index a78226c952..24ac16e9b9 100644 --- a/p2p/base/dtls_transport.h +++ b/p2p/base/dtls_transport.h @@ -133,12 +133,12 @@ class DtlsTransport : public DtlsTransportInternal { const rtc::scoped_refptr& certificate) override; rtc::scoped_refptr GetLocalCertificate() const override; - // SetRemoteFingerprint must be called after SetLocalCertificate, and any - // other methods like SetDtlsRole. It's what triggers the actual DTLS setup. - // TODO(deadbeef): Rename to "Start" like in ORTC? - bool SetRemoteFingerprint(absl::string_view digest_alg, - const uint8_t* digest, - size_t digest_len) override; + // SetRemoteParameters must be called after SetLocalCertificate. + webrtc::RTCError SetRemoteParameters( + absl::string_view digest_alg, + const uint8_t* digest, + size_t digest_len, + absl::optional role) override; // Called to send a packet (via DTLS, if turned on). int SendPacket(const char* data, @@ -226,6 +226,13 @@ class DtlsTransport : public DtlsTransportInternal { // Sets the DTLS state, signaling if necessary. void set_dtls_state(webrtc::DtlsTransportState state); + // SetRemoteFingerprint must be called after SetLocalCertificate, and any + // other methods like SetDtlsRole. It's what triggers the actual DTLS setup. + // TODO(deadbeef): Rename to "Start" like in ORTC? + bool SetRemoteFingerprint(absl::string_view digest_alg, + const uint8_t* digest, + size_t digest_len); + webrtc::SequenceChecker thread_checker_; const int component_; diff --git a/p2p/base/dtls_transport_internal.h b/p2p/base/dtls_transport_internal.h index 061d1e97f2..d9755309c0 100644 --- a/p2p/base/dtls_transport_internal.h +++ b/p2p/base/dtls_transport_internal.h @@ -89,10 +89,12 @@ class DtlsTransportInternal : public rtc::PacketTransportInternal { uint8_t* result, size_t result_len) = 0; - // Set DTLS remote fingerprint. Must be after local identity set. - virtual bool SetRemoteFingerprint(absl::string_view digest_alg, - const uint8_t* digest, - size_t digest_len) = 0; + // Set DTLS remote fingerprint and role. Must be after local identity set. + virtual webrtc::RTCError SetRemoteParameters( + absl::string_view digest_alg, + const uint8_t* digest, + size_t digest_len, + absl::optional role) = 0; ABSL_DEPRECATED("Set the max version via construction.") bool SetSslMaxProtocolVersion(rtc::SSLProtocolVersion version) { diff --git a/p2p/base/dtls_transport_unittest.cc b/p2p/base/dtls_transport_unittest.cc index cb04bf7bb1..69c36f8a59 100644 --- a/p2p/base/dtls_transport_unittest.cc +++ b/p2p/base/dtls_transport_unittest.cc @@ -56,11 +56,14 @@ void SetRemoteFingerprintFromCert( if (modify_digest) { ++fingerprint->digest.MutableData()[0]; } - // Even if digest is verified to be incorrect, should fail asynchrnously. - EXPECT_TRUE(transport->SetRemoteFingerprint( - fingerprint->algorithm, - reinterpret_cast(fingerprint->digest.data()), - fingerprint->digest.size())); + // Even if digest is verified to be incorrect, should fail asynchronously. + EXPECT_TRUE( + transport + ->SetRemoteParameters( + fingerprint->algorithm, + reinterpret_cast(fingerprint->digest.data()), + fingerprint->digest.size(), absl::nullopt) + .ok()); } class DtlsTestClient : public sigslot::has_slots<> { diff --git a/p2p/base/fake_dtls_transport.h b/p2p/base/fake_dtls_transport.h index 23d47107cf..571c0ed5a5 100644 --- a/p2p/base/fake_dtls_transport.h +++ b/p2p/base/fake_dtls_transport.h @@ -141,12 +141,15 @@ class FakeDtlsTransport : public DtlsTransportInternal { const rtc::SSLFingerprint& dtls_fingerprint() const { return dtls_fingerprint_; } - bool SetRemoteFingerprint(absl::string_view alg, - const uint8_t* digest, - size_t digest_len) override { - dtls_fingerprint_ = - rtc::SSLFingerprint(alg, rtc::MakeArrayView(digest, digest_len)); - return true; + webrtc::RTCError SetRemoteParameters(absl::string_view alg, + const uint8_t* digest, + size_t digest_len, + absl::optional role) { + if (role) { + SetDtlsRole(*role); + } + SetRemoteFingerprint(alg, digest, digest_len); + return webrtc::RTCError::OK(); } bool SetDtlsRole(rtc::SSLRole role) override { dtls_role_ = std::move(role); @@ -283,6 +286,14 @@ class FakeDtlsTransport : public DtlsTransportInternal { SignalNetworkRouteChanged(network_route); } + bool SetRemoteFingerprint(absl::string_view alg, + const uint8_t* digest, + size_t digest_len) { + dtls_fingerprint_ = + rtc::SSLFingerprint(alg, rtc::MakeArrayView(digest, digest_len)); + return true; + } + FakeIceTransport* ice_transport_; std::unique_ptr owned_ice_transport_; std::string transport_name_; diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index a2ac757297..dad415b93b 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc @@ -420,21 +420,9 @@ webrtc::RTCError JsepTransport::SetNegotiatedDtlsParameters( absl::optional dtls_role, rtc::SSLFingerprint* remote_fingerprint) { RTC_DCHECK(dtls_transport); - // Set SSL role. Role must be set before fingerprint is applied, which - // initiates DTLS setup. - if (dtls_role && !dtls_transport->SetDtlsRole(*dtls_role)) { - return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, - "Failed to set SSL role for the transport."); - } - // Apply remote fingerprint. - if (!remote_fingerprint || - !dtls_transport->SetRemoteFingerprint( - remote_fingerprint->algorithm, remote_fingerprint->digest.cdata(), - remote_fingerprint->digest.size())) { - return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, - "Failed to apply remote fingerprint."); - } - return webrtc::RTCError::OK(); + return dtls_transport->SetRemoteParameters( + remote_fingerprint->algorithm, remote_fingerprint->digest.cdata(), + remote_fingerprint->digest.size(), dtls_role); } bool JsepTransport::SetRtcpMux(bool enable, diff --git a/pc/jsep_transport_unittest.cc b/pc/jsep_transport_unittest.cc index d0cdb7500d..f057d37a0d 100644 --- a/pc/jsep_transport_unittest.cc +++ b/pc/jsep_transport_unittest.cc @@ -881,6 +881,61 @@ TEST_F(JsepTransport2Test, RemoteOfferThatChangesNegotiatedDtlsRole) { .ok()); } +// Test that a remote offer which changes both fingerprint and role is accepted. +TEST_F(JsepTransport2Test, RemoteOfferThatChangesFingerprintAndDtlsRole) { + rtc::scoped_refptr certificate = + rtc::RTCCertificate::Create( + rtc::SSLIdentity::Create("testing1", rtc::KT_ECDSA)); + rtc::scoped_refptr certificate2 = + rtc::RTCCertificate::Create( + rtc::SSLIdentity::Create("testing2", rtc::KT_ECDSA)); + bool rtcp_mux_enabled = true; + jsep_transport_ = CreateJsepTransport2(rtcp_mux_enabled, SrtpMode::kDtlsSrtp); + jsep_transport_->SetLocalCertificate(certificate); + + JsepTransportDescription remote_desc = + MakeJsepTransportDescription(rtcp_mux_enabled, kIceUfrag1, kIcePwd1, + certificate, CONNECTIONROLE_ACTPASS); + JsepTransportDescription remote_desc2 = + MakeJsepTransportDescription(rtcp_mux_enabled, kIceUfrag1, kIcePwd1, + certificate2, CONNECTIONROLE_ACTPASS); + + JsepTransportDescription local_desc = + MakeJsepTransportDescription(rtcp_mux_enabled, kIceUfrag2, kIcePwd2, + certificate, CONNECTIONROLE_ACTIVE); + + // Normal initial offer/answer with "actpass" in the offer and "active" in + // the answer. + ASSERT_TRUE( + jsep_transport_ + ->SetRemoteJsepTransportDescription(remote_desc, SdpType::kOffer) + .ok()); + ASSERT_TRUE( + jsep_transport_ + ->SetLocalJsepTransportDescription(local_desc, SdpType::kAnswer) + .ok()); + + // Sanity check that role was actually negotiated. + absl::optional role = jsep_transport_->GetDtlsRole(); + ASSERT_TRUE(role); + EXPECT_EQ(rtc::SSL_CLIENT, *role); + + // Subsequent exchange with new remote fingerprint and different role. + local_desc.transport_desc.connection_role = CONNECTIONROLE_PASSIVE; + EXPECT_TRUE( + jsep_transport_ + ->SetRemoteJsepTransportDescription(remote_desc2, SdpType::kOffer) + .ok()); + EXPECT_TRUE( + jsep_transport_ + ->SetLocalJsepTransportDescription(local_desc, SdpType::kAnswer) + .ok()); + + role = jsep_transport_->GetDtlsRole(); + ASSERT_TRUE(role); + EXPECT_EQ(rtc::SSL_SERVER, *role); +} + // Testing that a legacy client that doesn't use the setup attribute will be // interpreted as having an active role. TEST_F(JsepTransport2Test, DtlsSetupWithLegacyAsAnswerer) { @@ -912,7 +967,7 @@ TEST_F(JsepTransport2Test, DtlsSetupWithLegacyAsAnswerer) { absl::optional role = jsep_transport_->GetDtlsRole(); ASSERT_TRUE(role); - // Since legacy answer ommitted setup atribute, and we offered actpass, we + // Since legacy answer omitted setup atribute, and we offered actpass, we // should act as passive (server). EXPECT_EQ(rtc::SSL_SERVER, *role); }