From 6c6c9df99db678abc7b504e29cb64088dd0d5a19 Mon Sep 17 00:00:00 2001 From: Benjamin Wright Date: Thu, 25 Oct 2018 01:16:26 -0700 Subject: [PATCH] Refactor: Renaming ssl_cert_chain to GetSSLCertificateChain() Underscore methods in the middle of classes is against the chromium style guide this change is part of a long series of changes to refactor crypto code in WebRTC to conform to the chromium standard better. 1. ssl_cert() -> GetSSLCertificate() 2. ssl_cert_chain() -> GetSSLCertificateChain() 3. Small tidying up in rtccertificategenerator.cc Bug: webrtc:9860 Change-Id: I670f76e31d6d4f873034edb72d958b3c227379cb Reviewed-on: https://webrtc-review.googlesource.com/c/107802 Reviewed-by: Karl Wiberg Commit-Queue: Benjamin Wright Cr-Commit-Position: refs/heads/master@{#25371} --- p2p/base/dtlstransport_unittest.cc | 12 ++++++------ p2p/base/transportdescriptionfactory_unittest.cc | 6 +++--- pc/jseptransport_unittest.cc | 2 +- pc/peerconnection_integrationtest.cc | 8 ++++---- pc/rtcstatscollector.cc | 2 +- pc/rtcstatscollector_unittest.cc | 15 +++++++++------ pc/statscollector.cc | 4 ++-- rtc_base/rtccertificate.cc | 11 ++++++++--- rtc_base/rtccertificate.h | 10 +++++++--- rtc_base/rtccertificategenerator.cc | 14 ++++++-------- rtc_base/sslfingerprint.cc | 2 +- 11 files changed, 48 insertions(+), 38 deletions(-) diff --git a/p2p/base/dtlstransport_unittest.cc b/p2p/base/dtlstransport_unittest.cc index a9000c38fe..e60a3a5b42 100644 --- a/p2p/base/dtlstransport_unittest.cc +++ b/p2p/base/dtlstransport_unittest.cc @@ -513,8 +513,8 @@ TEST_F(DtlsTransportTest, TestCertificatesBeforeConnect) { // remote certificate, because connection has not yet occurred. auto certificate1 = client1_.dtls_transport()->GetLocalCertificate(); auto certificate2 = client2_.dtls_transport()->GetLocalCertificate(); - ASSERT_NE(certificate1->ssl_certificate().ToPEMString(), - certificate2->ssl_certificate().ToPEMString()); + ASSERT_NE(certificate1->GetSSLCertificate().ToPEMString(), + certificate2->GetSSLCertificate().ToPEMString()); ASSERT_FALSE(client1_.dtls_transport()->GetRemoteSSLCertChain()); ASSERT_FALSE(client2_.dtls_transport()->GetRemoteSSLCertChain()); } @@ -527,8 +527,8 @@ TEST_F(DtlsTransportTest, TestCertificatesAfterConnect) { // After connection, each side has a distinct local certificate. auto certificate1 = client1_.dtls_transport()->GetLocalCertificate(); auto certificate2 = client2_.dtls_transport()->GetLocalCertificate(); - ASSERT_NE(certificate1->ssl_certificate().ToPEMString(), - certificate2->ssl_certificate().ToPEMString()); + ASSERT_NE(certificate1->GetSSLCertificate().ToPEMString(), + certificate2->GetSSLCertificate().ToPEMString()); // Each side's remote certificate is the other side's local certificate. std::unique_ptr remote_cert1 = @@ -536,13 +536,13 @@ TEST_F(DtlsTransportTest, TestCertificatesAfterConnect) { ASSERT_TRUE(remote_cert1); ASSERT_EQ(1u, remote_cert1->GetSize()); ASSERT_EQ(remote_cert1->Get(0).ToPEMString(), - certificate2->ssl_certificate().ToPEMString()); + certificate2->GetSSLCertificate().ToPEMString()); std::unique_ptr remote_cert2 = client2_.dtls_transport()->GetRemoteSSLCertChain(); ASSERT_TRUE(remote_cert2); ASSERT_EQ(1u, remote_cert2->GetSize()); ASSERT_EQ(remote_cert2->Get(0).ToPEMString(), - certificate1->ssl_certificate().ToPEMString()); + certificate1->GetSSLCertificate().ToPEMString()); } // Test that packets are retransmitted according to the expected schedule. diff --git a/p2p/base/transportdescriptionfactory_unittest.cc b/p2p/base/transportdescriptionfactory_unittest.cc index a3cdb805ee..c46630ace2 100644 --- a/p2p/base/transportdescriptionfactory_unittest.cc +++ b/p2p/base/transportdescriptionfactory_unittest.cc @@ -167,7 +167,7 @@ TEST_F(TransportDescriptionFactoryTest, TestOfferDtls) { f1_.set_certificate(cert1_); std::string digest_alg; ASSERT_TRUE( - cert1_->ssl_certificate().GetSignatureDigestAlgorithm(&digest_alg)); + cert1_->GetSSLCertificate().GetSignatureDigestAlgorithm(&digest_alg)); std::unique_ptr desc( f1_.CreateOffer(TransportOptions(), NULL, &ice_credentials_)); CheckDesc(desc.get(), "", "", "", digest_alg); @@ -192,7 +192,7 @@ TEST_F(TransportDescriptionFactoryTest, TestOfferDtlsReofferDtls) { f1_.set_certificate(cert1_); std::string digest_alg; ASSERT_TRUE( - cert1_->ssl_certificate().GetSignatureDigestAlgorithm(&digest_alg)); + cert1_->GetSSLCertificate().GetSignatureDigestAlgorithm(&digest_alg)); std::unique_ptr old_desc( f1_.CreateOffer(TransportOptions(), NULL, &ice_credentials_)); ASSERT_TRUE(old_desc.get() != NULL); @@ -269,7 +269,7 @@ TEST_F(TransportDescriptionFactoryTest, TestAnswerDtlsToDtls) { // answer must contain fingerprint lines with cert2_'s digest algorithm. std::string digest_alg2; ASSERT_TRUE( - cert2_->ssl_certificate().GetSignatureDigestAlgorithm(&digest_alg2)); + cert2_->GetSSLCertificate().GetSignatureDigestAlgorithm(&digest_alg2)); std::unique_ptr offer( f1_.CreateOffer(TransportOptions(), NULL, &ice_credentials_)); diff --git a/pc/jseptransport_unittest.cc b/pc/jseptransport_unittest.cc index 748a4e078f..b8265de6bb 100644 --- a/pc/jseptransport_unittest.cc +++ b/pc/jseptransport_unittest.cc @@ -383,7 +383,7 @@ TEST_P(JsepTransport2WithRtcpMux, VerifyCertificateFingerprint) { ASSERT_NE(nullptr, certificate); std::string digest_algorithm; - ASSERT_TRUE(certificate->ssl_certificate().GetSignatureDigestAlgorithm( + ASSERT_TRUE(certificate->GetSSLCertificate().GetSignatureDigestAlgorithm( &digest_algorithm)); ASSERT_FALSE(digest_algorithm.empty()); std::unique_ptr good_fingerprint = diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc index b645ab1ad2..236a8bb97d 100644 --- a/pc/peerconnection_integrationtest.cc +++ b/pc/peerconnection_integrationtest.cc @@ -1821,26 +1821,26 @@ TEST_P(PeerConnectionIntegrationTest, auto caller_remote_cert = GetRemoteAudioSSLCertificate(caller()); ASSERT_TRUE(caller_remote_cert); - EXPECT_EQ(callee_cert->ssl_certificate().ToPEMString(), + EXPECT_EQ(callee_cert->GetSSLCertificate().ToPEMString(), caller_remote_cert->ToPEMString()); auto callee_remote_cert = GetRemoteAudioSSLCertificate(callee()); ASSERT_TRUE(callee_remote_cert); - EXPECT_EQ(caller_cert->ssl_certificate().ToPEMString(), + EXPECT_EQ(caller_cert->GetSSLCertificate().ToPEMString(), callee_remote_cert->ToPEMString()); auto caller_remote_cert_chain = GetRemoteAudioSSLCertChain(caller()); ASSERT_TRUE(caller_remote_cert_chain); ASSERT_EQ(1U, caller_remote_cert_chain->GetSize()); auto remote_cert = &caller_remote_cert_chain->Get(0); - EXPECT_EQ(callee_cert->ssl_certificate().ToPEMString(), + EXPECT_EQ(callee_cert->GetSSLCertificate().ToPEMString(), remote_cert->ToPEMString()); auto callee_remote_cert_chain = GetRemoteAudioSSLCertChain(callee()); ASSERT_TRUE(callee_remote_cert_chain); ASSERT_EQ(1U, callee_remote_cert_chain->GetSize()); remote_cert = &callee_remote_cert_chain->Get(0); - EXPECT_EQ(caller_cert->ssl_certificate().ToPEMString(), + EXPECT_EQ(caller_cert->GetSSLCertificate().ToPEMString(), remote_cert->ToPEMString()); } diff --git a/pc/rtcstatscollector.cc b/pc/rtcstatscollector.cc index d8839430eb..32e0b65557 100644 --- a/pc/rtcstatscollector.cc +++ b/pc/rtcstatscollector.cc @@ -1388,7 +1388,7 @@ RTCStatsCollector::PrepareTransportCertificateStats_n( rtc::scoped_refptr local_certificate; if (pc_->GetLocalCertificate(transport_name, &local_certificate)) { certificate_stats_pair.local = - local_certificate->ssl_cert_chain().GetStats(); + local_certificate->GetSSLCertificateChain().GetStats(); } std::unique_ptr remote_cert_chain = diff --git a/pc/rtcstatscollector_unittest.cc b/pc/rtcstatscollector_unittest.cc index 1a03e9899b..3fc0127b48 100644 --- a/pc/rtcstatscollector_unittest.cc +++ b/pc/rtcstatscollector_unittest.cc @@ -143,7 +143,7 @@ std::unique_ptr CreateFakeCertificateAndInfoFromDers( } // Fingerprints for the whole certificate chain, starting with leaf // certificate. - const rtc::SSLCertChain& chain = info->certificate->ssl_cert_chain(); + const rtc::SSLCertChain& chain = info->certificate->GetSSLCertificateChain(); std::unique_ptr fp; for (size_t i = 0; i < chain.GetSize(); i++) { fp = rtc::SSLFingerprint::Create("sha-1", chain.Get(i)); @@ -703,7 +703,8 @@ TEST_F(RTCStatsCollectorTest, CollectRTCCertificateStatsSingle) { CreateFakeCertificateAndInfoFromDers( std::vector({"(remote) single certificate"})); pc_->SetRemoteCertChain( - kTransportName, remote_certinfo->certificate->ssl_cert_chain().Clone()); + kTransportName, + remote_certinfo->certificate->GetSSLCertificateChain().Clone()); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -817,7 +818,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCCertificateStatsMultiple) { std::vector({"(remote) audio"})); pc_->SetRemoteCertChain( kAudioTransport, - audio_remote_certinfo->certificate->ssl_cert_chain().Clone()); + audio_remote_certinfo->certificate->GetSSLCertificateChain().Clone()); pc_->AddVideoChannel("video", kVideoTransport); std::unique_ptr video_local_certinfo = @@ -829,7 +830,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCCertificateStatsMultiple) { std::vector({"(remote) video"})); pc_->SetRemoteCertChain( kVideoTransport, - video_remote_certinfo->certificate->ssl_cert_chain().Clone()); + video_remote_certinfo->certificate->GetSSLCertificateChain().Clone()); rtc::scoped_refptr report = stats_->GetStatsReport(); ExpectReportContainsCertificateInfo(report, *audio_local_certinfo); @@ -853,7 +854,8 @@ TEST_F(RTCStatsCollectorTest, CollectRTCCertificateStatsChain) { "(remote) another", "(remote) chain"}); pc_->SetRemoteCertChain( - kTransportName, remote_certinfo->certificate->ssl_cert_chain().Clone()); + kTransportName, + remote_certinfo->certificate->GetSSLCertificateChain().Clone()); rtc::scoped_refptr report = stats_->GetStatsReport(); ExpectReportContainsCertificateInfo(report, *local_certinfo); @@ -1954,7 +1956,8 @@ TEST_F(RTCStatsCollectorTest, CollectRTCTransportStats) { CreateFakeCertificateAndInfoFromDers( {"(remote) local", "(remote) chain"}); pc_->SetRemoteCertChain( - kTransportName, remote_certinfo->certificate->ssl_cert_chain().Clone()); + kTransportName, + remote_certinfo->certificate->GetSSLCertificateChain().Clone()); report = stats_->GetFreshStatsReport(); diff --git a/pc/statscollector.cc b/pc/statscollector.cc index 23a449308c..a1d52db04e 100644 --- a/pc/statscollector.cc +++ b/pc/statscollector.cc @@ -783,8 +783,8 @@ void StatsCollector::ExtractSessionInfo() { StatsReport::Id local_cert_report_id, remote_cert_report_id; rtc::scoped_refptr certificate; if (pc_->GetLocalCertificate(transport_name, &certificate)) { - StatsReport* r = - AddCertificateReports(certificate->ssl_cert_chain().GetStats()); + StatsReport* r = AddCertificateReports( + certificate->GetSSLCertificateChain().GetStats()); if (r) local_cert_report_id = r->id(); } diff --git a/rtc_base/rtccertificate.cc b/rtc_base/rtccertificate.cc index 17c157535f..875068f4fc 100644 --- a/rtc_base/rtccertificate.cc +++ b/rtc_base/rtccertificate.cc @@ -32,7 +32,7 @@ RTCCertificate::RTCCertificate(SSLIdentity* identity) : identity_(identity) { RTCCertificate::~RTCCertificate() {} uint64_t RTCCertificate::Expires() const { - int64_t expires = ssl_certificate().CertificateExpirationTime(); + int64_t expires = GetSSLCertificate().CertificateExpirationTime(); if (expires != -1) return static_cast(expires) * kNumMillisecsPerSec; // If the expiration time could not be retrieved return an expired timestamp. @@ -43,17 +43,22 @@ bool RTCCertificate::HasExpired(uint64_t now) const { return Expires() <= now; } +const SSLCertificate& RTCCertificate::GetSSLCertificate() const { + return identity_->certificate(); +} + +// Deprecated: TODO(benwright) - Remove once chromium is updated. const SSLCertificate& RTCCertificate::ssl_certificate() const { return identity_->certificate(); } -const SSLCertChain& RTCCertificate::ssl_cert_chain() const { +const SSLCertChain& RTCCertificate::GetSSLCertificateChain() const { return identity_->cert_chain(); } RTCCertificatePEM RTCCertificate::ToPEM() const { return RTCCertificatePEM(identity_->PrivateKeyToPEMString(), - ssl_certificate().ToPEMString()); + GetSSLCertificate().ToPEMString()); } scoped_refptr RTCCertificate::FromPEM( diff --git a/rtc_base/rtccertificate.h b/rtc_base/rtccertificate.h index 0adefbce28..561ea0f9e7 100644 --- a/rtc_base/rtccertificate.h +++ b/rtc_base/rtccertificate.h @@ -58,11 +58,15 @@ class RTCCertificate : public RefCountInterface { // Checks if the certificate has expired, where |now| is expressed in ms // relative to epoch, 1970-01-01T00:00:00Z. bool HasExpired(uint64_t now) const; + + const SSLCertificate& GetSSLCertificate() const; + const SSLCertChain& GetSSLCertificateChain() const; + + // Deprecated: TODO(benwright) - Remove once chromium is updated. const SSLCertificate& ssl_certificate() const; - const SSLCertChain& ssl_cert_chain() const; // TODO(hbos): If possible, remove once RTCCertificate and its - // ssl_certificate() is used in all relevant places. Should not pass around + // GetSSLCertificate() is used in all relevant places. Should not pass around // raw SSLIdentity* for the sake of accessing SSLIdentity::certificate(). // However, some places might need SSLIdentity* for its public/private key... SSLIdentity* identity() const { return identity_.get(); } @@ -80,7 +84,7 @@ class RTCCertificate : public RefCountInterface { private: // The SSLIdentity is the owner of the SSLCertificate. To protect our - // ssl_certificate() we take ownership of |identity_|. + // GetSSLCertificate() we take ownership of |identity_|. std::unique_ptr identity_; }; diff --git a/rtc_base/rtccertificategenerator.cc b/rtc_base/rtccertificategenerator.cc index 3867ceb53b..114b35c665 100644 --- a/rtc_base/rtccertificategenerator.cc +++ b/rtc_base/rtccertificategenerator.cc @@ -28,7 +28,6 @@ namespace { // A certificates' subject and issuer name. const char kIdentityName[] = "WebRTC"; - const uint64_t kYearInSeconds = 365 * 24 * 60 * 60; enum { @@ -65,11 +64,9 @@ class RTCCertificateGenerationTask : public RefCountInterface, switch (msg->message_id) { case MSG_GENERATE: RTC_DCHECK(worker_thread_->IsCurrent()); - // Perform the certificate generation work here on the worker thread. certificate_ = RTCCertificateGenerator::GenerateCertificate( key_params_, expires_ms_); - // Handle callbacks on signaling thread. Pass on the |msg->pdata| // (which references |this| with ref counting) to that thread. signaling_thread_->Post(RTC_FROM_HERE, this, MSG_GENERATE_DONE, @@ -77,14 +74,12 @@ class RTCCertificateGenerationTask : public RefCountInterface, break; case MSG_GENERATE_DONE: RTC_DCHECK(signaling_thread_->IsCurrent()); - // Perform callback with result here on the signaling thread. if (certificate_) { callback_->OnSuccess(certificate_); } else { callback_->OnFailure(); } - // Destroy |msg->pdata| which references |this| with ref counting. This // may result in |this| being deleted - do not touch member variables // after this line. @@ -110,9 +105,11 @@ class RTCCertificateGenerationTask : public RefCountInterface, scoped_refptr RTCCertificateGenerator::GenerateCertificate( const KeyParams& key_params, const absl::optional& expires_ms) { - if (!key_params.IsValid()) + if (!key_params.IsValid()) { return nullptr; - SSLIdentity* identity; + } + + SSLIdentity* identity = nullptr; if (!expires_ms) { identity = SSLIdentity::Generate(kIdentityName, key_params); } else { @@ -129,8 +126,9 @@ scoped_refptr RTCCertificateGenerator::GenerateCertificate( identity = SSLIdentity::GenerateWithExpiration(kIdentityName, key_params, cert_lifetime_s); } - if (!identity) + if (!identity) { return nullptr; + } std::unique_ptr identity_sptr(identity); return RTCCertificate::Create(std::move(identity_sptr)); } diff --git a/rtc_base/sslfingerprint.cc b/rtc_base/sslfingerprint.cc index db13cf625c..b296d33ddd 100644 --- a/rtc_base/sslfingerprint.cc +++ b/rtc_base/sslfingerprint.cc @@ -79,7 +79,7 @@ std::unique_ptr SSLFingerprint::CreateUniqueFromRfc4572( std::unique_ptr SSLFingerprint::CreateFromCertificate( const RTCCertificate& cert) { std::string digest_alg; - if (!cert.ssl_certificate().GetSignatureDigestAlgorithm(&digest_alg)) { + if (!cert.GetSSLCertificate().GetSignatureDigestAlgorithm(&digest_alg)) { RTC_LOG(LS_ERROR) << "Failed to retrieve the certificate's digest algorithm"; return nullptr;