From 69d23c93866bcb63b21bb7e5c9d88b717d051c23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Mon, 26 Sep 2022 14:13:17 +0200 Subject: [PATCH] Add RTCCertificateStats cache to avoid rtc::SSLCertChain::GetStats. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unlike the cache of the entire stats report which is time limited, this certificate cache is valid for an unlimited amount of time, but is cleared at ClearCachedStatsReport() which is already called on each SLD/SRD call. Since certificates can only change by negotiation, this cache is ensured to always be invalidated when certificates change. Since ClearCachedStatsReport() can happen for other reasons than certificates changing we may clear the cache more often then is necessary, but arguably this is seldom enough that we don't have to create a separate "ClearCertificateStats()" method. Keep it simple? The cache specifically avoids rtc::SSLCertChain::GetStats which trigger rtc::SSLCertificate::GetStats and rtc::Base64::EncodeFromArray. Bug: webrtc:14458 Change-Id: I5f95a4a5eb51cc4462147270fdae7bb9fb7bc822 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/276602 Reviewed-by: Harald Alvestrand Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/main@{#38205} --- pc/rtc_stats_collector.cc | 56 +++++++++---- pc/rtc_stats_collector.h | 14 +++- pc/rtc_stats_collector_unittest.cc | 130 +++++++++++++++++++++++++++++ rtc_base/ssl_certificate.cc | 6 ++ rtc_base/ssl_certificate.h | 2 + 5 files changed, 190 insertions(+), 18 deletions(-) diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc index 464e22c3f5..407f33b237 100644 --- a/pc/rtc_stats_collector.cc +++ b/pc/rtc_stats_collector.cc @@ -1254,6 +1254,14 @@ rtc::scoped_refptr CreateReportFilteredBySelector( } // namespace +RTCStatsCollector::CertificateStatsPair +RTCStatsCollector::CertificateStatsPair::Copy() const { + CertificateStatsPair copy; + copy.local = local ? local->Copy() : nullptr; + copy.remote = remote ? remote->Copy() : nullptr; + return copy; +} + RTCStatsCollector::RequestInfo::RequestInfo( rtc::scoped_refptr callback) : RequestInfo(FilterMode::kAll, std::move(callback), nullptr, nullptr) {} @@ -1386,6 +1394,7 @@ void RTCStatsCollector::GetStatsReportInternal( void RTCStatsCollector::ClearCachedStatsReport() { RTC_DCHECK_RUN_ON(signaling_thread_); cached_report_ = nullptr; + cached_certificates_by_transport_.clear(); } void RTCStatsCollector::WaitForPendingRequest() { @@ -2309,29 +2318,44 @@ void RTCStatsCollector::ProduceTransportStats_n( std::map RTCStatsCollector::PrepareTransportCertificateStats_n( const std::map& - transport_stats_by_name) const { + transport_stats_by_name) { RTC_DCHECK_RUN_ON(network_thread_); rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; std::map transport_cert_stats; - for (const auto& entry : transport_stats_by_name) { - const std::string& transport_name = entry.first; - - CertificateStatsPair certificate_stats_pair; - rtc::scoped_refptr local_certificate; - if (pc_->GetLocalCertificate(transport_name, &local_certificate)) { - certificate_stats_pair.local = - local_certificate->GetSSLCertificateChain().GetStats(); + if (!cached_certificates_by_transport_.empty()) { + // Copy the certificate info from the cache, avoiding expensive + // rtc::SSLCertChain::GetStats() calls. + for (const auto& pair : cached_certificates_by_transport_) { + transport_cert_stats.insert( + std::make_pair(pair.first, pair.second.Copy())); } + } else { + // Collect certificate info. + for (const auto& entry : transport_stats_by_name) { + const std::string& transport_name = entry.first; - std::unique_ptr remote_cert_chain = - pc_->GetRemoteSSLCertChain(transport_name); - if (remote_cert_chain) { - certificate_stats_pair.remote = remote_cert_chain->GetStats(); + CertificateStatsPair certificate_stats_pair; + rtc::scoped_refptr local_certificate; + if (pc_->GetLocalCertificate(transport_name, &local_certificate)) { + certificate_stats_pair.local = + local_certificate->GetSSLCertificateChain().GetStats(); + } + + std::unique_ptr remote_cert_chain = + pc_->GetRemoteSSLCertChain(transport_name); + if (remote_cert_chain) { + certificate_stats_pair.remote = remote_cert_chain->GetStats(); + } + + transport_cert_stats.insert( + std::make_pair(transport_name, std::move(certificate_stats_pair))); + } + // Copy the result into the certificate cache for future reference. + for (const auto& pair : transport_cert_stats) { + cached_certificates_by_transport_.insert( + std::make_pair(pair.first, pair.second.Copy())); } - - transport_cert_stats.insert( - std::make_pair(transport_name, std::move(certificate_stats_pair))); } return transport_cert_stats; } diff --git a/pc/rtc_stats_collector.h b/pc/rtc_stats_collector.h index b1909cb4c4..e5c6aedd88 100644 --- a/pc/rtc_stats_collector.h +++ b/pc/rtc_stats_collector.h @@ -79,7 +79,11 @@ class RTCStatsCollector : public rtc::RefCountInterface, void GetStatsReport(rtc::scoped_refptr selector, rtc::scoped_refptr callback); // Clears the cache's reference to the most recent stats report. Subsequently - // calling `GetStatsReport` guarantees fresh stats. + // calling `GetStatsReport` guarantees fresh stats. This method must be called + // any time the PeerConnection visibly changes as a result of an API call as + // per + // https://w3c.github.io/webrtc-stats/#guidelines-for-getstats-results-caching-throttling + // and it must be called any time negotiation happens. void ClearCachedStatsReport(); // If there is a `GetStatsReport` requests in-flight, waits until it has been @@ -93,6 +97,8 @@ class RTCStatsCollector : public rtc::RefCountInterface, struct CertificateStatsPair { std::unique_ptr local; std::unique_ptr remote; + + CertificateStatsPair Copy() const; }; // Stats gathering on a particular thread. Virtual for the sake of testing. @@ -227,7 +233,7 @@ class RTCStatsCollector : public rtc::RefCountInterface, std::map PrepareTransportCertificateStats_n( const std::map& - transport_stats_by_name) const; + transport_stats_by_name); // The results are stored in `transceiver_stats_infos_` and `call_stats_`. void PrepareTransceiverStatsInfosAndCallStats_s_w_n(); @@ -279,6 +285,10 @@ class RTCStatsCollector : public rtc::RefCountInterface, // now get rid of the variable and keep the data scoped within a stats // collection sequence. std::vector transceiver_stats_infos_; + // This cache avoids having to call rtc::SSLCertChain::GetStats(), which can + // relatively expensive. ClearCachedStatsReport() needs to be called on + // negotiation to ensure the cache is not obsolete. + std::map cached_certificates_by_transport_; Call::Stats call_stats_; diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index 9d7ae249c1..88bb6aefa5 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc @@ -677,6 +677,18 @@ class RTCStatsCollectorTest : public ::testing::Test { } } + const RTCCertificateStats* GetCertificateStatsFromFingerprint( + const rtc::scoped_refptr& report, + const std::string& fingerprint) { + auto certificates = report->GetStatsOfType(); + for (const auto* certificate : certificates) { + if (*certificate->fingerprint == fingerprint) { + return certificate; + } + } + return nullptr; + } + struct ExampleStatsGraph { rtc::scoped_refptr sender; rtc::scoped_refptr receiver; @@ -1360,6 +1372,124 @@ TEST_F(RTCStatsCollectorTest, CollectRTCCertificateStatsChain) { ExpectReportContainsCertificateInfo(report, *remote_certinfo); } +TEST_F(RTCStatsCollectorTest, CertificateStatsCache) { + const char kTransportName[] = "transport"; + rtc::ScopedFakeClock fake_clock; + + pc_->AddVoiceChannel("audio", kTransportName); + + // Set local and remote cerificates. + std::unique_ptr initial_local_certinfo = + CreateFakeCertificateAndInfoFromDers({"LocalCertA", "LocalCertB"}); + pc_->SetLocalCertificate(kTransportName, initial_local_certinfo->certificate); + std::unique_ptr initial_remote_certinfo = + CreateFakeCertificateAndInfoFromDers({"RemoteCertA", "RemoteCertB"}); + pc_->SetRemoteCertChain( + kTransportName, + initial_remote_certinfo->certificate->GetSSLCertificateChain().Clone()); + ASSERT_EQ(initial_local_certinfo->fingerprints.size(), 2u); + ASSERT_EQ(initial_remote_certinfo->fingerprints.size(), 2u); + + rtc::scoped_refptr first_report = + stats_->GetStatsReport(); + const auto* first_local_cert0 = GetCertificateStatsFromFingerprint( + first_report, initial_local_certinfo->fingerprints[0]); + const auto* first_local_cert1 = GetCertificateStatsFromFingerprint( + first_report, initial_local_certinfo->fingerprints[1]); + const auto* first_remote_cert0 = GetCertificateStatsFromFingerprint( + first_report, initial_remote_certinfo->fingerprints[0]); + const auto* first_remote_cert1 = GetCertificateStatsFromFingerprint( + first_report, initial_remote_certinfo->fingerprints[1]); + ASSERT_TRUE(first_local_cert0); + ASSERT_TRUE(first_local_cert1); + ASSERT_TRUE(first_remote_cert0); + ASSERT_TRUE(first_remote_cert1); + EXPECT_EQ(first_local_cert0->timestamp_us(), rtc::TimeMicros()); + EXPECT_EQ(first_local_cert1->timestamp_us(), rtc::TimeMicros()); + EXPECT_EQ(first_remote_cert0->timestamp_us(), rtc::TimeMicros()); + EXPECT_EQ(first_remote_cert1->timestamp_us(), rtc::TimeMicros()); + + // Replace all certificates. + std::unique_ptr updated_local_certinfo = + CreateFakeCertificateAndInfoFromDers( + {"UpdatedLocalCertA", "UpdatedLocalCertB"}); + pc_->SetLocalCertificate(kTransportName, updated_local_certinfo->certificate); + std::unique_ptr updated_remote_certinfo = + CreateFakeCertificateAndInfoFromDers( + {"UpdatedRemoteCertA", "UpdatedRemoteCertB"}); + pc_->SetRemoteCertChain( + kTransportName, + updated_remote_certinfo->certificate->GetSSLCertificateChain().Clone()); + // This test assumes fingerprints are different for the old and new + // certificates. + EXPECT_NE(initial_local_certinfo->fingerprints, + updated_local_certinfo->fingerprints); + EXPECT_NE(initial_remote_certinfo->fingerprints, + updated_remote_certinfo->fingerprints); + + // Advance time to ensure a fresh stats report, but don't clear the + // certificate stats cache. + fake_clock.AdvanceTime(TimeDelta::Seconds(1)); + rtc::scoped_refptr second_report = + stats_->GetStatsReport(); + // We expect to see the same certificates as before due to not clearing the + // certificate cache. + const auto* second_local_cert0 = + second_report->GetAs(first_local_cert0->id()); + const auto* second_local_cert1 = + second_report->GetAs(first_local_cert1->id()); + const auto* second_remote_cert0 = + second_report->GetAs(first_remote_cert0->id()); + const auto* second_remote_cert1 = + second_report->GetAs(first_remote_cert1->id()); + ASSERT_TRUE(second_local_cert0); + ASSERT_TRUE(second_local_cert1); + ASSERT_TRUE(second_remote_cert0); + ASSERT_TRUE(second_remote_cert1); + // The information in the certificate stats are obsolete. + EXPECT_EQ(*second_local_cert0->fingerprint, + initial_local_certinfo->fingerprints[0]); + EXPECT_EQ(*second_local_cert1->fingerprint, + initial_local_certinfo->fingerprints[1]); + EXPECT_EQ(*second_remote_cert0->fingerprint, + initial_remote_certinfo->fingerprints[0]); + EXPECT_EQ(*second_remote_cert1->fingerprint, + initial_remote_certinfo->fingerprints[1]); + // But timestamps are up-to-date, because this is a fresh stats report. + EXPECT_EQ(second_local_cert0->timestamp_us(), rtc::TimeMicros()); + EXPECT_EQ(second_local_cert1->timestamp_us(), rtc::TimeMicros()); + EXPECT_EQ(second_remote_cert0->timestamp_us(), rtc::TimeMicros()); + EXPECT_EQ(second_remote_cert1->timestamp_us(), rtc::TimeMicros()); + // The updated certificates are not part of the report yet. + EXPECT_FALSE(GetCertificateStatsFromFingerprint( + second_report, updated_local_certinfo->fingerprints[0])); + EXPECT_FALSE(GetCertificateStatsFromFingerprint( + second_report, updated_local_certinfo->fingerprints[1])); + EXPECT_FALSE(GetCertificateStatsFromFingerprint( + second_report, updated_remote_certinfo->fingerprints[0])); + EXPECT_FALSE(GetCertificateStatsFromFingerprint( + second_report, updated_remote_certinfo->fingerprints[1])); + + // Clear the cache, including the cached certificates. + stats_->stats_collector()->ClearCachedStatsReport(); + rtc::scoped_refptr third_report = + stats_->GetStatsReport(); + // Now the old certificates stats should be deleted. + EXPECT_FALSE(third_report->Get(first_local_cert0->id())); + EXPECT_FALSE(third_report->Get(first_local_cert1->id())); + EXPECT_FALSE(third_report->Get(first_remote_cert0->id())); + EXPECT_FALSE(third_report->Get(first_remote_cert1->id())); + // And updated certificates exist. + EXPECT_TRUE(GetCertificateStatsFromFingerprint( + third_report, updated_local_certinfo->fingerprints[0])); + EXPECT_TRUE(GetCertificateStatsFromFingerprint( + third_report, updated_local_certinfo->fingerprints[1])); + EXPECT_TRUE(GetCertificateStatsFromFingerprint( + third_report, updated_remote_certinfo->fingerprints[0])); + EXPECT_TRUE(GetCertificateStatsFromFingerprint( + third_report, updated_remote_certinfo->fingerprints[1])); +} + TEST_F(RTCStatsCollectorTest, CollectTwoRTCDataChannelStatsWithPendingId) { pc_->AddSctpDataChannel(rtc::make_ref_counted( /*id=*/-1, DataChannelInterface::kConnecting)); diff --git a/rtc_base/ssl_certificate.cc b/rtc_base/ssl_certificate.cc index ddb1524f76..d1fd57fca5 100644 --- a/rtc_base/ssl_certificate.cc +++ b/rtc_base/ssl_certificate.cc @@ -44,6 +44,12 @@ SSLCertificateStats::SSLCertificateStats( SSLCertificateStats::~SSLCertificateStats() {} +std::unique_ptr SSLCertificateStats::Copy() const { + return std::make_unique( + std::string(fingerprint), std::string(fingerprint_algorithm), + std::string(base64_certificate), issuer ? issuer->Copy() : nullptr); +} + ////////////////////////////////////////////////////////////////////// // SSLCertificate ////////////////////////////////////////////////////////////////////// diff --git a/rtc_base/ssl_certificate.h b/rtc_base/ssl_certificate.h index 77fbba3e9e..2e198800c4 100644 --- a/rtc_base/ssl_certificate.h +++ b/rtc_base/ssl_certificate.h @@ -38,6 +38,8 @@ struct RTC_EXPORT SSLCertificateStats { std::string fingerprint_algorithm; std::string base64_certificate; std::unique_ptr issuer; + + std::unique_ptr Copy() const; }; // Abstract interface overridden by SSL library specific