From fb5fc4307d8df048fb6351b52fc627a179fbd9f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Terelius?= Date: Thu, 18 Aug 2022 11:48:23 +0000 Subject: [PATCH] Revert "dtls: allow dtls role to change during DTLS restart" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 02b5f3c9c12cddf3fc6e9125238b77ddb44f3b53. Reason for revert: SetRemoteFingerprint called by downstream code. Original change's description: > dtls: allow dtls role to change during DTLS restart > > which is characterized by a change in remote fingerprint and > causes a new DTLS handshake. This allows renegotiating the > client/server role as well. > Spec guidance is provided by > https://www.rfc-editor.org/rfc/rfc5763#section-6.6 > > BUG=webrtc:5768 > > Change-Id: I0e8630c0c5907cc92720762a4320ad21a6190d28 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/271680 > Reviewed-by: Harald Alvestrand > Commit-Queue: Philipp Hancke > Cr-Commit-Position: refs/heads/main@{#37821} Bug: webrtc:5768 Change-Id: I266b7fdc9cc0b6dc9d3fa732fca37407b98e0816 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272220 Owners-Override: Björn Terelius Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Commit-Queue: Björn Terelius Cr-Commit-Position: refs/heads/main@{#37822} --- p2p/base/dtls_transport.cc | 28 -------------- p2p/base/dtls_transport.h | 19 +++------- p2p/base/dtls_transport_internal.h | 10 ++--- p2p/base/dtls_transport_unittest.cc | 13 +++---- p2p/base/fake_dtls_transport.h | 23 +++--------- pc/jsep_transport.cc | 18 +++++++-- pc/jsep_transport_unittest.cc | 57 +---------------------------- 7 files changed, 37 insertions(+), 131 deletions(-) diff --git a/p2p/base/dtls_transport.cc b/p2p/base/dtls_transport.cc index 904a0cbbc9..2ebc983e10 100644 --- a/p2p/base/dtls_transport.cc +++ b/p2p/base/dtls_transport.cc @@ -227,34 +227,6 @@ 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 24ac16e9b9..a78226c952 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; - // 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; + // 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; // Called to send a packet (via DTLS, if turned on). int SendPacket(const char* data, @@ -226,13 +226,6 @@ 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 d9755309c0..061d1e97f2 100644 --- a/p2p/base/dtls_transport_internal.h +++ b/p2p/base/dtls_transport_internal.h @@ -89,12 +89,10 @@ class DtlsTransportInternal : public rtc::PacketTransportInternal { uint8_t* result, size_t result_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; + // 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; 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 69c36f8a59..cb04bf7bb1 100644 --- a/p2p/base/dtls_transport_unittest.cc +++ b/p2p/base/dtls_transport_unittest.cc @@ -56,14 +56,11 @@ void SetRemoteFingerprintFromCert( if (modify_digest) { ++fingerprint->digest.MutableData()[0]; } - // 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()); + // 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())); } class DtlsTestClient : public sigslot::has_slots<> { diff --git a/p2p/base/fake_dtls_transport.h b/p2p/base/fake_dtls_transport.h index 571c0ed5a5..23d47107cf 100644 --- a/p2p/base/fake_dtls_transport.h +++ b/p2p/base/fake_dtls_transport.h @@ -141,15 +141,12 @@ class FakeDtlsTransport : public DtlsTransportInternal { const rtc::SSLFingerprint& dtls_fingerprint() const { return dtls_fingerprint_; } - 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 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; } bool SetDtlsRole(rtc::SSLRole role) override { dtls_role_ = std::move(role); @@ -286,14 +283,6 @@ 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 dad415b93b..a2ac757297 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc @@ -420,9 +420,21 @@ webrtc::RTCError JsepTransport::SetNegotiatedDtlsParameters( absl::optional dtls_role, rtc::SSLFingerprint* remote_fingerprint) { RTC_DCHECK(dtls_transport); - return dtls_transport->SetRemoteParameters( - remote_fingerprint->algorithm, remote_fingerprint->digest.cdata(), - remote_fingerprint->digest.size(), dtls_role); + // 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(); } bool JsepTransport::SetRtcpMux(bool enable, diff --git a/pc/jsep_transport_unittest.cc b/pc/jsep_transport_unittest.cc index f057d37a0d..d0cdb7500d 100644 --- a/pc/jsep_transport_unittest.cc +++ b/pc/jsep_transport_unittest.cc @@ -881,61 +881,6 @@ 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) { @@ -967,7 +912,7 @@ TEST_F(JsepTransport2Test, DtlsSetupWithLegacyAsAnswerer) { absl::optional role = jsep_transport_->GetDtlsRole(); ASSERT_TRUE(role); - // Since legacy answer omitted setup atribute, and we offered actpass, we + // Since legacy answer ommitted setup atribute, and we offered actpass, we // should act as passive (server). EXPECT_EQ(rtc::SSL_SERVER, *role); }