Reland "dtls: allow dtls role to change during DTLS restart"
This is a reland of commit 02b5f3c9c12cddf3fc6e9125238b77ddb44f3b53
without making SetRemoteFingerprint private (but adding a deprecation warning)
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 <hta@webrtc.org>
> Commit-Queue: Philipp Hancke <phancke@microsoft.com>
> Cr-Commit-Position: refs/heads/main@{#37821}
Bug: webrtc:5768
Change-Id: I8dd674db8b683160013e1b4aa7776775d130978f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272221
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#37838}
diff --git a/p2p/base/dtls_transport.cc b/p2p/base/dtls_transport.cc
index 2ebc983..904a0cb 100644
--- a/p2p/base/dtls_transport.cc
+++ b/p2p/base/dtls_transport.cc
@@ -227,6 +227,34 @@
return dtls_->GetSslCipherSuite(cipher);
}
+webrtc::RTCError DtlsTransport::SetRemoteParameters(
+ absl::string_view digest_alg,
+ const uint8_t* digest,
+ size_t digest_len,
+ absl::optional<rtc::SSLRole> 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 a78226c..2b26e25 100644
--- a/p2p/base/dtls_transport.h
+++ b/p2p/base/dtls_transport.h
@@ -140,6 +140,13 @@
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<rtc::SSLRole> role) override;
+
// Called to send a packet (via DTLS, if turned on).
int SendPacket(const char* data,
size_t size,
diff --git a/p2p/base/dtls_transport_internal.h b/p2p/base/dtls_transport_internal.h
index 061d1e9..3d20d1b 100644
--- a/p2p/base/dtls_transport_internal.h
+++ b/p2p/base/dtls_transport_internal.h
@@ -90,10 +90,18 @@
size_t result_len) = 0;
// Set DTLS remote fingerprint. Must be after local identity set.
+ ABSL_DEPRECATED("Use SetRemoteParameters instead.")
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<rtc::SSLRole> role) = 0;
+
ABSL_DEPRECATED("Set the max version via construction.")
bool SetSslMaxProtocolVersion(rtc::SSLProtocolVersion version) {
return true;
diff --git a/p2p/base/dtls_transport_unittest.cc b/p2p/base/dtls_transport_unittest.cc
index cb04bf7..e338ab6 100644
--- a/p2p/base/dtls_transport_unittest.cc
+++ b/p2p/base/dtls_transport_unittest.cc
@@ -56,11 +56,15 @@
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<const uint8_t*>(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<const uint8_t*>(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 23d4710..283488b 100644
--- a/p2p/base/fake_dtls_transport.h
+++ b/p2p/base/fake_dtls_transport.h
@@ -141,9 +141,19 @@
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<rtc::SSLRole> 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 {
+ size_t digest_len) {
dtls_fingerprint_ =
rtc::SSLFingerprint(alg, rtc::MakeArrayView(digest, digest_len));
return true;
diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc
index a2ac757..dad415b 100644
--- a/pc/jsep_transport.cc
+++ b/pc/jsep_transport.cc
@@ -420,21 +420,9 @@
absl::optional<rtc::SSLRole> 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 d0cdb75..f057d37 100644
--- a/pc/jsep_transport_unittest.cc
+++ b/pc/jsep_transport_unittest.cc
@@ -881,6 +881,61 @@
.ok());
}
+// Test that a remote offer which changes both fingerprint and role is accepted.
+TEST_F(JsepTransport2Test, RemoteOfferThatChangesFingerprintAndDtlsRole) {
+ rtc::scoped_refptr<rtc::RTCCertificate> certificate =
+ rtc::RTCCertificate::Create(
+ rtc::SSLIdentity::Create("testing1", rtc::KT_ECDSA));
+ rtc::scoped_refptr<rtc::RTCCertificate> 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<rtc::SSLRole> 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 @@
absl::optional<rtc::SSLRole> 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);
}
