Revert "Modernize rtc::SSLCertificate"
This reverts commit 55cd3ac804811e02b9b14026c683f9b30ea0c0bb.
Reason for revert: Breaks Chrome compile: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8932588150164377824/+/steps/compile__with_patch_/0/stdout
Original change's description:
> Modernize rtc::SSLCertificate
>
> Bug: webrtc:9860
> Change-Id: Idfce546ded500d957397c5bd873200565d3e6b64
> Reviewed-on: https://webrtc-review.googlesource.com/c/105280
> Reviewed-by: Benjamin Wright <benwright@webrtc.org>
> Reviewed-by: Qingsi Wang <qingsi@webrtc.org>
> Commit-Queue: Steve Anton <steveanton@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#25150}
TBR=steveanton@webrtc.org,qingsi@webrtc.org,benwright@webrtc.org
# Not skipping CQ checks because original CL landed > 1 day ago.
Bug: webrtc:9860
Change-Id: I4ff090f2612252cd656a34a0181aff81488c6edf
Reviewed-on: https://webrtc-review.googlesource.com/c/105946
Reviewed-by: Niklas Enbom <niklas.enbom@webrtc.org>
Commit-Queue: Niklas Enbom <niklas.enbom@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25182}diff --git a/p2p/base/fakedtlstransport.h b/p2p/base/fakedtlstransport.h
index 44a12a5..c4f9d2c 100644
--- a/p2p/base/fakedtlstransport.h
+++ b/p2p/base/fakedtlstransport.h
@@ -179,10 +179,8 @@
return local_cert_;
}
std::unique_ptr<rtc::SSLCertChain> GetRemoteSSLCertChain() const override {
- if (!remote_cert_) {
- return nullptr;
- }
- return absl::make_unique<rtc::SSLCertChain>(remote_cert_->Clone());
+ return remote_cert_ ? absl::make_unique<rtc::SSLCertChain>(remote_cert_)
+ : nullptr;
}
bool ExportKeyingMaterial(const std::string& label,
const uint8_t* context,
diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index cce7528..0861246 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -3144,7 +3144,7 @@
if (!chain || !chain->GetSize()) {
return nullptr;
}
- return chain->Get(0).Clone();
+ return chain->Get(0).GetUniqueReference();
}
std::unique_ptr<rtc::SSLCertChain>
diff --git a/pc/rtcstatscollector_unittest.cc b/pc/rtcstatscollector_unittest.cc
index e565e46..7404d49 100644
--- a/pc/rtcstatscollector_unittest.cc
+++ b/pc/rtcstatscollector_unittest.cc
@@ -703,7 +703,8 @@
CreateFakeCertificateAndInfoFromDers(
std::vector<std::string>({"(remote) single certificate"}));
pc_->SetRemoteCertChain(
- kTransportName, remote_certinfo->certificate->ssl_cert_chain().Clone());
+ kTransportName,
+ remote_certinfo->certificate->ssl_cert_chain().UniqueCopy());
rtc::scoped_refptr<const RTCStatsReport> report = stats_->GetStatsReport();
@@ -817,7 +818,7 @@
std::vector<std::string>({"(remote) audio"}));
pc_->SetRemoteCertChain(
kAudioTransport,
- audio_remote_certinfo->certificate->ssl_cert_chain().Clone());
+ audio_remote_certinfo->certificate->ssl_cert_chain().UniqueCopy());
pc_->AddVideoChannel("video", kVideoTransport);
std::unique_ptr<CertificateInfo> video_local_certinfo =
@@ -829,7 +830,7 @@
std::vector<std::string>({"(remote) video"}));
pc_->SetRemoteCertChain(
kVideoTransport,
- video_remote_certinfo->certificate->ssl_cert_chain().Clone());
+ video_remote_certinfo->certificate->ssl_cert_chain().UniqueCopy());
rtc::scoped_refptr<const RTCStatsReport> report = stats_->GetStatsReport();
ExpectReportContainsCertificateInfo(report, *audio_local_certinfo);
@@ -853,7 +854,8 @@
"(remote) another",
"(remote) chain"});
pc_->SetRemoteCertChain(
- kTransportName, remote_certinfo->certificate->ssl_cert_chain().Clone());
+ kTransportName,
+ remote_certinfo->certificate->ssl_cert_chain().UniqueCopy());
rtc::scoped_refptr<const RTCStatsReport> report = stats_->GetStatsReport();
ExpectReportContainsCertificateInfo(report, *local_certinfo);
@@ -1954,7 +1956,8 @@
CreateFakeCertificateAndInfoFromDers(
{"(remote) local", "(remote) chain"});
pc_->SetRemoteCertChain(
- kTransportName, remote_certinfo->certificate->ssl_cert_chain().Clone());
+ kTransportName,
+ remote_certinfo->certificate->ssl_cert_chain().UniqueCopy());
report = stats_->GetFreshStatsReport();
diff --git a/pc/statscollector_unittest.cc b/pc/statscollector_unittest.cc
index cbd7cc3..7c61b38 100644
--- a/pc/statscollector_unittest.cc
+++ b/pc/statscollector_unittest.cc
@@ -642,7 +642,7 @@
std::unique_ptr<rtc::SSLIdentity>(local_identity.GetReference())));
pc->SetLocalCertificate(kTransportName, local_certificate);
pc->SetRemoteCertChain(kTransportName,
- remote_identity.cert_chain().Clone());
+ remote_identity.cert_chain().UniqueCopy());
stats->UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard);
diff --git a/pc/test/fakepeerconnectionforstats.h b/pc/test/fakepeerconnectionforstats.h
index af86639..ae329e4 100644
--- a/pc/test/fakepeerconnectionforstats.h
+++ b/pc/test/fakepeerconnectionforstats.h
@@ -319,7 +319,7 @@
const std::string& transport_name) override {
auto it = remote_cert_chains_by_transport_.find(transport_name);
if (it != remote_cert_chains_by_transport_.end()) {
- return it->second->Clone();
+ return it->second->UniqueCopy();
} else {
return nullptr;
}
diff --git a/rtc_base/fakesslidentity.cc b/rtc_base/fakesslidentity.cc
index 62ac9dd..80a3e78 100644
--- a/rtc_base/fakesslidentity.cc
+++ b/rtc_base/fakesslidentity.cc
@@ -29,8 +29,8 @@
FakeSSLCertificate::~FakeSSLCertificate() = default;
-std::unique_ptr<SSLCertificate> FakeSSLCertificate::Clone() const {
- return absl::make_unique<FakeSSLCertificate>(*this);
+FakeSSLCertificate* FakeSSLCertificate::GetReference() const {
+ return new FakeSSLCertificate(*this);
}
std::string FakeSSLCertificate::ToPEMString() const {
@@ -83,10 +83,10 @@
}
FakeSSLIdentity::FakeSSLIdentity(const FakeSSLCertificate& cert)
- : cert_chain_(absl::make_unique<SSLCertChain>(cert.Clone())) {}
+ : cert_chain_(absl::make_unique<SSLCertChain>(&cert)) {}
FakeSSLIdentity::FakeSSLIdentity(const FakeSSLIdentity& o)
- : cert_chain_(o.cert_chain_->Clone()) {}
+ : cert_chain_(o.cert_chain_->UniqueCopy()) {}
FakeSSLIdentity::~FakeSSLIdentity() = default;
diff --git a/rtc_base/fakesslidentity.h b/rtc_base/fakesslidentity.h
index 9d5770c..4494a52 100644
--- a/rtc_base/fakesslidentity.h
+++ b/rtc_base/fakesslidentity.h
@@ -28,7 +28,7 @@
~FakeSSLCertificate() override;
// SSLCertificate implementation.
- std::unique_ptr<SSLCertificate> Clone() const override;
+ FakeSSLCertificate* GetReference() const override;
std::string ToPEMString() const override;
void ToDER(Buffer* der_buffer) const override;
int64_t CertificateExpirationTime() const override;
diff --git a/rtc_base/opensslcertificate.cc b/rtc_base/opensslcertificate.cc
index 92443a4..ed67a89 100644
--- a/rtc_base/opensslcertificate.cc
+++ b/rtc_base/opensslcertificate.cc
@@ -130,11 +130,10 @@
#endif
OpenSSLCertificate::OpenSSLCertificate(X509* x509) : x509_(x509) {
- RTC_DCHECK(x509_ != nullptr);
- X509_up_ref(x509_);
+ AddReference();
}
-std::unique_ptr<OpenSSLCertificate> OpenSSLCertificate::Generate(
+OpenSSLCertificate* OpenSSLCertificate::Generate(
OpenSSLKeyPair* key_pair,
const SSLIdentityParams& params) {
SSLIdentityParams actual_params(params);
@@ -150,12 +149,12 @@
#if !defined(NDEBUG)
PrintCert(x509);
#endif
- auto ret = absl::make_unique<OpenSSLCertificate>(x509);
+ OpenSSLCertificate* ret = new OpenSSLCertificate(x509);
X509_free(x509);
return ret;
}
-std::unique_ptr<OpenSSLCertificate> OpenSSLCertificate::FromPEMString(
+OpenSSLCertificate* OpenSSLCertificate::FromPEMString(
const std::string& pem_string) {
BIO* bio = BIO_new_mem_buf(const_cast<char*>(pem_string.c_str()), -1);
if (!bio)
@@ -168,7 +167,7 @@
if (!x509)
return nullptr;
- auto ret = absl::make_unique<OpenSSLCertificate>(x509);
+ OpenSSLCertificate* ret = new OpenSSLCertificate(x509);
X509_free(x509);
return ret;
}
@@ -250,8 +249,8 @@
X509_free(x509_);
}
-std::unique_ptr<SSLCertificate> OpenSSLCertificate::Clone() const {
- return absl::make_unique<OpenSSLCertificate>(x509_);
+OpenSSLCertificate* OpenSSLCertificate::GetReference() const {
+ return new OpenSSLCertificate(x509_);
}
std::string OpenSSLCertificate::ToPEMString() const {
@@ -290,6 +289,11 @@
BIO_free(bio);
}
+void OpenSSLCertificate::AddReference() const {
+ RTC_DCHECK(x509_ != nullptr);
+ X509_up_ref(x509_);
+}
+
bool OpenSSLCertificate::operator==(const OpenSSLCertificate& other) const {
return X509_cmp(x509_, other.x509_) == 0;
}
diff --git a/rtc_base/opensslcertificate.h b/rtc_base/opensslcertificate.h
index 3b49f93..b7ecc3b 100644
--- a/rtc_base/opensslcertificate.h
+++ b/rtc_base/opensslcertificate.h
@@ -36,15 +36,13 @@
// OpenSSLCertificate share ownership.
explicit OpenSSLCertificate(X509* x509);
- static std::unique_ptr<OpenSSLCertificate> Generate(
- OpenSSLKeyPair* key_pair,
- const SSLIdentityParams& params);
- static std::unique_ptr<OpenSSLCertificate> FromPEMString(
- const std::string& pem_string);
+ static OpenSSLCertificate* Generate(OpenSSLKeyPair* key_pair,
+ const SSLIdentityParams& params);
+ static OpenSSLCertificate* FromPEMString(const std::string& pem_string);
~OpenSSLCertificate() override;
- std::unique_ptr<SSLCertificate> Clone() const override;
+ OpenSSLCertificate* GetReference() const override;
X509* x509() const { return x509_; }
@@ -71,6 +69,8 @@
int64_t CertificateExpirationTime() const override;
private:
+ void AddReference() const;
+
X509* x509_; // NOT OWNED
RTC_DISALLOW_COPY_AND_ASSIGN(OpenSSLCertificate);
};
diff --git a/rtc_base/opensslidentity.cc b/rtc_base/opensslidentity.cc
index a5bbd5d..a8c6919 100644
--- a/rtc_base/opensslidentity.cc
+++ b/rtc_base/opensslidentity.cc
@@ -316,7 +316,7 @@
OpenSSLIdentity* OpenSSLIdentity::GetReference() const {
return new OpenSSLIdentity(absl::WrapUnique(key_pair_->GetReference()),
- cert_chain_->Clone());
+ absl::WrapUnique(cert_chain_->Copy()));
}
bool OpenSSLIdentity::ConfigureIdentity(SSL_CTX* ctx) {
diff --git a/rtc_base/opensslstreamadapter.cc b/rtc_base/opensslstreamadapter.cc
index 38b4bb6..fd54a08 100644
--- a/rtc_base/opensslstreamadapter.cc
+++ b/rtc_base/opensslstreamadapter.cc
@@ -1091,7 +1091,7 @@
std::unique_ptr<SSLCertChain> OpenSSLStreamAdapter::GetPeerSSLCertChain()
const {
- return peer_cert_chain_ ? peer_cert_chain_->Clone() : nullptr;
+ return peer_cert_chain_ ? peer_cert_chain_->UniqueCopy() : nullptr;
}
int OpenSSLStreamAdapter::SSLVerifyCallback(X509_STORE_CTX* store, void* arg) {
diff --git a/rtc_base/sslcertificate.cc b/rtc_base/sslcertificate.cc
index 142543f..e40feec 100644
--- a/rtc_base/sslcertificate.cc
+++ b/rtc_base/sslcertificate.cc
@@ -30,7 +30,7 @@
std::string&& fingerprint,
std::string&& fingerprint_algorithm,
std::string&& base64_certificate,
- std::unique_ptr<SSLCertificateStats> issuer)
+ std::unique_ptr<SSLCertificateStats>&& issuer)
: fingerprint(std::move(fingerprint)),
fingerprint_algorithm(std::move(fingerprint_algorithm)),
base64_certificate(std::move(base64_certificate)),
@@ -70,30 +70,49 @@
std::move(der_base64), nullptr);
}
+std::unique_ptr<SSLCertificate> SSLCertificate::GetUniqueReference() const {
+ return absl::WrapUnique(GetReference());
+}
+
//////////////////////////////////////////////////////////////////////
// SSLCertChain
//////////////////////////////////////////////////////////////////////
-SSLCertChain::SSLCertChain(std::unique_ptr<SSLCertificate> single_cert) {
- certs_.push_back(std::move(single_cert));
-}
-
SSLCertChain::SSLCertChain(std::vector<std::unique_ptr<SSLCertificate>> certs)
: certs_(std::move(certs)) {}
+SSLCertChain::SSLCertChain(const std::vector<SSLCertificate*>& certs) {
+ RTC_DCHECK(!certs.empty());
+ certs_.resize(certs.size());
+ std::transform(
+ certs.begin(), certs.end(), certs_.begin(),
+ [](const SSLCertificate* cert) -> std::unique_ptr<SSLCertificate> {
+ return cert->GetUniqueReference();
+ });
+}
+
+SSLCertChain::SSLCertChain(const SSLCertificate* cert) {
+ certs_.push_back(cert->GetUniqueReference());
+}
+
SSLCertChain::SSLCertChain(SSLCertChain&& rhs) = default;
SSLCertChain& SSLCertChain::operator=(SSLCertChain&&) = default;
-SSLCertChain::~SSLCertChain() = default;
+SSLCertChain::~SSLCertChain() {}
-std::unique_ptr<SSLCertChain> SSLCertChain::Clone() const {
+SSLCertChain* SSLCertChain::Copy() const {
std::vector<std::unique_ptr<SSLCertificate>> new_certs(certs_.size());
- std::transform(
- certs_.begin(), certs_.end(), new_certs.begin(),
- [](const std::unique_ptr<SSLCertificate>& cert)
- -> std::unique_ptr<SSLCertificate> { return cert->Clone(); });
- return absl::make_unique<SSLCertChain>(std::move(new_certs));
+ std::transform(certs_.begin(), certs_.end(), new_certs.begin(),
+ [](const std::unique_ptr<SSLCertificate>& cert)
+ -> std::unique_ptr<SSLCertificate> {
+ return cert->GetUniqueReference();
+ });
+ return new SSLCertChain(std::move(new_certs));
+}
+
+std::unique_ptr<SSLCertChain> SSLCertChain::UniqueCopy() const {
+ return absl::WrapUnique(Copy());
}
std::unique_ptr<SSLCertificateStats> SSLCertChain::GetStats() const {
@@ -115,8 +134,7 @@
}
// static
-std::unique_ptr<SSLCertificate> SSLCertificate::FromPEMString(
- const std::string& pem_string) {
+SSLCertificate* SSLCertificate::FromPEMString(const std::string& pem_string) {
return OpenSSLCertificate::FromPEMString(pem_string);
}
diff --git a/rtc_base/sslcertificate.h b/rtc_base/sslcertificate.h
index c04852f..029404c 100644
--- a/rtc_base/sslcertificate.h
+++ b/rtc_base/sslcertificate.h
@@ -28,7 +28,7 @@
SSLCertificateStats(std::string&& fingerprint,
std::string&& fingerprint_algorithm,
std::string&& base64_certificate,
- std::unique_ptr<SSLCertificateStats> issuer);
+ std::unique_ptr<SSLCertificateStats>&& issuer);
~SSLCertificateStats();
std::string fingerprint;
std::string fingerprint_algorithm;
@@ -51,13 +51,17 @@
// The length of the string representation of the certificate is
// stored in *pem_length if it is non-null, and only if
// parsing was successful.
- static std::unique_ptr<SSLCertificate> FromPEMString(
- const std::string& pem_string);
- virtual ~SSLCertificate() = default;
+ // Caller is responsible for freeing the returned object.
+ static SSLCertificate* FromPEMString(const std::string& pem_string);
+ virtual ~SSLCertificate() {}
// Returns a new SSLCertificate object instance wrapping the same
- // underlying certificate, including its chain if present.
- virtual std::unique_ptr<SSLCertificate> Clone() const = 0;
+ // underlying certificate, including its chain if present. Caller is
+ // responsible for freeing the returned object. Use GetUniqueReference
+ // instead.
+ virtual SSLCertificate* GetReference() const = 0;
+
+ std::unique_ptr<SSLCertificate> GetUniqueReference() const;
// Returns a PEM encoded string representation of the certificate.
virtual std::string ToPEMString() const = 0;
@@ -90,8 +94,11 @@
// SSLCertificate pointers.
class SSLCertChain {
public:
- explicit SSLCertChain(std::unique_ptr<SSLCertificate> single_cert);
explicit SSLCertChain(std::vector<std::unique_ptr<SSLCertificate>> certs);
+ // These constructors copy the provided SSLCertificate(s), so the caller
+ // retains ownership.
+ explicit SSLCertChain(const std::vector<SSLCertificate*>& certs);
+ explicit SSLCertChain(const SSLCertificate* cert);
// Allow move semantics for the object.
SSLCertChain(SSLCertChain&&);
SSLCertChain& operator=(SSLCertChain&&);
@@ -105,8 +112,10 @@
const SSLCertificate& Get(size_t pos) const { return *(certs_[pos]); }
// Returns a new SSLCertChain object instance wrapping the same underlying
- // certificate chain.
- std::unique_ptr<SSLCertChain> Clone() const;
+ // certificate chain. Caller is responsible for freeing the returned object.
+ SSLCertChain* Copy() const;
+ // Same as above, but returning a unique_ptr for convenience.
+ std::unique_ptr<SSLCertChain> UniqueCopy() const;
// Gets information (fingerprint, etc.) about this certificate chain. This is
// used for certificate stats, see
diff --git a/rtc_base/sslidentity_unittest.cc b/rtc_base/sslidentity_unittest.cc
index 9560aae..68b5828 100644
--- a/rtc_base/sslidentity_unittest.cc
+++ b/rtc_base/sslidentity_unittest.cc
@@ -201,7 +201,7 @@
ASSERT_TRUE(identity_ecdsa1_);
ASSERT_TRUE(identity_ecdsa2_);
- test_cert_ = rtc::SSLCertificate::FromPEMString(kTestCertificate);
+ test_cert_.reset(rtc::SSLCertificate::FromPEMString(kTestCertificate));
ASSERT_TRUE(test_cert_);
}
diff --git a/rtc_base/sslstreamadapter_unittest.cc b/rtc_base/sslstreamadapter_unittest.cc
index ff4c7a0..389b0ea 100644
--- a/rtc_base/sslstreamadapter_unittest.cc
+++ b/rtc_base/sslstreamadapter_unittest.cc
@@ -588,7 +588,8 @@
chain = client_ssl_->GetPeerSSLCertChain();
else
chain = server_ssl_->GetPeerSSLCertChain();
- return (chain && chain->GetSize()) ? chain->Get(0).Clone() : nullptr;
+ return (chain && chain->GetSize()) ? chain->Get(0).GetUniqueReference()
+ : nullptr;
}
bool GetSslCipherSuite(bool client, int* retval) {
