From db67ba1c81f7b2aff2d017d635cdf9f25cefa758 Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Mon, 19 Mar 2018 17:41:42 -0700 Subject: [PATCH] Report SRTP error codes to UMA Bug: webrtc:8996 Change-Id: I75de77ed15c2829425c00f57ebd07109803425db Reviewed-on: https://webrtc-review.googlesource.com/63122 Reviewed-by: Zhi Huang Reviewed-by: Taylor Brandstetter Commit-Queue: Steve Anton Cr-Commit-Position: refs/heads/master@{#22521} --- api/umametrics.h | 3 +++ pc/BUILD.gn | 1 + pc/channel.cc | 15 +++++++++++++++ pc/channel.h | 5 +++++ pc/peerconnection.cc | 16 ++++++++++++++++ pc/rtptransport.h | 3 +++ pc/rtptransportinternal.h | 4 ++++ pc/rtptransportinternaladapter.h | 5 +++++ pc/srtpsession.cc | 13 +++++++++++++ pc/srtpsession.h | 6 ++++++ pc/srtpsession_unittest.cc | 19 +++++++++++++++++++ pc/srtptransport.cc | 27 +++++++++++++++++++++++++++ pc/srtptransport.h | 5 +++++ 13 files changed, 122 insertions(+) diff --git a/api/umametrics.h b/api/umametrics.h index 4de1ce492a..f885416f37 100644 --- a/api/umametrics.h +++ b/api/umametrics.h @@ -42,6 +42,9 @@ enum PeerConnectionEnumCounterType { kEnumCounterSdpSemanticNegotiated, kEnumCounterKeyProtocolMediaType, kEnumCounterSdpFormatReceived, + // The next 2 counters log the value of srtp_err_status_t defined in libsrtp. + kEnumCounterSrtpUnprotectError, + kEnumCounterSrtcpUnprotectError, kPeerConnectionEnumCounterMax }; diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 4f69eef7c6..cb167cd195 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -312,6 +312,7 @@ if (rtc_include_tests) { ":rtc_pc", ":rtc_pc_base", "../api:array_view", + "../api:fakemetricsobserver", "../api:libjingle_peerconnection_api", "../logging:rtc_event_log_api", "../media:rtc_media_base", diff --git a/pc/channel.cc b/pc/channel.cc index 9d7bf86afc..a7ec6680a4 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -144,6 +144,12 @@ void BaseChannel::ConnectToRtpTransport() { &BaseChannel::OnWritableState); rtp_transport_->SignalSentPacket.connect(this, &BaseChannel::SignalSentPacket_n); + // TODO(bugs.webrtc.org/8587): Set the metrics observer through + // JsepTransportController once it takes responsibility for creating + // RtpTransports. + if (metrics_observer_) { + rtp_transport_->SetMetricsObserver(metrics_observer_); + } } void BaseChannel::DisconnectFromRtpTransport() { @@ -153,6 +159,7 @@ void BaseChannel::DisconnectFromRtpTransport() { rtp_transport_->SignalNetworkRouteChanged.disconnect(this); rtp_transport_->SignalWritableState.disconnect(this); rtp_transport_->SignalSentPacket.disconnect(this); + rtp_transport_->SetMetricsObserver(nullptr); } void BaseChannel::Init_w(DtlsTransportInternal* rtp_dtls_transport, @@ -354,6 +361,14 @@ void BaseChannel::SetTransport_n( } } +void BaseChannel::SetMetricsObserver( + rtc::scoped_refptr metrics_observer) { + metrics_observer_ = metrics_observer; + if (rtp_transport_) { + rtp_transport_->SetMetricsObserver(metrics_observer); + } +} + bool BaseChannel::Enable(bool enable) { worker_thread_->Invoke( RTC_FROM_HERE, diff --git a/pc/channel.h b/pc/channel.h index 9ba0492e5d..6a8367a754 100644 --- a/pc/channel.h +++ b/pc/channel.h @@ -204,6 +204,9 @@ class BaseChannel transport_name_ = transport_name; } + void SetMetricsObserver( + rtc::scoped_refptr metrics_observer); + protected: virtual MediaChannel* media_channel() const { return media_channel_.get(); } @@ -395,6 +398,8 @@ class BaseChannel const bool rtcp_mux_required_; + rtc::scoped_refptr metrics_observer_; + // Separate DTLS/non-DTLS pointers to support using BaseChannel without DTLS. // Temporary measure until more refactoring is done. // If non-null, "X_dtls_transport_" will always equal "X_packet_transport_". diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 17977ad31b..64b4b6eeb2 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -2866,6 +2866,13 @@ void PeerConnection::RegisterUMAObserver(UMAObserver* observer) { transport_controller()->SetMetricsObserver(uma_observer_); } + for (auto transceiver : transceivers_) { + auto* channel = transceiver->internal()->channel(); + if (channel) { + channel->SetMetricsObserver(uma_observer_); + } + } + // Send information about IPv4/IPv6 status. if (uma_observer_) { port_allocator_->SetMetricsObserver(uma_observer_); @@ -5420,6 +5427,9 @@ cricket::VoiceChannel* PeerConnection::CreateVoiceChannel( this, &PeerConnection::OnDtlsSrtpSetupFailure); voice_channel->SignalSentPacket.connect(this, &PeerConnection::OnSentPacket_w); + if (uma_observer_) { + voice_channel->SetMetricsObserver(uma_observer_); + } return voice_channel; } @@ -5458,6 +5468,9 @@ cricket::VideoChannel* PeerConnection::CreateVideoChannel( this, &PeerConnection::OnDtlsSrtpSetupFailure); video_channel->SignalSentPacket.connect(this, &PeerConnection::OnSentPacket_w); + if (uma_observer_) { + video_channel->SetMetricsObserver(uma_observer_); + } return video_channel; } @@ -5511,6 +5524,9 @@ bool PeerConnection::CreateDataChannel(const std::string& mid, this, &PeerConnection::OnDtlsSrtpSetupFailure); rtp_data_channel_->SignalSentPacket.connect( this, &PeerConnection::OnSentPacket_w); + if (uma_observer_) { + rtp_data_channel_->SetMetricsObserver(uma_observer_); + } } return true; diff --git a/pc/rtptransport.h b/pc/rtptransport.h index 497a7487b4..637d447454 100644 --- a/pc/rtptransport.h +++ b/pc/rtptransport.h @@ -70,6 +70,9 @@ class RtpTransport : public RtpTransportInternal { void AddHandledPayloadType(int payload_type) override; + void SetMetricsObserver( + rtc::scoped_refptr metrics_observer) override {} + protected: // TODO(zstein): Remove this when we remove RtpTransportAdapter. RtpTransportAdapter* GetInternal() override; diff --git a/pc/rtptransportinternal.h b/pc/rtptransportinternal.h index 2ad66ab1c3..0665fd76bc 100644 --- a/pc/rtptransportinternal.h +++ b/pc/rtptransportinternal.h @@ -14,6 +14,7 @@ #include #include "api/ortc/rtptransportinterface.h" +#include "api/umametrics.h" #include "p2p/base/icetransportinternal.h" #include "rtc_base/networkroute.h" #include "rtc_base/sigslot.h" @@ -82,6 +83,9 @@ class RtpTransportInternal : public RtpTransportInterface, virtual bool HandlesPayloadType(int payload_type) const = 0; virtual void AddHandledPayloadType(int payload_type) = 0; + + virtual void SetMetricsObserver( + rtc::scoped_refptr metrics_observer) = 0; }; } // namespace webrtc diff --git a/pc/rtptransportinternaladapter.h b/pc/rtptransportinternaladapter.h index 402f9cf49b..6a2d7e2aa7 100644 --- a/pc/rtptransportinternaladapter.h +++ b/pc/rtptransportinternaladapter.h @@ -92,6 +92,11 @@ class RtpTransportInternalAdapter : public RtpTransportInternal { return transport_->GetParameters(); } + void SetMetricsObserver( + rtc::scoped_refptr metrics_observer) override { + transport_->SetMetricsObserver(metrics_observer); + } + protected: // Owned by the subclasses. RtpTransportInternal* transport_; diff --git a/pc/srtpsession.cc b/pc/srtpsession.cc index 347b099a96..c0d05b74f4 100644 --- a/pc/srtpsession.cc +++ b/pc/srtpsession.cc @@ -133,6 +133,10 @@ bool SrtpSession::UnprotectRtp(void* p, int in_len, int* out_len) { int err = srtp_unprotect(session_, p, out_len); if (err != srtp_err_status_ok) { RTC_LOG(LS_WARNING) << "Failed to unprotect SRTP packet, err=" << err; + if (metrics_observer_) { + metrics_observer_->IncrementSparseEnumCounter( + webrtc::kEnumCounterSrtpUnprotectError, err); + } return false; } return true; @@ -149,6 +153,10 @@ bool SrtpSession::UnprotectRtcp(void* p, int in_len, int* out_len) { int err = srtp_unprotect_rtcp(session_, p, out_len); if (err != srtp_err_status_ok) { RTC_LOG(LS_WARNING) << "Failed to unprotect SRTCP packet, err=" << err; + if (metrics_observer_) { + metrics_observer_->IncrementSparseEnumCounter( + webrtc::kEnumCounterSrtcpUnprotectError, err); + } return false; } return true; @@ -344,6 +352,11 @@ bool SrtpSession::UpdateKey(int type, return DoSetKey(type, cs, key, len, extension_ids); } +void SrtpSession::SetMetricsObserver( + rtc::scoped_refptr metrics_observer) { + metrics_observer_ = metrics_observer; +} + int g_libsrtp_usage_count = 0; rtc::GlobalLockPod g_libsrtp_lock; diff --git a/pc/srtpsession.h b/pc/srtpsession.h index a6e78fab6b..af534bea22 100644 --- a/pc/srtpsession.h +++ b/pc/srtpsession.h @@ -13,7 +13,9 @@ #include +#include "api/umametrics.h" #include "rtc_base/basictypes.h" +#include "rtc_base/scoped_ref_ptr.h" #include "rtc_base/thread_checker.h" // Forward declaration to avoid pulling in libsrtp headers here @@ -83,6 +85,9 @@ class SrtpSession { // been set. bool IsExternalAuthActive() const; + void SetMetricsObserver( + rtc::scoped_refptr metrics_observer); + private: bool DoSetKey(int type, int cs, @@ -122,6 +127,7 @@ class SrtpSession { int last_send_seq_num_ = -1; bool external_auth_active_ = false; bool external_auth_enabled_ = false; + rtc::scoped_refptr metrics_observer_; RTC_DISALLOW_COPY_AND_ASSIGN(SrtpSession); }; diff --git a/pc/srtpsession_unittest.cc b/pc/srtpsession_unittest.cc index dc325739e8..d73bcbe8f7 100644 --- a/pc/srtpsession_unittest.cc +++ b/pc/srtpsession_unittest.cc @@ -12,13 +12,18 @@ #include +#include "api/fakemetricsobserver.h" #include "media/base/fakertp.h" #include "pc/srtptestutil.h" #include "rtc_base/gunit.h" +#include "rtc_base/ptr_util.h" #include "rtc_base/sslstreamadapter.h" // For rtc::SRTP_* +#include "third_party/libsrtp/include/srtp.h" namespace rtc { +using webrtc::FakeMetricsObserver; + std::vector kEncryptedHeaderExtensionIds; class SrtpSessionTest : public testing::Test { @@ -131,6 +136,9 @@ TEST_F(SrtpSessionTest, TestGetSendStreamPacketIndex) { // Test that we fail to unprotect if someone tampers with the RTP/RTCP paylaods. TEST_F(SrtpSessionTest, TestTamperReject) { + rtc::scoped_refptr metrics_observer( + new rtc::RefCountedObject()); + s2_.SetMetricsObserver(metrics_observer); int out_len; EXPECT_TRUE(s1_.SetSend(SRTP_AES128_CM_SHA1_80, kTestKey1, kTestKeyLen, kEncryptedHeaderExtensionIds)); @@ -141,18 +149,29 @@ TEST_F(SrtpSessionTest, TestTamperReject) { rtp_packet_[0] = 0x12; rtcp_packet_[1] = 0x34; EXPECT_FALSE(s2_.UnprotectRtp(rtp_packet_, rtp_len_, &out_len)); + EXPECT_TRUE(metrics_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterSrtpUnprotectError, srtp_err_status_bad_param)); EXPECT_FALSE(s2_.UnprotectRtcp(rtcp_packet_, rtcp_len_, &out_len)); + EXPECT_TRUE(metrics_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterSrtcpUnprotectError, srtp_err_status_auth_fail)); } // Test that we fail to unprotect if the payloads are not authenticated. TEST_F(SrtpSessionTest, TestUnencryptReject) { + rtc::scoped_refptr metrics_observer( + new rtc::RefCountedObject()); + s2_.SetMetricsObserver(metrics_observer); int out_len; EXPECT_TRUE(s1_.SetSend(SRTP_AES128_CM_SHA1_80, kTestKey1, kTestKeyLen, kEncryptedHeaderExtensionIds)); EXPECT_TRUE(s2_.SetRecv(SRTP_AES128_CM_SHA1_80, kTestKey1, kTestKeyLen, kEncryptedHeaderExtensionIds)); EXPECT_FALSE(s2_.UnprotectRtp(rtp_packet_, rtp_len_, &out_len)); + EXPECT_TRUE(metrics_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterSrtpUnprotectError, srtp_err_status_auth_fail)); EXPECT_FALSE(s2_.UnprotectRtcp(rtcp_packet_, rtcp_len_, &out_len)); + EXPECT_TRUE(metrics_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterSrtcpUnprotectError, srtp_err_status_cant_check)); } // Test that we fail when using buffers that are too small. diff --git a/pc/srtptransport.cc b/pc/srtptransport.cc index 2c827afa58..5eff3c932b 100644 --- a/pc/srtptransport.cc +++ b/pc/srtptransport.cc @@ -259,6 +259,11 @@ bool SrtpTransport::SetRtcpParams(int send_cs, return false; } + if (metrics_observer_) { + send_rtcp_session_->SetMetricsObserver(metrics_observer_); + recv_rtcp_session_->SetMetricsObserver(metrics_observer_); + } + RTC_LOG(LS_INFO) << "SRTCP activated with negotiated parameters:" " send cipher_suite " << send_cs << " recv cipher_suite " << recv_cs; @@ -281,6 +286,10 @@ void SrtpTransport::ResetParams() { void SrtpTransport::CreateSrtpSessions() { send_session_.reset(new cricket::SrtpSession()); recv_session_.reset(new cricket::SrtpSession()); + if (metrics_observer_) { + send_session_->SetMetricsObserver(metrics_observer_); + recv_session_->SetMetricsObserver(metrics_observer_); + } if (external_auth_enabled_) { send_session_->EnableExternalAuth(); @@ -390,4 +399,22 @@ bool SrtpTransport::IsExternalAuthActive() const { return send_session_->IsExternalAuthActive(); } +void SrtpTransport::SetMetricsObserver( + rtc::scoped_refptr metrics_observer) { + metrics_observer_ = metrics_observer; + if (send_session_) { + send_session_->SetMetricsObserver(metrics_observer_); + } + if (recv_session_) { + recv_session_->SetMetricsObserver(metrics_observer_); + } + if (send_rtcp_session_) { + send_rtcp_session_->SetMetricsObserver(metrics_observer_); + } + if (recv_rtcp_session_) { + recv_rtcp_session_->SetMetricsObserver(metrics_observer_); + } + rtp_transport_->SetMetricsObserver(metrics_observer); +} + } // namespace webrtc diff --git a/pc/srtptransport.h b/pc/srtptransport.h index d8b0dbd35a..818aabb2a3 100644 --- a/pc/srtptransport.h +++ b/pc/srtptransport.h @@ -98,6 +98,9 @@ class SrtpTransport : public RtpTransportInternalAdapter { rtp_abs_sendtime_extn_id_ = rtp_abs_sendtime_extn_id; } + void SetMetricsObserver( + rtc::scoped_refptr metrics_observer) override; + private: void ConnectToRtpTransport(); void CreateSrtpSessions(); @@ -146,6 +149,8 @@ class SrtpTransport : public RtpTransportInternalAdapter { bool external_auth_enabled_ = false; int rtp_abs_sendtime_extn_id_ = -1; + + rtc::scoped_refptr metrics_observer_; }; } // namespace webrtc